Bug 16671 - Wrong itemtype picked in HoldsQueue.t
Summary: Wrong itemtype picked in HoldsQueue.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 14828
Blocks: 14514
  Show dependency treegraph
 
Reported: 2016-06-06 13:00 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16671: (bug 14828 follow-up) Pick the right itemtypes (1.71 KB, patch)
2016-06-06 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 16671: (bug 14828 follow-up) Pick the right itemtypes (1.83 KB, patch)
2016-06-06 19:21 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 16671: [QA Follow-up] Add new itemtype and remove bailout (1.47 KB, patch)
2016-06-27 07:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16671: [QA Follow-up] Add new itemtype and remove bailout (1.49 KB, patch)
2016-06-27 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16671: (bug 14828 follow-up) Pick the right itemtypes (1.93 KB, patch)
2016-06-28 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16671: [QA Follow-up] Add new itemtype and remove bailout (1.54 KB, patch)
2016-06-28 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16671: [QA Follow-up] Replace remaining itemtype lines (2.69 KB, patch)
2016-06-28 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-06-06 13:00:42 UTC
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.
Comment 1 Jonathan Druart 2016-06-06 13:05:06 UTC Comment hidden (obsolete)
Comment 2 Héctor Eduardo Castro Avalos 2016-06-06 19:21:03 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-06-24 11:51:53 UTC
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..
Comment 4 Jonathan Druart 2016-06-24 13:38:41 UTC
Marcel, I'd say the tests expect at least one notforloan itemtype, could you confirm that you have none?
Comment 5 Jonathan Druart 2016-06-24 13:39:11 UTC
(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!
Comment 6 Jonathan Druart 2016-06-24 13:41:10 UTC
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.
Comment 7 Marcel de Rooy 2016-06-27 07:00:07 UTC
(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
Comment 8 Marcel de Rooy 2016-06-27 07:52:50 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2016-06-27 07:53:33 UTC
Jonathan: Please confirm if this works for you too.
What about the other itemtype lines ?
Comment 10 Jonathan Druart 2016-06-27 12:16:39 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-06-27 12:17:49 UTC
(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.
Comment 12 Marcel de Rooy 2016-06-28 13:40:04 UTC
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.
Comment 13 Marcel de Rooy 2016-06-28 13:40:08 UTC
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>
Comment 14 Marcel de Rooy 2016-06-28 13:40:11 UTC
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>
Comment 15 Marcel de Rooy 2016-06-28 13:45:47 UTC
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.
Comment 16 Kyle M Hall 2016-07-08 12:47:58 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 17 Frédéric Demians 2016-08-01 13:21:41 UTC
Pushed in 16.05. Will be in 16.05.02.