Bug 10789

Summary: Excessive and often incorrect use of finish in C4::Acquisitions
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: AcquisitionsAssignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: abl, fridolin.somers, gitbot, gmcharlt, jonathan.druart, m.de.rooy, mtompset, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11550
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed Patch
Revised Patch
[Signed-Off] Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions
Bug 10789: Display the currency for baskets in a basketgroup
Bug 10789: Follow-up: Fix typo infermation
Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid
Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions
Bug 10789: Follow-up: Fix typo infermation
Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid

Description Colin Campbell 2013-08-27 15:00:54 UTC
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
Comment 1 Colin Campbell 2013-08-27 15:26:00 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-09-29 05:39:36 UTC
Patch applied cleanly, go forth and signoff
Comment 3 I'm just a bot 2013-10-24 19:37:58 UTC
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".
Comment 4 Colin Campbell 2013-12-08 22:32:47 UTC Comment hidden (obsolete)
Comment 5 Colin Campbell 2013-12-08 22:33:47 UTC
Patch revised to accomodate changes in C4::Acquisitions
Comment 6 Mark Tompsett 2013-12-14 02:17:32 UTC
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.
Comment 7 Marc Véron 2014-02-17 15:58:23 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-02-18 13:49:18 UTC
This patch breaks some UT:
  prove t/db_dependent/Acquisition/*

Marked as Failed QA.
Comment 9 Jonathan Druart 2014-02-18 14:09:27 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-02-18 14:10:11 UTC
Comment on attachment 25371 [details] [review]
Bug 10789: Display the currency for baskets in a basketgroup

wrong bug report!
Comment 11 Marcel de Rooy 2014-02-18 15:31:10 UTC
[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.
Comment 12 Marcel de Rooy 2014-02-18 15:31:56 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2014-02-18 15:32:01 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2014-02-18 15:32:34 UTC
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>
Comment 15 Marcel de Rooy 2014-02-18 15:34:20 UTC
Created attachment 25381 [details] [review]
Bug 10789: Follow-up: Fix typo infermation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2014-02-18 15:34:27 UTC
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>
Comment 17 Galen Charlton 2014-02-18 21:46:45 UTC
Pushed to master.  Thanks, Colin!
Comment 18 Fridolin Somers 2014-05-19 14:30:17 UTC
Pushed to 3.14.x, will be in 3.14.07.

This may improve performance, that's why I've integrated it.