PR Review updates - preferring inline code and moved specs to respect…

…ive servers_spec.rb

app/controllers/api/base_controller.rb

@@ -117,7 +117,11 @@ def settings
       case @req.method
       when :patch
         raise ForbiddenError, "You are not authorized to edit settings." unless super_admin?
-        resource_update_settings(resource, @req.json_body)
+        begin
+          resource_update_settings(resource, @req.json_body)
+        rescue VMDB::Config::Validator::Error => err
+          raise BadRequestError, "Settings validation failed - #{err}"
+        end
       when :delete
         raise ForbiddenError, "You are not authorized to remove settings." unless super_admin?
         resource.remove_settings_path_for_resource(*@req.json_body)
@@ -142,12 +146,6 @@ def resource_settings(resource)
       end
     end
 
-    def resource_update_settings(resource, updated_settings)
-      resource.add_settings_for_resource(updated_settings)
-    rescue VMDB::Config::Validator::Error => err
-      raise BadRequestError, "Settings validation failed - #{err}"
-    end
-
     def current_user
       @current_user ||= User.current_user
     end

spec/requests/servers_spec.rb

@@ -76,6 +76,22 @@
       expect(response).to have_http_status(:unauthorized)
     end
 
+    it "returns a bad_request to an update if the settings validation failed" do
+      api_basic_authorize(:user => super_admin.userid, :password => super_admin.password)
+
+      patch(api_server_settings_url(nil, server), :params => {:authentication => {:mode => "bogus_auth_mode"}})
+
+      expected = {
+        'error' => a_hash_including(
+          'kind'    => 'bad_request',
+          'message' => a_string_including('Settings validation failed - ')
+        )
+      }
+
+      expect(response).to have_http_status(:bad_request)
+      expect(response.parsed_body).to include(expected)
+    end
+
     context "with an existing settings change" do
       before do
         server.add_settings_for_resource("api" => {"authentication_timeout" => "7331.minutes"})

spec/requests/settings_spec.rb

@@ -3,25 +3,6 @@
 #
 describe "Settings API" do
   let(:api_settings) { Api::ApiConfig.collections[:settings][:categories] }
-  let(:server) { FactoryGirl.create(:miq_server) }
-  let(:super_admin) { FactoryGirl.create(:user, :role => 'super_administrator', :userid => 'alice', :password => 'alicepassword') }
-
-  context "Settings Update" do
-    it "updates to settings return a BadRequestError upon failed validation" do
-      api_basic_authorize(:user => super_admin.userid, :password => super_admin.password)
-
-      patch(api_server_settings_url(nil, server), :params => {:authentication => {:mode => "bogus_auth_mode"}})
-
-      expected = {
-        'error' => a_hash_including(
-          'kind'    => 'bad_request',
-          'message' => a_string_including('Settings validation failed - ')
-        )
-      }
-      expect(response).to have_http_status(:bad_request)
-      expect(response.parsed_body).to include(expected)
-    end
-  end
 
   context "Settings Queries" do
     it "tests queries of all exposed settings" do