Bug 10836 - New layout for placing holds in the OPAC
Summary: New layout for placing holds in the OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-06 19:48 UTC by Owen Leonard
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Screenshot of the revised interface (20.69 KB, image/png)
2013-09-06 19:48 UTC, Owen Leonard
Details
Bug 10836 - New layout for placing holds in the OPAC (31.09 KB, patch)
2013-09-09 16:05 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10836 - New layout for placing holds in the OPAC (31.11 KB, patch)
2013-09-24 12:26 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 10836 - New layout for placing holds in the OPAC (31.17 KB, patch)
2013-09-24 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10836 - New layout for placing holds in the OPAC (31.22 KB, patch)
2013-09-27 15:57 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10836: (follow-up) improve display toggles on the OPAC hold request form (3.07 KB, patch)
2013-09-27 15:57 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10836: (follow-up) improve display toggles on the OPAC hold request form (3.74 KB, patch)
2013-09-27 16:39 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2013-09-06 19:48:36 UTC
Created attachment 20837 [details]
Screenshot of the revised interface

I propose to change the layout of the OPAC place hold screen so that information is not displayed in a table. As options are added to the place hold process the table layout gets more an more unwieldy. This revised version makes the layout feel more open and provides a way to show and hide the finer hold controls.
Comment 1 Owen Leonard 2013-09-09 16:05:53 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2013-09-24 12:26:57 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2013-09-24 12:34:24 UTC
All good! :-)

Tested with different sysprefs configurations (not tested with IndependentBranches on)
Datepickers work as they should.

The only little thing I have to say, is when Javascript is disabled, you can pick option "A specific copy" without choosing a copy in the list of items, and then submit. No errors appear and the hold is placed on the next available copy.
But I think this should not prevent patch to be pushed.
Comment 4 Jonathan Druart 2013-09-24 13:42:59 UTC
Congratulations Owen for your patience!
Indeed, this code is really dirty and hard to change.

There are some inconsistencies, but not introduced by this patch.
It improves the ergonomics of the page and I didn't find any regression.
Since the patch does not affect core routines, there is no chance to find a big issue later.

Marked as Passed QA.
Comment 5 Jonathan Druart 2013-09-24 13:43:18 UTC Comment hidden (obsolete)
Comment 6 Galen Charlton 2013-09-27 14:46:36 UTC
One observation during testing:

- From search results, select multiple records and click the place hold button
- Select the "a specific copy" option for one of the titles.
- Select the "a specific copy" option for another one -- instead of it displaying the copies for that title, it instead display an alert whose text is "Expecting a specific copy selection".

I wouldn't expect that alert to occur prior to attempting to place the holds.
Comment 7 Galen Charlton 2013-09-27 14:49:53 UTC
(In reply to Galen Charlton from comment #6)
> One observation during testing:
> 
> - From search results, select multiple records and click the place hold
> button
> - Select the "a specific copy" option for one of the titles.
> - Select the "a specific copy" option for another one -- instead of it
> displaying the copies for that title, it instead display an alert whose text
> is "Expecting a specific copy selection".
> 
> I wouldn't expect that alert to occur prior to attempting to place the holds.

Further testing shows, however, that this was occurring before the patch.
Comment 8 Galen Charlton 2013-09-27 15:14:00 UTC
Comment on attachment 21416 [details] [review]
Bug 10836 - New layout for placing holds in the OPAC

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

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
@@ +184,5 @@
> +        var optionsID = this.id.replace("toggle-hold-options-","");
> +        var copiesRow = $("#copiesrow_"+optionsID)
> +        $("#hold-options-"+optionsID).toggle(0, function() {
> +            if( copiesRow.is(':visible') ){
> +                $("#reqany_"+optionsID).prop("checked",true);

I'm not sure about this -- if you expand options, choose to select a specific copy, then hide the options (say, because you're placing several holds at once and want to reclaim screen real estate), the request type gets silently changed to a title hold.  Is there a reason to do that?
Comment 9 Galen Charlton 2013-09-27 15:23:04 UTC
(In reply to Galen Charlton from comment #8)
> I'm not sure about this -- if you expand options, choose to select a
> specific copy, then hide the options (say, because you're placing several
> holds at once and want to reclaim screen real estate), the request type gets
> silently changed to a title hold.  Is there a reason to do that?

Looks like moving the copy table into the hold options div eliminates the need for special code to hide the copy table.
Comment 10 Galen Charlton 2013-09-27 15:57:05 UTC
Created attachment 21517 [details] [review]
Bug 10836 - New layout for placing holds in the OPAC

This patch changes the layout of the OPAC's place hold screen,
eliminating the table in favor of a more flexible, open display.

To test, place single and multiple holds under various conditions:

- Items which are not available to be place on hold
- With OpacHoldNotes on and off
- With OPACItemHolds on and off
- With OPACAllowHoldDateInFuture on and off
- With IndependentBranches on and off
- With JavaScript disabled

Test setting hold notes, holding specific copies, and setting hold start
dates. When setting a hold start date and hold expiration date, the two
datepickers should work together to prevent invalid date combinations
(dates after today, expiration dates which precede the start date).

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Comment 11 Galen Charlton 2013-09-27 15:57:17 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2013-09-27 15:58:11 UTC
I've attached a follow-up that addresses my concerns with the display toggling.  I want another pair of eyes on the follow-up; either a QA person or Owen can do it.
Comment 13 Owen Leonard 2013-09-27 16:39:27 UTC
Created attachment 21519 [details] [review]
[SIGNED-OFF] Bug 10836: (follow-up) improve display toggles on the OPAC hold request form

This patch improves the the toggles for displaying and hiding the hold
options div and the toggles for displaying the table of specific copies
to request.  It also moves the copy table into the hold options div.

To test:

[1] Select a single title in the OPAC and click on the place hold
    button.
[2] Click 'Show more options', and select a specific copy to request.
[3] Click 'Hide options'.  Verify that the hold options, including the
    copy table, are hidden.
[4] Click 'Show more options'.  Verify that the specific copy radio
    button is still selected and that the copy selected in step to is
    selected.
[5] Enable DisplayMultiPlaceHold and select several titles to
    request.
[6] Do steps 2, 3, and 4 on various titles on the request form.  Verify
    that toggling the options link for one title on the form affects
    the visibility only for that title.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

While signing off I removed one <br /> tag and corrected a float in the
CSS. This helps keep the border between individual hold listings.
Comment 14 Galen Charlton 2013-09-27 16:42:49 UTC
Pushed to master.  Thanks, Owen!