Bug 15642 - Batch patron deletion should not delete patrons with issues
Summary: Batch patron deletion should not delete patrons with issues
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-22 01:37 UTC by Nick Clemens
Modified: 2016-12-05 21:23 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15642 - Batch patron deletion should not delete patrons with issues (1.42 KB, patch)
2016-01-22 02:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15642 - Batch patron deletion should not delete patrons with issues (1.42 KB, patch)
2016-01-22 02:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15642 - Batch patron deletion should not delete patrons with issues (1.42 KB, patch)
2016-01-22 02:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15642 - Batch patron deletion should not delete patrons with issues (1.42 KB, patch)
2016-01-22 02:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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?