Bug 15642

Summary: Batch patron deletion should not delete patrons with issues
Product: Koha Reporter: Nick Clemens <nick>
Component: ToolsAssignee: Galen Charlton <gmcharlt>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10612
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 15642 - Batch patron deletion should not delete patrons with issues
Bug 15642 - Batch patron deletion should not delete patrons with issues
Bug 15642 - Batch patron deletion should not delete patrons with issues
Bug 15642 - Batch patron deletion should not delete patrons with issues

Description Nick Clemens 2016-01-22 01:37:38 UTC
Batch deletion of patrons should not include patron with checkouts
Comment 1 Nick Clemens 2016-01-22 02:05:55 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-01-22 02:12:48 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2016-01-22 02:13:41 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2016-01-22 02:17:33 UTC
Created attachment 47156 [details] [review]
Bug 15642 - Batch patron deletion should not delete patrons with issues

This patch changes _skip_borrowers_with_nonzero_balance to
_skip_borrowers_with_nonzero_balance_or_issues and excludes borrowrs
with issues from the batch deletion tool

(this change brings tools/cleanborrowers.pl into line with
 misc/cronjobs/delete_patrons.pl with uses C4::Members
 GetBorrowersToExpunge which excludes patrons with current issues)
Comment 5 Jonathan Druart 2016-01-27 14:30:53 UTC
Nick, tools/cleanborrowers.pl already call GetBorrowersToExpunge, so I am not sure this change is needed.
Comment 6 Jonathan Druart 2016-01-27 14:33:29 UTC
What's the point of this _skip_borrowers_with_nonzero_balance subroutine actually? :)

From my understanding, it removes the patrons with charges.

In misc/cronjobs/delete_patrons.pl it's done with:

 68     my $flags = C4::Members::patronflags( $member );
 69     if ( my $charges = $flags->{CHARGES}{amount} ) {
 70         say "Failed to delete patron $borrowernumber: patron has $charges in fines";
 71         next;
 72     }

Should not we simply move this check to the GetBorrowersToExpunge subroutine?
Comment 7 Nick Clemens 2016-01-28 15:43:39 UTC
> Nick, tools/cleanborrowers.pl already call GetBorrowersToExpunge, so I am
> not sure this change is needed.
Agreed, not needed here, but needed on bug 10612, will move back over there
Comment 8 Nick Clemens 2016-01-28 15:47:15 UTC
(In reply to Jonathan Druart from comment #6)
> In misc/cronjobs/delete_patrons.pl it's done with:
>  68     my $flags = C4::Members::patronflags( $member );
>  69     if ( my $charges = $flags->{CHARGES}{amount} ) {
>  70         say "Failed to delete patron $borrowernumber: patron has
> $charges in fines";
>  71         next;
>  72     }
> Should not we simply move this check to the GetBorrowersToExpunge subroutine?
FWIW I think I would rather see the checks done as above - so that the warning is visible that not all patrons in the category/list/etc were deleted.

If the checks were moved into GetBorrowersToExpunge it would be nice to make it return patrons who wont be deleted because of issues/fines

This is probably a separate enhancement though and I think this report can be closed.  Agreed?