Merge pull request #6169 from hstastna/Remove_bad_ops_rbac

Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role

app/controllers/ops_controller/ops_rbac.rb

@@ -670,6 +670,7 @@ def rbac_edit_save_or_add(what, rbac_suffix = what)
     when :group then
       record = @edit[:group_id] ? MiqGroup.find_by(:id => @edit[:group_id]) : MiqGroup.new
       validated = rbac_group_validate?
+      rbac_group_set_record_description_role(record) # Set new Description, Role and Tenant for a new Group
       rbac_group_set_record_vars(record) if validated
     when :role  then
       record = @edit[:role_id] ? MiqUserRole.find_by(:id => @edit[:role_id]) : MiqUserRole.new
@@ -756,17 +757,7 @@ def rbac_field_changed(rec_type)
 
     @edit[:new][:group] = rbac_user_get_group_ids.map(&:to_i) if rec_type == "user"
 
-    bad = case rec_type
-          when 'group'
-            @edit[:new].values_at(:role, :group_tenant, :description).any?(&:blank?)
-          when 'role'
-            @edit[:new][:name].blank?
-          else
-            false
-          end
-
-    # We need to consider also bad variable, for proper response of Add button
-    session[:changed] = changed = @edit[:new] != @edit[:current] && !bad
+    session[:changed] = changed = @edit[:new] != @edit[:current]
 
     render :update do |page|
       page << javascript_prologue
@@ -775,7 +766,6 @@ def rbac_field_changed(rec_type)
           page.replace("flash_msg_div", :partial => "layouts/flash_msg") if @refresh_div == "column_lists"
           page.replace(@refresh_div, :partial => @refresh_partial)
         end
-        bad = false
       else
         # only do following for user (adding/editing a user)
         if x_node.split("-").first == "u" || x_node == "xx-u"
@@ -1227,10 +1217,6 @@ def rbac_group_filter_expression_vars(field_expression, field_expression_table)
 
   # Set group record variables to new values
   def rbac_group_set_record_vars(group)
-    role = MiqUserRole.find(@edit[:new][:role])
-    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
@@ -1241,6 +1227,13 @@ def rbac_group_set_record_vars(group)
     rbac_group_set_filters(group) # Go set the filters for the group
   end
 
+  # Set group record variables such as Description, Role and Tenant to new values
+  def rbac_group_set_record_description_role(group)
+    group.description = @edit[:new][:description]
+    group.miq_user_role = MiqUserRole.find(@edit[:new][:role]) if @edit[:new][:role]
+    group.tenant = Tenant.find(@edit[:new][:group_tenant]) if @edit[:new][:group_tenant]
+  end
+
   # Set filters in the group record from the @edit[:new] hash values
   def rbac_group_set_filters(group)
     group.entitlement ||= Entitlement.new
@@ -1411,7 +1404,7 @@ def rbac_group_validate?
     @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)
+      add_flash(_("A Role must be assigned to this Group"), :error)
       return false
     end
     true

spec/controllers/ops_controller/ops_rbac_spec.rb

@@ -727,13 +727,13 @@
         let(:params) { {:id => "new", :group_tenant => tenant.id.to_s} }
         let(:edit) { {:new => {:role => 1}} }
 
-        it 'sets session[:changed] to false while filling in role and tenant' do
+        it 'sets session[:changed] to true while filling in role and tenant' do
           controller.send(:rbac_field_changed, rec_type)
-          expect(controller.session[:changed]).to be(false)
+          expect(controller.session[:changed]).to be(true)
         end
 
-        context 'filling in all the required info' do
-          let(:edit) { {:new => {:role => 1, :description => 'new_group'}} }
+        context 'filling in description' do
+          let(:edit) { {:new => {:description => 'new_group'}} }
 
           it 'sets session[:changed] to true' do
             controller.send(:rbac_field_changed, rec_type)
@@ -806,4 +806,23 @@
       end
     end
   end
+
+  describe '#rbac_edit_save_or_add' do
+    context 'adding new Group' do
+      before do
+        allow(controller).to receive(:load_edit).and_return(true)
+        allow(controller).to receive(:render_flash)
+        controller.instance_variable_set(:@edit, :new => {:description           => 'Description',
+                                                          :filters               => {},
+                                                          :filter_expression     => {'???' => '???'},
+                                                          :use_filter_expression => false})
+        controller.params = {:button => 'add'}
+      end
+
+      it 'sets @flash_array properly if user forgot to choose a Role for a Group' do
+        controller.send(:rbac_edit_save_or_add, 'group')
+        expect(controller.instance_variable_get(:@flash_array)).to eq([{:message => 'A Role must be assigned to this Group', :level => :error}])
+      end
+    end
+  end
 end