Bug 18169 - Date like 2999 should not be used arbitrarily
Summary: Date like 2999 should not be used arbitrarily
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 16966
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-24 12:25 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history (7.94 KB, patch)
2017-02-24 12:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18169: Add test without before parameter for search_patrons_to_anonymise (2.21 KB, patch)
2017-02-27 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18169: Make 'before' param non mandatory for Koha::Patrons->anonymise_issue_history (8.04 KB, patch)
2017-02-27 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18169: Add test without before parameter for search_patrons_to_anonymise (2.31 KB, patch)
2017-02-27 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.