Bug 14708

Summary: The patron set as the anonymous patron should not be deletable
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: PatronsAssignee: Kyle M Hall <kyle.m.hall>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrew, dcook, didier.gautheron, gmcharlt, jonathan.druart+koha, josef.moravec, kyle.m.hall, kyle, lucas, nick, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26170
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07
Bug Depends on:    
Bug Blocks: 24483, 26984, 27144    
Attachments: Bug 14708: The patron set as the anonymous patron should not be deletable.
Bug 14708: The patron set as the anonymous patron should not be deletable.
Bug 14708: The patron set as the anonymous patron should not be deletable.
Bug 14708: Enforce restriction in delete_patrons.pl
Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete
Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge
Bug 14708: Don't allow merging of Anonymous Patron into other patron records
Bug 14708: Don't allow merging of other patron records into Anonymous Patron
Bug 14708: The patron set as the anonymous patron should not be deletable.
Bug 14708: Enforce restriction in delete_patrons.pl
Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete
Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge
Bug 14708: Don't allow merging of Anonymous Patron into other patron records
Bug 14708: Don't allow merging of other patron records into Anonymous Patron
Bug 14708: The patron set as the anonymous patron should not be deletable.
Bug 14708: Enforce restriction in delete_patrons.pl
Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete
Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge
Bug 14708: Don't allow merging of Anonymous Patron into other patron records
Bug 14708: Don't allow merging of other patron records into Anonymous Patron
Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
Bug 14708: (QA follow-up) Use try/catch blocks when calling delete() on a patron
Bug 14708: (QA follow-up) Add tests

Description Kyle M Hall 2015-08-21 17:27:19 UTC
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.
Comment 1 Kyle M Hall 2019-02-27 12:00:55 UTC
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!
Comment 2 Devinim 2019-02-27 13:04:26 UTC
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>
Comment 3 Kyle M Hall 2019-02-27 15:26:52 UTC
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!
Comment 4 Kyle M Hall 2019-02-27 17:00:20 UTC
Created attachment 85789 [details] [review]
Bug 14708: Enforce restriction in delete_patrons.pl
Comment 5 Jonathan Druart 2019-02-27 18:01:21 UTC
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?
Comment 6 Josef Moravec 2019-03-05 17:22:05 UTC
(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...
Comment 7 Kyle M Hall 2020-04-15 17:21:04 UTC
Created attachment 103026 [details] [review]
Bug 14708: Prevent deletion of Anonymous Patron at Koha::Patron::delete
Comment 8 Kyle M Hall 2020-04-15 17:32:23 UTC
Created attachment 103027 [details] [review]
Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge
Comment 9 Kyle M Hall 2020-04-15 17:48:22 UTC
Created attachment 103028 [details] [review]
Bug 14708: Don't allow merging of Anonymous Patron into other patron records
Comment 10 Andrew Fuerste-Henry 2020-04-16 11:53:08 UTC
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!
Comment 11 Andrew Fuerste-Henry 2020-04-16 12:09:45 UTC
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.
Comment 12 Kyle M Hall 2020-04-29 11:22:50 UTC
Created attachment 103922 [details] [review]
Bug 14708: Don't allow merging of other patron records into Anonymous Patron
Comment 13 Kyle M Hall 2020-04-29 11:25:26 UTC
(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.
Comment 14 Didier Gautheron 2020-05-06 12:10:23 UTC
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
Comment 15 Andrew Fuerste-Henry 2020-05-18 14:39:07 UTC
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>
Comment 16 Andrew Fuerste-Henry 2020-05-18 14:39:11 UTC
Created attachment 105025 [details] [review]
Bug 14708: Enforce restriction in delete_patrons.pl

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 17 Andrew Fuerste-Henry 2020-05-18 14:39:14 UTC
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>
Comment 18 Andrew Fuerste-Henry 2020-05-18 14:39:18 UTC
Created attachment 105027 [details] [review]
Bug 14708: Skip AnonymousPatron in GetBorrowersToExpunge

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 19 Andrew Fuerste-Henry 2020-05-18 14:39:21 UTC
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>
Comment 20 Andrew Fuerste-Henry 2020-05-18 14:39:25 UTC
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>
Comment 21 Katrin Fischer 2020-05-21 13:06:26 UTC
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>
Comment 22 Katrin Fischer 2020-05-21 13:06:31 UTC
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>
Comment 23 Katrin Fischer 2020-05-21 13:06:36 UTC
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>
Comment 24 Katrin Fischer 2020-05-21 13:06:40 UTC
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>
Comment 25 Katrin Fischer 2020-05-21 13:06:44 UTC
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>
Comment 26 Katrin Fischer 2020-05-21 13:06:48 UTC
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>
Comment 27 Katrin Fischer 2020-05-21 13:07:03 UTC
Has strings!
Comment 28 Jonathan Druart 2020-06-15 09:20:21 UTC
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.
Comment 29 Katrin Fischer 2020-06-15 10:57:59 UTC
Hm, I think we always set the anonymous patron as staff user, that prevents that.
Comment 30 Nick Clemens 2020-10-21 11:01:51 UTC
(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?
Comment 31 Jonathan Druart 2020-10-25 22:33:58 UTC
hum, cannot recreate either now.
Comment 32 Jonathan Druart 2020-10-25 22:34:54 UTC
 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?
Comment 33 Jonathan Druart 2020-11-02 09:41:56 UTC
Waiting for an answer here.
Comment 34 Kyle M Hall 2020-11-02 13:12:18 UTC
Created attachment 112830 [details] [review]
Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
Comment 35 Kyle M Hall 2020-11-02 13:13:17 UTC
(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!
Comment 36 Kyle M Hall 2020-11-02 13:13:55 UTC
Created attachment 112831 [details] [review]
Bug 14708: (QA follow-up) Throw exception when deleting anonymous borrower
Comment 37 Kyle M Hall 2020-11-03 13:55:22 UTC
Created attachment 112922 [details] [review]
Bug 14708: (QA follow-up) Use try/catch blocks when calling delete() on a patron
Comment 38 David Cook 2020-11-04 00:14:40 UTC
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.)
Comment 39 David Cook 2020-11-04 00:16:13 UTC
(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.
Comment 40 Kyle M Hall 2020-11-04 14:27:29 UTC
(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.
Comment 41 David Cook 2020-11-04 23:35:32 UTC
(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...
Comment 42 Jonathan Druart 2020-11-05 14:04:06 UTC
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...)
Comment 43 Jonathan Druart 2020-11-05 14:20:45 UTC
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.
Comment 44 Tomás Cohen Arazi 2020-11-05 14:23:43 UTC
(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.
Comment 45 Tomás Cohen Arazi 2020-11-05 14:53:00 UTC
(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
Comment 46 Tomás Cohen Arazi 2020-11-05 14:56:58 UTC
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>
Comment 47 Jonathan Druart 2020-11-06 14:55:45 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 48 Jonathan Druart 2020-11-10 08:49:23 UTC
This patchset causes bug 26984 (tests are failing if anonymous patron is configured).
We must fix that before the 20.11 release!
Comment 49 Lucas Gass 2020-11-16 22:09:17 UTC
not pushing this to 20.05.x until I can also push the patch for bug 26984
Comment 50 Andrew Fuerste-Henry 2020-12-09 21:48:57 UTC
Pushed to 20.05.x for 20.05.07
Comment 51 Victor Grousset/tuxayo 2020-12-10 16:57:49 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.