Ensure ApplicationSetting#performance_bar_allowed_group_id is properly set when retrieved from cache

Signed-off-by: Rémy Coutable <remy@rymai.me>

4 files changed, 20 insertions, 37 deletions

diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index b948e431882..adc423af9e1 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -204,7 +204,7 @@ module ApplicationSettingsHelper
       :pages_domain_verification_enabled,
       :password_authentication_enabled_for_web,
       :password_authentication_enabled_for_git,
-      :performance_bar_allowed_group_id,
+      :performance_bar_allowed_group_path,
       :performance_bar_enabled,
       :plantuml_enabled,
       :plantuml_url,
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index e8ccb320fae..b12f7a2c83f 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -230,6 +230,7 @@ class ApplicationSetting < ActiveRecord::Base
   after_commit do
     reset_memoized_terms
   end
+  after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
 
   def self.defaults
     {
@@ -386,31 +387,6 @@ class ApplicationSetting < ActiveRecord::Base
     super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
   end
 
-  def performance_bar_allowed_group_id=(group_full_path)
-    group_full_path = nil if group_full_path.blank?
-
-    if group_full_path.nil?
-      if group_full_path != performance_bar_allowed_group_id
-        super(group_full_path)
-        Gitlab::PerformanceBar.expire_allowed_user_ids_cache
-      end
-
-      return
-    end
-
-    group = Group.find_by_full_path(group_full_path)
-
-    if group
-      if group.id != performance_bar_allowed_group_id
-        super(group.id)
-        Gitlab::PerformanceBar.expire_allowed_user_ids_cache
-      end
-    else
-      super(nil)
-      Gitlab::PerformanceBar.expire_allowed_user_ids_cache
-    end
-  end
-
   def performance_bar_allowed_group
     Group.find_by_id(performance_bar_allowed_group_id)
   end
@@ -420,15 +396,6 @@ class ApplicationSetting < ActiveRecord::Base
     performance_bar_allowed_group_id.present?
   end
 
-  # - If `enable` is true, we early return since the actual attribute that holds
-  #   the enabling/disabling is `performance_bar_allowed_group_id`
-  # - If `enable` is false, we set `performance_bar_allowed_group_id` to `nil`
-  def performance_bar_enabled=(enable)
-    return if Gitlab::Utils.to_boolean(enable)
-
-    self.performance_bar_allowed_group_id = nil
-  end
-
   # Choose one of the available repository storage options. Currently all have
   # equal weighting.
   def pick_repository_storage
@@ -506,4 +473,8 @@ class ApplicationSetting < ActiveRecord::Base
 
     errors.add(:terms, "You need to set terms to be enforced") unless terms.present?
   end
+
+  def expire_performance_bar_allowed_user_ids_cache
+    Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+  end
 end
diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb
index d6d3a661dab..e70445cfb67 100644
--- a/app/services/application_settings/update_service.rb
+++ b/app/services/application_settings/update_service.rb
@@ -3,6 +3,10 @@ module ApplicationSettings
     def execute
       update_terms(@params.delete(:terms))
 
+      if params.key?(:performance_bar_allowed_group_path)
+        params[:performance_bar_allowed_group_id] = performance_bar_allowed_group_id
+      end
+
       @application_setting.update(@params)
     end
 
@@ -18,5 +22,13 @@ module ApplicationSettings
       ApplicationSetting::Term.create(terms: terms)
       @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)
+      group_full_path = params.delete(:performance_bar_allowed_group_path)
+      return nil unless Gitlab::Utils.to_boolean(performance_bar_enabled)
+
+      Group.find_by_full_path(group_full_path)&.id if group_full_path.present?
+    end
   end
 end
diff --git a/app/views/admin/application_settings/_performance_bar.html.haml b/app/views/admin/application_settings/_performance_bar.html.haml
index 8001b42e2f9..030e8610b47 100644
--- a/app/views/admin/application_settings/_performance_bar.html.haml
+++ b/app/views/admin/application_settings/_performance_bar.html.haml
@@ -9,8 +9,8 @@
             = f.check_box :performance_bar_enabled
             Enable the Performance Bar
     .form-group.row
-      = f.label :performance_bar_allowed_group_id, 'Allowed group', class: 'col-form-label col-sm-2'
+      = f.label :performance_bar_allowed_group_path, 'Allowed group', class: 'col-form-label col-sm-2'
       .col-sm-10
-        = f.text_field :performance_bar_allowed_group_id, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
+        = f.text_field :performance_bar_allowed_group_path, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
 
   = f.submit 'Save changes', class: "btn btn-success"