Description
David Gustafsson
2021-09-30 12:51:55 UTC
Created attachment 125495 [details] [review] Bug 29145: use overdues restrict delays when removing overdues restriction upon return 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. *** Bug 16882 has been marked as a duplicate of this bug. *** 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 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 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 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. 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 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. Created attachment 141822 [details] [review] Bug 29145: Fix conditions for patron debarring overdues (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. Created attachment 141823 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Thanks a lot for the patches and the test plan! We can confirm that it works as expected :-) 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> 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> Created attachment 142396 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> (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 :) Please don't forget to fill in the Assignee field when taking on a bug, thx! 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)! 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> 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> Created attachment 142816 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> 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. Created attachment 143213 [details] [review] Bug 29145: (QA follow-up) Add missing POD and don't return undef Thanks for the feedback, will be sure to run qa-tools next time. After QA feedback it's usually ok to set back to signed off directly :) 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 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 Created attachment 144775 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> 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. Created attachment 144777 [details] [review] Bug 29145: (QA follow-up) Add missing POD and don't return undef 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. 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> 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> Created attachment 144780 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> 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. Created attachment 144782 [details] [review] Bug 29145: (QA follow-up) Add missing POD and don't return undef Forgot to add the signed-off lines for the recreacted commits (which should be identical to the previously signed-off ones). Created attachment 145720 [details] [review] Bug 29145: (QA follow-up) Fix typo Created attachment 145749 [details] [review] Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues Created attachment 145750 [details] [review] Bug 29145: (QA follow-up) Use objects instead of straight SQL queries Created attachment 145751 [details] [review] Bug 29145: (QA follow-up) Remove debugging line Thanks, Kyle! 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! This bug had status Passed QA , what happened? 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. DAvid, Kyle, can we rebase this ASAP please? Thanks! And please check the changes introduced by bug 31095. 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 :-) This has actually already passed qa.. it just needs a rebase to get it pushed. 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> 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> Created attachment 153806 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> 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. Created attachment 153808 [details] [review] Bug 29145: (QA follow-up) Add missing POD and don't return undef Created attachment 153809 [details] [review] Bug 29145: (QA follow-up) Fix typo Created attachment 153810 [details] [review] Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues Created attachment 153811 [details] [review] Bug 29145: (QA follow-up) Use objects instead of straight SQL queries Created attachment 153812 [details] [review] Bug 29145: (QA follow-up) Remove debugging line 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. Created attachment 153814 [details] [review] Bug 29145: Perltidy files and added code 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. 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> 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> Created attachment 156394 [details] [review] Bug 29145: Fix conditions for patron debarring overdues Signed-off-by: Michaela <michaela.sieber@kit.edu> 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. Created attachment 156396 [details] [review] Bug 29145: (QA follow-up) Add missing POD and don't return undef Created attachment 156397 [details] [review] Bug 29145: (QA follow-up) Fix typo Created attachment 156398 [details] [review] Bug 29145: (QA follow-up) Rename has_debarring_overdues to has_restricting_overdues Created attachment 156399 [details] [review] Bug 29145: (QA follow-up) Use objects instead of straight SQL queries Created attachment 156400 [details] [review] Bug 29145: (QA follow-up) Remove debugging line 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. Created attachment 156402 [details] [review] Bug 29145: Perltidy files and added code 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 Created attachment 156404 [details] [review] Bug 29145: (QA follow-up) Add +x to atomicupdate file Conflict was only in the test file, caused by bug 32496. Tests pass now :) Enhancement not pushed to 23.05.x |