Bug 22944

Summary: avoid AnonymousPatron in search_patrons_to_anonymise
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: PatronsAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart, kyle.m.hall, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02
Bug Depends on: 16966    
Bug Blocks:    
Attachments: Bug 22944: change unit tests
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise
Bug 22944: remove obsolete unit test
Bug 22944: remove obsolete unit test
Bug 22944: change unit tests
Bug 22944: avoid AnonymousPatron in search_patrons_to_anonymise
Bug 22944: remove obsolete unit test

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