Bug 9532 - reserve rules with itemtype on biblio
Summary: reserve rules with itemtype on biblio
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-01 16:51 UTC by Fridolin Somers
Modified: 2015-12-03 22:02 UTC (History)
6 users (show)

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


Attachments
Proposed patch (3.57 KB, patch)
2013-02-01 17:15 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9532: reserve rules with itemtype on biblio (3.73 KB, patch)
2013-02-02 02:28 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 9532: reserve rules with itemtype on biblio (3.69 KB, patch)
2013-03-19 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9532: Followup - GetItem must use item-level_itypes syspref (1000 bytes, patch)
2013-05-10 12:05 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9532: GetItem must use item-level_itypes syspref (1.03 KB, patch)
2013-05-30 04:06 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9532: reserve rules with itemtype on biblio (3.74 KB, patch)
2013-07-12 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9532: GetItem must use item-level_itypes syspref (1.09 KB, patch)
2013-07-12 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9532: reserve rules with itemtype on biblio (3.74 KB, patch)
2014-01-31 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2013-02-01 16:51:44 UTC
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.
Comment 1 Fridolin Somers 2013-02-01 17:15:10 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2013-02-02 02:28:07 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-03-11 14:23:47 UTC
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?
Comment 4 Jonathan Druart 2013-03-19 16:12:41 UTC
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.
Comment 5 Jonathan Druart 2013-03-19 16:12:50 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2013-03-20 12:28:08 UTC
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.
Comment 7 Fridolin Somers 2013-05-10 12:05:36 UTC Comment hidden (obsolete)
Comment 8 Fridolin Somers 2013-05-10 12:12:49 UTC
(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.
Comment 9 Chris Cormack 2013-05-30 04:06:14 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-07-12 16:57:53 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-07-12 16:58:05 UTC Comment hidden (obsolete)
Comment 12 Fridolin Somers 2013-10-17 13:05:00 UTC
Those patches are waiting to be pushed into master.
Is there a something blocking ?
Comment 13 Galen Charlton 2013-12-20 06:21:36 UTC
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.
Comment 14 Fridolin Somers 2013-12-31 11:33:27 UTC
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 15 Fridolin Somers 2013-12-31 11:34:37 UTC
Comment on attachment 19607 [details] [review]
Bug 9532: GetItem must use item-level_itypes syspref

(moved to Bug 11463)
Comment 16 Kyle M Hall 2014-01-31 15:14:14 UTC
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>
Comment 17 Galen Charlton 2014-05-05 17:18:29 UTC
Pushed to master, along with a regression test.  Thanks, Fridolin!
Comment 18 Fridolin Somers 2014-06-04 07:39:12 UTC
Pushed to 3.14.x, will be in 3.14.08