Bug 12868 - GetMemberDetails calls GetMemberAccountRecords incorrectly
Summary: GetMemberDetails calls GetMemberAccountRecords incorrectly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-03 23:07 UTC by M. Tompsett
Modified: 2016-06-21 21:36 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12868: Improving t/db_dependent/Member.t (2.55 KB, patch)
2014-09-03 23:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12868: Wrong variable used for borrower number (1.51 KB, patch)
2014-09-03 23:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12868: Improving t/db_dependent/Member.t (2.61 KB, patch)
2014-11-26 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12868: Wrong variable used for borrower number (1.57 KB, patch)
2014-11-26 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12868: Improving t/db_dependent/Member.t (2.71 KB, patch)
2014-12-11 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12868: Wrong variable used for borrower number (1.67 KB, patch)
2014-12-11 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2014-09-03 23:07:26 UTC
GetMemberDetails takes two parameters: borrowernumber and cardnumber.
Between the two of them, a borrower hash is created.
This borrower hash has a borrower number in it as well as a card number.
However, the outstanding amount is based on the borrowernumber parameter and not the discovered borrowernumber based on cardnumber when borrowernumber is passed as undef. It should use the borrower hash's borrower number instead.
Comment 1 M. Tompsett 2014-09-03 23:20:44 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2014-09-03 23:21:38 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2014-10-26 09:33:07 UTC
Now, on HEAD:

   prove -v t/db_dependent/Members.t

fails, without applying the first patch. So, I can't follow the test plan.

Otherwise, the second patch, fixing GetMemberDetails, make sense. IMHO it could be signed-off as it is, even without a test case...
Comment 4 Katrin Fischer 2014-10-26 09:41:45 UTC
Hi Frederic, the tests pass for me on a MARC21 database with the sample database data on current master - maybe it looks for some category codes that are not in your database or similar? Some of the db_dependent tests rely heavily on the sample data.
Comment 5 M. Tompsett 2014-11-20 18:06:07 UTC
(In reply to Frédéric Demians from comment #3)
> Now, on HEAD:
> 
>    prove -v t/db_dependent/Members.t
> 
> fails, without applying the first patch. So, I can't follow the test plan.
> 
> Otherwise, the second patch, fixing GetMemberDetails, make sense. IMHO it
> could be signed-off as it is, even without a test case...

Could you past the screen shot of this failing when you only apply the second patch?

I did try that case just now, and this is working for me as expected.
no patch: pass as expected.
patch 1 only: 1 failure as expected.
patch 2 only: pass as expected.
patch 1&2: pass as expected.
Comment 6 M. Tompsett 2014-11-20 21:33:54 UTC
(In reply to M. Tompsett from comment #5)
> (In reply to Frédéric Demians from comment #3)
> > Now, on HEAD:
> >    prove -v t/db_dependent/Members.t
> > fails ...

Nasty, I booted up my UNIMARC VM, updated to the latest origin/master, and ran the prove. KABOOM!

Turns out my categories table only has 'BIB', but line 873 in C4/Members.pm is trying to find 'S'. Which makes that variable undefined and a nasty kaboom!

Added S and PT. Retried. KABOOM! Branchcode issues.

Added CPL, MPL. Retried. Success. Okay.

Nothing applied. Success.
Both applies. Success.
Only 2 applied. Success.
Only 1 applied. Expected failure.

So, if you ensure that you have these Branch codes and Categories added, and then attempt the test plans in comment #1 and comment #2, there should be no problem.
Comment 7 M. Tompsett 2014-11-21 00:26:38 UTC
I'm not sure this is really an enhancement, since this is more of a bug fix. The testing is "enhanced", but that's only because it didn't catch the case that was failing.
Comment 8 Jonathan Druart 2014-11-25 16:20:54 UTC
Mark,
Looking at the code, I completely agree with your patch.
How did you find this problem?
Is there a place where an error/problem is visible on the interface?
Comment 9 M. Tompsett 2014-11-25 18:18:39 UTC
Greetings. I can't remember why, but I suspect it was due to some failures I was encountering in t/db_dependent/Members.t attempting to integrate a local modification into Koha, and when I started tracing GetMemberDetails, that problem jumped out at me.
Comment 10 Jonathan Druart 2014-11-26 08:24:41 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-11-26 08:24:45 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2014-12-11 12:49:14 UTC
Created attachment 34314 [details] [review]
Bug 12868: Improving t/db_dependent/Member.t

The mock function of GetMemberAccountRecord did not properly
account for the undef case. This was corrected.

Then all 4 combinations of borrower number and card number being
defined or not were called to GetMemberDetail.

The problematic test case is where the borrower number is
undefined and the cardnumber is defined.

TEST PLAN
---------
1) Apply just this first patch.
2) prove -v t/db_dependent/Member.t
   -- This should fail!
3) Run koha QA test tools.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2014-12-11 12:49:44 UTC
Created attachment 34315 [details] [review]
Bug 12868: Wrong variable used for borrower number

When only the card number is passed to GetMemberDetail, the
value of $borrowernumber is undefined. Even after finding the
correct borrower and providing a nice hash ($borrower), the
GetMemberAccountRecords is called with the wrong borrower number,
even though it is in the hash ($borrower).

This was fixed by changing $borrowernumber to
$borrower->{borrowernumber}, so that the hash's value will always
be used, since it is correct regardless of whether borrowernumber
or cardnumber were used to find the borrower.

TEST PLAN
---------
1) Apply both patches
2) prove -v t/db_dependent/Member.t
   -- This time the previously failing test will pass.
3) run koha QA test tools.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Tomás Cohen Arazi 2015-01-10 19:21:05 UTC
Patches pushed to master.

Thanks Mark!

AGJ345
Comment 15 Chris Cormack 2015-01-15 00:37:29 UTC
Pushed to 3.18.x will be in 3.18.3
Comment 16 Mason James 2015-01-16 23:41:30 UTC
Pushed to 3.16.x, will be in 3.16.7