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
Next

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
  • Loading branch information
martin-brennan committed Oct 31, 2019
commit 11ffc5a83ea5d08a39bf1e83748509d02d751423
@@ -169,8 +169,11 @@ 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 awarded_for_trust_level?
@@ -208,6 +211,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,10 @@ 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)
user_has_badge_like_title = title.present? && badges.find do |badge|
badge.allow_title? && (badge.display_name == title || badge.name == title)
end.present?
user_profile.update_column(:badge_granted_title, user_has_badge_like_title)
end
end

@@ -97,7 +97,8 @@ 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
)
end

@@ -171,7 +172,8 @@ def self.staff_actions
:embeddable_host_destroy,
:change_theme_setting,
:disable_theme_component,
:enable_theme_component
:enable_theme_component,
:revoke_title
]
end

@@ -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 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,16 @@ 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_check_email(user, opts = {})
raise Discourse::InvalidParameters.new(:user) unless user
UserHistory.create!(params(opts).merge(
@@ -3913,6 +3913,7 @@ en:
change_theme_setting: "change theme setting"
disable_theme_component: "disable theme component"
enable_theme_component: "enable theme component"
revoke_title: "revoke 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."
@@ -1997,12 +1997,33 @@ 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)

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)
end

after do
TranslationOverride.revert!(I18n.locale, Badge.i18n_key(badge.name))
end
end
end

describe '#next_best_title' do
@@ -196,6 +196,33 @@
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 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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.