Bug 10612 - Add ability to delete patrons with batch patron deletion tool
Summary: Add ability to delete patrons with batch patron deletion tool
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-18 13:59 UTC by Kyle M Hall
Modified: 2018-06-04 20:10 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10612 - Add ability to delete patrons with batch patron modification tool (17.03 KB, patch)
2013-07-18 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool (17.04 KB, patch)
2013-07-18 14:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool (17.10 KB, patch)
2013-09-13 11:59 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember (6.64 KB, patch)
2013-09-25 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember (6.69 KB, patch)
2013-09-25 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember (6.76 KB, patch)
2013-09-25 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool (16.79 KB, patch)
2013-11-08 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron modification tool - QA Followup - Move member deletion related calls to DelMember (6.76 KB, patch)
2013-11-08 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (11.92 KB, patch)
2014-06-11 12:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 [Code cleanup] - Rename some variables (6.29 KB, patch)
2014-06-11 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables (6.36 KB, patch)
2015-03-02 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10612 - Add ability to delete patrons with batch patron deletion tool (11.99 KB, patch)
2015-03-02 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables (6.36 KB, patch)
2015-03-02 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10612 [Code cleanup] - Rename some variables (6.36 KB, patch)
2015-03-02 14:17 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 10612 [QA Followup] - Remove user icons from buttons (2.23 KB, patch)
2015-07-31 18:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (12.04 KB, patch)
2015-08-05 07:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10612 [Code cleanup] - Rename some variables (6.42 KB, patch)
2015-08-05 07:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10612 [QA Followup] - Remove user icons from buttons (2.30 KB, patch)
2015-08-05 07:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (12.87 KB, patch)
2016-01-04 22:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (13.13 KB, patch)
2016-01-05 20:20 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt (2.68 KB, patch)
2016-01-05 20:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt (2.73 KB, patch)
2016-01-18 23:25 UTC, natasha
Details | Diff | Splinter Review
Bug 10612 [QA Followup] (2.05 KB, patch)
2016-01-22 02:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (12.50 KB, patch)
2016-01-22 21:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt (2.72 KB, patch)
2016-01-22 21:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 [QA Followup] (2.05 KB, patch)
2016-01-22 21:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 [QA Followup] (2.61 KB, patch)
2016-01-25 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 [QA Followup] - fix issues (3.72 KB, patch)
2016-01-25 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (12.50 KB, patch)
2016-01-28 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - [QA followup] Use font awesome icons in patron lists lists.tt (2.72 KB, patch)
2016-01-28 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 [QA Followup] - fix issues (3.76 KB, patch)
2016-01-28 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Change _skip_borrowers_with_fines to _skip_borrowers_with_fines_or_issues (1.02 KB, patch)
2016-01-28 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (14.86 KB, patch)
2016-01-29 20:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.47 KB, patch)
2016-03-09 02:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.49 KB, patch)
2016-03-09 15:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.44 KB, patch)
2016-03-09 16:31 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.60 KB, patch)
2016-03-09 17:13 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10612 [Follow-up] Combine and clarify menu items (6.31 KB, patch)
2016-03-09 17:13 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10612 [Follow-up] Combine and clarify menu items (6.32 KB, patch)
2016-03-09 18:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.11 KB, patch)
2016-03-25 18:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 [Follow-up] Combine and clarify menu items (6.51 KB, patch)
2016-03-25 18:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612: Follow-up - correcting icon to be Font Awesome (1.88 KB, patch)
2016-03-28 20:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10612 - Unit tests (5.66 KB, patch)
2016-03-30 21:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612 - Unit tests (7.27 KB, patch)
2016-03-31 15:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10612: (QA followup) (7.46 KB, patch)
2016-03-31 15:55 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 10612 - Add ability to delete patrons with batch patron deletion tool (15.18 KB, patch)
2016-04-01 05:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10612 [Follow-up] Combine and clarify menu items (6.58 KB, patch)
2016-04-01 05:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10612: Follow-up - correcting icon to be Font Awesome (1.95 KB, patch)
2016-04-01 05:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10612 - Unit tests (7.34 KB, patch)
2016-04-01 05:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10612: (QA followup) (7.52 KB, patch)
2016-04-01 05:52 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-07-18 13:59:51 UTC
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.
Comment 1 Kyle M Hall 2013-07-18 14:06:12 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-18 14:10:07 UTC Comment hidden (obsolete)
Comment 3 Liz Rea 2013-07-29 03:44:14 UTC
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
Comment 4 Kyle M Hall 2013-07-31 15:07:20 UTC
(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 5 Galen Charlton 2013-08-12 20:52:34 UTC
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.
Comment 6 Nora Blake 2013-09-09 13:05:01 UTC
I am extremely happy with this and would like to sign off on it.
Comment 7 Melissa Lefebvre 2013-09-13 11:59:55 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-09-19 10:46:28 UTC
Kyle, could you answer to Galen please (see comment 5).
A call is missing, marked as Failed QA.
Comment 9 Kyle M Hall 2013-09-25 14:16:04 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-09-25 14:17:11 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-09-25 14:18:48 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2013-09-28 10:58:52 UTC
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
Comment 13 Kyle M Hall 2013-11-08 16:53:38 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-11-08 16:53:51 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-11-12 15:38:15 UTC
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?
Comment 16 Jonathan Druart 2013-12-13 12:16:50 UTC
ping Kyle, park to in discussion.
Comment 17 Kyle M Hall 2013-12-13 13:05:24 UTC
(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.
Comment 18 Jonathan Druart 2013-12-13 13:50:31 UTC
(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.
Comment 19 Nora Blake 2013-12-13 15:15:35 UTC
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
Comment 20 Jonathan Druart 2013-12-13 15:49:25 UTC
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.
Comment 21 Kyle M Hall 2014-06-11 12:50:20 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2014-06-11 12:57:14 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2014-07-03 15:55:38 UTC
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.
Comment 24 Kyle M Hall 2014-07-11 16:57:18 UTC
(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.
Comment 25 Nick Clemens 2014-07-13 18:31:16 UTC
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
Comment 26 Owen Leonard 2014-08-06 15:46:00 UTC
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.
Comment 27 Jacek Ablewicz 2014-08-07 08:07:10 UTC
(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).
Comment 28 Owen Leonard 2014-08-07 19:00:09 UTC
(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) ?
Comment 29 Kyle M Hall 2015-03-02 14:12:57 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2015-03-02 14:15:40 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2015-03-02 14:16:11 UTC Comment hidden (obsolete)
Comment 32 Nicole C. Engard 2015-03-02 14:17:34 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2015-04-06 11:12:33 UTC
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.
Comment 34 Kyle M Hall 2015-07-31 18:29:59 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-07-31 18:33:22 UTC
> 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!
Comment 36 Katrin Fischer 2015-08-02 17:13:14 UTC
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.
Comment 37 Jonathan Druart 2015-08-05 07:47:55 UTC
(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
Comment 38 Jonathan Druart 2015-08-05 07:49:23 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2015-08-05 07:49:30 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2015-08-05 07:49:37 UTC Comment hidden (obsolete)
Comment 41 Katrin Fischer 2015-08-05 08:19:39 UTC
Hi Jonathan, thx for testing! - I will rerun my test and see if I did something differently.
Comment 42 Katrin Fischer 2015-09-14 05:58:20 UTC
- 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.
Comment 43 Katrin Fischer 2015-09-14 17:15:38 UTC
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.
Comment 44 Jonathan Druart 2015-09-15 07:11:33 UTC
(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)?
Comment 45 Katrin Fischer 2015-10-21 22:51:19 UTC
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
Comment 46 Nick Clemens 2016-01-04 22:14:39 UTC Comment hidden (obsolete)
Comment 47 Nick Clemens 2016-01-04 22:17:04 UTC
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
Comment 48 Jesse Weaver 2016-01-05 20:20:35 UTC Comment hidden (obsolete)
Comment 49 Nick Clemens 2016-01-05 20:45:01 UTC Comment hidden (obsolete)
Comment 50 natasha 2016-01-18 23:25:16 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2016-01-19 11:41:43 UTC
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).
Comment 52 Nick Clemens 2016-01-22 02:11:10 UTC Comment hidden (obsolete)
Comment 53 Nick Clemens 2016-01-22 21:57:55 UTC Comment hidden (obsolete)
Comment 54 Nick Clemens 2016-01-22 21:58:01 UTC Comment hidden (obsolete)
Comment 55 Nick Clemens 2016-01-22 21:59:03 UTC Comment hidden (obsolete)
Comment 56 Nick Clemens 2016-01-22 22:05:09 UTC
(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?
Comment 57 Jonathan Druart 2016-01-25 11:08:46 UTC
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.
Comment 58 Nick Clemens 2016-01-25 13:59:11 UTC Comment hidden (obsolete)
Comment 59 Nick Clemens 2016-01-25 14:37:20 UTC Comment hidden (obsolete)
Comment 60 Nick Clemens 2016-01-25 14:40:20 UTC
(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
Comment 61 Jonathan Druart 2016-01-27 14:34:02 UTC
Blocked by bug 15642.
Comment 62 Nick Clemens 2016-01-28 16:05:15 UTC Comment hidden (obsolete)
Comment 63 Nick Clemens 2016-01-28 16:05:23 UTC Comment hidden (obsolete)
Comment 64 Nick Clemens 2016-01-28 16:05:30 UTC Comment hidden (obsolete)
Comment 65 Nick Clemens 2016-01-28 16:05:43 UTC Comment hidden (obsolete)
Comment 66 Nick Clemens 2016-01-29 20:36:42 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2016-02-01 10:32:32 UTC
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.
Comment 68 Nick Clemens 2016-03-09 02:57:05 UTC Comment hidden (obsolete)
Comment 69 Nick Clemens 2016-03-09 15:48:34 UTC Comment hidden (obsolete)
Comment 70 Nick Clemens 2016-03-09 16:31:30 UTC Comment hidden (obsolete)
Comment 71 Owen Leonard 2016-03-09 17:13:20 UTC Comment hidden (obsolete)
Comment 72 Owen Leonard 2016-03-09 17:13:49 UTC Comment hidden (obsolete)
Comment 73 Nick Clemens 2016-03-09 18:46:56 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2016-03-11 08:36:52 UTC
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.
Comment 75 Nick Clemens 2016-03-25 18:34:35 UTC Comment hidden (obsolete)
Comment 76 Nick Clemens 2016-03-25 18:34:41 UTC Comment hidden (obsolete)
Comment 77 Nick Clemens 2016-03-25 18:36:34 UTC
(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
Comment 78 Katrin Fischer 2016-03-28 20:32:50 UTC Comment hidden (obsolete)
Comment 79 Katrin Fischer 2016-03-28 20:35:30 UTC
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.
Comment 80 Nick Clemens 2016-03-30 21:07:51 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2016-03-30 22:16:15 UTC
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?
Comment 82 Nick Clemens 2016-03-31 15:41:17 UTC Comment hidden (obsolete)
Comment 83 Nick Clemens 2016-03-31 15:55:59 UTC Comment hidden (obsolete)
Comment 84 Katrin Fischer 2016-04-01 05:51:23 UTC
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>
Comment 85 Katrin Fischer 2016-04-01 05:51:57 UTC
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>
Comment 86 Katrin Fischer 2016-04-01 05:52:01 UTC
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>
Comment 87 Katrin Fischer 2016-04-01 05:52:06 UTC
Created attachment 49769 [details] [review]
[PASSED QA] Bug 10612 - Unit tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 88 Katrin Fischer 2016-04-01 05:52:10 UTC
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>
Comment 89 Brendan Gallagher 2016-04-01 20:02:45 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 90 Katrin Fischer 2016-04-03 11:52:03 UTC
Tests are failing on Jenkins, but passing when I run them locally:
http://jenkins.koha-community.org/job/Koha_master_D7/lastCompletedBuild/testReport/
Comment 91 Jonathan Druart 2016-04-18 12:10:06 UTC
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.