Bug 30007 - Make ->anonymize methods throw an exception if AnonymousPatron is not set
Summary: Make ->anonymize methods throw an exception if AnonymousPatron is not set
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords: release-notes-needed
Depends on: 29843 29857
Blocks: 30023
  Show dependency treegraph
 
Reported: 2022-02-02 15:01 UTC by Tomás Cohen Arazi
Modified: 2023-12-28 20:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30007: Add Koha::Exceptions::SysPref (1.92 KB, patch)
2022-02-04 14:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30007: Regression tests (6.04 KB, patch)
2022-02-04 14:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set (2.78 KB, patch)
2022-02-04 14:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30007: Regression tests (6.09 KB, patch)
2022-02-04 19:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 30007: Add Koha::Exceptions::SysPref (1.96 KB, patch)
2022-02-04 19:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set (2.82 KB, patch)
2022-02-04 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 30007: Regression tests (6.21 KB, patch)
2022-02-08 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30007: Add Koha::Exceptions::SysPref (2.08 KB, patch)
2022-02-08 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set (2.94 KB, patch)
2022-02-08 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30007: (follow-up) Adjust Koha::Patrons->search_patrons_to_anonymise tests (2.24 KB, patch)
2022-03-02 12:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30007: (follow-up) Adapt tests (5.82 KB, patch)
2022-03-02 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-02-02 15:01:12 UTC
If the system preference AnonymousPatron is not set, the following methods should throw an exception:

- Koha::Old::Hold(s)->anonymize
- Koha::Old::Checkout(s)->anonymize
Comment 1 Tomás Cohen Arazi 2022-02-04 14:11:53 UTC
Created attachment 130163 [details] [review]
Bug 30007: Add Koha::Exceptions::SysPref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-02-04 14:11:57 UTC
Created attachment 130164 [details] [review]
Bug 30007: Regression tests
Comment 3 Tomás Cohen Arazi 2022-02-04 14:12:01 UTC
Created attachment 130165 [details] [review]
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set

This patch makes the ->anonymize methods throw a
Koha::Exceptions::SysPref::NotSet exception when trying to anonymize
holds and checkouts without AnonymousPatron properly set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.t \
           t/db_dependent/Koha/Old/Holds.t \
           t/db_dependent/Koha/Old/Hold.t
=> FAIL: Tests fail, no exception thrown
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 4 David Nind 2022-02-04 19:36:54 UTC
Created attachment 130175 [details] [review]
Bug 30007: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-02-04 19:36:59 UTC
Created attachment 130176 [details] [review]
Bug 30007: Add Koha::Exceptions::SysPref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2022-02-04 19:37:03 UTC
Created attachment 130177 [details] [review]
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set

This patch makes the ->anonymize methods throw a
Koha::Exceptions::SysPref::NotSet exception when trying to anonymize
holds and checkouts without AnonymousPatron properly set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.t \
           t/db_dependent/Koha/Old/Holds.t \
           t/db_dependent/Koha/Old/Hold.t
=> FAIL: Tests fail, no exception thrown
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Martin Renvoize 2022-02-08 09:48:16 UTC
Created attachment 130272 [details] [review]
Bug 30007: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

https://bugs.koha-community.org/show_bug.cgi?id=3007
Comment 8 Martin Renvoize 2022-02-08 09:48:19 UTC
Created attachment 130273 [details] [review]
Bug 30007: Add Koha::Exceptions::SysPref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

https://bugs.koha-community.org/show_bug.cgi?id=3007
Comment 9 Martin Renvoize 2022-02-08 09:48:24 UTC
Created attachment 130274 [details] [review]
Bug 30007: Make ->anonymize methods throw an exception if AnonymousPatron is not set

This patch makes the ->anonymize methods throw a
Koha::Exceptions::SysPref::NotSet exception when trying to anonymize
holds and checkouts without AnonymousPatron properly set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Old/Checkouts.t \
           t/db_dependent/Koha/Old/Holds.t \
           t/db_dependent/Koha/Old/Hold.t
=> FAIL: Tests fail, no exception thrown
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

https://bugs.koha-community.org/show_bug.cgi?id=3007
Comment 10 Martin Renvoize 2022-02-08 09:49:41 UTC
Solid improvement, Passing QA
Comment 11 Fridolin Somers 2022-02-22 01:30:06 UTC
Waiting for Bug 29857
Comment 12 Fridolin Somers 2022-03-02 08:49:01 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Tomás Cohen Arazi 2022-03-02 12:20:19 UTC
Created attachment 131239 [details] [review]
Bug 30007: (follow-up) Adjust Koha::Patrons->search_patrons_to_anonymise tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2022-03-02 12:22:43 UTC
Frido, I posted a follow-up for Koha/Patrons.t tests failures.
Comment 15 Fridolin Somers 2022-03-02 18:44:52 UTC
Patch "(follow-up) Adjust Koha::Patrons->search_patrons_to_anonymise tests" pushed to master
Comment 16 Fridolin Somers 2022-03-02 18:48:33 UTC
Ohhh but this changes the actual behavior.
Without this patch running anonymise with AnonymousPatron undefined (0 or empry string) is allowed : borrowernumber is set to null
This is usefull because AnonymousPatron default value is 0.

We agree on new behavior and create a unit test for this case.
Comment 17 Tomás Cohen Arazi 2022-03-02 20:50:43 UTC
Created attachment 131298 [details] [review]
Bug 30007: (follow-up) Adapt tests

This patch adapts tests to acknowledge ->cancel and ->fill methods won't
yield 'undef' borrowernumber when anonymization takes place, and even
further, and exception will be thrown and cancellation/fulfillment will
be rejected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Bonus: this problem was hidding bad tests caused by 29857, fixed on this
one as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2022-03-02 20:51:24 UTC
(In reply to Fridolin Somers from comment #16)
> Ohhh but this changes the actual behavior.
> Without this patch running anonymise with AnonymousPatron undefined (0 or
> empry string) is allowed : borrowernumber is set to null
> This is usefull because AnonymousPatron default value is 0.
> 
> We agree on new behavior and create a unit test for this case.

Sorry for the delay. Latest patch adapts the tests.
Comment 19 Fridolin Somers 2022-03-03 06:32:11 UTC
Patch "Bug 30007: (follow-up) Adapt tests" pushed to master
Comment 20 Fridolin Somers 2022-03-03 06:45:11 UTC
(In reply to Fridolin Somers from comment #16)
> Ohhh but this changes the actual behavior.
> Without this patch running anonymise with AnonymousPatron undefined (0 or
> empry string) is allowed : borrowernumber is set to null
> This is usefull because AnonymousPatron default value is 0.
> 
But do we all agree with this new behavior.

How manage existing databases upgrade having AnonymousPatron not configured but still use batch_anonyise.pl (we have that a lot at Biblibre) ?
Comment 21 Tomás Cohen Arazi 2022-03-03 11:12:08 UTC
(In reply to Fridolin Somers from comment #20)
> (In reply to Fridolin Somers from comment #16)
> > Ohhh but this changes the actual behavior.
> > Without this patch running anonymise with AnonymousPatron undefined (0 or
> > empry string) is allowed : borrowernumber is set to null
> > This is usefull because AnonymousPatron default value is 0.
> > 
> But do we all agree with this new behavior.
> 
> How manage existing databases upgrade having AnonymousPatron not configured
> but still use batch_anonyise.pl (we have that a lot at Biblibre) ?

For the record, check-in dies if AnonymousPatron is not set, since bug 9942. I see two options:

- Add an anonymous patron on a DBrev.
- Disable OPACPrivacy if AnonymousPatron is incorrectly set.

This is already highlighted in about.pl as far as I can tell (see bug 24327).
Comment 22 Fridolin Somers 2022-11-30 07:58:23 UTC
We should remove this help text :
https://git.koha-community.org/Koha-community/Koha/src/branch/22.11.x/misc/cronjobs/batch_anonymise.pl#L41

"Note: If the system preference 'AnonymousPatron' is not defined, NULL will be used."