Bug 20287

Summary: Move AddMember and ModMember to Koha::Patron
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: amitddng135, josef.moravec, katrin.fischer, kyle, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21022    
Bug Blocks: 16846, 20226, 21068, 21069, 21085, 21086, 21087, 21136, 21149, 21374, 21596, 22461, 22717, 26940    
Attachments: Bug 20287: Fix tests expecting a warning
Bug 20287: Move fixup_cardnumber
Bug 20287: Move trim values to a method
Bug 20287: Koha::Object->get_from_storage
Bug 20287: Move AddMember to Koha::Patron->store
Bug 20287: Replace occurrences of AddMember with Koha::Patron->new->store->borrowernumber
Bug 20287: Fix export issues in tests
Bug 20287: Remove warning
Bug 20287: New warning in t/db_dependent/Koha/Object.t (?)
Bug 20287: Use DBIC transaction instead of AutoCommit=0
Bug 20287: Remove AddMember_Auto
Bug 20287: Add plain_text_password (& Remove AddMember_Opac)
Bug 20287: Move ModMember to Koha::Patron
Bug 20287: ->store new deal with enrolment fee
Bug 20287: Fix test t/db_dependent/Members.t and prevent regressions
Bug 20287: There is a warning, we just want to unset the value here
Bug 20287: Replace ModMember in patrons import
Bug 20287: Fix update of patrons, clean the data before ->store
Bug 20287: generate_userid now set the userid
Bug 20287: Replace occurrences of AddMember in REST API
Bug 20287: Replace occurrences of ModMember in REST API
Bug 20287: Fix tests expecting a warning
Bug 20287: Use more simple object for Object.t
Bug 20287: Test exception DuplicateID for Koha::Patron->store
Bug 20287: (QA follow-up) Fix warning on undefined
Bug 20287: (follow-up) Test exception DuplicateID for Koha::Patron->store
Bug 20287: (RM follow-up) Restore use C4::Members

Description Jonathan Druart 2018-02-23 17:58:34 UTC

    
Comment 1 Jonathan Druart 2018-02-23 18:52:34 UTC
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.
Comment 2 Josef Moravec 2018-03-02 10:06:17 UTC
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.
Comment 3 Jonathan Druart 2018-03-28 20:33:32 UTC
Remove branch fixed and rebased.
Comment 4 Tomás Cohen Arazi 2018-03-29 16:38:14 UTC
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 5 Jonathan Druart 2018-03-29 17:26:20 UTC
Comment on attachment 73436 [details] [review]
Bug 20287: Fix tests expecting a warning

Pushed to the remote branch, thanks Tomas!
Comment 6 Katrin Fischer 2018-03-31 22:07:39 UTC
Tomas, could you sign-off?
Comment 7 Josef Moravec 2018-04-11 10:10:50 UTC
Signed of: https://gitlab.com/josef.moravec/Koha/tree/bug_20287
Comment 8 Kyle M Hall 2018-04-20 11:06:28 UTC
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
Comment 9 Jonathan Druart 2018-04-20 13:28:57 UTC
I have no idea how to fix this failure correctly, it comes from bug 20590.
Comment 10 Jonathan Druart 2018-05-16 15:58:09 UTC
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.
Comment 11 Tomás Cohen Arazi 2018-06-23 07:13:54 UTC
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.
Comment 12 Jonathan Druart 2018-06-25 16:18:59 UTC
(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.
Comment 13 Martin Renvoize 2018-07-12 14:33:04 UTC
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 :)
Comment 14 Jonathan Druart 2018-07-12 18:48:53 UTC
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>
Comment 15 Jonathan Druart 2018-07-12 18:48:58 UTC
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>
Comment 16 Jonathan Druart 2018-07-12 18:49:05 UTC
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>
Comment 17 Jonathan Druart 2018-07-12 18:49:15 UTC
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>
Comment 18 Jonathan Druart 2018-07-12 18:49:23 UTC
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>
Comment 19 Jonathan Druart 2018-07-12 18:49:27 UTC
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>
Comment 20 Jonathan Druart 2018-07-12 18:49:32 UTC
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>
Comment 21 Jonathan Druart 2018-07-12 18:49:36 UTC
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>
Comment 22 Jonathan Druart 2018-07-12 18:49:41 UTC
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>
Comment 23 Jonathan Druart 2018-07-12 18:49:46 UTC
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>
Comment 24 Jonathan Druart 2018-07-12 18:49:52 UTC
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>
Comment 25 Jonathan Druart 2018-07-12 18:49:57 UTC
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>
Comment 26 Jonathan Druart 2018-07-12 18:50:02 UTC
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>
Comment 27 Jonathan Druart 2018-07-12 18:50:06 UTC
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>
Comment 28 Jonathan Druart 2018-07-12 18:50:11 UTC
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>
Comment 29 Jonathan Druart 2018-07-12 18:50:17 UTC
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>
Comment 30 Jonathan Druart 2018-07-12 18:50:22 UTC
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>
Comment 31 Jonathan Druart 2018-07-12 18:50:26 UTC
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>
Comment 32 Jonathan Druart 2018-07-12 18:50:31 UTC
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>
Comment 33 Jonathan Druart 2018-07-12 18:50:35 UTC
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>
Comment 34 Jonathan Druart 2018-07-12 18:50:40 UTC
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>
Comment 35 Jonathan Druart 2018-07-12 18:50:45 UTC
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>
Comment 36 Jonathan Druart 2018-07-12 18:50:51 UTC
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>
Comment 37 Jonathan Druart 2018-07-12 18:50:56 UTC
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>
Comment 38 Jonathan Druart 2018-07-12 19:34:32 UTC
Created attachment 76927 [details] [review]
Bug 20287: (follow-up) Test exception DuplicateID for Koha::Patron->store
Comment 39 Nick Clemens 2018-07-18 19:18:52 UTC
Awesome work all!

Pushed to master for 18.11
Comment 40 Nick Clemens 2018-07-24 14:49:14 UTC
Created attachment 77221 [details] [review]
Bug 20287: (RM follow-up) Restore use C4::Members
Comment 41 Nick Clemens 2018-07-24 17:45:42 UTC
Follow-up patch pushed to master
Comment 42 Martin Renvoize 2018-08-01 08:09:11 UTC
Enhancement, not backporting to 18.05.x series.
Comment 43 Mark Tompsett 2018-09-06 17:00:26 UTC
*** Bug 21110 has been marked as a duplicate of this bug. ***
Comment 44 Martin Renvoize 2018-10-15 10:28:40 UTC
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)