[Bug 26517] New: Avoid deleting patrons with permission
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Bug ID: 26517 Summary: Avoid deleting patrons with permission Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: fridolin.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |fridolin.somers@biblibre.co |ity.org |m Status|NEW |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #1 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- We already have an example of library where delete patrons script deleted all there staff patrons XD -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #2 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 110606 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110606&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110606|0 |1 is obsolete| | --- Comment #3 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 110636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110636&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #4 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 110637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110637&action=edit Bug 26517: Add unit test Run prove t/db_dependent/Members.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110636|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 110656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110656&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110637|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 110657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110657&action=edit Bug 26517: Add unit test Run prove t/db_dependent/Members.t Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #8 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- $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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #9 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Adding something to the inconsistencies script sounds nice - but what would it test in this case? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #11 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #13 from Fridolin Somers <fridolin.somers@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27925 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110656|0 |1 is obsolete| | Attachment #110657|0 |1 is obsolete| | --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118524&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118525 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118525&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #16 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28001 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|21.05.00 |21.05.00,20.11.05 released in| | --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28191 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28191 [Bug 28191] Update wording on batch patron deletion to reflect changes from bug 26517 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.05.00,20.11.05 |21.05.00,20.11.05,20.05.11 released in| | Blocks|28191 | CC| |andrew@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable --- Comment #20 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 20.05.x for 20.05.11 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28191 [Bug 28191] Update wording on batch patron deletion to reflect changes from bug 26517 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28191 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28191 [Bug 28191] Update wording on batch patron deletion to reflect changes from bug 26517 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26517 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.05.00,20.11.05,20.05.11 |21.05.00,20.11.05,20.05.11, released in| |19.11.17 CC| |victor@tuxayo.net Status|Pushed to oldstable |Pushed to oldoldstable --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Backported: Pushed to 19.11.x branch for 19.11.17 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org