From bug 14828: -my @item_types = C4::ItemType->all; -my @for_loan = grep { $_->{notforloan} == 0 } @item_types - or BAIL_OUT("No adequate itemtype"); -my $itemtype = $for_loan[0]->{itemtype}; +my $itemtype = Koha::ItemTypes->search({ notforloan => 1 })->next; +$itemtype or BAIL_OUT("No adequate itemtype"); #FIXME Should be $itemtype = $itemtype->itemtype (First you are allowed to wonder what this FIXME has not been done...) The wrong itemtypes are picked from the DB, we want itemtypes with notforloan == 0, not 1.
Created attachment 52078 [details] [review] Bug 16671: (bug 14828 follow-up) Pick the right itemtypes Looking at commit aafe73eefb5151454fa8957bf188768324d23955 Bug 14828: Use Koha::ItemType[s] everywhere C4::ItemType was used -my @item_types = C4::ItemType->all; -my @for_loan = grep { $_->{notforloan} == 0 } @item_types - or BAIL_OUT("No adequate itemtype"); -my $itemtype = $for_loan[0]->{itemtype}; +my $itemtype = Koha::ItemTypes->search({ notforloan => 1 })->next; +$itemtype or BAIL_OUT("No adequate itemtype"); #FIXME Should be $itemtype = $itemtype->itemtype It seems that the tests expect itemtypes for loan. Test plan: prove t/db_dependent/HoldsQueue.t should still return green
Created attachment 52096 [details] [review] [SIGNED-OFF]Bug 16671: (bug 14828 follow-up) Pick the right itemtypes Looking at commit aafe73eefb5151454fa8957bf188768324d23955 Bug 14828: Use Koha::ItemType[s] everywhere C4::ItemType was used -my @item_types = C4::ItemType->all; -my @for_loan = grep { $_->{notforloan} == 0 } @item_types - or BAIL_OUT("No adequate itemtype"); -my $itemtype = $for_loan[0]->{itemtype}; +my $itemtype = Koha::ItemTypes->search({ notforloan => 1 })->next; +$itemtype or BAIL_OUT("No adequate itemtype"); #FIXME Should be $itemtype = $itemtype->itemtype It seems that the tests expect itemtypes for loan. Test plan: prove t/db_dependent/HoldsQueue.t should still return green Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as advertised. No qa test tool errors
not ok 11 - take from lowest cost branch (don't use cost matrix) pick up branch # got: undef # expected: 'KaDYNnP16' not ok 12 - take from lowest cost branch (use cost matrix) pick up branch # Failed test 'take from lowest cost branch (use cost matrix) pick up branch' # at t/db_dependent/HoldsQueue.t line 637. # got: undef # expected: 'KaDYNnP16' not ok 13 - GetHoldsQueueItems # Failed test 'GetHoldsQueueItems' # at t/db_dependent/HoldsQueue.t line 156. # Expected item for pick KaDYNnP16, hold OG7G, got $VAR1 = undef; not ok 14 - item type included in queued items list (bug 5825) # Failed test 'item type included in queued items list (bug 5825)' # at t/db_dependent/HoldsQueue.t line 160. # Looks like you planned 38 tests but ran 36. # Looks like you failed 4 tests of 36 run. Note that without your patch, this test passes with me. At least we need some clarification..
Marcel, I'd say the tests expect at least one notforloan itemtype, could you confirm that you have none?
(In reply to Jonathan Druart from comment #4) > Marcel, I'd say the tests expect at least one notforloan itemtype, could you > confirm that you have none? it's notforloan => 0, so you need at least 1 item type for loan!
Actually the tests should bailout if you have none. I can only say that the tests pass for me and that they were wrong (but passed) without this patch.
(In reply to Jonathan Druart from comment #5) > (In reply to Jonathan Druart from comment #4) > > Marcel, I'd say the tests expect at least one notforloan itemtype, could you > > confirm that you have none? > > it's notforloan => 0, so you need at least 1 item type for loan! I have both
Created attachment 52887 [details] [review] Bug 16671: [QA Follow-up] Add new itemtype and remove bailout No need to bail out, if we are using TestBuilder. Somehow, this test passes with me if we add a new record, and if we do not, it fails (using BK as the first itemtype with notforloan is 0). Looking further, we should imo remove the Koha::ItemTypes searches here and not depend on existing data. Just add some item types. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Jonathan: Please confirm if this works for you too. What about the other itemtype lines ?
Created attachment 52888 [details] [review] Bug 16671: [QA Follow-up] Add new itemtype and remove bailout No need to bail out, if we are using TestBuilder. Somehow, this test passes with me if we add a new record, and if we do not, it fails (using BK as the first itemtype with notforloan is 0). Looking further, we should imo remove the Koha::ItemTypes searches here and not depend on existing data. Just add some item types. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Marcel de Rooy from comment #9) > Jonathan: Please confirm if this works for you too. Yes it works! > What about the other itemtype lines ? Yes it would be better to create the data as well.
Created attachment 52921 [details] [review] Bug 16671: (bug 14828 follow-up) Pick the right itemtypes Looking at commit aafe73eefb5151454fa8957bf188768324d23955 Bug 14828: Use Koha::ItemType[s] everywhere C4::ItemType was used -my @item_types = C4::ItemType->all; -my @for_loan = grep { $_->{notforloan} == 0 } @item_types - or BAIL_OUT("No adequate itemtype"); -my $itemtype = $for_loan[0]->{itemtype}; +my $itemtype = Koha::ItemTypes->search({ notforloan => 1 })->next; +$itemtype or BAIL_OUT("No adequate itemtype"); #FIXME Should be $itemtype = $itemtype->itemtype It seems that the tests expect itemtypes for loan. Test plan: prove t/db_dependent/HoldsQueue.t should still return green Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as advertised. No qa test tool errors Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adding follow-up.
Created attachment 52922 [details] [review] Bug 16671: [QA Follow-up] Add new itemtype and remove bailout No need to bail out, if we are using TestBuilder. Somehow, this test passes with me if we add a new record, and if we do not, it fails (using BK as the first itemtype with notforloan is 0). Looking further, we should imo remove the Koha::ItemTypes searches here and not depend on existing data. Just add some item types. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52923 [details] [review] Bug 16671: [QA Follow-up] Replace remaining itemtype lines Instead of relying on existing data, we use new TestBuilder recs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This test does not depend on existing itemtypes anymore. That is fine. But I still have the feeling that there is some hidden bug in C4/HoldsQueue.pm somewhere.. (My suspect would be MapItemsToHoldRequests) The failing test always had to do with the first itemtype in the table (although it was for loan). But deleting all records and adding one did not fail.. Since it is not in the scope of this report, passing QA on this trivial correction.
Pushed to master for 16.11, thanks Jonathan!
Pushed in 16.05. Will be in 16.05.02.