diff options
author | Brett Walker <bwalker@gitlab.com> | 2019-06-28 00:05:12 +0300 |
---|---|---|
committer | Brett Walker <bwalker@gitlab.com> | 2019-09-07 02:04:33 +0300 |
commit | d44735f802180695b73ec2ee4c4d5fe72f6ef6d8 (patch) | |
tree | 257b3d77113ca8543f1f9d9ecab076d70419854d | |
parent | 1ffa66effbc9272ac9755ba29fab2fd1b3d93db5 (diff) |
Refactored settings 'show' panel to 'general'12307-errors-in-application-settings-geo-panel-shows-wrong-panel-ce
This keeps the routes consistent - each panel has it's own
specific route. In addition, makes the naming more
consistent, using 'General' for the panel name.
20 files changed, 55 insertions, 48 deletions
diff --git a/app/assets/javascripts/pages/admin/application_settings/show/index.js b/app/assets/javascripts/pages/admin/application_settings/general/index.js index 5ec9688a6e4..5ec9688a6e4 100644 --- a/app/assets/javascripts/pages/admin/application_settings/show/index.js +++ b/app/assets/javascripts/pages/admin/application_settings/general/index.js diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 99411641874..23446741f52 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/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 @@ -141,7 +142,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 diff --git a/app/views/admin/application_settings/_account_and_limit.html.haml b/app/views/admin/application_settings/_account_and_limit.html.haml index 9ed4bc44aae..4358365504a 100644 --- a/app/views/admin/application_settings/_account_and_limit.html.haml +++ b/app/views/admin/application_settings/_account_and_limit.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_diff_limits.html.haml b/app/views/admin/application_settings/_diff_limits.html.haml index 408e569fe07..137b7281e0f 100644 --- a/app/views/admin/application_settings/_diff_limits.html.haml +++ b/app/views/admin/application_settings/_diff_limits.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_external_authorization_service_form.html.haml b/app/views/admin/application_settings/_external_authorization_service_form.html.haml index 7587ecbf9d3..73412133979 100644 --- a/app/views/admin/application_settings/_external_authorization_service_form.html.haml +++ b/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 diff --git a/app/views/admin/application_settings/_signin.html.haml b/app/views/admin/application_settings/_signin.html.haml index 5f36358f599..0e45301b598 100644 --- a/app/views/admin/application_settings/_signin.html.haml +++ b/app/views/admin/application_settings/_signin.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_signup.html.haml b/app/views/admin/application_settings/_signup.html.haml index a0a58b811ee..7c1df78f30c 100644 --- a/app/views/admin/application_settings/_signup.html.haml +++ b/app/views/admin/application_settings/_signup.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_terminal.html.haml b/app/views/admin/application_settings/_terminal.html.haml index 49980e1e1a7..654aed54a15 100644 --- a/app/views/admin/application_settings/_terminal.html.haml +++ b/app/views/admin/application_settings/_terminal.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_terms.html.haml b/app/views/admin/application_settings/_terms.html.haml index ef58e9b1128..19e7ab7c99a 100644 --- a/app/views/admin/application_settings/_terms.html.haml +++ b/app/views/admin/application_settings/_terms.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/_visibility_and_access.html.haml b/app/views/admin/application_settings/_visibility_and_access.html.haml index c07bafbe302..e57ef1ea18f 100644 --- a/app/views/admin/application_settings/_visibility_and_access.html.haml +++ b/app/views/admin/application_settings/_visibility_and_access.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/admin/application_settings/show.html.haml b/app/views/admin/application_settings/general.html.haml index 31f18ba0d56..b9f49fdc9de 100644 --- a/app/views/admin/application_settings/show.html.haml +++ b/app/views/admin/application_settings/general.html.haml @@ -1,5 +1,5 @@ -- 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 diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index f76268bc29b..784fe556123 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/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 diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 6f9a5552564..a003ffca270 100644 --- a/config/routes/admin.rb +++ b/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 diff --git a/qa/qa/page/admin/settings/general.rb b/qa/qa/page/admin/settings/general.rb index 93b290f7e03..150775f57d2 100644 --- a/qa/qa/page/admin/settings/general.rb +++ b/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 diff --git a/spec/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb index 4eb0545eb6c..d62e0a97609 100644 --- a/spec/controllers/admin/application_settings_controller_spec.rb +++ b/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 diff --git a/spec/features/admin/admin_disables_git_access_protocol_spec.rb b/spec/features/admin/admin_disables_git_access_protocol_spec.rb index 4c3c0904a06..bc757d72a49 100644 --- a/spec/features/admin/admin_disables_git_access_protocol_spec.rb +++ b/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 diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index eb59de2e132..ccf38cf1bd1 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/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 diff --git a/spec/frontend/fixtures/application_settings.rb b/spec/frontend/fixtures/application_settings.rb index 38a060580c1..afe5949ed3b 100644 --- a/spec/frontend/fixtures/application_settings.rb +++ b/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 diff --git a/spec/javascripts/test_bundle.js b/spec/javascripts/test_bundle.js index ce453d7c483..c0a999cfaa6 100644 --- a/spec/javascripts/test_bundle.js +++ b/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() { diff --git a/spec/support/shared_examples/controllers/application_settings_shared_examples.rb b/spec/support/shared_examples/controllers/application_settings_shared_examples.rb new file mode 100644 index 00000000000..9619451cd14 --- /dev/null +++ b/spec/support/shared_examples/controllers/application_settings_shared_examples.rb @@ -0,0 +1,26 @@ +# 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 |