Bug 23822 - Regression: As of 19.05.04 deletion of patrons with outstanding credits is silently blocked
Summary: Regression: As of 19.05.04 deletion of patrons with outstanding credits is si...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 24008
  Show dependency treegraph
 
Reported: 2019-10-16 10:43 UTC by Theodoros Theodoropoulos
Modified: 2019-11-18 08:30 UTC (History)
8 users (show)

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


Attachments
Bug 23822: Fix deletion of patrons with credit (1.34 KB, patch)
2019-11-11 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23822: Fix deletion of patrons with credit (1.41 KB, patch)
2019-11-12 14:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23822: Fix deletion of patrons with credit (1.47 KB, patch)
2019-11-13 14:00 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 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