Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Badge and user title interaction fixes #8282

Merged
@@ -125,6 +125,8 @@ def update_badge_from_params(badge, opts = {})
badge.save!
end

BulkUserTitleUpdater.update_titles_for_granted_badge(badge.name, badge.id) if opts[:new].blank?

errors
rescue ActiveRecord::RecordInvalid
errors.push(*badge.errors.full_messages)
@@ -59,6 +59,8 @@ 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)
BulkUserTitleUpdater.update_titles_for_granted_badge(value, system_badge_id) if system_badge_id.present?
render_serialized(site_text, SiteTextSerializer, root: 'site_text', rest_serializer: true)
else
render json: failed_json.merge(
@@ -69,10 +71,13 @@ 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)
BulkUserTitleUpdater.reset_titles_for_granted_badge(system_badge_id) if system_badge_id.present?
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?

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

I think we shouldn't add a staff action log for a staff's own title change. Maybe it should be like if 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?

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Same here. It should be if 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
@@ -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)
@@ -1465,8 +1465,14 @@ 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.present? && badges.find do |badge|
badge.allow_title? && (badge.display_name == title || badge.name == title)
end
user_has_badge_like_title = badge_matching_title.present?
user_profile.update(
badge_granted_title: user_has_badge_like_title,
granted_title_badge_id: user_has_badge_like_title ? badge_matching_title.id : nil

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 7, 2019

Member

Super minor but you can do badge_matching_title&.id here instead of inline if.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 7, 2019

Author Contributor

No problem. We had banned the safe navigation operator at my old job so I need to get myself in the habit of using it

)
end
end

@@ -97,7 +97,9 @@ def self.actions
web_hook_deactivate: 76,
change_theme_setting: 77,
disable_theme_component: 78,
enable_theme_component: 79
enable_theme_component: 79,
revoke_title: 80,
change_title: 81
)
end

@@ -171,7 +173,9 @@ def self.staff_actions
:embeddable_host_destroy,
:change_theme_setting,
:disable_theme_component,
:enable_theme_component
:enable_theme_component,
:revoke_title,
:change_title
]
end

@@ -161,15 +161,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)
#
# 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
@@ -0,0 +1,31 @@
# frozen_string_literal: true

class BulkUserTitleUpdater
##
# 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 self.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 self.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
@@ -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(
@@ -3913,6 +3913,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"
screened_emails:
title: "Screened Emails"
description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."
@@ -0,0 +1,11 @@
# 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
end

def down
remove_column :user_profiles, :granted_title_badge_id
end
end
@@ -0,0 +1,36 @@
# frozen_string_literal: true
class BackfillGrantedTitleBadgeIdForUserProfile < ActiveRecord::Migration[6.0]

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Instead of creating a separate migration class, we can add the backfill in AddGrantedTitleBadgeIdToUserProfile class itself like this.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 6, 2019

Author Contributor

No problem, will do that.

def up
# 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.title = b.name

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Why we checking the users.title = b.name both in inner join condition and where condition?

AND user_profiles.granted_title_badge_id IS NULL
AND users.id = user_profiles.user_id
AND users.title IS NOT NULL AND users.title != ''

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

I think we don't have to check the users.title here again, since we already checked it like users.title = b.name. If it's empty it won't even satisfy the existing inner join condition.

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.title = translation_overrides.value
AND user_profiles.granted_title_badge_id IS NULL
AND users.id = user_profiles.user_id
AND users.title IS NOT NULL AND users.title != ''
SQL
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
@@ -95,6 +95,33 @@
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)
@@ -1997,12 +1997,35 @@ def filter_by(method)
expect(user.user_profile.reload.badge_granted_title).to eq(false)

badge.update!(allow_title: true)
user.badges.reload

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

We don't need to reload the badges here. Since we only set the title of user.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 6, 2019

Author Contributor

I do need to do that because the spec fails without doing so. The User#check_if_title_is_badged_granted method uses ruby Enumerable#find to save trip to the DB https://devdocs.io/ruby~2.6/enumerable#method-i-find.

badge_matching_title = title.present? && badges.find do |badge|
  badge.allow_title? && (badge.display_name == title || badge.name == title)
end

Because the user's badges AR association is already loaded it will not query the DB again to fetch the updated allow_title. I can alternatively call user.badges.find { |b| b.id == badge.id }.update!(allow_title: true) but I think that's much of a muchness really. Or I can change the User model code to fetch the badges from the db instead of using badges.find. Is there a downside to reloading that I'm not aware of?

This comment has been minimized.

Copy link
@martin-brennan
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)
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

describe '#next_best_title' do
@@ -153,6 +153,24 @@
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' do
put "/admin/badges/#{badge.id}.json", params: {
name: "Shieldbearer"
}

expect(user_with_badge_title.reload.title).to eq('Shieldbearer')
end
end
end
end
end
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.