Bug 24075 - Backdating a return to the exact due date and time results in the fine not being refunded
Summary: Backdating a return to the exact due date and time results in the fine not be...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 14591
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-20 20:06 UTC by Donna
Modified: 2019-11-27 10:31 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (1.89 KB, patch)
2019-11-20 20:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (2.14 KB, patch)
2019-11-20 20:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (2.14 KB, patch)
2019-11-21 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Unit tests (2.33 KB, patch)
2019-11-21 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Unit tests (2.39 KB, patch)
2019-11-21 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (2.15 KB, patch)
2019-11-21 19:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Unit tests (2.39 KB, patch)
2019-11-21 19:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Alternative 1 (2.67 KB, patch)
2019-11-22 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24075: Alternative 2 (2.43 KB, patch)
2019-11-22 12:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (3.25 KB, patch)
2019-11-22 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Unit tests (2.39 KB, patch)
2019-11-22 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded (3.29 KB, patch)
2019-11-22 14:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24075: Unit tests (2.42 KB, patch)
2019-11-22 14:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Donna 2019-11-20 20:06:09 UTC
When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Confirmed as a problem in master by kidclamp and khall.
Comment 1 Kyle M Hall 2019-11-20 20:19:48 UTC
Created attachment 95625 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.
Comment 2 Kyle M Hall 2019-11-20 20:21:56 UTC
Created attachment 95626 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Test Plan:
1) Back date a checkout so it is overdue
2) Run fines.pl to generate the fine
3) Return the item, backdating to the same date/time is was due
4) Note the fine was not removed
5) Apply this patch
6) Repeat steps 1-3
7) Fine should be zeroed out now!
Comment 3 Kyle M Hall 2019-11-21 11:37:05 UTC
Created attachment 95654 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Test Plan:
1) Back date a checkout so it is overdue
2) Run fines.pl to generate the fine
3) Return the item, backdating to the same date/time is was due
4) Note the fine was not removed
5) Apply this patch
6) Repeat steps 1-3
7) Fine should be zeroed out now!
Comment 4 Kyle M Hall 2019-11-21 11:37:14 UTC
Created attachment 95655 [details] [review]
Bug 24075: Unit tests
Comment 5 Kyle M Hall 2019-11-21 11:39:28 UTC
Created attachment 95656 [details] [review]
Bug 24075: Unit tests
Comment 6 Martin Renvoize 2019-11-21 16:58:13 UTC
Comment on attachment 95654 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

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

::: C4/Circulation.pm
@@ +1966,4 @@
>                  MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy );
>              };
>              unless ( $@ ) {
> +                if ( $return_date_is_forced || ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue && !$item->itemlost ) ) {

This feels somewhat wrong.. I'm sure Katrin would say "But we don't ever want to recalculate fines if we have 'CalculateFinesOnReturn' turned off"

So.. could this perhaps be:

`if (  C4::Context->preference('CalculateFinesOnReturn') && ($return_date_is_forced || $is_overdue) && !$item->itemlost ) {`

Instead?
Comment 7 Kyle M Hall 2019-11-21 19:05:16 UTC
I like it! Updated patch incoming!

(In reply to Martin Renvoize from comment #6)
> Comment on attachment 95654 [details] [review] [review]
> Bug 24075: Backdating a return to the exact due date and time results in the
> fine not being refunded
> 
> Review of attachment 95654 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Circulation.pm
> @@ +1966,4 @@
> >                  MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy );
> >              };
> >              unless ( $@ ) {
> > +                if ( $return_date_is_forced || ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue && !$item->itemlost ) ) {
> 
> This feels somewhat wrong.. I'm sure Katrin would say "But we don't ever
> want to recalculate fines if we have 'CalculateFinesOnReturn' turned off"
> 
> So.. could this perhaps be:
> 
> `if (  C4::Context->preference('CalculateFinesOnReturn') &&
> ($return_date_is_forced || $is_overdue) && !$item->itemlost ) {`
> 
> Instead?
Comment 8 Kyle M Hall 2019-11-21 19:06:42 UTC
Created attachment 95706 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Test Plan:
1) Back date a checkout so it is overdue
2) Run fines.pl to generate the fine
3) Return the item, backdating to the same date/time is was due
4) Note the fine was not removed
5) Apply this patch
6) Repeat steps 1-3
7) Fine should be zeroed out now!
Comment 9 Kyle M Hall 2019-11-21 19:06:51 UTC
Created attachment 95707 [details] [review]
Bug 24075: Unit tests
Comment 10 Jonathan Druart 2019-11-22 09:34:47 UTC
I would say it has been caused by
  commit 467c24cb9da02deb00bf49333351a0843cdeda10
  Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines
Comment 11 Martin Renvoize 2019-11-22 12:22:34 UTC
Created attachment 95719 [details] [review]
Bug 24075: Alternative 1
Comment 12 Martin Renvoize 2019-11-22 12:31:41 UTC
Created attachment 95720 [details] [review]
Bug 24075: Alternative 2
Comment 13 Martin Renvoize 2019-11-22 12:32:23 UTC
Lets golf a little.. I've uploaded a couple of alternatives.. let me know which of the three options seems clearest
Comment 14 Kyle M Hall 2019-11-22 13:59:04 UTC
Created attachment 95723 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Test Plan:
1) Back date a checkout so it is overdue
2) Run fines.pl to generate the fine
3) Return the item, backdating to the same date/time is was due
4) Note the fine was not removed
5) Apply this patch
6) Repeat steps 1-3
7) Fine should be zeroed out now!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2019-11-22 13:59:14 UTC
Created attachment 95724 [details] [review]
Bug 24075: Unit tests
Comment 16 Tomás Cohen Arazi 2019-11-22 14:33:30 UTC
Tests pass:

prove t/db_dependent/Circulation/CalcFine.t t/db_dependent/Fines.t t/db_dependent/Circulation.t

Behavior is the described one. Passing!
Comment 17 Tomás Cohen Arazi 2019-11-22 14:34:17 UTC
Created attachment 95727 [details] [review]
Bug 24075: Backdating a return to the exact due date and time results in the fine not being refunded

When you use the date picker or book drop mode and check in an item that is due on that date, a fine is assessed and not refunded.

For example: item 12345 is due on 11/19/2109 23:59.  On 11/20/2019 I check the item in using date picker/book drop setting the check in date to 11/19/2019 23:59, the patron is charged a fine, and the fine is not cleared as would be expected, since the item is being checked in before it is overdue.

Test Plan:
1) Back date a checkout so it is overdue
2) Run fines.pl to generate the fine
3) Return the item, backdating to the same date/time is was due
4) Note the fine was not removed
5) Apply this patch
6) Repeat steps 1-3
7) Fine should be zeroed out now!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2019-11-22 14:34:21 UTC
Created attachment 95728 [details] [review]
Bug 24075: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Martin Renvoize 2019-11-22 15:08:57 UTC
Nice work!

Pushed to master for 19.11.00
Comment 20 Martha Fuerst 2019-11-26 20:30:22 UTC
Will this be backdated to 19.05?

Thanks!

-Marti Fuerst
Comment 21 Fridolin SOMERS 2019-11-27 10:31:29 UTC
Pushed to 19.05.x for 19.05.05