Bug 6837 - When AllowOnShelfHolds is OFF then holds on records with available items should not be possible
Summary: When AllowOnShelfHolds is OFF then holds on records with available items shou...
Status: RESOLVED DUPLICATE of bug 15534
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Barry Cannon
QA Contact: Bugs List
URL: /cgi-bin/koha/opac-detail.pl?biblionu...
Keywords:
Depends on: 2339
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 11:36 UTC by Axel Jacquin
Modified: 2016-01-19 14:28 UTC (History)
8 users (show)

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


Attachments
Screenshot (36.71 KB, image/png)
2011-09-02 11:36 UTC, Axel Jacquin
Details
Patch to resolve AllowOnShelfHolds conflict on available items. (11.79 KB, patch)
2013-11-08 19:05 UTC, Barry Cannon
Details | Diff | Splinter Review
Supersedes previous patch. Fixes original problem with out extra SysPref (3.45 KB, patch)
2013-11-18 16:44 UTC, Barry Cannon
Details | Diff | Splinter Review
Patch to enforce AllowOnShelfHolds System Preference (3.92 KB, patch)
2013-11-18 17:27 UTC, Barry Cannon
Details | Diff | Splinter Review
Patch to fix Bug 6837 (12.37 KB, patch)
2013-11-26 16:23 UTC, Barry Cannon
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Axel Jacquin 2011-09-02 11:36:39 UTC
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.
Comment 1 Owen Leonard 2013-03-26 16:35:52 UTC
This bug is still valid in master as of 3.12 beta1.
Comment 2 Barry Cannon 2013-11-08 19:05:53 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2013-11-14 18:52:08 UTC
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?
Comment 4 Katrin Fischer 2013-11-14 19:03:27 UTC
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 :)
Comment 5 Barry Cannon 2013-11-14 20:19:44 UTC
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.
Comment 6 Katrin Fischer 2013-11-15 14:28:37 UTC
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?
Comment 7 Katrin Fischer 2013-11-15 14:29:13 UTC
And thank you for taking this on - we support libraries that will really like that option!
Comment 8 Barry Cannon 2013-11-15 15:30:09 UTC
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.
Comment 9 Barry Cannon 2013-11-18 16:44:46 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-11-18 17:02:11 UTC
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 11 Barry Cannon 2013-11-18 17:19:17 UTC
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
>
Comment 12 Barry Cannon 2013-11-18 17:27:11 UTC Comment hidden (obsolete)
Comment 13 Barry Cannon 2013-11-18 20:40:19 UTC
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.
Comment 14 Barry Cannon 2013-11-26 16:23:13 UTC
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.
Comment 15 Katrin Fischer 2013-11-27 08:28:03 UTC
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.
Comment 16 Barry Cannon 2013-11-27 13:32:55 UTC
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
Comment 17 Katrin Fischer 2013-11-27 13:44:02 UTC
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.
Comment 18 Nicole C. Engard 2013-11-27 14:01:11 UTC
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
Comment 19 Katrin Fischer 2013-11-27 14:10:09 UTC
>* 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
Comment 20 Kyle M Hall 2013-11-27 14:10:54 UTC
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!
Comment 21 Pongtawat 2014-09-15 03:51:51 UTC
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.
Comment 22 Pongtawat 2014-09-15 04:29:23 UTC
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)
Comment 23 Owen Leonard 2016-01-15 14:23:18 UTC
Is this bug invalid since the AllowOnShelfHolds system preferences is now in circulation and fine rules? (Bug 5786)
Comment 24 Katrin Fischer 2016-01-15 14:31:00 UTC
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.
Comment 25 Kyle M Hall 2016-01-19 14:28:57 UTC

*** This bug has been marked as a duplicate of bug 15534 ***