[Bug 12830] New: Move the order-related code into its own module
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Bug ID: 12830 Summary: Move the order-related code into its own module Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: jonathan.druart@biblibre.com QA Contact: testopia@bugs.koha-community.org The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. I propose to create a new Koha::Acquisition::Order module and to put in it the code related to the order management. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|gmcharlt@gmail.com |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |12827 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31178 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31178&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com --- Comment #2 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Good stuff breaking down C4::acquisition into more manageable parts is long overdue. When fetching the order ... ordernumber is the primary key so why not simply call $schema->resultset('Aqorder')->find($ordernumber);? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31180 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31180&action=edit Bug 12826: use find instead of search Since ordernumber is the pk, it is stupid to use search. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Colin Campbell from comment #2)
When fetching the order ... ordernumber is the primary key so why not simply call $schema->resultset('Aqorder')->find($ordernumber);?
There is no reason :) I added a followup for that. Thanks! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31180|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31237 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31237&action=edit Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12826 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12896 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |paola.rossi@cineca.it --- Comment #6 from Paola Rossi <paola.rossi@cineca.it> --- I've tried to apply the patch against master 3.17.00.023 Applying: Bug 12830: Move the order-related code into Koha::Acquisition::Order fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 12830: Move the order-related code into Koha::Acquisition::Order So I pass the patch to "Patch doesn't apply" status. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #7 from Jonathan Druart <jonathan.druart@biblibre.com> --- Paola, did you apply dependent patches? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #8 from Paola Rossi <paola.rossi@cineca.it> --- (In reply to Jonathan Druart from comment #7)
Paola, did you apply dependent patches?
No, so I pass the patch back to "Needs Signoff"! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Just a note, DBIC's find doesn't need a key/value pair when finding on a table with a single column primary key. You can just pass in the value. That being said, what you have will work just fine. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #10 from Paola Rossi <paola.rossi@cineca.it> --- I've tried to apply the patch against 3.17.00.023. Firstly I applied the 12827 patch. I unfortunately see the same conflict as yesterday: Applying: Bug 12830: Move the order-related code into Koha::Acquisition::Order fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 12830: Move the order-related code into Koha::Acquisition::Order So I pass the patch to "Patch doesn't apply" status. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31178|0 |1 is obsolete| | Attachment #31237|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31498 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31498&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31499 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31499&action=edit Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #13 from Paola Rossi <paola.rossi@cineca.it> --- I've applied the patches against master 3.17.00.023 I was trying to create an order. After selecting "Save", this error appeared: Software error: DBIx::Class::Relationship::CascadeActions::update(): Can't update Koha::Schema::Result::Aqorder=HASH(0x5c577e8): row not found at /var/root-koha/bug-XXXX/Koha/Acquisition/Order.pm line 50 [nonetheless the order has been created] So I pass the patch to "Failed QA" status. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #14 from Paola Rossi <paola.rossi@cineca.it> --- To complete the content of the Comment 13: 1) the software error DBIx.... occurs either testing on a DB without orders, or testing on a DB with former orders. 2) prove t/db_dependent/Acquisition/CancelReceipt.t t/db_dependent/Acquisition/CancelReceipt.t .. 1/4 Open of share file /tmp/sharefile-koha-koha failed: Permesso negato at /usr/lib/perl5/Cache/FastMmap.pm line 640. # Looks like you planned 4 tests but ran 1. # Looks like your test exited with 13 just after 1. t/db_dependent/Acquisition/CancelReceipt.t .. Dubious, test returned 13 (wstat 3328, 0xd00) Failed 3/4 subtests Test Summary Report ------------------- t/db_dependent/Acquisition/CancelReceipt.t (Wstat: 3328 Tests: 1 Failed: 0) Non-zero exit status: 13 Parse errors: Bad plan. You planned 4 tests but ran 1. Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.91 cusr 0.08 csys = 1.01 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Francois Charbonnier <francois.charbonnier@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |francois.charbonnier@inlibr | |o.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Paola Rossi from comment #13)
I've applied the patches against master 3.17.00.023
I was trying to create an order. After selecting "Save", this error appeared:
Software error:
DBIx::Class::Relationship::CascadeActions::update(): Can't update Koha::Schema::Result::Aqorder=HASH(0x5c577e8): row not found at /var/root-koha/bug-XXXX/Koha/Acquisition/Order.pm line 50
Hi Paola, Yes this bug is introduced by bug 12626 (pushed 1 day after I submitted these patches :-/). I proposed a patch on bug 12891, but it needs QA. I cannot continue to rebase the patch until it is not pushed. Will do as soon as it is pushed! (In reply to Paola Rossi from comment #14)
To complete the content of the Comment 13:
1) the software error DBIx.... occurs either testing on a DB without orders, or testing on a DB with former orders.
2) prove t/db_dependent/Acquisition/CancelReceipt.t t/db_dependent/Acquisition/CancelReceipt.t .. 1/4 Open of share file /tmp/sharefile-koha-koha failed: Permesso negato at /usr/lib/perl5/Cache/FastMmap.pm line 640.
It's not introduced by this patch, you should remove the file /tmp/sharefile-koha-koha or remove the fastmap package (libcache-fastmmap-perl). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |12891 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31498|0 |1 is obsolete| | Attachment #31499|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31521 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31521&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #17 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31522 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31522&action=edit Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|12891 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31521|0 |1 is obsolete| | --- Comment #18 from Paola Rossi <paola.rossi@cineca.it> --- Created attachment 31549 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31549&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order I've applied the patches against master 3.17.00.023 I've tested varying "Acqcreateitem" pref on all the 3 options. Everything's OK. So I pass the patches to "Signed Off" status. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31522|0 |1 is obsolete| | --- Comment #19 from Paola Rossi <paola.rossi@cineca.it> --- Created attachment 31550 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31550&action=edit Bug 12830: use find instead of search -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |12953 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31549|0 |1 is obsolete| | Attachment #31550|0 |1 is obsolete| | --- Comment #20 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31714 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31714&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #21 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31715 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31715&action=edit Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |12844 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31714|0 |1 is obsolete| | Attachment #31715|0 |1 is obsolete| | --- Comment #22 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31827 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31827&action=edit Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #23 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 31828 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31828&action=edit Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31827|0 |1 is obsolete| | Attachment #31828|0 |1 is obsolete| | --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 32810 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32810&action=edit [PASSED QA] Bug 12830: Move the order-related code into Koha::Acquisition::Order The C4::Acquisition module should be exploded in order to add readability and maintainability to this part of the code. This patch is a POC, it introduces a new Koha::Acquisition::Order module and put in it the code from NewOrder and NewOrderItem. Test plan: 1/ Create an order, modify it, receive it, cancel the receipt. 2/ Launch the prove command on all unit tests modified by this patch and verify that all pass. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|Small patch |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 --- Comment #25 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 32811 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32811&action=edit [PASSED QA] Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> All acquisition related tests and the QA script pass. I can't find any regressions with these patches. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Keep the good work Jonathan! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12830 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13319 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org