Bug 15344 - GetMemberDetails called unecessary
Summary: GetMemberDetails called unecessary
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15342 15511 16009
  Show dependency treegraph
 
Reported: 2015-12-09 17:02 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15344: Remove unucessary call to GetMemberDetails (14.89 KB, patch)
2015-12-09 17:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15344: Remove some other calls of GetMemberDetails from pl scripts (26.77 KB, patch)
2015-12-10 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15344: Remove unucessary call to GetMemberDetails (15.03 KB, patch)
2015-12-11 11:10 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15344: Remove some other calls of GetMemberDetails from pl scripts (26.93 KB, patch)
2015-12-11 11:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15344: Remove unucessary call to GetMemberDetails (15.08 KB, patch)
2015-12-18 17:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15344: Remove some other calls of GetMemberDetails from pl scripts (26.98 KB, patch)
2015-12-18 17:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15344: Remove one occurrence in circ/circulation.pl (1.10 KB, patch)
2015-12-31 11:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15344: Remove one occurrence in circ/circulation.pl (1.35 KB, patch)
2015-12-31 12:43 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-12-09 17:02:42 UTC
This one is crazy, prepare yourself to dream.

For any pages opened from the staff or the OPAC interfaces, the GetMemberDetails is called from get_template_and_user.
This subroutine calls GetMemberAccountRecords which retrieve the items infos for all accountlines entries of the logged in user.
Comment 1 Jonathan Druart 2015-12-09 17:10:19 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-12-09 17:12:08 UTC
Not that this patch could be improved and the call to GetMember removed.
But to be easily backportable, I'd prefer to keep it light.
Comment 3 Jonathan Druart 2015-12-10 10:01:01 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2015-12-11 11:10:56 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-12-11 11:16:07 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-12-18 17:21:23 UTC
Created attachment 45848 [details] [review]
Bug 15344: Remove unucessary call to GetMemberDetails

This subroutine does a lot a processing and should only be called when
necessary.
In the get_template_and_user subroutine (so called from any pages of
Koha), it is call to pass the branchcode, title, firstname, surname and
borrowernumber values for the logged in user.
This subroutine calls GetMemberAccountRecords which retrieve the items
infos for all accountlines entries of the logged in user.
On members/members.pl, let's say you have 74 entries in the accountlines
tables, the page will execute 115 SELECT instead of 35 if you don't have any
accountlines entries.
With this patch, the number of SELECT is always 31.

To test this patch you should have technical skills to know what to do.

Note that USER_INFO was an array of... 1 element. Now it's a hashref.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2015-12-18 17:22:06 UTC
Created attachment 45849 [details] [review]
Bug 15344: Remove some other calls of GetMemberDetails from pl scripts

Same as previously.
For these files it's a bit less obvious.
To make sure these changes won't introduce any regression, check that
the variable returned by GetMember is never used to get something
else than a borrower fields.
The 'flags' should not be get neither.
For opac-user.tt it's different, other keys are got but there are defined
in the pl script.

On the way:
- 'showname' is removed (never used)
- fix scope var issue in opac-user.tt (BORROWER_INF.OPACPatronDetails vs
OPACPatronDetails)

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2015-12-30 12:15:38 UTC
Pushed to master, thanks Jonathan!
Comment 9 Julian Maurice 2015-12-31 09:30:09 UTC
Patches pushed to 3.22.x, will be in 3.22.2
Comment 10 Jonathan Druart 2015-12-31 11:39:58 UTC
Check out explodes with a software error:
Can't use string ("1900156") as a HASH ref while "strict refs" in use at /home/koha/src/C4/Circulation.pm line 813.
Comment 11 Jonathan Druart 2015-12-31 11:45:29 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-12-31 11:46:00 UTC
Kyle and Julian,
Have a quick look at the third patch, it fixes a blocking issue.
Comment 13 Marc Véron 2015-12-31 12:43:52 UTC
Created attachment 46129 [details] [review]
Bug 15344: Remove one occurrence in circ/circulation.pl

In C4::Circulation::CanBookBeIssued, $borrower->{flags} is called and
should be populated by GetMemberDetails

It fixes the following error on checking out:
Can't use string ("1900156") as a HASH ref while "strict refs" in use at
/home/koha/src/C4/Circulation.pm line 813.

Note: Error occurs with patrons with staff permissions, not with 'normal' patrons.
Error is fixed with this patch.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 14 Marc Véron 2015-12-31 14:10:45 UTC
Setting to blocker because of software error while checking out to patron with staff permissions.

Issue is fixed by Jonathan's latest patch.
Comment 15 Frédéric Demians 2015-12-31 18:30:22 UTC
This patch has been pushed to 3.20.x, will be in 3.20.8.
Comment 16 Marc Véron 2016-01-01 11:42:06 UTC
(In reply to Frédéric Demians from comment #15)
> This patch has been pushed to 3.20.x, will be in 3.20.8.

Was it pushed without the third patch? - That would be a blocker, because I still get the error from comment #13 (on master), and it seems that the third patch that fixes it is not on master.

The offending line is #273 in circ/circulation.pl, we need the member details here (for useres with staff permission), otherwise we get the software error:
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/kohaclone/C4/Circulation.pm line 815.
Comment 17 Frédéric Demians 2016-01-01 12:45:01 UTC
Marc and Johnatan, thanks. I confirm 3rd patch is required. I've pushed it directly to 3.20.x branch. It still has to be pushed to master/stable.
Comment 18 Kyle M Hall 2016-01-04 15:50:58 UTC
(In reply to Frédéric Demians from comment #17)
> Marc and Johnatan, thanks. I confirm 3rd patch is required. I've pushed it
> directly to 3.20.x branch. It still has to be pushed to master/stable.

Third patch has been pushed to master! Thanks for the followup Jonathan!
Comment 19 Julian Maurice 2016-01-05 08:15:09 UTC
(In reply to Kyle M Hall from comment #18)
> Third patch has been pushed to master!

Pushed to 3.22.x as well