FIX: Add DB constraints for post & topic counter cache for `UserStat` (#15626)

Ensures that `UserStat#post_count` and `UserStat#topic_count` does not
go below 0. When it does like it did now, we tend to have bugs in our
code since we're usually coding with the assumption that the count isn't
negative.

In order to support the constraints, our post and topic fabricators in
tests will now automatically increment the count for the respective
user's `UserStat` as well. We have to do this because our fabricators
bypasss `PostCreator` which holds the responsibility of updating `UserStat#post_count` and
`UserStat#topic_count`.

app/models/topic_converter.rb

@@ -63,7 +63,7 @@ class TopicConverter
   private
 
   def posters
-    @posters ||= @topic.posts.distinct.pluck(:user_id).to_a
+    @posters ||= @topic.posts.where("post_number > 1").distinct.pluck(:user_id)
   end
 
   def update_user_stats

app/services/user_stat_count_updater.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+class UserStatCountUpdater
+  class << self
+    def increment!(post, user_stat: nil)
+      update!(post, user_stat: user_stat)
+    end
+
+    def decrement!(post, user_stat: nil)
+      update!(post, user_stat: user_stat, action: :decrement!)
+    end
+
+    private
+
+    def update!(post, user_stat: nil, action: :increment!)
+      return if !post.topic
+      return if post.topic.private_message?
+      stat = user_stat || post.user.user_stat
+
+      if post.is_first_post?
+        stat.public_send(action, :topic_count)
+      elsif post.post_type == Post.types[:regular]
+        stat.public_send(action, :post_count)
+      end
+    end
+  end
+end

db/migrate/20220118065658_add_constraints_to_user_stat.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+class AddConstraintsToUserStat < ActiveRecord::Migration[6.1]
+  def up
+    execute(<<~SQL)
+    UPDATE user_stats
+    SET post_count = 0
+    WHERE post_count < 0
+    SQL
+
+    execute(<<~SQL)
+    UPDATE user_stats
+    SET topic_count = 0
+    WHERE topic_count < 0
+    SQL
+
+    execute "ALTER TABLE user_stats ADD CONSTRAINT topic_count_positive CHECK (topic_count >= 0)"
+    execute "ALTER TABLE user_stats ADD CONSTRAINT post_count_positive CHECK (post_count >= 0)"
+  end
+
+  def down
+    execute "ALTER TABLE user_stats DROP CONSTRAINT topic_count_positive"
+    execute "ALTER TABLE user_stats DROP CONSTRAINT post_count_positive"
+  end
+end

lib/guardian/user_guardian.rb

@@ -60,6 +60,7 @@ module UserGuardian
 
   def can_delete_user?(user)
     return false if user.nil? || user.admin?
+
     if is_me?(user)
       !SiteSetting.enable_discourse_connect &&
       !user.has_more_posts_than?(SiteSetting.delete_user_self_max_post_count)

lib/post_creator.rb

@@ -599,15 +599,10 @@ class PostCreator
     @user.create_user_stat if @user.user_stat.nil?
 
     if @user.user_stat.first_post_created_at.nil?
-      @user.user_stat.first_post_created_at = @post.created_at
+      @user.user_stat.update!(first_post_created_at: @post.created_at)
     end
 
-    unless @post.topic.private_message?
+    UserStatCountUpdater.increment!(@post)
-      @user.user_stat.post_count += 1 if @post.post_type == Post.types[:regular] && !@post.is_first_post?
-      @user.user_stat.topic_count += 1 if @post.is_first_post?
-    end
-
-    @user.user_stat.save!
 
     if !@topic.private_message? && @post.post_type != Post.types[:whisper]
       @user.update(last_posted_at: @post.created_at)

lib/post_destroyer.rb

@@ -155,8 +155,9 @@ class PostDestroyer
         make_previous_post_the_last_one
         mark_topic_changed
         clear_user_posted_flag
-        Topic.reset_highest(@post.topic_id)
       end
+
+      Topic.reset_highest(@post.topic_id)
       trash_public_post_actions
       trash_revisions
       trash_user_actions
@@ -177,7 +178,7 @@ class PostDestroyer
       end
       TopicLink.where(link_post_id: @post.id).destroy_all
       update_associated_category_latest_topic
-      update_user_counts
+      update_user_counts if !permanent?
       TopicUser.update_post_action_cache(post_id: @post.id)
 
       DB.after_commit do
@@ -387,17 +388,10 @@ class PostDestroyer
     author.create_user_stat if author.user_stat.nil?
 
     if @post.created_at == author.user_stat.first_post_created_at
-      author.user_stat.first_post_created_at = author.posts.order('created_at ASC').first.try(:created_at)
+      author.user_stat.update!(first_post_created_at: author.posts.order('created_at ASC').first.try(:created_at))
     end
 
-    if @post.topic && !@post.topic.private_message?
+    UserStatCountUpdater.decrement!(@post)
-      if @post.post_type == Post.types[:regular] && !@post.is_first_post? && !@topic.nil?
-        author.user_stat.post_count -= 1
-      end
-      author.user_stat.topic_count -= 1 if @post.is_first_post?
-    end
-
-    author.user_stat.save!
 
     if @post.created_at == author.last_posted_at
       author.last_posted_at = author.posts.order('created_at DESC').first.try(:created_at)
@@ -407,6 +401,7 @@ class PostDestroyer
     if @post.is_first_post? && @post.topic && !@post.topic.private_message?
       # Update stats of all people who replied
       counts = Post.where(post_type: Post.types[:regular], topic_id: @post.topic_id).where('post_number > 1').group(:user_id).count
+
       counts.each do |user_id, count|
         if user_stat = UserStat.where(user_id: user_id).first
           user_stat.update(post_count: user_stat.post_count - count)

lib/post_revisor.rb

@@ -437,22 +437,19 @@ class PostRevisor
       private_message = @topic.private_message?
 
       prev_owner_user_stat = prev_owner.user_stat
+
       unless private_message
-        prev_owner_user_stat.post_count -= 1 if @post.post_type == Post.types[:regular]
+        UserStatCountUpdater.decrement!(@post, user_stat: prev_owner_user_stat) if !@post.trashed?
-        prev_owner_user_stat.topic_count -= 1 if @post.is_first_post?
         prev_owner_user_stat.likes_received -= likes
       end
 
       if @post.created_at == prev_owner.user_stat.first_post_created_at
-        prev_owner_user_stat.first_post_created_at = prev_owner.posts.order('created_at ASC').first.try(:created_at)
+        prev_owner_user_stat.update!(first_post_created_at: prev_owner.posts.order('created_at ASC').first.try(:created_at))
       end
 
-      prev_owner_user_stat.save!
-
       new_owner_user_stat = new_owner.user_stat
       unless private_message
-        new_owner_user_stat.post_count += 1 if @post.post_type == Post.types[:regular]
+        UserStatCountUpdater.increment!(@post, user_stat: new_owner_user_stat) if !@post.trashed?
-        new_owner_user_stat.topic_count += 1 if @post.is_first_post?
         new_owner_user_stat.likes_received += likes
       end
       new_owner_user_stat.save!

spec/components/guardian/user_guardian_spec.rb

@@ -243,10 +243,11 @@ describe UserGuardian do
       end
 
       it "isn't allowed when user created too many posts" do
-        Fabricate(:post, user: user)
+        topic = Fabricate(:topic)
+        Fabricate(:post, topic: topic, user: user)
         expect(guardian.can_delete_user?(user)).to eq(true)
 
-        Fabricate(:post, user: user)
+        Fabricate(:post, topic: topic, user: user)
         expect(guardian.can_delete_user?(user)).to eq(false)
       end
 
@@ -319,16 +320,18 @@ describe UserGuardian do
       end
 
       it "correctly respects the delete_user_self_max_post_count setting" do
+        topic = Fabricate(:topic)
+
         SiteSetting.delete_user_self_max_post_count = 0
         expect(guardian.can_delete_user?(user)).to eq(true)
 
-        Fabricate(:post, user: user)
+        Fabricate(:post, topic: topic, user: user)
 
         expect(guardian.can_delete_user?(user)).to eq(false)
         SiteSetting.delete_user_self_max_post_count = 1
         expect(guardian.can_delete_user?(user)).to eq(true)
 
-        Fabricate(:post, user: user)
+        Fabricate(:post, topic: topic, user: user)
 
         expect(guardian.can_delete_user?(user)).to eq(false)
         SiteSetting.delete_user_self_max_post_count = 2

spec/components/guardian_spec.rb

@@ -1184,6 +1184,8 @@ describe Guardian do
   end
 
   describe "can_recover_topic?" do
+    fab!(:topic) { Fabricate(:topic, user: user) }
+    fab!(:post) { Fabricate(:post, user: user, topic: topic) }
 
     it "returns false for a nil user" do
       expect(Guardian.new(nil).can_recover_topic?(topic)).to be_falsey
@@ -1198,11 +1200,6 @@ describe Guardian do
     end
 
     context 'as a moderator' do
-      before do
-        topic.save!
-        post.save!
-      end
-
       describe 'when post has been deleted' do
         it "should return the right value" do
           expect(Guardian.new(moderator).can_recover_topic?(topic)).to be_falsey
@@ -1227,9 +1224,6 @@ describe Guardian do
       fab!(:group_user) { Fabricate(:group_user) }
 
       before do
-        topic.save!
-        post.save!
-
         SiteSetting.enable_category_group_moderation = true
         PostDestroyer.new(moderator, topic.first_post).destroy
         topic.reload
@@ -1262,10 +1256,8 @@ describe Guardian do
     end
 
     context 'as a moderator' do
-      before do
+      fab!(:topic) { Fabricate(:topic, user: user) }
-        topic.save!
+      fab!(:post) { Fabricate(:post, user: user, topic: topic) }
-        post.save!
-      end
 
       describe 'when post has been deleted' do
         it "should return the right value" do

spec/components/new_post_manager_spec.rb

@@ -4,12 +4,12 @@ require 'rails_helper'
 require 'new_post_manager'
 
 describe NewPostManager do
-
+  fab!(:user) { Fabricate(:user) }
   fab!(:topic) { Fabricate(:topic) }
 
   context "default action" do
     it "creates the post by default" do
-      manager = NewPostManager.new(topic.user, raw: 'this is a new post', topic_id: topic.id)
+      manager = NewPostManager.new(user, raw: 'this is a new post', topic_id: topic.id)
       result = manager.perform
 
       expect(result.action).to eq(:create_post)
@@ -25,7 +25,7 @@ describe NewPostManager do
     it "doesn't enqueue private messages" do
       SiteSetting.approve_unless_trust_level = 4
 
-      manager = NewPostManager.new(topic.user,
+      manager = NewPostManager.new(user,
                                    raw: 'this is a new post',
                                    title: 'this is a new title',
                                    archetype: Archetype.private_message,
@@ -40,7 +40,7 @@ describe NewPostManager do
       expect(result.post).to be_a(Post)
 
       # It doesn't enqueue replies to the private message either
-      manager = NewPostManager.new(topic.user,
+      manager = NewPostManager.new(user,
                                    raw: 'this is a new reply',
                                    topic_id: result.post.topic_id)
 
@@ -56,7 +56,7 @@ describe NewPostManager do
   end
 
   context "default handler" do
-    let(:manager) { NewPostManager.new(topic.user, raw: 'this is new post content', topic_id: topic.id) }
+    let(:manager) { NewPostManager.new(user, raw: 'this is new post content', topic_id: topic.id) }
 
     context 'with the settings zeroed out' do
       before do
@@ -126,8 +126,9 @@ describe NewPostManager do
     context 'with a high approval post count, but TL2' do
       before do
         SiteSetting.approve_post_count = 100
-        topic.user.trust_level = 2
+        user.update!(trust_level: 2)
       end
+
       it "will return an enqueue result" do
         result = NewPostManager.default_handler(manager)
         expect(result).to be_nil
@@ -188,8 +189,9 @@ describe NewPostManager do
     context 'with staged moderation setting enabled' do
       before do
         SiteSetting.approve_unless_staged = true
-        topic.user.staged = true
+        user.update!(staged: true)
       end
+
       it "will return an enqueue result" do
         result = NewPostManager.default_handler(manager)
         expect(NewPostManager.queue_enabled?).to eq(true)
@@ -209,17 +211,17 @@ describe NewPostManager do
     end
 
     context 'with a fast typer' do
-      let(:user) { manager.user }
-
       before do
         user.update!(trust_level: 0)
       end
 
       it "adds the silence reason in the system locale" do
         manager = build_manager_with('this is new post content')
+
         I18n.with_locale(:fr) do # Simulate french user
           result = NewPostManager.default_handler(manager)
         end
+
         expect(user.silenced?).to eq(true)
         expect(user.silence_reason).to eq(I18n.t("user.new_user_typed_too_fast", locale: :en))
       end
@@ -235,12 +237,11 @@ describe NewPostManager do
       end
 
       def build_manager_with(raw)
-        NewPostManager.new(topic.user, raw: raw, topic_id: topic.id, first_post_checks: true)
+        NewPostManager.new(user, raw: raw, topic_id: topic.id, first_post_checks: true)
       end
     end
 
     context 'with media' do
-      let(:user) { manager.user }
       let(:manager_opts) do
         {
           raw: 'this is new post content', topic_id: topic.id, first_post_checks: false,
@@ -258,7 +259,7 @@ describe NewPostManager do
 
       it 'queues the post for review because if it contains embedded media.' do
         SiteSetting.review_media_unless_trust_level = 1
-        manager = NewPostManager.new(topic.user, manager_opts)
+        manager = NewPostManager.new(user, manager_opts)
 
         result = NewPostManager.default_handler(manager)
 
@@ -268,7 +269,7 @@ describe NewPostManager do
 
       it 'does not enqueue the post if the poster is a trusted user' do
         SiteSetting.review_media_unless_trust_level = 0
-        manager = NewPostManager.new(topic.user, manager_opts)
+        manager = NewPostManager.new(user, manager_opts)
 
         result = NewPostManager.default_handler(manager)
 
@@ -278,7 +279,7 @@ describe NewPostManager do
   end
 
   context "new topic handler" do
-    let(:manager) { NewPostManager.new(topic.user, raw: 'this is new topic content', title: 'new topic title') }
+    let(:manager) { NewPostManager.new(user, raw: 'this is new topic content', title: 'new topic title') }
     context 'with a high trust level setting for new topics' do
       before do
         SiteSetting.approve_new_topics_unless_trust_level = 4
@@ -351,7 +352,7 @@ describe NewPostManager do
     end
 
     it "calls custom handlers" do
-      manager = NewPostManager.new(topic.user, raw: 'this post increases counter', topic_id: topic.id)
+      manager = NewPostManager.new(user, raw: 'this post increases counter', topic_id: topic.id)
 
       result = manager.perform
 
@@ -409,7 +410,7 @@ describe NewPostManager do
     end
 
     it "if nothing returns a result it creates a post" do
-      manager = NewPostManager.new(topic.user, raw: 'this is a new post', topic_id: topic.id)
+      manager = NewPostManager.new(user, raw: 'this is a new post', topic_id: topic.id)
 
       result = manager.perform
 

spec/components/post_destroyer_spec.rb

@@ -995,7 +995,7 @@ describe PostDestroyer do
     end
 
     it 'should destroy internal links when moderator deletes post' do
-      new_post = Post.create!(user: user, topic: topic, raw: "Link to other topic:\n\n#{url}\n")
+      new_post = create_post(user: user, topic: topic, raw: "Link to other topic:\n\n#{url}\n")
       TopicLink.extract_from(new_post)
       link_counts = TopicLink.counts_for(guardian, other_topic.reload, [other_post])
       expect(link_counts.count).to eq(1)

spec/fabricators/post_fabricator.rb

@@ -5,6 +5,11 @@ Fabricator(:post) do
   topic { |attrs| Fabricate(:topic, user: attrs[:user]) }
   raw "Hello world"
   post_type Post.types[:regular]
+
+  # Fabrication bypasses PostCreator, for performance reasons, where the counts are updated so we have to handle this manually here.
+  after_save do |post, _transients|
+    UserStatCountUpdater.increment!(post)
+  end
 end
 
 Fabricator(:post_with_long_raw_content, from: :post) do

spec/fabricators/topic_fabricator.rb

@@ -6,6 +6,13 @@ Fabricator(:topic) do
   category_id do |attrs|
     attrs[:category] ? attrs[:category].id : SiteSetting.uncategorized_category_id
   end
+
+  # Fabrication bypasses PostCreator, for performance reasons, where the counts are updated so we have to handle this manually here.
+  after_save do |topic, _transients|
+    if !topic.private_message?
+      topic.user.user_stat.increment!(:topic_count)
+    end
+  end
 end
 
 Fabricator(:deleted_topic, from: :topic) do

spec/models/topic_converter_spec.rb

@@ -68,7 +68,7 @@ describe TopicConverter do
         expect(private_message.user.user_stat.post_count).to eq(0)
         private_message.convert_to_public_topic(admin)
         expect(private_message.reload.user.user_stat.topic_count).to eq(1)
-        expect(private_message.user.user_stat.post_count).to eq(1)
+        expect(private_message.user.user_stat.post_count).to eq(0)
         expect(topic_user.reload.notification_level).to eq(TopicUser.notification_levels[:watching])
       end
 

spec/models/topic_link_spec.rb

@@ -83,15 +83,14 @@ describe TopicLink do
 
     it "extracts onebox" do
       other_topic = Fabricate(:topic, user: user)
-      other_topic.posts.create(user: user, raw: "some content for the first post")
+      Fabricate(:post, topic: other_topic, user: user, raw: "some content for the first post")
-      other_post = other_topic.posts.create(user: user, raw: "some content for the second post")
+      other_post = Fabricate(:post, topic: other_topic, user: user, raw: "some content for the second post")
 
       url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}/#{other_post.post_number}"
       invalid_url = "http://#{test_uri.host}/t/#{other_topic.slug}/9999999999999999999999999999999"
 
-      topic.posts.create(user: user, raw: 'initial post')
+      Fabricate(:post, topic: topic, user: user, raw: 'initial post')
-      post = topic.posts.create(user: user, raw: "Link to another topic:\n\n#{url}\n\n#{invalid_url}")
+      post = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic:\n\n#{url}\n\n#{invalid_url}")
-      post.reload
 
       TopicLink.extract_from(post)
 
@@ -110,7 +109,7 @@ describe TopicLink do
       fab!(:moderator) { Fabricate(:moderator) }
 
       let(:post) do
-        other_topic.posts.create(user: user, raw: "some content")
+        Fabricate(:post, topic: other_topic, user: user, raw: "some content")
       end
 
       it 'works' do
@@ -120,8 +119,8 @@ describe TopicLink do
 
         url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
 
-        topic.posts.create(user: user, raw: 'initial post')
+        Fabricate(:post, topic: topic, user: user, raw: 'initial post')
-        linked_post = topic.posts.create(user: user, raw: "Link to another topic: #{url}")
+        linked_post = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{url}")
 
         # this is subtle, but we had a bug were second time
         # TopicLink.extract_from was called a reflection was nuked
@@ -169,8 +168,8 @@ describe TopicLink do
       it 'works without id' do
         post
         url = "http://#{test_uri.host}/t/#{other_topic.slug}"
-        topic.posts.create(user: user, raw: 'initial post')
+        Fabricate(:post, topic: topic, user: user, raw: 'initial post')
-        linked_post = topic.posts.create(user: user, raw: "Link to another topic: #{url}")
+        linked_post = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{url}")
 
         TopicLink.extract_from(linked_post)
         link = topic.topic_links.first
@@ -191,8 +190,8 @@ describe TopicLink do
         post
         url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
 
-        topic.posts.create(user: user, raw: 'initial post')
+        Fabricate(:post, topic: topic, user: user, raw: 'initial post')
-        linked_post = topic.posts.create(user: user, raw: "Link to another topic: #{url}")
+        linked_post = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{url}")
         TopicLink.extract_from(linked_post)
         expect(other_topic.reload.topic_links.where(link_post_id: linked_post.id).count).to eq(1)
 
@@ -207,10 +206,11 @@ describe TopicLink do
         long_title = "Καλημερα σε ολους και ολες" * 9 # 234 chars, but the encoded slug will be 1224 chars in length
         other_topic = Fabricate(:topic, user: user, title: long_title)
         expect(other_topic.slug.length).to be > TopicLink.max_url_length
-        other_topic.posts.create(user: user, raw: 'initial post')
+
+        Fabricate(:post, topic: other_topic, user: user, raw: 'initial post')
         other_topic_url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
 
-        post_with_link = topic.posts.create(user: user, raw: "Link to another topic: #{other_topic_url}")
+        post_with_link = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{other_topic_url}")
         TopicLink.extract_from(post_with_link)
         topic.reload
         link = topic.topic_links.first
@@ -226,10 +226,10 @@ describe TopicLink do
         topic_url = "http://#{test_uri.host}/t/#{topic.slug}/#{topic.id}"
 
         other_topic = Fabricate(:topic, user: user)
-        other_topic.posts.create(user: user, raw: 'initial post')
+        Fabricate(:post, topic: other_topic, user: user, raw: 'initial post')
         other_topic_url = "http://#{test_uri.host}/t/#{other_topic.slug}/#{other_topic.id}"
 
-        post_with_link = topic.posts.create(user: user, raw: "Link to another topic: #{other_topic_url}")
+        post_with_link = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{other_topic_url}")
         expect { TopicLink.extract_from(post_with_link) }.to_not raise_error
 
         other_topic.reload
@@ -240,7 +240,8 @@ describe TopicLink do
     end
 
     context "link to a user on discourse" do
-      let(:post) { topic.posts.create(user: user, raw: "<a href='/u/#{user.username_lower}'>user</a>") }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: "<a href='/u/#{user.username_lower}'>user</a>") }
+
       before do
         TopicLink.extract_from(post)
       end
@@ -251,7 +252,7 @@ describe TopicLink do
     end
 
     context "link to a discourse resource like a FAQ" do
-      let(:post) { topic.posts.create(user: user, raw: "<a href='/faq'>faq link here</a>") }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: "<a href='/faq'>faq link here</a>") }
       before do
         TopicLink.extract_from(post)
       end
@@ -262,7 +263,7 @@ describe TopicLink do
     end
 
     context "mention links" do
-      let(:post) { topic.posts.create(user: user, raw: "Hey #{user.username_lower}") }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: "Hey #{user.username_lower}") }
 
       before do
         TopicLink.extract_from(post)
@@ -275,14 +276,14 @@ describe TopicLink do
 
     context "email address" do
       it "does not extract a link" do
-        post = topic.posts.create(user: user, raw: "Valid email: foo@bar.com\n\nInvalid email: rfc822;name@domain.com")
+        post = Fabricate(:post, topic: topic, user: user, raw: "Valid email: foo@bar.com\n\nInvalid email: rfc822;name@domain.com")
         TopicLink.extract_from(post)
         expect(topic.topic_links).to be_blank
       end
     end
 
     context "mail link" do
-      let(:post) { topic.posts.create(user: user, raw: "[email]bar@example.com[/email]") }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: "[email]bar@example.com[/email]") }
 
       it 'does not extract a link' do
         TopicLink.extract_from(post)
@@ -292,7 +293,7 @@ describe TopicLink do
 
     context "quote links" do
       it "sets quote correctly" do
-        linked_post = topic.posts.create(user: user, raw: "my test post")
+        linked_post = Fabricate(:post, topic: topic, user: user, raw: "my test post")
         quoting_post = Fabricate(:post, raw: "[quote=\"#{user.username}, post: #{linked_post.post_number}, topic: #{topic.id}\"]\nquote\n[/quote]")
 
         TopicLink.extract_from(quoting_post)
@@ -304,7 +305,7 @@ describe TopicLink do
     end
 
     context "link to a local attachments" do
-      let(:post) { topic.posts.create(user: user, raw: '<a class="attachment" href="/uploads/default/208/87bb3d8428eb4783.rb?foo=bar">ruby.rb</a>') }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: '<a class="attachment" href="/uploads/default/208/87bb3d8428eb4783.rb?foo=bar">ruby.rb</a>') }
 
       it "extracts the link" do
         TopicLink.extract_from(post)
@@ -324,7 +325,7 @@ describe TopicLink do
     end
 
     context "link to an attachments uploaded on S3" do
-      let(:post) { topic.posts.create(user: user, raw: '<a class="attachment" href="//s3.amazonaws.com/bucket/2104a0211c9ce41ed67989a1ed62e9a394c1fbd1446.rb">ruby.rb</a>') }
+      let(:post) { Fabricate(:post, topic: topic, user: user, raw: '<a class="attachment" href="//s3.amazonaws.com/bucket/2104a0211c9ce41ed67989a1ed62e9a394c1fbd1446.rb">ruby.rb</a>') }
 
       it "extracts the link" do
         TopicLink.extract_from(post)
@@ -347,12 +348,12 @@ describe TopicLink do
   describe 'internal link from pm' do
     it 'works' do
       pm = Fabricate(:topic, user: user, category_id: nil, archetype: 'private_message')
-      pm.posts.create(user: user, raw: "some content")
+      Fabricate(:post, topic: pm, user: user, raw: "some content")
 
       url = "http://#{test_uri.host}/t/topic-slug/#{topic.id}"
 
-      pm.posts.create(user: user, raw: 'initial post')
+      Fabricate(:post, topic: pm, user: user, raw: 'initial post')
-      linked_post = pm.posts.create(user: user, raw: "Link to another topic: #{url}")
+      linked_post = Fabricate(:post, topic: pm, user: user, raw: "Link to another topic: #{url}")
 
       TopicLink.extract_from(linked_post)
 
@@ -367,8 +368,8 @@ describe TopicLink do
       unlisted_topic = Fabricate(:topic, user: user, visible: false)
       url = "http://#{test_uri.host}/t/topic-slug/#{topic.id}"
 
-      unlisted_topic.posts.create(user: user, raw: 'initial post')
+      Fabricate(:post, topic: unlisted_topic, user: user, raw: 'initial post')
-      linked_post = unlisted_topic.posts.create(user: user, raw: "Link to another topic: #{url}")
+      linked_post = Fabricate(:post, topic: unlisted_topic, user: user, raw: "Link to another topic: #{url}")
 
       TopicLink.extract_from(linked_post)
 
@@ -384,7 +385,7 @@ describe TopicLink do
       alternate_uri = URI.parse(Discourse.base_url)
 
       url = "http://#{alternate_uri.host}:5678/t/topic-slug/#{other_topic.id}"
-      post = topic.posts.create(user: user, raw: "Link to another topic: #{url}")
+      post = Fabricate(:post, topic: topic, user: user, raw: "Link to another topic: #{url}")
       TopicLink.extract_from(post)
       reflection = other_topic.topic_links.first
 

spec/models/topic_spec.rb

@@ -2933,12 +2933,12 @@ describe Topic do
     end
 
     it 'returns error message if topic has more posts' do
-      post_2 = PostCreator.create!(Fabricate(:user), topic_id: topic.id, raw: 'some post content')
+      post_2 = create_post(user: user, topic_id: topic.id, raw: 'some post content')
 
       PostDestroyer.new(admin, post).destroy
       expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(I18n.t('post.cannot_permanently_delete.many_posts'))
 
-      PostDestroyer.new(admin, post_2).destroy
+      PostDestroyer.new(admin, post_2.reload).destroy
       expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(nil)
     end
 

spec/models/user_spec.rb

@@ -287,28 +287,31 @@ describe User do
   end
 
   describe 'delete posts in batches' do
-    before_all do
+    fab!(:post1) { Fabricate(:post) }
-      @post1 = Fabricate(:post)
+    fab!(:user) { post1.user }
-      @user = @post1.user
+    fab!(:post2) { Fabricate(:post, topic: post1.topic, user: user) }
-      @post2 = Fabricate(:post, topic: @post1.topic, user: @user)
+    fab!(:post3) { Fabricate(:post, user: user) }
-      @post3 = Fabricate(:post, user: @user)
+    fab!(:posts) { [post1, post2, post3] }
-      @posts = [@post1, @post2, @post3]
+    fab!(:post_ids) { [post1.id, post2.id, post3.id] }
-      @guardian = Guardian.new(Fabricate(:admin))
+    fab!(:guardian) { Guardian.new(Fabricate(:admin)) }
-      Fabricate(:reviewable_queued_post, created_by: @user)
+    fab!(:reviewable_queued_post) { Fabricate(:reviewable_queued_post, created_by: user) }
-    end
 
     it 'deletes only one batch of posts' do
-      deleted_posts = @user.delete_posts_in_batches(@guardian, 1)
+      post2
-      expect(Post.where(id: @posts.map(&:id)).count).to eq(2)
+      deleted_posts = user.delete_posts_in_batches(guardian, 1)
+      expect(Post.where(id: post_ids).count).to eq(2)
       expect(deleted_posts.length).to eq(1)
-      expect(deleted_posts[0]).to eq(@post2)
+      expect(deleted_posts[0]).to eq(post2)
     end
 
     it 'correctly deletes posts and topics' do
-      @user.delete_posts_in_batches(@guardian, 20)
+      posts
-      expect(Post.where(id: @posts.map(&:id))).to be_empty
+      user.delete_posts_in_batches(guardian, 20)
-      expect(Reviewable.where(created_by: @user).count).to eq(0)
+
-      @posts.each do |p|
+      expect(Post.where(id: post_ids)).to be_empty
+      expect(Reviewable.where(created_by: user).count).to eq(0)
+
+      posts.each do |p|
         if p.is_first_post?
           expect(Topic.find_by(id: p.topic_id)).to be_nil
         end
@@ -319,10 +322,10 @@ describe User do
       invalid_guardian = Guardian.new(Fabricate(:user))
 
       expect do
-        @user.delete_posts_in_batches(invalid_guardian)
+        user.delete_posts_in_batches(invalid_guardian)
       end.to raise_error Discourse::InvalidAccess
 
-      @posts.each do |p|
+      posts.each do |p|
         p.reload
         expect(p).to be_present
         expect(p.topic).to be_present

spec/requests/posts_controller_spec.rb

@@ -874,6 +874,8 @@ describe PostsController do
     end
 
     describe "when logged in" do
+      fab!(:user) { Fabricate(:user) }
+
       before do
         sign_in(user)
       end
@@ -919,7 +921,7 @@ describe PostsController do
         end
 
         it "doesn't enqueue posts when user first creates a topic" do
-          user.user_stat.update_column(:topic_count, 1)
+          Fabricate(:topic, user: user)
 
           Draft.set(user, "should_clear", 0, "{'a' : 'b'}")
 

spec/services/post_owner_changer_spec.rb

@@ -112,7 +112,7 @@ describe PostOwnerChanger do
 
         p1user.user_stat.update!(
           topic_count: 1,
-          post_count: 1,
+          post_count: 0,
           first_post_created_at: p1.created_at,
         )
 
@@ -151,7 +151,7 @@ describe PostOwnerChanger do
         expect(p2user.topic_count).to eq(0)
         expect(p2user.post_count).to eq(0)
         expect(user_a.topic_count).to eq(1)
-        expect(user_a.post_count).to eq(2)
+        expect(user_a.post_count).to eq(1)
 
         p1_user_stat = p1user.user_stat