[Bug 39532] New: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Bug ID: 39532 Summary: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Command-line Utilities Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: robin@catalyst.net.nz The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. -- 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=39532 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15157 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15157 [Bug 15157] Cronjob to automatically restrict patrons with pending/unpaid charges -- 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=39532 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- 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=39532 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #1 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 180419 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=180419&action=edit Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. Test Plan: 1) Set up a patron with fines 2) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 3) Note the restriction is created 4) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 5) Note another restriction is created 6) Delete the restrictions 7) Apply this patch 8) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 9) Note the restriction is created 10) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 11) Note another restriction is *not* created! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #180419|0 |1 is obsolete| | --- Comment #2 from Magnus Enger <magnus@libriotech.no> --- Created attachment 180493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=180493&action=edit Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. Test Plan: 1) Set up a patron with fines 2) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 3) Note the restriction is created 4) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 5) Note another restriction is created 6) Delete the restrictions 7) Apply this patch 8) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 9) Note the restriction is created 10) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 11) Note another restriction is *not* created! Signed-off-by: Magnus Enger <magnus@libriotech.no> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #180493|0 |1 is obsolete| | --- Comment #3 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 181335 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181335&action=edit Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. Test Plan: 1) Set up a patron with fines 2) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 3) Note the restriction is created 4) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 5) Note another restriction is created 6) Delete the restrictions 7) Apply this patch 8) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 9) Note the restriction is created 10) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 11) Note another restriction is *not* created! Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #181335|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 182271 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182271&action=edit Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. Test Plan: 1) Set up a patron with fines 2) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 3) Note the restriction is created 4) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 5) Note another restriction is created 6) Delete the restrictions 7) Apply this patch 8) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 9) Note the restriction is created 10) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 11) Note another restriction is *not* created! Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Script |Script |debar_patrons_with_fines.pl |debar_patrons_with_fines.pl |creates repeat |should use SUSPENSION |restrictions, uses wrong |restriction type |restriction type | Version|unspecified |Main Status|Passed QA |Failed QA --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Have to confess I am a bit worried about this interfering with any existing SUSPENSION restrictions that are set with specific expiration dates. See: _calculate_new_debar_dt As I see it running this scripts will just overwrite any existing SUSPENSION restriction with a date provided in the script when run. It feels to risky especially as a change in behavior that might not be noticed in time. I feel like we should not reuse the code here but make the code to use configurable. If we need to pick I think OVERDUES would be the better choice as this is just an indefinite one where the risk of interfering is lower. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Script |Script |debar_patrons_with_fines.pl |debar_patrons_with_fines.pl |should use SUSPENSION |should not use MANUAL |restriction type |restriction type -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed and reverted... sorry! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> ---
As I see it running this scripts will just overwrite any existing SUSPENSION restriction with a date provided in the script when run. It feels to risky especially as a change in behavior that might not be noticed in time.
That is actually a feature of the SUSPENSION restriction. It's a singleton that should be updated to the longer of the new date or the existing date ( if there is one ). This is its very purpose. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Kyle M Hall (khall) from comment #7)
As I see it running this scripts will just overwrite any existing SUSPENSION restriction with a date provided in the script when run. It feels to risky especially as a change in behavior that might not be noticed in time.
That is actually a feature of the SUSPENSION restriction. It's a singleton that should be updated to the longer of the new date or the existing date ( if there is one ). This is its very purpose.
Yes, I am aware, but I will we are mixing use cases here that are different and we might "disturb" the original use of SUSPENSION with this feature. Say we have a library system with mulitple branches. Some use the fine in days feature that sets the SUSPENSION restriction on checkin for overdue items. Some use fines (only for those the CLI script would be interesting) OR: we have one library, that has some item types they charge fines for and fine days for others. They run the script and it will overwrite the existing time limited SUSPENSION to be indefinite. What happens if more overdue items are returned that would add days to that indefinite suspension? Do libraries really expect the time limited SUSPENSION to be turned into INDEFINITE? I agree that you need a unique restriction type here to avoid the repeat restrictions. I just argue against re-using the existing SUSPENSION. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189719 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189719&action=edit Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type The cronjob debar_patrons_with_fines.pl is meant to add a restriction based on a fines threshold. The problem is that the script adds a MANUAL restriction, which makes no sense. MANUAL restrictions are meant to be manually created, never automatic as this script does. In addition, they are repeatable, not unique. Every time this script runs, it will add an additional restriction. There is already a restriction type meant for this type of use case: SUSPENSION We should change this script to create SUSPENSION restrictions instead of MANUAL restrictions. In summary, this script should use the unique restriction type SUSPENSION to prevent multiple additional restrictions from being created on each run. Test Plan: 1) Set up a patron with fines 2) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 3) Note the restriction is created 4) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 5) Note another restriction is created 6) Delete the restrictions 7) Apply this patch 8) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" 9) Note the restriction is created 10) Run misc/cronjobs/debar_patrons_with_fines.pl -c -m "Test message" again 11) Note another restriction is *not* created! Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189720 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189720&action=edit Bug 39532: Use new FINES restriction type to avoid conflict with SUSPENSION This patch introduces a new unique restriction type 'FINES' specifically for the debar_patrons_with_fines.pl script to prevent duplicate restrictions on repeated runs. The original fix used 'SUSPENSION' but this conflicted with the fine-in-days feature which uses SUSPENSION for time-limited restrictions when overdue items are returned. Running the script would overwrite those time-limited restrictions with indefinite ones. Changes: - Add new 'FINES' system restriction type to patron_restriction_types.yml for new installations - Create database update to add 'FINES' type to existing installations - Update debar_patrons_with_fines.pl to use 'FINES' instead of 'SUSPENSION' - Update AddUniqueDebarment and DelUniqueDebarment POD documentation This keeps SUSPENSION exclusively for the fine-in-days feature while preventing duplicate restrictions from the fines script. Test plan: 1. Run database update 2. Verify new FINES restriction type exists in restriction_types table 3. Run debar_patrons_with_fines.pl multiple times 4. Confirm only one FINES restriction per patron (no duplicates) 5. Verify SUSPENSION restrictions from fine-in-days are not affected 6. Run tests: prove t/db_dependent/Patron/Borrower_Debarments.t Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182271|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Resurrected with a follow-up that resolves all Katrins raised issues. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #12 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|25.11.00 |25.11.00,25.05.06 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 --- Comment #13 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39532 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to stable |Needs documenting --- Comment #14 from Fridolin Somers <fridolin.somers@biblibre.com> --- I prefer not impact 24.11.x LTS for stability -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org