From 0f9678fe490ad0bde8b5411f60f980cf32932917 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 23 Jul 2014 11:42:24 +1000 Subject: [PATCH] FIX: faster update of all badges Introduced badge triggers, introduced concept of badge that happens due to a post but has the post hidden Delta badge grant happens once a minute, backed by redis --- app/jobs/regular/update_badges.rb | 42 --------- app/jobs/scheduled/process_badge_backlog.rb | 8 ++ app/models/badge.rb | 44 ++++++---- app/models/post_action.rb | 4 +- app/models/user.rb | 19 ++--- app/models/user_profile.rb | 8 +- app/models/user_stat.rb | 7 ++ app/serializers/user_badge_serializer.rb | 3 +- app/services/badge_granter.rb | 78 +++++++++++++++-- db/fixtures/006_badges.rb | 13 ++- ...20140723011456_add_show_posts_to_badges.rb | 6 ++ lib/post_creator.rb | 1 + lib/post_revisor.rb | 1 + lib/promotion.rb | 2 +- .../user_badges_controller_spec.rb | 14 +++ spec/models/post_alert_observer_spec.rb | 4 +- spec/models/post_timing_spec.rb | 11 +-- spec/services/badge_granter_spec.rb | 85 ++++++++++++++----- 18 files changed, 229 insertions(+), 121 deletions(-) delete mode 100644 app/jobs/regular/update_badges.rb create mode 100644 app/jobs/scheduled/process_badge_backlog.rb create mode 100644 db/migrate/20140723011456_add_show_posts_to_badges.rb diff --git a/app/jobs/regular/update_badges.rb b/app/jobs/regular/update_badges.rb deleted file mode 100644 index 5895835ce..000000000 --- a/app/jobs/regular/update_badges.rb +++ /dev/null @@ -1,42 +0,0 @@ -module Jobs - class UpdateBadges < Jobs::Base - - def execute(args) - self.send(args[:action], args) - end - - def trust_level_change(args) - user = User.find(args[:user_id]) - trust_level = user.trust_level - Badge.trust_level_badge_ids.each do |badge_id| - user_badge = UserBadge.find_by(user_id: user.id, badge_id: badge_id) - if user_badge - # Revoke the badge if trust level was lowered. - BadgeGranter.revoke(user_badge) if trust_level < badge_id - else - # Grant the badge if trust level was increased. - badge = Badge.find(badge_id) - BadgeGranter.grant(badge, user) if trust_level >= badge_id - end - end - end - - def post_like(args) - post = Post.find(args[:post_id]) - user = post.user - - # Grant "Welcome" badge to the user if they do not already have it. - BadgeGranter.grant(Badge.find(Badge::Welcome), user) - - Badge.like_badge_counts.each do |badge_id, count| - if post.like_count >= count - BadgeGranter.grant(Badge.find(badge_id), user, post_id: post.id) - else - user_badge = UserBadge.find_by(badge_id: badge_id, user_id: user.id, post_id: post.id) - user_badge && BadgeGranter.revoke(user_badge) - end - end - end - - end -end diff --git a/app/jobs/scheduled/process_badge_backlog.rb b/app/jobs/scheduled/process_badge_backlog.rb new file mode 100644 index 000000000..231c31882 --- /dev/null +++ b/app/jobs/scheduled/process_badge_backlog.rb @@ -0,0 +1,8 @@ +module Jobs + class ProcessBadgeBacklog < Jobs::Scheduled + every 1.minute + def execute(args) + BadgeGranter.process_queue! + end + end +end diff --git a/app/models/badge.rb b/app/models/badge.rb index db24b720a..51333529a 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -19,16 +19,15 @@ class Badge < ActiveRecord::Base module Trigger PostAction = 1 - ReadGuidelines = 2 - PostRevision = 4 - TrustLevelChange = 8 - UserChange = 16 + PostRevision = 2 + TrustLevelChange = 4 + UserChange = 8 end module Queries Reader = < #{Badge::AutobiographerMinBioLength} AND @@ -143,7 +151,7 @@ SQL def self.trust_level(level) # we can do better with dates, but its hard work figuring this out historically " - SELECT u.id user_id, current_timestamp granted_at FROM users u + SELECT u.id user_id, current_timestamp granted_at, NULL post_id FROM users u WHERE trust_level >= #{level.to_i} " end diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 8608e7264..a5b5e308c 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -152,8 +152,8 @@ class PostAction < ActiveRecord::Base if row_count == 0 post_action = create(where_attrs.merge(action_attributes)) - if post_action && post_action.is_like? - BadgeGranter.update_badges(action: :post_like, post_id: post.id) + if post_action && post_action.errors.count == 0 + BadgeGranter.queue_badge_grant(Badge::Trigger::PostAction, post_action: post_action) end else post_action = PostAction.where(where_attrs).first diff --git a/app/models/user.rb b/app/models/user.rb index 0f97f1072..6762bcfdf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -81,6 +81,7 @@ class User < ActiveRecord::Base after_create :create_user_profile after_create :ensure_in_trust_level_group after_save :refresh_avatar + after_save :badge_grant before_destroy do # These tables don't have primary keys, so destroying them with activerecord is tricky: @@ -603,25 +604,15 @@ class User < ActiveRecord::Base if !self.uploaded_avatar_id && gravatar_downloaded self.update_column(:uploaded_avatar_id, avatar.gravatar_upload_id) - grant_autobiographer - else - if uploaded_avatar_id_changed? - grant_autobiographer - end - end - - end - - def grant_autobiographer - if self.user_profile.bio_raw && - self.user_profile.bio_raw.strip.length > Badge::AutobiographerMinBioLength && - uploaded_avatar_id - BadgeGranter.grant(Badge.find(Badge::Autobiographer), self) end end protected + def badge_grant + BadgeGranter.queue_badge_grant(Badge::Trigger::UserChange, user: self) + end + def update_tracked_topics return unless auto_track_topics_after_msecs_changed? TrackedTopicsUpdater.new(id, auto_track_topics_after_msecs).call diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index d7c0bb343..6b662bdb2 100644 --- a/app/models/user_profile.rb +++ b/app/models/user_profile.rb @@ -3,7 +3,7 @@ class UserProfile < ActiveRecord::Base validates :user, presence: true before_save :cook - after_save :assign_autobiographer + after_save :trigger_badges def bio_excerpt excerpt = PrettyText.excerpt(bio_cooked, 350) @@ -38,10 +38,8 @@ class UserProfile < ActiveRecord::Base protected - def assign_autobiographer - if bio_raw_changed? - user.grant_autobiographer - end + def trigger_badges + BadgeGranter.queue_badge_grant(Badge::Trigger::UserChange, user: self) end private diff --git a/app/models/user_stat.rb b/app/models/user_stat.rb index 76461d4c8..276a08c22 100644 --- a/app/models/user_stat.rb +++ b/app/models/user_stat.rb @@ -1,6 +1,7 @@ class UserStat < ActiveRecord::Base belongs_to :user + after_save :trigger_badges # Updates the denormalized view counts for all users def self.update_view_counts @@ -64,6 +65,12 @@ class UserStat < ActiveRecord::Base cache_last_seen(Time.now.to_f) end + protected + + def trigger_badges + BadgeGranter.queue_badge_grant(Badge::Trigger::UserChange, user: self.user) + end + private def last_seen_key diff --git a/app/serializers/user_badge_serializer.rb b/app/serializers/user_badge_serializer.rb index 4f67731a2..cb6d2341b 100644 --- a/app/serializers/user_badge_serializer.rb +++ b/app/serializers/user_badge_serializer.rb @@ -11,10 +11,11 @@ class UserBadgeSerializer < ApplicationSerializer end def include_post_id? - object.post_id && object.post + object.badge.show_posts && object.post_id && object.post end alias :include_topic? :include_post_id? + alias :include_post_number? :include_post_id? def post_number object.post && object.post.post_number diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index ad6b28255..3c5f9c663 100644 --- a/app/services/badge_granter.rb +++ b/app/services/badge_granter.rb @@ -57,13 +57,77 @@ class BadgeGranter end end - def self.update_badges(args) - Jobs.enqueue(:update_badges, args) + def self.queue_badge_grant(type,opt) + payload = nil + + case type + when Badge::Trigger::PostRevision + post = opt[:post] + payload = { + type: "PostRevision", + post_ids: [post.id] + } + when Badge::Trigger::UserChange + user = opt[:user] + payload = { + type: "UserChange", + user_ids: [user.id] + } + when Badge::Trigger::TrustLevelChange + user = opt[:user] + payload = { + type: "TrustLevelChange", + user_ids: [user.id] + } + when Badge::Trigger::PostAction + action = opt[:post_action] + payload = { + type: "PostAction", + post_ids: [action.post_id, action.related_post_id].compact! + } + end + + $redis.lpush queue_key, payload.to_json if payload end - def self.backfill(badge) + def self.clear_queue! + $redis.del queue_key + end + + def self.process_queue! + limit = 1000 + items = [] + while limit > 0 && item = $redis.lpop(queue_key) + items << JSON.parse(item) + limit -= 1 + end + + items = items.group_by{|i| i["type"]} + + items.each do |type, list| + post_ids = list.map{|i| i["post_ids"]}.flatten.compact.uniq + user_ids = list.map{|i| i["user_ids"]}.flatten.compact.uniq + + next unless post_ids.present? || user_ids.present? + find_by_type(type).each{|badge| backfill(badge, post_ids: post_ids, user_ids: user_ids)} + end + end + + def self.find_by_type(type) + id = "Badge::Trigger::#{type}".constantize + Badge.where(trigger: id) + end + + def self.queue_key + "badge_queue".freeze + end + + def self.backfill(badge, opts=nil) return unless badge.query.present? && badge.enabled + post_ids = opts[:post_ids] if opts + user_ids = opts[:user_ids] if opts + post_clause = badge.target_posts ? "AND q.post_id = ub.post_id" : "" post_id_field = badge.target_posts ? "q.post_id" : "NULL" @@ -77,7 +141,7 @@ class BadgeGranter WHERE ub.badge_id = :id AND q.user_id IS NULL )" - Badge.exec_sql(sql, id: badge.id) if badge.auto_revoke + Badge.exec_sql(sql, id: badge.id) if badge.auto_revoke && !post_ids && !user_ids sql = "INSERT INTO user_badges(badge_id, user_id, granted_at, granted_by_id, post_id) SELECT :id, q.user_id, q.granted_at, -1, #{post_id_field} @@ -85,11 +149,15 @@ class BadgeGranter LEFT JOIN user_badges ub ON ub.badge_id = :id AND ub.user_id = q.user_id #{post_clause} - WHERE ub.badge_id IS NULL AND q.user_id <> -1 + /*where*/ RETURNING id, user_id, granted_at " builder = SqlBuilder.new(sql) + builder.where("ub.badge_id IS NULL AND q.user_id <> -1") + builder.where("q.post_id in (:post_ids)", post_ids: post_ids) if post_ids.present? + builder.where("q.user_id in (:user_ids)", user_ids: user_ids) if user_ids.present? + builder.map_exec(OpenStruct, id: badge.id).each do |row| # old bronze badges do not matter diff --git a/db/fixtures/006_badges.rb b/db/fixtures/006_badges.rb index ac37ad4b3..6890d8a94 100644 --- a/db/fixtures/006_badges.rb +++ b/db/fixtures/006_badges.rb @@ -65,6 +65,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = false + b.show_posts = false b.query = Badge::Queries::Reader b.default_badge_grouping_id = BadgeGrouping::GettingStarted b.auto_revoke = false @@ -76,9 +77,10 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = false + b.show_posts = false b.query = Badge::Queries::ReadGuidelines b.default_badge_grouping_id = BadgeGrouping::GettingStarted - b.trigger = Badge::Trigger::ReadGuidelines + b.trigger = Badge::Trigger::UserChange end Badge.seed do |b| @@ -87,6 +89,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = true + b.show_posts = true b.query = Badge::Queries::FirstLink b.default_badge_grouping_id = BadgeGrouping::GettingStarted b.trigger = Badge::Trigger::PostRevision @@ -98,6 +101,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = true + b.show_posts = true b.query = Badge::Queries::FirstQuote b.default_badge_grouping_id = BadgeGrouping::GettingStarted b.trigger = Badge::Trigger::PostRevision @@ -109,6 +113,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = true + b.show_posts = true b.query = Badge::Queries::FirstLike b.default_badge_grouping_id = BadgeGrouping::GettingStarted b.trigger = Badge::Trigger::PostAction @@ -119,7 +124,8 @@ Badge.seed do |b| b.default_name = "First Flag" b.badge_type_id = BadgeType::Bronze b.multiple_grant = false - b.target_posts = false + b.target_posts = true + b.show_posts = false b.query = Badge::Queries::FirstFlag b.default_badge_grouping_id = BadgeGrouping::Community b.trigger = Badge::Trigger::PostAction @@ -131,6 +137,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = true + b.show_posts = true b.query = Badge::Queries::FirstShare b.default_badge_grouping_id = BadgeGrouping::GettingStarted b.trigger = Badge::Trigger::PostRevision @@ -142,6 +149,7 @@ Badge.seed do |b| b.badge_type_id = BadgeType::Bronze b.multiple_grant = false b.target_posts = true + b.show_posts = true b.query = Badge::Queries::Welcome b.default_badge_grouping_id = BadgeGrouping::Community b.trigger = Badge::Trigger::PostAction @@ -182,6 +190,7 @@ like_badges.each do |spec| b.badge_type_id = spec[:type] b.multiple_grant = spec[:multiple] b.target_posts = true + b.show_posts = true b.query = Badge::Queries.like_badge(Badge.like_badge_counts[spec[:id]]) b.default_badge_grouping_id = BadgeGrouping::Posting b.trigger = Badge::Trigger::PostAction diff --git a/db/migrate/20140723011456_add_show_posts_to_badges.rb b/db/migrate/20140723011456_add_show_posts_to_badges.rb new file mode 100644 index 000000000..d830861af --- /dev/null +++ b/db/migrate/20140723011456_add_show_posts_to_badges.rb @@ -0,0 +1,6 @@ +class AddShowPostsToBadges < ActiveRecord::Migration + def change + # show posts to users on badge show page + add_column :badges, :show_posts, :boolean, null: false, default: false + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 073f7844d..ecf30b7ca 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -79,6 +79,7 @@ class PostCreator handle_spam unless @opts[:import_mode] track_latest_on_category enqueue_jobs + BadgeGranter.queue_badge_grant(Badge::Trigger::PostRevision, post: @post) end @post diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 73047c8be..b95681aeb 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -28,6 +28,7 @@ class PostRevisor @post.advance_draft_sequence PostAlerter.new.after_save_post(@post) publish_revision + BadgeGranter.queue_badge_grant(Badge::Trigger::PostRevision, post: @post) true end diff --git a/lib/promotion.rb b/lib/promotion.rb index 95879d6df..a26a772a1 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -72,7 +72,7 @@ class Promotion @user.user_profile.recook_bio @user.user_profile.save! Group.user_trust_level_change!(@user.id, @user.trust_level) - BadgeGranter.update_badges(action: :trust_level_change, user_id: @user.id) + BadgeGranter.queue_badge_grant(Badge::Trigger::TrustLevelChange, user: @user) end true diff --git a/spec/controllers/user_badges_controller_spec.rb b/spec/controllers/user_badges_controller_spec.rb index d93e990ea..24a1071df 100644 --- a/spec/controllers/user_badges_controller_spec.rb +++ b/spec/controllers/user_badges_controller_spec.rb @@ -4,6 +4,20 @@ describe UserBadgesController do let(:user) { Fabricate(:user) } let(:badge) { Fabricate(:badge) } + context 'index' do + it 'doest not leak private info' do + badge = Fabricate(:badge, target_posts: true, show_posts: false) + p = create_post + UserBadge.create(badge: badge, user: user, post_id: p.id, granted_by_id: -1, granted_at: Time.now) + + xhr :get, :index, badge_id: badge.id + response.status.should == 200 + parsed = JSON.parse(response.body) + parsed["topics"].should be_nil + parsed["user_badges"][0]["post_id"].should == nil + end + end + context 'index' do let!(:user_badge) { UserBadge.create(badge: badge, user: user, granted_by: Discourse.system_user, granted_at: Time.now) } diff --git a/spec/models/post_alert_observer_spec.rb b/spec/models/post_alert_observer_spec.rb index 64da4ecfd..5604feca2 100644 --- a/spec/models/post_alert_observer_spec.rb +++ b/spec/models/post_alert_observer_spec.rb @@ -15,8 +15,8 @@ describe PostAlertObserver do it 'creates a notification' do lambda { PostAction.act(evil_trout, post, PostActionType.types[:like]) - # one like and one welcome badge - }.should change(Notification, :count).by(2) + # one like (welcome badge deferred) + }.should change(Notification, :count).by(1) end end diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb index 7b7564e21..081534395 100644 --- a/spec/models/post_timing_spec.rb +++ b/spec/models/post_timing_spec.rb @@ -2,9 +2,6 @@ require 'spec_helper' describe PostTiming do - it { should belong_to :topic } - it { should belong_to :user } - it { should validate_presence_of :post_number } it { should validate_presence_of :msecs } @@ -76,14 +73,14 @@ describe PostTiming do PostAction.act(user2, post, PostActionType.types[:like]) - post.user.unread_notifications.should == 2 - post.user.unread_notifications_by_type.should == {Notification.types[:granted_badge] => 1, Notification.types[:liked] => 1 } + post.user.unread_notifications.should == 1 + post.user.unread_notifications_by_type.should == {Notification.types[:liked] => 1 } PostTiming.process_timings(post.user, post.topic_id, 1, [[post.post_number, 100]]) post.user.reload - post.user.unread_notifications_by_type.should == {Notification.types[:granted_badge] => 1} - post.user.unread_notifications.should == 1 + post.user.unread_notifications_by_type.should == {} + post.user.unread_notifications.should == 0 end end diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index b2845aeb7..7143d98be 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -48,21 +48,6 @@ describe BadgeGranter do end end - describe 'autobiographer' do - it 'grants autobiographer correctly' do - user = Fabricate(:user) - user.user_profile.bio_raw = "I filled my bio" - user.user_profile.save! - - Badge.find(Badge::Autobiographer).grant_count.should == 0 - - user.uploaded_avatar_id = 100 - user.save - - Badge.find(Badge::Autobiographer).grant_count.should == 1 - end - end - describe 'grant' do it 'grants a badge' do @@ -128,10 +113,57 @@ describe BadgeGranter do let(:user) { Fabricate(:user) } let(:liker) { Fabricate(:user) } + before do + BadgeGranter.clear_queue! + end + + it "grants autobiographer" do + user.user_profile.bio_raw = "THIS IS MY bio it a long bio I like my bio" + user.uploaded_avatar_id = 10 + user.user_profile.save + user.save + + BadgeGranter.process_queue! + UserBadge.where(user_id: user.id, badge_id: Badge::Autobiographer).count.should eq(1) + end + + it "grants read guidlines" do + user.user_stat.read_faq = Time.now + user.user_stat.save + + BadgeGranter.process_queue! + UserBadge.where(user_id: user.id, badge_id: Badge::ReadGuidelines).count.should eq(1) + end + + it "grants first link" do + post = create_post + post2 = create_post(raw: "#{Discourse.base_url}/t/slug/#{post.topic_id}") + + BadgeGranter.process_queue! + UserBadge.where(user_id: post2.user.id, badge_id: Badge::FirstLink).count.should eq(1) + end + + it "grants first edit" do + SiteSetting.ninja_edit_window = 0 + post = create_post + user = post.user + + UserBadge.where(user_id: user.id, badge_id: Badge::Editor).count.should eq(0) + + PostRevisor.new(post).revise!(user, "This is my new test 1235 123") + BadgeGranter.process_queue! + + UserBadge.where(user_id: user.id, badge_id: Badge::Editor).count.should eq(1) + end + it "grants and revokes trust level badges" do user.change_trust_level!(:elder) + BadgeGranter.process_queue! UserBadge.where(user_id: user.id, badge_id: Badge.trust_level_badge_ids).count.should eq(4) + user.change_trust_level!(:basic) + BadgeGranter.backfill(Badge.find(1)) + BadgeGranter.backfill(Badge.find(2)) UserBadge.where(user_id: user.id, badge_id: 1).first.should_not be_nil UserBadge.where(user_id: user.id, badge_id: 2).first.should be_nil end @@ -139,26 +171,35 @@ describe BadgeGranter do it "grants system like badges" do post = create_post(user: user) # Welcome badge - PostAction.act(liker, post, PostActionType.types[:like]) + action = PostAction.act(liker, post, PostActionType.types[:like]) + BadgeGranter.process_queue! UserBadge.find_by(user_id: user.id, badge_id: 5).should_not be_nil + # Nice post badge post.update_attributes like_count: 10 - BadgeGranter.update_badges(action: :post_like, post_id: post.id) - BadgeGranter.update_badges(action: :post_like, post_id: post.id) + + BadgeGranter.queue_badge_grant(Badge::Trigger::PostAction, post_action: action) + BadgeGranter.process_queue! + UserBadge.find_by(user_id: user.id, badge_id: 6).should_not be_nil UserBadge.where(user_id: user.id, badge_id: 6).count.should == 1 + # Good post badge post.update_attributes like_count: 25 - BadgeGranter.update_badges(action: :post_like, post_id: post.id) + BadgeGranter.queue_badge_grant(Badge::Trigger::PostAction, post_action: action) + BadgeGranter.process_queue! UserBadge.find_by(user_id: user.id, badge_id: 7).should_not be_nil + # Great post badge post.update_attributes like_count: 50 - BadgeGranter.update_badges(action: :post_like, post_id: post.id) + BadgeGranter.queue_badge_grant(Badge::Trigger::PostAction, post_action: action) + BadgeGranter.process_queue! UserBadge.find_by(user_id: user.id, badge_id: 8).should_not be_nil + # Revoke badges on unlike post.update_attributes like_count: 49 - BadgeGranter.update_badges(action: :post_like, post_id: post.id) - UserBadge.find_by(user_id: user.id, badge_id: 8).should be_nil + BadgeGranter.backfill(Badge.find(Badge::GreatPost)) + UserBadge.find_by(user_id: user.id, badge_id: Badge::GreatPost).should be_nil end end