Bug 22944 - avoid AnonymousPatron in search_patrons_to_anonymise
Summary: avoid AnonymousPatron in search_patrons_to_anonymise
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Keywords:
Depends on: 16966
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-21 10:05 UTC by Fridolin SOMERS
Modified: 2019-07-16 06:44 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02


Attachments
Bug 22944: change unit tests (2.41 KB, patch)
2019-05-21 10:13 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise (1.77 KB, patch)
2019-05-21 10:13 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22944: remove obsolete unit test (1.63 KB, patch)
2019-06-11 13:33 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22944: remove obsolete unit test (1.69 KB, patch)
2019-06-14 18:51 UTC, Arthur Bousquet
Details | Diff | Splinter Review
Bug 22944: change unit tests (2.46 KB, patch)
2019-06-21 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise (1.83 KB, patch)
2019-06-21 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22944: remove obsolete unit test (1.75 KB, patch)
2019-06-21 12:13 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2019-05-21 10:05:27 UTC
In Koha::Patrons::search_patrons_to_anonymise() old issues already affected to the anonymous patron should be avoided. It is useless and can impact performance.

It is already working if AnymousPatron is not defined or 0 because of :
  'old_issues.borrowernumber' => { 'not' => undef }
Comment 1 Fridolin SOMERS 2019-05-21 10:07:46 UTC
In theory the anymous patron should have privacy=0 but in case its not it should be explicitly avoided.
Comment 2 Fridolin SOMERS 2019-05-21 10:13:17 UTC
Created attachment 89932 [details] [review]
Bug 22944: change unit tests

Add a dedicated UT and correct a test return value
Comment 3 Fridolin SOMERS 2019-05-21 10:13:31 UTC
Created attachment 89933 [details] [review]
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise

In Koha::Patrons::search_patrons_to_anonymise() old issues already affected to the anonymous patron should be avoided. It is useless and can impact performance.

It is already working if AnymousPatron is not defined or 0 because of :
  'old_issues.borrowernumber' => { 'not' => undef }

In theory the anymous patron should have privacy=0 but in case its not it should be explicitly avoided.

Test plan :
Run t/db_dependent/Koha/Patrons.t
Comment 4 Jonathan Druart 2019-05-25 01:44:13 UTC
You are not explaining in the test description what is really expected. We do not want the update to affect any rows because privacy is set to 0, right?
Comment 5 Jonathan Druart 2019-05-25 01:50:49 UTC
(In reply to Jonathan Druart from comment #4)
> You are not explaining in the test description what is really expected. We
> do not want the update to affect any rows because privacy is set to 0, right?

Actually it returns > 0 because of previous tests, so it should be adjusted (ideally tests must be independent from each others).

I think this test must be removed as it is not testing anything useful: the previous statement will explode if something went wrong.
Comment 6 Fridolin SOMERS 2019-05-27 12:03:03 UTC Comment hidden (obsolete)
Comment 7 Fridolin SOMERS 2019-05-27 12:03:51 UTC
(In reply to Jonathan Druart from comment #5)
> I think this test must be removed as it is not testing anything useful: the
> previous statement will explode if something went wrong.

Hi, you mean this test ?

  is( $rows_affected, 0, 'AnonymiseIssueHistory should not return any error if success' );
Comment 8 Jonathan Druart 2019-05-28 22:29:33 UTC
(In reply to Fridolin SOMERS from comment #7)
> (In reply to Jonathan Druart from comment #5)
> > I think this test must be removed as it is not testing anything useful: the
> > previous statement will explode if something went wrong.
> 
> Hi, you mean this test ?
> 
>   is( $rows_affected, 0, 'AnonymiseIssueHistory should not return any error
> if success' );

yes
Comment 9 Fridolin SOMERS 2019-06-11 13:33:27 UTC
Created attachment 90488 [details] [review]
Bug 22944: remove obsolete unit test

Test 'AnonymiseIssueHistory should not return any error if success' is obsolete,
previous statement will explode if something went wrong.

Test plan :
Run t/db_dependent/Koha/Patrons.t
Comment 10 Arthur Bousquet 2019-06-14 18:51:01 UTC
Created attachment 90628 [details] [review]
Bug 22944: remove obsolete unit test

Test 'AnonymiseIssueHistory should not return any error if success' is obsolete,
previous statement will explode if something went wrong.

Test plan :
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Arthur Bousquet <arthur.bousquet@inlibro.com>


 Current status: Signed Off
Comment 11 Nick Clemens 2019-06-21 12:13:27 UTC
Created attachment 90904 [details] [review]
Bug 22944: change unit tests

Add a dedicated UT and correct a test return value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2019-06-21 12:13:30 UTC
Created attachment 90905 [details] [review]
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise

In Koha::Patrons::search_patrons_to_anonymise() old issues already affected to the anonymous patron should be avoided. It is useless and can impact performance.

It is already working if AnymousPatron is not defined or 0 because of :
  'old_issues.borrowernumber' => { 'not' => undef }

In theory the anymous patron should have privacy=0 but in case its not it should be explicitly avoided.

Test plan :
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2019-06-21 12:13:33 UTC
Created attachment 90906 [details] [review]
Bug 22944: remove obsolete unit test

Test 'AnonymiseIssueHistory should not return any error if success' is obsolete,
previous statement will explode if something went wrong.

Test plan :
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Arthur Bousquet <arthur.bousquet@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Martin Renvoize 2019-06-24 13:50:39 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Martin Renvoize 2019-06-24 14:48:10 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin SOMERS 2019-07-16 06:44:58 UTC
Pushed to 19.05.x for 19.05.02