Description
Jonathan Druart
2014-08-27 10:42:21 UTC
Created attachment 31178 [details] [review] 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. 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);? Created attachment 31180 [details] [review] Bug 12826: use find instead of search Since ordernumber is the pk, it is stupid to use search. (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! Created attachment 31237 [details] [review] Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. 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. Paola, did you apply dependent patches? (In reply to Jonathan Druart from comment #7) > Paola, did you apply dependent patches? No, so I pass the patch back to "Needs Signoff"! 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. 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. Created attachment 31498 [details] [review] 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. Created attachment 31499 [details] [review] Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. 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. 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 (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). Created attachment 31521 [details] [review] 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. Created attachment 31522 [details] [review] Bug 12830: use find instead of search Since ordernumber is the pk, it is stupid to use search. Created attachment 31549 [details] [review] 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. Created attachment 31550 [details] [review] Bug 12830: use find instead of search Created attachment 31714 [details] [review] 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> Created attachment 31715 [details] [review] 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> Created attachment 31827 [details] [review] 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> Created attachment 31828 [details] [review] 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> Created attachment 32810 [details] [review] [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> Created attachment 32811 [details] [review] [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. Patches pushed to master. Keep the good work Jonathan! |