Bug 18361

Summary: Koha::Objects->find should accept composite primary keys
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18427
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18182, 18539    
Bug Blocks: 18951    
Attachments: Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Additional tests for Koha::Objects->find
Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Additional tests for Koha::Objects->find
Bug 18361: (QA followup) Add tests for the no params case
Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Additional tests for Koha::Objects->find
Bug 18361: (QA followup) Add tests for the no params case
Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Additional tests for Koha::Objects->find
Bug 18361: (QA followup) Add tests for the no params case
Bug 18361: Koha::Objects->find should accept composite primary keys
Bug 18361: Additional tests for Koha::Objects->find
Bug 18361: (QA followup) Add tests for the no params case
Bug 18361: [QA Follow-up] Resolve warning on undef values supplied
Bug 18361: [SQUASHED FOR 16.11] Koha::Objects->find should accept composite primary keys

Description Marcel de Rooy 2017-03-31 11:46:52 UTC
Originates from bug 18182. Example is Koha::IssuingRule
Comment 1 Marcel de Rooy 2017-03-31 11:59:29 UTC
Created attachment 61754 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-03-31 12:01:44 UTC
The indirect way of testing the changed find via TestBuilder works.
Is it enough or should we add find tests in Koha/Objects.t?
Note that I perhaps would have expected such a subtest already, but it does not exist ;)
Comment 3 Jonathan Druart 2017-03-31 17:54:09 UTC
(In reply to Marcel de Rooy from comment #2)
> The indirect way of testing the changed find via TestBuilder works.
> Is it enough or should we add find tests in Koha/Objects.t?
> Note that I perhaps would have expected such a subtest already, but it does
> not exist ;)

Yes we should definitely have one in Objects.t
Comment 4 Tomás Cohen Arazi 2017-04-04 13:31:06 UTC
Comment on attachment 61754 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

> sub find {
>-    my ( $self, $id ) = @_;
>+    my ( $self, @id ) = @_;

While I agree with the general idea, I'd prefer that we keep in mind that this is a passthrough to DBIC ->find method. In that sense, please look at ResultSet.pm:

sub find {
  my $self = shift;
  my $attrs = (@_ > 1 && ref $_[$#_] eq 'HASH' ? pop(@_) : {});

IMHO we should implement exactly that, probably accept whatever is passed to K:O->find and just pass it to RS->find.
Comment 5 Marcel de Rooy 2017-04-04 13:38:22 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-04-13 11:20:29 UTC
Created attachment 62142 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-04-13 11:20:33 UTC
Created attachment 62143 [details] [review]
Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-04-13 11:23:13 UTC
Tomas: I adjusted the POD to make your concerns more visible, and renamed the @id array to a more generic name @pars, since it indeed may contain an attrs hash.
Additionally, I added a find subtest in Objects.t that contains a few tests where an attr hash is passed along to find.
Signoff time :)
Comment 9 Marcel de Rooy 2017-04-13 11:23:49 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > The indirect way of testing the changed find via TestBuilder works.
> > Is it enough or should we add find tests in Koha/Objects.t?
> > Note that I perhaps would have expected such a subtest already, but it does
> > not exist ;)
> 
> Yes we should definitely have one in Objects.t

Done
Comment 10 Tomás Cohen Arazi 2017-04-19 15:36:54 UTC
Created attachment 62395 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2017-04-19 15:37:01 UTC
Created attachment 62396 [details] [review]
Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-04-19 15:37:49 UTC
Created attachment 62397 [details] [review]
Bug 18361: (QA followup) Add tests for the no params case

This patch adds a test for the trivial case in which no param is passed
and the ->find method returns undef.

For completeness purposes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Marcel de Rooy 2017-04-20 06:29:15 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Thx
Comment 14 Marcel de Rooy 2017-05-01 11:11:58 UTC
Created attachment 62909 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Marcel de Rooy 2017-05-01 11:12:03 UTC
Created attachment 62910 [details] [review]
Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Marcel de Rooy 2017-05-01 11:12:07 UTC
Created attachment 62911 [details] [review]
Bug 18361: (QA followup) Add tests for the no params case

This patch adds a test for the trivial case in which no param is passed
and the ->find method returns undef.

For completeness purposes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Marcel de Rooy 2017-05-01 11:12:44 UTC
Jonathan: Please try again. I actually did not change anything.
Comment 18 Marcel de Rooy 2017-06-02 09:47:28 UTC
ping Jonathan
Comment 19 Jonathan Druart 2017-06-15 16:18:35 UTC
Created attachment 64339 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2017-06-15 16:19:26 UTC
Created attachment 64340 [details] [review]
Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2017-06-15 16:20:01 UTC
Created attachment 64341 [details] [review]
Bug 18361: (QA followup) Add tests for the no params case

This patch adds a test for the trivial case in which no param is passed
and the ->find method returns undef.

For completeness purposes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2017-06-15 18:31:57 UTC
Created attachment 64347 [details] [review]
Bug 18361: Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2017-06-15 18:32:03 UTC
Created attachment 64348 [details] [review]
Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2017-06-15 18:32:08 UTC
Created attachment 64349 [details] [review]
Bug 18361: (QA followup) Add tests for the no params case

This patch adds a test for the trivial case in which no param is passed
and the ->find method returns undef.

For completeness purposes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2017-06-15 18:33:50 UTC
Patches rebased on top of bug 18539
Comment 26 Jonathan Druart 2017-06-15 18:44:10 UTC
I get warnings from tests:

t/db_dependent/Letters.t ..................................... 3/82 DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): NULL/undef values supplied for requested unique constraint 'primary' (NULL values in column(s): 'id'). This is almost certainly not what you wanted, though you can set DBIC_NULLABLE_KEY_NOWARN to disable this warning. at /home/vagrant/kohaclone/Koha/Objects.pm line 91
Comment 27 Marcel de Rooy 2017-06-16 06:18:39 UTC
(In reply to Jonathan Druart from comment #26)
> I get warnings from tests:
> 
> t/db_dependent/Letters.t ..................................... 3/82
> DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint():
> NULL/undef values supplied for requested unique constraint 'primary' (NULL
> values in column(s): 'id'). This is almost certainly not what you wanted,
> though you can set DBIC_NULLABLE_KEY_NOWARN to disable this warning. at
> /home/vagrant/kohaclone/Koha/Objects.pm line 91

Good catch
Comment 28 Marcel de Rooy 2017-06-16 06:19:04 UTC
Created attachment 64372 [details] [review]
Bug 18361: [QA Follow-up] Resolve warning on undef values supplied

The following warning was raised in Letters.t:
DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint():
NULL/undef values supplied for requested unique constraint 'primary' (NULL
values in column(s): 'id'). This is almost certainly not what you wanted,
though you can set DBIC_NULLABLE_KEY_NOWARN to disable this warning.

This warning is triggered by this line in C4/Letters.pm:
    Koha::SMS::Providers->find( $member->{'sms_provider_id'} );
As you already guessed, the sms_provider_id returns undef.

Resolved in sub find by testing if there are parameters and if so, they
should not be all undefined. (In most cases there will be only one
parameter; but this report is about composite keys.)

Added a trivial test case in Objects.t too.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/Letters.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Jonathan Druart 2017-06-16 20:05:00 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 30 Fridolin Somers 2017-06-27 14:28:07 UTC
Enhancement not pushed to 17.05.x
Comment 31 Fridolin Somers 2017-08-03 07:47:01 UTC
I finally to push to 17.05.x in order to follow master using this change.

Will be in 17.05.03.
Comment 32 Katrin Fischer 2017-08-15 10:16:09 UTC
Conflicts with 16.11.x. If you want this to be included, please rebase on 16.11.x. Thx!
Comment 33 Marcel de Rooy 2017-08-15 12:12:22 UTC
Created attachment 66006 [details] [review]
Bug 18361: [SQUASHED FOR 16.11] Koha::Objects->find should accept composite primary keys

Changes the $id parameter to an array. (IssuingRule has three keys.)
The build_object method in TestBuilder.pm has been adjusted to pass
multiple primary key values to find.

Also adjusted the POD section to show more clearly that we accept
the same parameters as DBIx ResultSet does.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18361: Additional tests for Koha::Objects->find

Adding a subtest find in t/db_dependent/Koha/Objects.t.

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18361: (QA followup) Add tests for the no params case

This patch adds a test for the trivial case in which no param is passed
and the ->find method returns undef.

For completeness purposes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18361: [QA Follow-up] Resolve warning on undef values supplied

The following warning was raised in Letters.t:
DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint():
NULL/undef values supplied for requested unique constraint 'primary' (NULL
values in column(s): 'id'). This is almost certainly not what you wanted,
though you can set DBIC_NULLABLE_KEY_NOWARN to disable this warning.

This warning is triggered by this line in C4/Letters.pm:
    Koha::SMS::Providers->find( $member->{'sms_provider_id'} );
As you already guessed, the sms_provider_id returns undef.

Resolved in sub find by testing if there are parameters and if so, they
should not be all undefined. (In most cases there will be only one
parameter; but this report is about composite keys.)

Added a trivial test case in Objects.t too.

Test plan:
Run t/db_dependent/Koha/Object.t
Run t/db_dependent/Koha/Objects.t
Run t/db_dependent/Letters.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2017-08-15 12:12:50 UTC
(In reply to Katrin Fischer from comment #32)
> Conflicts with 16.11.x. If you want this to be included, please rebase on
> 16.11.x. Thx!

That you should do it..
Comment 35 Katrin Fischer 2017-08-16 20:57:00 UTC
Thx Marcel - patches apply now and tests pass.

This patch has been pushed to 16.11.x and will be in 16.11.11.