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.
Created attachment 15016 [details] [review] Proposed patch See commit message
Created attachment 15018 [details] [review] 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.
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?
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.
Created attachment 16419 [details] [review] 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.
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.
Created attachment 18061 [details] [review] Bug 9532: Followup - GetItem must use item-level_itypes syspref
(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.
Created attachment 18487 [details] [review] Bug 9532: GetItem must use item-level_itypes syspref Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 19606 [details] [review] 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>
Created attachment 19607 [details] [review] 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>
Those patches are waiting to be pushed into master. Is there a something blocking ?
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.
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.
Comment on attachment 19607 [details] [review] Bug 9532: GetItem must use item-level_itypes syspref (moved to Bug 11463)
Created attachment 24949 [details] [review] 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>
Pushed to master, along with a regression test. Thanks, Fridolin!
Pushed to 3.14.x, will be in 3.14.08