Bug 14784 - Missing checkin message for debarred patrons when issuing rules 'fine days = 0'
Summary: Missing checkin message for debarred patrons when issuing rules 'fine days = 0'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor with 1 vote (vote)
Assignee: Alexis Ripetti
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-03 20:05 UTC by Genevieve Plantin
Modified: 2023-06-08 22:32 UTC (History)
33 users (show)

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


Attachments
CAM 2267 - Bug 14784 : Fix checkin message for debarred patrons (1.80 KB, patch)
2015-09-03 20:42 UTC, Genevieve Plantin
Details | Diff | Splinter Review
Bug 14784 : Fix checkin message for debarred patrons (1.79 KB, patch)
2016-03-17 18:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14784 : Fix checkin message for debarred patrons (1.84 KB, patch)
2016-03-17 18:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 14784 : Fix checkin message for debarred patrons (2.51 KB, patch)
2016-03-21 06:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug14784 - Fix checkin message for debarred patrons (2.13 KB, patch)
2016-12-15 17:33 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Attachment to Bug 14784 - Missing checkin message for debarred patrons when issuing rules 'fine days = 0' (3.35 KB, patch)
2017-09-12 19:00 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 14784 - Fix checkin message for debarred patrons (3.41 KB, patch)
2017-10-14 20:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14784 - Fix checkin message for debarred patrons (4.36 KB, patch)
2017-11-22 20:46 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 14784 - Fix checkin message for debarred patrons (4.45 KB, patch)
2017-11-22 21:11 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 14784 - Fix checkin message for debarred patrons (4.52 KB, patch)
2018-08-10 14:38 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 14784 - Fix checkin message for debarred patrons (4.65 KB, patch)
2018-10-02 15:26 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for debarred patrons (4.07 KB, patch)
2020-07-13 04:11 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 14784: Add tests (2.53 KB, patch)
2020-07-13 04:11 UTC, Maryse Simard
Details | Diff | Splinter Review
restriction (47.19 KB, image/png)
2020-07-29 12:04 UTC, Kelly McElligott
Details
Bug 14784: Add tests (7.24 KB, patch)
2020-09-21 19:12 UTC, Alexis Ripetti
Details | Diff | Splinter Review
Bug 14784: Add tests (7.30 KB, patch)
2020-09-21 21:07 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for debarred patrons (4.14 KB, patch)
2020-09-25 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14784: Add tests (7.37 KB, patch)
2020-09-25 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for debarred patrons (4.49 KB, patch)
2020-10-30 15:56 UTC, Alexis Ripetti
Details | Diff | Splinter Review
Bug 14784: Add tests (6.66 KB, patch)
2020-10-30 15:56 UTC, Alexis Ripetti
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for restricted patrons (2.50 KB, patch)
2022-06-13 18:55 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for restricted patrons (2.55 KB, patch)
2022-06-18 22:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for restricted patrons (2.55 KB, patch)
2022-07-14 14:48 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 14784: (follow-up) Unit tests (2.57 KB, patch)
2022-07-14 14:48 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 14784: Fix checkin message for restricted patrons (2.60 KB, patch)
2023-01-13 20:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14784: (follow-up) Unit tests (2.62 KB, patch)
2023-01-13 20:13 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Genevieve Plantin 2015-09-03 20:05:21 UTC
Context : a patron was previously debarred and has returned a overdue book.

There should be a message saying : "Check in message - Reminder: Patron was earlier restricted until ..."

However, when the column finedays in the table issuingrules equals 0, no message is shown.
Comment 1 Genevieve Plantin 2015-09-03 20:42:52 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2015-12-23 20:44:28 UTC
Hi Genevieve

I am not quite sure how to test this, could you please add a test plan
Comment 3 Rémi Mayrand-Provencher 2016-02-03 20:04:05 UTC Comment hidden (obsolete)
Comment 4 Aleisha Amohia 2016-02-05 00:29:15 UTC
I don't get any kind of message after checking in the book at step 5/6.
Comment 5 Rémi Mayrand-Provencher 2016-02-18 15:37:30 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2016-03-17 18:49:59 UTC Comment hidden (obsolete)
Comment 7 Biblibre Sandboxes 2016-03-17 18:54:18 UTC
Patch tested with a sandbox, by barton <barton@bywatersolutions.com>
Comment 8 Biblibre Sandboxes 2016-03-17 18:54:42 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2016-03-21 06:48:04 UTC
Hi Genevieve, some things to look out for next time:

1) Please don't use tabs, but 4 spaces in your patches. The QA tools can help you to find such problems, instructions on how to install them are on the wiki:

 FAIL	C4/Circulation.pm
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 2100)
		forbidden pattern: tab char (line 2092)
		forbidden pattern: tab char (line 2098)
		forbidden pattern: tab char (line 2097)
		forbidden pattern: tab char (line 2094)

2) Please add line breaks to your patch descriptions as that makes them easier to read, see here: https://wiki.koha-community.org/wiki/Commit_messages

3) Please include the test plan in the commit message next time.

I will attach an amended patch in a moment :)
Comment 10 Katrin Fischer 2016-03-21 06:48:51 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-03-23 17:41:57 UTC
Sorry but the test plan described what I get against master.
It seems a bit weird to have a "show_something_on_the_interface" in a module.
Tests are missing anyhow.
Comment 12 Rémi Mayrand-Provencher 2016-04-19 18:11:58 UTC
I just tested on master too and it is true that my old test plan now works properly on master, was this fixed in another bug?
Comment 13 Katrin Fischer 2016-04-19 22:18:22 UTC
Hi Rémi, maybe check the history on the files in question? 
Closing this for now, please reopen if the problem remains.
Comment 14 Bouzid Fergani 2016-12-15 17:33:06 UTC Comment hidden (obsolete)
Comment 15 Baptiste 2017-02-16 15:13:48 UTC
Hi,
The scenario 2 leads me to a message, even without patching
Comment 16 Owen Leonard 2017-04-17 14:24:10 UTC
I don't think I am able to reproduce this bug. Either that or I misunderstand what the bug is. As far as I can tell I'm getting the correct checkin messages for restricted patrons.
Comment 17 Owen Leonard 2017-04-25 12:43:59 UTC
Marking this Failed QA until we get some feedback on the testing results.
Comment 18 Philippe Audet-Fortin 2017-05-17 13:00:38 UTC
I tested Bouzid scenarios and I was able to reproduce the problem so it seems to be a real issue. 
After applying the patch I was able to see the message as expected.
Comment 19 David Bourgault 2017-09-12 19:00:07 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2017-10-08 09:38:36 UTC
Hi David,
I've tried several things, but I think we are missing something in the test plan. That's what I've tried and I've always seen a message on check-in:

1) Created an manual indefinite restriction for the patron. 
2) Created a manual restriction with a date in 2 weeks. 


Question:
- You say checkout and checkin - I can only do so by overwriting the restriction temporarily. Is that what you do as well?
- You say fines by days should be 0. I was testing with a manual restriction, should this be a restriction of a certain type? If you want suspension, why then fines in days = 0?
Comment 21 David Bourgault 2017-10-12 19:49:25 UTC
Hi Katrin,

I can still reproduce the problem on my end... Maybe this more detailed test plan can help :

0) Create a circulation rules under Koha > Admin > Circulation and fine rules with the value "0" for "Suspension days" (this maps to finedays in the database)
1) Select a test patron with the corresponding patron category.
2) Checkout an item (with the item_type corresponding to the rule set in step #0)for this patron, setting a due date in the past.
3) Manually set an indefinite restriction on this user
4) Check-in the item.
5) There should be a warning reminding that the user is debarred/restricted, but it is not there.

If finedays is > 0, the reminder (or the new suspension) appears.
If the item is not overdue, the reminder appears.
If the item is overdue AND finedays is 0, the reminder does not appear.

To answer your questions :
A) You can restrict the user before or after checking out, this does not affect the result in my testing (but you will need to override the restriction).
B) You might manually restrict users for different reasons, regardless of your circulation rules. This bug is only about a missing warning, not a missing suspension.

The issue happens because of a "else if" which does not make sense if finedays is set to 0.
Comment 22 Katrin Fischer 2017-10-14 20:11:11 UTC
Created attachment 68132 [details] [review]
Bug 14784 - Fix checkin message for debarred patrons

Tested and bug still exists, however previous patch did not apply and relied on obsolete code.

Test plan:

Before

1) Select a user with active restrictions
2) Make sure finesday=0 for the user category
3) Checkout and return an item (not overdue) : a previous restriction reminder will appear
4) Checkout and return an overdue item : no previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

P.S.: The "finesday" setting is called "Suspension days" in the web interface, if you're searching
for it like I did...

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2017-10-14 20:12:18 UTC
Hi David, thx for the updated test plan. I reset my system since and I could see the bug and confirm the fix now.
Comment 24 Marcel de Rooy 2017-11-17 12:01:33 UTC
QA: Looking here now
Comment 25 Marcel de Rooy 2017-11-17 13:18:29 UTC
Changing status. Comment follows.
Comment 26 Marcel de Rooy 2017-11-17 13:33:53 UTC
QA Comment:
Thx David for your brave changes. Looks good, still asking some attention.

-        } elsif ( $issue->date_due and $patron->debarred ) {
+        if ( $new_debar == 0 and $patron->debarred ) {
Why do you remove the test on date_due (might be just theoretical)?
Since you change the program flow by changing an elsif into an if, the game may change here. 
The comment on top is no longer valid at least ("There is no overdue..."); seems ok for the rest.

Variable $is_a_reminder is no longer used.
# if borrower was already debarred but does not get an extra debarment
Your changes make this comment look bad.
Since _debar_user_on_return now seems to return new or extended debarment dates only, please adjust POD.
In AddReturn adjust varname accordingly (like newdebardate or so )?
You do not need $debardate and $new_debar now; $newdebardate would be enough to test.

See Comment11 too. We still need additional unit testing.
Comment 27 David Bourgault 2017-11-22 20:46:33 UTC Comment hidden (obsolete)
Comment 28 David Bourgault 2017-11-22 21:11:27 UTC Comment hidden (obsolete)
Comment 29 Fridolin Somers 2018-02-20 08:07:56 UTC
(In reply to David Bourgault from comment #28)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Is this patch signed off ?
Or does it need a second signing ?
Comment 30 Fridolin Somers 2018-02-20 08:17:07 UTC
It also corrects a small bug, when restriction is infinite, the date "31/12/9999" was displayed in return page.
This is because new code always checks for this infinite date and returns message "ForeverDebarred" instead of "PrevDebarred".
Comment 31 Katrin Fischer 2018-03-17 20:19:58 UTC
Hi Frido, maybe you can sign-off again? I think David made changes after my initial sign-off (would have been better in a little follow-up patch)
Comment 32 Charles Farmer 2018-08-10 14:38:50 UTC
Created attachment 77683 [details] [review]
Bug 14784 - Fix checkin message for debarred patrons

After thinking about it I changed the code's behavior to always display a reminder if the patron was previously debarred.
This makes sense to me (it's only a reminder) and it correctly stacks with other messages.

--

Test plan:
Before

1) Select a user with active restrictions
2) Make sure finesday=0 for the user category
3) Checkout and return an item (not overdue) : a previous restriction reminder will appear
4) Checkout and return an overdue item : no previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

P.S.: The "finesday" setting is called "Suspension days" in the web interface, if you're searching
for it like I did...

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Victor Grousset/tuxayo 2018-10-02 15:26:03 UTC
Created attachment 79826 [details] [review]
Bug 14784 - Fix checkin message for debarred patrons

After thinking about it I changed the code's behavior to always display a
reminder if the patron was previously debarred.
This makes sense to me (it's only a reminder) and it correctly stacks with
other messages.

--

Test plan:
Before

1) Select a user with active restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching
for it like I did...

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 34 Victor Grousset/tuxayo 2018-10-02 15:27:50 UTC
Signed off and updated test plan:
- add "(manual restriction works)"
- update text 'called "Suspension in days"' to match the UI

Thanks Frido for guiding me :)
Comment 35 Jonathan Druart 2018-10-03 10:54:07 UTC
(In reply to Jonathan Druart from comment #11)
> Tests are missing anyhow.

(In reply to Marcel de Rooy from comment #26)
> See Comment11 too. We still need additional unit testing.

And... still missing.
Comment 36 Fridolin Somers 2019-12-24 10:30:21 UTC
> Tests are missing anyhow
This is a very important code, if anyone is willing to provide tests, you are welcome :)
Comment 37 Katrin Fischer 2019-12-29 15:15:12 UTC
This makes changes to a core routine - AddReturn - so asking for tests is normal and they should be provided by the patch author ideally - anyone can, but it's not QA's job.
Comment 38 Maryse Simard 2020-07-13 04:11:02 UTC
Created attachment 106808 [details] [review]
Bug 14784: Fix checkin message for debarred patrons

After thinking about it I changed the code's behavior to always display a
reminder if the patron was previously debarred.
This makes sense to me (it's only a reminder) and it correctly stacks with
other messages.

--

Test plan:
Before

1) Select a user with active restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching
for it like I did...

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 39 Maryse Simard 2020-07-13 04:11:06 UTC
Created attachment 106809 [details] [review]
Bug 14784: Add tests

This patch adds tests for PrevDebarred message of AddReturn when finesdays = 0.

To test:
1. Apply patches
2. prove -v t/db_dependent/Circulation.t

When applying only this patch, one test should fail.
Comment 40 Maryse Simard 2020-07-13 04:11:24 UTC
Rebased the patch and added a commit for unit tests.

I was unsure which status to set this to, so I've set it back to needs signoff.
Comment 41 Katrin Fischer 2020-07-13 21:08:02 UTC
(In reply to Maryse Simard from comment #40)
> Rebased the patch and added a commit for unit tests.
> 
> I was unsure which status to set this to, so I've set it back to needs
> signoff.

If the rebase was not too hard you can switch off back to Signed off usually. Also added tests are ok as a QA follow-up :)
Comment 42 Maryse Simard 2020-07-14 14:36:55 UTC
(In reply to Katrin Fischer from comment #41)
> If the rebase was not too hard you can switch off back to Signed off
> usually. Also added tests are ok as a QA follow-up :)

Thank you for the reply! In that case, I'll set the status back to signed off.
Comment 43 Katrin Fischer 2020-07-26 14:52:03 UTC
I am sorry, but the tests are failing with the patch set applied:

t/db_dependent/Circulation.t .. 49/49 # Looks like you failed 1 test of 49.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/49 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 49 Failed: 1)
  Failed test:  29
  Non-zero exit status: 1
Files=1, Tests=49, 34 wallclock secs ( 0.10 usr  0.00 sys + 24.89 cusr  2.61 csys = 27.60 CPU)
Result: FAIL
Comment 44 Kelly McElligott 2020-07-29 12:04:25 UTC
Created attachment 107487 [details]
restriction
Comment 45 Alexis Ripetti 2020-09-21 19:12:00 UTC Comment hidden (obsolete)
Comment 46 Alexis Ripetti 2020-09-21 19:22:09 UTC
This patch adds tests for PrevDebarred message of AddReturn when finesdays = 0.
I also changed one thing that seemed weird to me in the C4/Circulation.pm file.

Test plan:
Before

1) Select a user with active restrictions
2) Make sure finesday=0 for the user category
3) Checkout and return an item (not overdue) : a previous restriction reminder will appear
4) Checkout and return an overdue item : no previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

And finally:
5) prove -v t/db_dependent/Circulation.t


All the tests should now pass.

Waiting for your comments.
Comment 47 Arthur Suzuki 2020-09-21 21:07:16 UTC
Created attachment 110514 [details] [review]
Bug 14784: Add tests

This patch adds tests for PrevDebarred message of AddReturn when finesdays = 0.

To test:
1. Apply patches
2. prove -v t/db_dependent/Circulation.t

When applying only this patch, one test should fail.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 48 Arthur Suzuki 2020-09-21 21:12:14 UTC
Test patch works as expected (there is one test which doesn't pass when test patch is applied alone).
However the test patch seems to change things in C4/Circulation as well,
is this due to a change in the db structure,
and shouldn't this change be in the main patch anyway?
Other than this comment, happy to signoff :)
Comment 49 Alexis Ripetti 2020-09-22 13:29:43 UTC
(In reply to Arthur Suzuki from comment #48)
> Test patch works as expected (there is one test which doesn't pass when test
> patch is applied alone).
> However the test patch seems to change things in C4/Circulation as well,
> is this due to a change in the db structure,
> and shouldn't this change be in the main patch anyway?
> Other than this comment, happy to signoff :)

The change in the C4/Circulation.pm file should have been applied on the main patch.
You previously said that a test should fail when patches are applied. But which test does fail for you and which one is supposed to fail? Because when I apply both patches every tests pass.
Comment 50 Kyle M Hall 2020-09-25 14:39:46 UTC
Created attachment 110763 [details] [review]
Bug 14784: Fix checkin message for debarred patrons

After thinking about it I changed the code's behavior to always display a
reminder if the patron was previously debarred.
This makes sense to me (it's only a reminder) and it correctly stacks with
other messages.

--

Test plan:
Before

1) Select a user with active restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching
for it like I did...

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Kyle M Hall 2020-09-25 14:40:04 UTC
Created attachment 110764 [details] [review]
Bug 14784: Add tests

This patch adds tests for PrevDebarred message of AddReturn when finesdays = 0.

To test:
1. Apply patches
2. prove -v t/db_dependent/Circulation.t

When applying only this patch, one test should fail.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Jonathan Druart 2020-09-28 13:58:09 UTC
1. The second patch "Add tests" adds a change to the module, is that expected?

2. You are modifying a test that is not (at first glance) directly related to your changes, why?

@@ -2094,7 +2094,7 @@ subtest 'AddReturn + CumulativeRestrictionPeriods' => sub {
     is( scalar(@$debarments), 1 );
     $expected_expiration = output_pref(
         {
-            dt => $now->clone->add( days => ( 5 - 1 ) * 2 + ( 10 - 1 ) * 2 ),
+            dt => $now->clone->add( days => ( 10 - 1 ) * 2 ),

3. 

         # there's no overdue on the item but borrower had been previously debarred
-        } elsif ( $issue->date_due and $patron->debarred ) {
+        if ( $patron->debarred ) {

Comment and code not kept in sync.

Please clarify.
Comment 53 Alexis Ripetti 2020-09-28 15:27:48 UTC
(In reply to Jonathan Druart from comment #52)
> 1. The second patch "Add tests" adds a change to the module, is that
> expected?
> 
> 2. You are modifying a test that is not (at first glance) directly related
> to your changes, why?
>
>3.


1. As I said in a previous comment, the change in the C4/Circulation.pm file should have been applied on the main patch.

2. I was waiting for an answer of Arthur Suzuki about the test who should fail.
I think that I corrected this test to pass but it may need to fail ? I am still waiting for an answer about the comment 49.

3. We can just modify the comment : # there's no overdue on the item but borrower is debarred
Comment 54 Arthur Suzuki 2020-10-28 23:10:52 UTC
Hi Alexis,
Sorry for the late reply...
After applying both patches I still get a failed tests:
<pre>
root@kohadevbox:koha(master)$ prove t/db_dependent/Circulation.t 
t/db_dependent/Circulation.t .. 27/50 
    #   Failed test 'AddReturn must have debarred the patron'
    #   at t/db_dependent/Circulation.t line 89.
    #          got: ''
    #     expected: '1'
    # AddReturn returned message $VAR1 = {
    #           'WasReturned' => 1
    #         };
    # Looks like you failed 1 test of 29.
t/db_dependent/Circulation.t .. 29/50 
#   Failed test 'AddReturn + suspension_chargeperiod'
#   at t/db_dependent/Circulation.t line 2397.
t/db_dependent/Circulation.t .. 48/50 # Looks like you failed 1 test of 50.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/50 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 50 Failed: 1)
  Failed test:  29
  Non-zero exit status: 1
Files=1, Tests=50, 22 wallclock secs ( 0.07 usr  0.01 sys + 17.66 cusr  1.62 csys = 19.36 CPU)
Result: FAIL
</pre>
Comment 55 Alexis Ripetti 2020-10-30 15:56:38 UTC
Created attachment 112724 [details] [review]
Bug 14784: Fix checkin message for debarred patrons

After thinking about it I changed the code's behavior to always display a
reminder if the patron was previously debarred.
This makes sense to me (it's only a reminder) and it correctly stacks with
other messages.

--

Test plan:
Before

1) Select a user with active restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

I've also tested with finesday=1 with "no overdue", "grace period overdue" and "long overdue" test
cases. The appropriate nessage appeared for each test.

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching
for it like I did...
Comment 56 Alexis Ripetti 2020-10-30 15:56:58 UTC
Created attachment 112725 [details] [review]
Bug 14784: Add tests

This patch adds tests for PrevDebarred message of AddReturn when finesdays = 0.

To test:
1. Apply patches
2. prove -v t/db_dependent/Circulation.t

When applying only this patch, one test should fail.
Comment 57 Alexis Ripetti 2020-10-30 17:45:55 UTC
So, I fixed the test that does not pass.
I also put the changes in the good patch.
When I apply both patches, I got this :

<pre>
not ok 8

    #   Failed test at t/db_dependent/Circulation.t line 2102.
    #          got: '2020-11-17'
    #     expected: '2020-11-25'
    # Looks like you failed 1 test of 8.
not ok 28 - AddReturn + CumulativeRestrictionPeriods

#   Failed test 'AddReturn + CumulativeRestrictionPeriods'
#   at t/db_dependent/Circulation.t line 2103.
</pre>

Is this the one who should fail ?

I also got this :

<pre>
not ok 14 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 0 (168h * 0.25u)

    #   Failed test 'Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 0 (168h * 0.25u)'
    #   at t/db_dependent/Circulation.t line 3554.
    #          got: '47.75'
    #     expected: '42'
    ok 15 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 0, for renewal (312h - 168h * 0.25u)
    not ok 16 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1 and closed Saturday (168h - 24h * 0.25u)

    #   Failed test 'Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1 and closed Saturday (168h - 24h * 0.25u)'
    #   at t/db_dependent/Circulation.t line 3568.
    #          got: '35.75'
    #     expected: '36'
    ok 17 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1 and closed Saturday, for renewal (312h - 168h - 24h * 0.25u
    not ok 18 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1 (168h - 0h * 0.25u

    #   Failed test 'Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1 (168h - 0h * 0.25u'
    #   at t/db_dependent/Circulation.t line 3582.
    #          got: '41.75'
    #     expected: '42'
    ok 19 - Hourly rental charge calculated correctly with rentalcharge_hourly_calendar = 1, for renewal (312h - 168h - 0h * 0.25u)
    # Looks like you failed 3 tests of 19.
not ok 42 - Incremented fee tests

#   Failed test 'Incremented fee tests'
#   at t/db_dependent/Circulation.t line 3592.
</pre>

These failed tests are also here when I do not apply both patches.
Do you get the same errors?
Comment 58 Koha Team University Lyon 3 2021-02-16 08:01:59 UTC
Hello,
does this one need new tests or is there just QA issues ?
Sonia
Comment 59 Arthur Suzuki 2021-02-17 01:17:09 UTC
Hi Sonia,
Just managed to get my Koha testing environment up again.
Unfortunately, after applying both tests patch and fix patchs, some tests are still failing.
Still a bit of work needed on that patch.
I see that assignee changed, is there someone working on that one?
King regards,
Comment 60 David Nind 2021-04-10 22:28:41 UTC
Patch still applies!

Failed test still though:
- not ok 30 - AddReturn + CumulativeRestrictionPeriods
Comment 61 The Minh Luong 2022-02-16 17:28:35 UTC
I have tested before applying the patch and everything seems to work WITHOUT it. The tests seems to work fine without the patch, but one fail when applying the patch. 

Here is what I did: 

Before applying the patch:
1) Select a user with active restrictions (manual restriction works) and note his category.
2) Make sure finesday=0 for the user's category noted in step 1. 
3) Checkout and return an item (not overdue).
   A previous restriction reminder will appear (Message in yellow box)
4) Checkout and return an overdue item (change the date at checkout)
   A previous restriction reminder will also appear (Message in yellow box)
5) Run prove -l t/db_dependent/Circulation.t (ALL tests should pass)

After applying patch:
Same steps from 1 to 5 , but at step 5, one test fails (AddReturn + CumulativeRestrictionPeriods).
Comment 62 Florian 2022-04-28 15:44:53 UTC
I concur with The Minh Luong, if I run the test plan without patching I don't have any issues, but after patching, step 5 (running prove -l t/db_dependent/Circulation.t) gets me not one, but 31 failed tests.
Comment 63 Katrin Fischer 2022-04-28 15:46:48 UTC
Changing status to indicate more clearly that these patches haven't been pushed.
Comment 64 Shi Yao Wang 2022-06-13 18:55:22 UTC
Created attachment 136040 [details] [review]
Bug 14784: Fix checkin message for restricted patrons

Test plan:
Before

1) Select a user with active indefinite or definite restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching for it like I did...
Comment 65 Shi Yao Wang 2022-06-13 19:09:45 UTC
Here is a patch that fixes the missing checkin message without changing code in _calculate_new_debar_dt or _debar_user_on_return. So it shouldn't affect other functionality too much. It shouldn't cause additional test fails.

Only little thing is that it duplicates the code because it has the same block of code (inside elsif).

What do you think?
Comment 66 David Nind 2022-06-18 22:37:34 UTC
Created attachment 136327 [details] [review]
Bug 14784: Fix checkin message for restricted patrons

Test plan:
Before

1) Select a user with active indefinite or definite restrictions (manual restriction works)
2) Make sure finesday=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at cehckout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

[1] The "finesday" setting is called "Suspension in days" in the web interface, if you're searching for it like I did...

Signed-off-by: David Nind <david@davidnind.com>
Comment 67 David Nind 2022-06-18 22:39:47 UTC
My testing notes (using koha-testing-docker)

Step 1: To add a restriction:
        . Select and edit a patron 
        . Scroll down to the 'Patron restrictions' section
        . Add a manual restriction

Step 2: In KTD "Suspension in days" is already set to 0 for the current default rules.

Steps 3 and 4: Use the "Override restriction temporarily" when checking out an item.

Tests should continue to pass: prove -v t/db_dependent/Circulation.t

See also comment #21.
Comment 68 Jonathan Druart 2022-07-06 09:05:35 UTC
Did we lose the tests?
Comment 69 Shi Yao Wang 2022-07-13 14:18:08 UTC
The current patch is quite different than the previous one so I marked the previous tests as obsolete.
Comment 70 Victor Grousset/tuxayo 2022-07-13 23:16:15 UTC
> The current patch is quite different than the previous one so I marked the previous tests as obsolete.

Makes sense.
Tests on the new changes are still needed for this to be ready for QA. These are not trivial changes and AddReturn() in Circulation.pm is like super critical code ^^"
Comment 71 Shi Yao Wang 2022-07-14 14:48:21 UTC
Created attachment 137727 [details] [review]
Bug 14784: Fix checkin message for restricted patrons

Test plan:
Before

1) Select a user with active indefinite or definite restrictions (manual restriction works)
2) Make sure finedays=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at checkout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

[1] The "finedays" setting is called "Suspension in days" in the web interface, if you're searching for it like I did...

Signed-off-by: David Nind <david@davidnind.com>
Comment 72 Shi Yao Wang 2022-07-14 14:48:27 UTC
Created attachment 137728 [details] [review]
Bug 14784: (follow-up) Unit tests
Comment 73 Shi Yao Wang 2022-07-14 14:51:20 UTC
Added tests and fixed some typos in main patch commit message.

Putting back to Signed Off.
Comment 74 Martin Renvoize 2022-07-15 14:50:05 UTC
Comment on attachment 137727 [details] [review]
Bug 14784: Fix checkin message for restricted patrons

Review of attachment 137727 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +2296,4 @@
>          if ( $issue and $issue->is_overdue($return_date) ) {
>          # fix fine days
>              my ($debardate,$reminder) = _debar_user_on_return( $patron_unblessed, $item->unblessed, dt_from_string($issue->date_due), $return_date );
> +            if ($debardate and $debardate ne "9999-12-31") {

There really shouldn't be any 9999-12-31 dates left for debardate.. we tried to clean them all up back with the 3.15.00.029 database update.

@@ +2302,5 @@
> +                } else {
> +                    $messages->{'Debarred'} = $debardate;
> +                }
> +            } elsif ($patron->debarred) {
> +                if ( $patron->debarred eq "9999-12-31") {

As above
Comment 75 Katrin Fischer 2022-07-15 14:56:13 UTC
Not sure about the date, actually:

If you create an unrestricted debarment you get:

borrowers_debarments.expiration = NULL
borrowers.debarred = 9999-12-31
Comment 76 Blou 2022-11-24 16:14:43 UTC
So, Signed Off or Failed QA ?
Seems the 9999 is still a valid test, per Cate.
Comment 77 Katrin Fischer 2022-12-04 12:10:19 UTC
(In reply to Katrin Fischer from comment #75)
> Not sure about the date, actually:
> 
> If you create an unrestricted debarment you get:
> 
> borrowers_debarments.expiration = NULL
> borrowers.debarred = 9999-12-31

Looking at the code, it checked borrowers.debarred:

+            } elsif ($patron->debarred) {
+                if ( $patron->debarred eq "9999-12-31") {

So this would be the correct checked for an unlimited restriction indeed. 

There is some history there as in that debarred and debarredcomment where kept around when borrowers_debarments were added for backwards compatibility. It should always contain the longest restriction at that moment with more than one restriction. And it's also still used in patron import for adding a restriction.

And in borrowers.debarred we can't use NULL to indicate an unlimited debarment... as that would mean NO restriction. So the 'magic' date is still used there.

Setting back to Signed off.
Comment 78 Katrin Fischer 2022-12-04 12:12:02 UTC
@Kyle: could you maybe weigh in here? IIRC you were the one to write a part of this code at least?
Comment 79 Kyle M Hall 2023-01-13 20:09:45 UTC
(In reply to Katrin Fischer from comment #78)
> @Kyle: could you maybe weigh in here? IIRC you were the one to write a part
> of this code at least?

Yes, considering Koha::Paron::is_debarred checks that calculated field, I think it's passible, but it would be best practice to search for patron restrictions that have no time limit. We don't have classes for borrower_debarments at the moment, so I think that would be a bit onerous to require.
Comment 80 Kyle M Hall 2023-01-13 20:13:06 UTC
Created attachment 145320 [details] [review]
Bug 14784: Fix checkin message for restricted patrons

Test plan:
Before

1) Select a user with active indefinite or definite restrictions (manual restriction works)
2) Make sure finedays=0 for the user category. See [1]
3) Checkout and return an item (not overdue)
   A previous restriction reminder will appear
4) Checkout and return an overdue item (change the date at checkout)
   No previous restriction reminder will appear

After applying patch:
Same steps, but a reminder should appear for step 4)

[1] The "finedays" setting is called "Suspension in days" in the web interface, if you're searching for it like I did...

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 81 Kyle M Hall 2023-01-13 20:13:20 UTC
Created attachment 145321 [details] [review]
Bug 14784: (follow-up) Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 82 Martin Renvoize 2023-01-17 09:40:25 UTC
Nice work everyone!

Pushed to 23.05.x for the next release
Comment 83 Matt Blenkinsop 2023-01-17 15:11:27 UTC
Nice work everyone!

Pushed to stable for 22.11.x