diff --git a/app/models/post.rb b/app/models/post.rb index ad33a08609c..3ddd2325ce3 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -263,12 +263,6 @@ class Post < ActiveRecord::Base PostCreator.before_create_tasks(self) end - # TODO: Move some of this into an asynchronous job? - # TODO: Move into PostCreator - after_create do - PostCreator.after_create_tasks(self) - end - # This calculates the geometric mean of the post timings and stores it along with # each post. def self.calculate_avg_time diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 69a2b88b33b..185bede5350 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -68,7 +68,6 @@ class TopicUser < ActiveRecord::Base # since there's more likely to be an existing record than not. If the update returns 0 rows affected # it then creates the row instead. def change(user_id, topic_id, attrs) - # Sometimes people pass objs instead of the ids. We can handle that. topic_id = topic_id.id if topic_id.is_a?(::Topic) user_id = user_id.id if user_id.is_a?(::User) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 3539774d114..dbde279b48d 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -66,6 +66,7 @@ class PostCreator store_unique_post_key send_notifications_for_private_message track_topic + update_topic_stats update_user_counts publish @post.advance_draft_sequence @@ -100,19 +101,6 @@ class PostCreator post.last_version_at ||= Time.now end - def self.after_create_tasks(post) - # Update attributes on the topic - featured users and last posted. - attrs = {last_posted_at: post.created_at, last_post_user_id: post.user_id} - attrs[:bumped_at] = post.created_at unless post.no_bump - post.topic.update_attributes(attrs) - - # Update topic user data - TopicUser.change(post.user.id, - post.topic.id, - posted: true, - last_read_post_number: post.post_number, - seen_post_count: post.post_number) - end protected @@ -184,6 +172,13 @@ class PostCreator @topic = topic end + def update_topic_stats + # Update attributes on the topic - featured users and last posted. + attrs = {last_posted_at: @post.created_at, last_post_user_id: @post.user_id} + attrs[:bumped_at] = @post.created_at unless @post.no_bump + @topic.update_attributes(attrs) + end + def setup_post post = @topic.posts.new(raw: @opts[:raw], user: @user, @@ -267,6 +262,12 @@ class PostCreator def track_topic unless @opts[:auto_track] == false TopicUser.auto_track(@user.id, @topic.id, TopicUser.notification_reasons[:created_post]) + # Update topic user data + TopicUser.change(@post.user.id, + @post.topic.id, + posted: true, + last_read_post_number: @post.post_number, + seen_post_count: @post.post_number) end end diff --git a/spec/components/jobs/feature_topic_users_spec.rb b/spec/components/jobs/feature_topic_users_spec.rb index 073579d04ba..5473ad1a3bf 100644 --- a/spec/components/jobs/feature_topic_users_spec.rb +++ b/spec/components/jobs/feature_topic_users_spec.rb @@ -12,12 +12,12 @@ describe Jobs::FeatureTopicUsers do end context 'with a topic' do - let!(:post) { Fabricate(:post) } + let!(:post) { create_post } let(:topic) { post.topic } let!(:coding_horror) { Fabricate(:coding_horror) } let!(:evil_trout) { Fabricate(:evil_trout) } - let!(:second_post) { Fabricate(:post, topic: topic, user: coding_horror)} - let!(:third_post) { Fabricate(:post, topic: topic, user: evil_trout)} + let!(:second_post) { create_post(topic: topic, user: coding_horror)} + let!(:third_post) { create_post(topic: topic, user: evil_trout)} it "won't feature the OP" do Jobs::FeatureTopicUsers.new.execute(topic_id: topic.id) diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index ae059fd4c3d..8aaa2fa2558 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -23,8 +23,8 @@ describe PostCreator do it "can be created with auto tracking disabled" do p = PostCreator.create(user, basic_topic_params.merge(auto_track: false)) - t = TopicUser.where(user_id: p.user_id, topic_id: p.topic_id).first - t.notification_level.should == TopicUser.notification_levels[:regular] + # must be 0 otherwise it will think we read the topic which is clearly untrue + TopicUser.where(user_id: p.user_id, topic_id: p.topic_id).count.should == 0 end it "ensures the user can create the topic" do @@ -154,8 +154,15 @@ describe PostCreator do it 'increases topic response counts' do first_post = creator.create - user2 = Fabricate(:coding_horror) + # ensure topic user is correct + topic_user = first_post.user.topic_users.where(topic_id: first_post.topic_id).first + topic_user.should be_present + topic_user.should be_posted + topic_user.last_read_post_number.should == first_post.post_number + topic_user.seen_post_count.should == first_post.post_number + + user2 = Fabricate(:coding_horror) user2.topic_reply_count.should == 0 first_post.user.reload.topic_reply_count.should == 0 diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index 6ed1f15d911..747df0fecba 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -8,7 +8,7 @@ describe PostDestroyer do end let(:moderator) { Fabricate(:moderator) } - let(:post) { Fabricate(:post) } + let(:post) { create_post } describe 'basic destroying' do @@ -56,10 +56,10 @@ describe PostDestroyer do context 'deleting the second post in a topic' do let(:user) { Fabricate(:user) } - let!(:post) { Fabricate(:post, user: user) } - let(:topic) { post.topic } + let!(:post) { create_post(user: user) } + let(:topic) { post.topic.reload } let(:second_user) { Fabricate(:coding_horror) } - let!(:second_post) { Fabricate(:post, topic: topic, user: second_user) } + let!(:second_post) { create_post(topic: topic, user: second_user) } before do PostDestroyer.new(moderator, second_post).destroy diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 7ae551735f4..1b44d360d1d 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -230,7 +230,7 @@ describe TopicQuery do end context 'created topics' do - let!(:created_topic) { Fabricate(:post, user: user).topic } + let!(:created_topic) { create_post(user: user).topic } it "includes the created topic" do topics.include?(created_topic).should be_true @@ -238,8 +238,8 @@ describe TopicQuery do end context "topic you've posted in" do - let(:other_users_topic) { Fabricate(:post, user: creator).topic } - let!(:your_post) { Fabricate(:post, user: user, topic: other_users_topic )} + let(:other_users_topic) { create_post(user: creator).topic } + let!(:your_post) { create_post(user: user, topic: other_users_topic )} it "includes the posted topic" do topics.include?(other_users_topic).should be_true diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index f769092fa67..e94bd3608cf 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -3,7 +3,7 @@ require 'topic_view' describe TopicView do - let(:topic) { Fabricate(:topic) } + let(:topic) { create_topic } let(:coding_horror) { Fabricate(:coding_horror) } let(:first_poster) { topic.user } @@ -109,53 +109,39 @@ describe TopicView do let(:path) { "/1234" } before do - topic.expects(:relative_url).returns(path) - described_class.any_instance.expects(:find_topic).with(1234).returns(topic) + topic.stubs(:relative_url).returns(path) + TopicView.any_instance.stubs(:find_topic).with(1234).returns(topic) end - context "without a post number" do - context "without a page" do - it "generates a canonical path for a topic" do - described_class.new(1234, user).canonical_path.should eql(path) - end - end - - context "with a page" do - let(:path_with_page) { "/1234?page=5" } - - it "generates a canonical path for a topic" do - described_class.new(1234, user, page: 5).canonical_path.should eql(path_with_page) - end - end + it "generates canonical path correctly" do + TopicView.new(1234, user).canonical_path.should eql(path) + TopicView.new(1234, user, page: 5).canonical_path.should eql("/1234?page=5") end - context "with a post number" do - let(:path_with_page) { "/1234?page=10" } - before { SiteSetting.stubs(:posts_per_page).returns(5) } - it "generates a canonical path for a topic" do - described_class.new(1234, user, post_number: 50).canonical_path.should eql(path_with_page) - end + it "generates a canonical correctly for paged results" do + SiteSetting.stubs(:posts_per_page).returns(5) + TopicView.new(1234, user, post_number: 50).canonical_path.should eql("/1234?page=10") end end describe "#next_page" do let(:p2) { stub(post_number: 2) } let(:topic) do - topic = Fabricate(:topic) + topic = create_topic topic.stubs(:highest_post_number).returns(5) topic end let(:user) { Fabricate(:user) } before do - described_class.any_instance.expects(:find_topic).with(1234).returns(topic) - described_class.any_instance.stubs(:filter_posts) - described_class.any_instance.stubs(:last_post).returns(p2) + TopicView.any_instance.expects(:find_topic).with(1234).returns(topic) + TopicView.any_instance.stubs(:filter_posts) + TopicView.any_instance.stubs(:last_post).returns(p2) SiteSetting.stubs(:posts_per_page).returns(2) end it "should return the next page" do - described_class.new(1234, user).next_page.should eql(2) + TopicView.new(1234, user).next_page.should eql(2) end end @@ -183,17 +169,17 @@ describe TopicView do end context '.read?' do - it 'is unread with no logged in user' do + it 'tracks correctly' do + # anon has nothing TopicView.new(topic.id).read?(1).should be_false - end - it 'makes posts as unread by default' do + # random user has nothing topic_view.read?(1).should be_false - end - it 'knows a post is read when it has a PostTiming' do - PostTiming.create(topic: topic, user: coding_horror, post_number: 1, msecs: 1000) - topic_view.read?(1).should be_true + # a real user that just read it should have it marked + PostTiming.process_timings(coding_horror, topic.id, 1, [[1,1000]]) + TopicView.new(topic.id, coding_horror).read?(1).should be_true + TopicView.new(topic.id, coding_horror).topic_user.should be_present end end @@ -201,10 +187,6 @@ describe TopicView do it 'returns nil when there is no user' do TopicView.new(topic.id, nil).topic_user.should be_blank end - - it 'returns a record once the user has some data' do - TopicView.new(topic.id, coding_horror).topic_user.should be_present - end end context '#recent_posts' do diff --git a/spec/integration/spam_rules_spec.rb b/spec/integration/spam_rules_spec.rb index df028ccbf5f..62a6e5ef744 100644 --- a/spec/integration/spam_rules_spec.rb +++ b/spec/integration/spam_rules_spec.rb @@ -19,8 +19,8 @@ describe PostAction do Given(:spammer) { Fabricate(:user, trust_level: TrustLevel.levels[:newuser]) } context 'spammer post is not flagged enough times' do - Given!(:spam_post) { Fabricate(:post, user: spammer) } - Given!(:spam_post2) { Fabricate(:post, user: spammer) } + Given!(:spam_post) { create_post(user: spammer) } + Given!(:spam_post2) { create_post(user: spammer) } When { PostAction.act(user1, spam_post, PostActionType.types[:spam]) } Then { expect(spam_post.reload).to_not be_hidden } diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 3eee76bae0b..be18c288ab5 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -14,6 +14,7 @@ describe PostAction do describe "flagged_posts_report" do it "operates correctly" do + post = create_post PostAction.act(codinghorror, post, PostActionType.types[:spam]) mod_message = PostAction.act(Fabricate(:user), post, PostActionType.types[:notify_moderators], message: "this is a 10") @@ -30,6 +31,7 @@ describe PostAction do describe "messaging" do it "notify moderators integration test" do + post = create_post mod = moderator action = PostAction.act(codinghorror, post, PostActionType.types[:notify_moderators], message: "this is my special long message"); @@ -100,6 +102,7 @@ describe PostAction do end it "should ignore validated flags" do + post = create_post admin = Fabricate(:admin) PostAction.act(codinghorror, post, PostActionType.types[:off_topic]) post.hidden.should be_false @@ -193,7 +196,7 @@ describe PostAction do context "flag_counts_for" do it "returns the correct flag counts" do - post = Fabricate(:post) + post = create_post SiteSetting.stubs(:flags_required_to_hide_post).returns(7) @@ -244,7 +247,7 @@ describe PostAction do end it 'should follow the rules for automatic hiding workflow' do - post = Fabricate(:post) + post = create_post u1 = Fabricate(:evil_trout) u2 = Fabricate(:walter_white) admin = Fabricate(:admin) # we need an admin for the messages diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 525e14133df..7d8e1d34855 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -570,19 +570,6 @@ describe Post do post.replies.should be_blank end - describe 'a forum topic user record for the topic' do - - let(:topic_user) { post.user.topic_users.where(topic_id: topic.id).first } - - it 'is set correctly' do - topic_user.should be_present - topic_user.should be_posted - topic_user.last_read_post_number.should == post.post_number - topic_user.seen_post_count.should == post.post_number - end - - end - describe 'extract_quoted_post_numbers' do let!(:post) { Fabricate(:post, post_args) } diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index bc287bed973..082ed41b695 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -339,7 +339,7 @@ describe Topic do it 'updates the bumped_at field when a new post is made' do @topic.bumped_at.should be_present lambda { - Fabricate(:post, topic: @topic, user: @topic.user) + create_post(topic: @topic, user: @topic.user) @topic.reload }.should change(@topic, :bumped_at) end @@ -621,8 +621,8 @@ describe Topic do context 'last_poster info' do before do - @user = Fabricate(:user) - @post = Fabricate(:post, user: @user) + @post = create_post + @user = @post.user @topic = @post.topic end @@ -633,7 +633,7 @@ describe Topic do context 'after a second post' do before do @second_user = Fabricate(:coding_horror) - @new_post = Fabricate(:post, topic: @topic, user: @second_user) + @new_post = create_post(topic: @topic, user: @second_user) @topic.reload end diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index cbb9daa7e4b..0d979ceb998 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -7,7 +7,7 @@ describe TopicTrackingState do end let(:post) do - Fabricate(:post) + create_post end it "can correctly publish unread" do @@ -39,7 +39,7 @@ describe TopicTrackingState do TopicTrackingState.report([user.id], post.topic_id + 1).should be_empty # when we reply the poster should have an unread row - Fabricate(:post, user: user, topic: post.topic) + create_post(user: user, topic: post.topic) report = TopicTrackingState.report([post.user_id, user.id]) report.length.should == 1 diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb index d6a412b61ff..ca848ec0663 100644 --- a/spec/models/topic_user_spec.rb +++ b/spec/models/topic_user_spec.rb @@ -232,6 +232,7 @@ describe TopicUser do it "is able to self heal" do p1 = Fabricate(:post) p2 = Fabricate(:post, user: p1.user, topic: p1.topic, post_number: 2) + p1.topic.notifier.watch_topic!(p1.user_id) TopicUser.exec_sql("UPDATE topic_users set seen_post_count=100, last_read_post_number=0 WHERE topic_id = :topic_id AND user_id = :user_id", topic_id: p1.topic_id, user_id: p1.user_id) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fa5e13a3e95..d48bf2643b6 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,6 +12,11 @@ require 'fakeweb' FakeWeb.allow_net_connect = false module Helpers + + def self.next_seq + @next_seq = (@next_seq || 0) + 1 + end + def log_in(fabricator=nil) user = Fabricate(fabricator || :user) log_in_user(user) @@ -126,6 +131,21 @@ def build(*args) Fabricate.build(*args) end +def create_topic(args={}) + args[:title] ||= "This is my title #{Helpers.next_seq}" + user = args.delete(:user) || Fabricate(:user) + guardian = Guardian.new(user) + TopicCreator.create(user, guardian, args) +end + +def create_post(args={}) + args[:title] ||= "This is my title #{Helpers.next_seq}" + args[:raw] ||= "This is the raw body of my post, it is cool #{Helpers.next_seq}" + args[:topic_id] = args[:topic].id if args[:topic] + user = args.delete(:user) || Fabricate(:user) + PostCreator.create(user, args) +end + module MessageBus::DiagnosticsHelper def publish(channel, data, opts = nil) id = super(channel, data, opts)