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.
Created attachment 89932 [details] [review] Bug 22944: change unit tests Add a dedicated UT and correct a test return value
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
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?
(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.
(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. H is( $rows_affected, 0, 'AnonymiseIssueHistory should not return any error if success' );
(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' );
(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
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
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
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>
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>
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>
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.02