Bug 25508 - Confusing renewal message when paying accruing fine with RenewAccruingItemWhenPaid turned off
Summary: Confusing renewal message when paying accruing fine with RenewAccruingItemWhe...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 23051
Blocks: 27927 28156
  Show dependency treegraph
 
Reported: 2020-05-14 21:34 UTC by Andrew Fuerste-Henry
Modified: 2021-04-25 20:37 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 25508: Do not attempt renewals if syspref is disabled (1.95 KB, patch)
2021-03-24 16:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Do not attempt renewals if syspref is disabled (3.16 KB, patch)
2021-03-25 10:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Do not attempt renewals if syspref is disabled (3.25 KB, patch)
2021-03-25 11:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Only return renewal outcomes to the controller (3.06 KB, patch)
2021-03-25 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Add Unit Tests (2.90 KB, patch)
2021-04-06 07:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Only return renewal outcomes to the controller (3.06 KB, patch)
2021-04-06 07:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25508: Add Unit Tests (2.97 KB, patch)
2021-04-06 12:55 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25508: Only return renewal outcomes to the controller (3.13 KB, patch)
2021-04-06 12:55 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25508: Add Unit Tests (3.02 KB, patch)
2021-04-15 15:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25508: Only return renewal outcomes to the controller (3.18 KB, patch)
2021-04-15 15:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25508: (QA follow-up) Add interface (1.01 KB, patch)
2021-04-15 15:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25508: Add info about caller responsabilities (903 bytes, patch)
2021-04-16 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-05-14 21:34:51 UTC
To recreate:
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
     Set finesMode = production
     perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on the following items were paid off, renewal results are displayed below:
No title ( ): Not renewed - Unknown error"

Looks like we're not properly hiding that alert when RenewAccruingItemWhenPaid is off? So it's reporting that nothing was renewed?
Comment 1 Katrin Fischer 2020-05-15 06:31:39 UTC
Adding the original bug introducing the feature in depends on - this will also help to get the developers and original testers informed :)
Comment 2 Donna 2021-03-24 14:47:14 UTC
While there is not a problem with the functionality, it is very confusing for users to see a string of error messages like this!
Comment 3 Mark 2021-03-24 14:55:32 UTC
Can confirm that our librarians double take every time they see this erroneous message. Thanks for any work to get this fixed!
Comment 4 Martin Renvoize 2021-03-24 16:43:42 UTC
Created attachment 118765 [details] [review]
Bug 25508: Do not attempt renewals if syspref is disabled

In the case where 'RenewAccruingItemWhenPaid' is disabled we were still
testing whether the fine item was renewable ragerdless.

This has two issued:

1. A minor performance issue with us running more code than required.
2. The return value from renew_item when the syspref is disabled was
   empty, but it would still get appended to the 'renew_outcomes' array
   which results in some 'dirty' messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.
Comment 5 Martin Renvoize 2021-03-24 16:46:42 UTC
Oops.. I'd meant to walk David through this one.. seems I completely forgot to do that.

Patch attached as I noticed it today by coincidence too.
Comment 6 Martin Renvoize 2021-03-25 10:50:38 UTC
Created attachment 118776 [details] [review]
Bug 25508: Do not attempt renewals if syspref is disabled

In the case where 'RenewAccruingItemWhenPaid' is disabled we were still
testing whether the fine item was renewable ragerdless.

This has two issued:

1. A minor performance issue with us running more code than required.
2. The return value from renew_item when the syspref is disabled was
   empty, but it would still get appended to the 'renew_outcomes' array
   which results in some 'dirty' messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.
Comment 7 Martin Renvoize 2021-03-25 11:07:50 UTC
Created attachment 118777 [details] [review]
Bug 25508: Do not attempt renewals if syspref is disabled

In the case where 'RenewAccruingItemWhenPaid' is disabled we were still
testing whether the fine item was renewable ragerdless.

This has two issued:

1. A minor performance issue with us running more code than required.
2. The return value from renew_item when the syspref is disabled was
   empty, but it would still get appended to the 'renew_outcomes' array
   which results in some 'dirty' messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.
Comment 8 Martin Renvoize 2021-03-25 11:13:23 UTC
Created attachment 118778 [details] [review]
Bug 25508: Only return renewal outcomes to the controller

There are a few cases where the `renew_item` method in
Koha::Account::Line will return `undef`. For these cases, we should not
pass the error up the chain to the controllers as it leads to malformed
error messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.
Comment 9 Martin Renvoize 2021-04-06 07:52:01 UTC
Created attachment 119191 [details] [review]
Bug 25508: Add Unit Tests

Enhance the unit tests to check for the return values of
Koha::Account->pay.

Test plan
1/ Apply just this patch
2/ Run t/db_dependent/Koha/Account.t and watch it fail
3/ Apply the rest of the patch series
4/ Run t/db_dependent/Koha/Account.t and watch it pass
5/ Signoff
Comment 10 Martin Renvoize 2021-04-06 07:52:04 UTC
Created attachment 119192 [details] [review]
Bug 25508: Only return renewal outcomes to the controller

There are a few cases where the `renew_item` method in
Koha::Account::Line will return `undef`. For these cases, we should not
pass the error up the chain to the controllers as it leads to malformed
error messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.
Comment 11 Andrew Fuerste-Henry 2021-04-06 12:55:28 UTC
Created attachment 119213 [details] [review]
Bug 25508: Add Unit Tests

Enhance the unit tests to check for the return values of
Koha::Account->pay.

Test plan
1/ Apply just this patch
2/ Run t/db_dependent/Koha/Account.t and watch it fail
3/ Apply the rest of the patch series
4/ Run t/db_dependent/Koha/Account.t and watch it pass
5/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Andrew Fuerste-Henry 2021-04-06 12:55:31 UTC
Created attachment 119214 [details] [review]
Bug 25508: Only return renewal outcomes to the controller

There are a few cases where the `renew_item` method in
Koha::Account::Line will return `undef`. For these cases, we should not
pass the error up the chain to the controllers as it leads to malformed
error messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Nick Clemens 2021-04-15 15:30:43 UTC
Created attachment 119639 [details] [review]
Bug 25508: Add Unit Tests

Enhance the unit tests to check for the return values of
Koha::Account->pay.

Test plan
1/ Apply just this patch
2/ Run t/db_dependent/Koha/Account.t and watch it fail
3/ Apply the rest of the patch series
4/ Run t/db_dependent/Koha/Account.t and watch it pass
5/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2021-04-15 15:30:49 UTC
Created attachment 119640 [details] [review]
Bug 25508: Only return renewal outcomes to the controller

There are a few cases where the `renew_item` method in
Koha::Account::Line will return `undef`. For these cases, we should not
pass the error up the chain to the controllers as it leads to malformed
error messages in the UI.

Test plan
1 - Make sure FinesMode is on, RenewAccruingItemWhenPaid is off
2 - Checkout an item to a patron and make it overdue (can backdate the
    checkout)
3 - Make sure the itemtype has fines that will be charged
4 - Charge the fines:
    Set finesMode = production
    perl misc/cronjobs/fines.pl -v
5 - Check the fine appears on the patrons account
6 - Pay off the fine
7 - Receive alert after payment that reads: "The fines on
    the following items were paid off, renewal results are
    displayed below:
        No title ( ): Not renewed - Unknown error"
8 - Apply the patch
9 - Repeat steps 1 through 6 and note that you no longer trigger the
    error message.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2021-04-15 15:30:55 UTC
Created attachment 119641 [details] [review]
Bug 25508: (QA follow-up) Add interface

These patches are making the call for an amount match the call for a specific line,
we should also match the passing of interface to ensure the correct pref is checked

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Jonathan Druart 2021-04-16 10:08:30 UTC
Created attachment 119702 [details] [review]
Bug 25508: Add info about caller responsabilities

We are removing the test on the existence of item and patron, callers
must call renewable before.
Comment 17 Jonathan Druart 2021-04-16 10:29:20 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Fridolin Somers 2021-04-22 13:32:35 UTC
Pushed to 20.11.x for 20.11.05
Comment 19 Andrew Fuerste-Henry 2021-04-25 15:38:46 UTC
Pushed to 20.05.x for 20.05.11
Comment 20 Victor Grousset/tuxayo 2021-04-25 20:37:29 UTC
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.