FIX: Badge and user title interaction fixes (#8282)

FIX: Badge and user title interaction fixes (#8282)

  • Fix user title logic when badge name customized
  • Fix an issue where a user’s title was not considered a badge granted title when the user used a badge for their title and the badge name was customized. this affected the effectiveness of revoke_ungranted_titles! which only operates on badge_granted_titles.
  • When a user’s title is set now it is considered a badge_granted_title if the badge name OR the badge custom name from TranslationOverride is the same as the title
  • When a user’s badge is revoked we now also revoke their title if the user’s title matches the badge name OR the badge custom name from TranslationOverride
  • Add a user history log when the title is revoked to remove confusion about why titles are revoked
  • Add granted_title_badge_id to user_profile, now when we set badge_granted_title on a user profile when updating a user’s title based on a badge, we also remember which badge matched the title
  • When badge name (or custom text) changes update titles of users in a background job
  • When the name of a badge changes, or in the case of system badges when their custom translation text changes, then we need to update the title of all corresponding users who have a badge_granted_title and matching granted_title_badge_id. In the case of system badges we need to first get the proper badge ID based on the translation key e.g. badges.regular.name
  • Add migration to backfill all granted_title_badge_ids for both normal badge name titles and titles using custom badge text.
diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb
index 0a1b741fd4..fe67475e26 100644
--- a/app/controllers/admin/badges_controller.rb
+++ b/app/controllers/admin/badges_controller.rb
@@ -125,6 +125,15 @@ class Admin::BadgesController < Admin::AdminController
       badge.save!
     end
 
+    if opts[:new].blank?
+      Jobs.enqueue(
+        :bulk_user_title_update,
+        new_title: badge.name,
+        granted_badge_id: badge.id,
+        action: Jobs::BulkUserTitleUpdate::UPDATE_ACTION
+      )
+    end
+
     errors
   rescue ActiveRecord::RecordInvalid
     errors.push(*badge.errors.full_messages)
diff --git a/app/controllers/admin/site_texts_controller.rb b/app/controllers/admin/site_texts_controller.rb
index ef594471d9..0f9d7bad65 100644
--- a/app/controllers/admin/site_texts_controller.rb
+++ b/app/controllers/admin/site_texts_controller.rb
@@ -59,6 +59,15 @@ class Admin::SiteTextsController < Admin::AdminController
 
     if translation_override.errors.empty?
       StaffActionLogger.new(current_user).log_site_text_change(id, value, old_value)
+      system_badge_id = Badge.find_system_badge_id_from_translation_key(id)
+      if system_badge_id.present?
+        Jobs.enqueue(
+          :bulk_user_title_update,
+          new_title: value,
+          granted_badge_id: system_badge_id,
+          action: Jobs::BulkUserTitleUpdate::UPDATE_ACTION
+        )
+      end
       render_serialized(site_text, SiteTextSerializer, root: 'site_text', rest_serializer: true)
     else
       render json: failed_json.merge(
@@ -69,10 +78,19 @@ class Admin::SiteTextsController < Admin::AdminController
 
   def revert
     site_text = find_site_text
-    old_text = I18n.t(site_text[:id])
-    TranslationOverride.revert!(I18n.locale, site_text[:id])
+    id = site_text[:id]
+    old_text = I18n.t(id)
+    TranslationOverride.revert!(I18n.locale, id)
     site_text = find_site_text
-    StaffActionLogger.new(current_user).log_site_text_change(site_text[:id], site_text[:value], old_text)
+    StaffActionLogger.new(current_user).log_site_text_change(id, site_text[:value], old_text)
+    system_badge_id = Badge.find_system_badge_id_from_translation_key(id)
+    if system_badge_id.present?
+      Jobs.enqueue(
+        :bulk_user_title_update,
+        granted_badge_id: system_badge_id,
+        action: Jobs::BulkUserTitleUpdate::RESET_ACTION
+      )
+    end
     render_serialized(site_text, SiteTextSerializer, root: 'site_text', rest_serializer: true)
   end
 
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 5498a9933b..5a113ca717 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -195,14 +195,36 @@ class UsersController < ApplicationController
     guardian.ensure_can_edit!(user)
 
     user_badge = UserBadge.find_by(id: params[:user_badge_id])
+    previous_title = user.title
     if user_badge && user_badge.user == user && user_badge.badge.allow_title?
       user.title = user_badge.badge.display_name
-      user.user_profile.badge_granted_title = true
       user.save!
-      user.user_profile.save!
+
+      log_params = {
+        details: "title matching badge id #{user_badge.badge.id}",
+        previous_value: previous_title,
+        new_value: user.title
+      }
+
+      if current_user.staff? && current_user != user
+        StaffActionLogger.new(current_user).log_title_change(user, log_params)
+      else
+        UserHistory.create!(log_params.merge(target_user_id: user.id, action: UserHistory.actions[:change_title]))
+      end
     else
       user.title = ''
       user.save!
+
+      log_params = {
+        revoke_reason: 'user title was same as revoked badge name or custom badge name',
+        previous_value: previous_title
+      }
+
+      if current_user.staff? && current_user != user
+        StaffActionLogger.new(current_user).log_title_revoke(user, log_params)
+      else
+        UserHistory.create!(log_params.merge(target_user_id: user.id, action: UserHistory.actions[:revoke_title]))
+      end
     end
 
     render body: nil
diff --git a/app/jobs/regular/bulk_user_title_update.rb b/app/jobs/regular/bulk_user_title_update.rb
new file mode 100644
index 0000000000..97d3f68ad5
--- /dev/null
+++ b/app/jobs/regular/bulk_user_title_update.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module Jobs
+  class BulkUserTitleUpdate < ::Jobs::Base
+    UPDATE_ACTION = 'update'.freeze
+    RESET_ACTION = 'reset'.freeze
+
+    def execute(args)
+      new_title = args[:new_title]
+      granted_badge_id = args[:granted_badge_id]
+      action = args[:action]
+
+      case action
+      when UPDATE_ACTION
+        update_titles_for_granted_badge(new_title, granted_badge_id)
+      when RESET_ACTION
+        reset_titles_for_granted_badge(granted_badge_id)
+      end
+    end
+
+    private
+
+    ##
+    # If a badge name or a system badge TranslationOverride changes
+    # then we need to set all titles granted based on that badge to
+    # the new name or custom translation
+    def update_titles_for_granted_badge(new_title, granted_badge_id)
+      DB.exec(<<~SQL, granted_title_badge_id: granted_badge_id, title: new_title, updated_at: Time.now)
+        UPDATE users AS u
+        SET title = :title, updated_at = :updated_at
+        FROM user_profiles AS up
+        WHERE up.user_id = u.id AND up.granted_title_badge_id = :granted_title_badge_id
+      SQL
+    end
+
+    ##
+    # Reset granted titles for a badge back to the original
+    # badge name. When a system badge has its TranslationOverride
+    # revoked we want to have all titles based on that translation
+    # for the badge reset.
+    def reset_titles_for_granted_badge(granted_badge_id)
+      DB.exec(<<~SQL, granted_title_badge_id: granted_badge_id, updated_at: Time.now)
+        UPDATE users AS u
+        SET title = badges.name, updated_at = :updated_at
+        FROM user_profiles AS up
+        INNER JOIN badges ON badges.id = up.granted_title_badge_id
+        WHERE up.user_id = u.id AND up.granted_title_badge_id = :granted_title_badge_id
+      SQL
+    end
+  end
+end
diff --git a/app/models/badge.rb b/app/models/badge.rb
index 3596190a8a..4db471bcbe 100644
--- a/app/models/badge.rb
+++ b/app/models/badge.rb
@@ -169,8 +169,17 @@ class Badge < ActiveRecord::Base
   end
 
   def self.display_name(name)
-    key = "badges.#{i18n_name(name)}.name"
-    I18n.t(key, default: name)
+    I18n.t(i18n_key(name), default: name)
+  end
+
+  def self.i18n_key(name)
+    "badges.#{i18n_name(name)}.name"
+  end
+
+  def self.find_system_badge_id_from_translation_key(translation_key)
+    return unless translation_key.starts_with?('badges.')
+    badge_name_klass = translation_key.split('.').second.camelize
+    "Badge::#{badge_name_klass}".constantize
   end
 
   def awarded_for_trust_level?
@@ -208,6 +217,10 @@ class Badge < ActiveRecord::Base
     self.class.display_name(name)
   end
 
+  def translation_key
+    self.class.i18n_key(name)
+  end
+
   def long_description
     key = "badges.#{i18n_name}.long_description"
     I18n.t(key, default: self[:long_description] || '', base_uri: Discourse.base_uri)
diff --git a/app/models/user.rb b/app/models/user.rb
index 2ecea38aad..a742682011 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1478,8 +1478,13 @@ class User < ActiveRecord::Base
 
   def check_if_title_is_badged_granted
     if title_changed? && !new_record? && user_profile
-      badge_granted_title = title.present? && badges.where(allow_title: true, name: title).exists?
-      user_profile.update_column(:badge_granted_title, badge_granted_title)
+      badge_matching_title = title && badges.find do |badge|
+        badge.allow_title? && (badge.display_name == title || badge.name == title)
+      end
+      user_profile.update(
+        badge_granted_title: badge_matching_title.present?,
+        granted_title_badge_id: badge_matching_title&.id
+      )
     end

[... diff too long, it was truncated ...]

GitHub sha: 56d3e29a

1 Like

Nitpick, but to prevent postgresql from updating all the rows, we usually add WHERE title <> :title so we are only updating the rows that needs to be updated.