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.
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.
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.
FAIL Koha/Account/Line.pm FAIL valid Global symbol "$charge_accountline" requires explicit package name (did you forget to declare "my $charge_accountline"?)
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?
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.
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.
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)
Created attachment 122606 [details] [review] Bug 28656: Allow cancellations in the UI This allows cancellation against "reduced" as well as "unpaid" debts.
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
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
+ 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?
+ '-and' => { '!=' => 'WRITEOFF' }, + { '!=' => 'REFUND' }, { '!=' => 'DISCOUNT' } What about: -not_in => [1, 2, 3]
(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 :)
(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 ;)
(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.
(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!
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.
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)
Created attachment 122622 [details] [review] Bug 28656: Allow cancellations in the UI This allows cancellation against "reduced" as well as "unpaid" debts.
$me.amount => { '<' => 0 } $me . amount => { '<' => 0 } Looks like syntax error ;)
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.
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 :)
Needs some adjustment/feedback
Ping Martin