Summary: | 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. | ||
---|---|---|---|
Product: | Koha | Reporter: | Enica Davis <enica> |
Component: | Patrons | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Severity: | major | ||
Priority: | P5 - low | CC: | andrew, caroline.cyr-la-rose, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lisettepalouse+koha, tomascohen, victor |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
21.05.00,20.11.01,20.05.07,19.11.14
|
|
Circulation function: | |||
Bug Depends on: | 14697 | ||
Bug Blocks: | |||
Attachments: |
Bug 27004: Prevent claim resolution to fail
Bug 27004: Prevent claim resolution to fail Bug 27004: Regression tests Bug 27004: Prevent claim resolution to fail |
Description
Enica Davis
2020-11-12 15:33:00 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. Could setting to NULL also be an option? I think it might work better with FK constraints than allowing a "dead" one :) (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. 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. :( Ok, so we can fix this outside the database and teach it to work with NULL borrowernumber then? 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 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. 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> It works :) Completed test plan + fixed "Check an item in" => "Check an item out" (maybe it's actually doable with a checkin) 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> 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> Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.01 Pushed to 20.05.x for 20.05.07 Backported: Pushed to 19.11.x branch for 19.11.14 |