Here is the whole stuff: https://gitlab.com/joubu/Koha/commits/bug_20287 I set the status Needs Signoff to ask for a code review. No test plan is provided here, you will have to read the code and find bugs or confirm everything sounds great.
Hi Jonathan, I've read the code and added few comment to commits in gitlab, hope you've received a notice about that ;) Also, there are some issues reported by QA tools: FAIL C4/Members.pm FAIL pod *** ERROR: Spurious =cut command in file C4/Members.pm FAIL opac/opac-messaging.pl FAIL valid Global symbol "$borrower" requires explicit package name opac/opac-messaging.pl had compilation errors. FAIL opac/opac-registration-verify.pl FAIL valid "generate_password" is not exported by the Koha::AuthUtils module Can't continue after import errors FAIL tools/modborrowers.pl FAIL valid Global symbol "$success" requires explicit package name tools/modborrowers.pl had compilation errors.
Remove branch fixed and rebased.
Created attachment 73436 [details] [review] Bug 20287: Fix tests expecting a warning The new Koha::Patron-based implementation encapsulates some error conditions that raised warnings and the tests expected that warning. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment on attachment 73436 [details] [review] Bug 20287: Fix tests expecting a warning Pushed to the remote branch, thanks Tomas!
Tomas, could you sign-off?
Signed of: https://gitlab.com/josef.moravec/Koha/tree/bug_20287
t/db_dependent/Koha/Object.t .. 3/11 DBD::mysql::st execute failed: Duplicate entry 'MB62Yq_MZzsGZK1vprKD' for key 'cardnumber' [for Statement "INSERT INTO `borrowers` ( `borrowernumber`, `branchcode`, `cardnumber`, `categorycode`, `dateenrolled`, `dateexpiry`, `dateofbirth`, `debarred`, `guarantorid`, `password`, `privacy`, `privacy_guarantor_checkouts`, `sms_provider_id`, `userid`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0=undef, 1='AofTnkLO7', 2='MB62Yq_MZzsGZK1vprKD', 3='WQQkZH3D', 4='2018-04-20T11:06:10', 5='4237-12-20T11:06:10', 6=undef, 7=undef, 8=undef, 9='!', 10=1, 11=0, 12=undef, 13='1'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. # Failed test 'Exception is thrown correctly' # at t/db_dependent/Koha/Object.t line 292. # expecting: Koha::Exceptions::Object::DuplicateID # found: normal exit # Looks like you planned 10 tests but ran 7. # Looks like you failed 1 test of 7 run. # Failed test 'store() tests' # at t/db_dependent/Koha/Object.t line 315. Can't call method "message" without a package or object reference at t/db_dependent/Koha/Object.t line 295. # Looks like your test exited with 255 just after 10. t/db_dependent/Koha/Object.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/11 subtests Test Summary Report ------------------- t/db_dependent/Koha/Object.t (Wstat: 65280 Tests: 10 Failed: 1) Failed test: 10 Non-zero exit status: 255 Parse errors: Bad plan. You planned 11 tests but ran 10. Files=1, Tests=10, 3 wallclock secs ( 0.04 usr 0.00 sys + 2.48 cusr 0.25 csys = 2.77 CPU) Result: FAIL
I have no idea how to fix this failure correctly, it comes from bug 20590.
The problem with the tests is that we are using Koha::Patron to test Koha::Object. We should use a more simple object for that, Koha::Patron->store does a lot of things now and so the behaviours changed.
Jonathan Pattern ======= I'm not sure about the pattern you are using in ->store. Take a look here: http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Storage.pm#txn_do Basically, txn_do is encapsulating the exception you are raising, and so the behaviour is not the expected one. Failure in Object.t =================== Regarding the failure in Object.t, I agree we should pick a more simple object (I guess Koha::Patron was the first we introduced and was pretty simple until this patchset). Exceptions ========== On bug 20590 we introduced Koha::Exceptions::Exception->full_message (which should be moved into the implementing class (in the case you implemented, Koha::Exceptions::Object) but the way it is implemented works for the use case you wrote, but not for Koha::Object->store.
(In reply to Tomás Cohen Arazi from comment #11) > Jonathan > > Pattern > ======= > I'm not sure about the pattern you are using in ->store. Take a look here: > > http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/Storage.pm#txn_do > > Basically, txn_do is encapsulating the exception you are raising, and so the > behaviour is not the expected one. I am not sure to understand your point here, Koha::Exceptions::Object::FKConstraint is raised and correctly propagated. > Failure in Object.t > =================== > Regarding the failure in Object.t, I agree we should pick a more simple > object (I guess Koha::Patron was the first we introduced and was pretty > simple until this patchset). I have added a new commit ("Use more simple object for Object.t") > Exceptions > ========== > On bug 20590 we introduced Koha::Exceptions::Exception->full_message (which > should be moved into the implementing class (in the case you implemented, > Koha::Exceptions::Object) but the way it is implemented works for the use > case you wrote, but not for Koha::Object->store. I think these patches highlight that the our exception system need to be reworked a bit and I think it's outside the scope of this bug. We should address it ASAP, once this pushed. See also new commit "Test exception DuplicateID for Koha::Patron->store" Remote branch has been rebased and the 2 commits added.
This has now had at least two QA runs (Tomas and Myself) and I believe all outstanding QA failing issues are resolved. I'm going to Pass QA and suggest we catch any fallout between now and release and even with all of our eyes on it there's bound to have been something creep through. Jonathan has done a great job, thankyou very much. As a side note.. after having discussed a few issues with him I have a few followup bugs to create that relate to this but should not hold up it's continuance. One being a case where none of us was entirely 100% sure about where a 'sync' key was passed into ModBiblio in the pate for the NL syncing code (which I was going to asign to Magnus) and a second where a few pre-existing race conditions are made much more obvious by this work. Passing QA :) Jonathan, can you add Signoff lines for Josef, Tomas and Me :)
Created attachment 76901 [details] [review] Bug 20287: Move fixup_cardnumber Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76902 [details] [review] Bug 20287: Move trim values to a method Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76903 [details] [review] Bug 20287: Koha::Object->get_from_storage May be part of a separate bug report if needed TODO - add POD + tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76904 [details] [review] Bug 20287: Move AddMember to Koha::Patron->store Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76905 [details] [review] Bug 20287: Replace occurrences of AddMember with Koha::Patron->new->store->borrowernumber Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76906 [details] [review] Bug 20287: Fix export issues in tests t/db_dependent/Holds/RevertWaitingStatus.t .. Undefined subroutine &C4::Circulation::MoveReserve called at /home/vagrant/kohaclone/C4/Circulation.pm line 1316. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76907 [details] [review] Bug 20287: Remove warning If there are no patrons in DB: Use of uninitialized value in addition (+) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76908 [details] [review] Bug 20287: New warning in t/db_dependent/Koha/Object.t (?) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76909 [details] [review] Bug 20287: Use DBIC transaction instead of AutoCommit=0 Remove error "Already in a transaction" Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76910 [details] [review] Bug 20287: Remove AddMember_Auto I am not sure I understood the point of this subroutine. Did I miss something here? Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76911 [details] [review] Bug 20287: Add plain_text_password (& Remove AddMember_Opac) But actually we could remove it if it does not make sense for other use. Callers could deal with it since the password is not generated here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76912 [details] [review] Bug 20287: Move ModMember to Koha::Patron Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76913 [details] [review] Bug 20287: ->store new deal with enrolment fee Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76914 [details] [review] Bug 20287: Fix test t/db_dependent/Members.t and prevent regressions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76915 [details] [review] Bug 20287: There is a warning, we just want to unset the value here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76916 [details] [review] Bug 20287: Replace ModMember in patrons import Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76917 [details] [review] Bug 20287: Fix update of patrons, clean the data before ->store Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76918 [details] [review] Bug 20287: generate_userid now set the userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76919 [details] [review] Bug 20287: Replace occurrences of AddMember in REST API Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76920 [details] [review] Bug 20287: Replace occurrences of ModMember in REST API Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76921 [details] [review] Bug 20287: Fix tests expecting a warning The new Koha::Patron-based implementation encapsulates some error conditions that raised warnings and the tests expected that warning. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76922 [details] [review] Bug 20287: Use more simple object for Object.t We initially use Koha::Patron to test Object.t, but now it overwrites ->store and so it is better to use a more simple object. ApiKey has foreign keys and unique key. We lost one test: there is only one unique key whereas we had 2 on borrowers (I did not find a better option) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76923 [details] [review] Bug 20287: Test exception DuplicateID for Koha::Patron->store Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76924 [details] [review] Bug 20287: (QA follow-up) Fix warning on undefined Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 76927 [details] [review] Bug 20287: (follow-up) Test exception DuplicateID for Koha::Patron->store
Awesome work all! Pushed to master for 18.11
Created attachment 77221 [details] [review] Bug 20287: (RM follow-up) Restore use C4::Members
Follow-up patch pushed to master
Enhancement, not backporting to 18.05.x series.
*** Bug 21110 has been marked as a duplicate of this bug. ***
Backported 'Bug 20287: Koha::Object->get_from_storage' patch only (as it's very helpful for bugfix's that create new tests relying upon it)