Bug 7060 - All instances of GetMemberDetails are unnecessary in C4::Circulation
Summary: All instances of GetMemberDetails are unnecessary in C4::Circulation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: PATCH-Sent (DO NOT USE) blocker
Assignee: Ian Walls
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 7058
  Show dependency tree / graph
 
Reported: 2011-10-18 23:50 UTC by Ian Walls
Modified: 2019-06-27 09:24 UTC (History)
2 users (show)

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

Attachments
Proposed patch (3.98 KB, patch)
2011-10-19 00:00 UTC, Ian Walls 		Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7060: GetMemberDetails unnecessary in C4::Circulation (4.06 KB, patch)
2011-10-19 19:48 UTC, Nicole C. Engard 		Details | Diff | Splinter Review
Followup Patch (1.13 KB, patch)
2011-10-21 14:54 UTC, Ian Walls 		Details | Diff | Splinter Review
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2011-10-18 23:50:54 UTC 
GetMemberDetails is used in the following C4::Circulation subroutines

CanBookBeIssued:  used twice, both times only cardnumber, surname, firstname, and borrowernumber are used, which are all accessible more efficiently from GetMember

AddReturn:  only borrowernumber and cardnumber are used.  the $borrower hashref is returned by AddReturn, but it is only captured in tools/inventory.pl and C4/SIP/ILS/Transaction/Checkin.pm, and in neither place is anything done with it.  GetMember could be used here without any issue

AddRenewal: uses only passes the $borrower object to CalcDateDue, which only uses categorycode and dateexpiry (both covered by GetMember hashref)

AddIssue:  uses an incoming $borrower object, but only the borrowernumber and categorycode fields, so we only need a GetMember object here.

GetRenewalCount:  only uses categorycode

Changing all calls of GetMemberDetails to GetMember in C4/Circulation.pm should be safe, and vastly more efficient
Comment 1 Ian Walls 2011-10-19 00:00:45 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-10-19 19:48:40 UTC 
Created attachment 5996 [details] [review]
[SIGNED-OFF] Bug 7060: GetMemberDetails unnecessary in C4::Circulation

Replaces all instances of GetMemberDetails with the more efficient GetMember.  Since
no hash values other than the borrowers table's fields are used in this module, the
transition is safe.

To test:

1.  Checkout a material; the page should load without error
2.  Renew a material: the page should load without error
3.  Return a material: the page should load without error

The above test plan invokes all the subroutines affected by this patch; any call to an undefined
value in a hashref should give a warning

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 3 Ian Walls 2011-10-19 22:18:16 UTC 
Marking Passed QA, RM please verify
Comment 4 Chris Cormack 2011-10-20 01:10:28 UTC 
Pushed, please test
Comment 5 Ian Walls 2011-10-21 14:47:30 UTC 
I made a mistake.  $borrower->{flags} are used by AddReturn in circ/returns.pl.  GetMemberDetails must be used, otherwise returns break.  working on fix now...
Comment 6 Ian Walls 2011-10-21 14:54:38 UTC 
Created attachment 6017 [details] [review]
Followup Patch

Restores GetMemberDetails usage in AddReturn; since that subroutine returned the borrower object, it was a mistake to change to GetMember, since key things like flags are not returned in that hashref.
Comment 7 Chris Cormack 2011-10-21 15:09:13 UTC 
Follow up pushed