C4/Acquisitions.pm contains a lot of calls to finish. These seem to be copied as magic by someone who is unaware of the usage (please read the DBI doc for full details) In brief finish is only required to be called after a select statement where you have not read back the full result set. It should never be called after a non-select statement. In fact there are a couple of places in C4::Acquisitions.pm where its not called where it should be. In practice a call to finish in top level code is almost always always a bad sign. Lets clean these up
Created attachment 20654 [details] [review] Proposed Patch Patch should not alter functionality.
Patch applied cleanly, go forth and signoff
Applying: Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions Using index info to reconstruct a base tree... M C4/Acquisition.pm Falling back to patching base and 3-way merge... Auto-merging C4/Acquisition.pm CONFLICT (content): Merge conflict in C4/Acquisition.pm Patch failed at 0001 Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 23349 [details] [review] Revised Patch Patch rebased against current master
Patch revised to accomodate changes in C4::Acquisitions
This patch touches: DelBasket ModBasket ModBasketHeader GetBasketsByBookseller ModBasketUsers GetBasketsByBasketgroup ModBasketgroup DelBasketgroup GetBasketgroup GetOrders GetOrdersByBiblionumber GetOrder GetLastOrderNotReceivedFromSubscriptionid GetLastOrderReceivedFromSubscriptionid ModOrder ModReceiveOrder DelOrder GetParcel GetParcels GetContracts There are no current tests which access DelBasket. I did not check others. As such, I would have to figure out how to trigger all of these functions. I was wondering if the existing test modules could be improved to call at least every function once.
Created attachment 25350 [details] [review] [Signed-Off] Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions C4::Acquisitions contained a number of unnecessary calls to finish. Removed these and the associated variables introduced to cache query results between fetch and the return Where finish was the end of the routine I have added an explicit return to document that no data is returned. A number of places made query calls and fetched a single row. Such a case could require an explicit finish. These assume that they are looking up with a unique key. To remove assumptions and isolate the code from future changes I've switched these to fetching all and returning the first row. I have commented these cases. For fuller explanation see perldoc DBI What I tested: Edit existing basket, chnged name Modify order line, change vendor price Create new basket and add order Delet this order Delte this basket New Basket, new order, user added, user removed Add contract to vendor, change details, delete contract Search order biblio Create basket group, add basket to group, remove basket from group Delete basket group Receive order Everything behaved as I expected Signed-off-by: Marc Véron <veron@veron.ch>
This patch breaks some UT: prove t/db_dependent/Acquisition/* Marked as Failed QA.
Created attachment 25371 [details] [review] Bug 10789: Display the currency for baskets in a basketgroup On editing a basketgroup, the currency for baskets in a basketgroup is always '0'. With this patch, the currency is correctly displayed. Test plan: Edit a basket group with baskets and verify the currenty is correct.
Comment on attachment 25371 [details] [review] Bug 10789: Display the currency for baskets in a basketgroup wrong bug report!
[Jonathan: We were looking at this simulaneously..] QA Comment 2: Code looks good to me (now). Fixed removed unused sql parameter too passing by. Few additional comments. Did some limited testing. Compliments for testing by Marc Veron, and test plan. This patch actually lacked a test plan. Small remark on ModReceiveOrder + # we assume we have a unique order + my $order = $result_set->[0]; The old code contained this assumption and you will most probably have one. But we could argue about testing it.. As in the meantime also seen by Jonathan, t/db_dependent/Acquisition/OrderFromSubscription.t complained: Illegal date specified (year = 2015, month = 31, day = 12) at /usr/share/koha/testclone/C4/SQLHelper.pm line 409. Use of uninitialized value $serialseq in string at /usr/share/koha/testclone/C4/Serials.pm line 1542. This is unrelated to this report. And: DBD::mysql::db selectall_arrayref failed: called with 1 bind variables when 2 are needed at /usr/share/koha/testclone/C4/Acquisition.pm line 1199. DBD::mysql::db selectall_arrayref failed: called with 1 bind variables when 2 are needed at /usr/share/koha/testclone/C4/Acquisition.pm line 1199. Fixed in the follow-up.
Created attachment 25378 [details] [review] Bug 10789: Follow-up: Fix typo infermation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 25379 [details] [review] Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 25380 [details] [review] Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions C4::Acquisitions contained a number of unnecessary calls to finish. Removed these and the associated variables introduced to cache query results between fetch and the return Where finish was the end of the routine I have added an explicit return to document that no data is returned. A number of places made query calls and fetched a single row. Such a case could require an explicit finish. These assume that they are looking up with a unique key. To remove assumptions and isolate the code from future changes I've switched these to fetching all and returning the first row. I have commented these cases. For fuller explanation see perldoc DBI What I tested: Edit existing basket, chnged name Modify order line, change vendor price Create new basket and add order Delet this order Delte this basket New Basket, new order, user added, user removed Add contract to vendor, change details, delete contract Search order biblio Create basket group, add basket to group, remove basket from group Delete basket group Receive order Everything behaved as I expected Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 25381 [details] [review] Bug 10789: Follow-up: Fix typo infermation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 25382 [details] [review] Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master. Thanks, Colin!
Pushed to 3.14.x, will be in 3.14.07. This may improve performance, that's why I've integrated it.