Bug 39532 - Script debar_patrons_with_fines.pl should not use MANUAL restriction type
Summary: Script debar_patrons_with_fines.pl should not use MANUAL restriction type
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 15157
Blocks:
  Show dependency treegraph
 
Reported: 2025-04-02 15:46 UTC by Kyle M Hall (khall)
Modified: 2025-05-16 12:34 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type (2.35 KB, patch)
2025-04-02 15:58 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type (2.38 KB, patch)
2025-04-03 11:57 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type (2.44 KB, patch)
2025-04-23 06:44 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39532: Script debar_patrons_with_fines.pl creates repeat restrictions, uses wrong restriction type (2.51 KB, patch)
2025-05-12 13:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2025-04-02 15:46:56 UTC
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.
Comment 1 Kyle M Hall (khall) 2025-04-02 15:58:03 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2025-04-03 11:57:31 UTC
Created attachment 180493 [details] [review]
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>
Comment 3 Emmi Takkinen 2025-04-23 06:44:24 UTC
Created attachment 181335 [details] [review]
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>
Comment 4 Martin Renvoize (ashimema) 2025-05-12 13:03:17 UTC
Created attachment 182271 [details] [review]
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>
Comment 5 Katrin Fischer 2025-05-13 16:22:27 UTC
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.
Comment 6 Katrin Fischer 2025-05-13 16:49:56 UTC
Pushed and reverted... sorry!
Comment 7 Kyle M Hall (khall) 2025-05-16 12:18:18 UTC
> 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.
Comment 8 Katrin Fischer 2025-05-16 12:34:50 UTC
(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.