Merge branch 'return-error-message-when-performance-bar-group-is-not-found' into 'master'

Return error message when performance bar group is not found

See merge request gitlab-org/gitlab!18444

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 = process_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)
@@ -65,12 +69,27 @@ module ApplicationSettings
       @application_setting.reset_memoized_terms
     end
 
-    def performance_bar_allowed_group_id
-      performance_bar_enabled = !params.key?(:performance_bar_enabled) || params.delete(:performance_bar_enabled)
+    def process_performance_bar_allowed_group_id
       group_full_path = params.delete(:performance_bar_allowed_group_path)
-      return unless Gitlab::Utils.to_boolean(performance_bar_enabled)
+      enable_param_on = Gitlab::Utils.to_boolean(params.delete(:performance_bar_enabled))
+      performance_bar_enabled = enable_param_on.nil? || enable_param_on # Default to true
+
+      return if group_full_path.blank?
+      return if enable_param_on == false # Explicitly disabling
+
+      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.chomp('/'))
+
+      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

@@ -149,33 +149,42 @@ describe ApplicationSettings::UpdateService do
     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
+          :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_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 +211,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 +238,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)