Bug 18169

Summary: Date like 2999 should not be used arbitrarily
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16966    
Bug Blocks:    
Attachments: Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history
Bug 18169: Add test without before parameter for search_patrons_to_anonymise
Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history
Bug 18169: Add test without before parameter for search_patrons_to_anonymise

Description Jonathan Druart 2017-02-24 12:25:11 UTC
From opac-privacy.pl:

    # delete all reading records for items returned
    # uses a hardcoded date ridiculously far in the future

    my $rows = eval {
        Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( { before => '2999-12-12' } );
    };  

It sounds better to make this before parameter not mandatory, and remove the condition from the sql query if it is not passed.
Comment 1 Jonathan Druart 2017-02-24 12:29:06 UTC
Created attachment 60655 [details] [review]
Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history

From opac-privacy.pl:

    # delete all reading records for items returned
    # uses a hardcoded date ridiculously far in the future

    my $rows = eval {
        Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( { before => '2999-12-12' } );
    };

It sounds better to make this before parameter not mandatory, and remove the condition from the sql query if it is not passed.

Test plan:
1. Anonymise your reading history at the OPAC.
2. Confirm that all your reading history has been anonymised
Comment 2 Marcel de Rooy 2017-02-27 13:01:03 UTC
Question: You adjust the test, but you do not call search_patrons_to_anonymise without the before parameter ??
Comment 3 Jonathan Druart 2017-02-27 13:28:45 UTC
Created attachment 60712 [details] [review]
Bug 18169: Add test without before parameter for search_patrons_to_anonymise
Comment 4 Jonathan Druart 2017-02-27 13:30:00 UTC
(In reply to Marcel de Rooy from comment #2)
> Question: You adjust the test, but you do not call
> search_patrons_to_anonymise without the before parameter ??

Nope because the first patch does not modify the behavior for search_patrons_to_anonymise but anonymise_issue_history.

This is more a follow-up for 16966, but indeed it makes sense to have a test for this case.
Comment 5 Marcel de Rooy 2017-02-27 13:52:49 UTC
Created attachment 60713 [details] [review]
Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history

From opac-privacy.pl:

    # delete all reading records for items returned
    # uses a hardcoded date ridiculously far in the future

    my $rows = eval {
        Koha::Patrons->search({ 'me.borrowernumber' => $borrowernumber })->anonymise_issue_history( { before => '2999-12-12' } );
    };

It sounds better to make this before parameter not mandatory, and remove the condition from the sql query if it is not passed.

Test plan:
1. Anonymise your reading history at the OPAC.
2. Confirm that all your reading history has been anonymised

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-02-27 13:52:53 UTC
Created attachment 60714 [details] [review]
Bug 18169: Add test without before parameter for search_patrons_to_anonymise

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-02-27 13:55:24 UTC
(In reply to Jonathan Druart from comment #4)
> This is more a follow-up for 16966, but indeed it makes sense to have a test
> for this case.

Taking the liberty to pass qa directly on this patch set, as we could indeed have handled both patches on the other report as well..
Comment 8 Kyle M Hall 2017-03-03 18:17:06 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 9 Katrin Fischer 2017-03-12 23:01:38 UTC
This won't get ported back to 16.11.x as it is an enhancement.