[Bug 29220] New: Minor fixes and improved code readability in circulation.pl
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Bug ID: 29220 Summary: Minor fixes and improved code readability in circulation.pl Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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/1a8ca21875f86f25c72baf38e29d27... 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/2e72eb888016f60ce15958ecb37e0a... "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/7a7a0a2474c958e85b96db55bba04e... 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 126199 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=126199&action=edit Bug 29220: Minor fixes and improved code readability -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #126199|0 |1 is obsolete| | --- Comment #2 from David Gustafsson <glasklas@gmail.com> --- Created attachment 126213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=126213&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #126213|0 |1 is obsolete| | --- Comment #3 from David Gustafsson <glasklas@gmail.com> --- Created attachment 126214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=126214&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #126214|0 |1 is obsolete| | --- Comment #4 from Fridolin Somers <fridolin.somers@biblibre.com> --- Created attachment 131098 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=131098&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #5 from Fridolin Somers <fridolin.somers@biblibre.com> --- Works as described in test plan. Thanks for working on this difficult code ;) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #131098|0 |1 is obsolete| | --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 131121 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=131121&action=edit 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> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #7 from Nick Clemens <nick@bywatersolutions.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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 --- Comment #8 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|22.05.00 |22.05.00,21.11.04 released in| | CC| |kyle@bywatersolutions.com --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.04 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00,21.11.04 |22.05.00,21.11.04,21.05.13 released in| | CC| |andrew@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable --- Comment #10 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.13 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29220 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED CC| |victor@tuxayo.net --- Comment #11 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org