Merge branch 'dz-fix-admin-import-sources' into 'master'

Don't reset application settings checkboxes when save another section

Closes #45343

See merge request gitlab-org/gitlab-ce!18366

6 files changed, 58 insertions, 27 deletions

diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index 145f74d9e59..8958eab0423 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -57,22 +57,17 @@ 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
+    if params[:application_setting].key?(:enabled_oauth_sign_in_sources)
+      enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources)
+      enabled_oauth_sign_in_sources&.delete("")
 
-    enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources)
-
-    params[:application_setting][:disabled_oauth_sign_in_sources] =
-      AuthHelper.button_based_providers.map(&:to_s) -
-      Array(enabled_oauth_sign_in_sources)
+      params[:application_setting][:disabled_oauth_sign_in_sources] =
+        AuthHelper.button_based_providers.map(&:to_s) -
+        Array(enabled_oauth_sign_in_sources)
+    end
 
+    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/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index b3b080e6dcf..3fbb32c5229 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -74,10 +74,12 @@ module ApplicationSettingsHelper
       css_class = 'btn'
       css_class << ' active' unless disabled
       checkbox_name = 'application_setting[enabled_oauth_sign_in_sources][]'
+      name = Gitlab::Auth::OAuth::Provider.label_for(source)
 
       label_tag(checkbox_name, class: css_class) do
         check_box_tag(checkbox_name, source, !disabled,
-                      autocomplete: 'off') + Gitlab::Auth::OAuth::Provider.label_for(source)
+                      autocomplete: 'off',
+                      id: name.tr(' ', '_')) + name
       end
     end
   end
diff --git a/app/views/admin/application_settings/_signin.html.haml b/app/views/admin/application_settings/_signin.html.haml
index 864e64b5fa9..48331c40bca 100644
--- a/app/views/admin/application_settings/_signin.html.haml
+++ b/app/views/admin/application_settings/_signin.html.haml
@@ -24,6 +24,7 @@
     - if omniauth_enabled? && button_based_providers.any?
       .form-group
         = f.label :enabled_oauth_sign_in_sources, 'Enabled OAuth sign-in sources', class: 'control-label col-sm-2'
+        = hidden_field_tag 'application_setting[enabled_oauth_sign_in_sources][]'
         .col-sm-10
           .btn-group{ data: { toggle: 'buttons' } }
             - oauth_providers_checkboxes.each do |source|
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/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb
index cc1b1e5039e..b4fc2aa326f 100644
--- a/spec/controllers/admin/application_settings_controller_spec.rb
+++ b/spec/controllers/admin/application_settings_controller_spec.rb
@@ -72,11 +72,10 @@ describe Admin::ApplicationSettingsController do
       expect(ApplicationSetting.current.restricted_visibility_levels).to eq([10, 20])
     end
 
-    it 'falls back to defaults when settings are omitted' do
-      put :update, application_setting: {}
+    it 'updates the restricted_visibility_levels when empty array is passed' do
+      put :update, application_setting: { restricted_visibility_levels: [] }
 
       expect(response).to redirect_to(admin_application_settings_path)
-      expect(ApplicationSetting.current.default_project_visibility).to eq(Gitlab::VisibilityLevel::PRIVATE)
       expect(ApplicationSetting.current.restricted_visibility_levels).to be_empty
     end
   end
diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb
index 846b8040be6..7853d2952ea 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'
@@ -62,6 +85,26 @@ feature 'Admin updates settings' do
     expect(page).to have_content "Application settings saved successfully"
   end
 
+  scenario 'Modify oauth providers' do
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to be_empty
+
+    page.within('.as-signin') do
+      uncheck 'Google'
+      click_button 'Save changes'
+    end
+
+    expect(page).to have_content "Application settings saved successfully"
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to include('google_oauth2')
+
+    page.within('.as-signin') do
+      check "Google"
+      click_button 'Save changes'
+    end
+
+    expect(page).to have_content "Application settings saved successfully"
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).not_to include('google_oauth2')
+  end
+
   scenario 'Change Help page' do
     page.within('.as-help-page') do
       fill_in 'Help page text', with: 'Example text'
@@ -211,16 +254,6 @@ feature 'Admin updates settings' do
     expect(find('#service_push_channel').value).to eq '#test_channel'
   end
 
-  context 'sign-in restrictions', :js do
-    it 'de-activates oauth sign-in source' do
-      page.within('.as-signin') do
-        find('input#application_setting_enabled_oauth_sign_in_sources_[value=gitlab]').send_keys(:return)
-
-        expect(find('.btn', text: 'GitLab.com')).not_to have_css('.active')
-      end
-    end
-  end
-
   scenario 'Change Keys settings' do
     page.within('.as-visibility-access') do
       select 'Are forbidden', from: 'RSA SSH keys'