Fixes #16043 - add 'select all hosts' option into hosts page #4338

amirfefer · 2017-02-26T15:50:46Z

Select all items checkbox in hosts page selects only the hosts within the page. when the user wish to select all hosts for a bulk action, he has to select all hosts page after page.

mention-bot · 2017-02-26T15:50:49Z

@amirfefer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @shlomizadok, @tbrisker and @ohadlevy to be potential reviewers.

ohadlevy · 2017-02-26T15:56:55Z

app/views/hosts/_selected_hosts.html.erb

@@ -1,4 +1,6 @@
 <div class="row">
+  <%= alert(:class => 'alert-info hide', :close => true, :header => '',


maybe this should be warning? @Rohoover ?

@ohadlevy For which part? The selection of all items or that all items are changing owners?

I'm looking for a way to say that you are seeing only the first 20 but you actually selected all of them, so if you take an action against them its the entire list not just he one you see (same workflow if you select all of your emails on gmail).

@ohadlevy

I actually like the way it's address in the gif. I don't think it should be a warning as nothing "wrong" happened.

tbrisker

Added a few comments inline. Seeing the amount of changes to host_checkbox.js.erb, I wonder if it makes sense to just rewrite it in ES6? afaict the only reason for it to be an .erb is for the path to the spinner, which I think can be done with just a class anyways.

tbrisker · 2017-02-27T15:51:14Z

app/helpers/layout_helper.rb

@@ -81,7 +81,7 @@ def page_entries_info(collection, options = {})
  end

  def will_paginate_with_info(collection = nil, options = {})
-    content_tag(:div, :id => "pagination", :class => "row") do
+    content_tag(:div, :id => "pagination", :class => "row", 'data-meta-count' => collection.count) do


Use collection.total_entries which caches to total count

tbrisker · 2017-02-27T16:01:25Z

app/controllers/hosts_controller.rb

@@ -750,8 +750,9 @@ def load_vars_for_ajax

  def find_multiple
  # Lets search by name or id and make sure one of them exists first
-    if params[:host_names].present? || params[:host_ids].present?
-      @hosts = resource_base.where("hosts.id IN (?) or hosts.name IN (?)", params[:host_ids], params[:host_names])
+    if params[:host_names].present? || params[:host_ids].present? || params.key?(:search)


mind turning all of these to .key??

tbrisker · 2017-02-27T16:02:26Z

app/assets/javascripts/host_checkbox.js.erb

  $("#confirmation-modal form").submit();
  $('#confirmation-modal').modal('hide');
 }

 function build_modal(element, url) {
-  var url = url + "?" + $.param({host_ids: $.foremanSelectedHosts});
+  if ($("#multiple-chosen").data('multiple')) {
+    var query = $("<input>")


why do you need to create an input here if the value is encoded directly to the query?

tbrisker · 2017-02-27T16:04:23Z

app/assets/javascripts/host_checkbox.js.erb

+    var query = $("<input>")
+            .attr("type", "hidden")
+            .attr("name", "search").val($("#search").val());
+    $("#confirmation-modal form").append($(query));


no need to wrap query here, it is already a jquery object

tbrisker · 2017-02-27T16:05:43Z

app/assets/javascripts/host_checkbox.js.erb

    rmHostId(cid);
+    if ($(".alert").length){


might this be a different alert?

amirfefer · 2017-03-01T12:13:12Z

thanks @tbrisker for your review, I've followed your comments.

ohadlevy · 2017-03-02T11:43:06Z

@tbrisker I would favor to move this to ES6 in a separate commit, as that most likely will invlove the entire file vs just this particular feature?

also the js.erb file has been fixed in #4349

tbrisker · 2017-03-08T09:55:11Z

In general this works well, however there is an issue when selecting a very large amount of hosts - loading the modal with the host list can take several minutes as it isn't paginated.
The table in the modal should be paginated or only display the matching search when "select all" is chosen. Also it seems to join tables that aren't needed for the fields it displays and can be removed to speed it up.

amirfefer · 2017-03-09T16:12:39Z

@Rohoover: What do you think is more suitable for the modal dialog alert?

Info alert:

Warning alert:

@tbrisker: Thanks, I think it better to display just the alert with hosts quantity and filter query, as you can see at the screenshots above. I've fixed the join issue.

Rohoover · 2017-03-09T16:25:46Z

Use the grey box if the action isn't particular serious.

If it has strong consequences use the warning (orange) box.

Remove the chevrons in both. It would be nice to have (but not required) if the messaging would read" Reminder: All 10 hosts are selected for query filter architecture=..." because the important part is not what query they searched but that they have them all selected.

amirfefer · 2017-03-09T23:48:28Z

@Rohoover Thanks 👍 . I've kept the grey box, and changed the bold string. otherwise the user would think he made something wrong. The chevron mark notes that the string is translatable.

tbrisker

Getting there, a few mostly minor inline comments.

tbrisker · 2017-03-12T13:06:10Z

app/views/hosts/_selected_hosts.html.erb

-  <table class="<%= table_css_classes %>">
-    <thead>
+  <%= alert(:class => 'alert-info hide', :id => 'multiple-modal-alert', :close => true, :header => '',
+            :text => _("Reminder: <strong> All #{hosts.count.to_s} hosts are selected </strong> for query filter #{multiple_filter}").html_safe) %>


I think it makes sense to only show the "for query filter..." part if there is a filter, otherwise it looks odd:

The filter should also be escaped to prevent XSS :

tbrisker · 2017-03-12T13:33:42Z

app/helpers/layout_helper.rb

@@ -81,7 +81,7 @@ def page_entries_info(collection, options = {})
  end

  def will_paginate_with_info(collection = nil, options = {})
-    content_tag(:div, :id => "pagination", :class => "row") do
+    content_tag(:div, :id => "pagination", :class => "row", 'data-meta-count' => collection.total_entries) do


We also have a meta tag on the host list for number per page, it makes sense to me to use the same here as well using a <meta name="totalResults" content="#{collection.total_entries}"/> tag.

I've added this particularly to pagination div, so the total collection size would be collectable from any pagination page.

As you can see at the following screenshot, the total size is visable though hardly collectable.

The meta name per page exists only in hosts page.

@tbrisker @amirfefer meta attribute was remove at ee7002f#diff-7eb571b855131f643240b7b93ddfd44dL4

I've removed the use of meta attribute, instead I've expanded will_paginate_with_info with include_data_tag flag.

tbrisker · 2017-03-12T13:51:46Z

app/views/hosts/index.html.erb

@@ -1,2 +1,4 @@
 <% title_actions multiple_actions_select, button_group(new_link(_("Create Host"))) %>
+<%= alert(:class => 'alert-info hide', :id => 'multiple-alert', :close => false, :header => '',
+          :text => _("All #{Setting[:entries_per_page]} hosts on this page are selected. ").html_safe + link_to_function((_("Select all <b> #{@hosts.count.to_s} </b> hosts")).html_safe, "multiple_selection()")) %>


This should also use the cached @hosts.total_entries instead of @hosts.count.

tbrisker · 2017-03-12T13:58:47Z

app/views/hosts/_selected_hosts.html.erb

+      </tbody>
+    </table>
+    <%= check_box_tag "keep_selected", "", false, :title => _("Keep selected hosts for a future action") %>
+    <%= _('Keep selected hosts for a future action') %><br/><br/>


shouldn't this be possible also when selecting all? (i.e. outside the if block)

I've thought at the beginning that it useless due to cookie implementation , but I've managed not to expand this cookie 😄

tbrisker · 2017-03-12T14:10:08Z

app/controllers/hosts_controller.rb

@@ -750,8 +761,9 @@ def load_vars_for_ajax

  def find_multiple
  # Lets search by name or id and make sure one of them exists first
-    if params[:host_names].present? || params[:host_ids].present?
-      @hosts = resource_base.where("hosts.id IN (?) or hosts.name IN (?)", params[:host_ids], params[:host_names])
+    if params.key?(:host_names) || params.key?(:host_ids) || multiple_with_filter?


I wonder - do we actually ever call this method with a host_names parameter?

I haven't found anything using host names in params...

tbrisker · 2017-03-12T14:10:50Z

app/controllers/hosts_controller.rb

+    if multiple_with_filter?
+      params[:search].blank? ? _("Empty") : params[:search]
+    end
+  end


I think this should go in the helper instead of the controller.

host_helper is too large, hence I've extract this logic into hosts_and_hostgroup_helper

tbrisker · 2017-03-12T14:12:10Z

test/controllers/hosts_controller_test.rb

+      assert_equal users(:one).id_and_type, @host1.reload.is_owned_by
+      assert_equal users(:one).id_and_type, @host2.reload.is_owned_by
+    end
+  end


Please add a test that has an actual search query and make sure only the matching hosts are selected.

I've added two additional tests

tbrisker · 2017-03-12T14:19:49Z

app/assets/javascripts/host_checkbox.js

+  var count = $("#pagination").data("meta-count");
+  var per_page = $("meta[name='itemsPerPage']").attr("content");
+  var alert_text = Jed.sprintf(__("All %s hosts on this page are selected. "), per_page);
+  var select_text = Jed.sprintf(__("Select all") + "<b> %s </b>" + __("hosts"), count);


This won't translate correctly in all languages as the strings Select all and hosts will be translated separately and not as one sentence. Also above on line 96.

amirfefer · 2017-03-12T22:50:39Z

@tbrisker I've fixed most of your inline comments, I've left some inline comments.

amirfefer · 2017-03-23T16:37:56Z

@tbrisker IMO the cookie is the problem here.
I think it better to keep the cookie behavior as it was, which means only for save specific hosts selection.
I don't see any benefit for saving the user's choice in a cookie when he select a whole collection by a search filter, (because it simple and quick to re-use a whole collection selection - just 2 clicks -> all_hosts checkbox >> select all x hosts).
Therefore I've removed the keep hosts selected checkbox when a user select all hosts by query filter.

tbrisker

Here is a screenshot explaining the problem with not clearing the cookie on a new search:

Note that the top shown "20 selected", but the bottom says "40 selected" which is the correct number. (to get to this situation, search for something, mark some hosts, search a different search and select all)

tbrisker · 2017-03-26T10:49:52Z

app/helpers/hosts_and_hostgroups_helper.rb

+  end
+
+  def per_page(collection)
+     collection.total_entries > Setting[:entries_per_page] ? Setting[:entries_per_page] : collection.total_entries


Shouldn't this be in layout helper?
also, how about [Setting[:entries_per_page], collection.total_entries].min?

tbrisker · 2017-03-27T08:49:02Z

app/assets/javascripts/host_checkbox.js

+
+function undo_multiple_selection(){
+  var count = $("#pagination").data("count");
+  var per_page = $("#pagination").data("per-page");


Please cache $("#pagination") to save dom lookups

tbrisker · 2017-03-27T08:49:22Z

app/assets/javascripts/host_checkbox.js

+  var per_page = $("#pagination").data("per-page");
+  var alert_text = Jed.sprintf(__("All %s hosts on this page are selected. "), per_page);
+  var select_text = Jed.sprintf(__("Select all<b> %s </b> hosts"), count);
+  $(".text").html('<span id="multiple-chosen">' + alert_text + '<a href="#" onclick="multiple_selection();">' + select_text + '</a></span>');


Isn't .text a bit of a generic selector?

tbrisker · 2017-03-27T08:50:06Z

app/assets/javascripts/host_checkbox.js

 function toggleCheck() {
  var checked = $("#check_all").is(':checked');
  $('.host_select_boxes').each(function(index, box) {
    box.checked = checked;
    hostChecked(box);
  });
-  if(!checked)
+  var per_page = $("#pagination").data("per-page");
+  var total_host = $("#pagination").data("count");


amirfefer · 2017-03-27T21:49:57Z

@tbrisker : From your screenshot It looks like the top number is presented correctly while the bottom does not.
This issue occurs due to a counter bug which exists in develop, as you can see in the following screenshot.

I've fixed the rest.

tbrisker

a few more comments, please also note related tests seem to be failing intermittently.

tbrisker · 2017-03-28T14:26:31Z

app/assets/javascripts/host_checkbox.js

@@ -56,6 +61,8 @@ function toggle_actions() {

 // setups checkbox values upon document load
 $(function() {
+  $.count = $("#pagination").data("count");
+  $.per_page = $("#pagination").data("per-page");


Why are you adding these to the jQuery object?

also please cache $("#pagination")

tbrisker · 2017-03-28T14:26:49Z

app/assets/javascripts/host_checkbox.js

    rmHostId(cid);
+    if ($("#multiple-alert").length){
+      $("#multiple-alert").hide('slow');
+      $("#multiple-alert").data('multiple', false)


$("#multiple-alert") should be cached to reduce lookups

tbrisker · 2017-03-28T14:28:07Z

app/assets/javascripts/host_checkbox.js

+  }
+  else if (!checked) {
+    $("#multiple-alert").hide('slow');
+    $("#multiple-alert").data('multiple', false);


please cache these too

amirfefer · 2017-03-30T12:40:32Z

@tbrisker , thanks, I've added caching to reduce lookups.

tbrisker · 2017-04-02T12:40:26Z

@amirfefer Following our talk, looks like you didn't push an updated version with correct caching for jquery objects?

amirfefer · 2017-04-03T01:40:32Z

@tbrisker , You're right, now you can see caching for jquery objects

tbrisker · 2017-04-03T10:53:31Z

Thanks @amirfefer ! I'd be happy to see all of this code rewritten in es6 or react, could you please open a refactor issue for that?

theforeman-bot added Needs testing Not yet reviewed labels Feb 26, 2017

amirfefer force-pushed the 16043 branch from e14fb67 to 7a4c89d Compare February 26, 2017 15:55

ohadlevy reviewed Feb 26, 2017

View reviewed changes

tbrisker requested changes Feb 27, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 27, 2017

amirfefer force-pushed the 16043 branch from 7a4c89d to 0d67eda Compare March 1, 2017 09:54

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 1, 2017

amirfefer force-pushed the 16043 branch from 0d67eda to 902995a Compare March 1, 2017 15:15

amirfefer force-pushed the 16043 branch from 902995a to 44851aa Compare March 9, 2017 15:54

amirfefer force-pushed the 16043 branch from 44851aa to 8a716a9 Compare March 9, 2017 16:14

amirfefer force-pushed the 16043 branch from 8a716a9 to e4f2dd7 Compare March 9, 2017 23:41

tbrisker requested changes Mar 12, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Mar 12, 2017

amirfefer force-pushed the 16043 branch from e4f2dd7 to dd51f13 Compare March 12, 2017 22:48

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 12, 2017

amirfefer force-pushed the 16043 branch from 563b444 to d263499 Compare March 23, 2017 16:04

tbrisker requested changes Mar 27, 2017

View reviewed changes

amirfefer force-pushed the 16043 branch from d263499 to a166e46 Compare March 27, 2017 21:57

tbrisker requested changes Mar 28, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Mar 28, 2017

amirfefer force-pushed the 16043 branch from a166e46 to 44bdab5 Compare March 30, 2017 12:32

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 30, 2017

This was referenced Apr 1, 2017

Fixes #19121 - clear selected hosts in new search #4426

Closed

Fixes #19122 - clear selected hosts in new search #4427

Merged

tbrisker added Waiting on contributor and removed Needs re-review Needs testing labels Apr 2, 2017

amirfefer force-pushed the 16043 branch from 44bdab5 to de3d107 Compare April 3, 2017 01:08

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Apr 3, 2017

amirfefer force-pushed the 16043 branch from de3d107 to af1eafd Compare April 3, 2017 01:24

Fixes #16043 - add select all hosts option

2bd1882

amirfefer force-pushed the 16043 branch from af1eafd to 2bd1882 Compare April 3, 2017 01:30

tbrisker merged commit 13a7bf0 into theforeman:develop Apr 3, 2017

ares mentioned this pull request May 25, 2017

Fixes #19672 - fix bulk actions for plugins #4557

Merged

		@@ -1,4 +1,6 @@
		<div class="row">
		<%= alert(:class => 'alert-info hide', :close => true, :header => '',

Fixes #16043 - add 'select all hosts' option into hosts page #4338

Fixes #16043 - add 'select all hosts' option into hosts page #4338

Conversation

amirfefer commented Feb 26, 2017

mention-bot commented Feb 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Mar 1, 2017

ohadlevy commented Mar 2, 2017

tbrisker commented Mar 8, 2017

amirfefer commented Mar 9, 2017

Rohoover commented Mar 9, 2017

amirfefer commented Mar 9, 2017 • edited

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Mar 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Mar 12, 2017

amirfefer commented Mar 23, 2017 • edited

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Mar 27, 2017

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Mar 30, 2017

tbrisker commented Apr 2, 2017

amirfefer commented Apr 3, 2017

tbrisker commented Apr 3, 2017

amirfefer commented Mar 9, 2017 •

edited

amirfefer Mar 13, 2017 •

edited

amirfefer commented Mar 23, 2017 •

edited