Bug 16966 - Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise
Summary: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 16961 16965
Blocks: 16846 18169 19599 20363
  Show dependency treegraph
 
Reported: 2016-07-22 16:35 UTC by Jonathan Druart
Modified: 2018-12-03 20:04 UTC (History)
5 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:
Version(s) released in:


Attachments
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.74 KB, patch)
2016-07-22 17:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.86 KB, patch)
2016-07-22 17:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (13.73 KB, patch)
2016-07-22 17:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (14.90 KB, patch)
2016-07-22 17:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.74 KB, patch)
2016-10-13 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.32 KB, patch)
2016-10-13 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.25 KB, patch)
2016-10-13 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (665 bytes, patch)
2016-10-13 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.81 KB, patch)
2016-10-19 06:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.38 KB, patch)
2016-10-19 06:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.31 KB, patch)
2016-10-19 06:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (734 bytes, patch)
2016-10-19 06:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.80 KB, patch)
2016-11-04 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.42 KB, patch)
2016-11-04 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.40 KB, patch)
2016-11-04 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.80 KB, patch)
2016-12-02 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.42 KB, patch)
2016-12-02 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.40 KB, patch)
2016-12-02 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.85 KB, patch)
2016-12-02 15:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.48 KB, patch)
2016-12-02 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.45 KB, patch)
2016-12-02 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.85 KB, patch)
2016-12-20 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.51 KB, patch)
2016-12-20 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.45 KB, patch)
2016-12-20 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Make the tests pass again (733 bytes, patch)
2016-12-20 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: The method should return a Koha::Patrons object (2.27 KB, patch)
2016-12-20 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.85 KB, patch)
2017-02-17 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.41 KB, patch)
2017-02-17 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.33 KB, patch)
2017-02-17 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Make the tests pass again (748 bytes, patch)
2017-02-17 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: The method should return a Koha::Patrons object (2.43 KB, patch)
2017-02-17 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Teach K::S::R::OldIssue its Koha objects class (879 bytes, patch)
2017-02-17 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.13 KB, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Make the tests pass again (748 bytes, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: The method should return a Koha::Patrons object (2.43 KB, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Teach K::S::R::OldIssue its Koha objects class (879 bytes, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: Fix small QA issues (1.91 KB, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: move parameters to hashref (7.87 KB, patch)
2017-02-24 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests (3.95 KB, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests (24.50 KB, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise (15.22 KB, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: Make the tests pass again (845 bytes, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: The method should return a Koha::Patrons object (2.52 KB, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: Teach K::S::R::OldIssue its Koha objects class (976 bytes, patch)
2017-02-27 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: Fix small QA issues (2.01 KB, patch)
2017-02-27 12:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: move parameters to hashref (7.96 KB, patch)
2017-02-27 12:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16966: [QA Follow-up] Check count, not size in template (2.34 KB, patch)
2017-02-27 12:46 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 2016-07-22 16:35:48 UTC
The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the number of patrons we will anonymise the history, the other one is used to anonymise the issues history. The problem is that the first one is not used to do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set: if 2+ patrons from different libraries match the date parameter, the interface will display "Checkout history for 2 patrons will be anonymized", when actually only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to forever (privacy=0), the same issue will appear.

The patches provided will refactor the code and fix these 2 issues.
Comment 1 Jonathan Druart 2016-07-22 17:06:15 UTC
Created attachment 53658 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.
Comment 2 Jonathan Druart 2016-07-22 17:06:18 UTC
Created attachment 53659 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.
Comment 3 Jonathan Druart 2016-07-22 17:06:21 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-07-22 17:14:52 UTC
Created attachment 53661 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.
Comment 5 Jonathan Druart 2016-10-13 12:41:51 UTC
Created attachment 56376 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.
Comment 6 Jonathan Druart 2016-10-13 12:41:55 UTC
Created attachment 56377 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.
Comment 7 Jonathan Druart 2016-10-13 12:42:00 UTC
Created attachment 56378 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.
Comment 8 Jonathan Druart 2016-10-13 12:42:04 UTC
Created attachment 56379 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests
Comment 9 Josef Moravec 2016-10-19 06:32:03 UTC
Created attachment 56639 [details] [review]
[SIGNED-OFF] Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2016-10-19 06:32:10 UTC
Created attachment 56640 [details] [review]
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2016-10-19 06:32:28 UTC
Created attachment 56641 [details] [review]
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2016-10-19 06:32:38 UTC
Created attachment 56642 [details] [review]
[SIGNED-OFF] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Jonathan Druart 2016-11-04 09:50:17 UTC
Created attachment 57170 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Jonathan Druart 2016-11-04 09:50:22 UTC
Created attachment 57171 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Jonathan Druart 2016-11-04 09:50:27 UTC
Created attachment 57172 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Jonathan Druart 2016-11-04 09:50:50 UTC
Patches rebased (and 2 squashed).
Comment 17 Tomás Cohen Arazi 2016-12-01 17:49:30 UTC
(In reply to Jonathan Druart from comment #16)
> Patches rebased (and 2 squashed).

I didn't succeed applying them in current master.
Comment 18 Jonathan Druart 2016-12-02 08:25:10 UTC
Created attachment 57885 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Jonathan Druart 2016-12-02 08:25:21 UTC
Created attachment 57886 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Jonathan Druart 2016-12-02 08:25:31 UTC
Created attachment 57887 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Kyle M Hall 2016-12-02 15:00:28 UTC
Created attachment 57924 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2016-12-02 15:01:07 UTC
Created attachment 57925 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2016-12-02 15:01:11 UTC
Created attachment 57926 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Jonathan Druart 2016-12-20 11:16:35 UTC
Created attachment 58282 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jonathan Druart 2016-12-20 11:16:41 UTC
Created attachment 58283 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Jonathan Druart 2016-12-20 11:16:45 UTC
Created attachment 58284 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Jonathan Druart 2016-12-20 11:16:49 UTC
Created attachment 58285 [details] [review]
Bug 16966: Make the tests pass again
Comment 28 Jonathan Druart 2016-12-20 11:16:52 UTC
Created attachment 58286 [details] [review]
Bug 16966: The method should return a Koha::Patrons object
Comment 29 Marcel de Rooy 2017-02-17 07:56:02 UTC
QA: Looking at this one now
Comment 30 Marcel de Rooy 2017-02-17 08:03:49 UTC
Merge conflicts on several patches.
Subtests in subtests.
Please rebase yourself.
Comment 31 Jonathan Druart 2017-02-17 09:39:37 UTC
Created attachment 60377 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Jonathan Druart 2017-02-17 09:39:42 UTC
Created attachment 60378 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Jonathan Druart 2017-02-17 09:39:47 UTC
Created attachment 60379 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2017-02-17 09:39:52 UTC
Created attachment 60380 [details] [review]
Bug 16966: Make the tests pass again
Comment 35 Jonathan Druart 2017-02-17 09:39:56 UTC
Created attachment 60381 [details] [review]
Bug 16966: The method should return a Koha::Patrons object
Comment 36 Jonathan Druart 2017-02-17 09:40:00 UTC
Created attachment 60382 [details] [review]
Bug 16966: Teach K::S::R::OldIssue its Koha objects class
Comment 37 Marcel de Rooy 2017-02-24 09:41:08 UTC
Patrons.t crashes now on overdrive_auth_token in subtest "move_to_deleted".
See bug 16034.
Comment 38 Marcel de Rooy 2017-02-24 09:55:39 UTC
2999-12-12 ??
Comment 39 Marcel de Rooy 2017-02-24 10:27:59 UTC
QA Comments:

 FAIL   Koha/Patrons.pm
   FAIL   pod
                *** ERROR:
                empty =head3
                 in file Koha/Patrons.pm

The year 2999 in Patrons.pm should not be there. Joubu promises to fix it on a new report. See IRC :) Please add a link.

tools/cleanborrowers.pl
+use C4::Members;        # GetBorrowersWhoHavexxxBorrowed.
Not sure if you need this module any more. Seems not.

search_patrons_to_anonymise
anonymise_issue_history
I'd rather have a parameter hash here now instead of these individual ones. There will be another parameter some day.
Yes, article_requests_finished should have it too :)

Typo should return Koha::Patrons => Patron in t/db_dependent/Koha/Patrons.t
Comment 40 Jonathan Druart 2017-02-24 11:35:27 UTC
Created attachment 60648 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Jonathan Druart 2017-02-24 11:35:31 UTC
Created attachment 60649 [details] [review]
Bug 16966: Make the tests pass again
Comment 42 Jonathan Druart 2017-02-24 11:35:35 UTC
Created attachment 60650 [details] [review]
Bug 16966: The method should return a Koha::Patrons object
Comment 43 Jonathan Druart 2017-02-24 11:35:40 UTC
Created attachment 60651 [details] [review]
Bug 16966: Teach K::S::R::OldIssue its Koha objects class
Comment 44 Jonathan Druart 2017-02-24 11:35:44 UTC
Created attachment 60652 [details] [review]
Bug 16966: Fix small QA issues
Comment 45 Jonathan Druart 2017-02-24 11:35:49 UTC
Created attachment 60653 [details] [review]
Bug 16966: move parameters to hashref
Comment 46 Jonathan Druart 2017-02-24 11:37:56 UTC
(In reply to Marcel de Rooy from comment #39)
> QA Comments:
> 
>  FAIL   Koha/Patrons.pm
>    FAIL   pod
>                 *** ERROR:
>                 empty =head3
>                  in file Koha/Patrons.pm

Fixed!

> The year 2999 in Patrons.pm should not be there. Joubu promises to fix it on
> a new report. See IRC :) Please add a link.

It's not in module, but opac-privacy. I will!

> tools/cleanborrowers.pl
> +use C4::Members;        # GetBorrowersWhoHavexxxBorrowed.
> Not sure if you need this module any more. Seems not.

I remove the GetBorrowersWhoHavexxxBorrowed part, caused by a bad merge conflict (re-edited original patch). C4::Members is still used (GetBorrowersToExpunge)

> search_patrons_to_anonymise
> anonymise_issue_history
> I'd rather have a parameter hash here now instead of these individual ones.
> There will be another parameter some day.
> Yes, article_requests_finished should have it too :)

Done in last patch.

> Typo should return Koha::Patrons => Patron in t/db_dependent/Koha/Patrons.t

Fixed!
Comment 47 Marcel de Rooy 2017-02-24 12:01:28 UTC
Will come back here on Monday
Comment 48 Jonathan Druart 2017-02-24 12:29:31 UTC
(In reply to Jonathan Druart from comment #46)
> > The year 2999 in Patrons.pm should not be there. Joubu promises to fix it on
> > a new report. See IRC :) Please add a link.
> 
> It's not in module, but opac-privacy. I will!

See bug 18169.
Comment 49 Marcel de Rooy 2017-02-27 12:45:30 UTC
Created attachment 60701 [details] [review]
Bug 16966: GetBorrowersWithIssuesHistoryOlderThan - Highlight the existing issue in tests

If IndependentBranches is set, the code is buggy. This patch only
highlight the bug by providing a test.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2017-02-27 12:45:36 UTC
Created attachment 60702 [details] [review]
Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests

The next patch will move C4::Circulation::AnonymiseIssueHistory and
C4::Members::GetBorrowersWithIssuesHistoryOlderThan to Koha::Patrons
This patch move the history anonymisation code to the Patrons.t test
file and the entire subtest related to StoreLastBorrower to
StoreLastBorrower.t

It just moves and add some minor adjustements.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2017-02-27 12:45:41 UTC
Created attachment 60703 [details] [review]
Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise

The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2017-02-27 12:45:46 UTC
Created attachment 60704 [details] [review]
Bug 16966: Make the tests pass again

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2017-02-27 12:45:52 UTC
Created attachment 60705 [details] [review]
Bug 16966: The method should return a Koha::Patrons object

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2017-02-27 12:45:57 UTC
Created attachment 60706 [details] [review]
Bug 16966: Teach K::S::R::OldIssue its Koha objects class

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2017-02-27 12:46:02 UTC
Created attachment 60707 [details] [review]
Bug 16966: Fix small QA issues

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Marcel de Rooy 2017-02-27 12:46:08 UTC
Created attachment 60708 [details] [review]
Bug 16966: move parameters to hashref

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2017-02-27 12:46:15 UTC
Created attachment 60709 [details] [review]
Bug 16966: [QA Follow-up] Check count, not size in template

The method size is not covered by tests at koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt line 163.
The method size is not covered by tests at koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt line 183.

Script cleanborrowers passes patrons_to_delete and patrons_to_anonymize to
the template. One is an array ref where we should use 'size', the other is a
Koha::Patrons object where we should use 'count'.
If we don't, we will not anonymize a lot of records ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2017-02-27 12:47:00 UTC
QA Comment:
Looks good to me. Added another follow-up.

No blocker, but just mentioning:
> To make sure all the conditions are met, the caller has the responsability to call search_patrons_to_anonymise to filter the Koha::Patrons set.
I am not sure if this is the most ideal solution.
Koha::Patrons->anonymise_issue_history just does all records and ignores privacy settings.
A parameter for patrons might be safer?
Comment 59 Kyle M Hall 2017-03-03 17:20:46 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 60 Katrin Fischer 2017-03-12 22:50:43 UTC
This won't get ported back to 16.11.x as it is an enhancement.