For patrons who have a large number of fines (whether they're paid off or not), the current implementation of GetMemberAccountRecords is needlessly slow, as it retrieves the title and author for every fine record that is linked to an item. This is particularly noticeable in setups where the database server is accessed over the network instead of a local socket. This is particularly obnoxious because the majority of callers of GetMemberAccountRecords() are only interested in the total fine balance, and toss away the detailed information. GetMemberDetails() is a good example -- and consequently, there are several contexts where it would be preferable to use GetMember() rather than GetMemberDetails().
Created attachment 70314 [details] [review] Bug 12001: Move C4::Members::GetMemberAccountRecords
Created attachment 70342 [details] [review] Bug 12001: Move GetMemberAccountRecords to the Koha namespace The GetMemberAccountRecords may be a perf killer, it retrieves all the account lines of a patron and then the related item and biblio information. Most of the time we only want to know how much the patron owns to the library (sum of amountoutstanding). We already have this information in Koha::Patron->account->balance. This patch replaces the occurrences of this subroutine by fetching only the information we need, either the balance, the detail, or both. It removes the formatting done in the module, to use the TT plugin 'Price' instead. There is a very weird and error-prone behavior/feature in GetMemberAccountBalance (FIXME): as the accountlines.accounttype is a varchar(5), the value of the authorised value used for the ManInvInNoissuesCharge pref (category MANUAL_INV) is truncated to the 5 first characters. That could lead to unexpected behaviors. On the way, this patchset also replace the GetMemberAccountBalance subroutine, which returns the balance, the non issues charges and the other charges. We only need to have the balance and the non issues charges to calcul the third one. Test plan: Add several fees for a patron and play with HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge. The information (biblio and item info, as well as the account line) must be correctly displayed on the different screens: 'Fines' module, fine slips, circulation module Note that this patchset could introduce regression on price formatting, but will be easy to fix using the TT plugin.
Created attachment 70343 [details] [review] Bug 12001: Add tests Yes, we need tests to make sure we understand what is going on in this subroutine. The different combination of HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge are tested here with the subroutine we are going to remove (GetMemberAccountBalance). In one of the next patches the new methods will be used to make sure we are not modifying the calculated values (except the formatting).
Created attachment 70344 [details] [review] Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges As said previously, GetMemberAccountBalance returns ( the balance, the non issues charges, the other charges) The other charges are the balance - the non issues charges. In this patch we remove the subroutine from C4::Members and use the new Koha::Account->non_issues_charges subroutine instead
Created attachment 70345 [details] [review] Bug 12001: Prove that result are the same \o/
Created attachment 70346 [details] [review] Bug 12001: Format DEBT correctly Before this patchset, DEBT was formatted in the module, now it should be done template-side.
Created attachment 71798 [details] [review] Bug 12001: Move GetMemberAccountRecords to the Koha namespace The GetMemberAccountRecords may be a perf killer, it retrieves all the account lines of a patron and then the related item and biblio information. Most of the time we only want to know how much the patron owns to the library (sum of amountoutstanding). We already have this information in Koha::Patron->account->balance. This patch replaces the occurrences of this subroutine by fetching only the information we need, either the balance, the detail, or both. It removes the formatting done in the module, to use the TT plugin 'Price' instead. There is a very weird and error-prone behavior/feature in GetMemberAccountBalance (FIXME): as the accountlines.accounttype is a varchar(5), the value of the authorised value used for the ManInvInNoissuesCharge pref (category MANUAL_INV) is truncated to the 5 first characters. That could lead to unexpected behaviors. On the way, this patchset also replace the GetMemberAccountBalance subroutine, which returns the balance, the non issues charges and the other charges. We only need to have the balance and the non issues charges to calcul the third one. Test plan: Add several fees for a patron and play with HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge. The information (biblio and item info, as well as the account line) must be correctly displayed on the different screens: 'Fines' module, fine slips, circulation module Note that this patchset could introduce regression on price formatting, but will be easy to fix using the TT plugin.
Created attachment 71799 [details] [review] Bug 12001: Add tests Yes, we need tests to make sure we understand what is going on in this subroutine. The different combination of HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge are tested here with the subroutine we are going to remove (GetMemberAccountBalance). In one of the next patches the new methods will be used to make sure we are not modifying the calculated values (except the formatting).
Created attachment 71800 [details] [review] Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges As said previously, GetMemberAccountBalance returns ( the balance, the non issues charges, the other charges) The other charges are the balance - the non issues charges. In this patch we remove the subroutine from C4::Members and use the new Koha::Account->non_issues_charges subroutine instead
Created attachment 71801 [details] [review] Bug 12001: Prove that result are the same \o/
Created attachment 71802 [details] [review] Bug 12001: Format DEBT correctly Before this patchset, DEBT was formatted in the module, now it should be done template-side.
Created attachment 71852 [details] [review] Bug 12001: Move GetMemberAccountRecords to the Koha namespace The GetMemberAccountRecords may be a perf killer, it retrieves all the account lines of a patron and then the related item and biblio information. Most of the time we only want to know how much the patron owns to the library (sum of amountoutstanding). We already have this information in Koha::Patron->account->balance. This patch replaces the occurrences of this subroutine by fetching only the information we need, either the balance, the detail, or both. It removes the formatting done in the module, to use the TT plugin 'Price' instead. There is a very weird and error-prone behavior/feature in GetMemberAccountBalance (FIXME): as the accountlines.accounttype is a varchar(5), the value of the authorised value used for the ManInvInNoissuesCharge pref (category MANUAL_INV) is truncated to the 5 first characters. That could lead to unexpected behaviors. On the way, this patchset also replace the GetMemberAccountBalance subroutine, which returns the balance, the non issues charges and the other charges. We only need to have the balance and the non issues charges to calcul the third one. Test plan: Add several fees for a patron and play with HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge. The information (biblio and item info, as well as the account line) must be correctly displayed on the different screens: 'Fines' module, fine slips, circulation module Note that this patchset could introduce regression on price formatting, but will be easy to fix using the TT plugin. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71853 [details] [review] Bug 12001: Add tests Yes, we need tests to make sure we understand what is going on in this subroutine. The different combination of HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge are tested here with the subroutine we are going to remove (GetMemberAccountBalance). In one of the next patches the new methods will be used to make sure we are not modifying the calculated values (except the formatting). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71854 [details] [review] Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges As said previously, GetMemberAccountBalance returns ( the balance, the non issues charges, the other charges) The other charges are the balance - the non issues charges. In this patch we remove the subroutine from C4::Members and use the new Koha::Account->non_issues_charges subroutine instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71855 [details] [review] Bug 12001: Prove that result are the same \o/ Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71856 [details] [review] Bug 12001: Format DEBT correctly Before this patchset, DEBT was formatted in the module, now it should be done template-side. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71857 [details] [review] Bug 12001: (QA follow-up) Explicit return Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 71979 [details] [review] Bug 12001: Move GetMemberAccountRecords to the Koha namespace The GetMemberAccountRecords may be a perf killer, it retrieves all the account lines of a patron and then the related item and biblio information. Most of the time we only want to know how much the patron owns to the library (sum of amountoutstanding). We already have this information in Koha::Patron->account->balance. This patch replaces the occurrences of this subroutine by fetching only the information we need, either the balance, the detail, or both. It removes the formatting done in the module, to use the TT plugin 'Price' instead. There is a very weird and error-prone behavior/feature in GetMemberAccountBalance (FIXME): as the accountlines.accounttype is a varchar(5), the value of the authorised value used for the ManInvInNoissuesCharge pref (category MANUAL_INV) is truncated to the 5 first characters. That could lead to unexpected behaviors. On the way, this patchset also replace the GetMemberAccountBalance subroutine, which returns the balance, the non issues charges and the other charges. We only need to have the balance and the non issues charges to calcul the third one. Test plan: Add several fees for a patron and play with HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge. The information (biblio and item info, as well as the account line) must be correctly displayed on the different screens: 'Fines' module, fine slips, circulation module Note that this patchset could introduce regression on price formatting, but will be easy to fix using the TT plugin. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71980 [details] [review] Bug 12001: Add tests Yes, we need tests to make sure we understand what is going on in this subroutine. The different combination of HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge are tested here with the subroutine we are going to remove (GetMemberAccountBalance). In one of the next patches the new methods will be used to make sure we are not modifying the calculated values (except the formatting). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71981 [details] [review] Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges As said previously, GetMemberAccountBalance returns ( the balance, the non issues charges, the other charges) The other charges are the balance - the non issues charges. In this patch we remove the subroutine from C4::Members and use the new Koha::Account->non_issues_charges subroutine instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71982 [details] [review] Bug 12001: Prove that result are the same \o/ Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71983 [details] [review] Bug 12001: Format DEBT correctly Before this patchset, DEBT was formatted in the module, now it should be done template-side. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71984 [details] [review] Bug 12001: (QA follow-up) Explicit return Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71985 [details] [review] Bug 12001: (QA follow-up) Fix printinvoice page Test plan: Try to print a fee --> without patch it explodes --> with patch it works and the date is formatted according to system wide date format setting Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71986 [details] [review] Bug 12001: (QA follow-up) Add test for Koha::Account::Lines object Test plan prove t/db_dependent/Koha/Account/Lines.t --> should be green Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71987 [details] [review] Bug 12001: (QA follow-up) Fix patron account summary print Test plan: Try to print patron account summary --> without patch it explodes --> with patch it works, and if there is a fee related to item, the title should be printed out Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 71988 [details] [review] Bug 12001: (QA follow-up) Fix showing links to items on boraccount page Test plan: Go to the Fines -> Account page of a patron with fees linked to items --> without patch there is no link to related item --> with patch the link is presented in description column Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to master for 18.05, thanks to everybody involved!
Enhancement, not backported for 17.11
Created attachment 77066 [details] [review] Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords I chose not to backport the main patchset as it was big and didn't apply clean, however, this problem seems to be an issue in 17.11.x This patchset removes the calls where only the total is fetched/used Please consider for backport To test: 1 - Have a patron with some fines 2 - View these pages and confirm fine amounts display: circ/circulation.pl opac/opac-main.pl opac/opac-reserve.pl opac/opac-user.pl reserve/request.pl 3 - Apply patch 4 - View pages again, confirm fines display correctly and no errors
Created attachment 77136 [details] [review] Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords I chose not to backport the main patchset as it was big and didn't apply clean, however, this problem seems to be an issue in 17.11.x This patchset removes the calls where only the total is fetched/used Please consider for backport To test: 1 - Have a patron with some fines 2 - View these pages and confirm fine amounts display: circ/circulation.pl opac/opac-main.pl opac/opac-reserve.pl opac/opac-user.pl reserve/request.pl 3 - Apply patch 4 - View pages again, confirm fines display correctly and no errors
I'm pushing the severity of to Normal -- this is causing significant issues at libraries where patrons have lots of fines, and I would like to see a back-port to 17.11 if possible.
Created attachment 77278 [details] [review] Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords I chose not to backport the main patchset as it was big and didn't apply clean, however, this problem seems to be an issue in 17.11.x This patchset removes the calls where only the total is fetched/used Please consider for backport To test: 1 - Have a patron with some fines 2 - View these pages and confirm fine amounts display: circ/circulation.pl opac/opac-main.pl opac/opac-reserve.pl opac/opac-user.pl reserve/request.pl 3 - Apply patch 4 - View pages again, confirm fines display correctly and no errors
Patch for 17.11.x tested
Commit : Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords Pushed to 17.11.x for 17.11.09
Created attachment 77279 [details] [review] Bug 12001: [17.11.X] correct ILSDI_Services.t UT extracted from commit in master : 6355791848 Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges