Bug 11250 - "too many holds" message appears inappropriately
Summary: "too many holds" message appears inappropriately
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 10912 (view as bug list)
Depends on:
Blocks: 12197
  Show dependency treegraph
 
Reported: 2013-11-14 15:22 UTC by Koha Team Lyon 3
Modified: 2016-06-21 21:39 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11250 - "too many holds" message appears inappropriately (3.08 KB, patch)
2015-07-02 10:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11250 - "too many holds" message appears inappropriately (3.13 KB, patch)
2015-09-17 12:48 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 11250 - "too many holds" message appears inappropriately (3.20 KB, patch)
2015-10-22 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Koha Team Lyon 3 2013-11-14 15:22:35 UTC
In the staff interface, when placing a hold from the holds tab of a detailed record, the message "Cannot place hold
Too Many Holds: Johnny SuchOne has too many holds" appears even if the unique reason is that there 's no holds allowed for the borrower.

seems because of this line (request.pl line 193) :
    unless ( $canbookbereserved ) {
        $maxreserves = 1;
    }
Comment 1 Aaron Sakovich 2014-03-11 19:47:08 UTC
Additionally, placing a hold on an item that is not allowed to be held (Holds allowed count = 0), will result in the "{username} has too many holds" instead of a more meaningful or pertinent "{item} is not allowed to be placed on hold".
Comment 2 Kyle M Hall 2015-07-02 10:09:37 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2015-07-02 22:53:06 UTC
*** Bug 10912 has been marked as a duplicate of this bug. ***
Comment 4 M. Tompsett 2015-07-03 00:49:32 UTC
Comment on attachment 40759 [details] [review]
Bug 11250 - "too many holds" message appears inappropriately

Review of attachment 40759 [details] [review]:
-----------------------------------------------------------------

Though I like the shift to the IF/ELSIF structure, I'm not sure it addresses the first two comments.

::: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ +309,5 @@
> +          <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 ( ageRestricted ) %]
> +          <li><strong>Age restricted</strong></li>
> +        [% ELSIF ( none_available ) %]
> +          <li> <strong>No items are available</strong> to be placed on hold</li>

There is a difference between none_available (but eventually some may) vs. comment #2's "{item} is not allowed to be placed on hold". Is there a way to check for this?

@@ +311,5 @@
> +          <li><strong>Age restricted</strong></li>
> +        [% ELSIF ( none_available ) %]
> +          <li> <strong>No items are available</strong> to be placed on hold</li>
> +        [% 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>

Similarly there is a minor difference between a borrower who has too many holds (non-zero), and one who isn't allowed to place any at all, as per my understanding of comment #1.
Comment 5 M. Tompsett 2015-07-03 00:50:41 UTC
Sorry for the off-by-1 error in my comment. :(
Comment 6 j.kylmala 2015-08-26 08:21:02 UTC
(In reply to M. Tompsett from comment #4)
> There is a difference between none_available (but eventually some may) vs.
> comment #2's "{item} is not allowed to be placed on hold". Is there a way to
> check for this?

What do you mean with this paragraph? I didn't understand it.
Comment 7 M. Tompsett 2015-08-26 12:18:00 UTC
(In reply to Joonas Kylmälä from comment #6)
> (In reply to M. Tompsett from comment #4)
> > There is a difference between none_available (but eventually some may) vs.
> > comment #2's "{item} is not allowed to be placed on hold". Is there a way to
> > check for this?
> 
> What do you mean with this paragraph? I didn't understand it.

"Too many holds" -- But I don't have any holds on the book at all! In this case, there is some sort of holds queue limitation.
"None available" -- Because there is a holds limitation preventing a hold from being placed.

"Too many holds" -- Right, because some items just can't be put on hold period. This is a specific case of the previous one.
"None available" -- And there never will be, because it can't be put on hold.

"Too many holds" -- Oh, but I'm insane and taking a quadruple major and need all 20 books! In this case, the patron is holds-happy putting everything they can on hold.
"None available" -- Because you are clearly an holds-happy patron!
Comment 8 j.kylmala 2015-08-26 13:06:02 UTC
M. Tompsett:

Still don't get what do you mean. What do you mean by "but eventually some may". And what is the thing you're trying to say with that there is a difference between none_available and "{item} is not allowed to be placed on hold"?

Or if you could rephrase the thing what you suggested to be done here:

::: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ +309,5 @@
> +          <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 ( ageRestricted ) %]
> +          <li><strong>Age restricted</strong></li>
> +        [% ELSIF ( none_available ) %]
> +          <li> <strong>No items are available</strong> to be placed on hold</li>
Comment 9 Nicole C. Engard 2015-09-17 12:48:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-09-17 14:37:01 UTC
I don't understand anything from comments 4 to 8, could you please clarify?
Comment 11 Katrin Fischer 2015-09-30 22:35:36 UTC
Same here about 4-8. New sequence looks good to me, but is there a reason we don't display all reasons why a hold can not be placed?
Comment 12 Kyle M Hall 2015-10-09 12:21:53 UTC
> There is a difference between none_available (but eventually some may) vs.
> comment #2's "{item} is not allowed to be placed on hold". Is there a way to
> check for this?

If I understand you correctly, the answer is no at this time. That would be outside the scope of this bug.


> Similarly there is a minor difference between a borrower who has too many
> holds (non-zero), and one who isn't allowed to place any at all, as per my
> understanding of comment #1.

That is correct, and this patch deals with that situation. The patron will get different messages if they exceed the maxreserves vs they can't place a hold on any of the items.
Comment 13 Jonathan Druart 2015-10-15 08:42:29 UTC
(In reply to Katrin Fischer from comment #11)
> Same here about 4-8. New sequence looks good to me, but is there a reason we
> don't display all reasons why a hold can not be placed?

Kyle,
I am not sure this patch addresses correctly the problem. I think the logic should be reworked in the script.
But yes I know it will certainly be more difficult that just switching some conditional blocks in the template, so maybe it is worth to go first with this patch.
Could you please answer Katrin?
Comment 14 Kyle M Hall 2015-10-19 13:44:22 UTC
(In reply to Katrin Fischer from comment #11)
> Same here about 4-8. New sequence looks good to me, but is there a reason we
> don't display all reasons why a hold can not be placed?

Basically, it causes confusion for librarians. For example, iirc, a patron may not be able to place holds on any of the items, and thus the librarian receives the "too many holds" error which is not actually true.
Comment 15 Katrin Fischer 2015-10-20 22:45:25 UTC
(In reply to Kyle M Hall from comment #14)
> (In reply to Katrin Fischer from comment #11)
> > Same here about 4-8. New sequence looks good to me, but is there a reason we
> > don't display all reasons why a hold can not be placed?
> 
> Basically, it causes confusion for librarians. For example, iirc, a patron
> may not be able to place holds on any of the items, and thus the librarian
> receives the "too many holds" error which is not actually true.

I agree that the 'too many' is often confusing, just wondering if we could be more precise as several of the reasons might apply at the same time - too many holds, restricted, too many fines...
Comment 16 Katrin Fischer 2015-10-20 22:55:56 UTC
Huh, I realize now that we do currently display all reasons. 

I think this patch might just hide the real problem - that the "too_many" is not accurate enough to be helpful. With the patch it will show up less, but it will still show up sometimes if none of the other reasons applies.

Maybe we could just rephrase?

Too many holds: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %] </a> has too many holds.

<Patron> can't place holds on these item types or already has too many holds placed.

Something like that? Can we pin down, when exactly the 'too many' is displayed and if that would cover it?

Ultimately I think we need to make the feedback more meaningful, but would like the option to see if multiple things block placing holds.
Comment 17 Kyle M Hall 2015-10-21 10:40:25 UTC
(In reply to Katrin Fischer from comment #16)
> Huh, I realize now that we do currently display all reasons. 
> 
> I think this patch might just hide the real problem - that the "too_many" is
> not accurate enough to be helpful. With the patch it will show up less, but
> it will still show up sometimes if none of the other reasons applies.
> 
> Maybe we could just rephrase?
> 
> Too many holds: </strong> <a
> href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber
> %]">[% borrowerfirstname %] [% borrowersurname %] </a> has too many holds.
> 
> <Patron> can't place holds on these item types or already has too many holds
> placed.
> 
> Something like that? Can we pin down, when exactly the 'too many' is
> displayed and if that would cover it?
> 
> Ultimately I think we need to make the feedback more meaningful, but would
> like the option to see if multiple things block placing holds.

Rephrasing does nothing to fix the issue. The problem isn't that the too_many message is wrong, it's that it is triggered by almost all situations where a patron cannot place holds. That makes it not useful. This patch changes that so that it displays only if no other limitations exist, which means the patron really has simply placed too many holds.
Comment 18 Katrin Fischer 2015-10-21 11:08:15 UTC
But it can also hide the information that you want to see - like no item available and age restriction could happen at the same time - that's what bugs me a bit about this approach.
Comment 19 Jonathan Druart 2015-10-22 11:00:32 UTC
Created attachment 43748 [details] [review]
Bug 11250 - "too many holds" message appears inappropriately

This patch gives the various reasons precedence in this order:
1) Already in possesion
2) Already on hold
3) Age restricted
4) No items available
5) Too many holds

Test Plan:
1) Attempt to place a hold where both "too many holds" and "no items available" messages display
2) Apply this patch
3) Refresh
4) Note only the "no items available" message displays

Signed-off-by: Heather Braum <hbraum@nekls.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Tomás Cohen Arazi 2015-10-22 13:48:07 UTC
Patch pushed to master.

Thanks Kyle!