FIX: Make PostRevisor more consistent (#14841)

FIX: Make PostRevisor more consistent (#14841)

  • FIX: Preserve field types when updating revision

When a post was edited quickly twice by the same user, the old post revision was updated with the newest changes. To check if the change was reverted (i.e. rename topic A to B and then back to A) a comparison of the initial value and last value is performed. If the check passes then the intermediary value is dismissed and only the initial value and the last ones are preserved. Otherwise, the modification is dismissed because the field returned to its initial value.

This used to work well for most fields, but failed for “tags” because the field is an array and the values were transformed to strings to perform the comparison.

  • FIX: Reset last_editor_id if revision is reverted

If a post was revised and then the same revision was reverted, last_editor_id was still set to the ID of the user who last edited the post. This was a problem because the same person could then edit the same post again and because it was the same user and same post, the system attempted to update the last one (that did not exist anymore).

diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb
index 42b70d0..f3b8aa7 100644
--- a/lib/post_revisor.rb
+++ b/lib/post_revisor.rb
@@ -531,9 +531,9 @@ class PostRevisor
 
     modifications.each_key do |field|
       if revision.modifications.has_key?(field)
-        old_value = revision.modifications[field][0].to_s
-        new_value = modifications[field][1].to_s
-        if old_value != new_value
+        old_value = revision.modifications[field][0]
+        new_value = modifications[field][1]
+        if old_value.to_s != new_value.to_s
           revision.modifications[field] = [old_value, new_value]
         else
           revision.modifications.delete(field)
@@ -545,6 +545,7 @@ class PostRevisor
     # should probably do this before saving the post!
     if revision.modifications.empty?
       revision.destroy
+      @post.last_editor_id = PostRevision.where(post_id: @post.id).order(number: :desc).pluck_first(:user_id) || @post.user_id
       @post.version -= 1
       @post.public_version -= 1
       @post.save
diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb
index 207f217..d0c318a 100644
--- a/spec/components/post_revisor_spec.rb
+++ b/spec/components/post_revisor_spec.rb
@@ -148,6 +148,22 @@ describe PostRevisor do
 
     subject { PostRevisor.new(post) }
 
+    it 'destroys last revision if edit is undone' do
+      old_raw = post.raw
+
+      subject.revise!(admin, raw: 'new post body', tags: ['new-tag'])
+      expect(post.topic.reload.tags.map(&:name)).to contain_exactly('new-tag')
+      expect(post.post_revisions.reload.size).to eq(1)
+
+      subject.revise!(admin, raw: old_raw, tags: [])
+      expect(post.topic.reload.tags.map(&:name)).to be_empty
+      expect(post.post_revisions.reload.size).to eq(0)
+
+      subject.revise!(admin, raw: 'next post body', tags: ['new-tag'])
+      expect(post.topic.reload.tags.map(&:name)).to contain_exactly('new-tag')
+      expect(post.post_revisions.reload.size).to eq(1)
+    end
+
     describe 'with the same body' do
       it "doesn't change version" do
         expect {
@@ -703,6 +719,17 @@ describe PostRevisor do
       expect(post.revisions.first.modifications["archetype"][1]).to eq(new_archetype)
     end
 
+    it "revises and tracks changes of topic tags" do
+      subject.revise!(admin, tags: ['new-tag'])
+      expect(post.post_revisions.last.modifications).to eq('tags' => [[], ['new-tag']])
+
+      subject.revise!(admin, tags: ['new-tag', 'new-tag-2'])
+      expect(post.post_revisions.last.modifications).to eq('tags' => [[], ['new-tag', 'new-tag-2']])
+
+      subject.revise!(admin, tags: ['new-tag-3'])
+      expect(post.post_revisions.last.modifications).to eq('tags' => [[], ['new-tag-3']])
+    end
+
     context "#publish_changes" do
       let!(:post) { Fabricate(:post, topic: topic) }
 

GitHub sha: ec3758b5736b8e3d1d4626311b948206aac55f0f

This commit appears in #14841 which was approved by ZogStriP. It was merged by udan11.