Summary: | Deleting patrons from REST API doesn't do any checks or move to deletedborrowers | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | REST API | Assignee: | Tomás Cohen Arazi <tomascohen> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | andrewfh, david, fridolin.somers, jonathan.druart, kyle, martin.renvoize, nugged, tomascohen, victor, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This fixes the REST API route for deleting patrons so that it now checks for guarantees, debts, and current checkouts. If any of these checks fail, the patron is not deleted.
|
Version(s) released in: |
22.05.00,21.11.02,21.05.08,20.11.15
|
Bug Depends on: | |||
Bug Blocks: | 28020, 29741 | ||
Attachments: |
Bug 29018: Regression tests
Bug 29018: Make DELETE /patrons/:patron_id check things Bug 29018: Regression tests Bug 29018: Make DELETE /patrons/:patron_id check things Bug 29018: Regression tests Bug 29018: Make DELETE /patrons/:patron_id check things [21.05.x] Bug 29018: Regression tests [21.05.x] Bug 29018: Make DELETE /patrons/:patron_id check things |
Description
Katrin Fischer
2021-09-14 15:16:33 UTC
I believe the API should not allow deletes on the same "hard reasons" as the GUI and take care of holds etc. silently as if it was confirmed in staff. Arg this is bad. We can see there is only one check from Bug 14708 Maybe we need to create like for Koha::Item : safe_to_delete() safe_delete() Or throw more exceptions. It's really bad but didn't get much attention yet. At the moment I would almost lean to deactivate that route as it might leave you with inconsistent data. (In reply to Katrin Fischer from comment #3) > It's really bad but didn't get much attention yet. At the moment I would > almost lean to deactivate that route as it might leave you with inconsistent > data. I'll fix it first thing in the morning next Monday :-) Created attachment 128762 [details] [review] Bug 29018: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 128763 [details] [review] Bug 29018: Make DELETE /patrons/:patron_id check things When the route was implemented, the checks were overlooked. This patch adds checks for: - Guarantees - Debts - Current checkouts Any of those will block deletion, as it should. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail, the route misses checks 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The three conditions prevent deletion! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Fridolin Somers from comment #2) > Arg this is bad. > > We can see there is only one check from Bug 14708 > > Maybe we need to create like for Koha::Item : > safe_to_delete() > safe_delete() Done safe_to_delete on bug 29741 Created attachment 128812 [details] [review] Bug 29018: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 128813 [details] [review] Bug 29018: Make DELETE /patrons/:patron_id check things When the route was implemented, the checks were overlooked. This patch adds checks for: - Guarantees - Debts - Current checkouts Any of those will block deletion, as it should. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail, the route misses checks 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The three conditions prevent deletion! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 128959 [details] [review] Bug 29018: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 128960 [details] [review] Bug 29018: Make DELETE /patrons/:patron_id check things When the route was implemented, the checks were overlooked. This patch adds checks for: - Guarantees - Debts - Current checkouts Any of those will block deletion, as it should. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail, the route misses checks 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The three conditions prevent deletion! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 22.05, thanks to everybody involved 🦄 I am late here, but thx a lot for fixing it! Pushed to 21.11.x for 21.11.02 Created attachment 129282 [details] [review] [21.05.x] Bug 29018: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 129283 [details] [review] [21.05.x] Bug 29018: Make DELETE /patrons/:patron_id check things When the route was implemented, the checks were overlooked. This patch adds checks for: - Guarantees - Debts - Current checkouts Any of those will block deletion, as it should. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail, the route misses checks 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The three conditions prevent deletion! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> The last patches should apply to 20.11 as well. Pushed to 21.05.x for 21.05.08 Backported: Pushed to 20.11.x branch for 20.11.15 Do I need to backport this to 19.11.x? I probably won't unless there is a need for it Not backported to 19.11.x. Please request it if you need it. |