Bug 30579 - When placing item level hold, some options that are not used are not disabled
Summary: When placing item level hold, some options that are not used are not disabled
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Kyle M Hall
URL:
Keywords:
: 30564 (view as bug list)
Depends on: 30577
Blocks:
  Show dependency treegraph
 
Reported: 2022-04-21 12:32 UTC by Nick Clemens
Modified: 2024-03-17 23:15 UTC (History)
14 users (show)

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


Attachments
Bug 30579: Disentangle multi-hold and single bib forms (51.50 KB, patch)
2022-04-21 12:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Group next available hold options, and disable when item is chosen (10.21 KB, patch)
2022-04-21 12:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (51.57 KB, patch)
2022-04-21 15:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30579: Group next available hold options, and disable when item is chosen (10.27 KB, patch)
2022-04-21 15:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (51.62 KB, patch)
2022-04-22 15:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30579: Group next available hold options, and disable when item is chosen (10.32 KB, patch)
2022-04-22 15:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Screenshot of place hold page with patch applied (141.48 KB, image/png)
2022-04-28 14:19 UTC, Katrin Fischer
Details
Suggested update (137.40 KB, image/gif)
2022-05-02 14:27 UTC, Owen Leonard
Details
Bug 30579: (follow-up) Update layout to make hold any vs. hold item clearer (19.22 KB, patch)
2022-05-02 16:14 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (51.62 KB, patch)
2022-05-03 10:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Group next available hold options, and disable when item is chosen (10.31 KB, patch)
2022-05-03 10:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Update layout to make hold any vs. hold item clearer (19.21 KB, patch)
2022-05-03 10:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Update layout to make hold any vs. hold item clearer (19.15 KB, patch)
2022-05-11 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30579: (follow-up) Expand range of controls which are disabled/enabled (4.62 KB, patch)
2022-05-11 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (9.19 KB, patch)
2022-09-23 15:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Whitespace (27.31 KB, patch)
2022-09-23 15:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Group next available hold options, and disable when item is chosen (9.77 KB, patch)
2022-09-23 15:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Update layout to make hold any vs. hold item clearer (10.64 KB, patch)
2022-09-23 15:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (48.94 KB, patch)
2023-03-28 14:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (50.16 KB, patch)
2023-03-29 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Update form checker for each form (3.10 KB, patch)
2023-04-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (53.03 KB, patch)
2023-05-05 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (53.08 KB, patch)
2023-05-05 18:25 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (53.17 KB, patch)
2023-05-19 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (53.21 KB, patch)
2023-05-19 18:44 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30579: Add missing for attribute on label (2.16 KB, patch)
2023-05-23 09:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (54.04 KB, patch)
2023-08-28 12:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Add missing for attribute on label (2.16 KB, patch)
2023-08-28 12:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Remove doubled notes field (1.28 KB, patch)
2023-08-28 12:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add fieldsets and style disabled elements (55.33 KB, patch)
2023-08-28 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (54.01 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Add missing for attribute on label (2.16 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Remove doubled notes field (1.28 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add fieldsets and style disabled elements (55.33 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options (1.91 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add required label and warning when hold levels required (2.80 KB, patch)
2023-10-25 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (54.08 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: Add missing for attribute on label (2.23 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: Remove doubled notes field (1.35 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add fieldsets and style disabled elements (55.40 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options (1.98 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add required label and warning when hold levels required (2.88 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: (follow-up) fix button id (2.12 KB, patch)
2023-10-26 17:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 30579: Disentangle multi-hold and single bib forms (53.43 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Add missing for attribute on label (2.23 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: Remove doubled notes field (1.35 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add fieldsets and style disabled elements (54.84 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options (1.98 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Add required label and warning when hold levels required (2.88 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) fix button id (2.12 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30579: (follow-up) Remove unnecessary nested form elements (1.38 KB, patch)
2024-03-15 18:04 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-04-21 12:32:05 UTC

    
Comment 1 Nick Clemens 2022-04-21 12:33:09 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2022-04-21 12:33:14 UTC Comment hidden (obsolete)
Comment 3 Andrew Fuerste-Henry 2022-04-21 15:08:44 UTC Comment hidden (obsolete)
Comment 4 Andrew Fuerste-Henry 2022-04-21 15:16:52 UTC Comment hidden (obsolete)
Comment 5 Andrew Fuerste-Henry 2022-04-21 15:16:57 UTC Comment hidden (obsolete)
Comment 6 Andrew Fuerste-Henry 2022-04-22 15:03:05 UTC Comment hidden (obsolete)
Comment 7 Andrew Fuerste-Henry 2022-04-22 15:03:09 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2022-04-28 14:19:59 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2022-05-02 14:27:40 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2022-05-02 16:14:09 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2022-05-03 10:22:44 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2022-05-03 10:22:48 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens 2022-05-03 10:22:52 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2022-05-03 10:23:56 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2022-05-11 12:23:22 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2022-05-11 12:23:27 UTC Comment hidden (obsolete)
Comment 17 Sally 2022-05-17 15:19:46 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2022-09-23 15:52:29 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2022-09-23 15:52:33 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2022-09-23 15:52:37 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2022-09-23 15:52:41 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2022-09-23 15:53:26 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2023-03-28 14:27:51 UTC
Created attachment 148835 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly
Comment 24 Matt Blenkinsop 2023-03-29 08:27:20 UTC
Discussed at hackfest - next available item from an item group needs a pickup location as an error is being thrown
Comment 25 Nick Clemens 2023-03-29 10:28:19 UTC
Created attachment 148898 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly
Comment 26 Matt Blenkinsop 2023-04-05 10:12:16 UTC
Hi Nick - I'm getting an alert asking me to select a pickup location when placing a hold on next available item, even when a location is selected. Could you check again at your end and see if you're seeing the same? Looks like the alert is being thrown by the else block in lines 1570 - 1575
Comment 27 Nick Clemens 2023-04-07 12:07:32 UTC
Created attachment 149271 [details] [review]
Bug 30579: (follow-up) Update form checker for each form
Comment 28 Matt Blenkinsop 2023-04-17 09:29:50 UTC
Still getting an error, this time when placing a hold on next available item in an item group - I'm getting an alert saying "Please select an item group to place hold", even though an item group is selected
Comment 29 Magnus Enger 2023-05-05 13:21:08 UTC
I don't think this works as intended for me. 

1. When I load the page, the radio button to the right of "Hold next available item" is active/selected. 
- Under "Hold next available item from an item group", the dropdown and the radio button is disabled. The dropdown is "greyed out" and nothing happens if I click on either of them. 
- But under "Place a hold on a specific item" the dropdown for "Allowed pickup locations" is not "greyed out" and I can interact with it. I can click on the radio buttons under "Hold", and if I do, the radio button to the right of "Hold next available item" is no longer active/selected. In fact, none of the three radio buttons to the right of the headings are active/selected.

2. If i click on the radio button for "Hold next available item from an item group"
- I can interact with the "Hold" radio button and the "Allowed pickup locations" dropdown under "Place a hold on a specific item". 
- If i click on a radio button under "Hold", then the radio button to the right of "Hold next available item from an item group" remains active/selected (this is different from what happens under 1 above).
Comment 30 Magnus Enger 2023-05-05 13:21:53 UTC
PS. Tested on 112.0.5615.49 snap (64-bit) on Ubuntu
Comment 31 Nick Clemens 2023-05-05 14:56:20 UTC
Created attachment 150767 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly
Comment 32 Hinemoea Viault 2023-05-05 18:25:03 UTC
Created attachment 150786 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly

Signed-off-by: Hinemoea Viault <hinemoea.viault@inlibro.com>
Comment 33 David Nind 2023-05-09 01:33:52 UTC
Hi Hinemoea.

As you have signed off this bug, could you update the bug status to Signed Off?

That way, you will get the credit on the dashboard (https://dashboard.koha-community.org/).

(It will also move in to the Needs QA queue, for the QA Team to do their thing!)

David
Comment 34 Sam Lau 2023-05-18 19:23:35 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Comment 35 Nick Clemens 2023-05-19 15:54:04 UTC
Created attachment 151483 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Comment 36 Sam Lau 2023-05-19 18:44:30 UTC
Created attachment 151493 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Place a hold for a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Confirm you can successfully place all 3 types of holds
10 - Confirm if placing a second hold that the type is forced correctly

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 37 Jonathan Druart 2023-05-23 09:36:58 UTC
Created attachment 151558 [details] [review]
Bug 30579: Add missing for attribute on label
Comment 38 Jonathan Druart 2023-05-23 09:41:30 UTC
That's better but we should do better in my opinion. First would be to have only 1 "place hold" button maybe? And hide the form when the related radio is not checked?

Owen, what do you think?
Comment 39 Marcel de Rooy 2023-06-30 08:00:10 UTC
This is a very brave effort to get this template more clear. But as it seems, not without any discussion. There was no feedback on the previous comment.

The number of adjustments
 .../prog/en/modules/reserve/request.tt        | 587 ++++++++++--------
Does not make it easier.
Holds is very bug prone. Not to mention recent additions like item groups etc.

I am not sure if this (simple) test plan covers all edge cases. Hard to verify. Does this need some broader consensus/discussion?
Should this be marked as an enhancment?
Comment 40 Owen Leonard 2023-06-30 14:44:09 UTC
I could see us using a Bootstrap Collapse to show the options, with the "next available" section displayed by default.

I could also see us keeping this version with the addition of making sure that all form fields are disabled when the section isn't "active." We could even set the opacity of the inactive fieldsets to .5 or something like that in order to indicate that the controls are not available.

I find that the pickup location dropdowns are not populating, and the form doesn't prevent me from placing the hold without selecting one. That must be resolved.

I agree that a more detailed test plan would be helpful.
Comment 41 Nick Clemens 2023-08-28 12:34:44 UTC
Created attachment 154881 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Click the 'Holds' tab and search for/select a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Check 'Hold next available item from an item group'
10 - Do not select an item group
11 - Click 'Place hold' and confirm you are notified of need to select an item group
     NOTE: if you are overrirding you may also have an alert that the items cannot normally be put on hold
12 - Click 'Place hold on a specific item' - but do not select an item
13 - Click place hold and confirm there is an alert and you cannot continue
14 - Click 'Hold next available item' and place hold
15 - Hold is successfully placed
16 - Place another hold for the same patron
17 - Only the 'Hold next available item' form is enabled
18 - Confirm you cannot switch hold type
19 - Place hold
20 - Select a new patron and place an item group hold
21 - Select the same patron and place another hold - you are forced to place an item group hold
22 - Select a new patron and place a hold on a specific item
23 - Place a second hold, confirm you can only place it on a specific item

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 42 Nick Clemens 2023-08-28 12:34:46 UTC
Created attachment 154882 [details] [review]
Bug 30579: Add missing for attribute on label
Comment 43 Nick Clemens 2023-08-28 12:34:49 UTC
Created attachment 154883 [details] [review]
Bug 30579: Remove doubled notes field
Comment 44 Nick Clemens 2023-08-28 14:43:40 UTC
(In reply to Marcel de Rooy from comment #39)
> I am not sure if this (simple) test plan covers all edge cases. 

Test plan updated

> Should this be marked as an enhancment?

Done

(In reply to Owen Leonard from comment #40)
> I could see us using a Bootstrap Collapse to show the options, with the
> "next available" section displayed by default.
> 
> I could also see us keeping this version with the addition of making sure
> that all form fields are disabled when the section isn't "active." We could
> even set the opacity of the inactive fieldsets to .5 or something like that
> in order to indicate that the controls are not available.

Follow-up patch will be attached

> I find that the pickup location dropdowns are not populating, and the form
> doesn't prevent me from placing the hold without selecting one. That must be
> resolved.

I cannot recreate, can you retest? Can you confirm API is loading content on other pages like http://localhost:8081/cgi-bin/koha/admin/branches.pl
Comment 45 Nick Clemens 2023-08-28 14:46:28 UTC
Created attachment 154886 [details] [review]
Bug 30579: (follow-up) Add fieldsets and style disabled elements

This patch surrounds each section with a fieldset which allow enabling/disabling
all the elements in one go. CSS is added to make it clear which fields are disabled.

Button for item specific is moved form the multi-hold section

Javascript is simplified to use the new fieldsets

To test:
1 - Repeate test plan on earlier commit
2 - Additionally, search and select multiple records and place hold
3 - Confirm multi-hold placing works :-)
Comment 46 Emily Lamancusa 2023-09-14 21:04:29 UTC
I really like where this is going! I found a few more bugs in testing, but it seems to be nearly there.

The only two that are definite blockers for me are some odd behavior with hold policy limits:

- When trying to place a hold on an item group, I always got the prompt: "None of these items can normally be put on hold for this patron. Place hold?" although there should be nothing forbidding that hold. (with item group holds enabled and regardless of patron or AllowHoldPolicyOverride value)

- The circ rule for maximum number of items allowed per record was not enforced for item-level holds. i.e. with a limit of 2, it was possible to place 3 item-level holds for the same patron even if overrides were not allowed. If overrides are allowed, there should be a warning at the top of the page, but the warning does not appear for a patron with the max number of item-level holds.


Not necessarily blockers but would be nice to fix:

- The select2's for pickup location are not actually disabled when the fieldset is disabled. (They are greyed out, and the data selected is not used, but they're still clickable.) Is this an oversight, or is it an unfortunate characteristic of select2's? (not necessarily a blocker for me because the visual cue and actual processing of data are correct)

- When enforcing that a patron with an existing item-level hold may only place item-level holds, the heading for item-level holds says (Required) in bright red. Same thing for item group holds. However, the heading for record-level holds does not say (Required). (this is the case in master, but if it's easy to add in here for consistency, I'd say do it)
Comment 47 Nick Clemens 2023-10-25 12:15:03 UTC
Created attachment 157787 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Click the 'Holds' tab and search for/select a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Check 'Hold next available item from an item group'
10 - Do not select an item group
11 - Click 'Place hold' and confirm you are notified of need to select an item group
     NOTE: if you are overrirding you may also have an alert that the items cannot normally be put on hold
12 - Click 'Place hold on a specific item' - but do not select an item
13 - Click place hold and confirm there is an alert and you cannot continue
14 - Click 'Hold next available item' and place hold
15 - Hold is successfully placed
16 - Place another hold for the same patron
17 - Only the 'Hold next available item' form is enabled
18 - Confirm you cannot switch hold type
19 - Place hold
20 - Select a new patron and place an item group hold
21 - Select the same patron and place another hold - you are forced to place an item group hold
22 - Select a new patron and place a hold on a specific item
23 - Place a second hold, confirm you can only place it on a specific item

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 48 Nick Clemens 2023-10-25 12:15:06 UTC
Created attachment 157788 [details] [review]
Bug 30579: Add missing for attribute on label
Comment 49 Nick Clemens 2023-10-25 12:15:09 UTC
Created attachment 157789 [details] [review]
Bug 30579: Remove doubled notes field
Comment 50 Nick Clemens 2023-10-25 12:15:12 UTC
Created attachment 157790 [details] [review]
Bug 30579: (follow-up) Add fieldsets and style disabled elements

This patch surrounds each section with a fieldset which allow enabling/disabling
all the elements in one go. CSS is added to make it clear which fields are disabled.

Button for item specific is moved form the multi-hold section

Javascript is simplified to use the new fieldsets

To test:
1 - Repeate test plan on earlier commit
2 - Additionally, search and select multiple records and place hold
3 - Confirm multi-hold placing works :-)
Comment 51 Nick Clemens 2023-10-25 12:15:15 UTC
Created attachment 157791 [details] [review]
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options
Comment 52 Nick Clemens 2023-10-25 12:15:18 UTC
Created attachment 157792 [details] [review]
Bug 30579: (follow-up) Add required label and warning when hold levels required
Comment 53 Nick Clemens 2023-10-25 12:19:31 UTC
(In reply to Emily Lamancusa from comment #46)
> I really like where this is going! I found a few more bugs in testing, but
> it seems to be nearly there.
> 
> The only two that are definite blockers for me are some odd behavior with
> hold policy limits:
> 
> - When trying to place a hold on an item group, I always got the prompt:
> "None of these items can normally be put on hold for this patron. Place
> hold?" although there should be nothing forbidding that hold. (with item
> group holds enabled and regardless of patron or AllowHoldPolicyOverride
> value)

Button updated to fix this
> 
> - The circ rule for maximum number of items allowed per record was not
> enforced for item-level holds. i.e. with a limit of 2, it was possible to
> place 3 item-level holds for the same patron even if overrides were not
> allowed. If overrides are allowed, there should be a warning at the top of
> the page, but the warning does not appear for a patron with the max number
> of item-level holds.

I don't recreate this one
> 
> 
> Not necessarily blockers but would be nice to fix:
> 
> - The select2's for pickup location are not actually disabled when the
> fieldset is disabled. 

I don't see a way to accomplish this, I think it's just select2

> - When enforcing that a patron with an existing item-level hold may only
> place item-level holds, the heading for item-level holds says (Required) in
> bright red. 

Added
Comment 54 Emily Lamancusa 2023-10-26 17:28:42 UTC
Created attachment 157937 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Click the 'Holds' tab and search for/select a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Check 'Hold next available item from an item group'
10 - Do not select an item group
11 - Click 'Place hold' and confirm you are notified of need to select an item group
     NOTE: if you are overrirding you may also have an alert that the items cannot normally be put on hold
12 - Click 'Place hold on a specific item' - but do not select an item
13 - Click place hold and confirm there is an alert and you cannot continue
14 - Click 'Hold next available item' and place hold
15 - Hold is successfully placed
16 - Place another hold for the same patron
17 - Only the 'Hold next available item' form is enabled
18 - Confirm you cannot switch hold type
19 - Place hold
20 - Select a new patron and place an item group hold
21 - Select the same patron and place another hold - you are forced to place an item group hold
22 - Select a new patron and place a hold on a specific item
23 - Place a second hold, confirm you can only place it on a specific item

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 55 Emily Lamancusa 2023-10-26 17:28:45 UTC
Created attachment 157938 [details] [review]
Bug 30579: Add missing for attribute on label

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 56 Emily Lamancusa 2023-10-26 17:28:48 UTC
Created attachment 157939 [details] [review]
Bug 30579: Remove doubled notes field

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 57 Emily Lamancusa 2023-10-26 17:28:51 UTC
Created attachment 157940 [details] [review]
Bug 30579: (follow-up) Add fieldsets and style disabled elements

This patch surrounds each section with a fieldset which allow enabling/disabling
all the elements in one go. CSS is added to make it clear which fields are disabled.

Button for item specific is moved form the multi-hold section

Javascript is simplified to use the new fieldsets

To test:
1 - Repeate test plan on earlier commit
2 - Additionally, search and select multiple records and place hold
3 - Confirm multi-hold placing works :-)

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 58 Emily Lamancusa 2023-10-26 17:28:53 UTC
Created attachment 157941 [details] [review]
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 59 Emily Lamancusa 2023-10-26 17:28:56 UTC
Created attachment 157942 [details] [review]
Bug 30579: (follow-up) Add required label and warning when hold levels required

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 60 Emily Lamancusa 2023-10-26 17:28:59 UTC
Created attachment 157943 [details] [review]
Bug 30579: (follow-up) fix button id

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 61 Emily Lamancusa 2023-10-26 17:29:21 UTC
Awesome, just tested the fixes and they all seem to be working!
Adding a tiny follow-up patch to fix what I assume is a copy/paste error, though.

In the "Use same conditions..." follow-up, shouldn't the button id still be "hold_grp_btn" though? (the patch changed it to "hold_any_btn")

> > - The circ rule for maximum number of items allowed per record was not
> > enforced for item-level holds. i.e. with a limit of 2, it was possible to
> > place 3 item-level holds for the same patron even if overrides were not
> > allowed. If overrides are allowed, there should be a warning at the top of
> > the page, but the warning does not appear for a patron with the max number
> > of item-level holds.
> 
> I don't recreate this one

I'm not recreating it anymore either. Maybe it was just something weird on my end.

I don't follow Koha's template logic well enough yet to be confident QAing this one, but I added an additional sign-off since I confirm everything worked as expected in my testing.
Comment 62 Nick Clemens 2023-10-27 13:32:38 UTC
(In reply to Emily Lamancusa from comment #61)
> Awesome, just tested the fixes and they all seem to be working!
> Adding a tiny follow-up patch to fix what I assume is a copy/paste error,
> though.
> 
> In the "Use same conditions..." follow-up, shouldn't the button id still be
> "hold_grp_btn" though? (the patch changed it to "hold_any_btn")

Yes, lazy copy/pasta error, thank you for follow-up :-)
Comment 63 Nick Clemens 2024-03-15 18:04:13 UTC
Created attachment 163260 [details] [review]
Bug 30579: Disentangle multi-hold and single bib forms

This patch alters the structure of the hold request page to make it clear
that "Hold next available", "Hold item group", and "Hold specific item" are
mutually-exclusive options.

While there is some duplication from this, it makes the sections easier to read
and allows for more variation in the two forms

To test:
 1 - Find a bib with multiple items
 2 - Enable item groups and item group holds in system preferences
 3 - Load the records detail page
 4 - Add an item group on the item groups tab
 5 - Select two items and add to the group
 6 - Click the 'Holds' tab and search for/select a patron
 7 - Confirm the three levels of holds are clear
 8 - Confirm checking the radio next to one disables the others
 9 - Check 'Hold next available item from an item group'
10 - Do not select an item group
11 - Click 'Place hold' and confirm you are notified of need to select an item group
     NOTE: if you are overrirding you may also have an alert that the items cannot normally be put on hold
12 - Click 'Place hold on a specific item' - but do not select an item
13 - Click place hold and confirm there is an alert and you cannot continue
14 - Click 'Hold next available item' and place hold
15 - Hold is successfully placed
16 - Place another hold for the same patron
17 - Only the 'Hold next available item' form is enabled
18 - Confirm you cannot switch hold type
19 - Place hold
20 - Select a new patron and place an item group hold
21 - Select the same patron and place another hold - you are forced to place an item group hold
22 - Select a new patron and place a hold on a specific item
23 - Place a second hold, confirm you can only place it on a specific item

Signed-off-by: hinemoea <hinemoea@inlibro.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 64 Nick Clemens 2024-03-15 18:04:16 UTC
Created attachment 163261 [details] [review]
Bug 30579: Add missing for attribute on label

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 65 Nick Clemens 2024-03-15 18:04:20 UTC
Created attachment 163262 [details] [review]
Bug 30579: Remove doubled notes field

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 66 Nick Clemens 2024-03-15 18:04:23 UTC
Created attachment 163263 [details] [review]
Bug 30579: (follow-up) Add fieldsets and style disabled elements

This patch surrounds each section with a fieldset which allow enabling/disabling
all the elements in one go. CSS is added to make it clear which fields are disabled.

Button for item specific is moved form the multi-hold section

Javascript is simplified to use the new fieldsets

To test:
1 - Repeate test plan on earlier commit
2 - Additionally, search and select multiple records and place hold
3 - Confirm multi-hold placing works :-)

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 67 Nick Clemens 2024-03-15 18:04:26 UTC
Created attachment 163264 [details] [review]
Bug 30579: (follow-up) Use same conditions on item groups place hold button as on other options

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 68 Nick Clemens 2024-03-15 18:04:29 UTC
Created attachment 163265 [details] [review]
Bug 30579: (follow-up) Add required label and warning when hold levels required

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 69 Nick Clemens 2024-03-15 18:04:32 UTC
Created attachment 163266 [details] [review]
Bug 30579: (follow-up) fix button id

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 70 Nick Clemens 2024-03-15 18:04:35 UTC
Created attachment 163267 [details] [review]
Bug 30579: (follow-up) Remove unnecessary nested form elements