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.
We already have an example of library where delete patrons script deleted all there staff patrons XD
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
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
Created attachment 110637 [details] [review] Bug 26517: Add unit test Run prove t/db_dependent/Members.t
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>
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>
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?
$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.
(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
Adding something to the inconsistencies script sounds nice - but what would it test in this case?
(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.
(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 :)
(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.
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>
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>
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
(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!
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.05
Pushed to 20.05.x for 20.05.11
Backported: Pushed to 19.11.x branch for 19.11.17