[Bug 9532] New: reserve rules with itemtype on biblio
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Bug ID: 9532 Summary: reserve rules with itemtype on biblio Classification: Unclassified Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: fridolyn.somers@biblibre.com CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com When itemtype is defined on biblio (item-level_itypes syspref), the method C4::Reserves::CanItemBeReserved uses item->{itemtype}. But item comes from C4::Items::GetItem and it does not have a 'itemtype' key, in this method the item type value is always in 'itype' key. The consequence is that if a rule defines 0 reservations for an itemtype and for example 10 for all itemtypes, the issuing rule used will always be the one with 10 and so will allow reservation. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |fridolyn.somers@biblibre.co |ity.org |m --- Comment #1 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 15016 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15016&action=edit Proposed patch See commit message -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15016|0 |1 is obsolete| | --- Comment #2 from Liz Rea <wizzyrea@gmail.com> --- Created attachment 15018 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15018&action=edit Bug 9532: reserve rules with itemtype on biblio When itemtype is defined on biblio (item-level_itypes syspref), the method C4::Reserves::CanItemBeReserved uses item->{itemtype}. But item comes from C4::Items::GetItem and it does not have a 'itemtype' key, in this method the item type value is always in 'itype' key. This patch corrects it. Test plan: You must have itemtype on biblio and 'item-level_itypes' syspref on biblio. This test plan is with ReservesControlBranch on ItemHomeLibrary. - Choose a branch, a borrower category and an item type, for example 'NYC', 'CHILD' and 'DVD' - Set an issuing rule for 'NYC', CHILD' and 'DVD' with 'Holds allowed' on 10 - Set an issuing rule for 'NYC', CHILD' and all item types with 'Holds allowed' on 0 - Choose an item of a biblio with itemtype 'DVD', that can be reserved, with 'NYC' as homebranch - Choose a borrower with category 'CHILD' - try to reverse the item for the borrower => without the patch, you can => with the patch, you can't You may check reserve is allowed with 'Holds allowed' > 0 on issuing rule for 'DVD'. Signed-off-by: Liz Rea <liz@catalyst.net.nz> Great test plan - thanks! Confirmed the bug, and the fix. Looks good to me. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- Hum... I don't see what this patch does :-/ There are 2 variables rename (itype with itemtypefield and itemtype with eruleitemtype) and what else? What I miss? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch QA Contact| |jonathan.druart@biblibre.co | |m --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- QA comment: After discussion with Fridolyn: In fact this patch would be: - my $itemtype = $item->{$itype}; + my $itemtype = $item->{'itype'}; Others changes are refactoring. I think a new patch could be fix C4::Items::GetItems in order to check the item-level_itypes syspref at this level. Marked as Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15018|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 16419 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16419&action=edit Bug 9532: reserve rules with itemtype on biblio When itemtype is defined on biblio (item-level_itypes syspref), the method C4::Reserves::CanItemBeReserved uses item->{itemtype}. But item comes from C4::Items::GetItem and it does not have a 'itemtype' key, in this method the item type value is always in 'itype' key. This patch corrects it. Test plan: You must have itemtype on biblio and 'item-level_itypes' syspref on biblio. This test plan is with ReservesControlBranch on ItemHomeLibrary. - Choose a branch, a borrower category and an item type, for example 'NYC', 'CHILD' and 'DVD' - Set an issuing rule for 'NYC', CHILD' and 'DVD' with 'Holds allowed' on 10 - Set an issuing rule for 'NYC', CHILD' and all item types with 'Holds allowed' on 0 - Choose an item of a biblio with itemtype 'DVD', that can be reserved, with 'NYC' as homebranch - Choose a borrower with category 'CHILD' - try to reverse the item for the borrower => without the patch, you can => with the patch, you can't You may check reserve is allowed with 'Holds allowed' > 0 on issuing rule for 'DVD'. Signed-off-by: Liz Rea <liz@catalyst.net.nz> Great test plan - thanks! Confirmed the bug, and the fix. Looks good to me. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #6 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- I am not sure this patch works correctly. Even with biblio-level item types, I am able to place a hold on an item which has the item-level item type set to Books when the biblio-level item type is "Computer Files," which do not allow holds. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 --- Comment #7 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 18061 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18061&action=edit Bug 9532: reserve rules with itemtype on biblio (followup) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18061|Bug 9532: reserve rules |Bug 9532: GetItem must use description|with itemtype on biblio |item-level_itypes syspref |(followup) | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18061|Bug 9532: GetItem must use |Bug 9532: Followup - description|item-level_itypes syspref |GetItem must use | |item-level_itypes syspref -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major --- Comment #8 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- (In reply to comment #6)
I am not sure this patch works correctly. Even with biblio-level item types, I am able to place a hold on an item which has the item-level item type set to Books when the biblio-level item type is "Computer Files," which do not allow holds.
Indeed, I ran the same test and notices that GetItem does not use item-level_itypes syspref, biblio itemtype is used only if items.itype is not defined. See followup. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18061|0 |1 is obsolete| | --- Comment #9 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 18487 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18487&action=edit Bug 9532: GetItem must use item-level_itypes syspref Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|jonathan.druart@biblibre.co | |m | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16419|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19606 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19606&action=edit Bug 9532: reserve rules with itemtype on biblio When itemtype is defined on biblio (item-level_itypes syspref), the method C4::Reserves::CanItemBeReserved uses item->{itemtype}. But item comes from C4::Items::GetItem and it does not have a 'itemtype' key, in this method the item type value is always in 'itype' key. This patch corrects it. Test plan: You must have itemtype on biblio and 'item-level_itypes' syspref on biblio. This test plan is with ReservesControlBranch on ItemHomeLibrary. - Choose a branch, a borrower category and an item type, for example 'NYC', 'CHILD' and 'DVD' - Set an issuing rule for 'NYC', CHILD' and 'DVD' with 'Holds allowed' on 10 - Set an issuing rule for 'NYC', CHILD' and all item types with 'Holds allowed' on 0 - Choose an item of a biblio with itemtype 'DVD', that can be reserved, with 'NYC' as homebranch - Choose a borrower with category 'CHILD' - try to reverse the item for the borrower => without the patch, you can => with the patch, you can't You may check reserve is allowed with 'Holds allowed' > 0 on issuing rule for 'DVD'. Signed-off-by: Liz Rea <liz@catalyst.net.nz> Great test plan - thanks! Confirmed the bug, and the fix. Looks good to me. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18487|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19607 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19607&action=edit Bug 9532: GetItem must use item-level_itypes syspref Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 --- Comment #12 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Those patches are waiting to be pushed into master. Is there a something blocking ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9351 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #13 from Galen Charlton <gmcharlt@gmail.com> --- Upon review, I strongly suspect that the change to GetItem() will damage statistics for stats-only patrons for libraries where item-level_itypes is off. Current behavior for such libraries is: 1. GetItem() retrieves the item. 2. items.itype is null, so it will will grab bibliotitems.itemtype to set $item->{itype} 3. CanBookBeIssued sets $item->{'itemtype'}=$item->{'itype'}; 4. For a stats-only checkout, the call to UpdateStats refers to $item->{'itemtype'}. This contains the bib-level item type, as expected. With the patch series, the behavior becomes: 1. GetItem() retrieves the item. 2. items.itype is null, but since item-level_itypes is off, $item->{itype} will remain undef. 3. CanBookBeIssued sets $item->{'itemtype'}=$item->{'itype'}; 4. For a stats-only checkout, the call to UpdateStats refers to $item->{'itemtype'}. This will be undef. Consequently, there's a regression, and I don't think that the patch you've proposed for 9351 will help. I'm setting to failed QA for this reason. Patches to core routines, *especially* ones that touch holds, items, and circulation, cry out for automated regression tests to accompany the main patch. I will be looking for test cases in any follow-ups you provide. One suggestion I have to improve the API: make GetItem() and routines that fetch item data add a calculated field called effective_item_type or the like. This would contain the biblio-level or item-level item type based on the system preference, but making a separate field with a name like that should make code that references it more readable. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |9351 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #14 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- In fact, the problem of GetItem() is not the main concern of this bug and it seems to be a very complex problem. I've created Bug 11463 to focus on it. It may be set has dependency but in my option it is not. Actually, with item-level_itypes off, the code uses $item->{'itemtype'}. But this key is never defined. That is what the patch corrects. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11463 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19607|0 |1 is obsolete| | --- Comment #15 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Comment on attachment 19607 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19607 Bug 9532: GetItem must use item-level_itypes syspref (moved to Bug 11463) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|9351 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19606|0 |1 is obsolete| | --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 24949 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=24949&action=edit Bug 9532: reserve rules with itemtype on biblio When itemtype is defined on biblio (item-level_itypes syspref), the method C4::Reserves::CanItemBeReserved uses item->{itemtype}. But item comes from C4::Items::GetItem and it does not have a 'itemtype' key, in this method the item type value is always in 'itype' key. This patch corrects it. Test plan: You must have itemtype on biblio and 'item-level_itypes' syspref on biblio. This test plan is with ReservesControlBranch on ItemHomeLibrary. - Choose a branch, a borrower category and an item type, for example 'NYC', 'CHILD' and 'DVD' - Set an issuing rule for 'NYC', CHILD' and 'DVD' with 'Holds allowed' on 10 - Set an issuing rule for 'NYC', CHILD' and all item types with 'Holds allowed' on 0 - Choose an item of a biblio with itemtype 'DVD', that can be reserved, with 'NYC' as homebranch - Choose a borrower with category 'CHILD' - try to reverse the item for the borrower => without the patch, you can => with the patch, you can't You may check reserve is allowed with 'Holds allowed' > 0 on issuing rule for 'DVD'. Signed-off-by: Liz Rea <liz@catalyst.net.nz> Great test plan - thanks! Confirmed the bug, and the fix. Looks good to me. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_3_16_candidate -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #17 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master, along with a regression test. Thanks, Fridolin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #18 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Pushed to 3.14.x, will be in 3.14.08 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 R. Crago <ridingthegravytrain@yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ridingthegravytrain@yahoo.c | |om -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9532 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_3_16_candidate | CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org