Bug 32696 - Recalls can inadvertently extend the due date
Summary: Recalls can inadvertently extend the due date
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Janusz Kaczmarek
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-20 23:08 UTC by Eric Phetteplace
Modified: 2024-11-03 23:58 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.04,23.11.10
Circulation function:


Attachments
Bug 32696: Recalls can inadvertently extend the due date (3.74 KB, patch)
2024-07-09 10:08 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 32696: Unit tests (4.86 KB, patch)
2024-07-09 10:08 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 32696: Recalls can inadvertently extend the due date (3.79 KB, patch)
2024-07-10 18:46 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 32696: Unit tests (4.91 KB, patch)
2024-07-10 18:46 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 32696: Recalls can inadvertently extend the due date (3.89 KB, patch)
2024-08-16 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32696: Unit tests (4.97 KB, patch)
2024-08-16 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Phetteplace 2023-01-20 23:08:28 UTC
If an item is due sooner than the recall due date interval then placing a recall on it can inadvertently extend the book's due date, possibly causing the recalling patron to receive the item later than they would otherwise.

To recreate the issue:

- turn on recalls with UseRecalls and scaffold out the minimal circulation rules needed to use the feature
- set the recall due date interval to 7 days or any other arbitrary period
- check out an item to one patron & set the due date to be before the recall due date interval will have elapsed (e.g. the next day)
- as a second patron, place a recall on the item in question

Actual behavior: the item's due date is extended to the current date plus the recalls due date interval.

Desired behavior: the item's due date should stay the same so the recalling patron doesn't face an additional delay in obtaining it. The logic is that the new due date should be either the existing due date or the current date plus the recall due date interval, whichever is sooner.
Comment 1 Kelly McElligott 2023-01-24 14:13:48 UTC
I am changing this from an enhancement bug to a minor bug, as this is an issue with the due dates of recalls.
Comment 2 Janusz Kaczmarek 2024-07-08 10:20:01 UTC
The issue with adjusting the checkout due date when placing recall is even more complicated and weird.  Consider the following scenario:

1. the issue is already overdue when placing the recall and has fine accruing;
2. adjusting due date, as it is now, stop accruing (since there is no longer overdue - cf. fines.pl);
3. when in turn, for such a case, a recall overdues, the previous fine will be overwritten by a new sum calculated from recall overdue fines.

But, if we didn't update the checkout due date, the new fine would be calculated as a product of total overdue days and recall overdue fine.  In case when the recall overdue fine were different (like e.g. 5 x the regular overdue fine), the newly calculated fine would be surprisingly high.

This leads to the conclusion that the issue is not as simple.  The checkout due date should definitely not be moved forward, but the method of fine calculation should probably be modified.  This may mean that we should record the recall due date separately (in the recalls table?).
Comment 3 Janusz Kaczmarek 2024-07-09 10:08:12 UTC
Created attachment 168640 [details] [review]
Bug 32696: Recalls can inadvertently extend the due date

If an item is due sooner than the recall due date interval then placing
a recall on it can inadvertently extend the book's due date, possibly
causing the recalling patron to receive the item later than they would
otherwise.

Test plan:
=========

1. Turn on recalls with UseRecalls.
2. In circulation rules, set the recall due date interval to 7 days
   or any other arbitrary period.
3. Check out an item to one patron & set the due date to be before the
   recall due date interval will have elapsed (e.g. the next day)
4. As a second patron, place a recall on the item in question.
5. See that the item's due date is extended to the current date plus
   the recalls due date interval.
6. Apply the patch, restart_all;
7. Repeat steps 3. and 4. See that the due date has not been extended.

BTW, the calculation of $due_interval has been changed, because with
the current code and empty 'Recall due date interval' $due_interval
is undefined, despite the intention of the author of the code.
(after calling get_effective_rule $recall_due_date_interval is defined, but
$recall_due_date_interval->rule_value is undefined;  the patron gets
a message: '... return the item within days, by ...' - no days count).

Sponsored-by: Ignatianum University in Cracow
Comment 4 Janusz Kaczmarek 2024-07-09 10:08:14 UTC
Created attachment 168641 [details] [review]
Bug 32696: Unit tests

Added unit tests.
Comment 5 Roman Dolny 2024-07-10 18:46:11 UTC
Created attachment 168764 [details] [review]
Bug 32696: Recalls can inadvertently extend the due date

If an item is due sooner than the recall due date interval then placing
a recall on it can inadvertently extend the book's due date, possibly
causing the recalling patron to receive the item later than they would
otherwise.

Test plan:
=========

1. Turn on recalls with UseRecalls.
2. In circulation rules, set the recall due date interval to 7 days
   or any other arbitrary period.
3. Check out an item to one patron & set the due date to be before the
   recall due date interval will have elapsed (e.g. the next day)
4. As a second patron, place a recall on the item in question.
5. See that the item's due date is extended to the current date plus
   the recalls due date interval.
6. Apply the patch, restart_all;
7. Repeat steps 3. and 4. See that the due date has not been extended.

BTW, the calculation of $due_interval has been changed, because with
the current code and empty 'Recall due date interval' $due_interval
is undefined, despite the intention of the author of the code.
(after calling get_effective_rule $recall_due_date_interval is defined, but
$recall_due_date_interval->rule_value is undefined;  the patron gets
a message: '... return the item within days, by ...' - no days count).

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 6 Roman Dolny 2024-07-10 18:46:14 UTC
Created attachment 168765 [details] [review]
Bug 32696: Unit tests

Added unit tests.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 7 Marcel de Rooy 2024-08-16 08:43:08 UTC
Created attachment 170438 [details] [review]
Bug 32696: Recalls can inadvertently extend the due date

If an item is due sooner than the recall due date interval then placing
a recall on it can inadvertently extend the book's due date, possibly
causing the recalling patron to receive the item later than they would
otherwise.

Test plan:
=========

1. Turn on recalls with UseRecalls.
2. In circulation rules, set the recall due date interval to 7 days
   or any other arbitrary period.
3. Check out an item to one patron & set the due date to be before the
   recall due date interval will have elapsed (e.g. the next day)
4. As a second patron, place a recall on the item in question.
5. See that the item's due date is extended to the current date plus
   the recalls due date interval.
6. Apply the patch, restart_all;
7. Repeat steps 3. and 4. See that the due date has not been extended.

BTW, the calculation of $due_interval has been changed, because with
the current code and empty 'Recall due date interval' $due_interval
is undefined, despite the intention of the author of the code.
(after calling get_effective_rule $recall_due_date_interval is defined, but
$recall_due_date_interval->rule_value is undefined;  the patron gets
a message: '... return the item within days, by ...' - no days count).

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2024-08-16 08:43:11 UTC
Created attachment 170439 [details] [review]
Bug 32696: Unit tests

Added unit tests.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Katrin Fischer 2024-08-19 15:09:11 UTC
Makes sense to me.
Comment 10 Katrin Fischer 2024-08-19 15:30:58 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 11 Lucas Gass (lukeg) 2024-09-12 21:59:22 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 12 Fridolin Somers 2024-10-21 12:24:56 UTC
Pushed to 23.11.x for 23.11.10
Comment 13 Wainui Witika-Park 2024-11-03 23:58:31 UTC
Not backporting to 23.05.x unless requested