Skip to content
Permalink
Browse files

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.
  • Loading branch information...
martin-brennan committed Nov 8, 2019
1 parent 64b4a7b commit 56d3e29a698e704f647cf2e500d0d94d7d9e21bf
@@ -125,6 +125,15 @@ def update_badge_from_params(badge, opts = {})
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)
@@ -59,6 +59,15 @@ def update

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 @@ def update

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

@@ -195,14 +195,36 @@ def badge_title
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
@@ -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

This comment has been minimized.

Copy link
@ZogStriP

ZogStriP Nov 8, 2019

Member

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.

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
@@ -169,8 +169,17 @@ def self.i18n_name(name)
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 @@ def display_name
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)
@@ -1478,8 +1478,13 @@ def trigger_user_updated_event

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

@@ -101,6 +101,8 @@ def self.actions
api_key_create: 80,
api_key_update: 81,
api_key_destroy: 82,
revoke_title: 83,
change_title: 84
)
end

@@ -175,9 +177,11 @@ def self.staff_actions
: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

@@ -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 @@ def website_domain_validator
# 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)

This comment has been minimized.

Copy link
@danielwaterworth

danielwaterworth Nov 19, 2019

Member

You can update these comments using bundle exec annotate.

#
# 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)
#
@@ -72,8 +72,19 @@ def self.revoke(user_badge, options = {})
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
@@ -352,6 +352,27 @@ def log_badge_revoke(user_badge, opts = {})
))
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(
@@ -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"
@@ -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
@@ -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

0 comments on commit 56d3e29

Please sign in to comment.
You can’t perform that action at this time.