Bug 32408 - If a fine can be overridden on checkout in Koha, what should the SIP client do?
Summary: If a fine can be overridden on checkout in Koha, what should the SIP client do?
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Matthias Meusburger
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-06 13:26 UTC by Matthias Meusburger
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03,22.05.10


Attachments
Subject: [PATCH] Bug 32408: Allow SIP checkout when the outstanding amount is under noissuescharge (7.56 KB, patch)
2022-12-06 13:48 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 32408: Allow SIP checkout when the outstanding amount is under noissuescharge (7.55 KB, patch)
2022-12-07 14:12 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 32408: Allow SIP to override a fine on checkout if a librarian can (7.46 KB, patch)
2022-12-07 16:13 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 32408: Allow SIP to override a fine on checkout if a librarian can (7.52 KB, patch)
2022-12-07 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32408: Add unit tests to SIP checkout (6.48 KB, patch)
2022-12-08 10:41 UTC, Matthias Meusburger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2022-12-06 13:26:04 UTC
Bug 11633 blocked SIP checkout when the patron has fines under the noissuescharge amount (define maximum amount withstanding before checkouts are blocked).

Some libraries may want to allow SIP checkout in that case.
Comment 1 Matthias Meusburger 2022-12-06 13:48:23 UTC
Created attachment 144447 [details] [review]
Subject: [PATCH] Bug 32408: Allow SIP checkout when the outstanding amount is under noissuescharge
Comment 2 Katrin Fischer 2022-12-06 22:37:09 UTC
Hm, this sounds more like a bug, if noissuecharge is actually the one to blame.

Are we sure it's not AllowFineOverride that is to be blamed here? We found it blocking issues from the self check in the past.

There is a capitalization issue here:
+                    $message = 'Outstanding Fines block issue'
Comment 3 Matthias Meusburger 2022-12-07 14:12:09 UTC
Created attachment 144469 [details] [review]
Bug 32408: Allow SIP checkout when the outstanding amount is under noissuescharge

Test plan:

 - Set up a patron with an outstanding amount under the noissuescharge syspref

 - Do a SIP checkout, and check that it is blocked

 - Apply the patch, and check that under_noissuescharge_block_checkout is
   enabled in SIPconfig.xml

 - Do a SIP checkout, and check that it is still blocked

 - Disable under_noissuescharge_block_checkout in SIPconfig.xml

 - Do a SIP checkout, and check that is allowed

 - Prove t/db_dependent/SIP/Transaction.t

Additionaly, you can enable show_outstanding_amount in SIPconfig.xml to have
the outstanding amount displayed on SIP checkout.
Comment 4 Matthias Meusburger 2022-12-07 14:36:05 UTC
Thanks for your feedback, Katrin.

The capitalization issue has been fixed in the updated patch, reverting to Needs Signoff.

As for your other concern, the thing is that we may want to have a different behavior in SIP than in Koha.

You can see for example the following parameters that have already been added to SIPconfig.xml:

 - holds_block_checkin (previously no_holds_checkin):
 Bug 25541: Add ability to prevent checkin via SIP of items with holds

 - holds_get_captured:
 Bug 29936: Add holds_get_captured option to sip config

 - prevcheckout_block_checkout:
 Bug 26591: SIP option prevcheckout_block_checkout to block checkout of previously checked-out document

 - overdues_block_checkout (previously disable_too_many_overdue)

All of these parameters are designed to give the ability to have a specific behavior for SIP.

The one introduced in this patch is no different.

What do you think ?
Comment 5 Matthias Meusburger 2022-12-07 16:13:46 UTC
Created attachment 144473 [details] [review]
Bug 32408: Allow SIP to override a fine on checkout if a librarian can

Test plan:

 - Set up a patron with an outstanding amount under the noissuescharge syspref

 - Check that you can override the fine on checkout in Koha

 - Do a SIP checkout, and check that it is blocked

 - Apply the patch, and check that override_fine_on_checkout is
   disabled in SIPconfig.xml

 - Do a SIP checkout, and check that it is still blocked

 - Enable override_fine_on_checkout in SIPconfig.xml

 - Do a SIP checkout, and check that is allowed

 - Prove t/db_dependent/SIP/Transaction.t

Additionaly, you can enable show_outstanding_amount in SIPconfig.xml to have
the outstanding amount displayed on SIP checkout.
Comment 6 Matthias Meusburger 2022-12-07 16:15:48 UTC
I have edited the bug title, the parameter used, the patch title and the test plan to make things more clear about what this patch does. Hope it helps.
Comment 7 Martin Renvoize 2022-12-07 16:19:15 UTC
Created attachment 144474 [details] [review]
Bug 32408: Allow SIP to override a fine on checkout if a librarian can

Test plan:

 - Set up a patron with an outstanding amount under the noissuescharge syspref

 - Check that you can override the fine on checkout in Koha

 - Do a SIP checkout, and check that it is blocked

 - Apply the patch, and check that override_fine_on_checkout is
   disabled in SIPconfig.xml

 - Do a SIP checkout, and check that it is still blocked

 - Enable override_fine_on_checkout in SIPconfig.xml

 - Do a SIP checkout, and check that is allowed

 - Prove t/db_dependent/SIP/Transaction.t

Additionaly, you can enable show_outstanding_amount in SIPconfig.xml to have
the outstanding amount displayed on SIP checkout.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-12-07 16:21:01 UTC
I worked through understanding this with Matthias and we tweaked it a little to clarify intention.

It works well and is much clearer now too. No regressions, QA scripts are happy and it's well unit tested.

Going straight for a PQA
Comment 9 Martin Renvoize 2022-12-07 16:28:41 UTC
OK.. I just added this to the wiki page...

It highlighted we already have a system preference override for this maybe: https://wiki.koha-community.org/wiki/SIP2_configuration#syspref_overrides_2

Does the above achieve the same thing.. now I'm a little confused..

I'll ask Kyle to weight in.
Comment 10 Katrin Fischer 2022-12-07 21:06:37 UTC
What I thought was happening:

* Koha and self check block checkout, if amount > noissuecharge
* Koha asks for confirmation, if  AllFinesNeedOverride = require, no matter the amount
* Self check blocks checkout, if AllFinesNeedOverride = require, no matter the amount

I had wrongly copied  AllowFineOverride yesterday, but AllFinesNeedOverride was the one that I knew blocked checkouts.

So my thinking here was that we are not changing the behaviour of noissuecharge, but that you actually want to override AllFinesNeedOverride.
Comment 11 Matthias Meusburger 2022-12-08 10:31:56 UTC
Martin, Katrin, you were right, this can be done by overriding sysprefs.

This table sums it up (the columns Under and Over are referring to Under noissuescharge or Over noissuescharge):

+----------------------+-------------------+---------+---------+
| AllFinesNeedOverride | AllowFineOverride |  Under  |  Over   |
+----------------------+-------------------+---------+---------+
|                    0 |                 0 | allowed | blocked |
|                    0 |                 1 | allowed | blocked |
|                    1 |                 0 | blocked | blocked |
|                    1 |                 1 | blocked | blocked |
+----------------------+-------------------+---------+---------+

Since I wrote unit tests to generate this table, I will update the patch so they can be added to Koha.

However, the "allowed" cases are not returning any message to SIP, so I will open another BZ to have meaningful messages in those cases.
Comment 12 Matthias Meusburger 2022-12-08 10:41:55 UTC
Created attachment 144496 [details] [review]
Bug 32408: Add unit tests to SIP checkout

This patch adds unit tests to show SIP's behavior on checkout according to the
AllFinesNeedOverride, AllowFineOverride and noissuescharge sysprefs, knowingly:

+----------------------+-------------------+---------+---------+
| AllFinesNeedOverride | AllowFineOverride |  Under  |  Over   |
+----------------------+-------------------+---------+---------+
|                    0 |                 0 | allowed | blocked |
|                    0 |                 1 | allowed | blocked |
|                    1 |                 0 | blocked | blocked |
|                    1 |                 1 | blocked | blocked |
+----------------------+-------------------+---------+---------+

(the columns Under and Over are referring to "under noissuescharge" and
"over noissuescharge")
Comment 13 Tomás Cohen Arazi 2023-02-07 18:52:12 UTC
Cool, let's allow setting this things at the account level. That would be valuable in the long run.
Comment 14 Tomás Cohen Arazi 2023-02-08 12:06:04 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Pedro Amorim 2023-02-09 13:12:53 UTC
Nice work everyone!

Pushed to 22.11.x for next release
Comment 16 Lucas Gass 2023-02-15 21:48:31 UTC
Backported to 22.05.x for upcoming 22.05.10
Comment 17 Arthur Suzuki 2023-02-17 13:07:06 UTC
enhancement + conflicts on application on 21.11.x.
won't backport here