Bug 19487 - Internal server error when writing off lost fine for item not checked out
Summary: Internal server error when writing off lost fine for item not checked out
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19776 19513
  Show dependency treegraph
 
Reported: 2017-10-18 16:16 UTC by Nick Clemens
Modified: 2017-12-07 17:41 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19487: Do not return an item if not checked out (1.85 KB, patch)
2017-10-18 17:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19487: Do not return an item if not checked out (1.90 KB, patch)
2017-10-19 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19487: Fix MarkIssueReturned.t (3.07 KB, patch)
2017-10-19 12:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19487: Fix MarkIssueReturned.t (3.15 KB, patch)
2017-10-20 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19487: (QA follow-up) More changes to MarkIssueReturned.t (4.63 KB, patch)
2017-10-20 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19487: Do not return an item if not checked out (2.00 KB, patch)
2017-10-20 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19487: Refetch the object to get up-to-date values (896 bytes, patch)
2017-10-20 18:57 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-10-18 16:16:47 UTC
To recreate:
1 - Manually add a lost fine to a ptron and include a barcode
2 - Attempt to write off the fine
3 - Internal server error
4 - Checkout an item and mark lost to checkin and fine
5 - Attempt to write off line
6 - Internal server error

It appears we are trying to move a non-existent issue to old_issues
Comment 1 Jonathan Druart 2017-10-18 17:01:39 UTC
Created attachment 68275 [details] [review]
Bug 19487: Do not return an item if not checked out

To recreate:
1 - Manually add a lost fine to a ptron and include a barcode
2 - Attempt to write off the fine
3 - Internal server error
4 - Checkout an item and mark lost to checkin and fine
5 - Attempt to write off line
6 - Internal server error
Comment 2 Jonathan Druart 2017-10-18 17:02:07 UTC
Upping severity, it affects stable release (at least 17.05.x)
Comment 3 Nick Clemens 2017-10-19 10:58:23 UTC
Created attachment 68293 [details] [review]
Bug 19487: Do not return an item if not checked out

To recreate:
1 - Manually add a lost fine to a ptron and include a barcode
2 - Attempt to write off the fine
3 - Internal server error
4 - Checkout an item and mark lost to checkin and fine
5 - Attempt to write off line
6 - Internal server error
7 - Apply patch
8 - Repeat without errors

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Marcel de Rooy 2017-10-19 11:18:33 UTC
 perl t/db_dependent/Circulation/MarkIssueReturned.t
1..2
not ok 1
#   Failed test at t/db_dependent/Circulation/MarkIssueReturned.t line 50.
#                   ''
#     doesn't match '(?^u:Fatal error: the patron \(\d+\) .* AnonymousPatron)'
ok 2
# Looks like you failed 1 test of 2.
Comment 5 Jonathan Druart 2017-10-19 12:03:13 UTC
Created attachment 68294 [details] [review]
Bug 19487: Fix MarkIssueReturned.t

MarkIssueReturned must now return directly if the issue does not exist.
We then check if the anonymous patron configuration is correct.
Comment 6 Marcel de Rooy 2017-10-20 06:50:41 UTC
QA: Looking here now
Comment 7 Marcel de Rooy 2017-10-20 07:56:36 UTC
Created attachment 68312 [details] [review]
Bug 19487: Fix MarkIssueReturned.t

MarkIssueReturned must now return directly if the issue does not exist.
We then check if the anonymous patron configuration is correct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-10-20 07:56:39 UTC
Created attachment 68313 [details] [review]
Bug 19487: (QA follow-up) More changes to MarkIssueReturned.t

$dbh is not used
$categorycode is no longer used
%item_branch_infos not used
$borrowernumber is obsoleted by using $patron
No real need for $anonymous_borrowernumber
No AddMember calls, removing unneeded modules

Adding a test that calls MarkIssueReturned on an item already returned.

Test plan:
[1] Without the patch changing MarkIssueReturned, this test fails (3 of 6).
[2] With the patch, the test should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-10-20 07:56:43 UTC
Created attachment 68314 [details] [review]
Bug 19487: Do not return an item if not checked out

To recreate:
1 - Manually add a lost fine to a ptron and include a barcode
2 - Attempt to write off the fine
3 - Internal server error
4 - Checkout an item and mark lost to checkin and fine
5 - Attempt to write off line
6 - Internal server error

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Jonathan Druart 2017-10-20 18:36:40 UTC
Comment on attachment 68313 [details] [review]
Bug 19487: (QA follow-up) More changes to MarkIssueReturned.t

I am not willing to push this patch.
We want a fix for stable releases, so something easy to backport.
This change is quite big may slow down the backport.

Moreover it seems that at least one test has been removed:
-unlike ( $@, qr<Fatal error: the patron \(\d+\) .* AnonymousPatron>, );

and this line too, which makes me think you cut a bit too much:
t::lib::Mocks::mock_preference('AnonymousPatron', $anonymous_borrowernumber);

Please move it to its own bug report.
Comment 11 Jonathan Druart 2017-10-20 18:57:02 UTC
Created attachment 68330 [details] [review]
Bug 19487: Refetch the object to get up-to-date values

This has been raised by failures on t/db_dependent/Circulation/issue.t
(thanks tests!)
Comment 12 Jonathan Druart 2017-10-20 20:16:54 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 13 Marcel de Rooy 2017-10-23 11:17:12 UTC
(In reply to Jonathan Druart from comment #10)
> Comment on attachment 68313 [details] [review] [review]
> Bug 19487: (QA follow-up) More changes to MarkIssueReturned.t
> 
> I am not willing to push this patch.
But you wanted to be RM ;)

> We want a fix for stable releases, so something easy to backport.
> This change is quite big may slow down the backport.
OK OK

> Moreover it seems that at least one test has been removed:
> -unlike ( $@, qr<Fatal error: the patron \(\d+\) .* AnonymousPatron>, );
No it isnt. This one is even sharper..
+is( $@, '', 'No die triggered by invalid itemnumber' );

> and this line too, which makes me think you cut a bit too much:
> t::lib::Mocks::mock_preference('AnonymousPatron', $anonymous_borrowernumber);
Yes I removed it since it was made useless by the previous changes. When you pass a wrong item number, MarkIssueReturned will now return undef rightaway. The whole anynomous stuff is not in reach. Leaving it in could reflect that we are testing it, while we are not.

> Please move it to its own bug report.
Bug 19513
Comment 14 Fridolin SOMERS 2017-11-20 13:10:04 UTC
Pushed to 17.05.x, will be in 17.05.06
Comment 15 Katrin Fischer 2017-11-20 21:31:25 UTC
These patches have been pushed to 16.11.x and will be in 16.11.14.