FIX: Ensure revisions are made to store edit reasons and no reasons get wiped (#8363)

* Fix an issue where if an edit was made to a post with a reason provided, and then another edit was made with no reason, the original edit reason got wiped out
* We now always make a post revision (even with ninja edits) if an edit reason has been provided and it is different from the current edit reason

Co-Authored-By: Sam <sam.saffron@gmail.com>

lib/post_revisor.rb

@@ -130,8 +130,9 @@ class PostRevisor
     @fields[:user_id] = @fields[:user_id].to_i if @fields.has_key?(:user_id)
     @fields[:category_id] = @fields[:category_id].to_i if @fields.has_key?(:category_id)
 
-    # always reset edit_reason unless provided
-    @fields[:edit_reason] = nil unless @fields[:edit_reason].present?
+    # always reset edit_reason unless provided, do not set to nil else
+    # previous reasons are lost
+    @fields.delete(:edit_reason) if @fields[:edit_reason].blank?
 
     return false unless should_revise?
 
@@ -244,7 +245,11 @@ class PostRevisor
 
   def should_create_new_version?
     return false if @skip_revision
-    edited_by_another_user? || !ninja_edit? || owner_changed? || force_new_version?
+    edited_by_another_user? || !ninja_edit? || owner_changed? || force_new_version? || edit_reason_specified?
+  end
+
+  def edit_reason_specified?
+    @fields[:edit_reason].present? && @fields[:edit_reason] != @post.edit_reason
   end
 
   def edited_by_another_user?

spec/components/post_revisor_spec.rb

@@ -198,6 +198,35 @@ describe PostRevisor do
       end
     end
 
+    describe 'edit reasons' do
+      it "does create a new version if an edit reason is provided" do
+        post = Fabricate(:post, raw: 'hello world')
+        revisor = PostRevisor.new(post)
+        revisor.revise!(post.user, { raw: 'hello world123456789', edit_reason: 'this is my reason' }, revised_at: post.updated_at + 1.second)
+        post.reload
+        expect(post.version).to eq(2)
+        expect(post.revisions.count).to eq(1)
+      end
+
+      it "does not create a new version if an edit reason is provided and its the same as the current edit reason" do
+        post = Fabricate(:post, raw: 'hello world', edit_reason: 'this is my reason')
+        revisor = PostRevisor.new(post)
+        revisor.revise!(post.user, { raw: 'hello world123456789', edit_reason: 'this is my reason' }, revised_at: post.updated_at + 1.second)
+        post.reload
+        expect(post.version).to eq(1)
+        expect(post.revisions.count).to eq(0)
+      end
+
+      it "does not clobber the existing edit reason for a revision if it is not provided in a subsequent revision" do
+        post = Fabricate(:post, raw: 'hello world')
+        revisor = PostRevisor.new(post)
+        revisor.revise!(post.user, { raw: 'hello world123456789', edit_reason: 'this is my reason' }, revised_at: post.updated_at + 1.second)
+        post.reload
+        revisor.revise!(post.user, { raw: 'hello some other thing' }, revised_at: post.updated_at + 1.second)
+        expect(post.revisions.first.modifications[:edit_reason]).to eq([nil, 'this is my reason'])
+      end
+    end
+
     describe 'revision much later' do
 
       let!(:revised_at) { post.updated_at + 2.minutes }