Description
Chris Cormack
2014-12-30 20:58:13 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. Created attachment 34832 [details] [review] 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 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 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. (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 :) Created attachment 34834 [details] [review] 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> Created attachment 34835 [details] [review] Bug 13502: Add tests to highlight the problem Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 34836 [details] [review] Bug 13502: Add tests to highlight the problem Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 34847 [details] [review] 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> Created attachment 34848 [details] [review] 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> 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. 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> Passing Chris' original patch - I think the additional tests needs a little bit more work. Katrin, It seems you didn't run the new tests on top of the latest 1861 followups. 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> With the follow-up on 1861 applied, the tests pass! Katrin, your plain too efficient.. you beat me to it ;) Patches pushed to master. Thanks Chris and Jonathan! Pushed to 3.16.x, will be in 3.16.7 Hm, wondering, looks like 1861 is not in 3.16 - so should this be? (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 |