Created attachment 5285 [details] Screenshot The error occurs in OPAC even though the following parameters are set: AllowOnShelfHolds is OFF OpacItemHolds is OFF RequestOnOPAC is ON When a title has two items, one is checked out, the other is available, then Koha allows the borrower to place a hold, which is then stored in Koha. This should not be possible, because one of the two items is still available on the shelf.
This bug is still valid in master as of 3.12 beta1.
Created attachment 22847 [details] [review] Patch to resolve AllowOnShelfHolds conflict on available items. The 'AllowOnShelfHolds' seems to work to a certain extent. However, when enabled there was an least one case when Koha would allow a hold to be placed on an item that is on the shelf. This seems to stem from the item-level hold feature from the Staff client. A simple example with one bib and two attached items: if both items are on the shelf then the 'AllowOnShelfHolds' restriction would work, disabling the ability to place a hold. However, if one of these items was "checked out" the system would allow a hold to be placed based on the status of the one "checked-out" item. Some libraries may want to force the disabling of holds, via the OPAC, if ANY item is on the shelf. This patch adds a new SystemPref called 'PreventOPACHoldsAnyAvailableItem'. When this is enabled any item that Koha considers on the shelf (excluding withdrawn, lost etc) will be disabled from placing holds at the bib level. When 'PreventOPACHoldsAnyAvailableItem' is off the the original 'AllowOnShelfHolds' preference will enforce the restrictions, as they were.
I'm confused about the necessity of having a second preference. Why would it not be the case that the 'AllowOnShelfHolds' applies to any available item?
Hi Owen, I think it would be a big change from current behaviour - or do you mean just make it a third option? Maybe just hard to do that in a grammatically nice way :)
There is also argument for a third option - "AllowOnShelfHoldsFromRemoteBranchOnly". That is, allow an item to be reserved when the item is not in the users homebranch. I think the real benefit is the extra choice the library might require. It is a tricky matrix though and this patch is really only the first step to allowing the preference to be a bit more granular. Just an On/Off toggle seems a little limited, I feel.
Hi Barry, we discussed this feature on IRC yesterday: http://irc.koha-community.org/koha/2013-11-14#i_1422109 What we discussed was adding it as a third option to AllowOnShelfHolds - it would help avoid confusion about how 2 preferences work together and avoid another pref. Do you think this would be doable?
And thank you for taking this on - we support libraries that will really like that option!
I can see the confusion I have created! I suppose I should have fixed the AllowOnShelfHolds bug first. By enforcing the option of Allow or Disallow. However, when we got to the core of the problem the feedback from the libraries was that there should be options to allow holds for some items and not others (e.g items that were on the shelf in a remote branch etc) and in that instance an "On" or "Off" wasn't extensive enough. So, what is the general consenus? Should I just rollback and fix up the AllowOnShelfHolds bug first? Then create an enhancement for the other options? It does get a little complex when other branches are considered as there is the added problem of not knowing where the borrower's homebranch is until they login.
Created attachment 23002 [details] [review] Supersedes previous patch. Fixes original problem with out extra SysPref Supersedes previous patch. The previous patch should be ignored. This patch will just fix the original bug. To test: - Apply patch. - Set AllowOnShlefHolds to "Allow" - Search for bib with items and observe "Place Hold" option - Set AllowOnShelfHolds to "Don't Allow" - Repeat Search and observe the "Place Hold" option
Hi Barry, thx a lot for your patch. I have to admit I got a bit confused and I am not sure now what exactly your patch fixes. Could you add a problem description to your commit message? It's always good to state the behaviour before and after the patch, so that the change is clear without testing. If it fixes the problem that you can place holds when there are still available items, we might still have to 'sys pref' it, as this is quite a change in behaviour, behaviour some libraries might rely on. Not trying to scare you away - I'd really like to see this fixed! :)
Comment on attachment 23002 [details] [review] Supersedes previous patch. Fixes original problem with out extra SysPref >From 8ff46c33e6e1b3e4a59ef2956404a6ba6ba30ca2 Mon Sep 17 00:00:00 2001 >From: Barry Cannon <barry@oslo.ie> >Date: Mon, 18 Nov 2013 16:33:45 +0000 >Subject: [PATCH] Bug 6837 - When AllowOnShelfHolds is OFF then holds on > records with available items should not be possible > >The original "AllowOnShelfHolds" SysPref had two options: >"Allow" and "Don't Allow". When the preference was set to "Don't Allow" the system >would still allow a hold to be placed on a bib if at least one of it's items was not >on the shelf. This violated the SystemPreference. > >When this patch is implemented, and the "AllowShelfHolds" SysPref is set to "Don't Allow" >the system will not provide a "Place Hold" option to the OPAC user if ANY item is on >the shelf (in ANY location). Setting the SysPref to "Allow" will allow holds to be placed >on the bib if there is an attached item on the shelf. > >--- > koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc | 4 ---- > koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt | 4 ---- > koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt | 4 ---- > 3 files changed, 12 deletions(-) > >diff --git a/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc b/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc >index 0f16f3a..634fb95 100644 >--- a/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc >+++ b/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc >@@ -4,10 +4,6 @@ > [% IF ( RequestOnOpac ) %] > [% IF ( AllowOnShelfHolds ) %] > <li><a class="reserve" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% biblionumber %]">Place hold</a></li> >- [% ELSE %] >- [% IF ( ItemsIssued ) %] >- <li><a class="reserve" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% biblionumber %]">Place hold</a></li> >- [% END %] > [% END %] > [% END %] > [% END %] >diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt >index 2010cee..83d4a61 100644 >--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt >+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt >@@ -275,10 +275,6 @@ function highlightOn() { > [% IF ( opacuserlogin ) %] > [% IF ( AllowOnShelfHolds ) %] > <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% GROUP_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue--> >- [% ELSE %] >- [% IF ( GROUP_RESULT.itemsissued ) %] >- <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% GROUP_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue--> >- [% END %] > [% END %] > [% END %] > [% END %] >diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt >index 9235b2b..5598ca1 100644 >--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt >+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt >@@ -669,10 +669,6 @@ $(document).ready(function(){ > [% IF ( opacuserlogin ) %] > [% IF ( AllowOnShelfHolds ) %] > <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% SEARCH_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue--> >- [% ELSE %] >- [% IF ( SEARCH_RESULT.itemsissued ) %] >- <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% SEARCH_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue--> >- [% END %] > [% END %] > [% END %] > [% END %] >-- >1.7.10.4 >
Created attachment 23003 [details] [review] Patch to enforce AllowOnShelfHolds System Preference The original "AllowOnShelfHolds" SysPref had two options: "Allow" and "Don't Allow". When the preference was set to "Don't Allow" the system would still allow a hold to be placed on a bib if at least one of it's items was not on the shelf. This violated the SystemPreference. When this patch is implemented, and the "AllowShelfHolds" SysPref is set to "Don't Allow" the system will not provide a "Place Hold" option to the OPAC user if ANY item is on the shelf (in ANY location). Setting the SysPref to "Allow" will allow holds to be placed on the bib if there is an attached item on the shelf.
Best off holding off this patch. I have found an issue with it. I will try and get a resolution up as soon as I get a chance.
Created attachment 23162 [details] [review] Patch to fix Bug 6837 The original "AllowOnShelfHolds" SysPref had two options: "Allow" and "Don't Allow". When the preference was set to "Don't Allow" the system would still allow a hold to be placed on a bib if at least one of it's items was not on the shelf. This violated the SystemPreference. When this patch is implemented, and the "AllowShelfHolds" SysPref is set to "Don't Allow" the system will not provide a "Place Hold" option to the OPAC user if ANY item is on the shelf (in ANY location). Setting the SysPref to "Allow" will allow holds to be placed on the bib if there is an attached item on the shelf.
Hi Barry, I appreciate your work on this a lot, but I think we can't change existing behaviour like this. I think we really need to make your new behaviour a configuration option and keep the existing behaviour intact. It could work as a third option in the pull down of AllowOnShelfHolds, with some rephrasing. There is also another problem - GetReserveStatus doesn't work. See bug 10697. It's much better to use CheckReserves and it would probably also obsolete some of the checks you are doing.
Hi Katrin, Thanks for the feedback, especially regarding GetReserves, I will take another look. Could you expand a bit on what you mean by change in behaviour? This patch should just fix up the initial bug and shouldn't add any new functionality or preferences. Have I inadvertently changed something I shouldn't have!? Barry
The basic problem I see is that this might not qualify as a bug for some libraries, but as intended behaviour. So changing it to work like we want, will break it for others. By preserving existing behaviour we will avoid this conflict.
I see this both ways. The preference reads: Allow/Don't allow hold requests to be placed on items that are not checked out. It specifically says 'items that are not checked out' so it doesn't say 'titles with any items checked out' The preference works as it says it works. I think the preference in this case should be changed to read: ___ hold requests to be placed on ___ not checked out. Allow/Don't allow items that are/titles where all items are That way we retain the old behavior and add the new desired behavior. I'm open to other wording, but do agree with Katrin that to maintain the current behavior we need to offer other options to the existing preference. In the end there are three choices: * The system allows on shelf hold regardless * The system allows on shelf holds if at least one item is checked out * The system does not allow on shelf holds if at least one item is checked out
>* The system allows on shelf hold regardless >* The system allows on shelf holds if at least one item is checked out >* The system does not allow on shelf holds if at least one item is checked out I think maybe we coudl rephrase option 3 to read: * The system does not allow on shelf holds unless all items are checked out
This is a massive change in holds behavior. There is no way this patch can be pushed without an option to retain the systems current behavior. I think it's a great enhancement idea, but I don't believe most users would consider the current behavior a bug, as it has been this way since AllowOnShelfHolds was first implemented. Another QA pont: This patch was submitted yesterday, but uses DBI instead of DBIx::Class ( Koha::Database ). Please modify the patch to use DBIC instead of DBI. You only use DBI in one area, so this request shouldn't be too onerous. I do think this is a great idea. If you can fix these issues I'm sure we can get this pushed!
I think option 3 should read: * The system does not allow on shelf holds. (since if all items are check out, it's not on shelf hold anyway) This option 3 will also relate with the other preference "OPACItemHolds". That is, if option 3 is on and OPACItemHolds is on: - "Place Hold" menu should still be available (even if there are some items on shelf) - If there are some items on shelf, next available option should be disabled. - If all items are checked out, both options show as usual.
I would like to propose to change the preference to read: * Hold requests for on shelf items are: (allowed / allowed if at least one item is checked out / not allowed)
Is this bug invalid since the AllowOnShelfHolds system preferences is now in circulation and fine rules? (Bug 5786)
Yes, the behaviour has not changed with the move - but I think Kyle is working on something that will resolve this on another bug report. I will try to dig it up later.
*** This bug has been marked as a duplicate of bug 15534 ***