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?
Adding the original bug introducing the feature in depends on - this will also help to get the developers and original testers informed :)
While there is not a problem with the functionality, it is very confusing for users to see a string of error messages like this!
Can confirm that our librarians double take every time they see this erroneous message. Thanks for any work to get this fixed!
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.
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.
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.
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.
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.
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
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.
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>
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>
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>
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>
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>
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.
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.05
Pushed to 20.05.x for 20.05.11
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.
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>
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>
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>
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>