From dfcf843846228354a0f65b07e0a7fdc8e5f86a5f Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Thu, 7 Sep 2017 17:04:23 -0400
Subject: [PATCH 01/10] Add tag expression to the group editor

---
 app/controllers/ops_controller/ops_rbac.rb    | 73 +++++++++++++++++--
 .../_custom_button_expression.html.haml       | 13 ++--
 .../_group_filter_expression.html.haml        | 14 ++++
 .../_role_enablement_expression.html.haml     |  5 +-
 .../_role_visibility_expression.html.haml     |  3 +-
 .../ops/rbac_group/_customer_tags.html.haml   | 24 ++++--
 config/routes.rb                              |  3 +-
 7 files changed, 113 insertions(+), 22 deletions(-)
 create mode 100644 app/views/layouts/_group_filter_expression.html.haml

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 7d070090ed6..2c03a829f4f 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -69,9 +69,9 @@ def rbac_group_add
   def rbac_group_edit
     assert_privileges("rbac_group_edit")
     case params[:button]
-    when 'cancel'      then rbac_edit_cancel('group')
-    when 'save', 'add' then rbac_edit_save_or_add('group')
-    when 'reset', nil  then rbac_edit_reset(params[:typ], 'group', MiqGroup) # Reset or first time in
+    when 'cancel'            then rbac_edit_cancel('group')
+    when 'save', 'add'       then rbac_edit_save_or_add('group')
+    when 'reset', nil        then rbac_edit_reset(params[:typ], 'group', MiqGroup) # Reset or first time in
     end
   end
 
@@ -442,6 +442,15 @@ def rbac_group_seq_edit
     end
   end
 
+  def rbac_group_filter_expression
+    rbac_group_set_form_vars
+    @changed = session[:changed] = (@edit[:new] != @edit[:current])
+    @expkey = params[:button].to_sym
+    @edit[:filter_expression_table] = exp_build_table_or_nil(@edit[:new][:filter_expression])
+    @in_a_form = true
+    replace_right_cell(:nodetype => x_node)
+  end
+
   def rbac_group_seq_edit_screen
     @edit = {}
     @edit[:new] = {}
@@ -460,6 +469,7 @@ def rbac_group_seq_edit_screen
     session[:changed] = false
   end
 
+
   def move_cols_up
     return unless load_edit("rbac_group_edit__seq", "replace_cell__explorer")
     if !params[:seq_fields] || params[:seq_fields].empty? || params[:seq_fields][0] == ""
@@ -815,7 +825,7 @@ def rbac_build_list(rec_type)
   # AJAX driven routine to check for changes in ANY field on the form
   def rbac_field_changed(rec_type)
     id = params[:id].split('__').first # Get the record id
-    id = from_cid(id) unless id == "new" || id == "seq" # Decompress id if not "new"
+    id = from_cid(id) unless id == "new" || id == "seq" || !cid?(id) # Decompress id if not "new"
     return unless load_edit("rbac_#{rec_type}_edit__#{id}", "replace_cell__explorer")
 
     case rec_type
@@ -849,6 +859,8 @@ def rbac_field_changed(rec_type)
                      :partial => @refresh_partial,
                      :locals  => {:type => "classifications", :action_url => 'rbac_group_field_changed'}) if @refresh_div
 
+        page.replace("customer_tags_div", :partial => "ops/rbac_group/customer_tags") if params[:use_filter_expression].present?
+
         # Only update description field value if ldap group user field was selected
         page << "$('#description').val('#{j_str(@edit[:new][:ldap_groups_user])}');" if params[:ldap_groups_user]
 
@@ -906,7 +918,7 @@ def rbac_get_info
       @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "User"), :name => User.find(from_cid(id)).name}
       rbac_user_get_details(id)
     when "g"
-      @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "MiqGroup"), :name => MiqGroup.find(from_cid(id)).description}
+      @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "MiqGroup"), :name => MiqGroup.find(cid?(id) ? from_cid(id) : id).description}
       @edit = nil
       rbac_group_get_details(id)
     when "ur"
@@ -939,11 +951,14 @@ def rbac_tenant_get_details(id)
   end
 
   def rbac_group_get_details(id)
-    @record = @group = MiqGroup.find_by(:id => from_cid(id))
+    @record = @group = MiqGroup.find_by(:id => cid?(id) ? from_cid(id) : id)
     @belongsto = {}
     @filters = {}
+    @filter_expression = []
+    @use_filter_expression = false
     if @record.present?
       get_tagdata(@group)
+      @use_filter_expression = @group.entitlement[:filter_expression].kind_of?(MiqExpression)
       # Build the belongsto filters hash
       @group.get_belongsto_filters.each do |b| # Go thru the belongsto tags
         bobj = MiqFilter.belongsto2object(b) # Convert to an object
@@ -1143,6 +1158,16 @@ def rbac_group_get_form_vars
       end
     end
 
+    if params[:use_filter_expression]
+      @edit[:new][:use_filter_expression] = params[:use_filter_expression]
+      if params[:use_filter_expression] == 'false'
+        @edit[:new][:use_filter_expression] = false
+        rbac_group_right_tree(@edit[:new][:belongsto].keys)
+      elsif params[:use_filter_expression] == 'true'
+        @edit[:use_filter_expression] = true
+      end
+    end
+
     params[:tree_typ] ? params[:tree_typ] + "_tree" : nil
   end
 
@@ -1153,6 +1178,7 @@ def rbac_group_set_form_vars
     @edit = {
       :new => {
         :filters     => {},
+        :filter_expression => {},
         :belongsto   => {},
         :description => @group.description,
       },
@@ -1195,11 +1221,33 @@ def rbac_group_set_form_vars
     @edit[:projects_tenants].push(["Tenants", all_tenants]) unless all_tenants.blank?
     @edit[:new][:group_tenant] = @group.tenant_id
 
+    rbac_group_filter_expression_vars(:filter_expression, :filter_expression_table)
     @edit[:current] = copy_hash(@edit[:new])
 
     rbac_group_right_tree(@edit[:new][:belongsto].keys)
   end
 
+  def rbac_group_filter_expression_vars(field_expression, field_expression_table)
+    @edit[:new][field_expression] = @group.entitlement[field_expression].kind_of?(MiqExpression) ?  @group.entitlement[field_expression].exp : nil if @group
+    @edit[:new][:use_filter_expression] = true
+    # Populate exp editor fields for the expression column
+    @edit[field_expression] ||= ApplicationController::Filter::Expression.new
+    @edit[field_expression][:expression] = [] # Store exps in an array
+    if @edit[:new][field_expression].blank?
+      @edit[:new][:use_filter_expression] = false
+      @edit[field_expression][:expression] = {"???" => "???"} # Set as new exp element
+      @edit[:new][field_expression] = copy_hash(@edit[field_expression][:expression]) # Copy to new exp
+    else
+      @edit[field_expression][:expression] = copy_hash(@edit[:new][field_expression])
+    end
+    @edit[field_expression_table] = exp_build_table_or_nil(@edit[field_expression][:expression])
+
+    @expkey = field_expression # Set expression key to expression
+    @edit[field_expression].history.reset(@edit[field_expression][:expression])
+    @edit[field_expression][:exp_table] = exp_build_table(@edit[field_expression][:expression])
+    @edit[field_expression][:exp_model] = @group.class # Set model for the exp editor
+  end
+
   # Set group record variables to new values
   def rbac_group_set_record_vars(group)
     role = MiqUserRole.find(@edit[:new][:role])
@@ -1207,6 +1255,8 @@ def rbac_group_set_record_vars(group)
     group.miq_user_role = role
     group.tenant = Tenant.find(@edit[:new][:group_tenant]) if @edit[:new][:group_tenant]
     rbac_group_set_filters(group) # Go set the filters for the group
+    rbac_group_set_filter_expression(group) # Go set the filters for the group
+
   end
 
   # Set filters in the group record from the @edit[:new] hash values
@@ -1221,6 +1271,13 @@ def rbac_group_set_filters(group)
     group.entitlement.set_belongsto_filters(@edit[:new][:belongsto].values) # Set belongs to to hash values
   end
 
+  def rbac_group_set_filter_expression(group)
+    group.entitlement ||= Entitlement.new
+    exp_remove_tokens(@edit[:new][:filter_expression])
+    group.entitlement.filter_expression = @edit[:new][:filter_expression]["???"] ? nil : MiqExpression.new(@edit[:new][:filter_expression])
+    #group.save
+  end
+
   # Need to make arrays by category containing arrays of items so the filtering logic can apply
   # AND between the categories, but OR between the items within a category
   def rbac_group_make_subarrays
@@ -1240,7 +1297,8 @@ def rbac_role_set_form_vars
     vmr = @record.settings.fetch_path(:restrictions, :vms) if @record.settings
     @edit[:new][:vm_restriction] = vmr || :none
     @edit[:new][:features] = rbac_expand_features(@record.feature_identifiers).sort
-
+    @expkey = :filter_expression
+    @edit[:filter_expression_table] = exp_build_table_or_nil(@edit[:new][:filter_expression])
     @edit[:current] = copy_hash(@edit[:new])
 
     @role_features = @record.feature_identifiers.sort
@@ -1364,6 +1422,7 @@ def rbac_role_validate?
   # Validate some of the role fields
   def rbac_group_validate?
     @assigned_filters = [] if @edit[:new][:filters].empty?
+    @filter_expression = [] if @edit[:new][:filter_expression].empty?
     if @edit[:new][:role].nil? || @edit[:new][:role] == ""
       add_flash(_("A User Group must be assigned a Role"), :error)
       return false
diff --git a/app/views/layouts/_custom_button_expression.html.haml b/app/views/layouts/_custom_button_expression.html.haml
index 6d992b21be6..9b1c061a472 100644
--- a/app/views/layouts/_custom_button_expression.html.haml
+++ b/app/views/layouts/_custom_button_expression.html.haml
@@ -14,12 +14,13 @@
       %label.control-label.col-md-2
         = title
       .col-md-8
-        = link_to({:action => "button_update",
-                      :button => expression_type},
-                      "data-miq_sparkle_on" => true,
-                      "data-miq_sparkle_off" => true,
-                      :remote => true,
-                      "data-method" => :post) do
+        = link_to({:action => action,
+                   :id => id,
+                   :button => expression_type},
+                   "data-miq_sparkle_on" => true,
+                   "data-miq_sparkle_off" => true,
+                   :remote => true,
+                   "data-method" => :post) do
           %button.btn.btn-default
             - if @edit[table_key].nil?
               = _("Define Expression")
diff --git a/app/views/layouts/_group_filter_expression.html.haml b/app/views/layouts/_group_filter_expression.html.haml
new file mode 100644
index 00000000000..8399644a65f
--- /dev/null
+++ b/app/views/layouts/_group_filter_expression.html.haml
@@ -0,0 +1,14 @@
+- if @edit.nil?
+  %h3
+    = _('Filter Expression')
+  .form-horizontal.static
+    - if @group && @group.entitlement && @group.entitlement.filter_expression && @group.entitlement.filter_expression.kind_of?(MiqExpression)
+      = h(@group.entitlement.filter_expression.to_human)
+    - else
+      = render :partial => 'layouts/info_msg', :locals => {:message => _("No Filter Expression defined")}
+- else
+  = render :partial => 'layouts/custom_button_expression',
+           :locals => {:expression_type => :filter_expression,
+                       :action          => "rbac_group_edit",
+                       :id              => @edit[:group_id] || 'new',
+                       :title           => _('Expression')}
diff --git a/app/views/layouts/_role_enablement_expression.html.haml b/app/views/layouts/_role_enablement_expression.html.haml
index 25fc9411815..4ee4cfb4626 100644
--- a/app/views/layouts/_role_enablement_expression.html.haml
+++ b/app/views/layouts/_role_enablement_expression.html.haml
@@ -1,3 +1,4 @@
-= render :partial => 'layouts/custom_button_expression', 
+= render :partial => 'layouts/custom_button_expression',
          :locals => {:expression_type => :enablement_expression,
-                     :title => _('Expression')}
+                     :action          => "button_update",
+                     :title           => _('Expression')}
diff --git a/app/views/layouts/_role_visibility_expression.html.haml b/app/views/layouts/_role_visibility_expression.html.haml
index 8613081f1f6..bf52c1b9be0 100644
--- a/app/views/layouts/_role_visibility_expression.html.haml
+++ b/app/views/layouts/_role_visibility_expression.html.haml
@@ -1,3 +1,4 @@
 = render :partial => 'layouts/custom_button_expression',
          :locals => {:expression_type => :visibility_expression,
-                     :title => _('Expression')}
+                     :action          => "button_update",
+                     :title           => _('Expression')}
diff --git a/app/views/ops/rbac_group/_customer_tags.html.haml b/app/views/ops/rbac_group/_customer_tags.html.haml
index 7874a9c9076..06c8244fc6d 100644
--- a/app/views/ops/rbac_group/_customer_tags.html.haml
+++ b/app/views/ops/rbac_group/_customer_tags.html.haml
@@ -1,5 +1,19 @@
-%br
-= _("This user is limited to items with the selected tags.")
-%br
-%br
-= render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})
+#customer_tags_div
+  - url = url_for(:action => 'rbac_group_field_changed', :id => @edit && @edit[:group_id] ? @edit[:group_id] : @record.try(:id) || "new")
+  %br
+  = _("This user is limited to items with the selected tags.")
+  %br
+  %br
+  .form-horizontal
+    .form-group
+      .col-md-4
+        .col-md-8
+          - if @edit
+            = check_box_tag("use_filter_expression", true, @edit[:new][:use_filter_expression], :data => {:on_text => _('Yes'), :off_text => _('No')})
+          :javascript
+            miqInitBootstrapSwitch('use_filter_expression', "#{url}")
+  - if @edit && @edit[:new][:use_filter_expression]
+    = render(:partial => "layouts/group_filter_expression",
+             :locals  => {:id => @edit[:group_id] || "new"})
+  - else
+    = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})
diff --git a/config/routes.rb b/config/routes.rb
index 6a829ebc7fc..f0b36ae0bc4 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -2704,7 +2704,8 @@
         ls_select
         ldap_entry_changed
         ls_delete
-      )
+      ) +
+        exp_post
     },
 
     :orchestration_stack      => {

From 5ccb438e46925522febbfbc20e3a409cadb2d38e Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Wed, 4 Oct 2017 13:55:58 -0400
Subject: [PATCH 02/10] Allow only tags in building the group filter expression

---
 app/controllers/ops_controller/ops_rbac.rb   | 2 +-
 app/views/layouts/exp_atom/_editor.html.haml | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 2c03a829f4f..1fca3b94276 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -1245,7 +1245,7 @@ def rbac_group_filter_expression_vars(field_expression, field_expression_table)
     @expkey = field_expression # Set expression key to expression
     @edit[field_expression].history.reset(@edit[field_expression][:expression])
     @edit[field_expression][:exp_table] = exp_build_table(@edit[field_expression][:expression])
-    @edit[field_expression][:exp_model] = @group.class # Set model for the exp editor
+    @edit[field_expression][:exp_model] = @group.class.to_s # Set model for the exp editor
   end
 
   # Set group record variables to new values
diff --git a/app/views/layouts/exp_atom/_editor.html.haml b/app/views/layouts/exp_atom/_editor.html.haml
index 256fc4adb31..4a95d341801 100644
--- a/app/views/layouts/exp_atom/_editor.html.haml
+++ b/app/views/layouts/exp_atom/_editor.html.haml
@@ -53,6 +53,8 @@
             - opts.push([_('Field'), 'field'])
           - unless @edit[@expkey].tags_for_display_filters.empty?
             - opts.push([_('Tag'), 'tag'])
+        - when 'MiqGroup'
+          - opts += [[_('Tag'), 'tag']]
         - else
           - opts += exptypes
 

From 969176c83a997ca09dce89eb7f0e177ed0233797 Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Wed, 4 Oct 2017 15:59:17 -0400
Subject: [PATCH 03/10] Rename shared filter_expression layout. Fix
 customer_tags form

---
 app/controllers/ops_controller/ops_rbac.rb    | 17 ++++-----
 ...html.haml => _filter_expression.html.haml} |  3 +-
 .../_group_filter_expression.html.haml        |  2 +-
 .../_role_enablement_expression.html.haml     |  2 +-
 .../_role_visibility_expression.html.haml     |  2 +-
 .../ops/rbac_group/_customer_tags.html.haml   | 35 ++++++++++---------
 6 files changed, 32 insertions(+), 29 deletions(-)
 rename app/views/layouts/{_custom_button_expression.html.haml => _filter_expression.html.haml} (92%)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 1fca3b94276..8d4766bcf8e 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -469,7 +469,6 @@ def rbac_group_seq_edit_screen
     session[:changed] = false
   end
 
-
   def move_cols_up
     return unless load_edit("rbac_group_edit__seq", "replace_cell__explorer")
     if !params[:seq_fields] || params[:seq_fields].empty? || params[:seq_fields][0] == ""
@@ -1162,6 +1161,7 @@ def rbac_group_get_form_vars
       @edit[:new][:use_filter_expression] = params[:use_filter_expression]
       if params[:use_filter_expression] == 'false'
         @edit[:new][:use_filter_expression] = false
+        @group = MiqGroup.find_by(:id => @edit[:group_id])
         rbac_group_right_tree(@edit[:new][:belongsto].keys)
       elsif params[:use_filter_expression] == 'true'
         @edit[:use_filter_expression] = true
@@ -1228,7 +1228,11 @@ def rbac_group_set_form_vars
   end
 
   def rbac_group_filter_expression_vars(field_expression, field_expression_table)
-    @edit[:new][field_expression] = @group.entitlement[field_expression].kind_of?(MiqExpression) ?  @group.entitlement[field_expression].exp : nil if @group
+    @edit[:new][field_expression] = if @group && @group.entitlement && @group.entitlement[field_expression].kind_of?(MiqExpression)
+                                      @group.entitlement[field_expression].exp
+                                    else
+                                      nil
+                                    end
     @edit[:new][:use_filter_expression] = true
     # Populate exp editor fields for the expression column
     @edit[field_expression] ||= ApplicationController::Filter::Expression.new
@@ -1256,7 +1260,6 @@ def rbac_group_set_record_vars(group)
     group.tenant = Tenant.find(@edit[:new][:group_tenant]) if @edit[:new][:group_tenant]
     rbac_group_set_filters(group) # Go set the filters for the group
     rbac_group_set_filter_expression(group) # Go set the filters for the group
-
   end
 
   # Set filters in the group record from the @edit[:new] hash values
@@ -1275,7 +1278,6 @@ def rbac_group_set_filter_expression(group)
     group.entitlement ||= Entitlement.new
     exp_remove_tokens(@edit[:new][:filter_expression])
     group.entitlement.filter_expression = @edit[:new][:filter_expression]["???"] ? nil : MiqExpression.new(@edit[:new][:filter_expression])
-    #group.save
   end
 
   # Need to make arrays by category containing arrays of items so the filtering logic can apply
@@ -1297,8 +1299,7 @@ def rbac_role_set_form_vars
     vmr = @record.settings.fetch_path(:restrictions, :vms) if @record.settings
     @edit[:new][:vm_restriction] = vmr || :none
     @edit[:new][:features] = rbac_expand_features(@record.feature_identifiers).sort
-    @expkey = :filter_expression
-    @edit[:filter_expression_table] = exp_build_table_or_nil(@edit[:new][:filter_expression])
+
     @edit[:current] = copy_hash(@edit[:new])
 
     @role_features = @record.feature_identifiers.sort
@@ -1421,8 +1422,8 @@ def rbac_role_validate?
 
   # Validate some of the role fields
   def rbac_group_validate?
-    @assigned_filters = [] if @edit[:new][:filters].empty?
-    @filter_expression = [] if @edit[:new][:filter_expression].empty?
+    @assigned_filters = [] if @edit[:new][:filters].empty? || @edit[:new][:use_filter_expression]
+    @filter_expression = [] if @edit[:new][:filter_expression].empty? || @edit[:new][:use_filter_expression] == false
     if @edit[:new][:role].nil? || @edit[:new][:role] == ""
       add_flash(_("A User Group must be assigned a Role"), :error)
       return false
diff --git a/app/views/layouts/_custom_button_expression.html.haml b/app/views/layouts/_filter_expression.html.haml
similarity index 92%
rename from app/views/layouts/_custom_button_expression.html.haml
rename to app/views/layouts/_filter_expression.html.haml
index 9b1c061a472..6c47b9c7930 100644
--- a/app/views/layouts/_custom_button_expression.html.haml
+++ b/app/views/layouts/_filter_expression.html.haml
@@ -7,7 +7,7 @@
       %label.control-label.col-md-2
         = title
       .col-md-8
-        =_("Choose an element of the expression to edit")
+        = _("Choose an element of the expression to edit")
         = render :partial => 'layouts/exp_editor'
   - else
     .form-group
@@ -15,7 +15,6 @@
         = title
       .col-md-8
         = link_to({:action => action,
-                   :id => id,
                    :button => expression_type},
                    "data-miq_sparkle_on" => true,
                    "data-miq_sparkle_off" => true,
diff --git a/app/views/layouts/_group_filter_expression.html.haml b/app/views/layouts/_group_filter_expression.html.haml
index 8399644a65f..cf73546f120 100644
--- a/app/views/layouts/_group_filter_expression.html.haml
+++ b/app/views/layouts/_group_filter_expression.html.haml
@@ -7,7 +7,7 @@
     - else
       = render :partial => 'layouts/info_msg', :locals => {:message => _("No Filter Expression defined")}
 - else
-  = render :partial => 'layouts/custom_button_expression',
+  = render :partial => 'layouts/filter_expression',
            :locals => {:expression_type => :filter_expression,
                        :action          => "rbac_group_edit",
                        :id              => @edit[:group_id] || 'new',
diff --git a/app/views/layouts/_role_enablement_expression.html.haml b/app/views/layouts/_role_enablement_expression.html.haml
index 4ee4cfb4626..e1580526fb0 100644
--- a/app/views/layouts/_role_enablement_expression.html.haml
+++ b/app/views/layouts/_role_enablement_expression.html.haml
@@ -1,4 +1,4 @@
-= render :partial => 'layouts/custom_button_expression',
+= render :partial => 'layouts/filter_expression',
          :locals => {:expression_type => :enablement_expression,
                      :action          => "button_update",
                      :title           => _('Expression')}
diff --git a/app/views/layouts/_role_visibility_expression.html.haml b/app/views/layouts/_role_visibility_expression.html.haml
index bf52c1b9be0..549ed368cb9 100644
--- a/app/views/layouts/_role_visibility_expression.html.haml
+++ b/app/views/layouts/_role_visibility_expression.html.haml
@@ -1,4 +1,4 @@
-= render :partial => 'layouts/custom_button_expression',
+= render :partial => 'layouts/filter_expression',
          :locals => {:expression_type => :visibility_expression,
                      :action          => "button_update",
                      :title           => _('Expression')}
diff --git a/app/views/ops/rbac_group/_customer_tags.html.haml b/app/views/ops/rbac_group/_customer_tags.html.haml
index 06c8244fc6d..7a1b2c68866 100644
--- a/app/views/ops/rbac_group/_customer_tags.html.haml
+++ b/app/views/ops/rbac_group/_customer_tags.html.haml
@@ -1,19 +1,22 @@
 #customer_tags_div
-  - url = url_for(:action => 'rbac_group_field_changed', :id => @edit && @edit[:group_id] ? @edit[:group_id] : @record.try(:id) || "new")
-  %br
-  = _("This user is limited to items with the selected tags.")
-  %br
-  %br
-  .form-horizontal
-    .form-group
-      .col-md-4
-        .col-md-8
-          - if @edit
-            = check_box_tag("use_filter_expression", true, @edit[:new][:use_filter_expression], :data => {:on_text => _('Yes'), :off_text => _('No')})
-          :javascript
-            miqInitBootstrapSwitch('use_filter_expression', "#{url}")
-  - if @edit && @edit[:new][:use_filter_expression]
-    = render(:partial => "layouts/group_filter_expression",
-             :locals  => {:id => @edit[:group_id] || "new"})
+  - if @edit
+    - rec_id = @edit && @edit[:group_id] ? @edit[:group_id] : "new"
+    - url = url_for(:action => 'rbac_group_field_changed', :id => rec_id)
+    %br
+    = _("This user is limited to items with the selected tags.")
+    %br
+    %br
+    .form-horizontal
+      .form-group
+        .col-md-4
+          .col-md-8
+            - if @edit
+              = check_box_tag("use_filter_expression", true, @edit[:new][:use_filter_expression], :data => {:on_text => _('Yes'), :off_text => _('No')})
+            :javascript
+              miqInitBootstrapSwitch('use_filter_expression', "#{url}")
+    - if @edit[:new][:use_filter_expression]
+      = render(:partial => "layouts/group_filter_expression")
+    - else
+      = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})
   - else
     = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})

From e77f14fbaeab2dcd511bd3d4623b0712dc153ee8 Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Fri, 6 Oct 2017 14:56:06 -0400
Subject: [PATCH 04/10] Display group expression in form

Display group expression in form
---
 app/views/ops/rbac_group/_customer_tags.html.haml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/views/ops/rbac_group/_customer_tags.html.haml b/app/views/ops/rbac_group/_customer_tags.html.haml
index 7a1b2c68866..9c0aa49d893 100644
--- a/app/views/ops/rbac_group/_customer_tags.html.haml
+++ b/app/views/ops/rbac_group/_customer_tags.html.haml
@@ -18,5 +18,7 @@
       = render(:partial => "layouts/group_filter_expression")
     - else
       = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})
+  - elsif @use_filter_expression
+    = render(:partial => "layouts/group_filter_expression")
   - else
     = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})

From 04da5a5bae1157faf246b10cafbf68d8b0f9dc6d Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Sun, 8 Oct 2017 11:00:38 -0400
Subject: [PATCH 05/10] Save only one type of group tag filters -

---
 app/controllers/ops_controller/ops_rbac.rb | 32 +++++++++++++---------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 8d4766bcf8e..58a469a19d4 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -1258,28 +1258,34 @@ def rbac_group_set_record_vars(group)
     group.description = @edit[:new][:description]
     group.miq_user_role = role
     group.tenant = Tenant.find(@edit[:new][:group_tenant]) if @edit[:new][:group_tenant]
+    if @edit[:new][:use_filter_expression]
+      @edit[:new][:filters].clear
+    else
+      exp_remove_tokens(@edit[:new][:filter_expression])
+      @edit[:new][:filter_expression] = nil
+    end
+
     rbac_group_set_filters(group) # Go set the filters for the group
-    rbac_group_set_filter_expression(group) # Go set the filters for the group
   end
 
   # Set filters in the group record from the @edit[:new] hash values
   def rbac_group_set_filters(group)
-    @set_filter_values = []
-    @edit[:new][:filters].each do |_key, value|
-      @set_filter_values.push(value)
-    end
-    rbac_group_make_subarrays # Need to have category arrays of item arrays for and/or logic
     group.entitlement ||= Entitlement.new
-    group.entitlement.set_managed_filters(@set_filter_values)
+    if @edit[:new][:use_filter_expression]
+      group.entitlement.set_managed_filters(nil) if group.entitlement.get_managed_filters.present?
+      group.entitlement.filter_expression = @edit[:new][:filter_expression]["???"] ? nil : MiqExpression.new(@edit[:new][:filter_expression])
+    else
+      group.entitlement.filter_expression = nil if group.entitlement.filter_expression
+      @set_filter_values = []
+      @edit[:new][:filters].each do |_key, value|
+        @set_filter_values.push(value)
+      end
+      rbac_group_make_subarrays # Need to have category arrays of item arrays for and/or logic
+      group.entitlement.set_managed_filters(@set_filter_values)
+    end
     group.entitlement.set_belongsto_filters(@edit[:new][:belongsto].values) # Set belongs to to hash values
   end
 
-  def rbac_group_set_filter_expression(group)
-    group.entitlement ||= Entitlement.new
-    exp_remove_tokens(@edit[:new][:filter_expression])
-    group.entitlement.filter_expression = @edit[:new][:filter_expression]["???"] ? nil : MiqExpression.new(@edit[:new][:filter_expression])
-  end
-
   # Need to make arrays by category containing arrays of items so the filtering logic can apply
   # AND between the categories, but OR between the items within a category
   def rbac_group_make_subarrays

From 43c070d54218ef575ae913974383eb7268de84c6 Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Mon, 9 Oct 2017 09:35:32 -0400
Subject: [PATCH 06/10] Tag expression spec for the group editor

---
 .../ops_controller/ops_rbac_spec.rb           | 40 +++++++++++++------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/spec/controllers/ops_controller/ops_rbac_spec.rb b/spec/controllers/ops_controller/ops_rbac_spec.rb
index 82238a1a891..2f8de91a997 100644
--- a/spec/controllers/ops_controller/ops_rbac_spec.rb
+++ b/spec/controllers/ops_controller/ops_rbac_spec.rb
@@ -389,22 +389,38 @@
       MiqUserRole.seed
       MiqGroup.seed
       MiqRegion.seed
+
       stub_user(:features => :all)
+      @group = FactoryGirl.create(:miq_group)
+      @role = MiqUserRole.find_by_name("EvmRole-operator")
+      @exp = MiqExpression.new("=" => {:field => "name", :value => "Test"}, :token => 1)
     end
 
-    it "does not display tenant default groups in Edit Sequence" do
-      tg = FactoryGirl.create(:tenant).default_miq_group
-      g  = FactoryGirl.create(:miq_group)
+    it "saves the filters when use_filter_expression is false" do
+      @group.entitlement = Entitlement.create!
+      controller.instance_variable_set(:@edit, {:new => {:use_filter_expression => false,
+                                                         :name                  => 'Name',
+                                                         :description           => "Test",
+                                                         :role                  => @role.id,
+                                                         :filter_expression     => @exp.exp,
+                                                         :belongsto             => {},
+                                                         :filters               => {'managed/application/abrt' => '/managed/application/abrt'}}})
+      controller.send(:rbac_group_set_record_vars, @group)
+      expect(@group.entitlement.filter_expression).to be_nil
+      expect(@group.entitlement.get_managed_filters).to match([["/managed/application/abrt"]])
+    end
 
-      expect(MiqGroup.tenant_groups).to include(tg)
-      expect(MiqGroup.tenant_groups).not_to include(g)
-      session[:sandboxes] = {"ops" => {:active_tree => :rbac_tree}}
-      allow(controller).to receive(:replace_right_cell)
-      controller.send(:rbac_group_seq_edit)
-      expect(response.status).to eq(200)
-      edit = controller.instance_variable_get(:@edit)
-      expect(edit[:current][:ldap_groups].find { |lg| lg.group_type == 'tenant' }).to be(nil)
-      expect(edit[:current][:ldap_groups].find { |lg| lg.group_type == 'user' }).not_to be(nil)
+    it "saves the filter_expression when use_filter_expression true" do
+      controller.instance_variable_set(:@edit, {:new => {:use_filter_expression => true,
+                                                         :name                  => 'Name',
+                                                         :description           => "Test",
+                                                         :role                  => @role.id,
+                                                         :filter_expression     => @exp.exp,
+                                                         :belongsto             => {},
+                                                         :filters               => {'managed/application/abrt' => '/managed/application/abrt'}}})
+      controller.send(:rbac_group_set_record_vars, @group)
+      expect(@group.entitlement.get_managed_filters).to eq([])
+      expect(@group.entitlement.filter_expression.exp).to match(@exp.exp)
     end
   end
 

From b6ea8919c354e980ec5cb24e2cc353458ef5159d Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Mon, 9 Oct 2017 09:47:49 -0400
Subject: [PATCH 07/10] Rubocop style fixes

---
 app/controllers/ops_controller/ops_rbac.rb    | 10 +++----
 .../ops_controller/ops_rbac_spec.rb           | 30 +++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 58a469a19d4..bd07c22348d 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -1228,11 +1228,11 @@ def rbac_group_set_form_vars
   end
 
   def rbac_group_filter_expression_vars(field_expression, field_expression_table)
-    @edit[:new][field_expression] = if @group && @group.entitlement && @group.entitlement[field_expression].kind_of?(MiqExpression)
-                                      @group.entitlement[field_expression].exp
-                                    else
-                                      nil
-                                    end
+    if @group && @group.entitlement && @group.entitlement[field_expression].kind_of?(MiqExpression)
+      @edit[:new][field_expression] = @group.entitlement[field_expression].exp
+    else
+      @edit[:new][field_expression] = nil
+    end
     @edit[:new][:use_filter_expression] = true
     # Populate exp editor fields for the expression column
     @edit[field_expression] ||= ApplicationController::Filter::Expression.new
diff --git a/spec/controllers/ops_controller/ops_rbac_spec.rb b/spec/controllers/ops_controller/ops_rbac_spec.rb
index 2f8de91a997..4e0156ba744 100644
--- a/spec/controllers/ops_controller/ops_rbac_spec.rb
+++ b/spec/controllers/ops_controller/ops_rbac_spec.rb
@@ -392,32 +392,32 @@
 
       stub_user(:features => :all)
       @group = FactoryGirl.create(:miq_group)
-      @role = MiqUserRole.find_by_name("EvmRole-operator")
+      @role = MiqUserRole.find_by(:name, "EvmRole-operator")
       @exp = MiqExpression.new("=" => {:field => "name", :value => "Test"}, :token => 1)
     end
 
     it "saves the filters when use_filter_expression is false" do
       @group.entitlement = Entitlement.create!
-      controller.instance_variable_set(:@edit, {:new => {:use_filter_expression => false,
-                                                         :name                  => 'Name',
-                                                         :description           => "Test",
-                                                         :role                  => @role.id,
-                                                         :filter_expression     => @exp.exp,
-                                                         :belongsto             => {},
-                                                         :filters               => {'managed/application/abrt' => '/managed/application/abrt'}}})
+      controller.instance_variable_set(:@edit, :new => {:use_filter_expression => false,
+                                                        :name                  => 'Name',
+                                                        :description           => "Test",
+                                                        :role                  => @role.id,
+                                                        :filter_expression     => @exp.exp,
+                                                        :belongsto             => {},
+                                                        :filters               => {'managed/application/abrt' => '/managed/application/abrt'}})
       controller.send(:rbac_group_set_record_vars, @group)
       expect(@group.entitlement.filter_expression).to be_nil
       expect(@group.entitlement.get_managed_filters).to match([["/managed/application/abrt"]])
     end
 
     it "saves the filter_expression when use_filter_expression true" do
-      controller.instance_variable_set(:@edit, {:new => {:use_filter_expression => true,
-                                                         :name                  => 'Name',
-                                                         :description           => "Test",
-                                                         :role                  => @role.id,
-                                                         :filter_expression     => @exp.exp,
-                                                         :belongsto             => {},
-                                                         :filters               => {'managed/application/abrt' => '/managed/application/abrt'}}})
+      controller.instance_variable_set(:@edit, :new => {:use_filter_expression => true,
+                                                        :name                  => 'Name',
+                                                        :description           => "Test",
+                                                        :role                  => @role.id,
+                                                        :filter_expression     => @exp.exp,
+                                                        :belongsto             => {},
+                                                        :filters               => {'managed/application/abrt' => '/managed/application/abrt'}})
       controller.send(:rbac_group_set_record_vars, @group)
       expect(@group.entitlement.get_managed_filters).to eq([])
       expect(@group.entitlement.filter_expression.exp).to match(@exp.exp)

From 6c54188a652beb3ae9a11f6b526aa5635c4eb774 Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Mon, 9 Oct 2017 12:59:33 -0400
Subject: [PATCH 08/10] Replace the checkbox for the filter expression with a
 dropdown select

---
 .../ops/rbac_group/_customer_tags.html.haml   | 22 ++++++++++++-------
 .../ops_controller/ops_rbac_spec.rb           |  2 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/app/views/ops/rbac_group/_customer_tags.html.haml b/app/views/ops/rbac_group/_customer_tags.html.haml
index 9c0aa49d893..aab8ecb611f 100644
--- a/app/views/ops/rbac_group/_customer_tags.html.haml
+++ b/app/views/ops/rbac_group/_customer_tags.html.haml
@@ -3,17 +3,20 @@
     - rec_id = @edit && @edit[:group_id] ? @edit[:group_id] : "new"
     - url = url_for(:action => 'rbac_group_field_changed', :id => rec_id)
     %br
-    = _("This user is limited to items with the selected tags.")
-    %br
-    %br
     .form-horizontal
       .form-group
-        .col-md-4
-          .col-md-8
-            - if @edit
-              = check_box_tag("use_filter_expression", true, @edit[:new][:use_filter_expression], :data => {:on_text => _('Yes'), :off_text => _('No')})
+        %label.col-md-2.control-label
+          = _("This user is limited to ")
+        .col-md-8
+          - if @edit
+            = select_tag('use_filter_expression',
+                      options_for_select([[_("Specific Tags"), false],
+                                          [_("Tags Based On Expression"), true]],
+                      @edit[:new][:use_filter_expression]),
+                      :class    => "selectpicker")
             :javascript
-              miqInitBootstrapSwitch('use_filter_expression', "#{url}")
+               miqInitSelectPicker();
+               miqSelectPickerEvent('use_filter_expression', "#{url}")
     - if @edit[:new][:use_filter_expression]
       = render(:partial => "layouts/group_filter_expression")
     - else
@@ -21,4 +24,7 @@
   - elsif @use_filter_expression
     = render(:partial => "layouts/group_filter_expression")
   - else
+    = _("This user is limited to items with the selected tags.")
+    %br
+    %br
     = render(:partial => 'shared/tree', :locals => {:tree => @tags_tree, :name => @tags_tree.name})
diff --git a/spec/controllers/ops_controller/ops_rbac_spec.rb b/spec/controllers/ops_controller/ops_rbac_spec.rb
index 4e0156ba744..a076019200c 100644
--- a/spec/controllers/ops_controller/ops_rbac_spec.rb
+++ b/spec/controllers/ops_controller/ops_rbac_spec.rb
@@ -392,7 +392,7 @@
 
       stub_user(:features => :all)
       @group = FactoryGirl.create(:miq_group)
-      @role = MiqUserRole.find_by(:name, "EvmRole-operator")
+      @role = MiqUserRole.find_by(:name => "EvmRole-operator")
       @exp = MiqExpression.new("=" => {:field => "name", :value => "Test"}, :token => 1)
     end
 

From 0eccac3bd0b2249f125a17d4dbdc6b1a771e63ce Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Mon, 9 Oct 2017 16:49:27 -0400
Subject: [PATCH 09/10] Compact unless condition

---
 app/controllers/ops_controller/ops_rbac.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index bd07c22348d..7ad35f7bb3b 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -824,7 +824,7 @@ def rbac_build_list(rec_type)
   # AJAX driven routine to check for changes in ANY field on the form
   def rbac_field_changed(rec_type)
     id = params[:id].split('__').first # Get the record id
-    id = from_cid(id) unless id == "new" || id == "seq" || !cid?(id) # Decompress id if not "new"
+    id = from_cid(id) unless %w(new seq).include?(id) || !cid?(id)
     return unless load_edit("rbac_#{rec_type}_edit__#{id}", "replace_cell__explorer")
 
     case rec_type

From 84271bec5f37e8ea73854e9d5a69972824152706 Mon Sep 17 00:00:00 2001
From: lgalis <lgalis@redhat.com>
Date: Tue, 10 Oct 2017 11:36:28 -0400
Subject: [PATCH 10/10] Use from_cid only in find

---
 app/controllers/ops_controller/ops_rbac.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb
index 7ad35f7bb3b..ceaebc62224 100644
--- a/app/controllers/ops_controller/ops_rbac.rb
+++ b/app/controllers/ops_controller/ops_rbac.rb
@@ -824,7 +824,7 @@ def rbac_build_list(rec_type)
   # AJAX driven routine to check for changes in ANY field on the form
   def rbac_field_changed(rec_type)
     id = params[:id].split('__').first # Get the record id
-    id = from_cid(id) unless %w(new seq).include?(id) || !cid?(id)
+    id = from_cid(id) unless %w(new seq).include?(id)
     return unless load_edit("rbac_#{rec_type}_edit__#{id}", "replace_cell__explorer")
 
     case rec_type
@@ -917,7 +917,7 @@ def rbac_get_info
       @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "User"), :name => User.find(from_cid(id)).name}
       rbac_user_get_details(id)
     when "g"
-      @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "MiqGroup"), :name => MiqGroup.find(cid?(id) ? from_cid(id) : id).description}
+      @right_cell_text = _("%{model} \"%{name}\"") % {:model => ui_lookup(:model => "MiqGroup"), :name => MiqGroup.find(from_cid(id)).description}
       @edit = nil
       rbac_group_get_details(id)
     when "ur"
@@ -950,7 +950,7 @@ def rbac_tenant_get_details(id)
   end
 
   def rbac_group_get_details(id)
-    @record = @group = MiqGroup.find_by(:id => cid?(id) ? from_cid(id) : id)
+    @record = @group = MiqGroup.find_by(:id => from_cid(id))
     @belongsto = {}
     @filters = {}
     @filter_expression = []