Bug 12632 - Hold limits ignored for record level holds with item level itemtypes
Summary: Hold limits ignored for record level holds with item level itemtypes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 9809
Blocks: 13924
  Show dependency treegraph
 
Reported: 2014-07-23 12:07 UTC by Kyle M Hall
Modified: 2017-05-17 07:26 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.60 KB, patch)
2014-08-22 17:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.75 KB, patch)
2014-08-22 17:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.88 KB, patch)
2014-10-27 13:07 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12632 - Unit Tests (3.11 KB, patch)
2015-01-14 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.80 KB, patch)
2015-01-14 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Unit Tests (3.12 KB, patch)
2015-01-14 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.79 KB, patch)
2015-08-21 19:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Unit Tests (3.12 KB, patch)
2015-08-21 19:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Unit Tests (3.12 KB, patch)
2015-08-24 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes (2.86 KB, patch)
2015-08-25 15:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12632 - Unit Tests [For Backporting] (3.19 KB, patch)
2015-08-25 15:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12632 - Unit Tests [For Master] (3.19 KB, patch)
2015-08-25 18:53 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 Kyle M Hall 2014-07-23 12:07:07 UTC
The crux of the issue is that if you are using item level itemtypes, but are allowing biblio levels holds, those holds do not have items.

So, in CanItemBeReserved, when Koha counts the number of holds to compare against the given rule, it will always give 0 ( except of course for found holds, and the occasional item-level hold ).

So the query is saying "link each of these reserves to the reserved item, and count the number of reserves this patron where the itemtype is DVD". However, since these are all record level reserves, there are no items to link to, and so when it looks for all reserves this and item whose itemtype is DVD, it finds zero reserves!

I can see a few resolutions to this:
1) Add a system preference ReservesControlTypeLevel to allow a library to select between item and record level itemtypes for hold rules.
2) Use COALESCE(itype,itemtype) to more gracefully select record level itemtypes if the reserve is a record level hold
3) Add a conditional if to our query such that if the reserve has no itemtype, we should join on reserves.biblionumber=items.biblionumber instead of reserves.itemnumber=items.itemnumber, with a 'GROUP BY itemnumber' tacked on.
4) The same as 3, but instead of a conditional, join to items twice, once by itemnumber items_i, once by biblionumber items_b. Group by itemnumber, use COALESCE( items_i.itemtype, items_b.itemtype )
Comment 1 Kyle M Hall 2014-07-23 12:28:18 UTC
My preferences for the solutions from best to worst: 4,2,3,1
Comment 2 Kyle M Hall 2014-08-22 17:36:52 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-08-22 17:45:55 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2014-09-03 03:13:28 UTC
I tried testing but still seem to be able to place multiple record level holds.  If I place an item level hold I can no longer place any more holds.


Test steps:
 - set item-level_itypes to "specific item" : AllowHoldPolicyOverride to 'Don't Allow' : AllowOnShelfHolds to 'Allow'
 - created two records (of type music) each with one item (of type music)
 - added a circulation rule for all patrons of all libraries for item type music to allow 1 hold
 - place a 'next available' hold on the first record for patron
 - attempt to place a hold on second record, no warning
 - apply patch
 - attempt to place a hold on second record, no warning
 - placed hold with no error
 - deleted both holds
 - place an item level hold on first record
 - attempt to place a hold on second record, "too many holds" warning and 'no items available' warning and cannot place record or item specific hold.

Anything I am missing?
Comment 5 Katrin Fischer 2014-09-14 20:16:15 UTC
I have been thinking about this change and I think there are some use cases where this approach will or might create problems.

Right now, we use items for circulation conditions (with item-level_itypes at least) and the record is separate from those. With this patch, that will change.

There is currently no option to preserve a single field when overwriting a record using one of our import scripts or tool or the Z39.50 search. So one has to be very careful to not overwrite 942$c accidentally. It is mandatory by default if you edit it manually, but the mandatory fields are not enforced, when using scripts or staged import.

In fact, this is the reason the feature would not be usable for most of our libraries, as the databases are kept in sync with the union catalog and 942$c would be overwritten on a regular basis when importing the newer record from the union catalog. :(

That's why I'd prefer 4) in 'comment 0', but it reads like the patch implements 1)?

What happens if there is no itemtype on record level?
How will this work for consortia were libraries might be using different sets of itemtypes?
Comment 6 Kyle M Hall 2014-09-19 18:07:55 UTC
(In reply to Katrin Fischer from comment #5)
> I have been thinking about this change and I think there are some use cases
> where this approach will or might create problems.
> 
> Right now, we use items for circulation conditions (with item-level_itypes
> at least) and the record is separate from those. With this patch, that will
> change.

I'm not sure what you mean by this at all.
 
> There is currently no option to preserve a single field when overwriting a
> record using one of our import scripts or tool or the Z39.50 search. So one
> has to be very careful to not overwrite 942$c accidentally. It is mandatory
> by default if you edit it manually, but the mandatory fields are not
> enforced, when using scripts or staged import.

That's really an issue completely outside the scope of this bug report.

> In fact, this is the reason the feature would not be usable for most of our
> libraries, as the databases are kept in sync with the union catalog and
> 942$c would be overwritten on a regular basis when importing the newer
> record from the union catalog. :(

While that's a bummer, again, it's really outside the scope of this.

> That's why I'd prefer 4) in 'comment 0', but it reads like the patch
> implements 1)?

Based on your comments, *none* of the solutions will work for you as your record itemtype will be removed each time you sync your records.

However, this implementation is actually number 2. I tried 3 and 4 but found the solutions to be unworkable ideas.

> 
> What happens if there is no itemtype on record level?
> How will this work for consortia were libraries might be using different
> sets of itemtypes?

If there is no itemtype on the record, Koha will behave exactly as it has been. You won't see any improvement or detriment based on this patch.

Consortia would need to agree on the record itemtype, or have a separate record for each libraries items, or just have no record level itemtype and keep the existing behavior, or allow only item-level holds.
Comment 7 Owen Leonard 2014-09-30 14:09:06 UTC
(In reply to Kyle M Hall from comment #3)

> Test plan:
> 1) Enable item level itemtypes
> 2) Create two records with one item each of a given itemtype
> 3) Create a single issuing rule and limit the holds allowed for that
>    itemtype to 1
> 4) Place a record level hold on your first record
> 5) Attempt to place a record level hold for the same patron on your
>    second record. You should not be able to but you can!
> 6) Apply this patch
> 7) Repeat step 5, note you can no longer place the hold!

This isn't working for me. I've created a circulation rule which limits the itemtype to one hold. I've confirmed that the two records I'm testing with have no biblio-level item type. When I try to place each title on hold for the same patron I'm not prevented from placing the second.
Comment 8 Kyle M Hall 2014-10-20 12:38:42 UTC
(In reply to Owen Leonard from comment #7)
> (In reply to Kyle M Hall from comment #3)
> 
> > Test plan:
> > 1) Enable item level itemtypes
> > 2) Create two records with one item each of a given itemtype
> > 3) Create a single issuing rule and limit the holds allowed for that
> >    itemtype to 1
> > 4) Place a record level hold on your first record
> > 5) Attempt to place a record level hold for the same patron on your
> >    second record. You should not be able to but you can!
> > 6) Apply this patch
> > 7) Repeat step 5, note you can no longer place the hold!
> 
> This isn't working for me. I've created a circulation rule which limits the
> itemtype to one hold. I've confirmed that the two records I'm testing with
> have no biblio-level item type. When I try to place each title on hold for
> the same patron I'm not prevented from placing the second.

That makes perfect sense. I should have stated explicitly in the test plan to set the record-level itemtype to be the same as your item-level itemtypes.

2a) Ensure your record and item level itemtypes are set and match.

Kyle
Comment 9 Paola Rossi 2014-10-27 13:06:53 UTC
I've applied the patch against master 3.17.00.031

I set item-level_itypes to "specific item" [default] and AllowOnShelfHolds to 'Allow' [not default].
I set "Holds allowed (count)" at 1 for music type in the circulation rule cloned to my test library.
When I created the biblio records, I chose "music" as their "Koha item type".

Everything is OK as required by the test plan.

So I pass the patch to "Signed Off" status.

NB. 

If I didn't follow the test plan, but I :
1=) Enable item level itemtypes
2=) Create two records with one item each of a given itemtype
3=) Create a single issuing rule and limit the holds allowed for that
   itemtype (music) to 1
4) Apply this patch
5) Place no hold on the 2 music record
6) Select a patron X without holds, and select his "Search to hold" option
7) Search and find both the 2 records, and "place hold for X" on both them,

then Koha places both the 2 holds.

If the created records were 3, koha placed the 3 holds for the patron X.
And then, if I selected "Search to hold" on the same records for another patron Y without holds, Koha placed further holds on the records already hold by X.
Comment 10 Paola Rossi 2014-10-27 13:07:35 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-11-10 15:10:44 UTC
Kyle, please provide tests for this change.
Comment 12 Kyle M Hall 2015-01-14 16:43:28 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-01-14 16:43:40 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-01-14 16:45:31 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-01-29 14:15:20 UTC
First I set max holds to 5, and I place 4 biblio level holds in opac. OK
Then I select 2 biblios and try again to place a hold in opac on both (multiplacehold): the first hold is placed, the second is not.
There is no warning. If you would not check the list, you would think that both holds were placed.

Before these patches, these six biblio level holds would have been approved. Although you could consider this as outside the scope of this report, this report would be the cause of this step back.

Would you see opportunity for a small fix of this case?
Parking in FQA for now.
Comment 16 Kyle M Hall 2015-04-07 13:56:29 UTC
(In reply to M. de Rooy from comment #15)
> First I set max holds to 5, and I place 4 biblio level holds in opac. OK
> Then I select 2 biblios and try again to place a hold in opac on both
> (multiplacehold): the first hold is placed, the second is not.
> There is no warning. If you would not check the list, you would think that
> both holds were placed.
> 
> Before these patches, these six biblio level holds would have been approved.
> Although you could consider this as outside the scope of this report, this
> report would be the cause of this step back.
> 
> Would you see opportunity for a small fix of this case?
> Parking in FQA for now.

Yes, I would consider this to be outside the scope of this bug report. This patch makes no changes to the possible return values of CanItemBeReserved so that issue must be separate and distinct from this one. If you file a bug report for this issue and make it depend on this bug report, I'd be more than happy to take a crack a fixing it!
Comment 17 Katrin Fischer 2015-05-04 21:01:34 UTC
Marcel, can you check Kyle's last comment?
Comment 18 Marcel de Rooy 2015-05-06 11:21:15 UTC
(In reply to Katrin Fischer from comment #17)
> Marcel, can you check Kyle's last comment?

I still have some doubts if we should consider this to be outside the scope of this report. Maybe another QAer can have a look too. Thanks.
Comment 19 Kyle M Hall 2015-07-23 19:13:01 UTC
(In reply to Marcel de Rooy from comment #18)
> (In reply to Katrin Fischer from comment #17)
> > Marcel, can you check Kyle's last comment?
> 
> I still have some doubts if we should consider this to be outside the scope
> of this report. Maybe another QAer can have a look too. Thanks.

I think the general rule is when in doubt we should keep things split up. Scope creep can keep bugs from getting resolved.
Comment 20 Jonathan Druart 2015-08-19 11:45:12 UTC
One test does not pass for me:

#   Failed test 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed'
#   at t/db_dependent/Holds.t line 449.
#          got: 'OK'
#     expected: 'tooManyReserves'
Comment 21 Jonathan Druart 2015-08-21 10:38:04 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 22 Jonathan Druart 2015-08-21 10:41:02 UTC
I meant bug 9809.
Comment 23 Kyle M Hall 2015-08-21 19:04:05 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-08-21 19:04:13 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-08-24 13:29:17 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-08-24 13:31:05 UTC
(In reply to Jonathan Druart from comment #20)
> One test does not pass for me:
> 
> #   Failed test 'Patron cannot reserve item with hold limit of 1, 1 bib
> level hold placed'
> #   at t/db_dependent/Holds.t line 449.
> #          got: 'OK'
> #     expected: 'tooManyReserves'

Fixed! On master the issuing rule I created for the test was getting deleted before the new tests were run!
Comment 27 Jonathan Druart 2015-08-25 13:46:21 UTC
The tests pass even without the first patch.

Please provide a patch on top of bug 9809.

Marked as Failed QA.
Comment 28 Jonathan Druart 2015-08-25 13:46:49 UTC
(In reply to Jonathan Druart from comment #27)
> Please provide a patch on top of bug 9809.

Erk, maybe 1 on top for master, and another one for the stable branches.
Comment 29 Kyle M Hall 2015-08-25 14:52:59 UTC
(In reply to Jonathan Druart from comment #27)
> The tests pass even without the first patch.
> 
> Please provide a patch on top of bug 9809.
> 
> Marked as Failed QA.

The test 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' fails on master with the default sample data. It passes with the first patch applied. The other new unit tests are just testing existing functionality, so they *should* pass on master.

If you are finding that the test 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' passes for you without the main patch, I would suggest testing it on a fresh database with the sample data.
Comment 30 Kyle M Hall 2015-08-25 14:54:28 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Jonathan Druart from comment #27)
> > Please provide a patch on top of bug 9809.
> 
> Erk, maybe 1 on top for master, and another one for the stable branches.

Once 9809 is pushed to master, and this has passed qa, I will submit separate unit tests to update this patch set for master and retain the current unit tests patch for backporting.
Comment 31 Jonathan Druart 2015-08-25 15:38:24 UTC
Created attachment 41934 [details] [review]
Bug 12632 - Hold limits ignored for record level holds with item level itemtypes

The crux of the issue is that if you are using item level itemtypes, but
are allowing biblio levels holds, those holds do not have items.

So, in CanItemBeReserved, when Koha counts the number of holds to
compare against the given rule, it will always give 0 ( except of course
for found holds, and the occasional item-level hold ).

So the query is saying "link each of these reserves to the reserved
item, and count the number of reserves this patron where the itemtype is
DVD". However, since these are all record level reserves, there are no
items to link to, and so when it looks for all reserves this and item
whose itemtype is DVD, it finds zero reserves!

This patch solves the problem by looking first at the item level
itemtype, and if it does not exist, then it looks at the record
level itemtype. For installations using record level itemtypes, the
behavior remains unchanged.

Test plan:
1) Enable item level itemtypes
2) Create two records with one item each of a given itemtype
3) Create a single issuing rule and limit the holds allowed for that
   itemtype to 1
4) Place a record level hold on your first record
5) Attempt to place a record level hold for the same patron on your
   second record. You should not be able to but you can!
6) Apply this patch
7) Repeat step 5, note you can no longer place the hold!

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2015-08-25 15:38:32 UTC
Created attachment 41935 [details] [review]
Bug 12632 - Unit Tests [For Backporting]

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2015-08-25 15:39:02 UTC
RM: Do not push this patch after bug 9809 as it!
Comment 34 Kyle M Hall 2015-08-25 16:37:56 UTC
(In reply to Jonathan Druart from comment #33)
> RM: Do not push this patch after bug 9809 as it!

Jonathan, I've been looking at the tests and code, and I don't think this bug conflicts with 9809. With this on top of 9809 the tests still pass and fail correctly just like they do without 9809. The unit tests don't create any new holds, so they are unaffected by the changes made my 9809.

Can you confirm this?
Comment 35 Jonathan Druart 2015-08-25 17:12:45 UTC
(In reply to Kyle M Hall from comment #34)
> (In reply to Jonathan Druart from comment #33)
> > RM: Do not push this patch after bug 9809 as it!
> 
> Jonathan, I've been looking at the tests and code, and I don't think this
> bug conflicts with 9809. With this on top of 9809 the tests still pass and
> fail correctly just like they do without 9809. The unit tests don't create
> any new holds, so they are unaffected by the changes made my 9809.
> 
> Can you confirm this?

Yes it is :)

+my $res_id = AddReserve( $branch, $borrowernumbers[0], $bibnum, 'a', '', 1, );
Comment 36 Kyle M Hall 2015-08-25 18:46:49 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Kyle M Hall from comment #34)
> > (In reply to Jonathan Druart from comment #33)
> > > RM: Do not push this patch after bug 9809 as it!
> > 
> > Jonathan, I've been looking at the tests and code, and I don't think this
> > bug conflicts with 9809. With this on top of 9809 the tests still pass and
> > fail correctly just like they do without 9809. The unit tests don't create
> > any new holds, so they are unaffected by the changes made my 9809.
> > 
> > Can you confirm this?
> 
> Yes it is :)
> 
> +my $res_id = AddReserve( $branch, $borrowernumbers[0], $bibnum, 'a', '', 1,
> );

Thanks Jonathan! I totally missed that. As I'm typing this I realized it only worked for me because I didn't run updatedatabase.
Comment 37 Kyle M Hall 2015-08-25 18:53:15 UTC
Created attachment 41950 [details] [review]
Bug 12632 - Unit Tests [For Master]

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Tomás Cohen Arazi 2015-08-26 14:11:03 UTC
Patches pushed to master.

Good job Kyle!
Comment 39 Liz Rea 2015-09-21 04:40:25 UTC
Pushed to 3.18.x will be in 3.18.10.
Comment 40 Liz Rea 2015-09-21 04:47:19 UTC
3.18.11, I mean. *sigh*
Comment 41 Chris Cormack 2015-09-22 01:05:49 UTC
Pushed to 3.20.x will be in 3.20.4