factor out @post.revise into its own class. clean up PostRevisor class to be more readable

Author: Grant Ammons

app/models/post.rb

@@ -1,6 +1,7 @@
 require_dependency 'jobs'
 require_dependency 'pretty_text'
 require_dependency 'rate_limiter'
+require_dependency 'post_revisor'
 
 require 'archetype'
 require 'hpricot'
@@ -14,12 +15,6 @@ module HiddenReason
     FLAG_THRESHOLD_REACHED_AGAIN = 2
   end
 
-  # A custom rate limiter for edits
-  class EditRateLimiter < RateLimiter
-    def initialize(user)
-      super(user, "edit-post:#{Date.today.to_s}", SiteSetting.max_edits_per_day, 1.day.to_i)
-    end
-  end
 
   versioned
 
@@ -115,6 +110,11 @@ def cooked_document
     @cooked_document ||= Nokogiri::HTML.fragment(self.cooked)
   end
 
+  def reset_cooked
+    @cooked_document = nil
+    self.cooked = nil
+  end
+
   def image_count
     return 0 unless self.raw.present?
     cooked_document.search("img.emoji").remove
@@ -290,77 +290,14 @@ def unhide!
     self.save
   end
 
-  # Update the body of a post. Will create a new version when appropriate
-  def revise(updated_by, new_raw, opts={})
-
-    # Only update if it changes
-    return false if self.raw == new_raw
-
-    updater = lambda do |new_version=false|
-
-      # Raw is about to change, enable validations
-      @cooked_document = nil
-      self.cooked = nil
-
-      self.raw = new_raw
-      self.updated_by = updated_by
-      self.last_editor_id = updated_by.id
-
-      if self.hidden && self.hidden_reason_id == HiddenReason::FLAG_THRESHOLD_REACHED
-        self.hidden = false
-        self.hidden_reason_id = nil
-        self.topic.update_attributes(visible: true)
-
-        PostAction.clear_flags!(self, -1)
-      end
-
-      self.save
-    end
-
-    # We can optionally specify when this version was revised. Defaults to now.
-    revised_at = opts[:revised_at] || Time.now
-    new_version = false
-
-    # We always create a new version if the poster has changed
-    new_version = true if (self.last_editor_id != updated_by.id)
-
-    # We always create a new version if it's been greater than the ninja edit window
-    new_version = true if (revised_at - last_version_at) > SiteSetting.ninja_edit_window.to_i
-
-    new_version = true if opts[:force_new_version]
-
-    # Create the new version (or don't)
-    if new_version
-
-      self.cached_version = version + 1
-
-      Post.transaction do
-        self.last_version_at = revised_at
-        updater.call(true)
-        EditRateLimiter.new(updated_by).performed! unless opts[:bypass_rate_limiter]
-
-        # If a new version is created of the last post, bump it.
-        unless Post.where('post_number > ? and topic_id = ?', self.post_number, self.topic_id).exists?
-          topic.update_column(:bumped_at, Time.now) unless opts[:bypass_bump]
-        end
-      end
-
-    else
-      skip_version(&updater)
-    end
-
-    # Invalidate any oneboxes
-    self.invalidate_oneboxes = true
-    trigger_post_process
-
-    true
-  end
-
-
   def url
     "/t/#{Slug.for(topic.title)}/#{topic.id}/#{post_number}"
   end
 
+  def revise(updated_by, new_raw, opts={})
+    PostRevisor.new(self).revise!(updated_by, new_raw, opts)
+  end
+
   # Various callbacks
   before_create do
     self.post_number ||= Topic.next_post_number(topic_id, reply_to_post_number.present?)

lib/edit_rate_limiter.rb

@@ -0,0 +1,6 @@
+require 'rate_limiter'
+class EditRateLimiter < RateLimiter
+  def initialize(user)
+    super(user, "edit-post:#{Date.today.to_s}", SiteSetting.max_edits_per_day, 1.day.to_i)
+  end
+end

lib/post_revisor.rb

@@ -0,0 +1,83 @@
+require 'edit_rate_limiter'
+class PostRevisor
+  def initialize(post)
+    @post = post
+  end
+
+  def revise!(user, new_raw, opts = {})
+    @user, @new_raw, @opts = user, new_raw, opts
+    return false if not should_revise?
+    revise_post
+    post_process_post
+    true
+  end
+
+  private
+
+  def should_revise?
+    @post.raw != @new_raw
+  end
+
+  def revise_post
+    if should_create_new_version?
+      revise_and_create_new_version
+    else
+      revise_without_creating_a_new_version
+    end
+  end
+
+  def get_revised_at
+    @opts[:revised_at] || Time.now
+  end
+
+  def should_create_new_version?
+    (@post.last_editor_id != @user.id) or
+      ((get_revised_at - @post.last_version_at) > SiteSetting.ninja_edit_window.to_i) or
+      @opts[:force_new_version] == true
+  end
+
+  def revise_and_create_new_version
+    Post.transaction do
+      @post.cached_version = @post.version + 1
+      @post.last_version_at = get_revised_at
+      update_post
+      EditRateLimiter.new(@post.user).performed! unless @opts[:bypass_rate_limiter] == true
+      bump_topic unless @opts[:bypass_bump]
+    end
+  end
+
+  def revise_without_creating_a_new_version
+    @post.skip_version do
+      update_post
+    end
+  end
+
+  def bump_topic
+    unless Post.where('post_number > ? and topic_id = ?', @post.post_number, @post.topic_id).exists?
+      @post.topic.update_column(:bumped_at, Time.now)
+    end
+  end
+
+  def update_post
+    @post.reset_cooked
+
+    @post.raw = @new_raw
+    @post.updated_by = @user
+    @post.last_editor_id = @user.id
+
+    if @post.hidden && @post.hidden_reason_id == Post::HiddenReason::FLAG_THRESHOLD_REACHED
+      @post.hidden = false
+      @post.hidden_reason_id = nil
+      @post.topic.update_attributes(visible: true)
+
+      PostAction.clear_flags!(@post, -1)
+    end
+
+    @post.save
+  end
+
+  def post_process_post
+    @post.invalidate_oneboxes = true
+    @post.trigger_post_process
+  end
+end

spec/components/post_revisor_spec.rb

@@ -0,0 +1,163 @@
+require 'spec_helper'
+require 'post_revisor'
+
+describe PostRevisor do
+
+  let(:topic) { Fabricate(:topic) }
+  let(:post_args) { {user: topic.user, topic: topic} }
+
+  context 'revise' do
+
+    let(:post) { Fabricate(:post, post_args) }
+    let(:first_version_at) { post.last_version_at }
+
+    subject { described_class.new(post) }
+
+    describe 'with the same body' do
+
+      it 'returns false' do
+        subject.revise!(post.user, post.raw).should be_false
+      end
+
+      it "doesn't change cached_version" do
+        lambda { subject.revise!(post.user, post.raw); post.reload }.should_not change(post, :cached_version)
+      end
+
+    end
+
+    describe 'ninja editing' do
+      before do
+        SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i)
+        subject.revise!(post.user, 'updated body', revised_at: post.updated_at + 10.seconds)
+        post.reload
+      end
+
+      it 'does not update cached_version' do
+        post.cached_version.should == 1
+      end
+
+      it 'does not create a new version' do
+        post.all_versions.size.should == 1
+      end
+
+      it "doesn't change the last_version_at" do
+        post.last_version_at.should == first_version_at
+      end
+    end
+
+    describe 'revision much later' do
+
+      let!(:revised_at) { post.updated_at + 2.minutes }
+
+      before do
+        SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i)
+        subject.revise!(post.user, 'updated body', revised_at: revised_at)
+        post.reload
+      end
+
+      it 'updates the cached_version' do
+        post.cached_version.should == 2
+      end
+
+      it 'creates a new version' do
+        post.all_versions.size.should == 2
+      end
+
+      it "updates the last_version_at" do
+        post.last_version_at.to_i.should == revised_at.to_i
+      end
+
+      describe "new edit window" do
+
+        before do
+          subject.revise!(post.user, 'yet another updated body', revised_at: revised_at)
+          post.reload
+        end
+
+        it "doesn't create a new version if you do another" do
+          post.cached_version.should == 2
+        end
+
+        it "doesn't change last_version_at" do
+          post.last_version_at.to_i.should == revised_at.to_i
+        end
+
+        context "after second window" do
+
+          let!(:new_revised_at) {revised_at + 2.minutes}
+
+          before do
+            subject.revise!(post.user, 'yet another, another updated body', revised_at: new_revised_at)
+            post.reload
+          end
+
+          it "does create a new version after the edit window" do
+            post.cached_version.should == 3
+          end
+
+          it "does create a new version after the edit window" do
+            post.last_version_at.to_i.should == new_revised_at.to_i
+          end
+        end
+      end
+    end
+
+    describe 'rate limiter' do
+      let(:changed_by) { Fabricate(:coding_horror) }
+
+      it "triggers a rate limiter" do
+        EditRateLimiter.any_instance.expects(:performed!)
+        subject.revise!(changed_by, 'updated body')
+      end
+    end
+
+    describe 'with a new body' do
+      let(:changed_by) { Fabricate(:coding_horror) }
+      let!(:result) { subject.revise!(changed_by, 'updated body') }
+
+      it 'returns true' do
+        result.should be_true
+      end
+
+      it 'updates the body' do
+        post.raw.should == 'updated body'
+      end
+
+      it 'sets the invalidate oneboxes attribute' do
+        post.invalidate_oneboxes.should == true
+      end
+
+      it 'increased the cached_version' do
+        post.cached_version.should == 2
+      end
+
+      it 'has the new version in all_versions' do
+        post.all_versions.size.should == 2
+      end
+
+      it 'has versions' do
+        post.versions.should be_present
+      end
+
+      it "saved the user who made the change in the version" do
+        post.versions.first.user.should be_present
+      end
+
+      context 'second poster posts again quickly' do
+        before do
+          SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i)
+          subject.revise!(changed_by, 'yet another updated body', revised_at: post.updated_at + 10.seconds)
+          post.reload
+        end
+
+        it 'is a ninja edit, because the second poster posted again quickly' do
+          post.cached_version.should == 2
+        end
+
+        it 'is a ninja edit, because the second poster posted again quickly' do
+          post.all_versions.size.should == 2
+        end
+      end
+    end
+  end
+end

spec/models/post_spec.rb

@@ -370,7 +370,7 @@
       let(:changed_by) { Fabricate(:coding_horror) }
 
       it "triggers a rate limiter" do
-        Post::EditRateLimiter.any_instance.expects(:performed!)
+        EditRateLimiter.any_instance.expects(:performed!)
         post.revise(changed_by, 'updated body')
       end
     end