While debugging another issue I found myself severely confused by some parts of the code in circulation.pl. By reviewing some earlier commits I worked out what is most likely the intent of the code and refactored it to clearer communicate that intent. The culprit of my confusion was this: https://github.com/Koha-Community/Koha/commit/1a8ca21875f86f25c72baf38e29d27af05c988b0#diff-57dd130c647858ddbf96b97d8dc5bd4cd5344ce8b0420ae8e7f1c3c17050c810 While the previous version was straight forward, this change is confusing since trying to do at least two things at once. What we want to do as far as I understand is to check for some serious errors (in this case UNKNOWN_BARCODE, but theoretically could be others as well) and allow forced onsite checkouts for all errors except for those ones. Since the display of all non blocking errors are hidden in the staff client for forced onsite checkouts, for consistency, these errors should also be hidden if some blocking error occurs. The alternative behavior would be very confusing. Now, only by chance, UNKNOWN_BARCODE is the first error in CanBookBeIssued to be checked for and retursn immediately after, so no other errors are ever registered in that case. But this could change in the future, and would cause a behavior where all errors are shown if a blocking error occur, but no errors are shown if not (for forced on site checkouts). I refactored this part to fix this issue and more clearly communicate the intended behavior. I also discovered some minor bugs introduced in various commits: https://github.com/Koha-Community/Koha/commit/2e72eb888016f60ce15958ecb37e0ae64f0c8454#diff-57dd130c647858ddbf96b97d8dc5bd4cd5344ce8b0420ae8e7f1c3c17050c810 "delete $question->{'DEBT'} if ($debt_confirmed);" is accidentally moved inside an if condition, when it should really always be performed as far as I can tell. https://github.com/Koha-Community/Koha/commit/7a7a0a2474c958e85b96db55bba04e8d90fbed31#diff-57dd130c647858ddbf96b97d8dc5bd4cd5344ce8b0420ae8e7f1c3c17050c810 Here an explicit check for DEBT_GUARANTORS is added, as a blocking error (an error that will not be ignored for on site checkouts with OnSiteCheckoutsForce enabled. That seems inconsistent with the settings description "Enable/Disable the on-site for all cases (Even if a user is debarred, etc." and the fact that all other errors returned by CanBookBeIssued UNKNOWN_BARCODE are ignored. This includes DEBT_GUARANTEES, DEBT, RESTRICTED etc.
Created attachment 126199 [details] [review] Bug 29220: Minor fixes and improved code readability
Created attachment 126213 [details] [review] Bug 29220: Minor fixes and improved code readability The current handling of onsite checkouts with OnSiteCheckoutsForce syspref enabled is confusing and hard to understand. It's also fragile with a high risk future changes could result in subtle bugs. Also fix an incosistency in wich errors are considered blocking for forced onsite checkouts. To test: 1) Enable OnSiteCheckouts, disable OnSiteCheckoutsForce 2) Edit a biblio item and for example set Not for loan to a value restricting loans 3) Try to check out the item for some patron 4) A message that item is not for loan should be displayed 5) Select "On-site checkout" and try again 6) The same message should be displayed 7) Try to checkout with some invalid barcode 8) A message that barcode was not found should be displayed 9) Now enable OnSiteCheckoutsForce 10) Try to checkout the item for some patron 11) A message that item is not for loan should be displayed 12) Select "On-site checkout" and try again 13) Checkout should now succeed, no messages should be displayed 14) Try to checkout with some invalid barcode 15) A message that barcode was not found should be displayed 16) All of the above steps should produce the same result with and without this patch Sponsored-by: Gothenburg University Library
Created attachment 126214 [details] [review] Bug 29220: Minor fixes and improved code readability The current handling of onsite checkouts with OnSiteCheckoutsForce syspref enabled is confusing and hard to understand. It's also fragile with a high risk future changes could result in subtle bugs. Also fix an inconsistency in which errors are considered blocking for forced onsite checkouts. To test: 1) Enable OnSiteCheckouts, disable OnSiteCheckoutsForce 2) Edit a biblio item and for example set Not for loan to a value restricting loans 3) Try to check out the item for some patron 4) A message that item is not for loan should be displayed 5) Select "On-site checkout" and try again 6) The same message should be displayed 7) Try to checkout with some invalid barcode 8) A message that barcode was not found should be displayed 9) Now enable OnSiteCheckoutsForce 10) Try to checkout the item for some patron 11) A message that item is not for loan should be displayed 12) Select "On-site checkout" and try again 13) Checkout should now succeed, no messages should be displayed 14) Try to checkout with some invalid barcode 15) A message that barcode was not found should be displayed 16) All of the above steps should produce the same result with and without this patch Sponsored-by: Gothenburg University Library
Created attachment 131098 [details] [review] Bug 29220: Minor fixes and improved code readability The current handling of onsite checkouts with OnSiteCheckoutsForce syspref enabled is confusing and hard to understand. It's also fragile with a high risk future changes could result in subtle bugs. Also fix an inconsistency in which errors are considered blocking for forced onsite checkouts. To test: 1) Enable OnSiteCheckouts, disable OnSiteCheckoutsForce 2) Edit a biblio item and for example set Not for loan to a value restricting loans 3) Try to check out the item for some patron 4) A message that item is not for loan should be displayed 5) Select "On-site checkout" and try again 6) The same message should be displayed 7) Try to checkout with some invalid barcode 8) A message that barcode was not found should be displayed 9) Now enable OnSiteCheckoutsForce 10) Try to checkout the item for some patron 11) A message that item is not for loan should be displayed 12) Select "On-site checkout" and try again 13) Checkout should now succeed, no messages should be displayed 14) Try to checkout with some invalid barcode 15) A message that barcode was not found should be displayed 16) All of the above steps should produce the same result with and without this patch Sponsored-by: Gothenburg University Library Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Works as described in test plan. Thanks for working on this difficult code ;)
Created attachment 131121 [details] [review] Bug 29220: Minor fixes and improved code readability The current handling of onsite checkouts with OnSiteCheckoutsForce syspref enabled is confusing and hard to understand. It's also fragile with a high risk future changes could result in subtle bugs. Also fix an inconsistency in which errors (DEBT_GUARANTORS) are considered blocking for forced onsite checkouts. To test: 1) Enable OnSiteCheckouts, disable OnSiteCheckoutsForce 2) Edit a biblio item and for example set Not for loan to a value restricting loans 3) Try to check out the item for some patron 4) A message that item is not for loan should be displayed 5) Select "On-site checkout" and try again 6) The same message should be displayed 7) Try to checkout with some invalid barcode 8) A message that barcode was not found should be displayed 9) Now enable OnSiteCheckoutsForce 10) Try to checkout the item for some patron 11) A message that item is not for loan should be displayed 12) Select "On-site checkout" and try again 13) Checkout should now succeed, no messages should be displayed 14) Try to checkout with some invalid barcode 15) A message that barcode was not found should be displayed 16) All of the above steps should produce the same result with and without this patch Additional: Before patch: a) Set NoIssuesChargeGuarantorsWithGuarantees to 1 b) add a guarantee with another guarantor c) charge a 1.01 fine to other guarantor d) checkout is blocked due to fines Apply patch e) checkout is allowed when onsite checkouts forced Sponsored-by: Gothenburg University Library Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
This does make the code more readable, and fixes a bug/inconsistency - ideally this logic would happen in CanBookBeIssued and be covered with tests, but is an improvement
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.04
Pushed to 21.05.x for 21.05.13
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.