Bug 12001

Summary: GetMemberAccountRecords slows down display of patron details and checkout pages
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: normal    
Priority: P5 - low CC: barton, fridolin.somers, jonathan.druart, josef.moravec, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18786
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 16846, 19933, 19936, 20832, 22006    
Attachments: Bug 12001: Move C4::Members::GetMemberAccountRecords
Bug 12001: Move GetMemberAccountRecords to the Koha namespace
Bug 12001: Add tests
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Bug 12001: Prove that result are the same \o/
Bug 12001: Format DEBT correctly
Bug 12001: Move GetMemberAccountRecords to the Koha namespace
Bug 12001: Add tests
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Bug 12001: Prove that result are the same \o/
Bug 12001: Format DEBT correctly
Bug 12001: Move GetMemberAccountRecords to the Koha namespace
Bug 12001: Add tests
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Bug 12001: Prove that result are the same \o/
Bug 12001: Format DEBT correctly
Bug 12001: (QA follow-up) Explicit return
Bug 12001: Move GetMemberAccountRecords to the Koha namespace
Bug 12001: Add tests
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Bug 12001: Prove that result are the same \o/
Bug 12001: Format DEBT correctly
Bug 12001: (QA follow-up) Explicit return
Bug 12001: (QA follow-up) Fix printinvoice page
Bug 12001: (QA follow-up) Add test for Koha::Account::Lines object
Bug 12001: (QA follow-up) Fix patron account summary print
Bug 12001: (QA follow-up) Fix showing links to items on boraccount page
Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords
Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords
Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords
Bug 12001: [17.11.X] correct ILSDI_Services.t UT

Description Galen Charlton 2014-03-26 18:27:13 UTC
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().
Comment 1 Jonathan Druart 2018-01-05 19:47:58 UTC
Created attachment 70314 [details] [review]
Bug 12001: Move C4::Members::GetMemberAccountRecords
Comment 2 Jonathan Druart 2018-01-09 14:04:51 UTC
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.
Comment 3 Jonathan Druart 2018-01-09 14:04:55 UTC
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).
Comment 4 Jonathan Druart 2018-01-09 14:04:58 UTC
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
Comment 5 Jonathan Druart 2018-01-09 14:05:02 UTC
Created attachment 70345 [details] [review]
Bug 12001: Prove that result are the same \o/
Comment 6 Jonathan Druart 2018-01-09 14:05:05 UTC
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.
Comment 7 Jonathan Druart 2018-02-16 14:39:19 UTC
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.
Comment 8 Jonathan Druart 2018-02-16 14:39:22 UTC
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).
Comment 9 Jonathan Druart 2018-02-16 14:39:26 UTC
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
Comment 10 Jonathan Druart 2018-02-16 14:39:29 UTC
Created attachment 71801 [details] [review]
Bug 12001: Prove that result are the same \o/
Comment 11 Jonathan Druart 2018-02-16 14:39:32 UTC
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.
Comment 12 Tomás Cohen Arazi 2018-02-16 18:22:39 UTC
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>
Comment 13 Tomás Cohen Arazi 2018-02-16 18:22:44 UTC
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>
Comment 14 Tomás Cohen Arazi 2018-02-16 18:22:51 UTC
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>
Comment 15 Tomás Cohen Arazi 2018-02-16 18:22:56 UTC
Created attachment 71855 [details] [review]
Bug 12001: Prove that result are the same \o/

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2018-02-16 18:23:01 UTC
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>
Comment 17 Tomás Cohen Arazi 2018-02-16 18:23:06 UTC
Created attachment 71857 [details] [review]
Bug 12001: (QA follow-up) Explicit return

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Josef Moravec 2018-02-20 05:08:45 UTC
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>
Comment 19 Josef Moravec 2018-02-20 05:08:49 UTC
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>
Comment 20 Josef Moravec 2018-02-20 05:08:54 UTC
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>
Comment 21 Josef Moravec 2018-02-20 05:08:58 UTC
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>
Comment 22 Josef Moravec 2018-02-20 05:09:02 UTC
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>
Comment 23 Josef Moravec 2018-02-20 05:09:06 UTC
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>
Comment 24 Josef Moravec 2018-02-20 05:09:11 UTC
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>
Comment 25 Josef Moravec 2018-02-20 05:09:15 UTC
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>
Comment 26 Josef Moravec 2018-02-20 05:09:20 UTC
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>
Comment 27 Josef Moravec 2018-02-20 05:09:25 UTC
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>
Comment 28 Jonathan Druart 2018-02-26 14:18:25 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 29 Nick Clemens 2018-03-23 01:19:00 UTC
Enhancement, not backported for 17.11
Comment 30 Nick Clemens 2018-07-18 11:39:50 UTC
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
Comment 31 Nick Clemens 2018-07-19 15:02:07 UTC
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
Comment 32 Barton Chittenden 2018-07-23 15:37:21 UTC
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.
Comment 33 Fridolin Somers 2018-07-26 10:45:31 UTC
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
Comment 34 Fridolin Somers 2018-07-26 10:47:15 UTC
Patch for 17.11.x tested
Comment 35 Fridolin Somers 2018-07-26 10:48:55 UTC
Commit :
  Bug 12001: [17.11.X] Remove simple calls to GetMemberAccountRecords
Pushed to 17.11.x for 17.11.09
Comment 36 Fridolin Somers 2018-07-26 11:33:30 UTC
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