From 367babec773437f6ab167bb0c4c085661703eb8d Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 5 Feb 2019 21:02:19 +0530 Subject: Incorporate review comments --- .../projects/settings/operations_controller.rb | 11 ++++--- app/services/projects/operations/update_service.rb | 37 +++++++++------------- .../settings/operations_controller_spec.rb | 17 +++++----- .../projects/operations/update_service_spec.rb | 20 ++++++------ 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 -- cgit v1.2.1