The batch patron deletion/anonymization does not allow for batch deletion of arbitrary lists of patrons. The batch patron modification tool allows for modification of arbitrary lists of patrons, but not deletion. If would be highly beneficial to add patron deletion to the batch patron modification tool.
Created attachment 19766 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool The batch patron deletion/anonymization does not allow for batch deletion of arbitrary lists of patrons. The batch patron modification tool allows for modification of arbitrary lists of patrons, but not deletion. If would be highly beneficial to add patron deletion to the batch patron modification tool. Test Plan: 1) Apply all dependencies for this patch 2) Apply this patch 3) Create a list of patrons with the new Patron Lists feature a) Include at least one patron owing fines b) Include at least one patron with items currently checked out c) Include at least one patron not falling into a) or b) 4) Browse to the batch patron modifications tool 5) Select your list from the pulldown, and submit 6) Check the "Delete patrons" checkbox, the click the submit button 7) You should a list of errors for the patrons with fines or issues and a table of patrons that were deleted successfully. 8) Click the link for a deleted patron, you should get a "patron not found" message.
Created attachment 19767 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool The batch patron deletion/anonymization does not allow for batch deletion of arbitrary lists of patrons. The batch patron modification tool allows for modification of arbitrary lists of patrons, but not deletion. If would be highly beneficial to add patron deletion to the batch patron modification tool. Test Plan: 1) Apply all dependencies for this patch 2) Apply this patch 3) Create a list of patrons with the new Patron Lists feature a) Include at least one patron owing fines b) Include at least one patron with items currently checked out c) Include at least one patron not falling into a) or b) 4) Browse to the batch patron modifications tool 5) Select your list from the pulldown, and submit 6) Check the "Delete patrons" checkbox, the click the submit button 7) You should a list of errors for the patrons with fines or issues and a table of patrons that were deleted successfully. 8) Click the link for a deleted patron, you should get a "patron not found" message.
Hi, Tried to test this, got the following: Falling back to patching base and 3-way merge... Auto-merging tools/modborrowers.pl CONFLICT (content): Merge conflict in tools/modborrowers.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt Failed to merge in the changes. Patch failed at 0001 Bug 10612 - Add ability to delete patrons with batch patron modification tool
(In reply to Liz Rea from comment #3) > Hi, > > Tried to test this, got the following: > > Falling back to patching base and 3-way merge... > Auto-merging tools/modborrowers.pl > CONFLICT (content): Merge conflict in tools/modborrowers.pl > Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt > Failed to merge in the changes. > Patch failed at 0001 Bug 10612 - Add ability to delete patrons with batch > patron modification tool It applied fine for me. Did you apply all the dependencies? First you need to apply 8798, then 10565, then finally this patch.
Comment on attachment 19767 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool Review of attachment 19767 [details] [review]: ----------------------------------------------------------------- ::: tools/modborrowers.pl @@ +259,5 @@ > + { error => "fees_owed", borrower => $borrower } ); > + } > + else { > + MoveMemberToDeleted($borrowernumber); > + DelMember($borrowernumber); This is missing a call to C4::VirtualShelves::HandleDelBorrower. Also, with the proposed patron purging cronjob (bug 10419) also in play, I am getting concerned about the number of different places in the code that (a) check whether a patron record is allowed to be deleted and (b) do the deletion, particularly since the original patch series for bug 10419 had also missed HandleDelBorrower.
I am extremely happy with this and would like to sign off on it.
Created attachment 21081 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool
Kyle, could you answer to Galen please (see comment 5). A call is missing, marked as Failed QA.
Created attachment 21444 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember
Created attachment 21445 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember
Created attachment 21446 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember
Hi Kyle, I am sorry, but this patch no longer applies and I was not sure how to resolve the conflict, could you please take a look? [~/kohaclone (73-10612-deletepatronsbatch)]> git bz apply 10612 Bug 10612 - Add ability to delete patrons with batch patron modification tool 21081 - Bug 10612 - Add ability to delete patrons with batch patron modification tool 21446 - Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 10612 - Add ability to delete patrons with batch patron modification tool /home/katrin/kohaclone/.git/rebase-apply/patch:41: trailing whitespace. Cannot delete patron /home/katrin/kohaclone/.git/rebase-apply/patch:49: trailing whitespace. Cannot delete patron /home/katrin/kohaclone/.git/rebase-apply/patch:101: trailing whitespace. warning: 3 lines add whitespace errors. Using index info to reconstruct a base tree... <stdin>:41: trailing whitespace. Cannot delete patron <stdin>:49: trailing whitespace. Cannot delete patron <stdin>:101: trailing whitespace. warning: 3 lines applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging tools/modborrowers.pl CONFLICT (content): Merge conflict in tools/modborrowers.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt Failed to merge in the changes. Patch failed at 0001 Bug 10612 - Add ability to delete patrons with batch patron modification tool When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-10612---Add-ability-to-delete-patrons-with-bat-_BRqbC.patch
Created attachment 22837 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool The batch patron deletion/anonymization does not allow for batch deletion of arbitrary lists of patrons. The batch patron modification tool allows for modification of arbitrary lists of patrons, but not deletion. If would be highly beneficial to add patron deletion to the batch patron modification tool. Test Plan: 1) Apply all dependencies for this patch 2) Apply this patch 3) Create a list of patrons with the new Patron Lists feature a) Include at least one patron owing fines b) Include at least one patron with items currently checked out c) Include at least one patron not falling into a) or b) 4) Browse to the batch patron modifications tool 5) Select your list from the pulldown, and submit 6) Check the "Delete patrons" checkbox, the click the submit button 7) You should a list of errors for the patrons with fines or issues and a table of patrons that were deleted successfully. 8) Click the link for a deleted patron, you should get a "patron not found" message. Signed-off-by: Nora Blake <nblake@masslibsystem.org>
Created attachment 22838 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember
Kyle, Why don't you use the Batch patron deletion? Maybe we could have a new entry "Delete patrons in the list <select>". Don't you think it is not consistent to allow to delete patron in 2 different places?
ping Kyle, park to in discussion.
(In reply to Jonathan Druart from comment #15) > Kyle, > Why don't you use the Batch patron deletion? Maybe we could have a new entry > "Delete patrons in the list <select>". > Don't you think it is not consistent to allow to delete patron in 2 > different places? The code is an extension of the addition of lists to the patron modification tool. I understand what you mean. However I'd suggest that over time we, instead of having two separate tools that function in different ways, unify both the existing patron modification and deletion tools in a manner equivalent to the batch item and modification deletion which is just the same script with a flag passed in for deletion. If you consider this change in behavior a QA deal breaker, please let me know by setting this bug to Failed QA.
(In reply to Kyle M Hall from comment #17) > The code is an extension of the addition of lists to the patron modification > tool. I understand what you mean. However I'd suggest that over time we, > instead of having two separate tools that function in different ways, unify > both the existing patron modification and deletion tools in a manner > equivalent to the batch item and modification deletion which is just the > same script with a flag passed in for deletion. I don't think the batch item mod/del is a good example :) The code is not clean and not easy to read/understand. I think it is a good thing to keep 2 pages and 2 scripts: 1 for the deletion and 1 for the modification. The borrower modification script has 400 lines and will be difficult to maintain if it grows too much. Moreover the "Delete patrons" checkbox is not ergonomic, I think: the user can edit patron fields and at the same time, delete patrons. Maybe should we have another point of view.
Hello, I am the force behind this whole development and I can tell you what our situation is and what we are trying to accomplish. We have many schools in our consortium. They want to be able to remove classes of students each year after they graduate. Our schools do not treat their patron accounts in the same way that they treat books, so they do not have the student barcodes on hand or available in the same way that they can grab a stack of books and scan them all in to the batch item deletion tool. They need to be able to search for a group of patrons by a common piece of information stored in a single field. In our case, we store year of graduation information in either the "sort 1" or "sort 2" field of the patron record. They need to be able to search for that common piece of information easily, obtain a grouping of patrons from the search and then decide whether they want to delete the whole class of kids from the system (probably the most common event we will have) OR change their patron type from "student" to "alumni" or something else. They don't use patron card numbers to find patron accounts. Often they checkout based on a name search or they may scan the kid's school ID, but they won't have those ID cards on hand when it comes time to delete them from the system. So they need to be able to perform a search for the "class of 2014" or "2014" or "grade 12," depending on what they have entered in the child's record and get a group of patrons together in one place so they can then delete them (most common need) or modify their patron type or modify their year of graduation if several are being held back or do something else that I can't think of. I hope this helps clarify why Kyle is looking to do what he is proposing. Thanks, Nora Blake
Hello Nora, Thanks for sharing your experience. What I suggested to Kyle is compatible with what you describe. If we can choose a patron list in both the modification and the deletion tools, I don't see any problem. Kyle, maybe the best way to do could be to add a link to jump from the patron list module to the modification or deletion tool. This way, when the patron list is created, the librarian can directly modify or delete the patrons from the list. Let me know if I missed something or if it does not make sense.
Created attachment 28779 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table
Created attachment 28780 [details] [review] Bug 10612 [Code cleanup] - Rename some variables To move towards a more consistent codebase, I have changed some variables from being camelCased or onelongname to the more standard convention of words_separated_with_underscores. In addition, this changes the use of the word 'member' to the word 'patron' to move towards unification of our terminology. Our internal terminology should move towards matching our external terminology ( members, borrowers => patrons, reserves => holds, etc ).
I got some odd results when testing: 1 Added four patrons to the list: One with a checkout One with a fine Two with empty accounts Patron modification link seemed to work as expected Patron deletion link said 3 patrons would be deleted, I deleted, it said 3 were gone, 2 were left in list (one with checkout, one with fine) I would be nice on the deletion screen to know who was going to be deleted/who was deleted, I can't tell from my results which patron (issue or fine) it was planning to delete. If not a list before deletion, maybe a results list after the action. I also found that I had two identical lists, both affected by deletion, but don't know if that is from this patch (In reply to Kyle M Hall from comment #1) > Created attachment 19766 [details] [review] [review] > Bug 10612 - Add ability to delete patrons with batch patron modification tool > > The batch patron deletion/anonymization does not allow for batch > deletion of arbitrary lists of patrons. The batch patron modification > tool allows for modification of arbitrary lists of patrons, but not > deletion. If would be highly beneficial to add patron deletion to the > batch patron modification tool. > > Test Plan: > 1) Apply all dependencies for this patch > 2) Apply this patch > 3) Create a list of patrons with the new Patron Lists feature > a) Include at least one patron owing fines > b) Include at least one patron with items currently checked out > c) Include at least one patron not falling into a) or b) > 4) Browse to the batch patron modifications tool > 5) Select your list from the pulldown, and submit > 6) Check the "Delete patrons" checkbox, the click the submit button > 7) You should a list of errors for the patrons with fines or issues > and a table of patrons that were deleted successfully. > 8) Click the link for a deleted patron, you should get a "patron not > found" message.
(In reply to Nick Clemens from comment #23) > I got some odd results when testing: > > 1 Added four patrons to the list: > One with a checkout > One with a fine > Two with empty accounts > > Patron modification link seemed to work as expected Excellent! > Patron deletion link said 3 patrons would be deleted, I deleted, it said 3 > were gone, 2 were left in list (one with checkout, one with fine) > > I would be nice on the deletion screen to know who was going to be > deleted/who was deleted, I can't tell from my results which patron (issue or > fine) it was planning to delete. If not a list before deletion, maybe a > results list after the action. Those would be great features, but are general enhancements to the batch patron deletion tool and are outside the scope of this bug report. > I also found that I had two identical lists, both affected by deletion, but > don't know if that is from this patch Any deleted patrons would disappear from all patron lists, so that is to be expected.
Understood on all, I was unclear in my comment though, sorry for that I checked off all 4 patrons for deletion, the deletion tool said it would delete 3, and that it had deleted 3, but only 2 were actually deleted. It seemed to work as expected, not deleting patrons with fines or checkouts, but the message to the user was unclear
Are patron automatically excluded from deletion for some reason? I tried to delete based on a patron list that contains 82 patrons and the tool offers to delete 2 patrons.
(In reply to Owen Leonard from comment #26) > Are patron automatically excluded from deletion for some reason? I tried to > delete based on a patron list that contains 82 patrons and the tool offers > to delete 2 patrons. Only patrons with non-zero account balance (with unpaid fines/charges) are explicitly excluded when deleting from a list. Batch deleting (from a list) patrons with active check-outs really needs some thought, IMO (apart from the somehow missleading count mentioned in comment #25). While such patron records would not get actually deleted (due to the foreign key constraint in issues table): - their reserves and lists/virtual shelves (if any) would be deleted, - if the choosen action is 'Move these patrons to the trash', I think that the surplus record would be created in the deletedborrowers table, with original patron record still remaining in borrowers table. Also, when deleting from a list, it's possible to (accidentally) delete a staff patron, and a patron which is a guarantor for some other patron[s]. On an unrelated subject: 'patron category' filter in batch patron deletion/anonymization tool seems to be badly broken in the current master (different issue, encountered while testing this patch - I will file separate bug report for it).
(In reply to Jacek Ablewicz from comment #27) > Only patrons with non-zero account balance (with unpaid fines/charges) are > explicitly excluded when deleting from a list. Thanks that's what was blocking the deletions. It would be nice if one could get an error report on records which were not deleted (out of scope for this bug of course). > Batch deleting (from a list) patrons with active check-outs really needs > some thought, IMO Is this issue separate from batch deleting patrons using the other options (inactivity, expiration, or category) ?
Created attachment 36311 [details] [review] [SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables To move towards a more consistent codebase, I have changed some variables from being camelCased or onelongname to the more standard convention of words_separated_with_underscores. In addition, this changes the use of the word 'member' to the word 'patron' to move towards unification of our terminology. Our internal terminology should move towards matching our external terminology ( members, borrowers => patrons, reserves => holds, etc ). Signed-off-by: Nicole <nicole@bywatersolutions.com>
Created attachment 36313 [details] [review] [SIGNED-OFF] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table Signed-off-by: Nicole <nicole@bywatersolutions.com>
Created attachment 36314 [details] [review] [SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables To move towards a more consistent codebase, I have changed some variables from being camelCased or onelongname to the more standard convention of words_separated_with_underscores. In addition, this changes the use of the word 'member' to the word 'patron' to move towards unification of our terminology. Our internal terminology should move towards matching our external terminology ( members, borrowers => patrons, reserves => holds, etc ). Signed-off-by: Nicole <nicole@bywatersolutions.com>
Created attachment 36315 [details] [review] [SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables To move towards a more consistent codebase, I have changed some variables from being camelCased or onelongname to the more standard convention of words_separated_with_underscores. In addition, this changes the use of the word 'member' to the word 'patron' to move towards unification of our terminology. Our internal terminology should move towards matching our external terminology ( members, borrowers => patrons, reserves => holds, etc ). Signed-off-by: Nicole <nicole@bywatersolutions.com>
Some notes about this: 1) I know it's outside the scope of this bug, but we are about to make the feature much more prominent and easier to use with this patch. I am worried about the lack of good error messages and possible missing checks in the patron deletion tool (see also comment#27). In general we always need to check at least for: - checkouts - fines - guarantor relationships Example: Argh? I created a patron list with 6 patrons, I checked out a book to one of the patrons. The test run on the patron delete tells me the that 6 patrons would have been deleted. I feel like we need to get the base feature working well, before we can extend it's use like that. 2) Patron list page: I don't like the "2 symbol" concept on the buttons, because that won't work well for translations. You can't assume safely that the Verb is always on the left side and the subject on the right. For example in the German translations we could have both: Lösche Benutzer and Benutzer löschen.
Created attachment 41289 [details] [review] Bug 10612 [QA Followup] - Remove user icons from buttons
> 1) I know it's outside the scope of this bug, but we are about to make the > feature much more prominent and easier to use with this patch. > > I am worried about the lack of good error messages and possible missing > checks in the patron deletion tool (see also comment#27). In general we > always need to check at least for: > - checkouts > - fines > - guarantor relationships > > Example: Argh? I created a patron list with 6 patrons, I checked out a book > to one of the patrons. The test run on the patron delete tells me the that 6 > patrons would have been deleted. I agree this is out of the scope of this bug. Can you file a separate bug for that? > 2) Patron list page: I don't like the "2 symbol" concept on the buttons, > because that won't work well for translations. You can't assume safely that > the Verb is always on the left side and the subject on the right. For > example in the German translations we could have both: Lösche Benutzer and > Benutzer löschen. I've removed the double icons and just kept the left most icon for each!
Hi Kyle, could you do me a favour and try the test case I described? I feel if we push this without checking the basic functionality of the tool is correct, we will just end up with unhappy feedback on a good new feature. > Example: Argh? I created a patron list with 6 patrons, I checked out a book > to one of the patrons. The test run on the patron delete tells me the that 6 > patrons would have been deleted.
(In reply to Katrin Fischer from comment #36) > Hi Kyle, > > could you do me a favour and try the test case I described? I feel if we > push this without checking the basic functionality of the tool is correct, > we will just end up with unhappy feedback on a good new feature. > > > Example: Argh? I created a patron list with 6 patrons, I checked out a book > > to one of the patrons. The test run on the patron delete tells me the that 6 > > patrons would have been deleted. Katrin, I have tried and it works: Create patrons A and B Check an item out to B Add A and B to a patron list Use the tool to delete patrons from this list Test tun tells me 1 patron will be deleted Permanently delete the patrons delete only 1 patron
Created attachment 41363 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table Signed-off-by: Nicole <nicole@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41364 [details] [review] Bug 10612 [Code cleanup] - Rename some variables To move towards a more consistent codebase, I have changed some variables from being camelCased or onelongname to the more standard convention of words_separated_with_underscores. In addition, this changes the use of the word 'member' to the word 'patron' to move towards unification of our terminology. Our internal terminology should move towards matching our external terminology ( members, borrowers => patrons, reserves => holds, etc ). Signed-off-by: Nicole <nicole@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41365 [details] [review] Bug 10612 [QA Followup] - Remove user icons from buttons Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Hi Jonathan, thx for testing! - I will rerun my test and see if I did something differently.
- Add 4 patrons to a patron list, one has a checkout - Select patron list in batch tool - Reports: 4 patrons will be deleted (misleading, only 3 should) - Choose test run: 4 patrons would have been removed (if it wasn't a test run) In my eyes, if a test run doesn't reflect what would be happening, it's useless. - Go back, change to "move to trash" 4 patrons have been successfully moved to trash - The user with the checkout has NOT been deleted (thankfully) Now the next strange thing: - Checking the deletedborrowers table - there is my user with the checkout. So it actually moved all the borrowers to deletedborrowers and I have ended up with the same borrowernumber in borrowers and deletedborrowers. I know this is not related directly to the feature, but this tool gives me a headache as it is. I don't trust it at all. In my opinion it's very buggy and shouldn't be promoted before fixed.
Hm, it seems I tested the same as you but with very different results... what could be the difference? My results look worrysome, especially the double existance of borrower data in borrowers and deletedborrowers.
(In reply to Katrin Fischer from comment #43) > Hm, it seems I tested the same as you but with very different results... > what could be the difference? My results look worrysome, especially the > double existance of borrower data in borrowers and deletedborrowers. Tested again right now, and it works for me. Nothing in the logs? What happen if you use the tool using the 2 other criteria (not borrowed since, expiration date)?
I retested, this still has issues for me :( Trying again with a test plan, hoping someone can find out what's going on: - Created a patron list "Delete" with 2 borrowers from the sample data: Nicole Schneider - has 1 checkout Keith Hendrix - Chose Delete and checked the checkbox, no other changes - Chose "mose to trash" - Tool reports: 2 patrons have been successfully moved to trash - Nicole Schneider now exists in both borrowers and deletedborrowers The logs show: [Thu Oct 22 00:47:50.410257 2015] [cgi:error] [pid 11589] [client 127.0.0.1:36610] AH01215: [Thu Oct 22 00:47:50 2015] cleanborrowers.pl: DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) at /home/katrin/kohaclone/C4/Members.pm line 1813., referer: http://localhost:8080/cgi-bin/koha/tools/cleanborrowers.pl
Created attachment 46239 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches with a check added for issues before deleting borrowers. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table
I squashed patches, rebased, and added a check for issues to the subroutine: _skip_borrowers_with_nonzero_balance making it: _skip_borrowers_with_nonzero_balance_or_issues
Created attachment 46279 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches with a check added for issues before deleting borrowers. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com> ByWater internal signoff, still needs signoff from community. Patch seems kosher, passes QA tools. Needs followup to fix one small issue: squished patch undoes switch to FontAwesome icons in lists.tt.
Created attachment 46280 [details] [review] Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt
Created attachment 46834 [details] [review] Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt Signed-off-by: Natasha <tasham_8@hotmail.com>
Comment on attachment 46279 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool Review of attachment 46279 [details] [review]: ----------------------------------------------------------------- ::: tools/cleanborrowers.pl @@ +85,5 @@ > > + my $patrons_to_delete; > + if ($patron_list_id) { > + my @patron_list_patrons = > + Koha::Database->new()->schema()->resultset('PatronListPatron') It would be better to use Koha::List::Patron instead. @@ +169,5 @@ > trash => ( $radio eq "trash" ) ? (1) : (0), > testrun => ( $radio eq "testrun" ) ? 1: 0, > ); > +} else { # $step == 1 > + $template->param( patron_lists => [ GetPatronLists() ] ); I'd make more sense to pass the non empty patron lists only. @@ +185,4 @@ > #writing the template > output_html_with_http_headers $cgi, $cookie, $template->output; > > +sub _skip_borrowers_with_nonzero_balance_or_issues { You are modifying the behavior of the tool. This change should be moved to another bug report (misc/cronjobs/delete_patrons.pl will need the change too).
Created attachment 47153 [details] [review] Bug 10612 [QA Followup] 1 - Use Koha::List::Patron 2 - Return only non-empty lists for deletion selection
Created attachment 47224 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches with a check added for issues before deleting borrowers. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com> ByWater internal signoff, still needs signoff from community. Patch seems kosher, passes QA tools. Needs followup to fix one small issue: squished patch undoes switch to FontAwesome icons in lists.tt.
Created attachment 47225 [details] [review] Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt Signed-off-by: Natasha <tasham_8@hotmail.com>
Created attachment 47226 [details] [review] Bug 10612 [QA Followup] 1 - Use Koha::List::Patron 2 - Return only non-empty lists for deletion selection
(In reply to Jonathan Druart from comment #51) > It would be better to use Koha::List::Patron instead. Done > I'd make more sense to pass the non empty patron lists only. Done > You are modifying the behavior of the tool. > This change should be moved to another bug report > (misc/cronjobs/delete_patrons.pl will need the change too). Done, moved to bug 15642 as a dependency. However, looking over the code again I noted that while the tool did use this subroutine initially, it used it in conjunction with C4::Members GetBorrowersToExpunge which checks for borrowers w/issues in the SQl query it builds and eliminates those borrowers from consideration. The new deletion by list feature doesn't use GetBorrowersToExpunge (since it is basing from a list) so needs to make a check for issues. It seems logical enough to add the check in this sub since we are already calling GetMemberIssuesAndFines and really I just pickup the return value instead of ignoring it. So, I can either leave dependent and wait on 15642, close the other bug and replace the code here, or create a new separate sub to check for issues. Thoughts?
I get Undefined subroutine &main::_skip_borrowers_with_nonzero_balance_or_issues called at /home/koha/src/tools/cleanborrowers.pl line 103. then, afer renaming the subroutine correctly: Can't call method "patron_list_patrons" on an undefined value at /home/koha/src/tools/cleanborrowers.pl line 128.
Created attachment 47255 [details] [review] Bug 10612 [QA Followup] 1 - Use Koha::List::Patron 2 - Return only non-empty lists for deletion selection 3 - Fix _skip_bo... to accept patro_list_patrons3 - Fix _skip_bo... to accept patro_list_patrons3 - Fix _skip_bo... to accept patro_list_patrons
Created attachment 47259 [details] [review] Bug 10612 [QA Followup] - fix issues 1 - Use Koha::List::Patron 2 - Return only non-empty lists for deletion selection 3 - Fix _skip_bo... to accept patro_list_patrons3 - Fix _skip_borrowers_with_fines_or_issues to accept patro_list_patrons3 4 - Fix missed closing quote in cleanborrowers.tt
(In reply to Jonathan Druart from comment #57) > Undefined subroutine &main::_skip_borrowers_with_nonzero_balance_or_issues > called at /home/koha/src/tools/cleanborrowers.pl line 103. I don't get this one, did you apply 15642? > Can't call method "patron_list_patrons" on an undefined value at > /home/koha/src/tools/cleanborrowers.pl line 128. Fixed missing closing quote on patron_list_id value which caused this
Blocked by bug 15642.
Created attachment 47393 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches with a check added for issues before deleting borrowers. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com> ByWater internal signoff, still needs signoff from community. Patch seems kosher, passes QA tools. Needs followup to fix one small issue: squished patch undoes switch to FontAwesome icons in lists.tt.
Created attachment 47394 [details] [review] Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt Signed-off-by: Natasha <tasham_8@hotmail.com>
Created attachment 47395 [details] [review] Bug 10612 [QA Followup] - fix issues 1 - Use Koha::List::Patron 2 - Return only non-empty lists for deletion selection 3 - Fix _skip_bo... to accept patro_list_patrons3 - Fix _skip_borrowers_with_fines_or_issues to accept patro_list_patrons3 4 - Fix missed closing quote in cleanborrowers.tt
Created attachment 47396 [details] [review] Bug 10612 - Change _skip_borrowers_with_fines to _skip_borrowers_with_fines_or_issues Restore code that was moved to bug 15642
Created attachment 47478 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed
Nick, it's much more better, but there is something wrong. The change to C4::Members::GetBorrowersToExpunge is not consistent: we expect the subroutine behaves the same for patron list than for other params. For instance, the category_code != 'S' and the guarantor tests are not done if patron list is passed. Moreover, it's not obvious that the patron_list_id is independent from other params. I'd say you should join the patron_list_patrons when the param is given. There is also a lack of tests.
Created attachment 48833 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed
Created attachment 48881 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed
Created attachment 48888 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed
Created attachment 48890 [details] [review] [SIGNED-OFF] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed Signed-off-by: Owen Leonard <oleonard@myacpl.org> Patron batches are correctly passed to the edit and delete pages.
Created attachment 48891 [details] [review] Bug 10612 [Follow-up] Combine and clarify menu items This follow-up patch for Bug 10612 moves all list operations into one menu while (hopefully) clarifying the labels for each. Also in this patch: Changed datatables sorting configuration to use the "nosort" attribute. To test, apply the patch and go to Tools -> Patron lists. - On a patron list with no patrons, confirm that the "Actions" menu lists "Add patrons," "Edit list," and "Delete list." - On a patron list containing one or more patrons, confirm that the "Actions" menu /also/ has these options: "Print patron cards," "Batch edit patrons," and "Batch delete patrons." - Confirm that the "actions" column is not sortable. - Test logging in as a user who has various combinations of permission to batch edit and batch delete.
Created attachment 48892 [details] [review] Bug 10612 [Follow-up] Combine and clarify menu items This follow-up patch for Bug 10612 moves all list operations into one menu while (hopefully) clarifying the labels for each. Also in this patch: Changed datatables sorting configuration to use the "nosort" attribute. To test, apply the patch and go to Tools -> Patron lists. - On a patron list with no patrons, confirm that the "Actions" menu lists "Add patrons," "Edit list," and "Delete list." - On a patron list containing one or more patrons, confirm that the "Actions" menu /also/ has these options: "Print patron cards," "Batch edit patrons," and "Batch delete patrons." - Confirm that the "actions" column is not sortable. - Test logging in as a user who has various combinations of permission to batch edit and batch delete. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment on attachment 48890 [details] [review] [SIGNED-OFF] Bug 10612 - Add ability to delete patrons with batch patron deletion tool Review of attachment 48890 [details] [review]: ----------------------------------------------------------------- ::: tools/cleanborrowers.pl @@ +40,4 @@ > use C4::Members; # GetBorrowersWhoHavexxxBorrowed. > use C4::Circulation; # AnonymiseIssueHistory. > use Koha::DateUtils qw( dt_from_string output_pref ); > +use Koha::Database; Not used. @@ +128,5 @@ > $radio = $params->{'radio'}; > for ( my $i = 0 ; $i < $totalDel ; $i++ ) { > $radio eq 'testrun' && last; > + my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'} > + || $patrons_to_delete->[$i]->get_column('borrowernumber'); I don't understand this line, GetBorrowersToExpunge won't return a DBIx::Class object, so the || $patrons_to_delete->[$i]->get_column('borrowernumber'); part is not necessary. @@ +179,4 @@ > my $borrowers = shift; > my $balance; > @$borrowers = map { > + (undef, undef, $balance) = GetMemberIssuesAndFines( $_->{borrowernumber} || $_->get_column('borrowernumber') ); Same here.
Created attachment 49596 [details] [review] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed Signed-off-by: Owen Leonard <oleonard@myacpl.org> Patron batches are correctly passed to the edit and delete pages.
Created attachment 49597 [details] [review] Bug 10612 [Follow-up] Combine and clarify menu items This follow-up patch for Bug 10612 moves all list operations into one menu while (hopefully) clarifying the labels for each. Also in this patch: Changed datatables sorting configuration to use the "nosort" attribute. To test, apply the patch and go to Tools -> Patron lists. - On a patron list with no patrons, confirm that the "Actions" menu lists "Add patrons," "Edit list," and "Delete list." - On a patron list containing one or more patrons, confirm that the "Actions" menu /also/ has these options: "Print patron cards," "Batch edit patrons," and "Batch delete patrons." - Confirm that the "actions" column is not sortable. - Test logging in as a user who has various combinations of permission to batch edit and batch delete. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #74) > > +use Koha::Database; > Not used. Removed > @@ +128,5 @@ > I don't understand this line, GetBorrowersToExpunge won't return a > DBIx::Class object, so the || > $patrons_to_delete->[$i]->get_column('borrowernumber'); part is not > necessary. > @@ +179,4 @@ > Same here. Both removed
Created attachment 49607 [details] [review] Bug 10612: Follow-up - correcting icon to be Font Awesome - check print icon on the patron list summary page is correct
Repeated my test from comment#45 - this looks much better now. Fixed a FA icon that got undone by the patch set in a tiny follow-up. Please provide tests for the changes made to Members.pm in order for this to move forward.
Created attachment 49694 [details] [review] Bug 10612 - Unit tests
Tested: - patron with fines - not deleted - patron with issues - not deleted - patron who is a guarantor - not deleted - patron who has none of that and is not a guarantor - deleted This looks good now! Tests all pass and are nicely documented. Nick, could you add just one more thing? A test to ensure a patron being a guarantor is not deleted?
Created attachment 49740 [details] [review] Bug 10612 - Unit tests
Created attachment 49741 [details] [review] Bug 10612: (QA followup) Rename not_borrowered_since to not_borrowed_since
Created attachment 49766 [details] [review] [PASSED QA] Bug 10612 - Add ability to delete patrons with batch patron deletion tool This patch adds the ability to select a patron list for deletetion when using the Batch patron deletion/anonymization tool. It also adds buttons to the the patron lists table to access both the batch deletion and batch modification directly from the lists view. This is a squash of previous patches but now adds a patron_list_id parameter to C4::Members::GetBorrowersToExpunge and uses that routine to fetch patrons from a list. Test Plan: 1) Apply this patch 2) Create a list of patrons with the new Patron Lists feature 3) Try using the batch edit link form the lists table 4) Try using the batch delete link from the lists table 5) Verify previous functionality has not changed Signed-off-by: Owen Leonard <oleonard@myacpl.org> Patron batches are correctly passed to the edit and delete pages. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 49767 [details] [review] [PASSED QA] Bug 10612 [Follow-up] Combine and clarify menu items This follow-up patch for Bug 10612 moves all list operations into one menu while (hopefully) clarifying the labels for each. Also in this patch: Changed datatables sorting configuration to use the "nosort" attribute. To test, apply the patch and go to Tools -> Patron lists. - On a patron list with no patrons, confirm that the "Actions" menu lists "Add patrons," "Edit list," and "Delete list." - On a patron list containing one or more patrons, confirm that the "Actions" menu /also/ has these options: "Print patron cards," "Batch edit patrons," and "Batch delete patrons." - Confirm that the "actions" column is not sortable. - Test logging in as a user who has various combinations of permission to batch edit and batch delete. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 49768 [details] [review] [PASSED QA] Bug 10612: Follow-up - correcting icon to be Font Awesome - check print icon on the patron list summary page is correct Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 49769 [details] [review] [PASSED QA] Bug 10612 - Unit tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 49770 [details] [review] [PASSED QA] Bug 10612: (QA followup) Rename not_borrowered_since to not_borrowed_since Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to Master - Should be in the May 2016 release. Thanks!
Tests are failing on Jenkins, but passing when I run them locally: http://jenkins.koha-community.org/job/Koha_master_D7/lastCompletedBuild/testReport/
We need to fix these tests ASAP, a bug may be hidden. I have managed to reproduce the failure on my local install once, but cannot anymore. It would be good to see several fellow trying to: 1. backup the DB 2. prove t/db_dependent/Members.t If the tests fail, keep the backup safe! Reinsert the data and try to recreate the failure. And then debug :) I don't understand the following lines: 295 $builder->build({ 296 source => 'OldIssue', 297 value => { 298 borrowernumber => $bor2inlist, 299 timestamp => '2016-01-01', 300 }, 301 }); Why do we need an entry in the old_issues table? I have commented them but the tests keep passing.