Bug 14699 - Intranet search history issues due to DataTables pagination
Summary: Intranet search history issues due to DataTables pagination
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 12460
  Show dependency treegraph
 
Reported: 2015-08-20 13:37 UTC by Julian Maurice
Modified: 2017-06-14 22:17 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14699: Fix intranet search history issues due to pagination (4.86 KB, patch)
2015-08-20 13:43 UTC, Julian Maurice
Details | Diff | Splinter Review
[Signed-off] Bug 14699: Fix intranet search history issues due to pagination (5.04 KB, patch)
2015-09-03 14:24 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14699: Show number of selected searches in search history (2.62 KB, patch)
2015-09-08 07:27 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14699: Reword "Select all/Clear all" and change their behaviour (5.47 KB, patch)
2016-10-13 09:12 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14699: Show number of selected searches in search history (2.72 KB, patch)
2016-11-21 13:54 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14699: Reword "Select all/Clear all" and change their behaviour (5.57 KB, patch)
2016-11-21 13:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14699: Fix intranet search history issues due to pagination (5.09 KB, patch)
2016-11-25 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14699: Show number of selected searches in search history (2.73 KB, patch)
2016-11-25 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14699: Reword "Select all/Clear all" and change their behaviour (5.58 KB, patch)
2016-11-25 08:25 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 Julian Maurice 2015-08-20 13:37:09 UTC
DataTables removes hidden rows from the DOM. Because of that, "Select all", "Clear all" and the form submission don't work correctly (basically they act only on the currently displayed page).
As OPAC search history doesn't use DataTables, this bug only affects intranet.

To reproduce:
1/ Go to your search history page
2/ Make you sure you have at least 21 entries in your search history. If not, do some searches and come back here.
3/ Click "Select all" and change page. The checkboxes on the new page are not checked.
4/ Check some checkboxes on this new page and click "Clear all". Go back to the previous page, checkboxes are still checked.
5/ Check some checkboxes on at least 2 different pages and click "Delete". Only search history entries selected on the current page were deleted.

This bug affects both biblio and authority search history.
Comment 1 Julian Maurice 2015-08-20 13:43:09 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-09-03 14:24:07 UTC
Created attachment 42330 [details] [review]
[Signed-off] Bug 14699: Fix intranet search history issues due to pagination

DataTables removes hidden rows from the DOM. Because of that, "Select
all", "Clear all" and the form submission don't work correctly
(basically they act only on the currently displayed page).

This patch fixes just that.

Test plan:
1/ Go to your search history page
2/ Make you sure you have at least 21 entries in your search history. If
not, do some searches and come back here.
3/ Click "Select all" and change page. The checkboxes on the new page
should be checked.
4/ Check some checkboxes on this new page and click "Clear all". Go back
to the previous page, checkboxes shouldn't be checked.
5/ Check some checkboxes on at least 2 different pages and click
"Delete". All selected search history entries should be deleted.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Jonathan Druart 2015-09-03 15:35:46 UTC
What you describe is the expected behavior, I think.
With your patch if you filter the table and click "select all", it will select all row, even the filtered ones.
Comment 4 Julian Maurice 2015-09-04 06:36:56 UTC
(In reply to Jonathan Druart from comment #3)
> What you describe is the expected behavior, I think.
> With your patch if you filter the table and click "select all", it will
> select all row, even the filtered ones.

Then maybe "select/clear all" should be reworded to something like "select/clear all visible rows" to remove ambuiguity. Or maybe we should make a difference between invisible rows due to pagination and invisible rows due to filtering ?
But I believe "Delete" should work on all checked checkboxes, not only the visible ones (and even the filtered ones).
Comment 5 Julian Maurice 2015-09-08 07:27:11 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2015-09-12 16:40:49 UTC
Moving this to in discussion.

I think the expected behavior is somewhere between the original and that with the patches applied.  I would expect either:

1 - Preserve current behavior and change just the labels to "Select all visible / clear all visible"

2 - Select all searches but respect filters.  If I filter down to 8 searches it should select only those.  If my filter finds 21 searches (default 20 visible) it should select all 21, but not searches limited by filter.  Labels would say "With ## selected searches: delete"

A couple other things from testing:

1 - On master selecting any search past 20 doesn't change the look of the delete button, though it is enabled.  It does only function on the visible searches either way 
i.e. select search 21, button no visibly enabled, but clickable. Select #1 and #21 (with 20 visible) button is enabled, only #1 is deleted.
With the patches the link still isn't visibly enabled but will delete all selected regardless of visibility

2 - The phrasing "With selected searches: Delete" is awkward.  I think it should be "Delete selected searches" and button can simply be grayed out when none are selected With these patches phrasing would then be "Delete ## selected searches"
Comment 7 Owen Leonard 2016-01-15 18:07:14 UTC
(In reply to Nick Clemens from comment #6)

> 2 - Select all searches but respect filters.  If I filter down to 8 searches
> it should select only those.  If my filter finds 21 searches (default 20
> visible) it should select all 21, but not searches limited by filter. 
> Labels would say "With ## selected searches: delete"

I think this is the correct goal. With a filtered result clicking "select all" shouldn't select lines which are not visible because of the filter. However, with a filtered or unfiltered result clicking "select all" should select records across all pages.

> 1 - On master selecting any search past 20 doesn't change the look of the
> delete button, though it is enabled.  It does only function on the visible
> searches either way

Confirmed. Checking a box on any but the first page does not change the appearance of the delete button or show the number of selected lines even though the delete button will work.

> 2 - The phrasing "With selected searches: Delete" is awkward.

This is a pattern used elsewhere, although it's not quite as useful when there is only one "action" available. See the OPAC list interface where we have 

"Select titles to: Place hold | Remove from list"

...which changes to:

"With selected titles: Place hold | Remove from list"

I think the advantage to doing it this way is that the interface is explicit about *why* an option is disabled.
Comment 8 Julian Maurice 2016-08-22 12:52:42 UTC
(In reply to Owen Leonard from comment #7)
> (In reply to Nick Clemens from comment #6)
> 
> > 2 - Select all searches but respect filters.  If I filter down to 8 searches
> > it should select only those.  If my filter finds 21 searches (default 20
> > visible) it should select all 21, but not searches limited by filter. 
> > Labels would say "With ## selected searches: delete"
> 
> I think this is the correct goal. With a filtered result clicking "select
> all" shouldn't select lines which are not visible because of the filter.
> However, with a filtered or unfiltered result clicking "select all" should
> select records across all pages.

I think there is a problem with this solution: we can't do that with AJAX-paginated tables. It's not the case of this particular table, but if we want to have a consistent behaviour for all the "Select all/Clear all" buttons we must do something that is possible on every table.
So, for this particular table I suggest to keep the current behaviour, change only the labels to "Select all visible rows" or something similar, and maybe add an action "Clear all history" ?
Comment 9 Nick Clemens 2016-08-23 14:05:48 UTC
(In reply to Julian Maurice from comment #8)
> (In reply to Owen Leonard from comment #7)
> > (In reply to Nick Clemens from comment #6)
>I suggest to keep the current behaviour,
> change only the labels to "Select all visible rows" or something similar,
> and maybe add an action "Clear all history" ?

That sounds fine to me

The problem in comment 6 with the delete button should also be addressed:
with these patches the delete link still isn't visibly enabled but will delete all selected regardless of visibility
Comment 10 Julian Maurice 2016-10-13 09:12:52 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2016-11-21 13:54:49 UTC
Created attachment 57684 [details] [review]
[SIGNED-OFF] Bug 14699: Show number of selected searches in search history

With the number displayed, it should remove the ambiguity about what
"Select all" and "Clear all" do.
The number is repeated in the "Delete" confirmation dialog.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Owen Leonard 2016-11-21 13:54:57 UTC
Created attachment 57685 [details] [review]
[SIGNED-OFF] Bug 14699: Reword "Select all/Clear all" and change their behaviour

"Select all" -> "Select all visible rows"
"Clear all" -> "Clear selection on visible rows"

+ fix the bug where the Delete link was visually enabled only if you
checked a box in the first page (now every checkbox enable the link)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Jonathan Druart 2016-11-25 08:25:24 UTC
Created attachment 57764 [details] [review]
Bug 14699: Fix intranet search history issues due to pagination

DataTables removes hidden rows from the DOM. Because of that, "Select
all", "Clear all" and the form submission don't work correctly
(basically they act only on the currently displayed page).

This patch fixes just that.

Test plan:
1/ Go to your search history page
2/ Make you sure you have at least 21 entries in your search history. If
not, do some searches and come back here.
3/ Click "Select all" and change page. The checkboxes on the new page
should be checked.
4/ Check some checkboxes on this new page and click "Clear all". Go back
to the previous page, checkboxes shouldn't be checked.
5/ Check some checkboxes on at least 2 different pages and click
"Delete". All selected search history entries should be deleted.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Jonathan Druart 2016-11-25 08:25:31 UTC
Created attachment 57765 [details] [review]
Bug 14699: Show number of selected searches in search history

With the number displayed, it should remove the ambiguity about what
"Select all" and "Clear all" do.
The number is repeated in the "Delete" confirmation dialog.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 15 Jonathan Druart 2016-11-25 08:25:36 UTC
Created attachment 57766 [details] [review]
Bug 14699: Reword "Select all/Clear all" and change their behaviour

"Select all" -> "Select all visible rows"
"Clear all" -> "Clear selection on visible rows"

+ fix the bug where the Delete link was visually enabled only if you
checked a box in the first page (now every checkbox enable the link)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Kyle M Hall 2016-11-29 17:43:37 UTC
Pushed to master for 17.05, thanks Julian!
Comment 17 Katrin Fischer 2016-11-29 23:11:10 UTC
This patch has been pushed to 16.11.x, will be in 16.11.01!