diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb index 0a1b741fd45..fe67475e26a 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 ef594471d9f..0f9d7bad657 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 5498a9933ba..5a113ca717a 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 00000000000..97d3f68ad53 --- /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 3596190a8a2..4db471bcbeb 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 2ecea38aade..a7426820110 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 end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index ec6fec4f11e..dcb11d5ea60 100644 --- a/app/models/user_history.rb +++ b/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 diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index 7f16cfe90eb..ed50e908534 100644 --- a/app/models/user_profile.rb +++ b/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) # diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index b2f302f9547..c7c6049ddbf 100644 --- a/app/services/badge_granter.rb +++ b/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 diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index f6264da3eb4..c6cbb48d0fe 100644 --- a/app/services/staff_action_logger.rb +++ b/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( diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 125fae82231..e2eeb2590d5 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/db/migrate/20191031052711_add_granted_title_badge_id_to_user_profile.rb b/db/migrate/20191031052711_add_granted_title_badge_id_to_user_profile.rb new file mode 100644 index 00000000000..f76ea34ef49 --- /dev/null +++ b/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 diff --git a/spec/jobs/regular/bulk_user_title_update_spec.rb b/spec/jobs/regular/bulk_user_title_update_spec.rb new file mode 100644 index 00000000000..f6cf97eb38c --- /dev/null +++ b/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 diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index 41cf3edcfd0..bd1639a69c6 100644 --- a/spec/models/badge_spec.rb +++ b/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) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 73eb8d7cacf..f34a895b444 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/requests/admin/badges_controller_spec.rb b/spec/requests/admin/badges_controller_spec.rb index a13af50bc93..5ea9d5fd144 100644 --- a/spec/requests/admin/badges_controller_spec.rb +++ b/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 diff --git a/spec/requests/admin/site_texts_controller_spec.rb b/spec/requests/admin/site_texts_controller_spec.rb index fcc3be2aedc..356dcfc07cd 100644 --- a/spec/requests/admin/site_texts_controller_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 0d454970f3b..276901b6d12 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index 5063cba02bf..1606a27ea88 100644 --- a/spec/services/badge_granter_spec.rb +++ b/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