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*
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 [U+1F984]
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.