Bug 18182 - TestBuilder should be able to return Koha::Object objects
Summary: TestBuilder should be able to return Koha::Object objects
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on: 18427
Blocks: 18339 18361
  Show dependency treegraph
 
Reported: 2017-02-28 11:55 UTC by Tomás Cohen Arazi
Modified: 2017-04-24 18:21 UTC (History)
5 users (show)

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


Attachments
Bug 18182: Make TestBuilder capable of returning Koha::Object (3.91 KB, patch)
2017-03-03 23:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: (followup) Add POD (952 bytes, patch)
2017-03-03 23:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: Make TestBuilder capable of returning Koha::Object (4.17 KB, patch)
2017-03-29 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: Make TestBuilder capable of returning Koha::Object (4.26 KB, patch)
2017-03-31 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback (2.20 KB, patch)
2017-03-31 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18182: Tests all Koha::Objects-based modules (1.83 KB, patch)
2017-04-12 18:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18182: Fix Koha::RefundLostItemFeeRules (664 bytes, patch)
2017-04-12 18:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18182: Tests all Koha::Objects-based modules (1.97 KB, patch)
2017-04-13 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18182: Fix Koha::RefundLostItemFeeRules (761 bytes, patch)
2017-04-13 08:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18182: Make TestBuilder capable of returning Koha::Object (4.28 KB, patch)
2017-04-19 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback (2.22 KB, patch)
2017-04-19 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: Tests all Koha::Objects-based modules (1.99 KB, patch)
2017-04-19 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: Fix Koha::RefundLostItemFeeRules (776 bytes, patch)
2017-04-19 14:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18182: Make TestBuilder capable of returning Koha::Object (4.35 KB, patch)
2017-04-19 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback (2.29 KB, patch)
2017-04-19 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18182: Tests all Koha::Objects-based modules (2.06 KB, patch)
2017-04-19 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18182: Fix Koha::RefundLostItemFeeRules (850 bytes, patch)
2017-04-19 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-02-28 11:55:15 UTC
There's a common pattern in most tests we write:

my $object_hash = $builder->build({ source => 'TheSchema', value => { fixed => values, ... } });
my $id = $object->{the_id_field};
my $object = Koha::Objects->find($id);

< the actual tests >

It would be handy to have TestBuilder provide a way to actually retrieve the Koha::Object instance.
Comment 1 Tomás Cohen Arazi 2017-03-03 23:10:45 UTC
Created attachment 60819 [details] [review]
Bug 18182: Make TestBuilder capable of returning Koha::Object

This patch adds a new method to t::lib::TestBuilder so it can return
Koha::Object-derived objects. The new method is called ->build_object
and requires the plural of the target class to be passed, along with the
field to be used as id on searching the object.

The plural is because the singular form is not aware of its plural (I
always thought we should move plural stuff into the singular class, this
is one of the reasons).

The id is required because that's the way to find the recently created
object.

If any of those parameters is ommited, a warning is raised and undef is
returned.

To test:
- Apply the patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/TestBuilder.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 2 Tomás Cohen Arazi 2017-03-03 23:10:50 UTC
Created attachment 60820 [details] [review]
Bug 18182: (followup) Add POD
Comment 3 Marcel de Rooy 2017-03-06 14:03:24 UTC
Great, Tomas.

Few remarks:
I would rather remove the id column. We c/should take care of that in the code.
Like:
my @pk = $schema->source('Borrower')->primary_columns;
Note that we theoretically can have multiple columns. (We don't have such Koha objects, but iirc we have one or two tables like that.)

Similarly, it would be more consistent and less confusing to use the singular form. We do so in build; I would expect the same for build_object. Additionally, you return just one singular object.
Comment 4 Tomás Cohen Arazi 2017-03-06 14:10:05 UTC
(In reply to Marcel de Rooy from comment #3)
> Great, Tomas.
> 
> Few remarks:
> I would rather remove the id column. We c/should take care of that in the
> code.
> Like:
> my @pk = $schema->source('Borrower')->primary_columns;
> Note that we theoretically can have multiple columns. (We don't have such
> Koha objects, but iirc we have one or two tables like that.)

I agree. Read the next answer to have context.

> Similarly, it would be more consistent and less confusing to use the
> singular form. We do so in build; I would expect the same for build_object.
> Additionally, you return just one singular object.

Singular classes are not aware of the plural class name. And we need the plural to ->find the created object. That's the design decision I don't like about koha::object(s). I'm open to opinions, but the only solutions are
-annotate singular classes so they know their plural
-move the plural methods to the singular classes and get rid of plural

I vote for the latter, of course. But that would be a bigger dev.
Comment 5 Marcel de Rooy 2017-03-06 14:22:24 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> Singular classes are not aware of the plural class name. And we need the
> plural to ->find the created object. That's the design decision I don't like
> about koha::object(s). I'm open to opinions, but the only solutions are
> -annotate singular classes so they know their plural
> -move the plural methods to the singular classes and get rid of plural
> 
> I vote for the latter, of course. But that would be a bigger dev.

Did you look at this one: sub _get_objects_class
$ git grep koha_objects_class
Koha/Objects.pm:    if( $type->can('koha_objects_class') ) {
Koha/Objects.pm:        return $type->koha_objects_class;
Koha/Schema/Result/Borrower.pm:sub koha_objects_class {
Koha/Schema/Result/Branch.pm:sub koha_objects_class {
Koha/Schema/Result/OldIssue.pm:sub koha_objects_class {

So cool :)
Comment 6 Tomás Cohen Arazi 2017-03-06 16:08:20 UTC
(In reply to Marcel de Rooy from comment #5)
> (In reply to Tomás Cohen Arazi from comment #4)
> > Singular classes are not aware of the plural class name. And we need the
> > plural to ->find the created object. That's the design decision I don't like
> > about koha::object(s). I'm open to opinions, but the only solutions are
> > -annotate singular classes so they know their plural
> > -move the plural methods to the singular classes and get rid of plural
> > 
> > I vote for the latter, of course. But that would be a bigger dev.
> 
> Did you look at this one: sub _get_objects_class
> $ git grep koha_objects_class
> Koha/Objects.pm:    if( $type->can('koha_objects_class') ) {
> Koha/Objects.pm:        return $type->koha_objects_class;
> Koha/Schema/Result/Borrower.pm:sub koha_objects_class {
> Koha/Schema/Result/Branch.pm:sub koha_objects_class {
> Koha/Schema/Result/OldIssue.pm:sub koha_objects_class {
> 
> So cool :)

So we annotate our singular classes.
Comment 7 Tomás Cohen Arazi 2017-03-29 19:36:11 UTC
Created attachment 61702 [details] [review]
Bug 18182: Make TestBuilder capable of returning Koha::Object

This patch adds a new method to t::lib::TestBuilder so it can return
Koha::Object-derived objects. The new method is called ->build_object
and requires the plural of the target class to be passed.

'class' is a mandatory param, and a warning is raised and undef is
returned if absent.

It accepts 'value' as the original ->build() method, and that is passed as-is
to ->build().

To test:
- Apply the patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/TestBuilder.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 8 Tomás Cohen Arazi 2017-03-29 19:40:49 UTC
Marcel, I thought a bit more about this and...

(In reply to Marcel de Rooy from comment #3)
> Great, Tomas.
> 
> Few remarks:
> I would rather remove the id column. We c/should take care of that in the
> code.
> Like:
> my @pk = $schema->source('Borrower')->primary_columns;
> Note that we theoretically can have multiple columns. (We don't have such
> Koha objects, but iirc we have one or two tables like that.)

I fixed it so the id is retrieved from the source. I did it so multi-pk works as expected, and changed the tests so they use a multi key sample Koha::Object.

> Similarly, it would be more consistent and less confusing to use the
> singular form. We do so in build; I would expect the same for build_object.
> Additionally, you return just one singular object.

I decided not to do it, but instead enforce the idea that plurals should be used. I even changed the wiki about this. Because singular vs. plural leads (everywhere) to circular deps (boo). And so we need to ONLY include plurals everywhere. The plural already loads the singular, so it will always be available. This should be explained more bradly, but I think we should leave the plural here to simplify things.

I hope you agree and we move this forward, as it will make writing tests more seamless.
Comment 9 Marcel de Rooy 2017-03-31 11:12:11 UTC
$categorycode, 'Firstname correctly set' );

LOL
Comment 10 Marcel de Rooy 2017-03-31 11:28:49 UTC
This line in Koha::Objects prevents us from just sending @pks to find:

my $result = $self->_resultset()->find($id);

Koha::Objects here assumes one id, which is not correct.
Since Koha::IssuingRule has a composite primary key.
Your workaround is fine, but I think we should solve this on a new report.
Comment 11 Marcel de Rooy 2017-03-31 11:36:22 UTC
Created attachment 61752 [details] [review]
Bug 18182: Make TestBuilder capable of returning Koha::Object

This patch adds a new method to t::lib::TestBuilder so it can return
Koha::Object-derived objects. The new method is called ->build_object
and requires the plural of the target class to be passed.

'class' is a mandatory param, and a warning is raised and undef is
returned if absent.

It accepts 'value' as the original ->build() method, and that is passed as-is
to ->build().

To test:
- Apply the patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/TestBuilder.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-03-31 11:36:26 UTC
Created attachment 61753 [details] [review]
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback

Since we here only use one rollback and this test does not care, it is
more consistent to keep that pattern.
Additionally, promoting two globals to our.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-03-31 11:41:52 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> I decided not to do it, but instead enforce the idea that plurals should be
> used. I even changed the wiki about this. Because singular vs. plural leads
> (everywhere) to circular deps (boo). And so we need to ONLY include plurals
> everywhere. The plural already loads the singular, so it will always be
> available. This should be explained more bradly, but I think we should leave
> the plural here to simplify things.

OK. Not sure if we should call it a circular dependency btw, since it is fully OO.
Comment 14 Jonathan Druart 2017-04-12 18:20:36 UTC
Created attachment 62113 [details] [review]
Bug 18182: Tests all Koha::Objects-based modules
Comment 15 Jonathan Druart 2017-04-12 18:20:41 UTC
Created attachment 62114 [details] [review]
Bug 18182: Fix Koha::RefundLostItemFeeRules
Comment 16 Jonathan Druart 2017-04-12 18:27:38 UTC
I have added tests, but one fails on 'Koha::Serial::Item'.
Could you take a look?
Comment 17 Marcel de Rooy 2017-04-13 08:19:22 UTC
(In reply to Jonathan Druart from comment #16)
> I have added tests, but one fails on 'Koha::Serial::Item'.
> Could you take a look?

 CREATE TABLE `serialitems` (
  `itemnumber` int(11) NOT NULL,
  `serialid` int(11) NOT NULL,
  UNIQUE KEY `serialitemsidx` (`itemnumber`),
  KEY `serialitems_sfk_1` (`serialid`),
  CONSTRAINT `serialitems_sfk_1` FOREIGN KEY (`serialid`) REFERENCES `serial` (`serialid`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `serialitems_sfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE CASCADE ON UPDATE CASCADE
)

Serialitems should have a primary key. Since itemnumber is a unique key, we could promote it to PK as well. Should be solved on a different report.
Comment 18 Marcel de Rooy 2017-04-13 08:23:34 UTC
Created attachment 62121 [details] [review]
Bug 18182: Tests all Koha::Objects-based modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added test description on line 387.
Comment 19 Marcel de Rooy 2017-04-13 08:23:38 UTC
Created attachment 62122 [details] [review]
Bug 18182: Fix Koha::RefundLostItemFeeRules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2017-04-13 12:09:26 UTC
(In reply to Marcel de Rooy from comment #17)
> Serialitems should have a primary key. Since itemnumber is a unique key, we
> could promote it to PK as well. Should be solved on a different report.

Solved on bug 18427 now.
Comment 21 Tomás Cohen Arazi 2017-04-19 14:23:29 UTC
Created attachment 62371 [details] [review]
Bug 18182: Make TestBuilder capable of returning Koha::Object

This patch adds a new method to t::lib::TestBuilder so it can return
Koha::Object-derived objects. The new method is called ->build_object
and requires the plural of the target class to be passed.

'class' is a mandatory param, and a warning is raised and undef is
returned if absent.

It accepts 'value' as the original ->build() method, and that is passed as-is
to ->build().

To test:
- Apply the patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/TestBuilder.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2017-04-19 14:23:35 UTC
Created attachment 62372 [details] [review]
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback

Since we here only use one rollback and this test does not care, it is
more consistent to keep that pattern.
Additionally, promoting two globals to our.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2017-04-19 14:23:43 UTC
Created attachment 62373 [details] [review]
Bug 18182: Tests all Koha::Objects-based modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added test description on line 387.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2017-04-19 14:23:49 UTC
Created attachment 62374 [details] [review]
Bug 18182: Fix Koha::RefundLostItemFeeRules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Jonathan Druart 2017-04-19 14:47:55 UTC
Created attachment 62383 [details] [review]
Bug 18182: Make TestBuilder capable of returning Koha::Object

This patch adds a new method to t::lib::TestBuilder so it can return
Koha::Object-derived objects. The new method is called ->build_object
and requires the plural of the target class to be passed.

'class' is a mandatory param, and a warning is raised and undef is
returned if absent.

It accepts 'value' as the original ->build() method, and that is passed as-is
to ->build().

To test:
- Apply the patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/TestBuilder.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

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 26 Jonathan Druart 2017-04-19 14:48:00 UTC
Created attachment 62384 [details] [review]
Bug 18182: [QA Follow-up] Fix a few typos, consistent rollback

Since we here only use one rollback and this test does not care, it is
more consistent to keep that pattern.
Additionally, promoting two globals to our.

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 27 Jonathan Druart 2017-04-19 14:48:15 UTC
Created attachment 62385 [details] [review]
Bug 18182: Tests all Koha::Objects-based modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added test description on line 387.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2017-04-19 14:48:21 UTC
Created attachment 62386 [details] [review]
Bug 18182: Fix Koha::RefundLostItemFeeRules

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 29 Kyle M Hall 2017-04-21 18:12:10 UTC
Pushed to master for 17.05, thanks Tomas, Marcel, Jonathan!
Comment 30 Katrin Fischer 2017-04-21 23:19:56 UTC
This won't get ported back to 16.11.x as it is an enhancement.