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.

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)

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
 

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

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

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)

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
   end
 

app/models/user_history.rb

@@ -101,6 +101,8 @@ class UserHistory < ActiveRecord::Base
       api_key_create: 80,
       api_key_update: 81,
       api_key_destroy: 82,
+      revoke_title: 83,
+      change_title: 84
     )
   end
 
@@ -175,9 +177,11 @@ class UserHistory < ActiveRecord::Base
       :change_theme_setting,
       :disable_theme_component,
       :enable_theme_component,
+      :revoke_title,
+      :change_title,
       :api_key_create,
       :api_key_update,
-      :api_key_destroy,
+      :api_key_destroy
     ]
   end
 

app/models/user_profile.rb

@@ -9,6 +9,7 @@ class UserProfile < ActiveRecord::Base
   belongs_to :user, inverse_of: :user_profile
   belongs_to :card_background_upload, class_name: "Upload"
   belongs_to :profile_background_upload, class_name: "Upload"
+  belongs_to :granted_title_badge, class_name: "Badge"
 
   validates :bio_raw, length: { maximum: 3000 }
   validates :website, url: true, allow_blank: true, if: Proc.new { |c| c.new_record? || c.website_changed? }
@@ -161,15 +162,18 @@ end
 #  views                        :integer          default(0), not null
 #  profile_background_upload_id :integer
 #  card_background_upload_id    :integer
+#  granted_title_badge_id       :integer
 #
 # Indexes
 #
 #  index_user_profiles_on_bio_cooked_version  (bio_cooked_version)
 #  index_user_profiles_on_card_background     (card_background)
 #  index_user_profiles_on_profile_background  (profile_background)
+#  index_user_profiles_on_granted_title_badge_id (granted_title_badge)
 #
 # Foreign Keys
 #
 #  fk_rails_...  (card_background_upload_id => uploads.id)
 #  fk_rails_...  (profile_background_upload_id => uploads.id)
+#  fk_rails_...  (granted_title_badge_id => badges.id)
 #

app/services/badge_granter.rb

@@ -72,8 +72,19 @@ class BadgeGranter
         StaffActionLogger.new(options[:revoked_by]).log_badge_revoke(user_badge)
       end
 
-      # If the user's title is the same as the badge name, remove their title.
-      if user_badge.user.title == user_badge.badge.name
+      # If the user's title is the same as the badge name OR the custom badge name, remove their title.
+      custom_badge_name = TranslationOverride.find_by(translation_key: user_badge.badge.translation_key)&.value
+      user_title_is_badge_name = user_badge.user.title == user_badge.badge.name
+      user_title_is_custom_badge_name = custom_badge_name.present? && user_badge.user.title == custom_badge_name
+
+      if user_title_is_badge_name || user_title_is_custom_badge_name
+        if options[:revoked_by]
+          StaffActionLogger.new(options[:revoked_by]).log_title_revoke(
+            user_badge.user,
+            revoke_reason: 'user title was same as revoked badge name or custom badge name',
+            previous_value: user_badge.user.title
+          )
+        end
         user_badge.user.title = nil
         user_badge.user.save!
       end

app/services/staff_action_logger.rb

@@ -352,6 +352,27 @@ class StaffActionLogger
     ))
   end
 
+  def log_title_revoke(user, opts = {})
+    raise Discourse::InvalidParameters.new(:user) unless user
+    UserHistory.create!(params(opts).merge(
+      action: UserHistory.actions[:revoke_title],
+      target_user_id: user.id,
+      details: opts[:revoke_reason],
+      previous_value: opts[:previous_value]
+    ))
+  end
+
+  def log_title_change(user, opts = {})
+    raise Discourse::InvalidParameters.new(:user) unless user
+    UserHistory.create!(params(opts).merge(
+      action: UserHistory.actions[:change_title],
+      target_user_id: user.id,
+      details: opts[:details],
+      new_value: opts[:new_value],
+      previous_value: opts[:previous_value]
+    ))
+  end
+
   def log_check_email(user, opts = {})
     raise Discourse::InvalidParameters.new(:user) unless user
     UserHistory.create!(params(opts).merge(

config/locales/client.en.yml

@@ -3930,6 +3930,8 @@ en:
             change_theme_setting: "change theme setting"
             disable_theme_component: "disable theme component"
             enable_theme_component: "enable theme component"
+            revoke_title: "revoke title"
+            change_title: "change title"
             api_key_create: "api key create"
             api_key_update: "api key update"
             api_key_destroy: "api key destroy"

db/migrate/20191031052711_add_granted_title_badge_id_to_user_profile.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+class AddGrantedTitleBadgeIdToUserProfile < ActiveRecord::Migration[6.0]
+  def up
+    add_reference :user_profiles, :granted_title_badge, foreign_key: { to_table: :badges }, index: true, null: true
+
+    # update all the regular badge derived titles based
+    # on the normal badge name
+    ActiveRecord::Base.connection.execute <<-SQL
+      UPDATE user_profiles
+      SET granted_title_badge_id = b.id
+      FROM users
+      INNER JOIN badges b ON users.title = b.name
+      WHERE users.id = user_profiles.user_id
+        AND user_profiles.granted_title_badge_id IS NULL
+    SQL
+
+    # update all of the system badge derived titles where the
+    # badge has had custom text set for it via TranslationOverride
+    ActiveRecord::Base.connection.execute <<-SQL
+      UPDATE user_profiles
+      SET granted_title_badge_id = badges.id
+      FROM users
+      JOIN translation_overrides ON translation_overrides.value = users.title
+      JOIN badges ON ('badges.' || LOWER(REPLACE(badges.name, ' ', '_')) || '.name') = translation_overrides.translation_key
+      JOIN user_badges ON user_badges.user_id = users.id AND user_badges.badge_id = badges.id
+      WHERE users.id = user_profiles.user_id
+        AND user_profiles.granted_title_badge_id IS NULL
+    SQL
+  end
+
+  def down
+    remove_column :user_profiles, :granted_title_badge_id
+  end
+end

spec/jobs/regular/bulk_user_title_update_spec.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Jobs::BulkUserTitleUpdate do
+  fab!(:badge) { Fabricate(:badge, name: 'Protector of the Realm', allow_title: true) }
+  fab!(:user) { Fabricate(:user) }
+  fab!(:other_user) { Fabricate(:user) }
+
+  describe 'update action' do
+    before do
+      BadgeGranter.grant(badge, user)
+      user.update(title: badge.name)
+    end
+
+    it 'updates the title of all users with the attached granted title badge id on their profile' do
+      execute_update
+      expect(user.reload.title).to eq('King of the Forum')
+    end
+
+    it 'does not set the title for any other users' do
+      execute_update
+      expect(other_user.reload.title).not_to eq('King of the Forum')
+    end
+
+    def execute_update
+      described_class.new.execute(new_title: 'King of the Forum', granted_badge_id: badge.id, action: described_class::UPDATE_ACTION)
+    end
+  end
+
+  describe 'reset action' do
+    let(:customized_badge_name) { 'Merit Badge' }
+
+    before do
+      TranslationOverride.upsert!(I18n.locale, Badge.i18n_key(badge.name), customized_badge_name)
+      BadgeGranter.grant(badge, user)
+      user.update(title: customized_badge_name)
+    end
+
+    it 'updates the title of all users back to the original badge name' do
+      expect(user.reload.title).to eq(customized_badge_name)
+      described_class.new.execute(granted_badge_id: badge.id, action: described_class::RESET_ACTION)
+      expect(user.reload.title).to eq('Protector of the Realm')
+    end
+
+    after do
+      TranslationOverride.revert!(I18n.locale, Badge.i18n_key(badge.name))
+    end
+  end
+end

spec/models/badge_spec.rb

@@ -95,6 +95,33 @@ describe Badge do
     end
   end
 
+  describe '.find_system_badge_id_from_translation_key' do
+    let(:translation_key) { 'badges.regular.name' }
+
+    it 'uses a translation key to get a system badge id, mainly to find which badge a translation override corresponds to' do
+      expect(Badge.find_system_badge_id_from_translation_key(translation_key)).to eq(
+        Badge::Regular
+      )
+    end
+
+    context 'when the translation key is snake case' do
+      let(:translation_key) { 'badges.crazy_in_love.name' }
+
+      it 'works to get the badge' do
+        expect(Badge.find_system_badge_id_from_translation_key(translation_key)).to eq(
+          Badge::CrazyInLove
+        )
+      end
+    end
+
+    context 'when a translation key not for a badge is provided' do
+      let(:translation_key) { 'reports.flags.title' }
+      it 'returns nil' do
+        expect(Badge.find_system_badge_id_from_translation_key(translation_key)).to eq(nil)
+      end
+    end
+  end
+
   context "First Quote" do
     let(:quoted_post_badge) do
       Badge.find(Badge::FirstQuote)

spec/models/user_spec.rb

@@ -1957,11 +1957,35 @@ describe User do
       expect(user.user_profile.reload.badge_granted_title).to eq(false)
 
       badge.update!(allow_title: true)
+      user.badges.reload
       user.update!(title: badge.name)
       expect(user.user_profile.reload.badge_granted_title).to eq(true)
+      expect(user.user_profile.reload.granted_title_badge_id).to eq(badge.id)
 
       user.update!(title: nil)
       expect(user.user_profile.reload.badge_granted_title).to eq(false)
+      expect(user.user_profile.granted_title_badge_id).to eq(nil)
+    end
+
+    context 'when a custom badge name has been set and it matches the title' do
+      let(:customized_badge_name) { 'Merit Badge' }
+
+      before do
+        TranslationOverride.upsert!(I18n.locale, Badge.i18n_key(badge.name), customized_badge_name)
+      end
+
+      it 'sets badge_granted_title correctly' do
+        BadgeGranter.grant(badge, user)
+
+        badge.update!(allow_title: true)
+        user.update!(title: customized_badge_name)
+        expect(user.user_profile.reload.badge_granted_title).to eq(true)
+        expect(user.user_profile.reload.granted_title_badge_id).to eq(badge.id)
+      end
+
+      after do
+        TranslationOverride.revert!(I18n.locale, Badge.i18n_key(badge.name))
+      end
     end
   end
 

spec/requests/admin/badges_controller_spec.rb

@@ -153,6 +153,29 @@ describe Admin::BadgesController do
         expect(badge.name).to eq('123456')
         expect(badge.query).to eq(sql)
       end
+
+      context 'when there is a user with a title granted using the badge' do
+        fab!(:user_with_badge_title) { Fabricate(:active_user) }
+        fab!(:badge) { Fabricate(:badge, name: 'Oathbreaker', allow_title: true) }
+
+        before do
+          BadgeGranter.grant(badge, user_with_badge_title)
+          user_with_badge_title.update(title: 'Oathbreaker')
+        end
+
+        it 'updates the user title in a job' do
+          Jobs.expects(:enqueue).with(
+            :bulk_user_title_update,
+            new_title: 'Shieldbearer',
+            granted_badge_id: badge.id,
+            action: Jobs::BulkUserTitleUpdate::UPDATE_ACTION
+          )
+
+          put "/admin/badges/#{badge.id}.json", params: {
+            name: "Shieldbearer"
+          }
+        end
+      end
     end
   end
 end

spec/requests/admin/site_texts_controller_spec.rb

@@ -415,6 +415,37 @@ RSpec.describe Admin::SiteTextsController do
         json = ::JSON.parse(response.body)
         expect(json['site_text']['value']).to_not eq(ru_mf_text)
       end
+
+      context 'when updating a translation override for a system badge' do
+        fab!(:user_with_badge_title) { Fabricate(:active_user) }
+        let(:badge) { Badge.find(Badge::Regular) }
+
+        before do
+          BadgeGranter.grant(badge, user_with_badge_title)
+          user_with_badge_title.update(title: 'Regular')
+        end
+
+        it 'updates matching user titles to the override text in a job' do
+          Jobs.expects(:enqueue).with(
+            :bulk_user_title_update,
+            new_title: 'Terminator',
+            granted_badge_id: badge.id,
+            action: Jobs::BulkUserTitleUpdate::UPDATE_ACTION
+          )
+          put '/admin/customize/site_texts/badges.regular.name.json', params: {
+            site_text: { value: 'Terminator' }
+          }
+
+          Jobs.expects(:enqueue).with(
+            :bulk_user_title_update,
+            granted_badge_id: badge.id,
+            action: Jobs::BulkUserTitleUpdate::RESET_ACTION
+          )
+
+          # Revert
+          delete "/admin/customize/site_texts/badges.regular.name.json"
+        end
+      end
     end
 
     context "reseeding" do

spec/requests/users_controller_spec.rb

@@ -1911,11 +1911,17 @@ describe UsersController do
 
       expect(user.reload.title).to eq(badge.display_name)
       expect(user.user_profile.badge_granted_title).to eq(true)
+      expect(user.user_profile.granted_title_badge_id).to eq(badge.id)
 
-      user.title = "testing"
-      user.save
+      badge.update allow_title: false
+
+      put "/u/#{user.username}/preferences/badge_title.json", params: { user_badge_id: user_badge.id }
+
+      user.reload
       user.user_profile.reload
+      expect(user.title).to eq('')
       expect(user.user_profile.badge_granted_title).to eq(false)
+      expect(user.user_profile.granted_title_badge_id).to eq(nil)
     end
 
     context "with overrided name" do

spec/services/badge_granter_spec.rb

@@ -196,6 +196,33 @@ describe BadgeGranter do
       expect(user.reload.title).to eq(nil)
     end
 
+    context 'when the badge name is customized, and the customized name is the same as the user title' do
+      let(:customized_badge_name) { 'Merit Badge' }
+
+      before do
+        TranslationOverride.upsert!(I18n.locale, Badge.i18n_key(badge.name), customized_badge_name)
+      end
+
+      it 'revokes the badge and title and does necessary cleanup' do
+        user.title = customized_badge_name; user.save!
+        expect(badge.reload.grant_count).to eq(1)
+        StaffActionLogger.any_instance.expects(:log_badge_revoke).with(user_badge)
+        StaffActionLogger.any_instance.expects(:log_title_revoke).with(
+          user,
+          revoke_reason: 'user title was same as revoked badge name or custom badge name',
+          previous_value: user_badge.user.title
+        )
+        BadgeGranter.revoke(user_badge, revoked_by: admin)
+        expect(UserBadge.find_by(user: user, badge: badge)).not_to be_present
+        expect(badge.reload.grant_count).to eq(0)
+        expect(user.notifications.where(notification_type: Notification.types[:granted_badge])).to be_empty
+        expect(user.reload.title).to eq(nil)
+      end
+
+      after do
+        TranslationOverride.revert!(I18n.locale, Badge.i18n_key(badge.name))
+      end
+    end
   end
 
   context "update_badges" do