|
Description
Andrew Isherwood
2019-06-05 13:13:41 UTC
Created attachment 90559 [details] [review] Bug 23051: Add RenewAccruingItemWhenPaid syspref This patch adds the new syspref "RenewAccruingItemWhenPaid" 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. Created attachment 90561 [details] [review] Bug 23051: Add unit tests This patch adds unit tests for all modules affected by this bug 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 Created attachment 91058 [details] [review] Bug 23051: Add RenewAccruingItemWhenPaid syspref This patch adds the new syspref "RenewAccruingItemWhenPaid" 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. Created attachment 91060 [details] [review] Bug 23051: Add unit tests This patch adds unit tests for all modules affected by this bug 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 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. Created attachment 91793 [details] [review] Bug 23051: Add RenewAccruingItemWhenPaid syspref This patch adds the new syspref "RenewAccruingItemWhenPaid" Created attachment 91794 [details] [review] Bug 23051: Add unit tests This patch adds unit tests for all modules affected by this bug 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. Thanks for the heads up Alex. This should be fine now :) Created attachment 92684 [details] [review] Bug 23051: Add RenewAccruingItemWhenPaid syspref This patch adds the new syspref "RenewAccruingItemWhenPaid" Created attachment 92685 [details] [review] Bug 23051: Add unit tests This patch adds unit tests for all modules affected by this bug 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. 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 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 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 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 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 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 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 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. 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... 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? 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. Ah, thanks! I had forgotten about that one. 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 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 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 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. 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 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. 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 Created attachment 94402 [details] [review] Bug 23051: (follow-up) Fix unit tests As raised by Nick in comment #35 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 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 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 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?
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. 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. (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 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 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 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 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. 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 Created attachment 94609 [details] [review] Bug 23051: (follow-up) Fix unit tests As raised by Nick in comment #35 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 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 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 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. 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 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 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 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 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. 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 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 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 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. Another monster rebase done. Good lord I hope this one gets into master soon. 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 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 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 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. 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 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 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 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.
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. 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> 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> 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> 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> 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> 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> 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> OPAC renewals not tested (i don't have the plugins) but are syspref controlled and should work similarly - passing QA Nice work everyone! Pushed to master for 20.05 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> Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x |