Fix: When you split topics, featured users and like counts were incorrect.

This commit is contained in:
Robin Ward 2013-03-12 12:33:42 -04:00
parent 1e017ddc62
commit 1103dde5cd
4 changed files with 83 additions and 29 deletions

View File

@ -136,7 +136,7 @@ class PostAction < ActiveRecord::Base
# Voting also changes the sort_order # Voting also changes the sort_order
if post_action_type == :vote if post_action_type == :vote
Post.update_all ["vote_count = vote_count + :delta, sort_order = :max - (vote_count + :delta)", delta: delta, max: Topic::MAX_SORT_ORDER], id: post_id Post.update_all ["vote_count = vote_count + :delta, sort_order = :max - (vote_count + :delta)", delta: delta, max: Topic.max_sort_order], id: post_id
else else
Post.update_all ["#{column} = #{column} + ?", delta], id: post_id Post.update_all ["#{column} = #{column} + ?", delta], id: post_id
end end

View File

@ -8,8 +8,13 @@ class Topic < ActiveRecord::Base
include ActionView::Helpers include ActionView::Helpers
include RateLimiter::OnCreateRecord include RateLimiter::OnCreateRecord
MAX_SORT_ORDER = 2**31 - 1 def self.max_sort_order
FEATURED_USERS = 4 2**31 - 1
end
def self.featured_users_count
4
end
versioned if: :new_version_required? versioned if: :new_version_required?
acts_as_paranoid acts_as_paranoid
@ -123,7 +128,7 @@ class Topic < ActiveRecord::Base
return title unless SiteSetting.title_fancy_entities? return title unless SiteSetting.title_fancy_entities?
# We don't always have to require this, if fancy is disabled # We don't always have to require this, if fancy is disabled
# see: http://meta.discourse.org/t/pattern-for-defer-loading-gems-and-profiling-with-perftools-rb/4629 # see: http://meta.discourse.org/t/pattern-for-defer-loading-gems-and-profiling-with-perftools-rb/4629
require 'redcarpet' unless defined? Redcarpet require 'redcarpet' unless defined? Redcarpet
Redcarpet::Render::SmartyPants.render(title) Redcarpet::Render::SmartyPants.render(title)
@ -416,8 +421,6 @@ class Topic < ActiveRecord::Base
end end
# Update denormalized values since we've manually moved stuff # Update denormalized values since we've manually moved stuff
Topic.reset_highest(topic.id)
Topic.reset_highest(id)
end end
# Add a moderator post explaining that the post was moved # Add a moderator post explaining that the post was moved
@ -427,15 +430,61 @@ class Topic < ActiveRecord::Base
add_moderator_post(moved_by, I18n.t("move_posts.moderator_post", count: post_ids.size, topic_link: topic_link), post_number: first_post_number) add_moderator_post(moved_by, I18n.t("move_posts.moderator_post", count: post_ids.size, topic_link: topic_link), post_number: first_post_number)
Jobs.enqueue(:notify_moved_posts, post_ids: post_ids, moved_by_id: moved_by.id) Jobs.enqueue(:notify_moved_posts, post_ids: post_ids, moved_by_id: moved_by.id)
topic.update_statistics
update_statistics
end end
topic topic
end end
# Updates the denormalized statistics of a topic including featured posters. They shouldn't
# go out of sync unless you do something drastic live move posts from one topic to another.
# this recalculates everything.
def update_statistics
feature_topic_users
update_action_counts
Topic.reset_highest(id)
end
def update_flagged_posts_count def update_flagged_posts_count
PostAction.update_flagged_posts_count PostAction.update_flagged_posts_count
end end
def update_action_counts
PostActionType.types.keys.each do |type|
count_field = "#{type}_count"
update_column(count_field, Post.where(topic_id: id).sum(count_field))
end
end
# Chooses which topic users to feature
def feature_topic_users(args={})
reload
to_feature = posts
# Don't include the OP or the last poster
to_feature = to_feature.where('user_id NOT IN (?, ?)', user_id, last_post_user_id)
# Exclude a given post if supplied (in the case of deletes)
to_feature = to_feature.where("id <> ?", args[:except_post_id]) if args[:except_post_id].present?
# Clear the featured users by default
Topic.featured_users_count.times do |i|
send("featured_user#{i+1}_id=", nil)
end
# Assign the featured_user{x} columns
to_feature = to_feature.group(:user_id).order('count_all desc').limit(Topic.featured_users_count)
to_feature.count.keys.each_with_index do |user_id, i|
send("featured_user#{i+1}_id=", user_id)
end
save
end
# Create the summary of the interesting posters in a topic. Cheats to avoid # Create the summary of the interesting posters in a topic. Cheats to avoid
# many queries. # many queries.

View File

@ -6,28 +6,7 @@ module Jobs
topic = Topic.where(id: args[:topic_id]).first topic = Topic.where(id: args[:topic_id]).first
raise Discourse::InvalidParameters.new(:topic_id) unless topic.present? raise Discourse::InvalidParameters.new(:topic_id) unless topic.present?
to_feature = topic.posts topic.feature_topic_users(args)
# Don't include the OP or the last poster
to_feature = to_feature.where('user_id <> ?', topic.user_id)
to_feature = to_feature.where('user_id <> ?', topic.last_post_user_id)
# Exclude a given post if supplied (in the case of deletes)
to_feature = to_feature.where("id <> ?", args[:except_post_id]) if args[:except_post_id].present?
# Clear the featured users by default
Topic::FEATURED_USERS.times do |i|
topic.send("featured_user#{i+1}_id=", nil)
end
# Assign the featured_user{x} columns
to_feature = to_feature.group(:user_id).order('count_all desc').limit(Topic::FEATURED_USERS)
to_feature.count.keys.each_with_index do |user_id, i|
topic.send("featured_user#{i+1}_id=", user_id)
end
topic.save
end end
end end

View File

@ -181,13 +181,19 @@ describe Topic do
context 'move_posts' do context 'move_posts' do
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user) }
let(:another_user) { Fabricate(:evil_trout) }
let(:category) { Fabricate(:category, user: user) } let(:category) { Fabricate(:category, user: user) }
let!(:topic) { Fabricate(:topic, user: user, category: category) } let!(:topic) { Fabricate(:topic, user: user, category: category) }
let!(:p1) { Fabricate(:post, topic: topic, user: user) } let!(:p1) { Fabricate(:post, topic: topic, user: user) }
let!(:p2) { Fabricate(:post, topic: topic, user: user)} let!(:p2) { Fabricate(:post, topic: topic, user: another_user)}
let!(:p3) { Fabricate(:post, topic: topic, user: user)} let!(:p3) { Fabricate(:post, topic: topic, user: user)}
let!(:p4) { Fabricate(:post, topic: topic, user: user)} let!(:p4) { Fabricate(:post, topic: topic, user: user)}
before do
# add a like to a post
PostAction.act(another_user, p4, PostActionType.types[:like])
end
context 'success' do context 'success' do
it "enqueues a job to notify users" do it "enqueues a job to notify users" do
@ -233,10 +239,22 @@ describe Topic do
new_topic.should be_present new_topic.should be_present
end end
it "has the featured user" do
new_topic.featured_user1_id.should == another_user.id
end
it "has the correct like_count" do
new_topic.like_count.should == 1
end
it "has the correct category" do it "has the correct category" do
new_topic.category.should == category new_topic.category.should == category
end end
it "has removed the second poster from the featured users, since they moved" do
topic.featured_user1_id.should be_blank
end
it "has two posts" do it "has two posts" do
new_topic.reload new_topic.reload
new_topic.posts_count.should == 2 new_topic.posts_count.should == 2
@ -277,6 +295,14 @@ describe Topic do
topic.reload topic.reload
end end
it "has removed the second poster from the featured users, since they moved" do
topic.featured_user1_id.should be_blank
end
it "has the correct like_count" do
topic.like_count.should == 0
end
it "has 2 posts now" do it "has 2 posts now" do
topic.posts_count.should == 2 topic.posts_count.should == 2
end end