Bug 29759 - Refund credit when cancelling an article request
Summary: Refund credit when cancelling an article request
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 27946 29757
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-22 20:33 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 29759: Unit tests (2.41 KB, patch)
2021-12-22 21:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29759: Make cancelling an AR refund (1.97 KB, patch)
2021-12-22 21:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29759: Unit tests (2.47 KB, patch)
2022-01-07 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29759: Make cancelling an AR refund (2.03 KB, patch)
2022-01-07 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29759: Unit tests (2.53 KB, patch)
2022-01-07 14:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29759: Make cancelling an AR refund (2.09 KB, patch)
2022-01-07 14:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29757: (QA follow-up) Remove abs from the test (1.16 KB, patch)
2022-01-11 14:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29759: (QA follow-up) Remove abs from the test (1.16 KB, patch)
2022-01-11 14:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29759: Unit tests (2.62 KB, patch)
2022-01-11 15:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29759: Make cancelling an AR refund (2.19 KB, patch)
2022-01-11 15:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29759: (QA follow-up) Remove abs from the test (1.16 KB, patch)
2022-01-11 15:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29759: (follow-up) Fix TemplateToolkit.t (2.31 KB, patch)
2022-02-02 12:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29759: (follow-up) Fix TemplateToolkit.t (2.38 KB, patch)
2022-02-02 12:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29759: (follow-up) Fix api/v1/article_requests.t (1.82 KB, patch)
2022-02-02 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29759: (follow-up) Fix api/v1/article_requests.t (1.90 KB, patch)
2022-02-02 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-12-22 20:33:52 UTC
Bug 27946 adds the ability to charge a fee when the patron makes an article request. This development will make the fee be reverted. Reversible credits will generate a refund. Methods introduced by bug 29757 will be used.
Comment 1 Tomás Cohen Arazi 2021-12-22 21:21:04 UTC
Created attachment 128874 [details] [review]
Bug 29759: Unit tests
Comment 2 Tomás Cohen Arazi 2021-12-22 21:21:08 UTC
Created attachment 128875 [details] [review]
Bug 29759: Make cancelling an AR refund

This patch makes the Koha::ArticleRequest->cancel method perform a
refund if it applies.

The sequence is:
- Find refundable (potentially partial) payments against the fee
- Generate a refund for the refundable amount
- Reduce the fee to zero
=> RESULT: the patron owes nothing, any credits applied to the debit are
refunded.

1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> FAIL: Nothing is refunded
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Refunds take place!
5. Try on the UI
=> SUCCESS: All good
6. Sign off :-D
Comment 3 Kyle M Hall 2022-01-07 13:29:26 UTC
Created attachment 129162 [details] [review]
Bug 29759: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2022-01-07 13:29:57 UTC
Created attachment 129163 [details] [review]
Bug 29759: Make cancelling an AR refund

This patch makes the Koha::ArticleRequest->cancel method perform a
refund if it applies.

The sequence is:
- Find refundable (potentially partial) payments against the fee
- Generate a refund for the refundable amount
- Reduce the fee to zero
=> RESULT: the patron owes nothing, any credits applied to the debit are
refunded.

1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> FAIL: Nothing is refunded
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Refunds take place!
5. Try on the UI
=> SUCCESS: All good
6. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Martin Renvoize 2022-01-07 14:38:33 UTC
Created attachment 129181 [details] [review]
Bug 29759: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-01-07 14:38:36 UTC
Created attachment 129182 [details] [review]
Bug 29759: Make cancelling an AR refund

This patch makes the Koha::ArticleRequest->cancel method perform a
refund if it applies.

The sequence is:
- Find refundable (potentially partial) payments against the fee
- Generate a refund for the refundable amount
- Reduce the fee to zero
=> RESULT: the patron owes nothing, any credits applied to the debit are
refunded.

1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> FAIL: Nothing is refunded
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Refunds take place!
5. Try on the UI
=> SUCCESS: All good
6. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-01-07 14:56:27 UTC
hmm.. I'm still not 100% sure of this.

Scenario 1
1. Article request placed, fee added
2. Article request cancelled, fee cancelled (currently appears as a refund)

Scenario 2
1. Article request placed, fee added
2. Patron pays full amount
3. Article request cannot be fulfilled so is cancelled
4. Paid fee is refunded.. I feel like this should be linked to the 'payment' rather than the original 'charge' ?

Scenario 3
1. Article request placed, fee added
2. Patron pays some amount
3. Article request is cancelled
4. Patron is 'refunded' the amount paid (but it links to the charge, not the payment)
5. Patron is also 'refunded' the outstanding amount (again, links to the charge)

I think these could end up rather confusing in the accounts screen.
Comment 8 Martin Renvoize 2022-01-07 14:57:11 UTC
This gives us an opportunity to create something consistent for lost item charge refunds too
Comment 9 Tomás Cohen Arazi 2022-01-07 14:59:04 UTC
(In reply to Martin Renvoize from comment #7)
> hmm.. I'm still not 100% sure of this.
> 
> Scenario 1
> 1. Article request placed, fee added
> 2. Article request cancelled, fee cancelled (currently appears as a refund)
> 
> Scenario 2
> 1. Article request placed, fee added
> 2. Patron pays full amount
> 3. Article request cannot be fulfilled so is cancelled
> 4. Paid fee is refunded.. I feel like this should be linked to the 'payment'
> rather than the original 'charge' ?
> 
> Scenario 3
> 1. Article request placed, fee added
> 2. Patron pays some amount
> 3. Article request is cancelled
> 4. Patron is 'refunded' the amount paid (but it links to the charge, not the
> payment)
> 5. Patron is also 'refunded' the outstanding amount (again, links to the
> charge)
> 
> I think these could end up rather confusing in the accounts screen.

Nice catch! I think it is clear this needs to point to the payment!
Comment 10 Marcel de Rooy 2022-01-10 13:04:04 UTC
+    is( abs $patron->account->balance, $payed_amount, 'The patron has been refunded the right value' );

Frowning by intuition. abs ?
Comment 11 Marcel de Rooy 2022-01-10 15:22:59 UTC
Still looking
Comment 12 Marcel de Rooy 2022-01-11 14:33:49 UTC
Created attachment 129297 [details] [review]
Bug 29757: (QA follow-up) Remove abs from the test

Obviously abs has two sides, and we could allow wrong results. We
can be more precise here.
Trivial exercise.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-01-11 14:34:15 UTC
Created attachment 129298 [details] [review]
Bug 29759: (QA follow-up) Remove abs from the test

Obviously abs has two sides, and we could allow wrong results. We
can be more precise here.
Trivial exercise.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-01-11 14:34:36 UTC
Comment on attachment 129297 [details] [review]
Bug 29757: (QA follow-up) Remove abs from the test

Wrong bug number
Comment 15 Marcel de Rooy 2022-01-11 14:35:10 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to Martin Renvoize from comment #7)
> > hmm.. I'm still not 100% sure of this.
> > 
> > Scenario 1
> > 1. Article request placed, fee added
> > 2. Article request cancelled, fee cancelled (currently appears as a refund)
> > 
> > Scenario 2
> > 1. Article request placed, fee added
> > 2. Patron pays full amount
> > 3. Article request cannot be fulfilled so is cancelled
> > 4. Paid fee is refunded.. I feel like this should be linked to the 'payment'
> > rather than the original 'charge' ?
> > 
> > Scenario 3
> > 1. Article request placed, fee added
> > 2. Patron pays some amount
> > 3. Article request is cancelled
> > 4. Patron is 'refunded' the amount paid (but it links to the charge, not the
> > payment)
> > 5. Patron is also 'refunded' the outstanding amount (again, links to the
> > charge)
> > 
> > I think these could end up rather confusing in the accounts screen.
> 
> Nice catch! I think it is clear this needs to point to the payment!

Still looking here
Comment 16 Marcel de Rooy 2022-01-11 14:44:23 UTC
Martin, what about scenario 4:

Fee charged 10
Paid 2
Paid 3
Cancelled

Now we cannot link to "the payment" because we have two payments. We can split the credit because we want to link to all individual payments. But I feel that does not make much sense?
Comment 17 Marcel de Rooy 2022-01-11 15:07:30 UTC
Bit out of scope, but anyway: When I have an AR fee, I pay an amount, and then void the payment on Transactions, I can afterwards issue a refund on the voided credit. Should that really be possible? 
Similarly, I can do Void payment on the Refund. Huh?
Then i can click Pay on Payment from library to patron. Huh?

Not sure if we should allow all these actions which can really mess up the whole balance?
Comment 18 Marcel de Rooy 2022-01-11 15:18:19 UTC
(In reply to Marcel de Rooy from comment #16)
> Martin, what about scenario 4:
> 
> Fee charged 10
> Paid 2
> Paid 3
> Cancelled
> 
> Now we cannot link to "the payment" because we have two payments. We can
> split the credit because we want to link to all individual payments. But I
> feel that does not make much sense?

Having the impression that the argument of confusing transactions definitely applies on voiding individual offsets here. I would go for the cumulative credit that Tomas implemented.
Comment 19 Marcel de Rooy 2022-01-11 15:23:20 UTC
And as a final note:

If we could rework $accountline->cancel someday, we could perhaps move this code out of the article requests domain.

$debit_accountline->cancel();
Cancel a charge. It will mark the debit as 'cancelled' by updating its status to 'CANCELLED'. Charges that have been fully or partially paid cannot be cancelled.

Since we can void payments as well, it would be nice to allow cancelling of partially paid debit account lines. Moving the add_credit/reduce thing to where it really belongs.

In the meantime this looks good enough to me.
Comment 20 Marcel de Rooy 2022-01-11 15:25:16 UTC
Created attachment 129302 [details] [review]
Bug 29759: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2022-01-11 15:25:20 UTC
Created attachment 129303 [details] [review]
Bug 29759: Make cancelling an AR refund

This patch makes the Koha::ArticleRequest->cancel method perform a
refund if it applies.

The sequence is:
- Find refundable (potentially partial) payments against the fee
- Generate a refund for the refundable amount
- Reduce the fee to zero
=> RESULT: the patron owes nothing, any credits applied to the debit are
refunded.

1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> FAIL: Nothing is refunded
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! Refunds take place!
5. Try on the UI
=> SUCCESS: All good
6. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2022-01-11 15:25:24 UTC
Created attachment 129304 [details] [review]
Bug 29759: (QA follow-up) Remove abs from the test

Obviously abs has two sides, and we could allow wrong results. We
can be more precise here.
Trivial exercise.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2022-01-31 07:20:16 UTC
Fridolin: push this one too it belongs to the AR series (on top of 29757)
Comment 24 Fridolin Somers 2022-02-01 07:57:06 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 25 Fridolin Somers 2022-02-02 10:05:38 UTC
Oupsy looks like it breaks the test suite, see :
https://jenkins.koha-community.org/job/Koha_Master_D10/519/
Comment 26 Jonathan Druart 2022-02-02 10:15:08 UTC
t/db_dependent/Letters/TemplateToolkit.t .. 5/28         # No tests run!

    #   Failed test 'No tests run for subtest "AR_*"'
    #   at t/db_dependent/Letters/TemplateToolkit.t line 408.
    # Looks like you planned 8 tests but ran 2.
    # Looks like you failed 1 test of 2 run.
t/db_dependent/Letters/TemplateToolkit.t .. 24/28 
#   Failed test 'regression tests'
#   at t/db_dependent/Letters/TemplateToolkit.t line 1028.
Account line 24 is not a debit# Looks like your test exited with 11 just after 24.
Comment 27 Tomás Cohen Arazi 2022-02-02 12:35:57 UTC
Created attachment 130093 [details] [review]
Bug 29759: (follow-up) Fix TemplateToolkit.t

This patch makes the tests set debit_id to undef instead of random data.
This way tests don't break when $article_request->cancel is invoked and
a refund is tried.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Letters/TemplateToolkit.t
=> FAIL: Boo, tests fail, random garbage is not a debit
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Jonathan Druart 2022-02-02 12:40:05 UTC
Created attachment 130094 [details] [review]
Bug 29759: (follow-up) Fix TemplateToolkit.t

This patch makes the tests set debit_id to undef instead of random data.
This way tests don't break when $article_request->cancel is invoked and
a refund is tried.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Letters/TemplateToolkit.t
=> FAIL: Boo, tests fail, random garbage is not a debit
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Tomás Cohen Arazi 2022-02-02 12:51:03 UTC
Created attachment 130095 [details] [review]
Bug 29759: (follow-up) Fix api/v1/article_requests.t

This patch makes the tests set debit_id to undef instead of random data.
This way tests don't break when $article_request->cancel is invoked and
a refund is tried.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Letters/TemplateToolkit.t
=> FAIL: Boo, tests fail, random garbage is not a debit
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2022-02-02 12:53:32 UTC
Created attachment 130096 [details] [review]
Bug 29759: (follow-up) Fix api/v1/article_requests.t

This patch makes the tests set debit_id to undef instead of random data.
This way tests don't break when $article_request->cancel is invoked and
a refund is tried.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Letters/TemplateToolkit.t
=> FAIL: Boo, tests fail, random garbage is not a debit
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Fridolin Somers 2022-02-02 19:39:59 UTC
New patches pushed to master :
b62fcec5cc Bug 29759: (follow-up) Fix api/v1/article_requests.t
8c80db886a Bug 29759: (follow-up) Fix TemplateToolkit.t