From bcf7a7e76cc8aa4216f1df88be47172b9880ba24 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 13 Apr 2018 14:52:54 +0300 Subject: [PATCH] Don't reset application settings import sources If form does not have import sources checkboxes we should not reset import sources to empty. This fixes issue when import sources got reset after user modifies unrelated settings section like GitLab pages Signed-off-by: Dmitriy Zaporozhets --- .../admin/application_settings_controller.rb | 10 +------- .../_visibility_and_access.html.haml | 1 + spec/features/admin/admin_settings_spec.rb | 23 +++++++++++++++++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 145f74d9e59..5b1b26a0ed7 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -57,15 +57,6 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController def application_setting_params params[:application_setting] ||= {} - import_sources = params[:application_setting][:import_sources] - - if import_sources.nil? - params[:application_setting][:import_sources] = [] - else - import_sources.map! do |source| - source.to_str - end - end enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources) @@ -73,6 +64,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController AuthHelper.button_based_providers.map(&:to_s) - Array(enabled_oauth_sign_in_sources) + params[:application_setting][:import_sources]&.delete("") params[:application_setting][:restricted_visibility_levels]&.delete("") params.delete(:domain_blacklist_raw) if params[:domain_blacklist_file] 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 cbc779548f6..a75dd90fe6b 100644 --- a/app/views/admin/application_settings/_visibility_and_access.html.haml +++ b/app/views/admin/application_settings/_visibility_and_access.html.haml @@ -32,6 +32,7 @@ .form-group = f.label :import_sources, class: 'control-label col-sm-2' .col-sm-10 + = hidden_field_tag 'application_setting[import_sources][]' - import_sources_checkboxes('import-sources-help').each do |source| .checkbox= source %span.help-block#import-sources-help diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 846b8040be6..50d0a7abe59 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -32,6 +32,29 @@ feature 'Admin updates settings' do expect(find('#application_setting_visibility_level_20')).not_to be_checked end + scenario 'Modify import sources' do + expect(Gitlab::CurrentSettings.import_sources).not_to be_empty + + page.within('.as-visibility-access') do + Gitlab::ImportSources.options.map do |name, _| + uncheck name + end + + click_button 'Save changes' + end + + expect(page).to have_content "Application settings saved successfully" + expect(Gitlab::CurrentSettings.import_sources).to be_empty + + page.within('.as-visibility-access') do + check "Repo by URL" + click_button 'Save changes' + end + + expect(page).to have_content "Application settings saved successfully" + expect(Gitlab::CurrentSettings.import_sources).to eq(['git']) + end + scenario 'Change Visibility and Access Controls' do page.within('.as-visibility-access') do uncheck 'Project export enabled'