Bug 20468

(In reply to Dilan Johnpullé from comment #2)
> Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Thx Dilan

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).

(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?

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."

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."

(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.

(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 ;)

(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 ;)

Merged the last two patches into the last one of Owen.
Thx Owen. Great design improvements !

There are several "Reason for cancellation:" pop-ups, it that expected?

(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..

undefined vs null is confusing here.

Moreover cannot we display the title of the biblio in the popup?

(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?

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.

(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.

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>

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>

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>

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>

Awesome work all!

Pushed to master for 18.11

Enhancement, won't be backported to 18.05.x series.