Bug 20287 - Move AddMember and ModMember to Koha::Patron
Summary: Move AddMember and ModMember to Koha::Patron
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 16846
  Show dependency treegraph
 
Reported: 2018-02-23 17:58 UTC by Jonathan Druart
Modified: 2018-06-23 07:13 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20287: Fix tests expecting a warning (1.73 KB, patch)
2018-03-29 16:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.