|
Description
Matthias Meusburger
2022-12-06 13:26:04 UTC
Created attachment 144447 [details] [review] Subject: [PATCH] Bug 32408: Allow SIP checkout when the outstanding amount is under noissuescharge 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' 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. 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 ? 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. 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. 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> 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 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. 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. 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. 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") Cool, let's allow setting this things at the account level. That would be valuable in the long run. Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to 22.11.x for next release Backported to 22.05.x for upcoming 22.05.10 enhancement + conflicts on application on 21.11.x. won't backport here |