Bug 10789 - Excessive and often incorrect use of finish in C4::Acquisitions
Summary: Excessive and often incorrect use of finish in C4::Acquisitions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Colin Campbell
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-27 15:00 UTC by Colin Campbell
Modified: 2014-12-07 20:05 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (11.19 KB, patch)
2013-08-27 15:26 UTC, Colin Campbell
Details | Diff | Splinter Review
Revised Patch (11.36 KB, patch)
2013-12-08 22:32 UTC, Colin Campbell
Details | Diff | Splinter Review
[Signed-Off] Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions (11.90 KB, patch)
2014-02-17 15:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 10789: Display the currency for baskets in a basketgroup (2.08 KB, patch)
2014-02-18 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10789: Follow-up: Fix typo infermation (811 bytes, patch)
2014-02-18 15:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid (958 bytes, patch)
2014-02-18 15:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10789 Remove unnecessary calls to finish in C4::Acquisitions (11.90 KB, patch)
2014-02-18 15:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10789: Follow-up: Fix typo infermation (811 bytes, patch)
2014-02-18 15:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid (958 bytes, patch)
2014-02-18 15:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.