Incorporate review comments

4 files changed, 41 insertions, 44 deletions

diff --git a/app/controllers/projects/settings/operations_controller.rb b/app/controllers/projects/settings/operations_controller.rb
index b8c0aaf9339..1ba0ff8be7b 100644
--- a/app/controllers/projects/settings/operations_controller.rb
+++ b/app/controllers/projects/settings/operations_controller.rb
@@ -30,14 +30,17 @@ module Projects
 
       def render_update_json_response(result)
         if result[:status] == :success
-          render json:
-            result.slice(:status).merge({
+          render json: {
+            status: result[:status],
             message: _('Your changes have been saved')
-          })
+          }
         else
           render(
             status: result[:http_status] || :bad_request,
-            json: result.slice(:status, :message)
+            json: {
+              status: result[:status],
+              message: result[:message]
+            }
           )
         end
       end
diff --git a/app/services/projects/operations/update_service.rb b/app/services/projects/operations/update_service.rb
index dbcbbcf639d..aedf79c86d7 100644
--- a/app/services/projects/operations/update_service.rb
+++ b/app/services/projects/operations/update_service.rb
@@ -12,35 +12,28 @@ module Projects
       private
 
       def project_update_params
-        attribs = params.slice(:error_tracking_setting_attributes)
-
-        if attribs[:error_tracking_setting_attributes].present?
-          attribs[:error_tracking_setting_attributes] =
-            construct_error_tracking_setting_params(attribs[:error_tracking_setting_attributes])
-        end
-
-        attribs
+        error_tracking_params
       end
 
-      def construct_error_tracking_setting_params(settings)
-        settings[:api_url] = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from(
+      def error_tracking_params
+        settings = params[:error_tracking_setting_attributes]
+        return {} if settings.blank?
+
+        api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from(
           api_host: settings[:api_host],
           project_slug: settings.dig(:project, :slug),
           organization_slug: settings.dig(:project, :organization_slug)
         )
 
-        settings[:project_name] = settings.dig(:project, :name).presence
-        settings[:organization_name] = settings.dig(:project, :organization_name).presence
-
-        settings[:token] = settings[:token].presence
-
-        settings.slice(
-          :api_url,
-          :token,
-          :enabled,
-          :project_name,
-          :organization_name
-        )
+        {
+          error_tracking_setting_attributes: {
+            api_url: api_url,
+            token: settings[:token],
+            enabled: settings[:enabled],
+            project_name: settings.dig(:project, :name),
+            organization_name: settings.dig(:project, :organization_name)
+          }
+        }
       end
     end
   end
diff --git a/spec/controllers/projects/settings/operations_controller_spec.rb b/spec/controllers/projects/settings/operations_controller_spec.rb
index 60b79597a00..e77f2d08d1f 100644
--- a/spec/controllers/projects/settings/operations_controller_spec.rb
+++ b/spec/controllers/projects/settings/operations_controller_spec.rb
@@ -91,17 +91,18 @@ describe Projects::Settings::OperationsController do
       end
 
       context 'format json' do
-        let(:body) { Gitlab::Utils.deep_indifferent_access(JSON.parse(response.body)) }
-
         context 'when update succeeds' do
           before do
             stub_operations_update_service_returning(status: :success)
           end
 
           it 'returns success status' do
-            patch :update, params: project_params(project, error_tracking_params, format: :json)
+            patch :update, params: project_params(project, error_tracking_params), format: :json
 
-            expect(body).to eq('status' => 'success', 'message' => 'Your changes have been saved')
+            expect(json_response).to eq(
+              'status' => 'success',
+              'message' => _('Your changes have been saved')
+            )
           end
         end
 
@@ -111,10 +112,10 @@ describe Projects::Settings::OperationsController do
           end
 
           it 'returns error' do
-            patch :update, params: project_params(project, error_tracking_params, format: :json)
+            patch :update, params: project_params(project, error_tracking_params), format: :json
 
             expect(response).to have_gitlab_http_status(:bad_request)
-            expect(body[:message]).not_to be_nil
+            expect(json_response['message']).not_to be_nil
           end
         end
       end
@@ -157,11 +158,11 @@ describe Projects::Settings::OperationsController do
 
   private
 
-  def project_params(project, project_params = {}, other_params = {})
+  def project_params(project, project_params = {})
     {
       namespace_id: project.namespace,
       project_id: project,
       project: project_params
-    }.merge(other_params)
+    }
   end
 end
diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb
index a26def4b258..037345a21e9 100644
--- a/spec/services/projects/operations/update_service_spec.rb
+++ b/spec/services/projects/operations/update_service_spec.rb
@@ -14,7 +14,7 @@ describe Projects::Operations::UpdateService do
     context 'error tracking' do
       context 'with existing error tracking setting' do
         let(:params) do
-          ActionController::Parameters.new({
+          {
             error_tracking_setting_attributes: {
               enabled: false,
               api_host: 'http://gitlab.com/',
@@ -26,7 +26,7 @@ describe Projects::Operations::UpdateService do
                 organization_name: 'Org'
               }
             }
-          }).permit!
+          }
         end
 
         before do
@@ -40,8 +40,8 @@ describe Projects::Operations::UpdateService do
           expect(project.error_tracking_setting).not_to be_enabled
           expect(project.error_tracking_setting.api_url).to eq('http://gitlab.com/api/0/projects/org/project/')
           expect(project.error_tracking_setting.token).to eq('token')
-          expect(project.error_tracking_setting.read_attribute(:project_name)).to eq('Project')
-          expect(project.error_tracking_setting.read_attribute(:organization_name)).to eq('Org')
+          expect(project.error_tracking_setting[:project_name]).to eq('Project')
+          expect(project.error_tracking_setting[:organization_name]).to eq('Org')
         end
 
         context 'disable error tracking' do
@@ -57,15 +57,15 @@ describe Projects::Operations::UpdateService do
             expect(project.error_tracking_setting).not_to be_enabled
             expect(project.error_tracking_setting.api_url).to be_nil
             expect(project.error_tracking_setting.token).to eq('token')
-            expect(project.error_tracking_setting.read_attribute(:project_name)).to eq('Project')
-            expect(project.error_tracking_setting.read_attribute(:organization_name)).to eq('Org')
+            expect(project.error_tracking_setting[:project_name]).to eq('Project')
+            expect(project.error_tracking_setting[:organization_name]).to eq('Org')
           end
         end
       end
 
       context 'without an existing error tracking setting' do
         let(:params) do
-          ActionController::Parameters.new({
+          {
             error_tracking_setting_attributes: {
               enabled: true,
               api_host: 'http://gitlab.com/',
@@ -77,7 +77,7 @@ describe Projects::Operations::UpdateService do
                 organization_name: 'Org'
               }
             }
-          }).permit!
+          }
         end
 
         it 'creates a setting' do
@@ -86,8 +86,8 @@ describe Projects::Operations::UpdateService do
           expect(project.error_tracking_setting).to be_enabled
           expect(project.error_tracking_setting.api_url).to eq('http://gitlab.com/api/0/projects/org/project/')
           expect(project.error_tracking_setting.token).to eq('token')
-          expect(project.error_tracking_setting.read_attribute(:project_name)).to eq('Project')
-          expect(project.error_tracking_setting.read_attribute(:organization_name)).to eq('Org')
+          expect(project.error_tracking_setting[:project_name]).to eq('Project')
+          expect(project.error_tracking_setting[:organization_name]).to eq('Org')
         end
       end