From 701ecddac222248bd6d70bd28f78c851db0a588d Mon Sep 17 00:00:00 2001 From: Grant Ammons Date: Sat, 9 Feb 2013 10:33:07 -0500 Subject: [PATCH] factor out @post.revise into its own class. clean up PostRevisor class to be more readable --- app/models/post.rb | 83 ++------------ lib/edit_rate_limiter.rb | 6 + lib/post_revisor.rb | 83 ++++++++++++++ spec/components/post_revisor_spec.rb | 163 +++++++++++++++++++++++++++ spec/models/post_spec.rb | 2 +- 5 files changed, 263 insertions(+), 74 deletions(-) create mode 100644 lib/edit_rate_limiter.rb create mode 100644 lib/post_revisor.rb create mode 100644 spec/components/post_revisor_spec.rb diff --git a/app/models/post.rb b/app/models/post.rb index 2e71eb425b9..066cac96ba7 100644 --- a/app/models/post.rb +++ b/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 @@ class Post < ActiveRecord::Base 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 @@ class Post < ActiveRecord::Base @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 @@ class Post < ActiveRecord::Base 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?) diff --git a/lib/edit_rate_limiter.rb b/lib/edit_rate_limiter.rb new file mode 100644 index 00000000000..f91291edae2 --- /dev/null +++ b/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 diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb new file mode 100644 index 00000000000..c8ec6449eac --- /dev/null +++ b/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 diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb new file mode 100644 index 00000000000..386211f472a --- /dev/null +++ b/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 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index c58f44c2b63..88c943ec0ca 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -370,7 +370,7 @@ describe Post do 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