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
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
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>
Works for me, signing off.
Why explicit return of undef here?
Created attachment 90775 [details] [review] Bug 23103: (QA Follow-up) Return undef implicitly
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...
Martin: * Commit title does not contain 'follow-up' correctly spelt - 093cc6da89 * Commit title does not contain '(QA follow-up)' correctly spelt - 093cc6da89 ;)
QA: Looking here
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.
(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.
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>
Created attachment 90879 [details] [review] Bug 23103: (QA follow-up) Return undef implicitly Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK Fixed the test with rollback problems on bug 23177. Test passes now continuously.
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>
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>
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.02
backported to 18.11.x for 18.11.09