Bug 20468 - Multiselect on staff article requests form
Summary: Multiselect on staff article requests form
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20469 20472
  Show dependency treegraph
 
Reported: 2018-03-26 06:33 UTC by Marcel de Rooy
Modified: 2018-08-10 13:23 UTC (History)
6 users (show)

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


Attachments
Bug 20468: Add multi-select to staff article request form (14.94 KB, patch)
2018-03-26 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: Add multi-select to staff article request form (15.08 KB, patch)
2018-04-23 10:32 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 20468: (QA follow-up) Design issues (1.95 KB, patch)
2018-06-01 07:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: (follow-up) Changes to checkbox/menu interactions (11.63 KB, patch)
2018-06-01 16:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20468: (follow-up) Changes to checkbox/menu interactions (11.85 KB, patch)
2018-06-04 07:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: (follow-up) Cancel multiple items for same reason (2.45 KB, patch)
2018-06-06 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: (follow-up) Cancel multiple items for same reason (2.58 KB, patch)
2018-06-07 06:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: (QA follow-up) Deactivate select/clear all for empty tab (1.39 KB, patch)
2018-07-02 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20468: Add multi-select to staff article request form (15.16 KB, patch)
2018-07-20 16:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20468: (follow-up) Changes to checkbox/menu interactions (11.89 KB, patch)
2018-07-20 16:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20468: (follow-up) Cancel multiple items for same reason (2.62 KB, patch)
2018-07-20 16:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20468: (QA follow-up) Deactivate select/clear all for empty tab (1.43 KB, patch)
2018-07-20 16:33 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 Marcel de Rooy 2018-03-26 06:33:54 UTC

    
Comment 1 Marcel de Rooy 2018-03-26 10:01:48 UTC Comment hidden (obsolete)
Comment 2 Dilan Johnpullé 2018-04-23 10:32:44 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2018-04-23 10:53:08 UTC
(In reply to Dilan Johnpullé from comment #2)
> Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Thx Dilan
Comment 4 Jonathan Druart 2018-05-30 20:42:22 UTC
Why did you put it at the bottom of the page?

IMO it seems better to place it on top of the table (inside the tab).
Comment 5 Marcel de Rooy 2018-05-31 06:39:26 UTC
(In reply to Jonathan Druart from comment #4)
> Why did you put it at the bottom of the page?
> 
> IMO it seems better to place it on top of the table (inside the tab).

IMO it apparently did not ;) Belongs somehow to the actions on selected rows too.
No blocker for QA, i guess?
Comment 6 Owen Leonard 2018-05-31 12:15:28 UTC
I agree with Joubu. In order to be consistent with every similar interface in Koha the select all/clear all links should be at the top of the table, inside the tab. 

Ideally the multi-action button should be disabled until there is a selection. If not, there should at least be a warning triggered when you choose one of those actions. Something like, "You must select one or more titles."
Comment 7 Owen Leonard 2018-05-31 12:18:03 UTC
By the way, I get an error when I try to print a slip: "Undefined subroutine &C4::Letters::GetPreparedLetter called at /home/vagrant/kohaclone/circ/article-request-slip.pl line 49."
Comment 8 Marcel de Rooy 2018-06-01 07:25:13 UTC
(In reply to Owen Leonard from comment #7)
> By the way, I get an error when I try to print a slip: "Undefined subroutine
> &C4::Letters::GetPreparedLetter called at
> /home/vagrant/kohaclone/circ/article-request-slip.pl line 49."

Could not reproduce this one.
But to be safe, added the module on bug 20851.
Not related to this report btw.
Comment 9 Marcel de Rooy 2018-06-01 07:26:20 UTC
(In reply to Owen Leonard from comment #6)
> I agree with Joubu. In order to be consistent with every similar interface
> in Koha the select all/clear all links should be at the top of the table,
> inside the tab. 
> 
> Ideally the multi-action button should be disabled until there is a
> selection. If not, there should at least be a warning triggered when you
> choose one of those actions. Something like, "You must select one or more
> titles."

Design failures are the worst ;)
Comment 10 Marcel de Rooy 2018-06-01 07:37:33 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2018-06-01 07:38:23 UTC
(In reply to Marcel de Rooy from comment #10)
> Created attachment 75720 [details] [review] [review]
> Bug 20468: (QA follow-up) Design issues
> 
> Moving selection stuff to the top.
> Why does the dropdown menu act so weird ?

Owen, any idea why the menu does like it does?
I remember spending too much time on it before already ;)
Comment 12 Owen Leonard 2018-06-01 16:41:05 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2018-06-04 07:02:25 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2018-06-04 07:03:30 UTC
Merged the last two patches into the last one of Owen.
Thx Owen. Great design improvements !
Comment 15 Jonathan Druart 2018-06-05 16:42:08 UTC
There are several "Reason for cancellation:" pop-ups, it that expected?
Comment 16 Marcel de Rooy 2018-06-06 11:22:10 UTC
(In reply to Jonathan Druart from comment #15)
> There are several "Reason for cancellation:" pop-ups, it that expected?

Yes, HandleMulti makes repeated calls here. I agree that it is not very nice. Although you can specify a different reason per request, it is unclear which request you are cancelling.
Taking a look if a small follow-up can make a difference..
Comment 17 Marcel de Rooy 2018-06-06 11:51:37 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2018-06-06 15:49:32 UTC
undefined vs null is confusing here.

Moreover cannot we display the title of the biblio in the popup?
Comment 19 Marcel de Rooy 2018-06-07 06:32:05 UTC
(In reply to Jonathan Druart from comment #18)
> undefined vs null is confusing here.
Null comes back from a cancelled prompt. And I want to differentiate between no prompt and a cancelled prompt. I do not think it is confusing. It might need a comment.

> Moreover cannot we display the title of the biblio in the popup?
The popup is not introduced in this report. Can we postpone that?
Comment 20 Marcel de Rooy 2018-06-07 06:49:25 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2018-06-28 15:01:00 UTC
Marcel, (last patch) could not we use only null (instead of undefined vs null)?

There is also a tiny ergonomic issue, hit /circ/article-requests.pl, click the "processing" tab. There are the "select all" and "clear all" links, if the table is empty and you click on one of them they will disappear.
Comment 22 Marcel de Rooy 2018-07-02 12:20:00 UTC
(In reply to Jonathan Druart from comment #21)
> Marcel, (last patch) could not we use only null (instead of undefined vs
> null)?
Please see comment19. I need both values. These are standard values in javascript helping us to make the distinction between not yet initialized and null returned by a cancel. In order to meet your comment, I added comments.

> There is also a tiny ergonomic issue, hit /circ/article-requests.pl, click
> the "processing" tab. There are the "select all" and "clear all" links, if
> the table is empty and you click on one of them they will disappear.
Good catch. Fixed on last follow-up.
Comment 23 Marcel de Rooy 2018-07-02 12:20:23 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2018-07-20 16:33:00 UTC
Created attachment 77168 [details] [review]
Bug 20468: Add multi-select to staff article request form

Adds a column with a checkbox. Adds buttons under the table for Select,
Clear and Actions menu.
An additional javascript function HandleMulti is placed between the form
and the functions handling individual requests.

Note: The Actions menu below does not contain Print slip. This does not
work in its current form. This could be handled on a separate report.

Test plan:
[1] Enable Article Requests. Add a few requests.
[2] Test the Select all / Clear all functionality on the form.
[3] Verify that the menu options Process, Complete and Cancel work as
    expected both from the individual Actions menu as from the shared
    Actions menu for selected requests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Patch applies and functions as described.
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2018-07-20 16:33:04 UTC
Created attachment 77169 [details] [review]
Bug 20468: (follow-up) Changes to checkbox/menu interactions

This patch makes a couple of minor interface changes and updates the way
checkbox change events are handled:

 - jQueryUI tabs initialization sets a variable for "active table" which
   can be re-used by functions which affect only the visible table.
 - The batch "Actions" menu is shown or hidden based on whether there
   are checked items.
 - The item selection tools are shown or hidden based on whether there
   are rows in the table.

To test, apply the patch and test various article request actions:

 - Single "process," "complete," and "cancel" operations.
 - Select all/ clear all operations on both tabs.
 - Batch operations with checked requests.
 - Process all pending requests to confirm the selection controls for
   that table are hidden.
 - Complete all processing requests to confirm the selection controls
   for that table are hidden.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2018-07-20 16:33:08 UTC
Created attachment 77170 [details] [review]
Bug 20468: (follow-up) Cancel multiple items for same reason

The repeated prompts for cancelling multiple selected items are
confusing.
The wording is slightly adjusted. The reason is asked only once per
group of selected items.

Test plan:
Create three requests.
Select two requests and cancel (from top menu) for reason A.
Cancel third request (from item menu) for reason B.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2018-07-20 16:33:12 UTC
Created attachment 77171 [details] [review]
Bug 20468: (QA follow-up) Deactivate select/clear all for empty tab

This only pertains to the second (Processing) tab.

Test plan:
[1] Have one entry in Pending and zero in Processing.
[2] Click on Processing tab.
[3] Without this patch, you would see Select all/Clear all.
    With this patch, you won't.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Nick Clemens 2018-08-10 12:56:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 29 Martin Renvoize 2018-08-10 13:23:22 UTC
Enhancement, won't be backported to 18.05.x series.