Bug 29018

Summary: Deleting patrons from REST API doesn't do any checks or move to deletedborrowers
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: REST APIAssignee: 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
When deleting a patron via the REST API it doesn't block on
- has a guarantee
- has checkouts
- has pending fines

and it doesn't move the patron to deletedborrowers :(

It looks like the code around deleting patrons could do with a little work:

Patrons batch deletes appear to use: 
https://git.koha-community.org/Koha-community/Koha/src/commit/42ce8c29960dc8d133f8403db31459bae810f8e1/C4/Members.pm#L367

Deleting a single patron from the account in staff has a lot of additional checks in the .pl
https://git.koha-community.org/Koha-community/Koha/src/commit/9d6d641d1f8b77271800f43bc027b651f9aea52b/members/deletemem.pl

And then there are delete and move_to_deleted in Koha/Patron*
Comment 1 Katrin Fischer 2021-09-14 15:17:28 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.
Comment 2 Fridolin Somers 2021-12-18 07:50:18 UTC
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.
Comment 3 Katrin Fischer 2021-12-18 21:35:29 UTC
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.
Comment 4 Tomás Cohen Arazi 2021-12-18 21:38:31 UTC
(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 :-)
Comment 5 Tomás Cohen Arazi 2021-12-20 15:19:12 UTC
Created attachment 128762 [details] [review]
Bug 29018: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-12-20 15:19:18 UTC
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>
Comment 7 Tomás Cohen Arazi 2021-12-20 19:02:16 UTC
(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
Comment 8 David Nind 2021-12-21 00:23:04 UTC
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>
Comment 9 David Nind 2021-12-21 00:23:08 UTC
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>
Comment 10 Jonathan Druart 2022-01-03 13:36:45 UTC
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>
Comment 11 Jonathan Druart 2022-01-03 13:36:49 UTC
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>
Comment 12 Fridolin Somers 2022-01-05 23:35:15 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Katrin Fischer 2022-01-06 13:54:26 UTC
I am late here, but thx a lot for fixing it!
Comment 14 Kyle M Hall 2022-01-07 11:26:08 UTC
Pushed to 21.11.x for 21.11.02
Comment 15 Tomás Cohen Arazi 2022-01-10 18:34:48 UTC
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>
Comment 16 Tomás Cohen Arazi 2022-01-10 18:34:53 UTC
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>
Comment 17 Tomás Cohen Arazi 2022-01-10 18:54:40 UTC
The last patches should apply to 20.11 as well.
Comment 18 Andrew Fuerste-Henry 2022-01-10 22:07:30 UTC
Pushed to 21.05.x for 21.05.08
Comment 19 Victor Grousset/tuxayo 2022-02-07 18:51:26 UTC
Backported: Pushed to 20.11.x branch for 20.11.15
Comment 20 wainuiwitikapark 2022-02-15 03:55:44 UTC
Do I need to backport this to 19.11.x? I probably won't unless there is a need for it
Comment 21 wainuiwitikapark 2022-02-21 04:47:27 UTC
Not backported to 19.11.x. Please request it if you need it.