|
Description
Tomás Cohen Arazi (tcohen)
2022-02-02 15:01:12 UTC
Created attachment 130163 [details] [review] Bug 30007: Add Koha::Exceptions::SysPref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 130164 [details] [review] Bug 30007: Regression tests 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 Created attachment 130175 [details] [review] Bug 30007: Regression tests Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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 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 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 Solid improvement, Passing QA Waiting for Bug 29857 Pushed to master for 22.05, thanks to everybody involved [U+1F984] 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> Frido, I posted a follow-up for Koha/Patrons.t tests failures. Patch "(follow-up) Adjust Koha::Patrons->search_patrons_to_anonymise tests" pushed to master 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. 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> (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. Patch "Bug 30007: (follow-up) Adapt tests" pushed to master (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) ? (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). 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." |