Bug 26517 - Avoid deleting patrons with permission
Summary: Avoid deleting patrons with permission
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 28191
  Show dependency treegraph
 
Reported: 2020-09-23 14:44 UTC by Fridolin Somers
Modified: 2021-04-22 14:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11,19.11.17


Attachments
Bug 26517: Avoid deleting patrons with permission (1.46 KB, patch)
2020-09-23 15:00 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26517: Avoid deleting patrons with permission (1.46 KB, patch)
2020-09-24 07:11 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26517: Add unit test (4.76 KB, patch)
2020-09-24 07:11 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26517: Avoid deleting patrons with permission (1.50 KB, patch)
2020-09-24 13:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 26517: Add unit test (4.81 KB, patch)
2020-09-24 13:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 26517: Avoid deleting patrons with permission (1.56 KB, patch)
2021-03-19 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26517: Add unit test (4.87 KB, patch)
2021-03-19 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2020-09-23 14:44:38 UTC
The patron deletion script misc/cronjobs/delete_patrons.pl uses C4::Member::GetBorrowersToExpunge() to get patrons that may be deleted.
This method filters patrons from a staff category.
I propose to also filter patrons having permission, so a staff member.
Some small libraries do not define a "staff" category and give permissions to regular patrons.
Comment 1 Fridolin Somers 2020-09-23 14:51:14 UTC
We already have an example of library where delete patrons script deleted all there staff patrons XD
Comment 2 Fridolin Somers 2020-09-23 15:00:31 UTC
Created attachment 110606 [details] [review]
Bug 26517: Avoid deleting patrons with permission

The patron deletion script misc/cronjobs/delete_patrons.pl uses C4::Member::GetBorrowersToExpunge() to get patrons that may be deleted.
This method filters patrons from a staff category.
I propose to also filter patrons having permission, so a staff member.
Some small libraries do not define a "staff" category and give permissions to regular patrons.

Test plan :
1) Create a patron on adult type category with expiry date in the past and permission to access staff interface
2) Without patch
3) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
4) You see the patron will be deleted
5) Apply patch
6) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
7) You dont see the patron
Comment 3 Fridolin Somers 2020-09-24 07:11:08 UTC
Created attachment 110636 [details] [review]
Bug 26517: Avoid deleting patrons with permission

The patron deletion script misc/cronjobs/delete_patrons.pl uses C4::Member::GetBorrowersToExpunge() to get patrons that may be deleted.
This method filters patrons from a staff category.
I propose to also filter patrons having permission, so a staff member.
Some small libraries do not define a "staff" category and give permissions to regular patrons.

Test plan :
1) Create a patron on adult type category with expiry date in the past and permission to access staff interface
2) Without patch
3) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
4) You see the patron will be deleted
5) Apply patch
6) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
7) You dont see the patron
Comment 4 Fridolin Somers 2020-09-24 07:11:31 UTC
Created attachment 110637 [details] [review]
Bug 26517: Add unit test

Run prove t/db_dependent/Members.t
Comment 5 David Nind 2020-09-24 13:32:40 UTC
Created attachment 110656 [details] [review]
Bug 26517: Avoid deleting patrons with permission

The patron deletion script misc/cronjobs/delete_patrons.pl uses C4::Member::GetBorrowersToExpunge() to get patrons that may be deleted.
This method filters patrons from a staff category.
I propose to also filter patrons having permission, so a staff member.
Some small libraries do not define a "staff" category and give permissions to regular patrons.

Test plan :
1) Create a patron on adult type category with expiry date in the past and permission to access staff interface
2) Without patch
3) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
4) You see the patron will be deleted
5) Apply patch
6) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
7) You dont see the patron

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2020-09-24 13:32:45 UTC
Created attachment 110657 [details] [review]
Bug 26517: Add unit test

Run prove t/db_dependent/Members.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Katrin Fischer 2020-09-24 20:01:07 UTC
Hm, a few small things:

1) if a user has only subpermissions, flags are 0. Should we extend the check to take care of these? I could imagine someone removing catalog permission for someone temporarily not working at the library, but expected to return.
2) I think all the tests for GetBororwersToExpunge should be grouped into a subtest (but this was an issue before your patch)
3) All patrons I see in the tests have flags = undef... am I missing the one blocked from deleting for flags?
Comment 8 Fridolin Somers 2020-09-25 14:15:19 UTC
$patron->set({ flags => 0 })->store;(In reply to Katrin Fischer from comment #7)
> Hm, a few small things:
> 
> 1) if a user has only subpermissions, flags are 0. Should we extend the
> check to take care of these? I could imagine someone removing catalog
> permission for someone temporarily not working at the library, but expected
> to return.
Ich, another problem from this strange permissions storage.


> 2) I think all the tests for GetBororwersToExpunge should be grouped into a
> subtest (but this was an issue before your patch)
Agreed. I'll try to find some time to patch.

> 3) All patrons I see in the tests have flags = undef... am I missing the one
> blocked from deleting for flags?
In tests I add I use : $patron->set({ flags => 4 })->store.
I add in other tests flags => undef for them to pass, otherwise the value is random.
Comment 9 Fridolin Somers 2020-09-25 14:21:03 UTC
(In reply to Katrin Fischer from comment #7)
> Hm, a few small things:
> 
> 1) if a user has only subpermissions, flags are 0. Should we extend the
> check to take care of these? I could imagine someone removing catalog
> permission for someone temporarily not working at the library, but expected
> to return.

Note that the correct behavior is to define staff patrons inside a category type S which is skipped from purge.
Maybe we could had a check in search_for_data_inconsistencies.pl
Comment 10 Katrin Fischer 2020-09-29 18:51:39 UTC
Adding something to the inconsistencies script sounds nice - but what would it test in this case?
Comment 11 Fridolin Somers 2020-09-30 08:48:20 UTC
(In reply to Katrin Fischer from comment #10)
> Adding something to the inconsistencies script sounds nice - but what would
> it test in this case?

Search for patrons with (sub-)permission(s) not in a category type S.
Comment 12 Katrin Fischer 2020-10-19 15:56:42 UTC
(In reply to Fridolin SOMERS from comment #11)
> (In reply to Katrin Fischer from comment #10)
> > Adding something to the inconsistencies script sounds nice - but what would
> > it test in this case?
> 
> Search for patrons with (sub-)permission(s) not in a category type S.

Ok :)
Comment 13 Fridolin Somers 2021-03-11 11:46:13 UTC
(In reply to Katrin Fischer from comment #12)
> (In reply to Fridolin SOMERS from comment #11)
> > (In reply to Katrin Fischer from comment #10)
> > > Adding something to the inconsistencies script sounds nice - but what would
> > > it test in this case?
> > 
> > Search for patrons with (sub-)permission(s) not in a category type S.
> 
> Ok :)

See bug 27925

I set this one to signed off.
Comment 14 Nick Clemens 2021-03-19 12:31:50 UTC
Created attachment 118524 [details] [review]
Bug 26517: Avoid deleting patrons with permission

The patron deletion script misc/cronjobs/delete_patrons.pl uses C4::Member::GetBorrowersToExpunge() to get patrons that may be deleted.
This method filters patrons from a staff category.
I propose to also filter patrons having permission, so a staff member.
Some small libraries do not define a "staff" category and give permissions to regular patrons.

Test plan :
1) Create a patron on adult type category with expiry date in the past and permission to access staff interface
2) Without patch
3) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
4) You see the patron will be deleted
5) Apply patch
6) Run delete script : ./src/misc/cronjobs/delete_patrons.pl -v --expired_before='$(date -I)'
7) You dont see the patron

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2021-03-19 12:31:53 UTC
Created attachment 118525 [details] [review]
Bug 26517: Add unit test

Run prove t/db_dependent/Members.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2021-03-19 12:34:45 UTC
I think all of Katrin's points are valid, and the follow-up bug to show these patrons as inconsistencies would be nice. As it is though, this prevents patrons who (maybe) should not be deleted from being deleted so I think it is an improvement
Comment 17 Katrin Fischer 2021-03-21 15:22:29 UTC
(In reply to Nick Clemens from comment #16)
> I think all of Katrin's points are valid, and the follow-up bug to show
> these patrons as inconsistencies would be nice. As it is though, this
> prevents patrons who (maybe) should not be deleted from being deleted so I
> think it is an improvement

Agreed!
Comment 18 Jonathan Druart 2021-04-01 16:53:39 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2021-04-07 08:44:48 UTC
Pushed to 20.11.x for 20.11.05
Comment 20 Andrew Fuerste-Henry 2021-04-21 19:44:49 UTC
Pushed to 20.05.x for 20.05.11
Comment 21 Victor Grousset/tuxayo 2021-04-22 14:57:28 UTC
Backported: Pushed to 19.11.x branch for 19.11.17