Bug 25508 - Confusing renewal message when paying accruing fine with RenewAccruingItemWhenPaid turned off
Summary: Confusing renewal message when paying accruing fine with RenewAccruingItemWhe...
Status: CLOSED 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: 2023-02-02 14:20 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
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
Bug 25503: Make Koha::Exporter::Record tests pass with unimarc (8.46 KB, patch)
2023-02-02 14:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25503: Add failing test for _get_biblio_for_export (1.56 KB, patch)
2023-02-02 14:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25503: Make _get_biblio_for_export test pass (1.02 KB, patch)
2023-02-02 14:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25503: Add option to export items bundle contents in checkouts table (3.79 KB, patch)
2023-02-02 14:14 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 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.
Comment 21 Martin Renvoize 2023-02-02 14:14:21 UTC
Created attachment 146001 [details] [review]
Bug 25503: Make Koha::Exporter::Record tests pass with unimarc

Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-02-02 14:14:26 UTC
Created attachment 146002 [details] [review]
Bug 25503: Add failing test for _get_biblio_for_export

Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2023-02-02 14:14:32 UTC
Created attachment 146003 [details] [review]
Bug 25503: Make _get_biblio_for_export test pass

Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2023-02-02 14:14:37 UTC
Created attachment 146004 [details] [review]
Bug 25503: Add option to export items bundle contents in checkouts table

Test plan:
1. Enable system preference ExportCircHistory
2. Create an item bundle (see bug 28854 comment 458)
3. Check out the item bundle
4. In the checkouts table, check the box in column "Export",
   choose export format 'MARC with items', check "Export items bundle
   contents" and click on "Export" button
   The exported file should contain the biblio and item created at step
   2, but also the biblio and items that are part of the bundle

Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>