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.
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.
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.
Created attachment 53660 [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.
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.
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.
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.
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.
Created attachment 56379 [details] [review] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests
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>
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>
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>
Created attachment 56642 [details] [review] [SIGNED-OFF] Bug 16966: Koha::Patrons - Move AnonymiseIssueHistory tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
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>
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>
Patches rebased (and 2 squashed).
(In reply to Jonathan Druart from comment #16) > Patches rebased (and 2 squashed). I didn't succeed applying them in current master.
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 58285 [details] [review] Bug 16966: Make the tests pass again
Created attachment 58286 [details] [review] Bug 16966: The method should return a Koha::Patrons object
QA: Looking at this one now
Merge conflicts on several patches. Subtests in subtests. Please rebase yourself.
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>
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>
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>
Created attachment 60380 [details] [review] Bug 16966: Make the tests pass again
Created attachment 60381 [details] [review] Bug 16966: The method should return a Koha::Patrons object
Created attachment 60382 [details] [review] Bug 16966: Teach K::S::R::OldIssue its Koha objects class
Patrons.t crashes now on overdrive_auth_token in subtest "move_to_deleted". See bug 16034.
2999-12-12 ??
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
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>
Created attachment 60649 [details] [review] Bug 16966: Make the tests pass again
Created attachment 60650 [details] [review] Bug 16966: The method should return a Koha::Patrons object
Created attachment 60651 [details] [review] Bug 16966: Teach K::S::R::OldIssue its Koha objects class
Created attachment 60652 [details] [review] Bug 16966: Fix small QA issues
Created attachment 60653 [details] [review] Bug 16966: move parameters to hashref
(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!
Will come back here on Monday
(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.
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>
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>
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>
Created attachment 60704 [details] [review] Bug 16966: Make the tests pass again Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Created attachment 60707 [details] [review] Bug 16966: Fix small QA issues Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60708 [details] [review] Bug 16966: move parameters to hashref Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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?
Pushed to master for 17.05, thanks Jonathan, Marcel!
This won't get ported back to 16.11.x as it is an enhancement.