Bug 23822

Summary: Regression: As of 19.05.04 deletion of patrons with outstanding credits is silently blocked
Product: Koha Reporter: Theodoros Theodoropoulos <theod>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, martin.renvoize, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24007
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05
Bug Depends on:    
Bug Blocks: 24008    
Attachments: Bug 23822: Fix deletion of patrons with credit
Bug 23822: Fix deletion of patrons with credit
Bug 23822: Fix deletion of patrons with credit

Description Theodoros Theodoropoulos 2019-10-16 10:43:10 UTC
When one tries to delete a patron that has a credit in his account, no warning is produced. 
The deletion is not performed (which is the proper behavior), however one would expect to see a warning similar to the one that is produced when the patron has an unpaid fine, a checked out item, a hold, etc.

The following screencast was performed in ByWaters' sandboxes running 19.05 and shows the issue:
https://screencast-o-matic.com/watch/cq6D6vu4YJ
Comment 1 Jonathan Druart 2019-11-11 10:45:26 UTC
This statement:
  my $charges = $patron->account->non_issues_charges;
will return -15 on master and 0 on 19.05.X

Tomas, Martin, what is the expected behavior?
Comment 2 Martin Renvoize 2019-11-11 11:15:35 UTC
(In reply to Jonathan Druart from comment #1)
> This statement:
>   my $charges = $patron->account->non_issues_charges;
> will return -15 on master and 0 on 19.05.X
> 
> Tomas, Martin, what is the expected behavior?

Well.. non_issuing_charges should only return debts, so I'd say master has developed a bug. (The query should include a `AND debit_type_code IS NOT NULL` I believe.)
Comment 3 Martin Renvoize 2019-11-11 11:19:48 UTC
I think this actually exposes some wider questions regarding that routine.. see bug 23293 for example.. it doesn't use the non_issuing_charges method when it probably should (again, my bad :( )
Comment 4 Jonathan Druart 2019-11-11 11:38:21 UTC
Created attachment 95254 [details] [review]
Bug 23822: Fix deletion of patrons with credit

There are bugs in both master and 19.05, but different.

Anyway we should have this check to make sure a negative value will have
the same behavior: trigger the confirmation message (instead of a blank
page).

If we want to reject the deletion of a patron with credit we should
handle it on a separate bug report (behavior change)
Comment 5 Jonathan Druart 2019-11-11 11:39:09 UTC
I would suggest to push this patch into master even if this specific bug does not exist.
Comment 6 Martin Renvoize 2019-11-11 12:35:08 UTC
Updated the bug title to reflect the impact of the change and Jonathans bugfix.. Cloned the original bug to bug 24008 to record the enhancement request to add blocking of patron deletions if outstanding credits are found and additional warnings as this is an additional workflow that needs implementing.
Comment 7 Martin Renvoize 2019-11-12 14:46:40 UTC
Created attachment 95327 [details] [review]
Bug 23822: Fix deletion of patrons with credit

There are bugs in both master and 19.05, but different.

Anyway we should have this check to make sure a negative value will have
the same behavior: trigger the confirmation message (instead of a blank
page).

If we want to reject the deletion of a patron with credit we should
handle it on a separate bug report (behavior change)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-11-12 14:47:24 UTC
A simple patch that restores the original behaviour... Signing off.
Comment 9 Katrin Fischer 2019-11-13 14:00:05 UTC
Created attachment 95388 [details] [review]
Bug 23822: Fix deletion of patrons with credit

There are bugs in both master and 19.05, but different.

Anyway we should have this check to make sure a negative value will have
the same behavior: trigger the confirmation message (instead of a blank
page).

If we want to reject the deletion of a patron with credit we should
handle it on a separate bug report (behavior change)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Martin Renvoize 2019-11-13 16:23:48 UTC
Nice work!

Pushed to master for 19.11.00
Comment 11 Fridolin Somers 2019-11-15 12:33:27 UTC
Shouldn't we also fix the other line :
if ( $op eq 'delete_confirm' or $countissues > 0 or $charges or $is_guarantor ) {
Comment 12 Jonathan Druart 2019-11-18 08:04:34 UTC
(In reply to Fridolin SOMERS from comment #11)
> Shouldn't we also fix the other line :
> if ( $op eq 'delete_confirm' or $countissues > 0 or $charges or
> $is_guarantor ) {

Maybe, we are going to follow-up on bug 24008 anyway.
Comment 13 Fridolin Somers 2019-11-18 08:30:09 UTC
Pushed to 19.05.x for 19.05.05