[Bug 10789] New: Excessive and often incorrect use of finish in C4::Acquisitions
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Bug ID: 10789 Summary: Excessive and often incorrect use of finish in C4::Acquisitions Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: colin.campbell@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |colin.campbell@ptfs-europe. |ity.org |com --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 20654 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20654&action=edit Proposed Patch Patch should not alter functionality. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gitbot@bugs.koha-community. | |org When did the bot| |2013-09-29 last check this| | --- Comment #2 from I'm just a bot <gitbot@bugs.koha-community.org> --- Patch applied cleanly, go forth and signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply When did the bot|2013-09-29 00:00:00 |2013-10-25 last check this| | --- Comment #3 from I'm just a bot <gitbot@bugs.koha-community.org> --- 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". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20654|0 |1 is obsolete| | --- Comment #4 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 23349 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23349&action=edit Revised Patch Patch rebased against current master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #5 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Patch revised to accomodate changes in C4::Acquisitions -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 --- Comment #7 from Marc Véron <veron@veron.ch> --- Created attachment 25350 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25350&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23349|0 |1 is obsolete| | CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Marc Véron <veron@veron.ch> 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=10789 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- This patch breaks some UT: prove t/db_dependent/Acquisition/* Marked as Failed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 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=10789 --- Comment #9 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 25371 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25371&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25371|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@biblibre.com> --- Comment on attachment 25371 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25371 Bug 10789: Display the currency for baskets in a basketgroup wrong bug report! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #11 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 --- Comment #12 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25378 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25378&action=edit Bug 10789: Follow-up: Fix typo infermation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 --- Comment #13 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25379 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25379&action=edit Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 --- Comment #14 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25380 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25380&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25350|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25378|0 |1 is obsolete| | --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25381 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25381&action=edit Bug 10789: Follow-up: Fix typo infermation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25379|0 |1 is obsolete| | --- Comment #16 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25382 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25382&action=edit Bug 10789: Follow-up: Added removed second sql parameter in GetLastOrderReceivedFromSubscriptionid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> 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=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |gmcharlt@gmail.com --- Comment #17 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master. Thanks, Colin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11550 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10789 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolyn.somers@biblibre.co | |m Patch complexity|--- |Small patch --- Comment #18 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Pushed to 3.14.x, will be in 3.14.07. This may improve performance, that's why I've integrated it. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org