From ec3758b5736b8e3d1d4626311b948206aac55f0f Mon Sep 17 00:00:00 2001
From: Dan Ungureanu <dan@ungureanu.me>
Date: Tue, 9 Nov 2021 16:29:37 +0200
Subject: [PATCH] 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).
---
 lib/post_revisor.rb                  |  7 ++++---
 spec/components/post_revisor_spec.rb | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb
index 42b70d00380..f3b8aa7333e 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 207f2177830..d0c318a2c11 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) }