Bug 10912 - Streamline hold error messages in staff interface
Summary: Streamline hold error messages in staff interface
Status: RESOLVED DUPLICATE of bug 11250
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-18 20:16 UTC by Kyle M Hall
Modified: 2015-07-02 22:53 UTC (History)
4 users (show)

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


Attachments
Bug 10912 - Streamline hold error messages in staff interface (2.80 KB, patch)
2013-09-18 20:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10912 - Streamline hold error messages in staff interface (2.80 KB, patch)
2014-06-27 14:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10912 - Streamline hold error messages in staff interface (2.88 KB, patch)
2014-06-28 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10912 - Streamline hold error messages in staff interface (2.73 KB, patch)
2014-07-02 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10912 - Streamline hold error messages in staff interface (2.57 KB, patch)
2014-07-02 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-09-18 20:16:38 UTC
Right now, if a patron cannot place a hold on any items on a record, the error in the staff intranet looks like this:

Cannot place hold
Too Many Holds: Kyle Hall has too many holds.
No copies are available to be placed on hold

This can confuse some librarians, because the reason the patron cannot place the hold is not because the patron has too many holds, but only because there are no copies available.
Comment 1 Kyle M Hall 2013-09-18 20:19:41 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-09-25 18:32:36 UTC
I can't reproduce this error unless I manually recreate the old maxreserves system preference and populate it with a small number. Without it I can trigger the "no copies available" but not the "too many" error.

If the patron does have too many holds, and the item really does have no copies available, why shouldn't both error messages display?
Comment 3 Nora Blake 2013-10-02 14:20:06 UTC
Hello,

We were the source of this bug fix and in our estimation is solves the problem have been experiencing.  This patch seems good to us.

Nora Blake
MassCat
Comment 4 Nick Clemens 2014-05-31 16:08:22 UTC
Could not apply patch on sandbox:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 10912:
<h1>Something went wrong !</h1>Applying: Bug 10912 - Streamline hold error messages in staff interface
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 10912 - Streamline hold error messages in staff interface
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 10912 - Streamline hold error messages in staff interface

21215 - Bug 10912 - Streamline hold error messages in staff interface

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-10912---Streamline-hold-error-messages-in-staf-Psre_j.patch .
Comment 5 Kyle M Hall 2014-06-27 14:54:34 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2014-06-28 14:59:56 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2014-06-28 20:33:40 UTC
I am not sure about this patch - if there are multiple reasons why a hold is not possible, should we not display all of them instead of giving one reason preference of another? It seems like this patch will make the error message less accurate (elsif instead of if)?

+        [% ELSIF ( maxreserves ) %]
+          <li><strong>Too many holds: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %] </a> has too many holds.</li>
+        [% ELSIF ( alreadyreserved ) %]
+          <li><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %]</a> <strong>already has a hold</strong> on this item </li>
+        [% ELSIF ( alreadypossession ) %]
Comment 8 Nick Clemens 2014-06-29 12:54:34 UTC
I suspect the error we are seeing is from this check in request.pl:

    unless ( CanBookBeReserved($borrowerinfo->{borrowernumber}, $biblionumber) ) {
        $maxreserves = 1;
    }

It isn't checking against maxreserves or the number of reserves a patron has, just setting the maxreserves error flag when none of the items can be held.
Comment 9 Kyle M Hall 2014-07-02 13:08:04 UTC
The issue is that the extra message is incorrect! However, I think maybe my first patch was too aggressive. I'm going to file a new patch that only affects the "Too many holds" and "No items are available" sections.

(In reply to Katrin Fischer from comment #7)
> I am not sure about this patch - if there are multiple reasons why a hold is
> not possible, should we not display all of them instead of giving one reason
> preference of another? It seems like this patch will make the error message
> less accurate (elsif instead of if)?
> 
> +        [% ELSIF ( maxreserves ) %]
> +          <li><strong>Too many holds: </strong> <a
> href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber
> %]">[% borrowerfirstname %] [% borrowersurname %] </a> has too many
> holds.</li>
> +        [% ELSIF ( alreadyreserved ) %]
> +          <li><a
> href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber
> %]">[% borrowerfirstname %] [% borrowersurname %]</a> <strong>already has a
> hold</strong> on this item </li>
> +        [% ELSIF ( alreadypossession ) %]
Comment 10 Kyle M Hall 2014-07-02 13:08:22 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-07-02 13:09:31 UTC
Created attachment 29436 [details] [review]
Bug 10912 - Streamline hold error messages in staff interface

Right now, if a patron cannot place a hold on any items on a record, the
error in the staff intranet looks like this:

Cannot place hold
Too Many Holds: Kyle Hall has too many holds.
No copies are available to be placed on hold

This can confuse some librarians, because the reason the patron cannot
place the hold is not because the patron has too many holds, but only
because there are no copies available.

Test Plan:
1) Apply this patch
2) Set the number of holds allowed to 1
3) Disable AllowOnShelfHolds
4) Place a hold for a patron
5) Attempt to place a second hold for the patron
6) You should see the "Too many holds" message
7) Set the number of holds allowed to 2
8) Repeat 5, you should be allowed to place the hold, but don't
9) Attempt to place a hold on a record with only checked in items
10) You should get a "No items are available" message
Comment 12 Nick Clemens 2014-07-02 15:24:52 UTC
Hi Kyle,

I want to test this but I cannot produce the original error in master.

If I try to place a hold on an item with no defined rules I can get the "Too many/no available" message, adding a rule that states number of allowed holds gets rid of the 'Too Many'

With this patch I do see only one message, but the 'too many' seems valid if I don't have any rules defined.

Can you let me know if you can reproduce this?

Thanks,
Nick

Tangent: I thought this was related to us seeing "Too many holds" in VOKAL, however we only get that one message even though rules are defined for all categories but playing in master, I cannot get any warning boxes if I turn on 'AllowHoldsOverride' and our system has that on so I don't know if something changed there too.  Been puzzling over recreating the error we see but I am guessing it was already fixed in master
Comment 13 Owen Leonard 2014-08-13 16:16:32 UTC
I too would like more information about how to reproduce the problem which this patch tries to fix. In fact in master I'm unable to trigger a "too many holds" error message by trying to place more holds than are allowed in the rule defined in Circulation and fine rules.

I also don't understand what the "Disable AllowOnShelfHolds" step is for in the test plan.
Comment 14 Nora Blake 2014-08-13 20:23:04 UTC
(In reply to Owen Leonard from comment #13)
> I too would like more information about how to reproduce the problem which
> this patch tries to fix. In fact in master I'm unable to trigger a "too many
> holds" error message by trying to place more holds than are allowed in the
> rule defined in Circulation and fine rules.
> 
> I also don't understand what the "Disable AllowOnShelfHolds" step is for in
> the test plan.

Hi Owen and Nick,

In our shared instance of Koha, we routinely need to block holds on the collections located at our school libraries while they are closed for summer break.  When this happens, we end up with records that have items on them that are listed as available, but are in fact not able to receive hold requests because we have blocked their collections using the hold request controls available through the Circulation and Fines Rules administrative tool.

When a patron or staff member at one of our public or special libraries attempts to place a hold on a record where all the items have items that are blocked from hold requests because of these school closures they get a "Too many holds" message which is not accurate and then they start to wonder why the system is telling them that Patron X has too many holds when they do not.

The second part of the message, "No copies available to placed on hold" is accurate, but they never see that part because they are thrown by the "Too many Holds" part.

I hope this helps explain it.

Nora Blake
MassCat
Comment 15 Owen Leonard 2014-09-30 15:07:06 UTC
I tried to come up with a testing plan which matched Nora's description of their situation: I set up a Circulation and Fine rule for a Itemtype A at Branch A which set the hold limit to zero.

Logged in to Branch A I attempted to place a hold on a title with items of Itemtype A and from Branch A for a patron from Branch A.

In master the message I get is:

Cannot place hold
Too many holds: Fernando De la Cruz has too many holds.
No items are available to be placed on hold

After applying the patch I get this error:

Cannot place hold
Too many holds: Fernando De la Cruz has too many holds.

It looks to me like the wrong one of the two error messages was removed.

Of course I may be testing this incorrectly.

I still don't understand what the "Disable AllowOnShelfHolds" step is for in the test plan.
Comment 16 Nick Clemens 2015-07-02 22:53:06 UTC

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