Bug 19004

Summary: Koha/Patrons.t fails when item-level_itypes is not set
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Test SuiteAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, kyle, mtompset
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19004: Patrons.t should create its own data for enrollment fees.
Bug 19004: Adjust AddReturn for retrieving item type
Bug 19004: Patrons.t should create its own data for enrollment fees.
Bug 19004: Adjust AddReturn for retrieving item type
Bug 19004: build_object takes care of existing data
Bug 19004: [QA Follow-up] No need to check item-level_itypes again
Bug 19004: Patrons.t should create its own data for enrollment fees.
Bug 19004: Adjust AddReturn for retrieving item type
Bug 19004: [QA Follow-up] No need to check item-level_itypes again

Description Marcel de Rooy 2017-07-31 06:34:57 UTC
Problem is that TestBuilder generates two biblio records, one via item.biblionumber and one via items.biblioitemnumber => biblioitem.biblionumber.
(Which actually is bad design; the biblionumber should not be in the items table.) The biblio generated via items.biblionumber has no biblioitem.

AddReturn is called in Patrons.t. When item-level_itypes is false, it gets the itemtype from $biblio->biblioitem->itemtype. Which generates an error on the biblio since it has not biblioitem.
Comment 1 Marcel de Rooy 2017-08-10 08:09:34 UTC
Created attachment 65789 [details] [review]
Bug 19004: Patrons.t should create its own data for enrollment fees.

If the patron categories J, K, YA would not exist, Patrons.t would fail.

Test plan:
[1] Remove one of these patron categories.
[2] Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-08-10 08:09:38 UTC
Created attachment 65790 [details] [review]
Bug 19004: Adjust AddReturn for retrieving item type

In the regular situation, you can get itemtype via biblio and then
biblioitem as well as via biblioitem (at least when item-level_itypes
is set to biblio).

But since Koha unfortunately defined two relations in item, one for
biblioitemnumber (the good one) and one for biblionumber (redundant),
TestBuilder (correctly) builds one biblioitem and two biblios.

If you item-level_itypes to biblio record, this will result in failing tests
when calling AddReturn (in this case Koha/Patrons.t).
It will crash on:
    Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826.
Cause: AddReturn goes via the biblionumber to biblio and than to
biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder
but a database design problem.)

This patch makes a small change in AddReturn to retrieve the itemtype via
biblioitem. It actually is a shorter road than items->biblio->biblioitems.

Note: I do not test the Biblioitems->find call, since we already checked
the GetItem call before and we have a foreign key constraint.
I did not call $item->effective_itemtype since we still use GetItem; this
could be done later.

Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and
we called AddBiblio before, we should link biblioitemnumber as well.

Test plan:
[1] Do not apply this patch yet.
[2] Set item-level_itypes to biblio record.
[3] Run t/db_dependent/Koha/Patrons.t. (It should fail.)
[4] Apply this patch.
[5] Run t/db_dependent/Koha/Patrons.t again.
[6] Run t/db_dependent/Circulation/Returns.t
[7] Git grep on AddReturn and run a few other tests calling it.
    Note: Bugs 19070/19071 address three tests that call AddReturn too.
[8] In the interface, check in a book.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2017-08-10 08:16:11 UTC
Note:
The design problem around items.biblionumber is a hard one since it is widely used field.
Fixing the issue by adjusting TestBuilder is a workaround I thought about, but I did not want to introduce that change here.
The change on AddReturn is a tiny improvement and fixes some tests calling it in combination with biblio level itemtypes.
Comment 4 Mark Tompsett 2017-08-15 18:31:18 UTC
vagrant@kohadevbox:kohaclone(bug_19004)$ sudo koha-shell -c bash kohadev
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove `grep AddReturn \`find . -type f -name "*.t"\` | cut -f1 -d: | sort -u`
Comment 5 Mark Tompsett 2017-08-15 18:38:43 UTC
Created attachment 66037 [details] [review]
Bug 19004: Patrons.t should create its own data for enrollment fees.

If the patron categories J, K, YA would not exist, Patrons.t would fail.

Test plan:
[1] Remove one of these patron categories.
[2] Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2017-08-15 18:38:46 UTC
Created attachment 66038 [details] [review]
Bug 19004: Adjust AddReturn for retrieving item type

In the regular situation, you can get itemtype via biblio and then
biblioitem as well as via biblioitem (at least when item-level_itypes
is set to biblio).

But since Koha unfortunately defined two relations in item, one for
biblioitemnumber (the good one) and one for biblionumber (redundant),
TestBuilder (correctly) builds one biblioitem and two biblios.

If you item-level_itypes to biblio record, this will result in failing tests
when calling AddReturn (in this case Koha/Patrons.t).
It will crash on:
    Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826.
Cause: AddReturn goes via the biblionumber to biblio and than to
biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder
but a database design problem.)

This patch makes a small change in AddReturn to retrieve the itemtype via
biblioitem. It actually is a shorter road than items->biblio->biblioitems.

Note: I do not test the Biblioitems->find call, since we already checked
the GetItem call before and we have a foreign key constraint.
I did not call $item->effective_itemtype since we still use GetItem; this
could be done later.

Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and
we called AddBiblio before, we should link biblioitemnumber as well.

Test plan:
[1] Do not apply this patch yet.
[2] Set item-level_itypes to biblio record.
[3] Run t/db_dependent/Koha/Patrons.t. (It should fail.)
[4] Apply this patch.
[5] Run t/db_dependent/Koha/Patrons.t again.
[6] Run t/db_dependent/Circulation/Returns.t
[7] Git grep on AddReturn and run a few other tests calling it.
    Note: Bugs 19070/19071 address three tests that call AddReturn too.
[8] In the interface, check in a book.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Note: Bugs 19070 and 19071 are already pushed. The command in comment #4
      has all the tests successful.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Marcel de Rooy 2017-08-16 09:55:59 UTC
(In reply to M. Tompsett from comment #6)
> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Thanks
Comment 8 Jonathan Druart 2017-08-25 15:21:22 UTC
Created attachment 66486 [details] [review]
Bug 19004: build_object takes care of existing data

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2017-08-25 15:28:25 UTC
Comment on attachment 66038 [details] [review]
Bug 19004: Adjust AddReturn for retrieving item type

Review of attachment 66038 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +1822,5 @@
>  
>      my $itemnumber = $item->{ itemnumber };
> +    my $itemtype = C4::Context->preference("item-level_itypes")
> +        ? $item->{itype}
> +        : Koha::Biblioitems->find( $item->{biblioitemnumber} )->itemtype;

Was this code correct?
Sorry if this is silly, but I am lacking time at the moment to take a look correctly:
Why do not we just get $item->{itype} as the item-level_itypes is checked in GetItem (via Koha::Item->effective_itemtype)?
Comment 10 Jonathan Druart 2017-08-25 15:30:24 UTC
Actually either the previous version was good (to handle specific cases, i.e. wrong data) and yours is wrong, or we could just use $item->{itype}.
Comment 11 Marcel de Rooy 2017-08-28 06:46:15 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 66486 [details] [review] [review]
> Bug 19004: build_object takes care of existing data
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

It works, but it will trigger a "unique violation" warning from build. Which is valid since build should create data not find it. (I once proposed a flag to suppress this warning, but it was not welcomed.)
Comment 12 Marcel de Rooy 2017-08-28 06:51:06 UTC
(In reply to Jonathan Druart from comment #10)
> Actually either the previous version was good (to handle specific cases,
> i.e. wrong data) and yours is wrong, or we could just use $item->{itype}.

We can just use $item->{itype}.
Comment 13 Marcel de Rooy 2017-08-28 06:52:55 UTC
Created attachment 66546 [details] [review]
Bug 19004: [QA Follow-up] No need to check item-level_itypes again

As Jonathan pointed out, GetItem already called effective_itemtype.
So we can just use $item->{itype} here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2017-08-28 06:57:59 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #8)
> > Created attachment 66486 [details] [review] [review] [review]
> > Bug 19004: build_object takes care of existing data
> > 
> > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
> 
> It works, but it will trigger a "unique violation" warning from build. Which
> is valid since build should create data not find it. (I once proposed a flag
> to suppress this warning, but it was not welcomed.)

It does not work:
# Subtest: add_enrolment_fee_if_needed
    1..4
Violation of unique constraint in Category at /usr/share/koha/masterclone/t/lib/
TestBuilder.pm line 233.
    # No tests run!
not ok 14 - No tests run for subtest "add_enrolment_fee_if_needed"
#   Failed test 'No tests run for subtest "add_enrolment_fee_if_needed"'
#   at t/db_dependent/Koha/Patrons.t line 426.
Can't call method "enrolmentfee" on an undefined value at t/db_dependent/Koha/Pa
trons.t line 381.
# Looks like your test exited with 255 just after 14.

Obsoleting this patch.
Comment 15 Kyle M Hall 2017-09-01 12:46:46 UTC
Created attachment 66721 [details] [review]
Bug 19004: Patrons.t should create its own data for enrollment fees.

If the patron categories J, K, YA would not exist, Patrons.t would fail.

Test plan:
[1] Remove one of these patron categories.
[2] Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2017-09-01 12:46:53 UTC
Created attachment 66722 [details] [review]
Bug 19004: Adjust AddReturn for retrieving item type

In the regular situation, you can get itemtype via biblio and then
biblioitem as well as via biblioitem (at least when item-level_itypes
is set to biblio).

But since Koha unfortunately defined two relations in item, one for
biblioitemnumber (the good one) and one for biblionumber (redundant),
TestBuilder (correctly) builds one biblioitem and two biblios.

If you item-level_itypes to biblio record, this will result in failing tests
when calling AddReturn (in this case Koha/Patrons.t).
It will crash on:
    Can't call method "itemtype" on an undefined value at C4/Circulation.pm line 1826.
Cause: AddReturn goes via the biblionumber to biblio and than to
biblioitems, and it does not find a biblioitem. (Not a fault from TestBuilder
but a database design problem.)

This patch makes a small change in AddReturn to retrieve the itemtype via
biblioitem. It actually is a shorter road than items->biblio->biblioitems.

Note: I do not test the Biblioitems->find call, since we already checked
the GetItem call before and we have a foreign key constraint.
I did not call $item->effective_itemtype since we still use GetItem; this
could be done later.

Adjusted Circulation/Returns.t too: If we add an item with TestBuilder and
we called AddBiblio before, we should link biblioitemnumber as well.

Test plan:
[1] Do not apply this patch yet.
[2] Set item-level_itypes to biblio record.
[3] Run t/db_dependent/Koha/Patrons.t. (It should fail.)
[4] Apply this patch.
[5] Run t/db_dependent/Koha/Patrons.t again.
[6] Run t/db_dependent/Circulation/Returns.t
[7] Git grep on AddReturn and run a few other tests calling it.
    Note: Bugs 19070/19071 address three tests that call AddReturn too.
[8] In the interface, check in a book.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Note: Bugs 19070 and 19071 are already pushed. The command in comment #4
      has all the tests successful.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2017-09-01 12:46:56 UTC
Created attachment 66723 [details] [review]
Bug 19004: [QA Follow-up] No need to check item-level_itypes again

As Jonathan pointed out, GetItem already called effective_itemtype.
So we can just use $item->{itype} here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Jonathan Druart 2017-09-01 13:14:45 UTC
Note to myself:

-    my $item_level_itypes = C4::Context->preference("item-level_itypes");
-    my $biblio   = $item_level_itypes ? undef : Koha::Biblios->find( $item->{ biblionumber } ); # don't get bib data unless we need it
-    my $itemtype = $item_level_itypes ? $item->{itype} : $biblio->biblioitem->itemtype;
+    my $itemtype = $item->{itype}; # GetItem called effective_itemtype

This is not equal, but the previous code was buggy.
Comment 19 Jonathan Druart 2017-09-01 16:07:00 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 20 Fridolin Somers 2017-10-03 09:59:31 UTC
Pushed to 17.05.x, will be in 17.05.05.

effective_itemtype method is awsome, it should be used everywhere.
Comment 21 Katrin Fischer 2017-10-06 06:14:28 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.