Merge branch 'bw-refactor-settings-show-panel' into 'master'

Refactored settings 'show' panel to 'general'

See merge request gitlab-org/gitlab!16476

app/controllers/admin/application_settings_controller.rb

@@ -6,15 +6,16 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   before_action :set_application_setting
   before_action :whitelist_query_limiting, only: [:usage_data]
 
-  VALID_SETTING_PANELS = %w(show integrations repository templates
+  VALID_SETTING_PANELS = %w(general integrations repository templates
                             ci_cd reporting metrics_and_profiling
                             network geo preferences).freeze
 
-  def show
+  VALID_SETTING_PANELS.each do |action|
+    define_method(action) { perform_update if submitted? }
   end
 
-  (VALID_SETTING_PANELS - %w(show)).each do |action|
-    define_method(action) { perform_update if submitted? }
+  def show
+    render :general
   end
 
   def update
@@ -144,7 +145,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   end
 
   def render_update_error
-    action = VALID_SETTING_PANELS.include?(action_name) ? action_name : :show
+    action = VALID_SETTING_PANELS.include?(action_name) ? action_name : :general
 
     render action
   end

app/views/admin/application_settings/_account_and_limit.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-account-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-account-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_diff_limits.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-merge-request-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-merge-request-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_external_authorization_service_form.html.haml

@@ -8,7 +8,7 @@
       = _('External Classification Policy Authorization')
   .settings-content
 
-    = form_for @application_setting, url: admin_application_settings_path(anchor: 'js-external-auth-settings'), html: { class: 'fieldset-form' } do |f|
+    = form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-external-auth-settings'), html: { class: 'fieldset-form' } do |f|
       = form_errors(@application_setting)
 
       %fieldset

app/views/admin/application_settings/_signin.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-signin-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-signin-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_signup.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-signup-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-signup-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_terminal.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-terminal-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-terminal-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_terms.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-terms-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-terms-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/_visibility_and_access.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-visibility-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: general_admin_application_settings_path(anchor: 'js-visibility-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset

app/views/admin/application_settings/show.html.haml → app/views/admin/application_settings/general.html.haml

-- breadcrumb_title _("Settings")
-- page_title _("Settings")
+- breadcrumb_title _("General")
+- page_title _("General")
 - @content_class = "limit-container-width" unless fluid_layout
 
 %section.settings.as-visibility-access.no-animate#js-visibility-settings{ class: ('expanded' if expanded_by_default?) }
@@ -90,7 +90,7 @@
     %p
       = _('Manage Web IDE features')
   .settings-content
-    = form_for @application_setting, url: admin_application_settings_path(anchor: "#js-web-ide-settings"), html: { class: 'fieldset-form' } do |f|
+    = form_for @application_setting, url: general_admin_application_settings_path(anchor: "#js-web-ide-settings"), html: { class: 'fieldset-form' } do |f|
       = form_errors(@application_setting)
 
       %fieldset

app/views/layouts/nav/sidebar/_admin.html.haml

@@ -232,7 +232,7 @@
                 = _('Settings')
             %li.divider.fly-out-top-item
             = nav_link(path: 'application_settings#show') do
-              = link_to admin_application_settings_path, title: _('General'), class: 'qa-admin-settings-general-item' do
+              = link_to general_admin_application_settings_path, title: _('General'), class: 'qa-admin-settings-general-item' do
                 %span
                   = _('General')
             = nav_link(path: 'application_settings#integrations') do

config/routes/admin.rb

@@ -110,7 +110,7 @@ namespace :admin do
     put :reset_registration_token
     put :reset_health_check_token
     put :clear_repository_check_states
-    match :integrations, :repository, :templates, :ci_cd, :reporting, :metrics_and_profiling, :network, :geo, :preferences, via: [:get, :patch]
+    match :general, :integrations, :repository, :templates, :ci_cd, :reporting, :metrics_and_profiling, :network, :geo, :preferences, via: [:get, :patch]
     get :lets_encrypt_terms_of_service
   end
 

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -115,21 +115,21 @@ describe Admin::ApplicationSettingsController do
     it 'does not accept negative repository_size_limit' do
       put :update, params: { application_setting: { repository_size_limit: '-100' } }
 
-      expect(response).to render_template(:show)
+      expect(response).to render_template(:general)
       expect(assigns(:application_setting).errors[:repository_size_limit]).to be_present
     end
 
     it 'does not accept invalid repository_size_limit' do
       put :update, params: { application_setting: { repository_size_limit: 'one thousand' } }
 
-      expect(response).to render_template(:show)
+      expect(response).to render_template(:general)
       expect(assigns(:application_setting).errors[:repository_size_limit]).to be_present
     end
 
     it 'does not accept empty repository_size_limit' do
       put :update, params: { application_setting: { repository_size_limit: '' } }
 
-      expect(response).to render_template(:show)
+      expect(response).to render_template(:general)
       expect(assigns(:application_setting).errors[:repository_size_limit]).to be_present
     end
   end

qa/qa/page/admin/settings/general.rb

@@ -7,7 +7,7 @@ module QA
         class General < Page::Base
           include QA::Page::Settings::Common
 
-          view 'app/views/admin/application_settings/show.html.haml' do
+          view 'app/views/admin/application_settings/general.html.haml' do
             element :account_and_limit_settings
           end
 

spec/controllers/admin/application_settings_controller_spec.rb

@@ -118,32 +118,7 @@ describe Admin::ApplicationSettingsController do
     end
 
     describe 'verify panel actions' do
-      shared_examples 'renders correct panels' do
-        it 'renders correct action on error' do
-          expect_next_instance_of(ApplicationSettings::UpdateService) do |service|
-            allow(service).to receive(:execute).and_return(false)
-          end
-
-          patch action, params: { application_setting: { unused_param: true } }
-
-          expect(subject).to render_template(action)
-        end
-
-        it 'redirects to same panel on success' do
-          expect_next_instance_of(ApplicationSettings::UpdateService) do |service|
-            allow(service).to receive(:execute).and_return(true)
-          end
-
-          referer_path = public_send("#{action}_admin_application_settings_path")
-          request.env["HTTP_REFERER"] = referer_path
-
-          patch action, params: { application_setting: { unused_param: true } }
-
-          expect(subject).to redirect_to(referer_path)
-        end
-      end
-
-      (Admin::ApplicationSettingsController::VALID_SETTING_PANELS - %w(show templates geo)).each do |valid_action|
+      (Admin::ApplicationSettingsController::VALID_SETTING_PANELS - %w(templates geo)).each do |valid_action|
         it_behaves_like 'renders correct panels' do
           let(:action) { valid_action }
         end

spec/features/admin/admin_disables_git_access_protocol_spec.rb

@@ -76,6 +76,7 @@ describe 'Admin disables Git access protocol', :js do
   context 'with nothing disabled' do
     before do
       create(:personal_key, user: admin)
+      allow_all_protocols
     end
 
     it 'shows default SSH url and protocol selection dropdown' do
@@ -107,6 +108,10 @@ describe 'Admin disables Git access protocol', :js do
     visit project_path(project)
   end
 
+  def allow_all_protocols
+    switch_git_protocol(1)
+  end
+
   def disable_http_protocol
     switch_git_protocol(2)
   end

spec/features/admin/admin_settings_spec.rb

@@ -15,7 +15,7 @@ describe 'Admin updates settings' do
 
   context 'General page' do
     before do
-      visit admin_application_settings_path
+      visit general_admin_application_settings_path
     end
 
     it 'Change visibility settings' do

spec/frontend/fixtures/application_settings.rb

@@ -26,7 +26,7 @@ describe Admin::ApplicationSettingsController, '(JavaScript fixtures)', type: :c
   it 'application_settings/accounts_and_limit.html' do
     stub_application_setting(user_default_external: false)
 
-    get :show
+    get :general
 
     expect(response).to be_successful
   end

spec/javascripts/test_bundle.js

@@ -210,7 +210,7 @@ if (process.env.BABEL_ENV === 'coverage') {
     './terminal/terminal_bundle.js',
     './users/users_bundle.js',
     './issue_show/index.js',
-    './pages/admin/application_settings/show/index.js',
+    './pages/admin/application_settings/general/index.js',
   ];
 
   describe('Uncovered files', function() {

spec/support/shared_examples/controllers/application_settings_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'renders correct panels' do
+  it 'renders correct action on error' do
+    expect_next_instance_of(ApplicationSettings::UpdateService) do |service|
+      allow(service).to receive(:execute).and_return(false)
+    end
+
+    patch action, params: { application_setting: { unused_param: true } }
+
+    expect(subject).to render_template(action)
+  end
+
+  it 'redirects to same panel on success' do
+    expect_next_instance_of(ApplicationSettings::UpdateService) do |service|
+      allow(service).to receive(:execute).and_return(true)
+    end
+
+    referer_path = public_send("#{action}_admin_application_settings_path")
+    request.env["HTTP_REFERER"] = referer_path
+
+    patch action, params: { application_setting: { unused_param: true } }
+
+    expect(subject).to redirect_to(referer_path)
+  end
+end