Bug 29220

Summary:	Minor fixes and improved code readability in circulation.pl
Product:	Koha	Reporter:	David Gustafsson <glasklas>
Component:	Circulation	Assignee:	Bugs List <koha-bugs>
Status:	CLOSED FIXED	QA Contact:	Testopia <testopia>
Severity:	minor
Priority:	P5 - low	CC:	andrew, fridolin.somers, gmcharlt, kyle.m.hall, kyle, nick, victor
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	Small patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	22.05.00,21.11.04,21.05.13
Circulation function:
Attachments:	Bug 29220: Minor fixes and improved code readability Bug 29220: Minor fixes and improved code readability Bug 29220: Minor fixes and improved code readability Bug 29220: Minor fixes and improved code readability Bug 29220: Minor fixes and improved code readability

Description David Gustafsson 2021-10-13 13:33:12 UTC

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.

Comment 1 David Gustafsson 2021-10-13 13:40:30 UTC

Created attachment 126199 [details] [review]
Bug 29220: Minor fixes and improved code readability

Comment 2 David Gustafsson 2021-10-13 14:21:48 UTC

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

Comment 3 David Gustafsson 2021-10-13 14:22:53 UTC

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

Comment 4 Fridolin Somers 2022-02-25 06:40:34 UTC

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>

Comment 5 Fridolin Somers 2022-02-25 06:41:35 UTC

Works as described in test plan.

Thanks for working on this difficult code ;)

Comment 6 Nick Clemens (kidclamp) 2022-02-25 13:57:57 UTC

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>

Comment 7 Nick Clemens (kidclamp) 2022-02-25 13:58:55 UTC

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

Comment 8 Fridolin Somers 2022-03-04 02:11:59 UTC

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

Comment 9 Kyle M Hall (khall) 2022-03-04 14:41:46 UTC

Pushed to 21.11.x for 21.11.04

Comment 10 Andrew Fuerste-Henry 2022-03-20 15:44:49 UTC

Pushed to 21.05.x for 21.05.13

Comment 11 Victor Grousset/tuxayo 2022-03-21 00:18:29 UTC

Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.