From 9a6b9499a2e30e0df6eb9b356221eace2b20f739 Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 15:52:27 +0700 Subject: [PATCH 1/7] Use IntegrationsHelper for scoped paths In group / admin controllers, we had some redundant scoped edit paths. This is now replaced with a method in IntegrationsHelper so it's cleaner to consolidate here. --- app/controllers/admin/integrations_controller.rb | 4 ---- app/controllers/groups/settings/integrations_controller.rb | 5 +---- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb index a605ee0297fe23..32e34f7905554d 100644 --- a/app/controllers/admin/integrations_controller.rb +++ b/app/controllers/admin/integrations_controller.rb @@ -25,8 +25,4 @@ def overrides def find_or_initialize_non_project_specific_integration(name) Integration.find_or_initialize_non_project_specific_integration(name, instance: true) end - - def scoped_edit_integration_path(integration) - edit_admin_application_settings_integration_path(integration) - end end diff --git a/app/controllers/groups/settings/integrations_controller.rb b/app/controllers/groups/settings/integrations_controller.rb index 8e3b2cb5d1ba63..f7cde8af85e164 100644 --- a/app/controllers/groups/settings/integrations_controller.rb +++ b/app/controllers/groups/settings/integrations_controller.rb @@ -4,6 +4,7 @@ module Groups module Settings class IntegrationsController < Groups::ApplicationController include IntegrationsActions + include IntegrationsHelper before_action :authorize_admin_group! @@ -26,10 +27,6 @@ def edit def find_or_initialize_non_project_specific_integration(name) Integration.find_or_initialize_non_project_specific_integration(name, group_id: group.id) end - - def scoped_edit_integration_path(integration) - edit_group_settings_integration_path(group, integration) - end end end end -- GitLab From be9ddf357933b2f3b757d43a588dc6a6939efd4d Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 16:01:49 +0700 Subject: [PATCH 2/7] Add feature flag definition and controller check --- app/controllers/admin/integrations_controller.rb | 5 +++++ .../development/instance_level_integration_overrides.yml | 8 ++++++++ 2 files changed, 13 insertions(+) create mode 100644 config/feature_flags/development/instance_level_integration_overrides.yml diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb index 32e34f7905554d..d967859c4f8d6e 100644 --- a/app/controllers/admin/integrations_controller.rb +++ b/app/controllers/admin/integrations_controller.rb @@ -5,6 +5,7 @@ class Admin::IntegrationsController < Admin::ApplicationController include IntegrationsHelper before_action :not_found, unless: -> { instance_level_integrations? } + before_action :not_found, unless: -> { instance_level_integration_overrides? }, only: :overrides feature_category :integrations @@ -25,4 +26,8 @@ def overrides def find_or_initialize_non_project_specific_integration(name) Integration.find_or_initialize_non_project_specific_integration(name, instance: true) end + + def instance_level_integration_overrides? + Feature.enabled?(:instance_level_integration_overrides, default_enabled: :yaml) + end end diff --git a/config/feature_flags/development/instance_level_integration_overrides.yml b/config/feature_flags/development/instance_level_integration_overrides.yml new file mode 100644 index 00000000000000..f99b85b3c051f3 --- /dev/null +++ b/config/feature_flags/development/instance_level_integration_overrides.yml @@ -0,0 +1,8 @@ +--- +name: instance_level_integration_overrides +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66723 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336750 +milestone: '14.2' +type: development +group: group::ecosystem +default_enabled: false -- GitLab From 66b4b1174f998dab56ab9d82dd81da8837d7379f Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 16:14:43 +0700 Subject: [PATCH 3/7] Add controller action and HAML views HAML view is added to the shared/integrations folder to allow us to re-use this later for group-level integration overrides. --- app/controllers/admin/integrations_controller.rb | 2 +- app/controllers/concerns/integrations_actions.rb | 2 +- app/helpers/integrations_helper.rb | 10 ++++++++++ app/views/shared/integrations/_tabs.html.haml | 14 ++++++++++++++ app/views/shared/integrations/overrides.html.haml | 10 ++++++++++ 5 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 app/views/shared/integrations/_tabs.html.haml create mode 100644 app/views/shared/integrations/overrides.html.haml diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb index d967859c4f8d6e..5ccd1be420a480 100644 --- a/app/controllers/admin/integrations_controller.rb +++ b/app/controllers/admin/integrations_controller.rb @@ -17,7 +17,7 @@ def overrides render json: serializer.represent(projects) end - # TODO frontend will add format.html + format.html { render 'shared/integrations/overrides' } end end diff --git a/app/controllers/concerns/integrations_actions.rb b/app/controllers/concerns/integrations_actions.rb index f1fa5c845e22a9..d36af2f033e7eb 100644 --- a/app/controllers/concerns/integrations_actions.rb +++ b/app/controllers/concerns/integrations_actions.rb @@ -6,7 +6,7 @@ module IntegrationsActions included do include Integrations::Params - before_action :integration, only: [:edit, :update, :test] + before_action :integration, only: [:edit, :update, :overrides, :test] end def edit diff --git a/app/helpers/integrations_helper.rb b/app/helpers/integrations_helper.rb index ab305d822e834a..734820f0e74b1f 100644 --- a/app/helpers/integrations_helper.rb +++ b/app/helpers/integrations_helper.rb @@ -47,6 +47,10 @@ def scoped_edit_integration_path(integration) end end + def scoped_overrides_integration_path(integration, options = {}) + overrides_admin_application_settings_integration_path(integration, options) + end + def scoped_test_integration_path(integration) if @project.present? test_project_service_path(@project, integration) @@ -97,6 +101,12 @@ def integration_form_data(integration, group: nil) form_data end + def integration_overrides_data(integration) + { + overrides_path: scoped_overrides_integration_path(integration, format: :json) + } + end + def integration_list_data(integrations) { integrations: integrations.map { |i| serialize_integration(i) }.to_json diff --git a/app/views/shared/integrations/_tabs.html.haml b/app/views/shared/integrations/_tabs.html.haml new file mode 100644 index 00000000000000..ff97e371374d12 --- /dev/null +++ b/app/views/shared/integrations/_tabs.html.haml @@ -0,0 +1,14 @@ +.tabs.gl-tabs + %div + %ul.nav.gl-tabs-nav{ role: 'tablist' } + %li.nav-item{ role: 'presentation' } + %a.nav-link.gl-tab-nav-item{ role: 'tab', href: scoped_edit_integration_path(integration) } + = _('Settings') + + %li.nav-item{ role: 'presentation' } + %a.nav-link.gl-tab-nav-item.gl-tab-nav-item-active.gl-tab-nav-item-active-indigo.active{ role: 'tab', href: scoped_overrides_integration_path(integration) } + = s_('Integrations|Projects using custom settings') + + .tab-content.gl-tab-content + .tab-pane.active{ role: 'tabpanel' } + = yield diff --git a/app/views/shared/integrations/overrides.html.haml b/app/views/shared/integrations/overrides.html.haml new file mode 100644 index 00000000000000..4d8cc94e967bbe --- /dev/null +++ b/app/views/shared/integrations/overrides.html.haml @@ -0,0 +1,10 @@ +- add_to_breadcrumbs _('Integrations'), scoped_integrations_path +- breadcrumb_title @integration.title +- page_title @integration.title, _('Integrations') +- @content_class = 'limit-container-width' unless fluid_layout + +%h3.page-title + = @integration.title + += render 'shared/integrations/tabs', integration: @integration do + .js-vue-integration-overrides{ data: integration_overrides_data(@integration) } -- GitLab From 57b8567cc3b83371ff21913c860133990d16b6a8 Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 16:18:10 +0700 Subject: [PATCH 4/7] Add Vue app structure for integration overrides Only init the app for now --- .../components/integration_overrides.vue | 15 ++++++++++++ .../integrations/overrides/index.js | 23 +++++++++++++++++++ .../admin/integrations/overrides/index.js | 3 +++ 3 files changed, 41 insertions(+) create mode 100644 app/assets/javascripts/integrations/overrides/components/integration_overrides.vue create mode 100644 app/assets/javascripts/integrations/overrides/index.js create mode 100644 app/assets/javascripts/pages/admin/integrations/overrides/index.js diff --git a/app/assets/javascripts/integrations/overrides/components/integration_overrides.vue b/app/assets/javascripts/integrations/overrides/components/integration_overrides.vue new file mode 100644 index 00000000000000..bfb16779854fc3 --- /dev/null +++ b/app/assets/javascripts/integrations/overrides/components/integration_overrides.vue @@ -0,0 +1,15 @@ + + + diff --git a/app/assets/javascripts/integrations/overrides/index.js b/app/assets/javascripts/integrations/overrides/index.js new file mode 100644 index 00000000000000..0f03b23ba218d3 --- /dev/null +++ b/app/assets/javascripts/integrations/overrides/index.js @@ -0,0 +1,23 @@ +import Vue from 'vue'; +import IntegrationOverrides from './components/integration_overrides.vue'; + +export default () => { + const el = document.querySelector('.js-vue-integration-overrides'); + + if (!el) { + return null; + } + + const { overridesPath } = el.dataset; + + return new Vue({ + el, + render(createElement) { + return createElement(IntegrationOverrides, { + props: { + overridesPath, + }, + }); + }, + }); +}; diff --git a/app/assets/javascripts/pages/admin/integrations/overrides/index.js b/app/assets/javascripts/pages/admin/integrations/overrides/index.js new file mode 100644 index 00000000000000..b150470914433f --- /dev/null +++ b/app/assets/javascripts/pages/admin/integrations/overrides/index.js @@ -0,0 +1,3 @@ +import initIntegrationOverrides from '~/integrations/overrides'; + +initIntegrationOverrides(); -- GitLab From f0d12cca052ce256b55320e21878792dedf382c2 Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 16:45:34 +0700 Subject: [PATCH 5/7] Add basic controller specs - For format.html to make sure instance variable is set and template is rendered. - Also add the scenario when the feature flag is not enabled. - Update translations --- locale/gitlab.pot | 3 ++ .../admin/integrations_controller_spec.rb | 41 +++++++++++++++---- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index de72d782daf861..ca6d44c3274d01 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17656,6 +17656,9 @@ msgstr "" msgid "Integrations|Note: this integration only works with accounts on GitLab.com (SaaS)." msgstr "" +msgid "Integrations|Projects using custom settings" +msgstr "" + msgid "Integrations|Projects using custom settings will not be affected." msgstr "" diff --git a/spec/controllers/admin/integrations_controller_spec.rb b/spec/controllers/admin/integrations_controller_spec.rb index 617a43b37850d0..a8e900f2ee21ea 100644 --- a/spec/controllers/admin/integrations_controller_spec.rb +++ b/spec/controllers/admin/integrations_controller_spec.rb @@ -105,17 +105,44 @@ let_it_be(:overridden_other_integration) { create(:confluence_integration) } subject do - get :overrides, params: { id: instance_integration.class.to_param }, format: :json + get :overrides, params: { id: instance_integration.class.to_param }, format: format end - include_context 'JSON response' + context 'when format is HTML' do + let(:format) { :json } - it 'returns projects with overrides', :aggregate_failures do - subject + include_context 'JSON response' - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to contain_exactly(a_hash_including('full_name' => overridden_integration.project.full_name)) + it 'returns projects with overrides', :aggregate_failures do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to contain_exactly(a_hash_including('full_name' => overridden_integration.project.full_name)) + end + end + + context 'when format is HTML' do + let(:format) { :html } + + it 'renders template' do + subject + + expect(response).to render_template 'shared/integrations/overrides' + expect(assigns(:integration)).to eq(instance_integration) + end + + context 'when `instance_level_integration_overrides` is not enabled' do + before do + stub_feature_flags(instance_level_integration_overrides: false) + end + + it 'renders a 404' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end end -- GitLab From f05a9968d4ba46cb6e4173c813e224cbe585b55c Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Fri, 23 Jul 2021 19:26:42 +0700 Subject: [PATCH 6/7] Change logic for return 404 To avoid a failing spec and repeating the `not_found` before_action which causes issues. --- app/controllers/admin/integrations_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb index 5ccd1be420a480..a5a90dede69464 100644 --- a/app/controllers/admin/integrations_controller.rb +++ b/app/controllers/admin/integrations_controller.rb @@ -5,11 +5,12 @@ class Admin::IntegrationsController < Admin::ApplicationController include IntegrationsHelper before_action :not_found, unless: -> { instance_level_integrations? } - before_action :not_found, unless: -> { instance_level_integration_overrides? }, only: :overrides feature_category :integrations def overrides + return render_404 unless instance_level_integration_overrides? + respond_to do |format| format.json do projects = Project.with_active_integration(integration.class).merge(::Integration.not_inherited) -- GitLab From c1d3ce9451838f782377b376ba5cedf9b09f99c4 Mon Sep 17 00:00:00 2001 From: Justin Ho Date: Mon, 26 Jul 2021 10:51:02 +0700 Subject: [PATCH 7/7] Fix spec name and refactor controller code We can include IntegrationsHelper in the shared IntegrationsActions to make the admin / group controllers cleaner. --- app/controllers/admin/integrations_controller.rb | 1 - app/controllers/concerns/integrations_actions.rb | 1 + app/controllers/groups/settings/integrations_controller.rb | 1 - spec/controllers/admin/integrations_controller_spec.rb | 2 +- 4 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb index a5a90dede69464..e273c026993203 100644 --- a/app/controllers/admin/integrations_controller.rb +++ b/app/controllers/admin/integrations_controller.rb @@ -2,7 +2,6 @@ class Admin::IntegrationsController < Admin::ApplicationController include IntegrationsActions - include IntegrationsHelper before_action :not_found, unless: -> { instance_level_integrations? } diff --git a/app/controllers/concerns/integrations_actions.rb b/app/controllers/concerns/integrations_actions.rb index d36af2f033e7eb..dd066cc1b02ccb 100644 --- a/app/controllers/concerns/integrations_actions.rb +++ b/app/controllers/concerns/integrations_actions.rb @@ -5,6 +5,7 @@ module IntegrationsActions included do include Integrations::Params + include IntegrationsHelper before_action :integration, only: [:edit, :update, :overrides, :test] end diff --git a/app/controllers/groups/settings/integrations_controller.rb b/app/controllers/groups/settings/integrations_controller.rb index f7cde8af85e164..a7a1de03224e37 100644 --- a/app/controllers/groups/settings/integrations_controller.rb +++ b/app/controllers/groups/settings/integrations_controller.rb @@ -4,7 +4,6 @@ module Groups module Settings class IntegrationsController < Groups::ApplicationController include IntegrationsActions - include IntegrationsHelper before_action :authorize_admin_group! diff --git a/spec/controllers/admin/integrations_controller_spec.rb b/spec/controllers/admin/integrations_controller_spec.rb index a8e900f2ee21ea..3d37fe7bf79e17 100644 --- a/spec/controllers/admin/integrations_controller_spec.rb +++ b/spec/controllers/admin/integrations_controller_spec.rb @@ -108,7 +108,7 @@ get :overrides, params: { id: instance_integration.class.to_param }, format: format end - context 'when format is HTML' do + context 'when format is JSON' do let(:format) { :json } include_context 'JSON response' -- GitLab