Bug 22877 - Returning a lost item not marked as returned can generate additional overdue fines
Summary: Returning a lost item not marked as returned can generate additional overdue ...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
: 20769 22727 22902 (view as bug list)
Depends on: 21206
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-09 12:32 UTC by Nick Clemens
Modified: 2019-07-08 16:25 UTC (History)
11 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


Attachments
Bug 22877: Unit test to highlight problem (3.60 KB, patch)
2019-05-09 12:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.03 KB, patch)
2019-05-09 14:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Unit test to highlight problem (3.60 KB, patch)
2019-05-09 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.03 KB, patch)
2019-05-09 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (follow-up) Fix test plan count (811 bytes, patch)
2019-05-09 15:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Unit test to highlight problem (3.67 KB, patch)
2019-05-09 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.10 KB, patch)
2019-05-09 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (follow-up) Fix test plan count (876 bytes, patch)
2019-05-09 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (5.64 KB, patch)
2019-05-09 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (6.07 KB, patch)
2019-05-09 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (6.23 KB, patch)
2019-05-10 08:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Unit test to highlight problem (3.67 KB, patch)
2019-05-16 09:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.10 KB, patch)
2019-05-16 09:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (follow-up) Fix test plan count (876 bytes, patch)
2019-05-16 09:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (6.26 KB, patch)
2019-05-16 09:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22877: Unit test to highlight problem (3.71 KB, patch)
2019-05-16 16:20 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.14 KB, patch)
2019-05-16 16:20 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22877: (follow-up) Fix test plan count (921 bytes, patch)
2019-05-16 16:20 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (6.31 KB, patch)
2019-05-16 16:20 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22877: Unit test to highlight problem (3.81 KB, patch)
2019-05-31 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22877: Prevent double overdue processing for lost and returned (1.24 KB, patch)
2019-05-31 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22877: (follow-up) Fix test plan count (1019 bytes, patch)
2019-05-31 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22877: (QA follow-up) Clarify intent of tests and remove one. (6.41 KB, patch)
2019-05-31 06:28 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 Nick Clemens 2019-05-09 12:32:05 UTC
To recreate:
1 - Issue an item to a patron with a backdated due date
2 - Run fines.pl - note you generate a fine for the patron
3 - Run longoverdue.pl - make sure MarkLostItemsAsReturned is turned off and to charge
4 - Confirm the patron has a lost fee and an overdue
5 - Return the item
6 - Patron now has 2 overdue fines and a lost fine?
Comment 1 Nick Clemens 2019-05-09 12:33:09 UTC
Created attachment 89505 [details] [review]
Bug 22877: Unit test to highlight problem
Comment 2 Martin Renvoize 2019-05-09 14:30:51 UTC
I presume 'CalculateFinesOnReturn' comes into play here too?
Comment 3 Martin Renvoize 2019-05-09 14:31:30 UTC
(In reply to Martin Renvoize from comment #2)
> I presume 'CalculateFinesOnReturn' comes into play here too?

Sorry.. it's right there in the test as enabled at the top.
Comment 4 Martin Renvoize 2019-05-09 14:54:41 UTC
Created attachment 89507 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned
Comment 5 Martin Renvoize 2019-05-09 15:01:36 UTC
Created attachment 89508 [details] [review]
Bug 22877: Unit test to highlight problem
Comment 6 Martin Renvoize 2019-05-09 15:01:38 UTC
Created attachment 89509 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned
Comment 7 Martin Renvoize 2019-05-09 15:01:41 UTC
Created attachment 89510 [details] [review]
Bug 22877: (follow-up) Fix test plan count
Comment 8 Martin Renvoize 2019-05-09 15:36:31 UTC
Created attachment 89511 [details] [review]
Bug 22877: Unit test to highlight problem

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2019-05-09 15:36:34 UTC
Created attachment 89512 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-05-09 15:36:37 UTC
Created attachment 89513 [details] [review]
Bug 22877: (follow-up) Fix test plan count

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-05-09 15:36:40 UTC
Created attachment 89514 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

The intent of these tests as a set were not entirely clear. This patch
clarifies the intent for future developers and removes a superflous one.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-05-09 15:39:28 UTC
Test plan.. 
1. Recreate the issue as in the first comment
2. Apply the first patch and watch the test fail
3. Apply patches 2 and 3 and watch the test pass
4. Fail to recreate the issue as in the first comment
5. Apply the last patch and re-run the tests and watch them still pass
Comment 13 Jonathan Druart 2019-05-09 15:59:45 UTC
(In reply to Martin Renvoize from comment #11)
> Created attachment 89514 [details] [review] [review]
> Bug 22877: (QA follow-up) Clarify intent of tests and remove one.
> 
> The intent of these tests as a set were not entirely clear. This patch
> clarifies the intent for future developers and removes a superflous one.

Can you explain a bit more?
Comment 14 Martin Renvoize 2019-05-09 16:07:13 UTC
Created attachment 89516 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

The intent of these tests as a set were not entirely clear. This patch
clarifies the intent for future developers and removes a superflous one.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Jonathan Druart 2019-05-09 16:23:05 UTC
Wrong calls in test were caused by (just for ref):
  commit 004609e46814791fac540e1c8bfb21f647758143
  Bug 14591: Update unit tests
Comment 16 Lisette Scheer 2019-05-09 20:23:27 UTC
*** Bug 20769 has been marked as a duplicate of this bug. ***
Comment 17 Martin Renvoize 2019-05-10 08:37:45 UTC
Created attachment 89527 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

This patch embelishes the existing comments slightly and removes a test
which had a FIXME, but was in fact just a duplicate of the test directly
above it.  I beleive it became a duplicate after the subroutine signature
changed at some point recently.  I also move the bug 22877 specific test
into a subtest.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Jonathan Druart 2019-05-13 14:33:42 UTC
(In reply to Martin Renvoize from comment #17)
> Created attachment 89527 [details] [review] [review]
> Bug 22877: (QA follow-up) Clarify intent of tests and remove one.
> 
> This patch embelishes the existing comments slightly and removes a test
> which had a FIXME, but was in fact just a duplicate of the test directly
> above it.  I beleive it became a duplicate after the subroutine signature
> changed at some point recently.  I also move the bug 22877 specific test
> into a subtest.

The tests existed before the change of the signature (bcfa31a8e37f88ab07c1e7ed954ab57684d0dcfa).

The last 3 tests were:

+    # specify return date 5 days later => overdue
+    AddIssue( $patron->unblessed, $item->{barcode}, $ten_days_ago ); # date due was 10d ago
+    AddReturn( $item->{barcode}, $library->{branchcode}, undef, undef, $five_days_ago );
+    is( int($patron->account->balance()), 5, 'AddReturn: pass return_date => overdue' );
+    Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete;
 
+    # specify dropbox date 5 days before => no overdue
+    AddIssue( $patron->unblessed, $item->{barcode}, $five_days_ago ); # date due was 5d ago
+    AddReturn( $item->{barcode}, $library->{branchcode}, undef, 1, undef, $ten_days_ago );
+    is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue' );
+    Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete;
 
+    # specify dropbox date 5 days later => overdue, or... not
+    AddIssue( $patron->unblessed, $item->{barcode}, $ten_days_ago ); # date due was 10d ago
+    AddReturn( $item->{barcode}, $library->{branchcode}, undef, 1, undef, $five_days_ago );
+    is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue in dropbox mode' ); # FIXME? This is weird, the FU fine is created ( _CalculateAndUpdateFine > C4::Overdues::UpdateFine ) then remove later (in _FixOverduesOnReturn). Looks like it is a feature
+    Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete;

There are all completely different, in short:
- due 10d ago, checked in 5d ago => overdue
- due 5d ago, checked in 10d ago => no overdue
- due 10d ago, checked in 5d ago (using the dropbox feature) => FIXME

Would be good to see if the FIXME is still needed or not, I'd no as we removed the dropbox flag.
Comment 19 Jonathan Druart 2019-05-13 14:35:45 UTC
(In reply to Jonathan Druart from comment #18)
> Would be good to see if the FIXME is still needed or not, I'd no as we
> removed the dropbox flag.

Looks ok at first glance.
Comment 20 Liz Rea 2019-05-15 16:28:40 UTC
Hi, 

I don't think the tests are working right :

t/db_dependent/Circulation.t (Wstat: 256 Tests: 130 Failed: 1)
  Failed test:  117
  Non-zero exit status: 1

That's the one you added yeah?

Functionally, it does seem to fix the problem. Happy to retest when you think the tests are fixed up.

Cheers,
Liz
Comment 21 Martin Renvoize 2019-05-16 09:12:53 UTC
Created attachment 89818 [details] [review]
Bug 22877: Unit test to highlight problem

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-05-16 09:12:56 UTC
Created attachment 89819 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2019-05-16 09:12:59 UTC
Created attachment 89820 [details] [review]
Bug 22877: (follow-up) Fix test plan count

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2019-05-16 09:13:02 UTC
Created attachment 89821 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

This patch embelishes the existing comments slightly and removes a test
which had a FIXME, but was in fact just a duplicate of the test directly
above it.  I beleive it became a duplicate after the subroutine signature
changed at some point recently.  I also move the bug 22877 specific test
into a subtest.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2019-05-16 09:14:59 UTC
Doh, rookie mistake I forgot to update the test plan!

Sorted now.
Comment 26 Liz Rea 2019-05-16 16:20:25 UTC
Created attachment 89858 [details] [review]
Bug 22877: Unit test to highlight problem

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 27 Liz Rea 2019-05-16 16:20:33 UTC
Created attachment 89859 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 28 Liz Rea 2019-05-16 16:20:36 UTC
Created attachment 89860 [details] [review]
Bug 22877: (follow-up) Fix test plan count

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 29 Liz Rea 2019-05-16 16:20:40 UTC
Created attachment 89861 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

This patch embelishes the existing comments slightly and removes a test
which had a FIXME, but was in fact just a duplicate of the test directly
above it.  I beleive it became a duplicate after the subroutine signature
changed at some point recently.  I also move the bug 22877 specific test
into a subtest.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 30 Liz Rea 2019-05-16 18:17:05 UTC
*** Bug 22902 has been marked as a duplicate of this bug. ***
Comment 31 Marcel de Rooy 2019-05-31 06:15:50 UTC
QA: Looking here now
Comment 32 Marcel de Rooy 2019-05-31 06:28:26 UTC
Created attachment 90210 [details] [review]
Bug 22877: Unit test to highlight problem

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2019-05-31 06:28:31 UTC
Created attachment 90211 [details] [review]
Bug 22877: Prevent double overdue processing for lost and returned

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2019-05-31 06:28:36 UTC
Created attachment 90212 [details] [review]
Bug 22877: (follow-up) Fix test plan count

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2019-05-31 06:28:41 UTC
Created attachment 90213 [details] [review]
Bug 22877: (QA follow-up) Clarify intent of tests and remove one.

This patch embelishes the existing comments slightly and removes a test
which had a FIXME, but was in fact just a duplicate of the test directly
above it.  I beleive it became a duplicate after the subroutine signature
changed at some point recently.  I also move the bug 22877 specific test
into a subtest.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Martin Renvoize 2019-06-03 17:51:50 UTC
Nice work!

Pushed to master for 19.11.00
Comment 37 Andrew 2019-06-10 17:59:57 UTC
*** Bug 22727 has been marked as a duplicate of this bug. ***
Comment 38 Christopher Brannon 2019-07-03 14:33:05 UTC
I would really like to see this backported to 18.X