Bug 16787

Summary: 'Too many holds' message appears inappropriately and is missing data
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, axelle.clarisse, bdaeuber, benjamin.r.philbrook, david.bourgault, fridolin.somers, george, gmcharlt, hayleypelham, hughr, jonathan.druart, koha, kyle, martin.renvoize, sally.healey, severine.queune, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11160
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19540
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23404
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Attachments: Bug 16787 - 'Too many holds' message appears inappropriately and is missing data
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add tests
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add tests
Bug 16787: (follow-up) Return noReservesAllowed if 0 holds per record
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add tests
Bug 16787: (follow-up) Return noReservesAllowed if 0 holds per record
Bug 16787: Rebase fixes and updates
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DBIC changes
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Shcema changes
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Screenshot showing the behavior before the patch
Screenshot showing the behavior after the patch
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template
Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Bug 16787: Add noReservesAllowed to club holds
Bug 16787: DO NOT PUSH - Schema updates
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template
Bug 16787: (follow-up) Add noReservesAllowed to ERROR_MAP
Bug 16787: (follow-up) Add and adjust ILSDI tests

Description Nick Clemens 2016-06-21 13:58:47 UTC
bug 11250 changed the holds message to be and if,elsif loop, falling through to catch the first appropriate message

bug 13116 changed the reporting from CanItemBeReserved causing tooManyHolds to trigger when no holds are allowed

A side effect is that 'maxreserves' is only passed to the template when there are too many reserves, but the too many reserves message is called when no holds are available

To recreate:
Set maxreserves to 50
Set some item type to allow 0 holds
Attempt to place a hold on a record with that itemtype
Message will be 
"Too many holds: [Firstname Surname] can only place a maximum of total holds."
Message should be
"[Firstname Surname] cannot place any holds on these items."
Comment 1 Nick Clemens 2016-06-21 14:03:15 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-06-21 14:03:50 UTC
needs unit tests
Comment 3 George Williams (NEKLS) 2016-12-05 16:30:31 UTC
Tried to test in the sandbox but I kept getting an error message saying "CONFLICT (content): Merge conflict in C4/Reserves.pm"

George
Comment 4 Hugh Rundle 2017-10-17 00:44:30 UTC
Nick have you made any further progress on this? This bug is definitely confusing and we'd love it resolved at my library.
Comment 5 Jonathan Druart 2017-11-02 16:00:37 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2017-11-02 16:00:40 UTC Comment hidden (obsolete)
Comment 7 David Bourgault 2017-11-03 19:22:24 UTC
Is there a step missing form the test plan? I applied the patch, change my Circulation rules to set no holds for All/All (I have no other rules for my test patron's category) and tried to place a hold but got the message "No available items.", but I can see it as available on the OPAC detail page.
Comment 8 Nick Clemens 2017-11-04 01:42:40 UTC
(In reply to David Bourgault from comment #7)
> Is there a step missing form the test plan? I applied the patch, change my
> Circulation rules to set no holds for All/All (I have no other rules for my
> test patron's category) and tried to place a hold but got the message "No
> available items.", but I can see it as available on the OPAC detail page.

Sorry, I did miss a detail, this patch only affects the staff side, not the OPAC
Comment 9 Victor Grousset/tuxayo 2018-03-12 11:39:17 UTC
conflicts:

~/d/koha ❯❯❯ git bz apply 16787
Bug 16787 - 'Too many holds' message appears inappropriately and is missing data

68894 - Bug 16787: 'Too many holds' message appears inappropriately and is missing data
68895 - Bug 16787: Add tests

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
M	reserve/request.pl
Falling back to patching base and 3-way merge...
Auto-merging reserve/request.pl
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
Auto-merging C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16787: 'Too many holds' message appears inappropriately and is missing data
The copy of the patch that failed is found in: .git/rebase-apply/patch
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".
Patch left in /tmp/Bug-16787-Too-many-holds-message-appears-inappropr-CKsj5R.patch
Comment 10 Nick Clemens 2018-03-14 08:54:59 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2018-03-14 08:55:03 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2018-03-14 15:57:28 UTC Comment hidden (obsolete)
Comment 13 Axelle Aix-Marseille Université 2018-03-15 09:59:24 UTC
tested on biblibre sandbox 11 : 

- Set maxreserves to 50
- Set 'Holds per record' to 0 for some itemtype/patron combination
- Attempt to place a hold

No message in any case appears
Comment 14 Koha Team University Lyon 3 2018-10-11 09:31:04 UTC
i've tried to install it on a sandbox :

CONFLICT (content): Merge conflict in reserve/request.pl 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


The previous status was "failed QA" so there's also to review the patch as it doesn't seem to function correctly.
Comment 15 Nick Clemens 2019-07-01 14:02:35 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2019-07-01 14:02:39 UTC Comment hidden (obsolete)
Comment 17 Nick Clemens 2019-07-01 14:02:45 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2019-07-01 14:02:51 UTC Comment hidden (obsolete)
Comment 19 Hayley Pelham 2019-07-26 02:42:08 UTC
Patch doesn't apply:

Applying: Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Applying: Bug 16787: Add tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Holds.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Holds.t
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16787: Add tests
Comment 20 Nick Clemens 2019-12-10 12:42:02 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2020-01-15 17:01:17 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2020-02-26 15:52:55 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
CONFLICT (content): Merge conflict in C4/Reserves.pm
Comment 23 Jonathan Druart 2020-02-26 15:56:44 UTC
Looks like you will need to add the new statuses to the enum of club_holds_to_patron_holds.error_code
Comment 24 Nick Clemens 2020-03-02 16:50:30 UTC Comment hidden (obsolete)
Comment 25 Nick Clemens 2020-03-02 16:50:33 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2020-03-18 10:43:26 UTC
Missing signoff here.
Comment 27 Jonathan Druart 2020-03-18 10:46:08 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2020-03-18 10:47:28 UTC
Adding the DBIC changes to help testing in a sandbox.


AM123, Lyon3, are you still interested in this bugfix?
Comment 29 Nick Clemens 2020-03-23 16:21:19 UTC Comment hidden (obsolete)
Comment 30 Nick Clemens 2020-03-23 16:21:23 UTC Comment hidden (obsolete)
Comment 31 Nick Clemens 2020-03-23 16:21:27 UTC Comment hidden (obsolete)
Comment 32 Sally 2020-03-26 16:10:07 UTC
Patch no longer applies.
Comment 33 Nick Clemens 2020-03-26 16:47:27 UTC Comment hidden (obsolete)
Comment 34 Fridolin Somers 2020-03-27 10:51:00 UTC
Bug 23404 also fixed a itemtype issue in holds
Comment 35 Fridolin Somers 2020-03-27 11:03:24 UTC
> This patch also fixes a FIXME about using effective_itemtype to fetch item rules

Looks like there are several places where GetBranchItemRule() is called with $item->itype instead of $item->effective_itemtype.
Maybe we should create a new Bug for that.
Comment 36 Victor Grousset/tuxayo 2020-05-11 02:05:49 UTC
> 4 - The message will be 'Too many holds'

Is that valid if my messages are the following?

> Too many holds: $EXAMPLE $USER can only place a maximum of 0 total holds.

It's for the «itemtype to allow no holds» it's a book went to circ rules and added a rule:
all branches, all categories, type books: Holds allowed (total): 0

The other record I get

> Too many holds for this record: $EXAMPLE $USER can only place a maximum of 0 hold(s) on this record.

It's for the «Holds per record' to 0 for another itemtype/patron combination» it for "music" and I added a rule;
all branches, all categories, type music: Holds per record (count): 0
Comment 37 Sally 2020-06-23 15:24:06 UTC
Patch doesn't apply.
Comment 38 Nick Clemens 2020-08-04 18:41:55 UTC Comment hidden (obsolete)
Comment 39 Nick Clemens 2020-08-04 18:41:59 UTC Comment hidden (obsolete)
Comment 40 Nick Clemens 2020-08-04 18:43:53 UTC Comment hidden (obsolete)
Comment 41 Andrew Fuerste-Henry 2020-08-04 20:16:22 UTC
I'm getting errors when trying to place club holds. 
- create rule that says 0 holds allowed for a specific category and item
- create a club
- add 2 patrons to club -- one of the category from your rule, one of a category that can hold that item type
- attempt to place club hold for an item of the item type covered by your rule
- get error "Something went wrong, check Koha logs for details."

When placing individual holds, I'm not seeing a message of "No holds allowed: [Firstname Surname] cannot place holds on any of these items." Instead, I get "Cannot place hold: No items are available to be placed on hold." I'm happy enough with that message, but it's not what the test plan indicated.
Comment 42 Nick Clemens 2020-12-23 15:07:13 UTC Comment hidden (obsolete)
Comment 43 Nick Clemens 2020-12-23 15:07:17 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2020-12-23 15:07:21 UTC Comment hidden (obsolete)
Comment 45 Séverine Queune 2021-04-01 13:01:26 UTC
Needs rebase once more...

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Auto-merging C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Comment 46 Nick Clemens 2021-04-07 12:37:09 UTC Comment hidden (obsolete)
Comment 47 Nick Clemens 2021-04-07 12:37:13 UTC Comment hidden (obsolete)
Comment 48 Nick Clemens 2021-04-07 12:37:18 UTC Comment hidden (obsolete)
Comment 49 Nick Clemens 2021-04-08 15:55:31 UTC
Created attachment 119335 [details] [review]
Bug 16787: 'Too many holds' message appears inappropriately and is missing data

This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when
allowedreserves=0. This allows passing to the user an appropriate
message about the availability of items for holds

This patch also fixes a FIXME about using effective_itemtype to fetch item rules

To test:
1 - Set one itemtype to allow no holds
2 - Set 'Holds per record' to 0 for another itemtype/patron combination
3 - Create or find 2 records, each with items only of the itemtypes above
3 - Attempt to place a hold for a patron on each record above
4 - The message will be 'Too many holds'
5 - Apply patch and repeat
6 - Message should be "Cannot place hold: no item are available to be placed on hold"
7 - Try placing a multihold with either record above and a holdable record,
  message should end "Cannot place hold on some items'
8 - prove -v t/db_dependent/Holds.t

Rebase - Fix test expectation
Comment 50 Nick Clemens 2021-04-08 15:55:36 UTC
Created attachment 119336 [details] [review]
Bug 16787: Add noReservesAllowed to club holds
Comment 51 Nick Clemens 2021-04-08 15:55:40 UTC
Created attachment 119337 [details] [review]
Bug 16787: DO NOT PUSH - Schema updates
Comment 52 Owen Leonard 2021-04-15 11:42:43 UTC
Created attachment 119616 [details]
Screenshot showing the behavior before the patch

I see a difference in how the holds process behaves before and after this patch and I'm not sure it's intentional. It seems to be a regression. It happens when I try to access the multi-hold process by linking directly to the hold page.

The search results page will prevent me from submitting multiple titles for holds if one of them can't be placed on hold, but I can use a URL:

/cgi-bin/koha/reserve/request.pl?biblionumbers=2329/2596/4796/4439/4896/2655/2740/

...where one of those biblionumbers is a title with all non-holdable items.

From that page, select a patron and submit. The following page will warn you that one of the titles you submitted has no items which can be placed on hold.
Comment 53 Owen Leonard 2021-04-15 11:45:39 UTC
Created attachment 119617 [details]
Screenshot showing the behavior after the patch

After the patch, there is no warning.

I know that I'm describing a workflow that isn't one which will be followed under regular circumstances (accessing the page via a handmade URL), but I think the page should still be performing this check.
Comment 54 Owen Leonard 2021-04-15 12:15:56 UTC
(In reply to Owen Leonard from comment #52)

> I see a difference in how the holds process behaves before and after this
> patch and I'm not sure it's intentional.

This is not a problem after all because I had my item-level_itypes preference switched and I didn't realize it. After testing with  item-level_itypes ON and OFF, the behavior is correct.
Comment 55 Owen Leonard 2021-04-15 12:16:51 UTC
Created attachment 119619 [details] [review]
Bug 16787: 'Too many holds' message appears inappropriately and is missing data

This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when
allowedreserves=0. This allows passing to the user an appropriate
message about the availability of items for holds

This patch also fixes a FIXME about using effective_itemtype to fetch item rules

To test:
1 - Set one itemtype to allow no holds
2 - Set 'Holds per record' to 0 for another itemtype/patron combination
3 - Create or find 2 records, each with items only of the itemtypes above
3 - Attempt to place a hold for a patron on each record above
4 - The message will be 'Too many holds'
5 - Apply patch and repeat
6 - Message should be "Cannot place hold: no item are available to be placed on hold"
7 - Try placing a multihold with either record above and a holdable record,
  message should end "Cannot place hold on some items'
8 - prove -v t/db_dependent/Holds.t

Rebase - Fix test expectation

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 56 Owen Leonard 2021-04-15 12:16:55 UTC
Created attachment 119620 [details] [review]
Bug 16787: Add noReservesAllowed to club holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 57 Owen Leonard 2021-04-15 12:17:00 UTC
Created attachment 119621 [details] [review]
Bug 16787: DO NOT PUSH - Schema updates

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 58 Nick Clemens 2021-04-15 12:20:33 UTC
Created attachment 119622 [details] [review]
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template
Comment 59 Owen Leonard 2021-04-15 12:30:06 UTC
Created attachment 119623 [details] [review]
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 60 Martin Renvoize 2021-04-16 13:49:51 UTC
Created attachment 119738 [details] [review]
Bug 16787: 'Too many holds' message appears inappropriately and is missing data

This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when
allowedreserves=0. This allows passing to the user an appropriate
message about the availability of items for holds

This patch also fixes a FIXME about using effective_itemtype to fetch item rules

To test:
1 - Set one itemtype to allow no holds
2 - Set 'Holds per record' to 0 for another itemtype/patron combination
3 - Create or find 2 records, each with items only of the itemtypes above
3 - Attempt to place a hold for a patron on each record above
4 - The message will be 'Too many holds'
5 - Apply patch and repeat
6 - Message should be "Cannot place hold: no item are available to be placed on hold"
7 - Try placing a multihold with either record above and a holdable record,
  message should end "Cannot place hold on some items'
8 - prove -v t/db_dependent/Holds.t

Rebase - Fix test expectation

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Martin Renvoize 2021-04-16 13:49:56 UTC
Created attachment 119739 [details] [review]
Bug 16787: Add noReservesAllowed to club holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2021-04-16 13:50:01 UTC
Created attachment 119740 [details] [review]
Bug 16787: DO NOT PUSH - Schema updates

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2021-04-16 13:50:07 UTC
Created attachment 119741 [details] [review]
Bug 16787: (follow-up) Fix check to ensure reasons are passed to template

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2021-04-16 13:50:32 UTC
Tests pass, QA script is happy.. functionality is improved.

Passing QA
Comment 65 Jonathan Druart 2021-04-21 08:40:35 UTC
At first glance I'd say noReservesAllowed is missing from ERROR_MAP, could you confirm?
Comment 66 Nick Clemens 2021-04-21 11:29:07 UTC
Created attachment 119942 [details] [review]
Bug 16787: (follow-up) Add noReservesAllowed to ERROR_MAP
Comment 67 Jonathan Druart 2021-04-22 12:39:14 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 68 Jonathan Druart 2021-04-22 12:41:18 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2021-04-22 14:10:13 UTC
We have a test failing, please fix.

    #   Failed test 'Too many reserves'
    #   at t/db_dependent/ILSDI_Services.t line 372.
    #          got: 'noReservesAllowed'
    #     expected: 'tooManyReserves'
    # Looks like you failed 1 test of 8.
Comment 70 Nick Clemens 2021-04-22 15:23:17 UTC
Created attachment 120009 [details] [review]
Bug 16787: (follow-up) Add and adjust ILSDI tests
Comment 71 Jonathan Druart 2021-04-22 17:32:21 UTC
Follow-up pushed to master, thanks Nick!
Comment 72 Fridolin Somers 2021-04-29 14:05:13 UTC
Should we backported to 20.11.x ?
Or is it to much behavior change for a stable version ?

My opinion is more on no backport, reserves is really sensitive.