Bug 23051 - Add ability to optionally renew fine accruing items when all fines on item are paid off
Summary: Add ability to optionally renew fine accruing items when all fines on item ar...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 25508 25717
  Show dependency treegraph
 
Reported: 2019-06-05 13:13 UTC by Andrew Isherwood
Modified: 2020-07-20 15:12 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
With the addition of a new `RenewAccruingItemWhenPaid` system preference, we gain the ability to automatically renew items with accruing fines at the point of payment of those fines. **New system preferences**: `RenewAccruingItemWhenPaid` and `RenewAccruingItemInOpac` both default to disabled.
Version(s) released in:
20.05.00


Attachments
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.54 KB, patch)
2019-06-13 07:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.33 KB, patch)
2019-06-13 07:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.97 KB, patch)
2019-06-13 07:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.54 KB, patch)
2019-06-28 08:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.33 KB, patch)
2019-06-28 08:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (8.04 KB, patch)
2019-06-28 08:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.20 KB, patch)
2019-07-26 07:31 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.54 KB, patch)
2019-07-26 07:32 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (8.04 KB, patch)
2019-07-26 07:32 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.20 KB, patch)
2019-07-26 07:32 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.54 KB, patch)
2019-09-10 11:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.96 KB, patch)
2019-09-10 11:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.20 KB, patch)
2019-09-10 11:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2019-09-10 11:13 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 23051: Add unit tests (8.06 KB, patch)
2019-09-10 11:13 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.29 KB, patch)
2019-09-10 11:13 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2019-10-09 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.86 KB, patch)
2019-10-09 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.22 KB, patch)
2019-10-09 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor renewal code (6.88 KB, patch)
2019-10-09 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2019-10-11 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.86 KB, patch)
2019-10-11 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.22 KB, patch)
2019-10-11 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor renewal code (7.38 KB, patch)
2019-10-11 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Provide feedback (18.47 KB, patch)
2019-10-11 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix unit tests (2.22 KB, patch)
2019-10-18 09:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix/improve feedback (13.00 KB, patch)
2019-10-21 11:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor methods (13.42 KB, patch)
2019-10-21 15:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor methods (16.74 KB, patch)
2019-10-22 11:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.86 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.22 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor renewal code (7.38 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Provide feedback (18.46 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix unit tests (2.22 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix/improve feedback (13.01 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Refactor methods (16.74 KB, patch)
2019-10-23 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Add SIP summary (5.41 KB, patch)
2019-10-23 14:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Renewing in OPAC (8.88 KB, patch)
2019-10-24 10:12 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2019-11-19 14:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.86 KB, patch)
2019-11-19 14:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.18 KB, patch)
2019-11-19 14:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Squash of sign off follow-ups (42.35 KB, patch)
2019-11-19 14:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2020-03-04 14:29 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add unit tests (7.90 KB, patch)
2020-03-04 14:29 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.13 KB, patch)
2020-03-04 14:29 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Squash of sign off follow-ups (43.56 KB, patch)
2020-03-04 14:29 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.63 KB, patch)
2020-03-04 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: Add unit tests (8.51 KB, patch)
2020-03-04 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.14 KB, patch)
2020-03-04 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: Squash of sign off follow-up (42.30 KB, patch)
2020-03-04 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (QA follow-up) Missing curly and tabs and fix test (8.82 KB, patch)
2020-03-04 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix renew summary display (1.36 KB, patch)
2020-03-04 15:12 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: (follow-up) Remove call to GetFines (1.15 KB, patch)
2020-03-05 10:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23051: Add RenewAccruingItemWhenPaid syspref (3.69 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: Add unit tests (8.56 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: Renew items when fines paid off (9.19 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (follow-up) Add renewal feedback and move code to subroutines and test (42.40 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (QA follow-up) Missing curly and tabs and fix test (8.87 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (follow-up) Fix renew summary display (1.41 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (follow-up) Remove call to GetFines (1.21 KB, patch)
2020-03-05 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23051: (RM follow-up) Fix rebase error (1.23 KB, patch)
2020-03-06 13:15 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 Andrew Isherwood 2019-06-05 13:13:41 UTC
Currently, if a user pays an accruing fine on an overdue item but forgets to renew the item fines start accruing again. 

If a user pays off the amount outstanding on an item with an accruing fine (accounttype "OVERDUE" & status "UNRETURNED" in 19.05) via the OPAC or self-service, or a member of staff takes payment in the staff client, attempt to automatically renew the item according to circulation rules.

This behaviour should be controlled by a new syspref "RenewAccruingItemWhenPaid" which is initially turned off.
Comment 1 Andrew Isherwood 2019-06-13 07:44:29 UTC
Created attachment 90559 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"
Comment 2 Andrew Isherwood 2019-06-13 07:44:32 UTC
Created attachment 90560 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.
Comment 3 Andrew Isherwood 2019-06-13 07:44:34 UTC
Created attachment 90561 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug
Comment 4 Andrew Isherwood 2019-06-13 07:55:43 UTC
Test plan:

- In Administration, search for the new syspref "RenewAccruingItemWhenPaid"
- TEST: Observe that the new syspref exists and is turned off by default
- Turn on the syspref
- Loan an item to a patron, ensure you choose a patron, branch and item combination that will incur overdue fines
- Backdate the due date to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- Go to the patron's "Accounting" page and choose "Pay" against the item that has the fine
- Enter an amount to pay that will fully pay off the item and click "Confirm"
- Go to the patron's checkouts list
- TEST: Observe that the item has been renewed as part of the fine payment process
Comment 5 Andrew Isherwood 2019-06-28 08:10:44 UTC
Created attachment 91058 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"
Comment 6 Andrew Isherwood 2019-06-28 08:10:47 UTC
Created attachment 91059 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.
Comment 7 Andrew Isherwood 2019-06-28 08:10:50 UTC
Created attachment 91060 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug
Comment 8 Alex Buckley 2019-07-26 00:53:38 UTC
Hi there,

I am sorry this patch does not apply cleanly. 

vagrant@kohadevbox:kohaclone((b558895...))$ git bz apply 23051
Bug 23051 - Optionally renew fine accruing items when all fines on item are paid off

91058 - Bug 23051: Add RenewAccruingItemWhenPaid syspref
91059 - Bug 23051: Renew items when fines paid off
91060 - Bug 23051: Add unit tests

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23051: Add RenewAccruingItemWhenPaid syspref
Applying: Bug 23051: Renew items when fines paid off
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	Koha/Account.pm
M	Koha/Account/Line.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Account/Line.pm
Auto-merging Koha/Account.pm
CONFLICT (content): Merge conflict in Koha/Account.pm
Auto-merging C4/Circulation.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 23051: Renew items when fines paid off
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-23051-Renew-items-when-fines-paid-off-YTUZjH.patch

Can you please rebase it and re-attach it to this bug report, thanks for your contribution :)

Cheers,
Alex
Comment 9 Andrew Isherwood 2019-07-26 07:31:32 UTC
Created attachment 91792 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.
Comment 10 Andrew Isherwood 2019-07-26 07:32:09 UTC
Created attachment 91793 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"
Comment 11 Andrew Isherwood 2019-07-26 07:32:12 UTC
Created attachment 91794 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug
Comment 12 Andrew Isherwood 2019-07-26 07:32:15 UTC
Created attachment 91795 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.
Comment 13 Andrew Isherwood 2019-07-26 07:33:14 UTC
Thanks for the heads up Alex. This should be fine now :)
Comment 14 Andrew Isherwood 2019-09-10 11:07:54 UTC
Created attachment 92684 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"
Comment 15 Andrew Isherwood 2019-09-10 11:07:58 UTC
Created attachment 92685 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug
Comment 16 Andrew Isherwood 2019-09-10 11:08:03 UTC
Created attachment 92686 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.
Comment 17 PTFS Europe Sandboxes 2019-09-10 11:13:30 UTC
Created attachment 92687 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 18 PTFS Europe Sandboxes 2019-09-10 11:13:34 UTC
Created attachment 92688 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 19 PTFS Europe Sandboxes 2019-09-10 11:13:39 UTC
Created attachment 92689 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 20 Nick Clemens 2019-10-09 12:24:08 UTC
Hi Andrew,

Tests need a minor rebase to pass.

I have a few questions from reading the code:

1 - You duplicate the code in Account->pay and Line->apply, could that be a subroutine that can be shared and tested? It's minor but would be nice to avoid duplication

2 - I don't like the lack of feedback for the renewal/failure. At the very least maybe some feedback on the interface that the items may be renewed

3 - In the case where a renewal doesn't make the book current (i.e. the book is overdue longer than the renewal period) I think this would give strange results. Possibly you could consider this a configuration issue, but it should be warned somewhere at the least
Comment 21 Andrew Isherwood 2019-10-09 14:19:11 UTC
Created attachment 93922 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 22 Andrew Isherwood 2019-10-09 14:19:15 UTC
Created attachment 93923 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 23 Andrew Isherwood 2019-10-09 14:19:19 UTC
Created attachment 93924 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 24 Andrew Isherwood 2019-10-09 14:19:22 UTC
Created attachment 93925 [details] [review]
Bug 23051: (follow-up) Refactor renewal code

As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.
Comment 25 Andrew Isherwood 2019-10-09 14:39:18 UTC
Hi Nick

Thanks for taking a look at this.

>Tests need a minor rebase to pass.
Fixed

>You duplicate the code in Account->pay and Line->apply [..]
Indeed. I've now refactored the duplicated code into a couple of helper functions in Koha::Account.

>I don't like the lack of feedback for the renewal/failure [..]
This is a very tricky issue, and one I side-stepped during dev. Firstly, any feedback would only be available in the UI. Since there is more than one way of paying off fines that don't involve the Koha UI (e.g. SIP), those methods would not receive the feedback. Also, since the renewal is happening as a side effect of paying off a fine, anything that was saying "pay off the fine on this item" will have to be expecting something entirely unrelated, i.e. "this item couldn't renew" back, which feels wrong. I do agree that no feedback at all is bad though. I'll have a think and see if I can come up with.

>In the case where a renewal doesn't make the book current [..]
Same as the previous one really.

I'll do some pondering...
Comment 26 Andreas Hedström Mace 2019-10-11 09:13:40 UTC
I don't understand Nick's third point. Any attempted renewal should base that renewal on today's date and calculate the new due date based on the circulation rule for the item from that? Not the original due date. (At least that is what happens if I renew in our version of Koha, 18.11.)

So it could never happen that the item is not "current" after a renewal? Or am I reading that wrong?
Comment 27 Andrew Isherwood 2019-10-11 09:17:58 UTC
Hi Andreas

I believe this is determined by the "RenewalPeriodBase" syspref, if set to "the old due date of the checkout" then the scenario Nick describes could happpen.
Comment 28 Andreas Hedström Mace 2019-10-11 09:24:49 UTC
Ah, thanks! I had forgotten about that one.
Comment 29 Andrew Isherwood 2019-10-11 16:09:32 UTC
Created attachment 94020 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 30 Andrew Isherwood 2019-10-11 16:09:35 UTC
Created attachment 94021 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 31 Andrew Isherwood 2019-10-11 16:09:41 UTC
Created attachment 94022 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 32 Andrew Isherwood 2019-10-11 16:09:44 UTC
Created attachment 94023 [details] [review]
Bug 23051: (follow-up) Refactor renewal code

As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.
Comment 33 Andrew Isherwood 2019-10-11 16:09:48 UTC
Created attachment 94024 [details] [review]
Bug 23051: (follow-up) Provide feedback

For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.

Addresses the second point in Nick's comment #20
Comment 34 Andrew Isherwood 2019-10-11 16:19:06 UTC
Hi Nick

Further to the discussion on your third point. Personally I think this is outside of the scope of this work.

In the situation where the "RenewalPeriodBase" syspref is set to "the old due date of the checkout" and an outstanding fine on it is paid off, the item would be renewed but, as you say, it may still be overdue if the old due date is further in the past than the renewal period. However, this is also the case if a regular renewal is carried out in the Circulation module, no warning is given in this situation, so an item can be renewed but still be overdue.

I agree that this is not an ideal situation, but I feel that if such a warning is to be added, it should be done as a separate bug which addresses the (probably numerous) places where such a warning would need to be displayed.
Comment 35 Nick Clemens 2019-10-16 14:58:40 UTC
t/db_dependent/Koha/Account.t
  - test count needs adjustment
t/db_dependent/Koha/Account/Lines.t
  - tests are failing

The feedback is improved, it would be nice to get feedback on what was successfully renewed as well. Ideally SIP would get feedback in a screen message, and patrons would get a notice of renewal on the opac. I am also not sure about the code path if a patron paid fines on the opac (via paypal etc.) but renewals are not allowed on the opac. 

In general I think I would prefer to see 'ok' and 'not_ok' returned as a single 'renewal_results' array, then it can be parsed once and success and failure returned. Our trainers would very much like there to be feedback for any renewal, otherwise it feels a bit magic and unpredictable

There is no listing of errors if I use the 'pay' button on an individual fine, but there is when I use 'pay amount' or 'pay selected'

I don't really like that the functions are internal functions and then exported - fine_paid_renewable could be an accountline method? I think maybe_renew could also be an accountline method? it could act on the item attached to fine, and maybe names like:
renewable
renew_items


For the last point, I think the pref description should highlight that if 'RenewalPeriodBase' is set to due date, there may be doubled charges

I do really like the idea here, but I think it is a big change and needs some refining
Comment 36 Andrew Isherwood 2019-10-18 09:44:44 UTC
Created attachment 94402 [details] [review]
Bug 23051: (follow-up) Fix unit tests

As raised by Nick in comment #35
Comment 37 Andrew Isherwood 2019-10-21 11:11:24 UTC
Created attachment 94434 [details] [review]
Bug 23051: (follow-up) Fix/improve feedback

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- it would be nice to get feedback on what was successfully renewed as well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine
Comment 38 Andrew Isherwood 2019-10-21 15:09:30 UTC
Created attachment 94453 [details] [review]
Bug 23051: (follow-up) Refactor methods

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges
Comment 39 Andrew Isherwood 2019-10-22 11:11:54 UTC
Created attachment 94525 [details] [review]
Bug 23051: (follow-up) Refactor methods

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges
Comment 40 Andrew Isherwood 2019-10-22 13:26:25 UTC
Hi Nick

>I am also not sure about the code path if a patron paid fines on the opac (via paypal etc.) but renewals are not allowed on the opac.

Looking at the code, the OPAC PayPal payment would cause items to be renewed as part of the fine payment. The same applies if the site is using the WPM or Capita payment plugins, I'm not sure of any others though I suspect the same would also be true for any payment plugin, essentially anything that pays the fines using the Koha::Account::pay function.

I'm unsure what to do here. There seems to be a convention of passing a "note" parameter when calling the "pay" function. With the OPAC PayPal payment, the note is passed as "PayPal", with Capita it's "Pay360 Payment" etc. So, we could have a blacklist (not sure which would be best), which prevented the renewal from happening if the payment was passed a note containing one of these values, though that seems a bit fragile.

If we are going to prevent renewal for payments made via the OPAC, we should probably highlight that in the syspref.

Any thoughts?
Comment 41 Martin Renvoize 2019-10-22 13:37:21 UTC
As the maintainer of the Pay360 and WPM plugins, I'd be happy to remove the logic that attempts to renew items from within those plugins themselves and have that action taken from 'koha proper' as per sysprefs...

Just my two cents.
Comment 42 Andrew Isherwood 2019-10-22 13:58:50 UTC
Hi Martin - The renewal is carried out from within Koha::Account::pay if the syspref is turned on and the issue meets the required conditions, so even if the plugins are not explicitly calling the renewal, it will still happen anyway.
Comment 43 Nick Clemens 2019-10-23 10:47:10 UTC
(In reply to Andrew Isherwood from comment #42)
> Hi Martin - The renewal is carried out from within Koha::Account::pay if the
> syspref is turned on and the issue meets the required conditions, so even if
> the plugins are not explicitly calling the renewal, it will still happen
> anyway.

The logic could be moved higher? Or a switch in Koha::Account::pay to prevent the renewal code?

I also think it could be that this pref always renews, that just needs to be made clear to the end user - it isn't technically an 'opac renewal' if done by the accounts job. I would be more interested in making sure the patron knows if the renewal happened or not after the payment then preventing renewal in edge cases
Comment 44 Andrew Isherwood 2019-10-23 10:59:31 UTC
Created attachment 94604 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 45 Andrew Isherwood 2019-10-23 10:59:35 UTC
Created attachment 94605 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 46 Andrew Isherwood 2019-10-23 10:59:38 UTC
Created attachment 94606 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 47 Andrew Isherwood 2019-10-23 10:59:42 UTC
Created attachment 94607 [details] [review]
Bug 23051: (follow-up) Refactor renewal code

As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.
Comment 48 Andrew Isherwood 2019-10-23 10:59:45 UTC
Created attachment 94608 [details] [review]
Bug 23051: (follow-up) Provide feedback

For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.

Addresses the second point in Nick's comment #20
Comment 49 Andrew Isherwood 2019-10-23 10:59:49 UTC
Created attachment 94609 [details] [review]
Bug 23051: (follow-up) Fix unit tests

As raised by Nick in comment #35
Comment 50 Andrew Isherwood 2019-10-23 10:59:53 UTC
Created attachment 94610 [details] [review]
Bug 23051: (follow-up) Fix/improve feedback

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- it would be nice to get feedback on what was successfully renewed as well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine
Comment 51 Andrew Isherwood 2019-10-23 10:59:56 UTC
Created attachment 94611 [details] [review]
Bug 23051: (follow-up) Refactor methods

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges
Comment 52 Andrew Isherwood 2019-10-23 14:20:54 UTC
Created attachment 94629 [details] [review]
Bug 23051: (follow-up) Add SIP summary

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- Ideally SIP would get feedback in a screen message
Comment 53 Andrew Isherwood 2019-10-24 10:12:21 UTC
Created attachment 94649 [details] [review]
Bug 23051: (follow-up) Renewing in OPAC

This follow up patch addresses the following parts of Nick's feedback in
comment #35:

- I am also not sure about the code path if a patron paid fines on the
opac (via paypal etc.) but renewals are not allowed on the opac.

We've introduced the syspref RenewAccruingItemInOpac (default is off)
which, when enabled, will cause items attached to fines that are paid
off in the OPAC (via payment plugins), to be automatically renewed.
Comment 54 Andrew Isherwood 2019-10-24 10:39:23 UTC
Revised test plan:

- In Administration, search for the new syspref "RenewAccruingItemWhenPaid"
- TEST: Observe that the new syspref exists and is turned off by default
- Turn on the syspref

STAFF RENEWALS:
- Loan an item to a patron, ensure you choose a patron, branch and item combination that will incur overdue fines
- Backdate the due date to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- Go to the patron's "Accounting" page and choose "Pay" against the item that has the fine
- Enter an amount to pay that will fully pay off the item and click "Confirm"
- TEST: Observe that in the payment result pay, details of the item having been renewed are displayed
- Go to the patron's checkouts list
- TEST: Observe that the item has been renewed as part of the fine payment process
- Add some restriction that would prevent the item being renewed, maybe modify the number of renewals (issues.renewals in the DB) it has had such that it exceeds the allowed limit
- Backdate the due date of the item to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- Go to the patron's "Accounting" page and choose "Pay" against the item that has the fine
- Enter an amount to pay that will fully pay off the item and click "Confirm"
- TEST: Observe that in the payment result pay, the fact that the item was not renewed is summarised, with the reason for the renewal failure

SIP RENEWALS:
- Modify the item's renewal count back so that they are allowed
- Backdate the due date to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- Use sip_cli_emulator.pl to send a fee_paid message for the full amount oweing, for example:

./sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <selfcheck_user> -sp <selfcheck_password> --patron <patron_username> --password <patron_password> -l <location_code> -m fee_paid --fee-amount <fee_amount>

- TEST: Observe that the response message contains a summary message with details of renewal successes and failures

OPAC RENEWALS:
Note: Currently the WPM payment plugin and Capita payment plugin pass the required parameter that will allow this development to know that it is being called from the OPAC and should act or not depending on the new syspref "RenewAccruingItemInOpac". This portion of the development can only be tested with one of these plugins.
- In Administration, search for the new syspref "RenewAccruingItemInOpac"
- TEST: Observe that the new syspref exists and is turned off by default
- Ensure the syspref is turned off
- Backdate the due date to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- In the OPAC, pay the item in full
- TEST: Observe that the item is not renewed
- In Administration, search for the new syspref "RenewAccruingItemInOpac"
- TEST: Observe that the new syspref exists and is turned off by default
- Turn on the syspref
- Backdate the due date to the past so the loan will accrue fines
- Run the fines.pl cronjob (misc/cronjobs/fines.pl)
- In the OPAC, pay the item in full
- TEST: Observe that the item is renewed
Comment 55 Andrew Isherwood 2019-11-19 14:48:31 UTC
Created attachment 95557 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 56 Andrew Isherwood 2019-11-19 14:48:36 UTC
Created attachment 95558 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 57 Andrew Isherwood 2019-11-19 14:48:41 UTC
Created attachment 95559 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 58 Andrew Isherwood 2019-11-19 14:48:46 UTC
Created attachment 95560 [details] [review]
Bug 23051: Squash of sign off follow-ups

Rebasing was a nightmare, so I'm squashing the sign off follow-ups to
ease the pain with any future rebases

Includes:

Bug 23051: (follow-up) Refactor renewal code
As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.

Bug 23051: (follow-up) Provide feedback
For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.
Addresses the second point in Nick's comment #20

Bug 23051: (follow-up) Fix unit tests
As raised by Nick in comment #35

Bug 23051: (follow-up) Fix/improve feedback
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- it would be nice to get feedback on what was successfully renewed as well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine

Bug 23051: (follow-up) Refactor methods
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges

Bug 23051: (follow-up) Add SIP summary
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- Ideally SIP would get feedback in a screen message

Bug 23051: (follow-up) Renewing in OPAC
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I am also not sure about the code path if a patron paid fines on the
opac (via paypal etc.) but renewals are not allowed on the opac.

We've introduced the syspref RenewAccruingItemInOpac (default is off)
which, when enabled, will cause items attached to fines that are paid
off in the OPAC (via payment plugins), to be automatically renewed.
Comment 59 Andrew Isherwood 2020-03-04 14:29:27 UTC
Created attachment 100127 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 60 Andrew Isherwood 2020-03-04 14:29:31 UTC
Created attachment 100128 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 61 Andrew Isherwood 2020-03-04 14:29:34 UTC
Created attachment 100129 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 62 Andrew Isherwood 2020-03-04 14:29:38 UTC
Created attachment 100130 [details] [review]
Bug 23051: Squash of sign off follow-ups

Rebasing was a nightmare, so I'm squashing the sign off follow-ups to
ease the pain with any future rebases

Includes:

Bug 23051: (follow-up) Refactor renewal code
As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.

Bug 23051: (follow-up) Provide feedback
For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.
Addresses the second point in Nick's comment #20

Bug 23051: (follow-up) Fix unit tests
As raised by Nick in comment #35

Bug 23051: (follow-up) Fix/improve feedback
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- it would be nice to get feedback on what was successfully renewed as
well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine

Bug 23051: (follow-up) Refactor methods
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if
'RenewalPeriodBase'
is set to due date, there may be doubled charges

Bug 23051: (follow-up) Add SIP summary
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- Ideally SIP would get feedback in a screen message

Bug 23051: (follow-up) Renewing in OPAC
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I am also not sure about the code path if a patron paid fines on the
opac (via paypal etc.) but renewals are not allowed on the opac.

We've introduced the syspref RenewAccruingItemInOpac (default is off)
which, when enabled, will cause items attached to fines that are paid
off in the OPAC (via payment plugins), to be automatically renewed.
Comment 63 Andrew Isherwood 2020-03-04 14:30:05 UTC
Another monster rebase done. Good lord I hope this one gets into master soon.
Comment 64 Nick Clemens 2020-03-04 14:32:25 UTC
Created attachment 100131 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 65 Nick Clemens 2020-03-04 14:32:29 UTC
Created attachment 100132 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 66 Nick Clemens 2020-03-04 14:32:32 UTC
Created attachment 100133 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University
Comment 67 Nick Clemens 2020-03-04 14:32:36 UTC
Created attachment 100134 [details] [review]
Bug 23051: Squash of sign off follow-up

Rebasing was a nightmare, so I'm squashing the sign off follow-ups to
ease the pain with any future rebases

Includes:

Bug 23051: (follow-up) Refactor renewal code
As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.

Bug 23051: (follow-up) Provide feedback
For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.
Addresses the second point in Nick's comment #20

Bug 23051: (follow-up) Fix unit tests
As raised by Nick in comment #35

Bug 23051: (follow-up) Fix/improve feedback
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- it would be nice to get feedback on what was successfully renewed as well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine

Bug 23051: (follow-up) Refactor methods
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges

Bug 23051: (follow-up) Add SIP summary
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- Ideally SIP would get feedback in a screen message

Bug 23051: (follow-up) Renewing in OPAC
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I am also not sure about the code path if a patron paid fines on the
opac (via paypal etc.) but renewals are not allowed on the opac.

We've introduced the syspref RenewAccruingItemInOpac (default is off)
which, when enabled, will cause items attached to fines that are paid
off in the OPAC (via payment plugins), to be automatically renewed.
Comment 68 Nick Clemens 2020-03-04 14:32:40 UTC
Created attachment 100135 [details] [review]
Bug 23051: (QA follow-up) Missing curly and tabs and fix test

Some rebase issues, accounttype no longer exists, circ rules make
CanBookBeRenewed fail, so we mock that too

interface must be passed as a hashref
Comment 69 Nick Clemens 2020-03-04 14:35:59 UTC
SIP gives nice feedback, staff client does unless I click 'pay' next to the individual fine, can you check that one?

I do wonder about write-offs and whether they should also trigger renewal, but am calling that scope creep

I don't know about the call to 'GetFine' in the renew_item sub, seems an extra DB hit. I am not sure either - how do you end up with more than one OVERDUE fine on an item? if from an old checkout that was returned, do we care in this instance? I may be overthinking - not a blocker, but curious about the reasoning

Please check my work on rebasing and cleaning in the last patch
Comment 70 Andrew Isherwood 2020-03-04 15:12:32 UTC
Created attachment 100138 [details] [review]
Bug 23051: (follow-up) Fix renew summary display

As mentioned in comment #69:

>staff client does unless I click 'pay' next to the individual fine
Comment 71 Andrew Isherwood 2020-03-04 15:17:41 UTC
This looks really great Nick, thanks very much for looking at this. It looks like we were both rebasing the same bug at the same time! :-)

To respond to your comments:

>I don't know about the call to 'GetFine' in the renew_item sub, seems an extra
>DB hit. I am not sure either - how do you end up with more than one OVERDUE fine
>on an item?

It was a while ago, so I'm just speculating, but I suspect I did the call to GetFine and checked the fine to ensure that the fine had been fully paid off and no outstanding amount remained. Does that make sense?

Anyway, your rebasing work looks great! Many thanks again for your work on this.
Comment 72 Andrew Isherwood 2020-03-05 10:28:34 UTC
Created attachment 100172 [details] [review]
Bug 23051: (follow-up) Remove call to GetFines

As mentioned by Nick in comment #69 and subsequently discussed on Slack,
the call to GetFines in renew_item was unnececcesary. It has now been
removed.
Comment 73 Nick Clemens 2020-03-05 12:47:16 UTC
Created attachment 100182 [details] [review]
Bug 23051: Add RenewAccruingItemWhenPaid syspref

This patch adds the new syspref "RenewAccruingItemWhenPaid"

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Nick Clemens 2020-03-05 12:47:19 UTC
Created attachment 100183 [details] [review]
Bug 23051: Add unit tests

This patch adds unit tests for all modules affected by this bug

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2020-03-05 12:47:23 UTC
Created attachment 100184 [details] [review]
Bug 23051: Renew items when fines paid off

When the RenewAccruingItemWhenPaid syspref is enabled and all the fines
on an item that is accruing fines are paid, we automatically renew that
item to prevent it from starting to accrue fines again.

This patch adds an additional argument to C4::Circulation::AddRenewal
which allows us to skip the calculation of fines upon renewal, which we
don't want to do if the fines on that item have just been paid. Existing
calls to AddRenewal have not been amended because there seems to be a
convention of only passing undef when adding arguments that require
their positioning to be maintained. Since the new argument is the last
one, this is not the case with any existing call.

Signed-off-by: Lucy Harrison <L.M.Harrison@lboro.ac.uk>
Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Nick Clemens 2020-03-05 12:47:26 UTC
Created attachment 100185 [details] [review]
Bug 23051: (follow-up) Add renewal feedback and move code to subroutines and test

Rebasing was a nightmare, so I'm squashing the sign off follow-ups to
ease the pain with any future rebases

Includes:

Bug 23051: (follow-up) Refactor renewal code
As per Nick's first point in comment #20, the code that tests for
renewability and renews items has been refactored into it's own
function.

Bug 23051: (follow-up) Provide feedback
For renewals that fail when a fine is being paid off, this patch causes
any errors to be passed back to the template for display.
Addresses the second point in Nick's comment #20

Bug 23051: (follow-up) Fix unit tests
As raised by Nick in comment #35

Bug 23051: (follow-up) Fix/improve feedback
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- it would be nice to get feedback on what was successfully renewed as well
- In general I think I would prefer to see 'ok' and 'not_ok' returned as
a single 'renewal_results' array
- There is no listing of errors if I use the 'pay' button on an
individual fine

Bug 23051: (follow-up) Refactor methods
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I don't really like that the functions are internal functions and then
exported
- I think the pref description should highlight that if 'RenewalPeriodBase'
is set to due date, there may be doubled charges

Bug 23051: (follow-up) Add SIP summary
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- Ideally SIP would get feedback in a screen message

Bug 23051: (follow-up) Renewing in OPAC
This follow up patch addresses the following parts of Nick's feedback in
comment #35:
- I am also not sure about the code path if a patron paid fines on the
opac (via paypal etc.) but renewals are not allowed on the opac.

We've introduced the syspref RenewAccruingItemInOpac (default is off)
which, when enabled, will cause items attached to fines that are paid
off in the OPAC (via payment plugins), to be automatically renewed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Nick Clemens 2020-03-05 12:47:30 UTC
Created attachment 100186 [details] [review]
Bug 23051: (QA follow-up) Missing curly and tabs and fix test

Some rebase issues, accounttype no longer exists, circ rules make
CanBookBeRenewed fail, so we mock that too

interface must be passed as a hashref

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Nick Clemens 2020-03-05 12:47:33 UTC
Created attachment 100187 [details] [review]
Bug 23051: (follow-up) Fix renew summary display

As mentioned in comment #69:

>staff client does unless I click 'pay' next to the individual fine

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Nick Clemens 2020-03-05 12:47:37 UTC
Created attachment 100188 [details] [review]
Bug 23051: (follow-up) Remove call to GetFines

As mentioned by Nick in comment #69 and subsequently discussed on Slack,
the call to GetFines in renew_item was unnececcesary. It has now been
removed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens 2020-03-05 12:55:35 UTC
OPAC renewals not tested (i don't have the plugins) but are syspref controlled and should work similarly - passing QA
Comment 81 Martin Renvoize 2020-03-06 10:04:51 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 82 Martin Renvoize 2020-03-06 13:15:12 UTC
Created attachment 100271 [details] [review]
Bug 23051: (RM follow-up) Fix rebase error

Looks like at some point during reworking/rebasing the signature for
Koha::Account::Line->apply changed and was then reverted but one case of
the reversion got lost.

The error was highlighted in the patrons_accounts credit api tests.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2020-03-06 13:15:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 84 Joy Nelson 2020-04-01 22:36:42 UTC
enhancement not backported to 19.11.x