Bug 13502

Summary: A bug was introduced by 1861 - meaning borrowers sometimes cant be added
Product: Koha Reporter: Chris Cormack <chris>
Component: PatronsAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, martin.renvoize, mtj, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 1861    
Bug Blocks: 13602    
Attachments: Bug 13502 : Code introcduced in 1861 wrongly assumes a null userid is unique
Bug 13502 : Code introcduced in 1861 wrongly assumes a null userid is unique
Bug 13502: Add tests to highlight the problem
Bug 13502: Add tests to highlight the problem
Bug 13502: Code introcduced in 1861 wrongly assumes a null userid is unique
Bug 13502: Add tests to highlight the problem
[PASSED QA] Bug 13502: Code introcduced in 1861 wrongly assumes a null userid is unique
[PASSED QA] Bug 13502: Add tests to highlight the problem

Description Chris Cormack 2014-12-30 20:58:13 UTC
Bug 1861 introduces some changes,

Which is breaking some tests, and might break functional code also.

DBD::mysql::st execute failed: Duplicate entry '' for key 'userid' at /srv/jenkins/workspace/Koha_3.18.x_U14/C4/SQLHelper.pm line 184.

etc.

This is caused by 

my $borrowernumber1 = AddMember(categorycode => $categorycode, branchcode => $branchcode);

And then in the AddMember sub we do
$data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} )
      if ( $data{'userid'} eq '' || !Check_Userid( $data{'userid'} ) );

The problem here is if $data{'userid'} is undef the if fails and we don't create a new userid. ANd since the changes in bug 1861 make this column unique everything ASPLODES!
Comment 1 Chris Cormack 2014-12-30 21:10:03 UTC
This is because we do this

sub Check_Userid {
    my ( $uid, $borrowernumber ) = @_;

    return 1 unless ($uid);


And 1 means unique .. so we do Check_Userid ... it tells us, yes its unique .. 


This is wrong we should return 0 here. If it is empty, assume it is non unique.

This is a change in behaviour introduced by 1861.
Comment 2 Chris Cormack 2014-12-30 21:24:25 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-12-31 10:20:07 UTC
Chris,
With all patches from bug 1861, I don't get the error:

mysql> select count(*) from borrowers where userid="";
+----------+
| count(*) |
+----------+
|        1 |
+----------+
1 row in set (0.01 sec)

$ prove t/db_dependent/Circulation/CheckIfIssuedToPatron.t
t/db_dependent/Circulation/CheckIfIssuedToPatron.t .. ok     
All tests successful.
Files=1, Tests=22,  2 wallclock secs ( 0.04 usr  0.00 sys +  1.34 cusr  0.04 csys =  1.42 CPU)
Result: PASS
Comment 4 Chris Cormack 2014-12-31 10:49:10 UTC
Have you run the db updates? It won't manifest without it, if you check jenkins you can see it failing there too. Also the assumption that an undefined userid is unique is flawed. 

It's 12 mins until 2015 so happy new year.
Comment 5 Jonathan Druart 2014-12-31 11:00:02 UTC
(In reply to Chris Cormack from comment #4)
> Have you run the db updates? It won't manifest without it, if you check
> jenkins you can see it failing there too. Also the assumption that an
> undefined userid is unique is flawed. 
> 
> It's 12 mins until 2015 so happy new year.

Ha no, sorry about that!
Happy new year Chris :)
Comment 6 Jonathan Druart 2014-12-31 11:13:54 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-12-31 11:13:59 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-12-31 11:15:07 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2014-12-31 15:10:02 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2014-12-31 15:10:11 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-12-31 16:27:04 UTC
I am sorry, but something here is not quite right in my tests. I applied both patches, made sure the database is current:

perl t/db_dependent/Members.t
ok 67 - Borrower with duplicate userid has new userid generated
not ok 68 - AddMember should insert the given userid
#   Failed test 'AddMember should insert the given userid'
#   at t/db_dependent/Members.t line 305.
#          got: 'tomasito.none2'
#     expected: 'a_user_id'

I had to solve a conflict in Members.t.
Comment 12 Katrin Fischer 2014-12-31 16:29:36 UTC
Created attachment 34849 [details] [review]
[PASSED QA] Bug 13502: Code introcduced in 1861 wrongly assumes a null userid is unique

To test

1/ Create a borrower with '' as their userid, you may have to edit a
   row in the db to do this
2/ Run  perl t/db_dependent/Circulation/CheckIfIssuedToPatron.t
3/ Notice some tests fail and you see
   DBD::mysql::st execute failed: Duplicate entry '' for key 'userid'
   at /home/chrisc/git/catalyst-koha/C4/SQLHelper.pm line 184.
4/ Apply the patch
5/ Run the tests again, notice they now pass

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2014-12-31 16:30:48 UTC
Passing Chris' original patch - I think the additional tests needs a little bit more work.
Comment 14 Tomás Cohen Arazi 2014-12-31 16:56:09 UTC
Katrin, It seems you didn't run the new tests on top of the latest 1861 followups.
Comment 15 Katrin Fischer 2014-12-31 17:03:20 UTC
Created attachment 34850 [details] [review]
[PASSED QA] Bug 13502: Add tests to highlight the problem

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2014-12-31 17:04:02 UTC
With the follow-up on 1861 applied, the tests pass!
Comment 17 Martin Renvoize 2014-12-31 18:40:44 UTC
Katrin, your plain too efficient.. you beat me to it ;)
Comment 18 Tomás Cohen Arazi 2014-12-31 19:54:49 UTC
Patches pushed to master.

Thanks Chris and Jonathan!
Comment 19 Mason James 2015-01-16 20:46:17 UTC
Pushed to 3.16.x, will be in 3.16.7
Comment 20 Katrin Fischer 2015-01-16 21:01:13 UTC
Hm, wondering, looks like 1861 is not in 3.16 - so should this be?
Comment 21 Mason James 2015-01-17 00:11:04 UTC
(In reply to Katrin Fischer from comment #20)
> Hm, wondering, looks like 1861 is not in 3.16 - so should this be?

ah yes, i just noticed this myself - i'll revert it now