Bug 29018 - Deleting patrons from REST API doesn't do any checks or move to deletedborrowers
Summary: Deleting patrons from REST API doesn't do any checks or move to deletedborrowers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 28020 29741
  Show dependency treegraph
 
Reported: 2021-09-14 15:16 UTC by Katrin Fischer
Modified: 2022-06-06 20:31 UTC (History)
10 users (show)

See Also:
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


Attachments
Bug 29018: Regression tests (3.28 KB, patch)
2021-12-20 15:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29018: Make DELETE /patrons/:patron_id check things (2.97 KB, patch)
2021-12-20 15:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29018: Regression tests (3.33 KB, patch)
2021-12-21 00:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 29018: Make DELETE /patrons/:patron_id check things (3.02 KB, patch)
2021-12-21 00:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 29018: Regression tests (3.40 KB, patch)
2022-01-03 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29018: Make DELETE /patrons/:patron_id check things (3.09 KB, patch)
2022-01-03 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[21.05.x] Bug 29018: Regression tests (3.40 KB, patch)
2022-01-10 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29018: Make DELETE /patrons/:patron_id check things (3.17 KB, patch)
2022-01-10 18:34 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 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.