Show errors for invalid performance bar group ID

We should show errors in two cases:

1. The group path is set, but the performance bar isn't enabled.
2. The group path is set, the performance bar is enabled, but the group
   doesn't exist.

If the performance bar is disabled and the group path is not set, that's
valid because it's disabling the performance bar.

app/services/application_settings/update_service.rb

@@ -20,7 +20,11 @@ module ApplicationSettings
       add_to_outbound_local_requests_whitelist(@params.delete(:add_to_outbound_local_requests_whitelist))
 
       if params.key?(:performance_bar_allowed_group_path)
-        params[:performance_bar_allowed_group_id] = performance_bar_allowed_group_id
+        group_id = performance_bar_allowed_group_id
+
+        return false if application_setting.errors.any?
+
+        params[:performance_bar_allowed_group_id] = group_id
       end
 
       if usage_stats_updated? && !params.delete(:skip_usage_stats_user)
@@ -66,11 +70,28 @@ module ApplicationSettings
     end
 
     def performance_bar_allowed_group_id
-      performance_bar_enabled = !params.key?(:performance_bar_enabled) || params.delete(:performance_bar_enabled)
       group_full_path = params.delete(:performance_bar_allowed_group_path)
-      return unless Gitlab::Utils.to_boolean(performance_bar_enabled)
+      enable_param_set = params.key?(:performance_bar_enabled)
+      enable_param_on = Gitlab::Utils.to_boolean(params.delete(:performance_bar_enabled))
+      performance_bar_disabled_by_user = enable_param_set && !enable_param_on
+      performance_bar_enabled = !enable_param_set || enable_param_on
+
+      return if group_full_path.blank?
+      return if performance_bar_disabled_by_user
+
+      unless performance_bar_enabled
+        application_setting.errors.add(:performance_bar_allowed_group_id, 'not allowed when performance bar is disabled')
+        return
+      end
+
+      group = Group.find_by_full_path(group_full_path)
+
+      unless group
+        application_setting.errors.add(:performance_bar_allowed_group_id, 'not found')
+        return
+      end
 
-      Group.find_by_full_path(group_full_path)&.id if group_full_path.present?
+      group.id
     end
 
     def bypass_external_auth?

changelogs/unreleased/return-error-message-when-performance-bar-group-is-not-found.yml

+---
+title: Show error message when setting an invalid group ID for the performance bar
+merge_request:
+author:
+type: fixed

spec/services/application_settings/update_service_spec.rb

@@ -147,35 +147,43 @@ describe ApplicationSettings::UpdateService do
     using RSpec::Parameterized::TableSyntax
 
     where(:params_performance_bar_enabled,
-      :params_performance_bar_allowed_group_path,
-      :previous_performance_bar_allowed_group_id,
-      :expected_performance_bar_allowed_group_id) do
-      true | '' | nil | nil
-      true | '' | 42_000_000 | nil
-      true | nil | nil | nil
-      true | nil | 42_000_000 | nil
-      true | 'foo' | nil | nil
-      true | 'foo' | 42_000_000 | nil
-      true | 'group_a' | nil | 42_000_000
-      true | 'group_b' | 42_000_000 | 43_000_000
-      true | 'group_a' | 42_000_000 | 42_000_000
-      false | '' | nil | nil
-      false | '' | 42_000_000 | nil
-      false | nil | nil | nil
-      false | nil | 42_000_000 | nil
-      false | 'foo' | nil | nil
-      false | 'foo' | 42_000_000 | nil
-      false | 'group_a' | nil | nil
-      false | 'group_b' | 42_000_000 | nil
-      false | 'group_a' | 42_000_000 | nil
+          :params_performance_bar_allowed_group_path,
+          :previous_performance_bar_allowed_group_id,
+          :expected_performance_bar_allowed_group_id,
+          :expected_valid) do
+      true | '' | nil | nil | true
+      true | '' | 42_000_000 | nil | true
+      true | nil | nil | nil | true
+      true | nil | 42_000_000 | nil | true
+      true | 'foo' | nil | nil | false
+      true | 'foo' | 42_000_000 | 42_000_000 | false
+      true | 'group_a' | nil | 42_000_000 | true
+      true | 'group_b' | 42_000_000 | 43_000_000 | true
+      true | 'group_a' | 42_000_000 | 42_000_000 | true
+      false | '' | nil | nil | true
+      false | '' | 42_000_000 | nil | true
+      false | nil | nil | nil | true
+      false | nil | 42_000_000 | nil | true
+      false | 'foo' | nil | nil | true
+      false | 'foo' | 42_000_000 | nil | true
+      false | 'group_a' | nil | nil | true
+      false | 'group_b' | 42_000_000 | nil | true
+      false | 'group_a' | 42_000_000 | nil | true
+      nil | '' | nil | nil | true
+      nil | 'foo' | nil | nil | false
+      nil | 'group_a' | nil | 42_000_000 | true
     end
 
     with_them do
       let(:params) do
         {
-          performance_bar_enabled: params_performance_bar_enabled,
           performance_bar_allowed_group_path: params_performance_bar_allowed_group_path
-        }
+        }.tap do |params_hash|
+          # Treat nil in the table as missing
+          unless params_performance_bar_enabled.nil?
+            params_hash[:performance_bar_enabled] = params_performance_bar_enabled
+          end
+        end
       end
 
       before do
@@ -202,6 +210,14 @@ describe ApplicationSettings::UpdateService do
             .not_to change(application_settings, :performance_bar_allowed_group_id)
         end
       end
+
+      it 'adds errors to the model for invalid params' do
+        expect(subject.execute).to eq(expected_valid)
+
+        unless expected_valid
+          expect(application_settings.errors[:performance_bar_allowed_group_id]).to be_present
+        end
+      end
     end
 
     context 'when :performance_bar_allowed_group_path is not present' do
@@ -221,7 +237,7 @@ describe ApplicationSettings::UpdateService do
       let(:group) { create(:group) }
       let(:params) { { performance_bar_allowed_group_path: group.full_path } }
 
-      it 'implicitely defaults to true' do
+      it 'implicitly defaults to true' do
         expect { subject.execute }
           .to change(application_settings, :performance_bar_allowed_group_id)
           .from(nil).to(group.id)