Bug 23103 - Cannot checkin items lost by deleted patrons with fines attached
Summary: Cannot checkin items lost by deleted patrons with fines attached
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23177
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-12 13:11 UTC by Nick Clemens
Modified: 2019-08-14 17:17 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.02


Attachments
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached (3.51 KB, patch)
2019-06-12 19:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached (3.56 KB, patch)
2019-06-13 10:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23103: (QA Follow-up) Return undef implicitly (861 bytes, patch)
2019-06-19 11:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached (3.56 KB, patch)
2019-06-21 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23103: (QA follow-up) Return undef implicitly (910 bytes, patch)
2019-06-21 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached (3.66 KB, patch)
2019-06-21 09:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23103: (QA follow-up) Return undef implicitly (1008 bytes, patch)
2019-06-21 09:25 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-06-12 13:11:54 UTC
To recreate:
1 - Checkout an item to a patron
2 - Ensure the item has a replacement cost (or itemtype has default)
3 - Ensure patrons are charged when items lost
4 - Mark the item lost
5 - Confirm patron has a fine
6 - Write off the fine
7 - Delete the patron
8 - Check in the item
9 - Internal server error:
Can't call method "account" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2421

We should probably just limit the search to accountlines with a borrowernumber
Comment 1 Kyle M Hall 2019-06-12 19:13:13 UTC
Created attachment 90538 [details] [review]
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached

Test Plan:
1) Checkout an item to a patron
2) Ensure the item has a replacement cost (or itemtype has default)
3) Ensure patrons are charged when items lost
4) Mark the item lost
5) Confirm patron has a fine
6) Write off the fine
7) Delete the patron
8) Check in the item
9) Note the internal server error
10) Apply this patch
11) Repeat steps 1-8
12) Note there is no internal server error!
13) prove t/db_dependent/Circulation.t
Comment 2 Martin Renvoize 2019-06-13 10:57:00 UTC
Created attachment 90573 [details] [review]
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached

Test Plan:
1) Checkout an item to a patron
2) Ensure the item has a replacement cost (or itemtype has default)
3) Ensure patrons are charged when items lost
4) Mark the item lost
5) Confirm patron has a fine
6) Write off the fine
7) Delete the patron
8) Check in the item
9) Note the internal server error
10) Apply this patch
11) Repeat steps 1-8
12) Note there is no internal server error!
13) prove t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2019-06-13 10:57:14 UTC
Works for me, signing off.
Comment 4 Jonathan Druart 2019-06-19 01:19:33 UTC
Why explicit return of undef here?
Comment 5 Kyle M Hall 2019-06-19 11:01:14 UTC
Created attachment 90775 [details] [review]
Bug 23103: (QA Follow-up) Return undef implicitly
Comment 6 Jonathan Druart 2019-06-19 15:58:53 UTC
Should we really ignore this case? It sounds like the library owes money to somebody.
I have no idea how we could handle it properly however...
Comment 7 Marcel de Rooy 2019-06-21 07:39:58 UTC
Martin:
        * Commit title does not contain 'follow-up' correctly spelt - 093cc6da89
        * Commit title does not contain '(QA follow-up)' correctly spelt - 093cc6da89

;)
Comment 8 Marcel de Rooy 2019-06-21 07:40:25 UTC
QA: Looking here
Comment 9 Marcel de Rooy 2019-06-21 07:54:13 UTC
Strange. Circulation.t passes and fails with me:

    not ok 19 - AddReturn must have debarred the patron
    #   Failed test 'AddReturn must have debarred the patron'
    #   at t/db_dependent/Circulation.t line 3194.
    #          got: ''
    #     expected: '1'
    # AddReturn returned message $VAR1 = {
    #           'WasReturned' => 1
    #         };
    not ok 20 - Test at line 1918
    #   Failed test 'Test at line 1918'
    #   at t/db_dependent/Circulation.t line 3198.
    #          got: '0'
    #     expected: '1'
    not ok 21 - Test at line 1918
    #   Failed test 'Test at line 1918'
    #   at t/db_dependent/Circulation.t line 3201.
    #          got: undef
    #     expected: '2019-07-05'
    # Looks like you failed 3 tests of 21.

t/db_dependent/Circulation.t .. 130/131
    #   Failed test 'Item needs rentalcharge confirmation to be issued'
    #   at t/db_dependent/Circulation.t line 3358.
    #     Structures begin differing at:
    #          $got->{RENTALCHARGE} = Does not exist
    #     $expected->{RENTALCHARGE} = '1'

    #   Failed test 'Item needs rentalcharge confirmation to be issued, increment'
    #   at t/db_dependent/Circulation.t line 3362.
    #     Structures begin differing at:
    #          $got->{RENTALCHARGE} = Does not exist
    #     $expected->{RENTALCHARGE} = '3'
    # Looks like you failed 2 tests of 2.
t/db_dependent/Circulation.t .. 131/131
#   Failed test 'CanBookBeIssued & RentalFeesCheckoutConfirmation'
#   at t/db_dependent/Circulation.t line 3364.
Comment 10 Martin Renvoize 2019-06-21 08:24:04 UTC
(In reply to Jonathan Druart from comment #6)
> Should we really ignore this case? It sounds like the library owes money to
> somebody.
> I have no idea how we could handle it properly however...

I think this isn't a problem actually.. the use case is that patron lost the book and then left the library.. then at a later date someone came across the book, spotted the library tag and so was nice enough to return it to the library.  The original patron has left at this point, there's no way for us to refund the payment to them and I don't think there's a requirement to track them down to do so.
Comment 11 Martin Renvoize 2019-06-21 08:36:45 UTC
Created attachment 90878 [details] [review]
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached

Test Plan:
1) Checkout an item to a patron
2) Ensure the item has a replacement cost (or itemtype has default)
3) Ensure patrons are charged when items lost
4) Mark the item lost
5) Confirm patron has a fine
6) Write off the fine
7) Delete the patron
8) Check in the item
9) Note the internal server error
10) Apply this patch
11) Repeat steps 1-8
12) Note there is no internal server error!
13) prove t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-06-21 08:36:48 UTC
Created attachment 90879 [details] [review]
Bug 23103: (QA follow-up) Return undef implicitly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Marcel de Rooy 2019-06-21 09:24:35 UTC
OK Fixed the test with rollback problems on bug 23177. Test passes now continuously.
Comment 14 Marcel de Rooy 2019-06-21 09:25:19 UTC
Created attachment 90883 [details] [review]
Bug 23103: Cannot checkin items lost by deleted patrons with fines attached

Test Plan:
1) Checkout an item to a patron
2) Ensure the item has a replacement cost (or itemtype has default)
3) Ensure patrons are charged when items lost
4) Mark the item lost
5) Confirm patron has a fine
6) Write off the fine
7) Delete the patron
8) Check in the item
9) Note the internal server error
10) Apply this patch
11) Repeat steps 1-8
12) Note there is no internal server error!
13) prove t/db_dependent/Circulation.t

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2019-06-21 09:25:23 UTC
Created attachment 90884 [details] [review]
Bug 23103: (QA follow-up) Return undef implicitly

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Martin Renvoize 2019-06-21 11:38:01 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Fridolin SOMERS 2019-07-16 06:25:52 UTC
Pushed to 19.05.x for 19.05.02
Comment 18 Lucas Gass 2019-08-14 17:17:00 UTC
backported to 18.11.x for 18.11.09