Bug 27004 - Deleting a staff account who have created claims returned causes problem in the return_claims table because of a NULL value in return_claims.created_by.
Summary: Deleting a staff account who have created claims returned causes problem in t...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 14697
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-12 15:33 UTC by Enica Davis
Modified: 2021-01-15 01:06 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07,19.11.14


Attachments
Bug 27004: Prevent claim resolution to fail (1.93 KB, patch)
2020-12-21 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27004: Prevent claim resolution to fail (2.16 KB, patch)
2020-12-21 18:00 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27004: Regression tests (1.82 KB, patch)
2020-12-29 17:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27004: Prevent claim resolution to fail (2.21 KB, patch)
2020-12-29 17:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Enica Davis 2020-11-12 15:33:00 UTC
When a staff deletes another staff member's staff account and that particular account happens to have created return_claims, it creates a NULL value in the return_claims.created_by, causing a problem when another staff tries to resolve the claims. On the patron's account Claim tab, when one tries to resolve the claim, it will perpetually load without doing anything.

Only way to fix is update the NULL value on the created_by and add a borrowernumber. Is there a way to transfer the claim to a different staff account? Should Koha allow the deletion of an account when there are still return_claims unresolved?
Comment 1 Lisette Scheer 2020-11-13 02:36:04 UTC
(In reply to Enica Davis from comment #0)
> Should Koha allow the deletion of an account when there are still
> return_claims unresolved?

I think yes, either by transferring to another account or by storing a "dead" borrowernumber like in the reports table.
Comment 2 Katrin Fischer 2020-11-13 07:34:20 UTC
Could setting to NULL also be an option? I think it might work better with FK constraints than allowing a "dead" one :)
Comment 3 Lisette Scheer 2020-11-13 16:16:33 UTC
(In reply to Katrin Fischer from comment #2)
> Could setting to NULL also be an option? I think it might work better with
> FK constraints than allowing a "dead" one :)

Yeah I think that would work.
Comment 4 Enica Davis 2020-11-13 16:45:23 UTC
That's the problem: NULL value on return_claims.created_by would cause a problem in the staff client if you try to resolve the claim. The "Resolve" button would perpetually spin. It's a minor issue that the only quick fix is to update the database and assign a borrowernumber to return_claims.created_by. :(
Comment 5 Katrin Fischer 2020-11-13 20:10:41 UTC
Ok, so we can fix this outside the database and teach it to work with NULL borrowernumber then?
Comment 6 Jonathan Druart 2020-12-21 10:10:45 UTC
Created attachment 114547 [details] [review]
Bug 27004: Prevent claim resolution to fail

If the account of the patron who created a claim is removed, then the
claim won't be resolvable.
Trying to resolve the claim will result in a 500:
[2020/12/21 10:05:55] [ERROR] PUT /api/v1/return_claims/1/resolve: unhandled exception (Koha::Exceptions::Checkouts::ReturnClaims::NoCreatedBy)<<[created_by is mandatory]>> Koha::REST::Plugin::Exceptions::__ANON

We should not check if created_by exist when we update an existing hold,
only when we create it.

Test plan:
- Login with patron A
- Check an item in
- Claim returned it
- Login with patron B
- Delete patron A
- Resolve the claim
Comment 7 Jonathan Druart 2020-12-21 10:12:24 UTC
QA: should not we have a test/exception for resolved_by? We have one for created_by, it would make sense to have the same behaviour for *_by fields.
Comment 8 Victor Grousset/tuxayo 2020-12-21 18:00:59 UTC
Created attachment 114573 [details] [review]
Bug 27004: Prevent claim resolution to fail

If the account of the patron who created a claim is removed, then the
claim won't be resolvable.
Trying to resolve the claim will result in a 500:
[2020/12/21 10:05:55] [ERROR] PUT /api/v1/return_claims/1/resolve: unhandled exception (Koha::Exceptions::Checkouts::ReturnClaims::NoCreatedBy)<<[created_by is mandatory]>> Koha::REST::Plugin::Exceptions::__ANON

We should not check if created_by exist when we update an existing hold,
only when we create it.

Test plan:
- Login with patron A
- set a value to syspref ClaimReturnedLostValue
- Check an item out
- Claim returned it
- Login with patron B
- Delete patron A
- Resolve the claim
- ERROR in api-error.log an UI never confirming the resolution
- apply patch & restart services
- Resolve the claim
- it works

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2020-12-21 18:03:42 UTC
It works :)

Completed test plan + fixed "Check an item in" => "Check an item out" (maybe it's actually doable with a checkin)
Comment 10 Tomás Cohen Arazi 2020-12-29 17:42:20 UTC
Created attachment 114732 [details] [review]
Bug 27004: Regression tests

This patch introduces regression tests for the change in
Koha::Checkouts::ReturnClaim->store.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Checkouts/ReturnClaim.t
=> FAIL: Tests fail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2020-12-29 17:42:28 UTC
Created attachment 114733 [details] [review]
Bug 27004: Prevent claim resolution to fail

If the account of the patron who created a claim is removed, then the
claim won't be resolvable.
Trying to resolve the claim will result in a 500:
[2020/12/21 10:05:55] [ERROR] PUT /api/v1/return_claims/1/resolve: unhandled exception (Koha::Exceptions::Checkouts::ReturnClaims::NoCreatedBy)<<[created_by is mandatory]>> Koha::REST::Plugin::Exceptions::__ANON

We should not check if created_by exist when we update an existing hold,
only when we create it.

Test plan:
- Login with patron A
- set a value to syspref ClaimReturnedLostValue
- Check an item out
- Claim returned it
- Login with patron B
- Delete patron A
- Resolve the claim
- ERROR in api-error.log an UI never confirming the resolution
- apply patch & restart services
- Resolve the claim
- it works

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Jonathan Druart 2021-01-04 13:10:35 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 13 Fridolin Somers 2021-01-05 08:35:37 UTC
Pushed to 20.11.x for 20.11.01
Comment 14 Andrew Fuerste-Henry 2021-01-05 17:55:34 UTC
Pushed to 20.05.x for 20.05.07
Comment 15 Victor Grousset/tuxayo 2021-01-15 01:06:58 UTC
Backported: Pushed to 19.11.x branch for 19.11.14