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.
Created attachment 41734 [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.
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>
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.
(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).
Created attachment 42449 [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.
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"
(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.
(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" ?
(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
Created attachment 56313 [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)
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>
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>
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>
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>
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>
Pushed to master for 17.05, thanks Julian!
This patch has been pushed to 16.11.x, will be in 16.11.01!