Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable.
Created attachment 85766 [details] [review] Bug 14708: The patron set as the anonymous patron should not be deletable. Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable. Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message!
Created attachment 85770 [details] [review] Bug 14708: The patron set as the anonymous patron should not be deletable. Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable. Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message! Signed-off-by: Devinim <kohadevinim@devinim.com.tr>
Created attachment 85783 [details] [review] Bug 14708: The patron set as the anonymous patron should not be deletable. Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable. Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message!
Created attachment 85789 [details] [review] Bug 14708: Enforce restriction in delete_patrons.pl
Hi Kyle, At first glance I'd say tools/cleanborrowers.pl is missing. Maybe we should have this constraint in Koha::Patron->delete/move_to_deleted to make sure we are covering all cases?
(In reply to Jonathan Druart from comment #5) > Hi Kyle, > At first glance I'd say tools/cleanborrowers.pl is missing. > Maybe we should have this constraint in Koha::Patron->delete/move_to_deleted > to make sure we are covering all cases? Definitey... Another place could merging patrons.... so hard to not forget anything...
Created attachment 103026 [details] [review] Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete
Created attachment 103027 [details] [review] Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge
Created attachment 103028 [details] [review] Bug 14708: Don't allow merging of Anonymous Patron into other patron records
Fleshing out Kyle's test plan based on later additions: Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message! 5) Attempt to delete the patron from patch patron deletion 6) Note the error message! 7) Attempt to merge the patron into another record 8) Note the error message!
This technically works, but: - When merging patrons, I am able to merge another patron into my anonymous patron. Is this what we want? Could be, just wanted to make sure it'd been considered. - When I try to merge my anonymous patron into someone else, it fails as desired but the error given is "No valid patrons to merge were found." Would be nice to have something more specific about why the action cannot be completed. - When attempting to include the anonymous patron in a batch delete, Koha does not delete them and does not tell you why. This is really a larger issue, as that's standard batch patron delete behavior. I'm really just taking this opportunity to stump for bug 24483, which suggests adding some feedback about who wasn't batch deleted and why.
Created attachment 103922 [details] [review] Bug 14708: Don't allow merging of other patron records into Anonymous Patron
(In reply to Andrew Fuerste-Henry from comment #11) > This technically works, but: > - When merging patrons, I am able to merge another patron into my anonymous > patron. Is this what we want? Could be, just wanted to make sure it'd been > considered. Fixed with the latest patch! > - When I try to merge my anonymous patron into someone else, it fails as > desired but the error given is "No valid patrons to merge were found." Would > be nice to have something more specific about why the action cannot be > completed. That would definitely be a nice enhancement, but it out of the scope of this bug. Can you file a followup bug report for that? > - When attempting to include the anonymous patron in a batch delete, Koha > does not delete them and does not tell you why. This is really a larger > issue, as that's standard batch patron delete behavior. I'm really just > taking this opportunity to stump for bug 24483, which suggests adding some > feedback about who wasn't batch deleted and why. Also agreed, but is of course outside the scope of this bug report. I will make 14708 depend on this bug so the feedback can include anonymous borrower issues when merging.
Hi, nitpicking here but IMO: - tests in their own commit make it easier for double checking they fail on unpatched code, as they must. - some patches could be squashed, thinking of git bisect. - Tests are done with 'AnonymousPatron' set to $anonymous_patron, so far so good, but what if AnonymousPatron is set to a different patron or unset? It's not obvious if it's covered by older tests. Regards Didier
Created attachment 105024 [details] [review] Bug 14708: The patron set as the anonymous patron should not be deletable. Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable. Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message! Signed-off-by: Devinim <kohadevinim@devinim.com.tr> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105025 [details] [review] Bug 14708: Enforce restriction in delete_patrons.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105026 [details] [review] Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105027 [details] [review] Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105028 [details] [review] Bug 14708: Don't allow merging of Anonymous Patron into other patron records Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105029 [details] [review] Bug 14708: Don't allow merging of other patron records into Anonymous Patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105184 [details] [review] Bug 14708: The patron set as the anonymous patron should not be deletable. Right now it's possible to delete the anonymous patron at any time. When this happens the system will break just as if anonymouspatron was unset but patrons still have an "always anonymize" privacy setting. This issue occurs more frequently than one would expect. We should make it so that the patron set in AnonymousPatron is not deletable. Test Plan: 1) Apply this patch 2) Set a patron to by AnonymousPatron 3) Attempt to delete that patron from the members toolbar 4) Note the error message! Signed-off-by: Devinim <kohadevinim@devinim.com.tr> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105185 [details] [review] Bug 14708: Enforce restriction in delete_patrons.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105186 [details] [review] Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105187 [details] [review] Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105188 [details] [review] Bug 14708: Don't allow merging of Anonymous Patron into other patron records Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 105189 [details] [review] Bug 14708: Don't allow merging of other patron records into Anonymous Patron Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Has strings!
I used the "Batch patron deletion and anonymization" tool and the anonymous patron has moved to deletedborrowers. Is that expected? I don't think so.
Hm, I think we always set the anonymous patron as staff user, that prevents that.
(In reply to Jonathan Druart from comment #28) > I used the "Batch patron deletion and anonymization" tool and the anonymous > patron has moved to deletedborrowers. Is that expected? I don't think so. I don't recreate, can you provide your steps?
hum, cannot recreate either now.
sub delete { my ($self) = @_; + my $anonymous_patron = C4::Context->preference("AnonymousPatron"); + return $self if $anonymous_patron && $self->id eq $anonymous_patron; Shouldn't we raise an exception instead?
Waiting for an answer here.
Created attachment 112830 [details] [review] Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
(In reply to Jonathan Druart from comment #32) > sub delete { > my ($self) = @_; > > + my $anonymous_patron = C4::Context->preference("AnonymousPatron"); > + return $self if $anonymous_patron && $self->id eq $anonymous_patron; > > > Shouldn't we raise an exception instead? Here you go!
Created attachment 112831 [details] [review] Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
Created attachment 112922 [details] [review] Bug 14708: (QA follow-up) Use try/catch blocks when calling delete() on a patron
I like the idea that the Anonymous Patron cannot be deleted, but I think that this implementation is too narrowly cast. I think that we should consider doing Bug 26170 (Create "system" patrons that cannot be (easily) deleted via the web UI) instead of this one, since there are many scenarios where we don't want to be able to delete patron accounts that are important (e.g. SIP patron, REST API patron, etc.)
(In reply to David Cook from comment #38) > I like the idea that the Anonymous Patron cannot be deleted, but I think > that this implementation is too narrowly cast. > > I think that we should consider doing Bug 26170 (Create "system" patrons > that cannot be (easily) deleted via the web UI) instead of this one, since > there are many scenarios where we don't want to be able to delete patron > accounts that are important (e.g. SIP patron, REST API patron, etc.) Admittedly, I don't have any patches on Bug 26170, and it's not high on my priority list at the moment, so I don't see myself working on it any time soon. Kyle has done all this great work, and really if it could be adapted to be more generalizable for all kinds of "system" patrons, that would be amazing. It would be a very useful development.
(In reply to David Cook from comment #39) > (In reply to David Cook from comment #38) > > I like the idea that the Anonymous Patron cannot be deleted, but I think > > that this implementation is too narrowly cast. > > > > I think that we should consider doing Bug 26170 (Create "system" patrons > > that cannot be (easily) deleted via the web UI) instead of this one, since > > there are many scenarios where we don't want to be able to delete patron > > accounts that are important (e.g. SIP patron, REST API patron, etc.) > > Admittedly, I don't have any patches on Bug 26170, and it's not high on my > priority list at the moment, so I don't see myself working on it any time > soon. > > Kyle has done all this great work, and really if it could be adapted to be > more generalizable for all kinds of "system" patrons, that would be amazing. > It would be a very useful development. I was actually discussing that as the 'next step' after this development. We could easily add a 'read-only' flag to a given account and move the AnonymousPatron logic into a a new Patron method like is_deletable. This would be especially useful for accounts used for SIP, APIs, and so forth. I would consider this to be a building block for bug 26170.
(In reply to Kyle M Hall from comment #40) > I was actually discussing that as the 'next step' after this development. We > could easily add a 'read-only' flag to a given account and move the > AnonymousPatron logic into a a new Patron method like is_deletable. This > would be especially useful for accounts used for SIP, APIs, and so forth. I > would consider this to be a building block for bug 26170. That sounds good to me! I worry a bit that we're talking about refactoring a change before it's even pushed into Koha, but I suppose that I wouldn't really want to hold up this change either...
Having this code into master won't help anything: it will have to be rebuilt as it cannot be used as a good base. I am willing to push this patchset as it, given the low bug number (but not sure it's a good argument...)
There is a test failing however: # Failed test 'Patron not found' # at t/db_dependent/api/v1/patrons.t line 423. # got: '500' # expected: '404' And a test missing to cover the change from the last patch.
(In reply to Jonathan Druart from comment #42) > Having this code into master won't help anything: it will have to be rebuilt > as it cannot be used as a good base. > > I am willing to push this patchset as it, given the low bug number (but not > sure it's a good argument...) The argument is, this is a bugfix. And refactoring things the way they are discussing is a good path forward. But it won't happen in stable for a while.
(In reply to Jonathan Druart from comment #43) > There is a test failing however: > > # Failed test 'Patron not found' > # at t/db_dependent/api/v1/patrons.t line 423. > # got: '500' > # expected: '404' > > > And a test missing to cover the change from the last patch. BTW: I love tests :-D
Created attachment 113121 [details] [review] Bug 14708: (QA follow-up) Add tests This patch adds tests for the change. It also simplifies the delete() method structure a bit. It fixes the error 500 the tests were raising. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! The new behaviour (code 403) is tested! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 20.11, thanks to everybody involved!
This patchset causes bug 26984 (tests are failing if anonymous patron is configured). We must fix that before the 20.11 release!
not pushing this to 20.05.x until I can also push the patch for bug 26984
Pushed to 20.05.x for 20.05.07
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.