Update application settings using correct action

Updating multiple application settings panels through
a single action causes the incorrect action to be shown
when there are errors.  Instead, make each panel action
handle both updating and display.

28 files changed, 117 insertions, 68 deletions

diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index 57b976b9121..42634bf611e 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -6,56 +6,19 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   before_action :set_application_setting
   before_action :whitelist_query_limiting, only: [:usage_data]
 
-  def show
-  end
-
-  def integrations
-  end
-
-  def repository
-  end
-
-  def templates
-  end
-
-  def ci_cd
-  end
-
-  def reporting
-  end
-
-  def metrics_and_profiling
-  end
-
-  def network
-  end
+  VALID_SETTING_PANELS = %w(show integrations repository templates
+                            ci_cd reporting metrics_and_profiling
+                            network geo preferences).freeze
 
-  def geo
+  def show
   end
 
-  def preferences
+  (VALID_SETTING_PANELS - %w(show)).each do |action|
+    define_method(action) { perform_update if submitted? }
   end
 
   def update
-    successful = ApplicationSettings::UpdateService
-      .new(@application_setting, current_user, application_setting_params)
-      .execute
-
-    if recheck_user_consent?
-      session[:ask_for_usage_stats_consent] = current_user.requires_usage_stats_consent?
-    end
-
-    redirect_path = referer_path(request) || admin_application_settings_path
-
-    respond_to do |format|
-      if successful
-        format.json { head :ok }
-        format.html { redirect_to redirect_path, notice: _('Application settings saved successfully') }
-      else
-        format.json { head :bad_request }
-        format.html { render :show }
-      end
-    end
+    perform_update
   end
 
   def usage_data
@@ -149,6 +112,38 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     ]
   end
 
+  def submitted?
+    request.patch?
+  end
+
+  def perform_update
+    successful = ApplicationSettings::UpdateService
+      .new(@application_setting, current_user, application_setting_params)
+      .execute
+
+    if recheck_user_consent?
+      session[:ask_for_usage_stats_consent] = current_user.requires_usage_stats_consent?
+    end
+
+    redirect_path = referer_path(request) || admin_application_settings_path
+
+    respond_to do |format|
+      if successful
+        format.json { head :ok }
+        format.html { redirect_to redirect_path, notice: _('Application settings saved successfully') }
+      else
+        format.json { head :bad_request }
+        format.html { render_update_error }
+      end
+    end
+  end
+
+  def render_update_error
+    action = VALID_SETTING_PANELS.include?(action_name) ? action_name : :show
+
+    render action
+  end
+
   def lets_encrypt_visible_attributes
     return [] unless Feature.enabled?(:pages_auto_ssl)
 
diff --git a/app/views/admin/application_settings/_abuse.html.haml b/app/views/admin/application_settings/_abuse.html.haml
index 5f8bd799d23..ddffec32c41 100644
--- a/app/views/admin/application_settings/_abuse.html.haml
+++ b/app/views/admin/application_settings/_abuse.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-abuse-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: reporting_admin_application_settings_path(anchor: 'js-abuse-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_ci_cd.html.haml b/app/views/admin/application_settings/_ci_cd.html.haml
index b8c481df0d2..d1de4286ee7 100644
--- a/app/views/admin/application_settings/_ci_cd.html.haml
+++ b/app/views/admin/application_settings/_ci_cd.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-ci-cd-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: ci_cd_admin_application_settings_path(anchor: 'js-ci-cd-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_email.html.haml b/app/views/admin/application_settings/_email.html.haml
index 3f30c75fbb6..bd60ff0b99c 100644
--- a/app/views/admin/application_settings/_email.html.haml
+++ b/app/views/admin/application_settings/_email.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-email-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-email-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_gitaly.html.haml b/app/views/admin/application_settings/_gitaly.html.haml
index f39d5709811..1da02de0461 100644
--- a/app/views/admin/application_settings/_gitaly.html.haml
+++ b/app/views/admin/application_settings/_gitaly.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-gitaly-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-gitaly-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_help_page.html.haml b/app/views/admin/application_settings/_help_page.html.haml
index aa491c735d1..a869f1bd4df 100644
--- a/app/views/admin/application_settings/_help_page.html.haml
+++ b/app/views/admin/application_settings/_help_page.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-help-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-help-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_influx.html.haml b/app/views/admin/application_settings/_influx.html.haml
index dc5cbb8fa94..98c7a9659c3 100644
--- a/app/views/admin/application_settings/_influx.html.haml
+++ b/app/views/admin/application_settings/_influx.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-influx-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-influx-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_ip_limits.html.haml b/app/views/admin/application_settings/_ip_limits.html.haml
index 5a5681830b8..67a04fcf698 100644
--- a/app/views/admin/application_settings/_ip_limits.html.haml
+++ b/app/views/admin/application_settings/_ip_limits.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-ip-limits-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-ip-limits-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_localization.html.haml b/app/views/admin/application_settings/_localization.html.haml
index 95d016a94a5..bb4d1fa1241 100644
--- a/app/views/admin/application_settings/_localization.html.haml
+++ b/app/views/admin/application_settings/_localization.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-localization-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-localization-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_logging.html.haml b/app/views/admin/application_settings/_logging.html.haml
index 1da5f6fccd6..d57066bba01 100644
--- a/app/views/admin/application_settings/_logging.html.haml
+++ b/app/views/admin/application_settings/_logging.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-logging-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: reporting_admin_application_settings_path(anchor: 'js-logging-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %p
diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml
index dd56bb99a06..d16304ed338 100644
--- a/app/views/admin/application_settings/_outbound.html.haml
+++ b/app/views/admin/application_settings/_outbound.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-outbound-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-outbound-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_pages.html.haml b/app/views/admin/application_settings/_pages.html.haml
index 77795dbf913..d7d709ffd62 100644
--- a/app/views/admin/application_settings/_pages.html.haml
+++ b/app/views/admin/application_settings/_pages.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-pages-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-pages-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_performance.html.haml b/app/views/admin/application_settings/_performance.html.haml
index e7076e7ed2f..7821a83530f 100644
--- a/app/views/admin/application_settings/_performance.html.haml
+++ b/app/views/admin/application_settings/_performance.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-performance-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-performance-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_performance_bar.html.haml b/app/views/admin/application_settings/_performance_bar.html.haml
index 1e66b635038..f8bc29048f2 100644
--- a/app/views/admin/application_settings/_performance_bar.html.haml
+++ b/app/views/admin/application_settings/_performance_bar.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-performance-bar-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-performance-bar-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_plantuml.html.haml b/app/views/admin/application_settings/_plantuml.html.haml
index 5c2b7114426..86dc289dd7c 100644
--- a/app/views/admin/application_settings/_plantuml.html.haml
+++ b/app/views/admin/application_settings/_plantuml.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-plantuml-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: integrations_admin_application_settings_path(anchor: 'js-plantuml-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_prometheus.html.haml b/app/views/admin/application_settings/_prometheus.html.haml
index a923568e52d..4c0ff3a18e8 100644
--- a/app/views/admin/application_settings/_prometheus.html.haml
+++ b/app/views/admin/application_settings/_prometheus.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-prometheus-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-prometheus-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_realtime.html.haml b/app/views/admin/application_settings/_realtime.html.haml
index 92f0c02eb6a..8f6946534ea 100644
--- a/app/views/admin/application_settings/_realtime.html.haml
+++ b/app/views/admin/application_settings/_realtime.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-realtime-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: preferences_admin_application_settings_path(anchor: 'js-realtime-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_registry.html.haml b/app/views/admin/application_settings/_registry.html.haml
index 08c981db13f..77623e1495b 100644
--- a/app/views/admin/application_settings/_registry.html.haml
+++ b/app/views/admin/application_settings/_registry.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-registry-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: ci_cd_admin_application_settings_path(anchor: 'js-registry-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_repository_check.html.haml b/app/views/admin/application_settings/_repository_check.html.haml
index 925e39bc0a3..417916d8c25 100644
--- a/app/views/admin/application_settings/_repository_check.html.haml
+++ b/app/views/admin/application_settings/_repository_check.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-repository-check-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: repository_admin_application_settings_path(anchor: 'js-repository-check-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_repository_mirrors_form.html.haml b/app/views/admin/application_settings/_repository_mirrors_form.html.haml
index f2f2cd1282a..362f4a42464 100644
--- a/app/views/admin/application_settings/_repository_mirrors_form.html.haml
+++ b/app/views/admin/application_settings/_repository_mirrors_form.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-mirror-settings') do |f|
+= form_for @application_setting, url: repository_admin_application_settings_path(anchor: 'js-mirror-settings') do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_repository_storage.html.haml b/app/views/admin/application_settings/_repository_storage.html.haml
index 7a2bbfcdc4d..e5bcb180445 100644
--- a/app/views/admin/application_settings/_repository_storage.html.haml
+++ b/app/views/admin/application_settings/_repository_storage.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-repository-storage-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: repository_admin_application_settings_path(anchor: 'js-repository-storage-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_spam.html.haml b/app/views/admin/application_settings/_spam.html.haml
index 54cda531580..a34fc15acb1 100644
--- a/app/views/admin/application_settings/_spam.html.haml
+++ b/app/views/admin/application_settings/_spam.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-spam-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: reporting_admin_application_settings_path(anchor: 'js-spam-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_third_party_offers.html.haml b/app/views/admin/application_settings/_third_party_offers.html.haml
index fae5b0b965f..adde09f75e4 100644
--- a/app/views/admin/application_settings/_third_party_offers.html.haml
+++ b/app/views/admin/application_settings/_third_party_offers.html.haml
@@ -1,6 +1,6 @@
 - application_setting = local_assigns.fetch(:application_setting)
 
-= form_for application_setting, url: admin_application_settings_path(anchor: 'js-third-party-offers-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for application_setting, url: integrations_admin_application_settings_path(anchor: 'js-third-party-offers-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(application_setting)
 
   %fieldset
diff --git a/app/views/admin/application_settings/_usage.html.haml b/app/views/admin/application_settings/_usage.html.haml
index 788595877ea..d716b52be05 100644
--- a/app/views/admin/application_settings/_usage.html.haml
+++ b/app/views/admin/application_settings/_usage.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-usage-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-usage-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/changelogs/unreleased/57973-errors-in-application-settings-panel-shows-wrong-panel.yml b/changelogs/unreleased/57973-errors-in-application-settings-panel-shows-wrong-panel.yml
new file mode 100644
index 00000000000..2b62992eda0
--- /dev/null
+++ b/changelogs/unreleased/57973-errors-in-application-settings-panel-shows-wrong-panel.yml
@@ -0,0 +1,5 @@
+---
+title: Show poper panel when validation error occurs in admin settings panels
+merge_request: 25434
+author:
+type: fixed
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index ae79beb1dba..f609739d9fd 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
-    get :integrations, :repository, :templates, :ci_cd, :reporting, :metrics_and_profiling, :network, :geo, :preferences
+    match :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/spec/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb
index b89348b7a7e..c7f814c58c0 100644
--- a/spec/controllers/admin/application_settings_controller_spec.rb
+++ b/spec/controllers/admin/application_settings_controller_spec.rb
@@ -116,6 +116,34 @@ describe Admin::ApplicationSettingsController do
         end
       end
     end
+
+    describe 'verify panel actions' do
+      shared_examples 'renders correct panels' do
+        it 'renders correct action on error' do
+          allow_any_instance_of(ApplicationSettings::UpdateService).to receive(:execute).and_return(false)
+
+          patch action, params: { application_setting: { unused_param: true } }
+
+          expect(subject).to render_template(action)
+        end
+
+        it 'redirects to same panel on success' do
+          allow_any_instance_of(ApplicationSettings::UpdateService).to receive(:execute).and_return(true)
+          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|
+        it_behaves_like 'renders correct panels' do
+          let(:action) { valid_action }
+        end
+      end
+    end
   end
 
   describe 'PUT #reset_registration_token' do
diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb
index 93ccb03d822..45ef5d07ff0 100644
--- a/spec/features/admin/admin_settings_spec.rb
+++ b/spec/features/admin/admin_settings_spec.rb
@@ -379,6 +379,27 @@ describe 'Admin updates settings' do
       expect(page).to have_content "Application settings saved successfully"
     end
 
+    it 'Change Real-time features settings' do
+      page.within('.as-realtime') do
+        fill_in 'Polling interval multiplier', with: 5.0
+        click_button 'Save changes'
+      end
+
+      expect(Gitlab::CurrentSettings.polling_interval_multiplier).to eq 5.0
+      expect(page).to have_content "Application settings saved successfully"
+    end
+
+    it 'shows an error when validation fails' do
+      page.within('.as-realtime') do
+        fill_in 'Polling interval multiplier', with: -1.0
+        click_button 'Save changes'
+      end
+
+      expect(Gitlab::CurrentSettings.polling_interval_multiplier).not_to eq(-1.0)
+      expect(page)
+        .to have_content "The form contains the following error: Polling interval multiplier must be greater than or equal to 0"
+    end
+
     context 'When pages_auto_ssl is enabled' do
       before do
         stub_feature_flags(pages_auto_ssl: true)