Bug 29145 - Allow patrons to have overdue items that would not result in debarment when removing overdue debarments
Summary: Allow patrons to have overdue items that would not result in debarment when r...
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: David Gustafsson
QA Contact: Kyle M Hall
URL:
Keywords: release-notes-needed
: 16882 (view as bug list)
Depends on: 31095
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-30 12:51 UTC by David Gustafsson
Modified: 2024-02-05 15:34 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (6.89 KB, patch)
2021-09-30 12:54 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (6.96 KB, patch)
2022-09-21 14:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.58 KB, patch)
2022-09-21 14:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.99 KB, patch)
2022-10-13 13:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.94 KB, patch)
2022-10-13 13:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (7.01 KB, patch)
2022-10-21 13:50 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.63 KB, patch)
2022-10-21 13:50 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.99 KB, patch)
2022-10-21 13:51 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (7.01 KB, patch)
2022-10-30 11:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.63 KB, patch)
2022-10-30 11:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.99 KB, patch)
2022-10-30 11:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos (2.79 KB, patch)
2022-10-30 11:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add missing POD and don't return undef (1.15 KB, patch)
2022-11-04 14:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (6.96 KB, patch)
2022-12-21 16:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.60 KB, patch)
2022-12-21 16:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.99 KB, patch)
2022-12-21 16:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos (2.79 KB, patch)
2022-12-21 16:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add missing POD and don't return undef (1.15 KB, patch)
2022-12-21 16:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (7.01 KB, patch)
2022-12-21 16:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.65 KB, patch)
2022-12-21 16:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.99 KB, patch)
2022-12-21 16:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos (2.79 KB, patch)
2022-12-21 16:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add missing POD and don't return undef (1.15 KB, patch)
2022-12-21 16:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix typo (1.09 KB, patch)
2023-01-27 11:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues (1.78 KB, patch)
2023-01-27 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries (5.88 KB, patch)
2023-01-27 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Remove debugging line (1.04 KB, patch)
2023-01-27 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (7.08 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.06 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.94 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos (2.79 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add missing POD and don't return undef (1.15 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix typo (1.11 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues (1.81 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries (5.90 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Remove debugging line (1.06 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Replace GetDebarments in unit tests (2.65 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Perltidy files and added code (14.43 KB, patch)
2023-07-21 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: use overdues restrict delays when removing overdues restriction upon return (7.13 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Add tests and modify sysprefs (14.05 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Fix conditions for patron debarring overdues (9.89 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos (2.79 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add missing POD and don't return undef (1.15 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix typo (1.11 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues (1.81 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries (5.90 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Remove debugging line (1.06 KB, patch)
2023-09-29 14:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Replace GetDebarments in unit tests (2.65 KB, patch)
2023-09-29 14:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: Perltidy files and added code (14.33 KB, patch)
2023-09-29 14:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Fix tests removing 'unblessed' (2.79 KB, patch)
2023-09-29 14:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29145: (QA follow-up) Add +x to atomicupdate file (720 bytes, patch)
2023-09-29 14:08 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2021-09-30 12:51:55 UTC
The default behavior when returning an item, is to check if a patron is debarred, in that case check if the patron has any other overdue items left, and if not to lift the debarment. One might argue that it would be more logical to lift the debarment on the same conditions on which the Patron was debarred, so if the patron is in a state that would not cause debarment, the patron should not be debarred. That is: only if the patron has items that are overdue enough to cause restriction/debarment the debarment should be maintained.

This patch provides a system preference, that if set instead of checking for any delayed items on returning an item, uses the delay used for restrictions to check for items exceeding that limit, and if none are found lifts the debarment.

(Stefan Berndtsson at Gothenburg University library is the original author of this patch).
Comment 1 David Gustafsson 2021-09-30 12:54:13 UTC
Created attachment 125495 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return
Comment 2 Katrin Fischer 2022-07-26 13:52:03 UTC
Hi David and Stefan,

looking into this and trying to rephrase what I think this does:

If an item is returned, we check if the patron is still restricted by an OVERDUES restriction. If that's the case, we check if they have any remaining checkouts that have triggered an OVERDUES restriction (are overdue long enough). If there are no such remaining checkouts, we undebar.

Is that about right?

1) Could you please add a test plan and some description to the commit message?

2) You are missing the sysprefs.sql change for adding a new system preference in your patch. And in the pref file, don't use yes/no, but 0/1.

3) The new pref is named: ODueDebarmentRemovalAllowUnrestricted

I wonder if instead of using a new pref, it might be nicer to change the existing pref AutoRemoveOverduesRestrictions to have 3 options:

* Don't lift restriction
* Lift restriction if no more overdues
* Lift restriction if remaining overdues didn't trigger a restriction

(some native speaker might be able to help with a more elegant phrasing)

If we go with 2 separate preferences, they should be linked to each other in the .pref (there are a lot of examples for this in the files). Also a bit different name might be nice... but I have to admit, I don't have a good idea right now. I think I'd really prefer not to have them separate.

4) Missing unit tests for the changes in Circulation.pm and Patron.pm.
Comment 3 Katrin Fischer 2022-07-26 14:40:12 UTC
*** Bug 16882 has been marked as a duplicate of this bug. ***
Comment 4 Michael Spagnuolo 2022-09-14 17:57:16 UTC
Hi,

I would like to see movement on this. We currently allow a three day grace period after the due date, so we would like the restriction to be removed even if a patron still has overdues. 

Thanks
Comment 5 David Gustafsson 2022-09-21 14:14:50 UTC
Created attachment 140819 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library
Comment 6 David Gustafsson 2022-09-21 14:14:55 UTC
Created attachment 140820 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option
Comment 7 David Gustafsson 2022-09-21 14:17:51 UTC
Hi! Thanks for the review, good points. Have now added tests and changed AutoRemoveOverduesRestrictions to have three different options instead of using two system preferences. I think all issues should now be resolved.
Comment 8 Andrew Fuerste-Henry 2022-09-21 15:01:47 UTC
David, how's this for a test plan?

1: define overdue notice & status triggers such that a notice and restriction are generated at 2 days overdue
2: prepare 3 patrons with 1 item overdue by 1 day and 1 item overdue by 2 days. We'll call these Patron Group A
3: prepare 3 patrons with 2 items 2 days overdue. We'll call these Patron Group B
4: Run the overdues cron, confirm that notices and restrictions have generated on all of your patrons as desired
5: set AutoRemoveOverduesRestrictions to Don't.
6: Check in all items for a Group A patron. Confirm no restrictions are lifted.
7: Check in all items for a Group B patron. Confirm no restrictions are lifted.
8: set AutoRemoveOverduesRestrictions to "if patron has no remaining overdue items"
9: On a Group A patron, check in the 2 day overdue item. Confirm no restrictions are lifted.
10: On the same patron, check in the 1 day overdue item. Confirm the restriction is lifted
11: On a Group B patron, check in both items. Confirm the restriction is not lifted until both are returned.
12: set AutoRemoveOverduesRestrictions to "if patron has no remaining items that is cause for debarment"
13: On a Group A patron, check in the 2 day overdue item. Confirm the restriction is lifted.
14: On a Group B patron, check in both items. Confirm the restriction is not lifted until both are returned.

15: prove t/db_dependent/Circulation/MarkIssueReturned.t
Comment 9 Andrew Fuerste-Henry 2022-09-21 19:11:44 UTC
I went through the test plan I posted above, but hit an issue at step 14. I have a patron with two items both 2 days overdue. My overdue notice triggers generate a notice and a restriction at 2 days overdue. When I run the overdues cron, the notice and restriction generate as expected. I have AutoRemoveOverduesRestrictions set to when_no_overdue_causing_debarment. My restriction gets removed when I check in the first of the two items. This is incorrect; I should have to check in both items for the restriction to be removed.
Comment 10 David Gustafsson 2022-10-13 13:23:12 UTC
Created attachment 141822 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues
Comment 11 David Gustafsson 2022-10-13 13:30:24 UTC
(In reply to Andrew Fuerste-Henry from comment #9)
> I went through the test plan I posted above, but hit an issue at step 14. I
> have a patron with two items both 2 days overdue. My overdue notice triggers
> generate a notice and a restriction at 2 days overdue. When I run the
> overdues cron, the notice and restriction generate as expected. I have
> AutoRemoveOverduesRestrictions set to when_no_overdue_causing_debarment. My
> restriction gets removed when I check in the first of the two items. This is
> incorrect; I should have to check in both items for the restriction to be
> removed.

Hi! Thanks for the feedback and the test plan. Without it would not have discovered the edge-case where overdue items should trigger debarments when are delayed the same number of days that would trigger debarment. Have also updated the tests to reflect your test plan and result in the same error with the previous code.
Comment 12 David Gustafsson 2022-10-13 13:33:27 UTC
Created attachment 141823 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues
Comment 13 Michaela Sieber 2022-10-21 13:49:28 UTC
Thanks a lot for the patches and the test plan!
We can confirm that it works as expected :-)
Comment 14 Biblibre Sandboxes 2022-10-21 13:50:54 UTC
Created attachment 142394 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 15 Biblibre Sandboxes 2022-10-21 13:50:59 UTC
Created attachment 142395 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 16 Biblibre Sandboxes 2022-10-21 13:51:04 UTC
Created attachment 142396 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 17 Andrew Fuerste-Henry 2022-10-21 13:52:27 UTC
(In reply to Michaela Sieber from comment #13)
> Thanks a lot for the patches and the test plan!
> We can confirm that it works as expected :-)

Superb! I think this open source thing might just work :)
Comment 18 Katrin Fischer 2022-10-22 10:25:13 UTC
Please don't forget to fill in the Assignee field when taking on a bug, thx!
Comment 19 Katrin Fischer 2022-10-30 11:35:10 UTC
Hi David,

1) QA scipt

Please run the QA tests on your patches before submitting, they are catching lots of the tiny stuff that can otherwise hold things up unnecessarily. If you are using ktd for development, you can just run: qa -v 2 from kshell.

OK	C4/Circulation.pm
 FAIL	Koha/Patron.pm
   FAIL	  critic
		# Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s).  
   FAIL	  pod coverage
		POD is missing for has_debarring_overdues
		POD is missing for trim_whitespaces

 FAIL	installer/data/mysql/atomicupdate/bug_29145-modify_AutoRemoveOverduesRestrictions-syspref.pl
   FAIL	  file permissions
		File must have the exec flag
   FAIL	  spelling
		 prefernce ==> preference

 OK	installer/data/mysql/atomicupdate/odue-debarment-removal-allow-unrestricted.pl
 OK	installer/data/mysql/mandatory/sysprefs.sql
 OK	misc/cronjobs/overdue_notices.pl
 FAIL	t/db_dependent/Circulation/MarkIssueReturned.t
   FAIL	  spelling
		 remaning ==> remaining
		 remaning ==> remaining

I am providing a follow-up for the typos, but I'll need you to have a look at the missing POD for has_overdues (trim_whitespace is a false one) and especially the Perlcritic violation. I see that you have written a long comment on has_overdues, so this is about reformatting this into a proper POD entry.

2) Bug number was missing from database update.

Fixed together with the typos in a follow-up patch.

3) Changes to Patron.pm

We don't have a hard rule here yet, but we should always avoid using SQL. I am not sure if this qualifies as blocker right now as we don't have a written rule, so leaving as a note.

Please fix 1)!
Comment 20 Katrin Fischer 2022-10-30 11:35:43 UTC
Created attachment 142814 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 21 Katrin Fischer 2022-10-30 11:35:48 UTC
Created attachment 142815 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 22 Katrin Fischer 2022-10-30 11:35:53 UTC
Created attachment 142816 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 23 Katrin Fischer 2022-10-30 11:35:58 UTC
Created attachment 142817 [details] [review]
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos

Fixes some typos in test and update message that were highlighted
by the QA tools.
Adds missing bug number to the atomic update file.
Comment 24 David Gustafsson 2022-11-04 14:20:58 UTC
Created attachment 143213 [details] [review]
Bug 29145: (QA follow-up) Add missing POD and don't return undef
Comment 25 David Gustafsson 2022-11-04 14:22:58 UTC
Thanks for the feedback, will be sure to run qa-tools next time.
Comment 26 Katrin Fischer 2022-11-06 09:52:53 UTC
After QA feedback it's usually ok to set back to signed off directly :)
Comment 27 David Gustafsson 2022-12-21 16:42:26 UTC
Created attachment 144773 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library
Comment 28 David Gustafsson 2022-12-21 16:42:31 UTC
Created attachment 144774 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option
Comment 29 David Gustafsson 2022-12-21 16:42:37 UTC
Created attachment 144775 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 30 David Gustafsson 2022-12-21 16:42:42 UTC
Created attachment 144776 [details] [review]
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos

Fixes some typos in test and update message that were highlighted
by the QA tools.
Adds missing bug number to the atomic update file.
Comment 31 David Gustafsson 2022-12-21 16:42:47 UTC
Created attachment 144777 [details] [review]
Bug 29145: (QA follow-up) Add missing POD and don't return undef
Comment 32 David Gustafsson 2022-12-21 16:45:04 UTC
Ok! Got some weird error trying to apply the current patch, so reconstructed the patch set using our local branch. Should now apply to the current master.
Comment 33 David Gustafsson 2022-12-21 16:50:29 UTC
Created attachment 144778 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 34 David Gustafsson 2022-12-21 16:50:34 UTC
Created attachment 144779 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 35 David Gustafsson 2022-12-21 16:50:39 UTC
Created attachment 144780 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 36 David Gustafsson 2022-12-21 16:50:44 UTC
Created attachment 144781 [details] [review]
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos

Fixes some typos in test and update message that were highlighted
by the QA tools.

Add missing bug number to the atomic update file.
Comment 37 David Gustafsson 2022-12-21 16:50:51 UTC
Created attachment 144782 [details] [review]
Bug 29145: (QA follow-up) Add missing POD and don't return undef
Comment 38 David Gustafsson 2022-12-21 16:53:09 UTC
Forgot to add the signed-off lines for the recreacted commits (which should be identical to the previously signed-off ones).
Comment 39 Kyle M Hall 2023-01-27 11:29:48 UTC
Created attachment 145720 [details] [review]
Bug 29145: (QA follow-up) Fix typo
Comment 40 Kyle M Hall 2023-01-27 14:41:00 UTC
Created attachment 145749 [details] [review]
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues
Comment 41 Kyle M Hall 2023-01-27 14:41:13 UTC
Created attachment 145750 [details] [review]
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries
Comment 42 Kyle M Hall 2023-01-27 14:41:18 UTC
Created attachment 145751 [details] [review]
Bug 29145: (QA follow-up) Remove debugging line
Comment 43 Katrin Fischer 2023-01-27 18:20:20 UTC
Thanks, Kyle!
Comment 44 Andrew Fuerste-Henry 2023-05-19 20:01:14 UTC
Hi all! What's the current status of this patch? Is "Patch doesn't apply" accurate? David, will you be working on rebasing it? Thanks!
Comment 45 Michaela Sieber 2023-06-20 06:37:55 UTC
This bug had status Passed QA , what happened?
Comment 46 Katrin Fischer 2023-06-20 07:18:10 UTC
Koha's codebase moved on and this needs to be rebased. Rebasing means that conflicts with the now changed files in master need to be resolved, because this code doesn't 'fit' any more.
Comment 47 Tomás Cohen Arazi 2023-06-23 14:35:58 UTC
DAvid, Kyle, can we rebase this ASAP please? Thanks!
Comment 48 Tomás Cohen Arazi 2023-06-23 14:42:10 UTC
And please check the changes introduced by bug 31095.
Comment 49 Michaela Sieber 2023-07-18 06:32:45 UTC
Good Morning David,

we are very interested in this enhancement!
It would be great if you could do a rebase.
I could test your patches and sign off ASAP so we have a chance to bring it into 23.11 .

greetings from Germany :-)
Comment 50 Martin Renvoize 2023-07-21 10:42:18 UTC
This has actually already passed qa.. it just needs a rebase to get it pushed.
Comment 51 Katrin Fischer 2023-07-21 16:36:08 UTC
Created attachment 153804 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 52 Katrin Fischer 2023-07-21 16:36:12 UTC
Created attachment 153805 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 53 Katrin Fischer 2023-07-21 16:36:15 UTC
Created attachment 153806 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 54 Katrin Fischer 2023-07-21 16:36:18 UTC
Created attachment 153807 [details] [review]
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos

Fixes some typos in test and update message that were highlighted
by the QA tools.

Add missing bug number to the atomic update file.
Comment 55 Katrin Fischer 2023-07-21 16:36:22 UTC
Created attachment 153808 [details] [review]
Bug 29145: (QA follow-up) Add missing POD and don't return undef
Comment 56 Katrin Fischer 2023-07-21 16:36:25 UTC
Created attachment 153809 [details] [review]
Bug 29145: (QA follow-up) Fix typo
Comment 57 Katrin Fischer 2023-07-21 16:36:29 UTC
Created attachment 153810 [details] [review]
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues
Comment 58 Katrin Fischer 2023-07-21 16:36:32 UTC
Created attachment 153811 [details] [review]
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries
Comment 59 Katrin Fischer 2023-07-21 16:36:35 UTC
Created attachment 153812 [details] [review]
Bug 29145: (QA follow-up) Remove debugging line
Comment 60 Katrin Fischer 2023-07-21 16:36:39 UTC
Created attachment 153813 [details] [review]
Bug 29145: Replace GetDebarments in unit tests

GetDebarments was removed and replaced by $patron->restrictions.
The tests needed some adjustments to reflect that change.
Comment 61 Katrin Fischer 2023-07-21 16:36:42 UTC
Created attachment 153814 [details] [review]
Bug 29145: Perltidy files and added code
Comment 62 Katrin Fischer 2023-07-21 16:41:40 UTC
This was ultimately not a bad rebase. The trouble came from bug 31095 removing GetDebarments.

The only conflicts where in C4/Circulation.pm MarkIssueReturned in the last paragraph:

        # Possibly remove any OVERDUES related debarment
        my $overdue_restrictions = $patron->restrictions->search( { type => 'OVERDUES' } );
        if ( C4::Context->preference('AutoRemoveOverduesRestrictions') ne 'no' && $patron->is_debarred ) {
            my $remove_restrictions =
                C4::Context->preference('AutoRemoveOverduesRestrictions') eq 'when_no_overdue_causing_debarment'
                ? !$patron->has_restricting_overdues( { issue_branchcode => $issue_branchcode } )
                : !$patron->has_overdues;
            if ( $remove_restrictions && $overdue_restrictions->count ) {
                DelUniqueDebarment( { borrowernumber => $borrowernumber, type => 'OVERDUES' } );
            }
        }

Once I got it rebased, I rewrote the tests to use $patron->restrictions instead of GetDebarments. Tests pass now again!

I fixed some "John Doe" commit authorships from Kyle - realizing that our mailmap already resolves "John Doe" to Kyle automatically, which is a little confusing when tig, git log and the QA test show different things. Should be all nice now :)

And last but not least: I applied some perltidy to the files. I didn't get a perfect result sticking to the changed paragraphs, but it's very close.
Comment 63 Katrin Fischer 2023-09-29 14:07:33 UTC
Created attachment 156392 [details] [review]
Bug 29145: use overdues restrict delays when removing overdues restriction upon return

How to test:
1) Run tests in t/db_dependent/Circulation/MarkIssueReturned.t

Sponsored by: Gothenburg University Library

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 64 Katrin Fischer 2023-09-29 14:07:36 UTC
Created attachment 156393 [details] [review]
Bug 29145: Add tests and modify sysprefs

Add tests, remove ODueDebarmentRemovalAllowUnrestricted syspref and instead
modify AutoRemoveOverduesRestrictions to have a third option

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 65 Katrin Fischer 2023-09-29 14:07:39 UTC
Created attachment 156394 [details] [review]
Bug 29145: Fix conditions for patron debarring overdues

Signed-off-by: Michaela <michaela.sieber@kit.edu>
Comment 66 Katrin Fischer 2023-09-29 14:07:42 UTC
Created attachment 156395 [details] [review]
Bug 29145: (QA follow-up) Fix missing bug number in database update and typos

Fixes some typos in test and update message that were highlighted
by the QA tools.

Add missing bug number to the atomic update file.
Comment 67 Katrin Fischer 2023-09-29 14:07:46 UTC
Created attachment 156396 [details] [review]
Bug 29145: (QA follow-up) Add missing POD and don't return undef
Comment 68 Katrin Fischer 2023-09-29 14:07:49 UTC
Created attachment 156397 [details] [review]
Bug 29145: (QA follow-up) Fix typo
Comment 69 Katrin Fischer 2023-09-29 14:07:52 UTC
Created attachment 156398 [details] [review]
Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues
Comment 70 Katrin Fischer 2023-09-29 14:07:55 UTC
Created attachment 156399 [details] [review]
Bug 29145: (QA follow-up) Use objects instead of straight SQL queries
Comment 71 Katrin Fischer 2023-09-29 14:07:59 UTC
Created attachment 156400 [details] [review]
Bug 29145: (QA follow-up) Remove debugging line
Comment 72 Katrin Fischer 2023-09-29 14:08:02 UTC
Created attachment 156401 [details] [review]
Bug 29145: Replace GetDebarments in unit tests

GetDebarments was removed and replaced by $patron->restrictions.
The tests needed some adjustments to reflect that change.
Comment 73 Katrin Fischer 2023-09-29 14:08:05 UTC
Created attachment 156402 [details] [review]
Bug 29145: Perltidy files and added code
Comment 74 Katrin Fischer 2023-09-29 14:08:09 UTC
Created attachment 156403 [details] [review]
Bug 29145: (QA follow-up) Fix tests removing 'unblessed'

This fixes issues with the tests caused by:
Bug 32496: Reduce unnecessary unblessings of objects in Circulation.pm
Comment 75 Katrin Fischer 2023-09-29 14:08:12 UTC
Created attachment 156404 [details] [review]
Bug 29145: (QA follow-up) Add +x to atomicupdate file
Comment 76 Katrin Fischer 2023-09-29 14:12:34 UTC
Conflict was only in the test file, caused by bug 32496. Tests pass now :)
Comment 77 Fridolin Somers 2023-10-05 19:24:50 UTC
Enhancement not pushed to 23.05.x