Created attachment 61719 [details] [review] Bug 18349: SCO - Do not trust the confirmed flag The "confirmed" flag is used to know if that user confirmed a situation that needs a confirmation. But if the issue/renew is impossible the CanBookBeIssued and the 'impossible flags' should be checked. Otherwise a patron can checkout and renew bypassing the circulation rules (Understand 'no limit' here...) Test plan: Want to renew? Checkin $barcode, then /cgi-bin/koha/sco/sco-main.pl?patronid=$cardnumber&barcode=$barcode&confirmed=1&op=checkout Want to bypass the checkin? Same url...
Created attachment 61720 [details] [review] Bug 18349: Remove unused $issuenoconfirm var
Want some fun guys?
Hmm..With this patch 1 - I checkout item #1 to borrower A 2 - Sign in to SCO as borrower B 3 - Construct link as specified using barcode of item #1 and cardnumber of borrower B 4 - Item checks out to borrower B with no problem 5 - Renewals never work and no failure notice is passed
Created attachment 61755 [details] [review] Bug 18349: $borrower is a hashref I guess this code was never called before...
(In reply to Nick Clemens from comment #4) Sorry, I assumed that the existing code was working! Silly me...
Created attachment 61756 [details] [review] Bug 18349: If a confirmation is required, consider as blocker Only RENEW_ISSUE should not be considered as blocker, others should. Note that this code is not robust and the whole script as well as the return of CanBookBeIssued should be rewrittent completely. We need a small patch to easy backport to stable releases
Created attachment 61757 [details] [review] Bug 18349: Remove useless info in alert message Empty tags should be removed, and message id Note that not all error codes are covered here.
Created attachment 61908 [details] [review] Bug 18349: SCO - Do not trust the confirmed flag The "confirmed" flag is used to know if that user confirmed a situation that needs a confirmation. But if the issue/renew is impossible the CanBookBeIssued and the 'impossible flags' should be checked. Otherwise a patron can checkout and renew bypassing the circulation rules (Understand 'no limit' here...) Test plan: Want to renew? Checkin $barcode, then /cgi-bin/koha/sco/sco-main.pl?patronid=$cardnumber&barcode=$barcode&confirmed=1&op=checkout Want to bypass the checkin? Same url... Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 61909 [details] [review] Bug 18349: $borrower is a hashref I guess this code was never called before... Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 61910 [details] [review] Bug 18349: If a confirmation is required, consider as blocker Only RENEW_ISSUE should not be considered as blocker, others should. Note that this code is not robust and the whole script as well as the return of CanBookBeIssued should be rewrittent completely. We need a small patch to easy backport to stable releases Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 61911 [details] [review] Bug 18349: Remove useless info in alert message Empty tags should be removed, and message id Note that not all error codes are covered here. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
QA: Looking here now
Created attachment 61959 [details] [review] Bug 18349: [QA Follow-up] Add four missing error messages The error codes come from CanBookBeIssued. The warnings speak for themselves. Note that the GNA message is similar to the one used in opac-user.tt (gonenoaddress is translated to the patron as contact information not up-to-date). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 17.05, thanks Jonathan, Marcel!
These patches have been pushed to 16.11.x and will be in 16.11.07.
Pushed to 3.22.x for 3.22.20
Pushed to 16.05.x, for 16.05.12 release
The downside of this code is that it traps the user in a loop. They keep pressing "Renew item" in the "Please confirm the checkout" box, and it just loops back to the same screen prompting them to do the same thing.
This patch blocks the "OverduesBlockRenewing" system preference. That may or may not be intentional. But in any case we should alert the user that they're forbidden from doing what they're trying to do... or not show them the option at all... it's not clear what Koha is trying to do here.
David, if you think there is a bug can you open a new bug report with a step-by-step plan to recreate the problem?
(In reply to Jonathan Druart from comment #21) > David, if you think there is a bug can you open a new bug report with a > step-by-step plan to recreate the problem? Well, there is the issue of the looping bug, but then there's the issue of whether or not this patch made 100% sense in the first place, so good to alert the CC list to it. But... yeah I'll be looking into this more soon.
(In reply to Jonathan Druart from comment #7) > Note that this code is not robust and the whole script as well as the > return of CanBookBeIssued should be rewrittent completely. Agreed. The sco-main.pl script is terrible. Trying to patch it is a challenge. See Bug 26301...
(In reply to David Cook from comment #23) > (In reply to Jonathan Druart from comment #7) > > Note that this code is not robust and the whole script as well as the > > return of CanBookBeIssued should be rewrittent completely. > > Agreed. The sco-main.pl script is terrible. > > Trying to patch it is a challenge. > > See Bug 26301... Ok now I can't reproduce this issue in master. I thought I could, but maybe it's just limited to older versions of 19.11...
(In reply to David Cook from comment #24) > Ok now I can't reproduce this issue in master. I thought I could, but maybe > it's just limited to older versions of 19.11... Ah no it is definitely a problem when OverduesBlockCirc is set to "Ask for confirmation". Patching on Bug 26301 after all...