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?
Created attachment 89505 [details] [review] Bug 22877: Unit test to highlight problem
I presume 'CalculateFinesOnReturn' comes into play here too?
(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.
Created attachment 89507 [details] [review] Bug 22877: Prevent double overdue processing for lost and returned
Created attachment 89508 [details] [review] Bug 22877: Unit test to highlight problem
Created attachment 89509 [details] [review] Bug 22877: Prevent double overdue processing for lost and returned
Created attachment 89510 [details] [review] Bug 22877: (follow-up) Fix test plan count
Created attachment 89511 [details] [review] Bug 22877: Unit test to highlight problem Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 89513 [details] [review] Bug 22877: (follow-up) Fix test plan count Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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
(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?
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>
Wrong calls in test were caused by (just for ref): commit 004609e46814791fac540e1c8bfb21f647758143 Bug 14591: Update unit tests
*** Bug 20769 has been marked as a duplicate of this bug. ***
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>
(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.
(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.
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
Created attachment 89818 [details] [review] Bug 22877: Unit test to highlight problem Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 89820 [details] [review] Bug 22877: (follow-up) Fix test plan count Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Doh, rookie mistake I forgot to update the test plan! Sorted now.
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>
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>
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>
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>
*** Bug 22902 has been marked as a duplicate of this bug. ***
QA: Looking here now
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>
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>
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>
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>
Nice work! Pushed to master for 19.11.00
*** Bug 22727 has been marked as a duplicate of this bug. ***
I would really like to see this backported to 18.X
Pushed to 19.05.x for 19.05.01
This was "RESOLVED FIXED" as part of the automatic clean-up after release - I am reopening it because of Christopher's comment.