From e4023943758c6f1dee560cae5767da84185a12b1 Mon Sep 17 00:00:00 2001 From: Kyle Zhao Date: Fri, 21 Sep 2018 10:06:08 +0800 Subject: [PATCH] FEATURE: auto grant an available title when removing old title * FEATURE: auto grant an available title when removing old title --- app/models/badge.rb | 15 ++++++-- app/models/group_user.rb | 13 ++----- app/models/user.rb | 28 ++++++++++---- app/services/user_updater.rb | 3 -- spec/models/badge_spec.rb | 17 +++++++++ spec/models/group_spec.rb | 21 ++++++++--- spec/models/user_spec.rb | 71 ++++++++++++++++++++++++++++++++++++ 7 files changed, 139 insertions(+), 29 deletions(-) diff --git a/app/models/badge.rb b/app/models/badge.rb index ed73631eab8..7ab289cf134 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -159,6 +159,15 @@ class Badge < ActiveRecord::Base SQL end + def self.i18n_name(name) + name.downcase.tr(' ', '_') + end + + def self.display_name(name) + key = "badges.#{i18n_name(name)}.name" + I18n.t(key, default: name) + end + def awarded_for_trust_level? id <= 4 end @@ -191,8 +200,7 @@ class Badge < ActiveRecord::Base end def display_name - key = "badges.#{i18n_name}.name" - I18n.t(key, default: self.name) + self.class.display_name(name) end def long_description @@ -230,9 +238,8 @@ class Badge < ActiveRecord::Base end def i18n_name - self.name.downcase.tr(' ', '_') + @i18n_name ||= self.class.i18n_name(name) end - end # == Schema Information diff --git a/app/models/group_user.rb b/app/models/group_user.rb index ed528b3d3a0..330290a3fe6 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -5,7 +5,7 @@ class GroupUser < ActiveRecord::Base belongs_to :user after_save :update_title - after_destroy :remove_title + after_destroy :grant_other_available_title after_save :set_primary_group after_destroy :remove_primary_group, :recalculate_trust_level @@ -43,13 +43,9 @@ class GroupUser < ActiveRecord::Base ) end - def remove_title - if group.title.present? - DB.exec(" - UPDATE users SET title = NULL - WHERE title = :title AND id = :id", - id: user_id, title: group.title - ) + def grant_other_available_title + if group.title.present? && group.title == user.title + user.update!(title: user.next_best_title) end end @@ -85,7 +81,6 @@ class GroupUser < ActiveRecord::Base user.update!(group_locked_trust_level: highest_level) Promotion.recalculate(user) end - end # == Schema Information diff --git a/app/models/user.rb b/app/models/user.rb index 581528e75f1..f6583ad2cd8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -110,6 +110,7 @@ class User < ActiveRecord::Base before_save :update_username_lower before_save :ensure_password_is_hashed before_save :match_title_to_primary_group_changes + before_save :check_if_title_is_badged_granted after_save :expire_tokens_if_password_changed after_save :clear_global_notice_if_needed @@ -1000,13 +1001,6 @@ class User < ActiveRecord::Base @user_fields end - def title=(val) - write_attribute(:title, val) - if !new_record? && user_profile - user_profile.update_column(:badge_granted_title, false) - end - end - def number_of_deleted_posts Post.with_deleted .where(user_id: self.id) @@ -1123,6 +1117,19 @@ class User < ActiveRecord::Base from_staged? && self.created_at && self.created_at < 1.day.ago end + def next_best_title + group_titles_query = groups.where("groups.title <> ''") + group_titles_query = group_titles_query.order("groups.id = #{primary_group_id} DESC") if primary_group_id + group_titles_query = group_titles_query.order("groups.primary_group DESC").limit(1) + + if next_best_group_title = group_titles_query.pluck(:title).first + return next_best_group_title + end + + next_best_badge_title = badges.where(allow_title: true).limit(1).pluck(:name).first + next_best_badge_title ? Badge.display_name(next_best_badge_title) : nil + end + protected def badge_grant @@ -1285,6 +1292,13 @@ class User < ActiveRecord::Base private + 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) + end + end + def previous_visit_at_update_required?(timestamp) seen_before? && (last_seen_at < (timestamp - SiteSetting.previous_visit_timeout_hours.hours)) end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index de238e50ec0..8724bc5b906 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -66,9 +66,6 @@ class UserUpdater attributes[:title] != user.title && guardian.can_grant_title?(user, attributes[:title]) user.title = attributes[:title] - if user.badges.where(name: user.title).exists? - user_profile.badge_granted_title = true - end end CATEGORY_IDS.each do |attribute, level| diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index cbac526cb7f..5bc1d892eb2 100644 --- a/spec/models/badge_spec.rb +++ b/spec/models/badge_spec.rb @@ -80,4 +80,21 @@ describe Badge do it { is_expected.to be true } end end + + describe '.i18n_name' do + it 'transforms to lower case letters, and replaces spaces with underscores' do + expect(Badge.i18n_name('Basic User')).to eq('basic_user') + end + end + + describe '.display_name' do + it 'fetches from translations when i18n_name key exists' do + expect(Badge.display_name('basic_user')).to eq('Basic') + expect(Badge.display_name('Basic User')).to eq('Basic') + end + + it 'fallbacks to argument value when translation does not exist' do + expect(Badge.display_name('Not In Translations')).to eq('Not In Translations') + end + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 5d3aea6fdb2..33cd25b8c0c 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -675,13 +675,22 @@ describe Group do describe '#remove' do before { group.add(user) } - it "only strips user's title if exact match" do - group.update(title: 'Awesome') - expect { group.remove(user) }.to change { user.reload.title }.from('Awesome').to(nil) + context 'when stripping title' do + it "only strips user's title if exact match" do + group.update!(title: 'Awesome') + expect { group.remove(user) }.to change { user.reload.title }.from('Awesome').to(nil) - group.add(user) - user.update_columns(title: 'Different') - expect { group.remove(user) }.to_not change { user.reload.title } + group.add(user) + user.update_columns(title: 'Different') + expect { group.remove(user) }.to_not change { user.reload.title } + end + + it "grants another title when the user has other available titles" do + group.update!(title: 'Awesome') + Fabricate(:group, title: 'Super').add(user) + + expect { group.remove(user) }.to change { user.reload.title }.from('Awesome').to('Super') + end end it "unsets the user's primary group" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4516ff0c6f8..04338ffb6da 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1826,4 +1826,75 @@ describe User do expect { user.update(primary_group: primary_group_a) }.to_not change { user.reload.title } end end + + describe '#title=' do + let(:badge) { Fabricate(:badge, name: 'Badge', allow_title: false) } + + it 'sets badge_granted_title correctly' do + BadgeGranter.grant(badge, user) + + user.update!(title: badge.name) + expect(user.user_profile.reload.badge_granted_title).to eq(false) + + user.update!(title: 'Custom') + expect(user.user_profile.reload.badge_granted_title).to eq(false) + + badge.update!(allow_title: true) + 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 + end + + describe '#next_best_title' do + let(:group_a) { Fabricate(:group, title: 'Group A') } + let(:group_b) { Fabricate(:group, title: 'Group B') } + let(:group_c) { Fabricate(:group, title: 'Group C') } + let(:badge) { Fabricate(:badge, name: 'Badge', allow_title: true) } + + it 'only includes groups with title' do + group_a.add(user) + expect(user.next_best_title).to eq('Group A') + + group_a.update!(title: nil) + expect(user.next_best_title).to eq(nil) + end + + it 'only includes badges that allow to be set as title' do + BadgeGranter.grant(badge, user) + expect(user.next_best_title).to eq('Badge') + + badge.update!(allow_title: false) + expect(user.next_best_title).to eq(nil) + end + + it "picks the next best title in the order: user's primary group, primary group, groups, and badges" do + group_a.add(user) + group_b.add(user) + group_c.add(user) + BadgeGranter.grant(badge, user) + + group_a.update!(primary_group: true) + group_b.update!(primary_group: true) + user.update!(primary_group_id: group_a.id) + expect(user.next_best_title).to eq('Group A') + + user.update!(primary_group_id: group_b.id) + expect(user.next_best_title).to eq('Group B') + + group_b.remove(user) + expect(user.next_best_title).to eq('Group A') + + group_a.remove(user) + expect(user.next_best_title).to eq('Group C') + + group_c.remove(user) + expect(user.next_best_title).to eq('Badge') + + BadgeGranter.revoke(UserBadge.find_by(user_id: user.id, badge_id: badge.id)) + expect(user.next_best_title).to eq(nil) + end + end end