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

@@ -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
+        old_value = revision.modifications[field][0]
-        new_value = modifications[field][1].to_s
+        new_value = modifications[field][1]
-        if old_value != new_value
+        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

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) }