[Bug 12632] New: Hold limits ignored for record level holds with item level itemtypes
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Bug ID: 12632 Summary: Hold limits ignored for record level holds with item level itemtypes Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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 ) -- 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=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- 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=12632 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- My preferences for the solutions from best to worst: 4,2,3,1 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nengard@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 31106 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31106&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31106|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 31107 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31107&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@quecheelibrary.org --- Comment #4 from Nick Clemens <nick@quecheelibrary.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |paola.rossi@cineca.it --- Comment #9 from Paola Rossi <paola.rossi@cineca.it> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31107|0 |1 is obsolete| | --- Comment #10 from Paola Rossi <paola.rossi@cineca.it> --- Created attachment 32788 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32788&action=edit Bug 12632 - Hold limits ignored for record level holds with item level itemtypes -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- Kyle, please provide tests for this change. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32788|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35255 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35255&action=edit Bug 12632 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35256 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35256&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35255|0 |1 is obsolete| | --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35257 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35257&action=edit Bug 12632 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Marcel, can you check Kyle's last comment? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=8238 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13924 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #19 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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' -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14526 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Be care, AddReserve prototype changed by bug 14526 (contrainst param removed). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. |http://bugs.koha-community. |org/bugzilla3/show_bug.cgi? |org/bugzilla3/show_bug.cgi? |id=14526 |id=9809 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I meant bug 9809. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35256|0 |1 is obsolete| | Attachment #35257|0 |1 is obsolete| | --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41798 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41798&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41799 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41799&action=edit Bug 12632 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41799|0 |1 is obsolete| | --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41858 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41858&action=edit Bug 12632 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle.m.hall@gmail.com --- Comment #26 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- The tests pass even without the first patch. Please provide a patch on top of bug 9809. Marked as Failed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #29 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #30 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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=12632 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41798|0 |1 is obsolete| | Attachment #41858|0 |1 is obsolete| | --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41934 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41934&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41935 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41935&action=edit Bug 12632 - Unit Tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- RM: Do not push this patch after bug 9809 as it! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |9809 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|9809 | --- Comment #34 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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? Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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, ); -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #36 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |9809 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41935|Bug 12632 - Unit Tests |Bug 12632 - Unit Tests [For description| |Backporting] -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #37 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41950 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41950&action=edit Bug 12632 - Unit Tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41950|Bug 12632 - Unit Tests |Bug 12632 - Unit Tests [For description| |Master] -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #38 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Good job Kyle! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #39 from Liz Rea <liz@catalyst.net.nz> --- Pushed to 3.18.x will be in 3.18.10. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 --- Comment #40 from Liz Rea <liz@catalyst.net.nz> --- 3.18.11, I mean. *sigh* -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #41 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.20.x will be in 3.20.4 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org