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.
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>
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>
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.
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`
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>
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>
(In reply to M. Tompsett from comment #6) > Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Thanks
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 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)?
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}.
(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.)
(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}.
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>
(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.
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>
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>
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>
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.
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.05. effective_itemtype method is awsome, it should be used everywhere.
These patches have been pushed to 16.11.x and will be in 16.11.13.