diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 488fd00d9f4..13b8fe4f6bf 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1023,6 +1023,7 @@ en: download_remote_images_max_days_old: "Don't download remote images for posts that are more than n days old." disabled_image_download_domains: "Remote images will never be downloaded from these domains. Pipe-delimited list." editing_grace_period: "For (n) seconds after posting, editing will not create a new version in the post history." + editing_grace_period_max_diff: "Maximum number of character changes allowed in editing grace period, if more changed store another post revision" staff_edit_locks_post: "Posts will be locked from editing if they are edited by staff members" post_edit_time_limit: "The author can edit or delete their post for (n) minutes after posting. Set to 0 for forever." edit_history_visible_to_public: "Allow everyone to see previous versions of an edited post. When disabled, only staff members can view." diff --git a/config/site_settings.yml b/config/site_settings.yml index 04e47f67df6..dccfb5e750d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -534,6 +534,7 @@ posting: client: true validator: "EnablePrivateEmailMessagesValidator" editing_grace_period: 300 + editing_grace_period_max_diff: 20 staff_edit_locks_post: false post_edit_time_limit: default: 86400 diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 6e024a4c016..7346fca6679 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -214,7 +214,15 @@ class PostRevisor end def revise_post - should_create_new_version? ? revise_and_create_new_version : revise + if should_create_new_version? + revise_and_create_new_version + else + unless cached_original_raw + self.original_raw = @post.raw + self.original_cooked = @post.cooked + end + revise + end end def should_create_new_version? @@ -226,9 +234,57 @@ class PostRevisor @post.last_editor_id != @editor.id end + def original_raw_key + "original_raw_#{(@last_version_at.to_f * 1000).to_i}#{@post.id}" + end + + def original_cooked_key + "original_cooked_#{(@last_version_at.to_f * 1000).to_i}#{@post.id}" + end + + def cached_original_raw + @cached_original_raw ||= $redis.get(original_raw_key) + end + + def cached_original_cooked + @cached_original_cooked ||= $redis.get(original_cooked_key) + end + + def original_raw + cached_original_raw || @post.raw + end + + def original_raw=(val) + @cached_original_raw = val + $redis.setex(original_raw_key, SiteSetting.editing_grace_period + 1, val) + end + + def original_cooked=(val) + @cached_original_cooked = val + $redis.setex(original_cooked_key, SiteSetting.editing_grace_period + 1, val) + end + + def diff_size(before, after) + changes = 0 + ONPDiff.new(before, after).short_diff.each do |str, type| + next if type == :common + changes += str.length + end + changes + end + def ninja_edit? return false if @post.has_active_flag? - @revised_at - @last_version_at <= SiteSetting.editing_grace_period.to_i + return false if (@revised_at - @last_version_at) > SiteSetting.editing_grace_period.to_i + + if new_raw = @fields[:raw] + if (original_raw.length - new_raw.length).abs > SiteSetting.editing_grace_period_max_diff.to_i || + diff_size(original_raw, new_raw) > SiteSetting.editing_grace_period_max_diff.to_i + return false + end + end + + true end def owner_changed? @@ -368,6 +424,15 @@ class PostRevisor def create_revision modifications = post_changes.merge(@topic_changes.diff) + + if modifications["raw"] + modifications["raw"][0] = cached_original_raw || modifications["raw"][0] + end + + if modifications["cooked"] + modifications["cooked"][0] = cached_original_cooked || modifications["cooked"][0] + end + PostRevision.create!( user_id: @post.last_editor_id, post_id: @post.id, @@ -380,6 +445,7 @@ class PostRevisor return unless revision = PostRevision.find_by(post_id: @post.id, number: @post.version) revision.user_id = @post.last_editor_id modifications = post_changes.merge(@topic_changes.diff) + modifications.each_key do |field| if revision.modifications.has_key?(field) old_value = revision.modifications[field][0].to_s diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index b8be5cf46ec..cf04958ff3a 100644 --- a/spec/components/post_revisor_spec.rb +++ b/spec/components/post_revisor_spec.rb @@ -111,8 +111,30 @@ describe PostRevisor do expect(subject.category_changed).to be_blank end + it "does create a new version if a large diff happens" do + SiteSetting.editing_grace_period_max_diff = 10 + + post = Fabricate(:post, raw: 'hello world') + revisor = PostRevisor.new(post) + revisor.revise!(post.user, { raw: 'hello world123456789' }, revised_at: post.updated_at + 1.second) + + post.reload + + expect(post.version).to eq(1) + + revisor = PostRevisor.new(post) + revisor.revise!(post.user, { raw: 'hello world12345678901' }, revised_at: post.updated_at + 1.second) + + post.reload + expect(post.version).to eq(2) + + expect(post.revisions.first.modifications["raw"][0]).to eq("hello world") + expect(post.revisions.first.modifications["cooked"][0]).to eq("
hello world
") + end + it "doesn't create a new version" do SiteSetting.editing_grace_period = 1.minute + SiteSetting.editing_grace_period_max_diff = 100 # making a revision subject.revise!(post.user, { raw: 'updated body' }, revised_at: post.updated_at + SiteSetting.editing_grace_period + 1.seconds) @@ -317,48 +339,34 @@ describe PostRevisor do end describe 'with a new body' do + before do + SiteSetting.editing_grace_period_max_diff = 1000 + end + let(:changed_by) { Fabricate(:coding_horror) } let!(:result) { subject.revise!(changed_by, raw: "lets update the body. Здравствуйте") } - it 'returns true' do + it 'correctly updates raw' do expect(result).to eq(true) - end - - it 'updates the body' do expect(post.raw).to eq("lets update the body. Здравствуйте") - end - - it 'sets the invalidate oneboxes attribute' do expect(post.invalidate_oneboxes).to eq(true) - end - - it 'increased the versions' do expect(post.version).to eq(2) expect(post.public_version).to eq(2) - end - - it 'has the new revision' do expect(post.revisions.size).to eq(1) - end - - it "saved the user who made the change in the revisions" do expect(post.revisions.first.user_id).to eq(changed_by.id) - end - it "updates the word count" do + # updates word count expect(post.word_count).to eq(5) post.topic.reload expect(post.topic.word_count).to eq(5) end context 'second poster posts again quickly' do - before do + + it 'is a ninja edit, because the second poster posted again quickly' do SiteSetting.editing_grace_period = 1.minute subject.revise!(changed_by, { raw: '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 expect(post.version).to eq(2) expect(post.public_version).to eq(2) expect(post.revisions.size).to eq(1) diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index f706ea5003d..9336d2ad1b7 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -674,7 +674,7 @@ describe Post do context 'second poster posts again quickly' do it 'is a ninja edit, because the second poster posted again quickly' do - SiteSetting.expects(:editing_grace_period).returns(1.minute.to_i) + SiteSetting.editing_grace_period = 1.minute.to_i post.revise(changed_by, { raw: 'yet another updated body' }, revised_at: post.updated_at + 10.seconds) post.reload diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 559013b1526..f244004200b 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -734,8 +734,8 @@ describe Topic do it "doesn't bump the topic on an edit to the last post that doesn't result in a new version" do expect { - SiteSetting.expects(:editing_grace_period).returns(5.minutes) - @last_post.revise(@last_post.user, { raw: 'updated contents' }, revised_at: @last_post.created_at + 10.seconds) + SiteSetting.editing_grace_period = 5.minutes + @last_post.revise(@last_post.user, { raw: @last_post.raw + "a" }, revised_at: @last_post.created_at + 10.seconds) @topic.reload }.not_to change(@topic, :bumped_at) end