Bug 6280

Summary: Invalid SQL being passed in circulation checkout
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: CirculationAssignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: PATCH-Sent (DO NOT USE) CC: chris, dev_patches, gmcharlt, katrin.fischer, koha.sekjal, paul.poulain
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed Patch
Signed-off patch
proposed patch (passed QA)

Description Colin Campbell 2011-04-28 13:00:26 UTC
Bug 5952 changed the interface to C4::Members::GetPendingIssues however in circulation.pl it is now being called with an empty array in many cases so that a simple checkout is resulting in two calls the first (for the borrower succeeds) the second for the relatives fails to getting an error return from the database

This is not good
Comment 1 Colin Campbell 2011-04-28 13:48:46 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-06-09 13:42:04 UTC
Is an error this patch addresses?

circulation.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ORDER BY issues.issuedate' at line 25 at /kohaclone/C4/Members.pm line 1070
Comment 3 Colin Campbell 2011-06-09 14:22:39 UTC
Yes its executing bad syntax like WHERE ORDER BY and the second (failing) query is unnecessary
Comment 4 Owen Leonard 2011-06-09 14:32:11 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2011-07-08 10:30:47 UTC
*** Bug 6568 has been marked as a duplicate of this bug. ***
Comment 6 Paul Poulain 2011-08-10 13:00:53 UTC
Created attachment 4935 [details] [review]
proposed patch (passed QA)

QA comment

* works as announced, usefull patch, logs will thank you forever ;-)
* Just a minor comment/suggestion. You write:
+if ( @borrowernumbers ) {
+    $relissue    = GetPendingIssues(@borrowernumbers);
+}
It's correct although it's more perlish to have :
$relissue    = GetPendingIssues(@borrowernumbers) if @borrowernumbers;

I agree with the change 
-        ($_->{date_due} lt $today) and $_->{overdue} = 1;
to 
+        if ($_->{date_due}  and $_->{date_due} lt $today) {
+            $_->{overdue} = 1;
+        }
(we must be Perlish, but sometimes that result in really cryptic things...)


passed QA
Comment 7 Colin Campbell 2011-08-10 14:44:04 UTC
Paul it is considered bad practice in many perl circles these days to use postfix if except for flow of control statements e.g. " LAST if $x;". They cause problems because people often miss them when rereading code. The default policies in perlcritic will warn against them see Perl Best Practices for a rationale
Comment 8 Chris Cormack 2011-08-10 18:59:29 UTC
I agree with Colin, we should follow Perl best practices where ever we can.

And if you look at bug 6679, you will see we are extending the Perl::Critic tests slowly to cover everything.
Comment 9 Chris Cormack 2011-08-11 00:56:46 UTC
Pushed, please test
Comment 10 Ian Walls 2011-09-30 18:42:40 UTC
*** Bug 6805 has been marked as a duplicate of this bug. ***