[Bug 16671] New: Wrong itemtype picked in HoldsQueue.t
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Bug ID: 16671 Summary: Wrong itemtype picked in HoldsQueue.t Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|gmcharlt@gmail.com |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |14828 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14828 [Bug 14828] Move the item types related code to Koha::ItemTypes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52078 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52078&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #2 from Héctor Eduardo Castro Avalos <hector.hecaxmmx@gmail.com> --- Created attachment 52096 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52096&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Héctor Eduardo Castro Avalos <hector.hecaxmmx@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hector.hecaxmmx@gmail.com Attachment #52078|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Héctor Eduardo Castro Avalos <hector.hecaxmmx@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14514 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14514 [Bug 14514] LocalHoldsPriority and the HoldsQueue conflict with each other -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Marcel, I'd say the tests expect at least one notforloan itemtype, could you confirm that you have none? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52887 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52887&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Jonathan: Please confirm if this works for you too. What about the other itemtype lines ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52887|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52888 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52888&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #11 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52921 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52921&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52888|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52922 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52922&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 52923 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52923&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA --- Comment #15 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52096|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jonathan! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr Status|Pushed to Master |Pushed to Stable --- Comment #17 from Frédéric Demians <frederic@tamil.fr> --- Pushed in 16.05. Will be in 16.05.02. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16671 Bug 16671 depends on bug 14828, which changed state. Bug 14828 Summary: Move the item types related code to Koha::ItemTypes https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14828 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org