From e139d7af7923226bc39d3e42c200d5237c952e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 26 Feb 2018 18:21:41 +0100 Subject: Fix a performance/memory issue in lib/api/services.rb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/api/services.rb | 84 ++++++++++++++++++++++++++--------------------------- 1 file changed, 42 insertions(+), 42 deletions(-) (limited to 'lib/api') diff --git a/lib/api/services.rb b/lib/api/services.rb index 51e33e2c686..c5fb5db724f 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -1,6 +1,7 @@ +# frozen_string_literal: true module API class Services < Grape::API - chat_notification_settings = [ + CHAT_NOTIFICATION_SETTINGS = [ { required: true, name: :webhook, @@ -19,9 +20,9 @@ module API type: String, desc: 'The default chat channel' } - ] + ].freeze - chat_notification_flags = [ + CHAT_NOTIFICATION_FLAGS = [ { required: false, name: :notify_only_broken_pipelines, @@ -34,9 +35,9 @@ module API type: Boolean, desc: 'Send notifications only for the default branch' } - ] + ].freeze - chat_notification_channels = [ + CHAT_NOTIFICATION_CHANNELS = [ { required: false, name: :push_channel, @@ -85,9 +86,9 @@ module API type: String, desc: 'The name of the channel to receive wiki_page_events notifications' } - ] + ].freeze - chat_notification_events = [ + CHAT_NOTIFICATION_EVENTS = [ { required: false, name: :push_events, @@ -136,9 +137,9 @@ module API type: Boolean, desc: 'Enable notifications for wiki_page_events' } - ] + ].freeze - services = { + SERVICES = { 'asana' => [ { required: true, @@ -627,10 +628,10 @@ module API } ], 'slack' => [ - chat_notification_settings, - chat_notification_flags, - chat_notification_channels, - chat_notification_events + CHAT_NOTIFICATION_SETTINGS, + CHAT_NOTIFICATION_FLAGS, + CHAT_NOTIFICATION_CHANNELS, + CHAT_NOTIFICATION_EVENTS ].flatten, 'microsoft-teams' => [ { @@ -641,10 +642,10 @@ module API } ], 'mattermost' => [ - chat_notification_settings, - chat_notification_flags, - chat_notification_channels, - chat_notification_events + CHAT_NOTIFICATION_SETTINGS, + CHAT_NOTIFICATION_FLAGS, + CHAT_NOTIFICATION_CHANNELS, + CHAT_NOTIFICATION_EVENTS ].flatten, 'teamcity' => [ { @@ -672,9 +673,9 @@ module API desc: 'The password of the user' } ] - } + }.freeze - service_classes = [ + SERVICE_CLASSES = [ AsanaService, AssemblaService, BambooService, @@ -703,10 +704,10 @@ module API MattermostService, MicrosoftTeamsService, TeamcityService - ] + ].freeze if Rails.env.development? - services['mock-ci'] = [ + SERVICES['mock-ci'] = [ { required: true, name: :mock_service_url, @@ -714,17 +715,29 @@ module API desc: 'URL to the mock service' } ] - services['mock-deployment'] = [] - services['mock-monitoring'] = [] + SERVICES['mock-deployment'] = [] + SERVICES['mock-monitoring'] = [] - service_classes += [ + SERVICE_CLASSES += [ MockCiService, MockDeploymentService, MockMonitoringService ] end - trigger_services = { + SERVICE_CLASSES.each do |service| + event_names = service.try(:event_names) || next + event_names.each do |event_name| + SERVICES[service.to_param.tr("_", "-")] << { + required: false, + name: event_name.to_sym, + type: String, + desc: ServicesHelper.service_event_description(event_name) + } + end + end + + TRIGGER_SERVICES = { 'mattermost-slash-commands' => [ { name: :token, @@ -756,22 +769,9 @@ module API end end - services.each do |service_slug, settings| + SERVICES.each do |service_slug, settings| desc "Set #{service_slug} service for project" params do - service_classes.each do |service| - event_names = service.try(:event_names) || next - event_names.each do |event_name| - services[service.to_param.tr("_", "-")] << { - required: false, - name: event_name.to_sym, - type: String, - desc: ServicesHelper.service_event_description(event_name) - } - end - end - services.freeze - settings.each do |setting| if setting[:required] requires setting[:name], type: setting[:type], desc: setting[:desc] @@ -794,7 +794,7 @@ module API desc "Delete a service for project" params do - requires :service_slug, type: String, values: services.keys, desc: 'The name of the service' + requires :service_slug, type: String, values: SERVICES.keys, desc: 'The name of the service' end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) @@ -814,7 +814,7 @@ module API success Entities::ProjectService end params do - requires :service_slug, type: String, values: services.keys, desc: 'The name of the service' + requires :service_slug, type: String, values: SERVICES.keys, desc: 'The name of the service' end get ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) @@ -822,7 +822,7 @@ module API end end - trigger_services.each do |service_slug, settings| + TRIGGER_SERVICES.each do |service_slug, settings| helpers do def slash_command_service(project, service_slug, params) project.services.active.where(template: false).find do |service| -- cgit v1.2.1 From bf41063679b25371b2e64542f2f469b38502edf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Oct 2017 16:47:08 +0200 Subject: Remove explicit audit event log in MembershipActions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move it to Members::ApproveAccessRequestService. Also, note that there was a double audit event log for access request destruction. Signed-off-by: Rémy Coutable --- lib/api/access_requests.rb | 8 +++++--- lib/api/members.rb | 18 ++++++++++++------ lib/api/v3/members.rb | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-) (limited to 'lib/api') diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 60ae5e6b9a2..23ea05e1105 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -53,7 +53,10 @@ module API put ':id/access_requests/:user_id/approve' do source = find_source(source_type, params[:id]) - member = ::Members::ApproveAccessRequestService.new(source, current_user, declared_params).execute + access_requester = source.requesters.find_by!(user_id: params[:user_id]) + member = ::Members::ApproveAccessRequestService + .new(source, current_user, declared_params) + .execute(access_requester) status :created present member, with: Entities::Member @@ -70,8 +73,7 @@ module API member = source.requesters.find_by!(user_id: params[:user_id]) destroy_conditionally!(member) do - ::Members::DestroyService.new(source, current_user, params) - .execute(:requesters) + ::Members::DestroyService.new(source, current_user).execute(member) end end end diff --git a/lib/api/members.rb b/lib/api/members.rb index bc1de37284a..6367cde04bb 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -81,12 +81,18 @@ module API source = find_source(source_type, params.delete(:id)) authorize_admin_source!(source_type, source) - member = source.members.find_by!(user_id: params.delete(:user_id)) - - if member.update_attributes(declared_params(include_missing: false)) - present member, with: Entities::Member + member = source.members.find_by!(user_id: params[:user_id]) + updated_member = + ::Members::UpdateService.new( + source, + current_user, + declared_params(include_missing: false) + ).execute(member) + + if updated_member.valid? + present updated_member, with: Entities::Member else - render_validation_error!(member) + render_validation_error!(updated_member) end end @@ -99,7 +105,7 @@ module API member = source.members.find_by!(user_id: params[:user_id]) destroy_conditionally!(member) do - ::Members::DestroyService.new(source, current_user, declared_params).execute + ::Members::DestroyService.new(source, current_user).execute(member) end end end diff --git a/lib/api/v3/members.rb b/lib/api/v3/members.rb index d7bde8ceb89..dddc7d15d42 100644 --- a/lib/api/v3/members.rb +++ b/lib/api/v3/members.rb @@ -124,7 +124,7 @@ module API status(200 ) { message: "Access revoked", id: params[:user_id].to_i } else - ::Members::DestroyService.new(source, current_user, declared_params).execute + ::Members::DestroyService.new(source, current_user).execute(member) present member, with: ::API::Entities::Member end -- cgit v1.2.1 From 1c88d92b3fe174a56080575a14d6b473f17f7d8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 16 Feb 2018 15:10:22 +0100 Subject: Improve Member services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/api/access_requests.rb | 4 ++-- lib/api/members.rb | 10 ++++------ lib/api/v3/members.rb | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) (limited to 'lib/api') diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 23ea05e1105..ae13c248171 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -55,7 +55,7 @@ module API access_requester = source.requesters.find_by!(user_id: params[:user_id]) member = ::Members::ApproveAccessRequestService - .new(source, current_user, declared_params) + .new(current_user, declared_params) .execute(access_requester) status :created @@ -73,7 +73,7 @@ module API member = source.requesters.find_by!(user_id: params[:user_id]) destroy_conditionally!(member) do - ::Members::DestroyService.new(source, current_user).execute(member) + ::Members::DestroyService.new(current_user).execute(member) end end end diff --git a/lib/api/members.rb b/lib/api/members.rb index 6367cde04bb..8b12986d09e 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -83,11 +83,9 @@ module API member = source.members.find_by!(user_id: params[:user_id]) updated_member = - ::Members::UpdateService.new( - source, - current_user, - declared_params(include_missing: false) - ).execute(member) + ::Members::UpdateService + .new(current_user, declared_params(include_missing: false)) + .execute(member) if updated_member.valid? present updated_member, with: Entities::Member @@ -105,7 +103,7 @@ module API member = source.members.find_by!(user_id: params[:user_id]) destroy_conditionally!(member) do - ::Members::DestroyService.new(source, current_user).execute(member) + ::Members::DestroyService.new(current_user).execute(member) end end end diff --git a/lib/api/v3/members.rb b/lib/api/v3/members.rb index dddc7d15d42..88dd598f1e9 100644 --- a/lib/api/v3/members.rb +++ b/lib/api/v3/members.rb @@ -124,7 +124,7 @@ module API status(200 ) { message: "Access revoked", id: params[:user_id].to_i } else - ::Members::DestroyService.new(source, current_user).execute(member) + ::Members::DestroyService.new(current_user).execute(member) present member, with: ::API::Entities::Member end -- cgit v1.2.1 From ea32e1c41f2a1e2d5b68e2732d50caaec5bbb954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 27 Feb 2018 17:04:34 +0100 Subject: Fix a "can't modify frozen Hash" error in lib/api/services.rb in development env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/api/services.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'lib/api') diff --git a/lib/api/services.rb b/lib/api/services.rb index c5fb5db724f..6c97659166d 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -139,7 +139,7 @@ module API } ].freeze - SERVICES = { + services = { 'asana' => [ { required: true, @@ -673,9 +673,9 @@ module API desc: 'The password of the user' } ] - }.freeze + } - SERVICE_CLASSES = [ + service_classes = [ AsanaService, AssemblaService, BambooService, @@ -704,10 +704,10 @@ module API MattermostService, MicrosoftTeamsService, TeamcityService - ].freeze + ] if Rails.env.development? - SERVICES['mock-ci'] = [ + services['mock-ci'] = [ { required: true, name: :mock_service_url, @@ -715,16 +715,19 @@ module API desc: 'URL to the mock service' } ] - SERVICES['mock-deployment'] = [] - SERVICES['mock-monitoring'] = [] + services['mock-deployment'] = [] + services['mock-monitoring'] = [] - SERVICE_CLASSES += [ + service_classes += [ MockCiService, MockDeploymentService, MockMonitoringService ] end + SERVICES = services.freeze + SERVICE_CLASSES = service_classes.freeze + SERVICE_CLASSES.each do |service| event_names = service.try(:event_names) || next event_names.each do |event_name| -- cgit v1.2.1