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 )
My preferences for the solutions from best to worst: 4,2,3,1
Created attachment 31106 [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. Since Koha installations using record-level itemtypes will not have item-level itemtypes, it will fall back to record-level itemtypes gracefully. 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!
Created attachment 31107 [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!
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?
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?
(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.
(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.
(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
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.
Created attachment 32788 [details] [review] Bug 12632 - Hold limits ignored for record level holds with item level itemtypes
Kyle, please provide tests for this change.
Created attachment 35255 [details] [review] Bug 12632 - Unit Tests
Created attachment 35256 [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>
Created attachment 35257 [details] [review] Bug 12632 - Unit Tests
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.
(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!
Marcel, can you check Kyle's last comment?
(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.
(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.
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'
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
I meant bug 9809.
Created attachment 41798 [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>
Created attachment 41799 [details] [review] Bug 12632 - Unit Tests
Created attachment 41858 [details] [review] Bug 12632 - Unit Tests
(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!
The tests pass even without the first patch. Please provide a patch on top of bug 9809. Marked as Failed QA.
(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.
(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.
(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.
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>
Created attachment 41935 [details] [review] Bug 12632 - Unit Tests [For Backporting] Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
RM: Do not push this patch after bug 9809 as it!
(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?
(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, );
(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.
Created attachment 41950 [details] [review] Bug 12632 - Unit Tests [For Master] Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Patches pushed to master. Good job Kyle!
Pushed to 3.18.x will be in 3.18.10.
3.18.11, I mean. *sigh*
Pushed to 3.20.x will be in 3.20.4