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.
Created attachment 31354 [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.
Created attachment 31355 [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.
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...
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.
(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.
(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.
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.
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?
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.
Created attachment 33942 [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>
Created attachment 33943 [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>
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>
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>
Patches pushed to master. Thanks Mark! AGJ345
Pushed to 3.18.x will be in 3.18.3
Pushed to 3.16.x, will be in 3.16.7