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.
Created attachment 128874 [details] [review] Bug 29759: Unit tests
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
Created attachment 129162 [details] [review] Bug 29759: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
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>
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.
This gives us an opportunity to create something consistent for lost item charge refunds too
(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!
+ is( abs $patron->account->balance, $payed_amount, 'The patron has been refunded the right value' ); Frowning by intuition. abs ?
Still looking
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>
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 on attachment 129297 [details] [review] Bug 29757: (QA follow-up) Remove abs from the test Wrong bug number
(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
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?
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?
(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.
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.
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>
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>
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>
Fridolin: push this one too it belongs to the AR series (on top of 29757)
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Oupsy looks like it breaks the test suite, see : https://jenkins.koha-community.org/job/Koha_Master_D10/519/
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.
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>
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>
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>
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>
New patches pushed to master : b62fcec5cc Bug 29759: (follow-up) Fix api/v1/article_requests.t 8c80db886a Bug 29759: (follow-up) Fix TemplateToolkit.t