Bug 11960 - GetMemberDetails is unnecessarily called in CanBookBeRenewed
Summary: GetMemberDetails is unnecessarily called in CanBookBeRenewed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-19 15:38 UTC by Jonathan Druart
Modified: 2015-06-04 23:30 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11960: GetMemberDetails is unnecessarily called in CanBookBeRenewed (1.46 KB, patch)
2014-03-19 15:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11960: GetMemberDetails is unnecessarily called in CanBookBeRenewed (1.46 KB, patch)
2014-03-19 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11960: GetMemberDetails is unnecessarily called in CanBookBeRenewed (2.21 KB, patch)
2014-03-20 12:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11960: GetMemberDetails is unnecessarily called in CanBookBeRenewed (2.26 KB, patch)
2014-04-11 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-03-19 15:38:05 UTC
C4::Circulation::CanBookBeRenewed calls C4::Members::GetMemberDetails to retrieve categorycode and branchcode.

- categorycode is used to retrieve the issuing rule
- the borrower information is passed to C4::Circulation::_GetCircControlBranch. Which only uses the branchcode parameter.

GetMemberDetails does a lot of useless calls/queries (patronflags, account, etc.)

This call could be replaced with a call to C4::Members::GetMember
Comment 1 Jonathan Druart 2014-03-19 15:40:43 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-03-19 16:01:22 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2014-03-20 12:17:29 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-04-11 14:23:10 UTC
Created attachment 27026 [details] [review]
Bug 11960: GetMemberDetails is unnecessarily called in CanBookBeRenewed

C4::Circulation::CanBookBeRenewed calls C4::Members::GetMemberDetails to
retrieve categorycode and branchcode.

- categorycode is used to retrieve the issuing rule
- the borrower information is passed to
  C4::Circulation::_GetCircControlBranch. Which only uses the branchcode
  parameter.

GetMemberDetails does a lot of useless calls/queries (patronflags,
account, etc.).

This call could be replaced with a call to C4::Members::GetMember.

Note: I presented this small optimisation during a quick introduction to
NYTProf (hackfest 14 in Marseille).

Test plan:
- launch member unit tests
- check the code

Checking the code resulted in the following:
CanBookBeRenewed builds a hash reference from the borrowernumber
(2482). Note it is only used in this function and not passed in.

_GetCircControlBranch (2485) requires that hashreference to
have a branchcode key. As stated above.

The following line (2486) requires it have a categorycode key.

As such, C4::Members::GetMemberDetails is confirmed to be
overkill, and C4::Members::GetMember is sufficient.

Testing Done
------------
0) Back up DB
1) Make sure MPL is in the list of libraries.
2) Apply the patch.
3) run the koha qa test tool
4) prove -v t/db_dependent/Circulation.t

Patch applies cleanly. QA Test tool was all OK. All tests ran successfully.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Galen Charlton 2014-04-11 16:30:07 UTC
Pushed to master.  Thanks, Jonathan!
Comment 6 Fridolin Somers 2014-08-06 08:57:57 UTC
Pushed to 3.14.x, will be in 3.14.10