Bug 15391

Summary: HoldsQueue.t does not handle for loan itemtypes correctly
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: normal    
Priority: P5 - low CC: aleisha, brendan, frederic, julian.maurice, katrin.fischer, kyle, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14828
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12803    
Attachments: Bug 15391: Fix HoldsQueue.t tests
Bug 15391: Fix HoldsQueue.t tests
[PASSED QA] Bug 15391: Fix HoldsQueue.t tests

Description Jonathan Druart 2015-12-17 10:16:00 UTC
We are checking if we have at least 1 item type not for loan:
 63 my @item_types = C4::ItemType->all;
 64 my $itemtype = grep { $_->{notforloan} == 1 } @item_types
 65   or BAIL_OUT("No adequate itemtype");

Then we use the $itemtype variable (which contains the number of item types not for loan):

 92 $dbh->do("INSERT INTO biblioitems (biblionumber, marcxml, itemtype)
 93           VALUES                  ($biblionumber, '', '$itemtype')");

There is obviously something wrong here.

The code should be

 64 my @not_for_loan = grep { $_->{notforloan} == 1 } @item_types
 65   or BAIL_OUT("No adequate itemtype");
 66 my $itemtype = $not_for_loan[0]->{itemtype};

But then some tests don't pass:

# Looks like you failed 8 tests of 17 run.
Comment 1 Jonathan Druart 2015-12-17 10:16:37 UTC
Kyle, could you have a look please?
I don't manage to get what is wrong here.
Comment 2 Kyle M Hall 2015-12-30 15:49:19 UTC
FYI, this is still on my radar, I just haven't gotten to it yet.
Comment 3 Kyle M Hall 2016-01-15 19:35:23 UTC
The "itemtype" from line 64 is being inserted into a biblioitems row at line , which does not have a foreign key to itemtypes, so it succeeds. The itemtype is re-used at line 98 for an item. It appears the test actually requires the item to be *for* loan, rather than not for loan.

Since the itemtype doesn't really exists, is seems to fail as a for loan itemtype, even though it seems like we should get an execution error instead.

Then, at line 181 the itemtype is replace with an arbitrary itemtype selected from the database, which is probably just a lucky grab since the default data would give a for loan itemtype first.
Comment 4 Kyle M Hall 2016-01-15 19:37:26 UTC
Also, I kind of expected this tests to fail when I set item level itypes to biblio, but it still continued to succeed. It seems that the selection of a not for loan itemtype is just cruft at this point.
Comment 5 Jonathan Druart 2016-01-19 17:00:43 UTC Comment hidden (obsolete)
Comment 6 Aleisha Amohia 2016-01-19 20:20:20 UTC
t/db_dependent/HoldsQueue.t .. 3/23 
#   Failed test 'Bug 15062 - Holds queue with Transport Cost Matrix will transfer item even if transfers disabled'
#   at t/db_dependent/HoldsQueue.t line 413.
#          got: '1'
#     expected: '0'
# Looks like you failed 1 test of 23.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/23 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 256 Tests: 23 Failed: 1)
  Failed test:  23
  Non-zero exit status: 1
Files=1, Tests=23,  1 wallclock secs ( 0.02 usr  0.01 sys +  1.39 cusr  0.08 csys =  1.50 CPU)
Result: FAIL
Comment 7 Jonathan Druart 2016-01-20 08:38:21 UTC
Aleisha, could you confirm the tests pass before the patch?
Comment 8 Kyle M Hall 2016-01-20 11:44:23 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2016-01-20 22:27:37 UTC
Created attachment 47061 [details] [review]
[PASSED QA] Bug 15391: Fix HoldsQueue.t tests

Prior to this patch, in HoldsQueue.t:
 63 my @item_types = C4::ItemType->all;
 64 my $itemtype = grep { $_->{notforloan} == 1 } @item_types
 65   or BAIL_OUT("No adequate itemtype");

Then we use the $itemtype variable (which contains the number of item types not for loan):

 92 $dbh->do("INSERT INTO biblioitems (biblionumber, marcxml, itemtype)
 93           VALUES                  ($biblionumber, '', '$itemtype')");

There is obviously something wrong here.

The code should be

 64 my @not_for_loan = grep { $_->{notforloan} == 1 } @item_types
 65   or BAIL_OUT("No adequate itemtype");
 66 my $itemtype = $not_for_loan[0]->{itemtype};

But then some tests don't pass:

Actually the problem comes from:
commit bfbc646fdd9ca4b90a0bc2751d0faa95d9e93ba1
  Bug 10336: HoldsQueue.t needs to create its own data

-my $itemtype = $dbh->selectrow_array("SELECT min(itemtype) FROM itemtypes WHERE notforloan = 0")
+my @item_types = C4::ItemType->all;
+my $itemtype = grep { $_->{notforloan} == 1 } @item_types

The line should have been:
my $itemtype = grep { $_->{notforloan} == 0 } @item_types

Test plan:
Confirm that the tests still pass after this patch applied.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
All tests pass pre and post patch

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Brendan Gallagher 2016-01-27 01:46:55 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks
Comment 11 Julian Maurice 2016-01-29 10:45:06 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 12 Frédéric Demians 2016-02-12 06:37:33 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.