Bug 28656 - Cancellations should be allowed on paid/partially paid debts
Summary: Cancellations should be allowed on paid/partially paid debts
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 22435 29757
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-02 14:41 UTC by Martin Renvoize
Modified: 2021-12-22 21:22 UTC (History)
2 users (show)

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


Attachments
Bug 28656: Update the cancel method for paid debts (5.41 KB, patch)
2021-07-02 15:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Update the cancel method for paid debts (5.40 KB, patch)
2021-07-05 12:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Update the cancel method for paid debts (5.27 KB, patch)
2021-07-06 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Improve the transaction history (10.60 KB, patch)
2021-07-06 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Allow cancellations in the UI (2.11 KB, patch)
2021-07-06 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Update the cancel method for paid debts (5.27 KB, patch)
2021-07-06 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Improve the transaction history (9.79 KB, patch)
2021-07-06 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28656: Allow cancellations in the UI (2.78 KB, patch)
2021-07-06 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-07-02 14:41:11 UTC
It would be helpful to be able to still cancel a debt, even if it's already been partially or fully paid or written off.

If we update the method to allow this case, we can then use it internally for the refund of lost items found debts and other such use cases... see bug 27946 for such a case.
Comment 1 Martin Renvoize 2021-07-02 15:12:58 UTC
Created attachment 122551 [details] [review]
Bug 28656: Update the cancel method for paid debts

This patch updates the cancel method to allow cancellation of debts that
have been partially or fully paid off.  Payments will be included in the
cancellation amount allowing for later use as credit. Discounts,
Writeoffs and Refunds are excluded from the cancellation amount.
Comment 2 Martin Renvoize 2021-07-02 15:17:43 UTC
Initial concept attached.. tests need updating, as do existing callers.

I think we could certainly start by using this in the trigger in Koha::Item for when a lost item is found.
Comment 3 Marcel de Rooy 2021-07-05 06:36:47 UTC
 FAIL   Koha/Account/Line.pm
   FAIL   valid
                Global symbol "$charge_accountline" requires explicit package name (did you forget to declare "my $charge_accountline"?)
Comment 4 Marcel de Rooy 2021-07-05 08:47:46 UTC
Copied Martin's comment from 27946
> There's a little debate going on as to how we should record the cancellation over-credit if you cancel a debt in full when some of that debt has already been paid.. should we just create the single 'cancel' credit, offset it up to the amount outstanding on the original debt and then allow the rest of the outstanding to be 'paid out' or 'applied' to other debts.. or do we split the cancellation into 'cancellation' and 'overpayment credit'...

Does that really matter still? As I understand, the offsets table now offers you the info to 'replay what happened'.
If we would end up with a bunch of accountlines that sum up to zero in the end, how does Koha actually close these lines instead of keeping them open forever while they could be done away with?
Comment 5 Martin Renvoize 2021-07-05 12:32:27 UTC
Created attachment 122585 [details] [review]
Bug 28656: Update the cancel method for paid debts

This patch updates the cancel method to allow cancellation of debts that
have been partially or fully paid off.  Payments will be included in the
cancellation amount allowing for later use as credit. Discounts,
Writeoffs and Refunds are excluded from the cancellation amount.
Comment 6 Martin Renvoize 2021-07-06 08:51:17 UTC
Created attachment 122604 [details] [review]
Bug 28656: Update the cancel method for paid debts

This patch updates the cancel method to allow cancellation of debts that
have been partially or fully paid off.  Payments will be included in the
cancellation amount allowing for later use as credit. Discounts,
Writeoffs and Refunds are excluded from the cancellation amount.
Comment 7 Martin Renvoize 2021-07-06 08:51:20 UTC
Created attachment 122605 [details] [review]
Bug 28656: Improve the transaction history

This patch improves the transaction history for a cancellation action.

Test plan
1/ Set up a complex transaction that takes in various actions against an
accountline; for example
1a/ Add a manual debt for 25.00
1b/ Discount that debt by 5.00
1c/ Writeoff 2.50 from that debt
1d/ Writeoff 7.50 from that debt
1e/ Void the 7.40 Writeoff
1f/ Pay 3.50 of that debt
2/ Cancel the debt
3/ Looks at the 'Details' view for the debt
3a/ Confirm that all the original actions appear followed by the
cancellation amount of the original debt amount minus any reductions
(discounts, writeoffs)
3b/ After the cancellation line you should see some reversals for
non-reduction type offsets (i.e. payments)
Comment 8 Martin Renvoize 2021-07-06 08:51:24 UTC
Created attachment 122606 [details] [review]
Bug 28656: Allow cancellations in the UI

This allows cancellation against "reduced" as well as "unpaid" debts.
Comment 9 Martin Renvoize 2021-07-06 08:59:04 UTC
Still needs tests updating.

I'm not 100% sure about the mechanism if I'm honest... re-embursing the patron now has two differing options that have pretty different outcomes.. I almost feel like we should offer a system preference for the two ways of recording the history.

With this patch, the cancel method will 'reverse' any applied payment offsets.. thus giving back any outstanding balance to the original credit. (In the UI we actually still prevent 'Cancellation' on lines that have any such offsets however.. we only allow cancellation if no payments have been made but we do now allow discounted/written off debts to be cancelled).

The 'refund' option will add a refund accountline and immediately apply it to the debt.. any outstanding balance is then left on the 'refund' line which is returned from the method.. which can then be 'paid out'.  I'm was thinking it would be nice to split the 'refund' and 'refund credit'.  So this doesn't 'reverse' payments.. it adds a new 'credit' instead.. which can be immediately paid out.

Thoughts
Comment 10 Marcel de Rooy 2021-07-06 11:17:06 UTC
 FAIL   Koha/Account/Offsets.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   FAIL   pod
                *** ERROR: empty =head3  in file Koha/Account/Offsets.pm
                *** ERROR: empty =head3  in file Koha/Account/Offsets.pm
   FAIL   pod coverage
                POD is missing for filter_by_non_reversable
                POD is missing for filter_by_reversable
Comment 11 Marcel de Rooy 2021-07-06 11:25:12 UTC
+                type      => { '!=' => [ 'Writeoff', 'REFUND', 'DISCOUNT' ] }

+      $self->credits( { credit_type_code => [ 'WRITEOFF', 'DISCOUNT', 'REFUND' ] } )->total;

Any reason for case differences here?
If these three types make a special group, should we move that to a routine instead of hardcoding them here and there?
Comment 12 Marcel de Rooy 2021-07-06 11:33:22 UTC
+            '-and' => { '!=' => 'WRITEOFF' },
+            { '!=' => 'REFUND' }, { '!=' => 'DISCOUNT' }

What about:
-not_in => [1, 2, 3]
Comment 13 Marcel de Rooy 2021-07-06 11:59:14 UTC
(In reply to Martin Renvoize from comment #9)
> Still needs tests updating.
> 
> I'm not 100% sure about the mechanism if I'm honest... re-embursing the
> patron now has two differing options that have pretty different outcomes.. I
> almost feel like we should offer a system preference for the two ways of
> recording the history.
> 
> With this patch, the cancel method will 'reverse' any applied payment
> offsets.. thus giving back any outstanding balance to the original credit.
> (In the UI we actually still prevent 'Cancellation' on lines that have any
> such offsets however.. we only allow cancellation if no payments have been
> made but we do now allow discounted/written off debts to be cancelled).
> 
> The 'refund' option will add a refund accountline and immediately apply it
> to the debt.. any outstanding balance is then left on the 'refund' line
> which is returned from the method.. which can then be 'paid out'.  I'm was
> thinking it would be nice to split the 'refund' and 'refund credit'.  So
> this doesn't 'reverse' payments.. it adds a new 'credit' instead.. which can
> be immediately paid out.
> 
> Thoughts

Still need to get a better understanding of what is happening in Accounts. But intuitively the whole operation seems too complicated to me now. As a first observation :)
Comment 14 Marcel de Rooy 2021-07-06 12:11:06 UTC
(In reply to Marcel de Rooy from comment #4)

> If we would end up with a bunch of accountlines that sum up to zero in the
> end, how does Koha actually close these lines instead of keeping them open
> forever while they could be done away with?

misc/cronjobs/reconcile_balances ;)
Comment 15 Martin Renvoize 2021-07-06 12:25:28 UTC
(In reply to Marcel de Rooy from comment #11)
> +                type      => { '!=' => [ 'Writeoff', 'REFUND', 'DISCOUNT' ]
> }
> 
> +      $self->credits( { credit_type_code => [ 'WRITEOFF', 'DISCOUNT',
> 'REFUND' ] } )->total;
> 
> Any reason for case differences here?

Legacy: Offsets had 'Writeoff' but accountlines had 'WRITEOFF'.. we switch from using the offset type to using the accountline type in this line (as is expected with bug 22435)

> If these three types make a special group, should we move that to a routine
> instead of hardcoding them here and there?

Indeed, I started working on that as can be seen in the filter_by's added to Koha::Account::Offsets.. I can now start using those and add proper tests.
Comment 16 Martin Renvoize 2021-07-06 12:26:10 UTC
(In reply to Marcel de Rooy from comment #12)
> +            '-and' => { '!=' => 'WRITEOFF' },
> +            { '!=' => 'REFUND' }, { '!=' => 'DISCOUNT' }
> 
> What about:
> -not_in => [1, 2, 3]

Damn.. why didn't I spot that!
Comment 17 Martin Renvoize 2021-07-06 13:11:17 UTC
Created attachment 122620 [details] [review]
Bug 28656: Update the cancel method for paid debts

This patch updates the cancel method to allow cancellation of debts that
have been partially or fully paid off.  Payments will be included in the
cancellation amount allowing for later use as credit. Discounts,
Writeoffs and Refunds are excluded from the cancellation amount.
Comment 18 Martin Renvoize 2021-07-06 13:11:23 UTC
Created attachment 122621 [details] [review]
Bug 28656: Improve the transaction history

This patch improves the transaction history for a cancellation action.

Test plan
1/ Set up a complex transaction that takes in various actions against an
accountline; for example
1a/ Add a manual debt for 25.00
1b/ Discount that debt by 5.00
1c/ Writeoff 2.50 from that debt
1d/ Writeoff 7.50 from that debt
1e/ Void the 7.40 Writeoff
1f/ Pay 3.50 of that debt
2/ Cancel the debt
3/ Looks at the 'Details' view for the debt
3a/ Confirm that all the original actions appear followed by the
cancellation amount of the original debt amount minus any reductions
(discounts, writeoffs)
3b/ After the cancellation line you should see some reversals for
non-reduction type offsets (i.e. payments)
Comment 19 Martin Renvoize 2021-07-06 13:11:27 UTC
Created attachment 122622 [details] [review]
Bug 28656: Allow cancellations in the UI

This allows cancellation against "reduced" as well as "unpaid" debts.
Comment 20 Marcel de Rooy 2021-07-06 13:46:01 UTC
        $me.amount                    => { '<' => 0 }
        $me . amount    => { '<' => 0 }

Looks like syntax error ;)
Comment 21 Marcel de Rooy 2021-07-06 13:47:56 UTC
Something triggered
[2021/07/06 13:41:34] [WARN] CGI::param called in list context from /usr/share/koha/members/boraccount.pl line 57, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 412.
Comment 22 Marcel de Rooy 2021-07-06 14:11:54 UTC
can_be_cancelled:     return $self->debit_offsets->filter_by_reversable->count ? 1 : 0;
What is the exact reasoning behind this?
Dumb question: but if there is no reversible offset yet, this does not necessarily mean that the line cant be cancelled??


And that combined with the following template code:

[% IF account.is_debit && account.status != 'CANCELLED' && !(account.debit_type_code == 'PAYOUT') && !(account.debit_type_code == 'VOID') && !account.can_be_cancelled %]
          <form method="post" action="/cgi-bin/koha/members/cancel-charge.pl">

I am reading: If this account line can NOT be cancelled, then present a cancel form ?

Slightly confusing :)
Comment 23 Marcel de Rooy 2021-07-06 14:13:28 UTC
Needs some adjustment/feedback
Comment 24 Marcel de Rooy 2021-08-06 05:51:04 UTC
Ping Martin