FIX: remove latest empty revision

lib/post_revisor.rb

@@ -91,7 +91,7 @@ class PostRevisor
     @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.has_key?(:edit_reason)
+    @fields[:edit_reason] = nil unless @fields[:edit_reason].present?
 
     return false unless should_revise?
 
@@ -199,17 +199,18 @@ class PostRevisor
     create_or_update_revision
   end
 
-  def update_post
-    prev_owner, new_owner = nil, nil
-    if @fields.has_key?("user_id") && @fields["user_id"] != @post.user_id
-      prev_owner = User.find_by_id(@post.user_id)
-      new_owner = User.find_by_id(@fields["user_id"])
+  USER_ACTIONS_TO_REMOVE ||= [UserAction::NEW_TOPIC, UserAction::REPLY, UserAction::RESPONSE]
+
+  def update_post
+    if @fields.has_key?("user_id") && @fields["user_id"] != @post.user_id
+      prev_owner = User.find(@post.user_id)
+      new_owner = User.find(@fields["user_id"])
 
-      UserAction.where( target_post_id: @post.id,
-                        user_id: prev_owner.id,
-                        action_type: [UserAction::NEW_TOPIC, UserAction::REPLY, UserAction::RESPONSE] )
-                .find_each { |ua| ua.destroy }
       # UserActionObserver will create new UserAction records for the new owner
+      UserAction.where(target_post_id: @post.id)
+                .where(user_id: prev_owner.id)
+                .where(action_type: USER_ACTIONS_TO_REMOVE)
+                .destroy_all
     end
 
     POST_TRACKED_FIELDS.each do |field|
@@ -229,17 +230,20 @@ class PostRevisor
 
     # post owner changed
     if prev_owner && new_owner && prev_owner != new_owner
-      likes = UserAction.where( target_post_id: @post.id,
-                                user_id: prev_owner.id,
-                                action_type: UserAction::WAS_LIKED ).update_all(user_id: new_owner.id)
+      likes = UserAction.where(target_post_id: @post.id)
+                        .where(user_id: prev_owner.id)
+                        .where(action_type: UserAction::WAS_LIKED)
+                        .update_all(user_id: new_owner.id)
 
       prev_owner.user_stat.post_count -= 1
       prev_owner.user_stat.topic_count -= 1 if @post.is_first_post?
       prev_owner.user_stat.likes_received -= likes
       prev_owner.user_stat.update_topic_reply_count
+
       if @post.created_at == prev_owner.user_stat.first_post_created_at
         prev_owner.user_stat.first_post_created_at = prev_owner.posts.order('created_at ASC').first.try(:created_at)
       end
+
       prev_owner.user_stat.save
 
       new_owner.user_stat.post_count += 1
@@ -302,14 +306,26 @@ class PostRevisor
     modifications = post_changes.merge(@topic_changes.diff)
     modifications.each_key do |field|
       if revision.modifications.has_key?(field)
-        old_value = revision.modifications[field][0]
-        new_value = modifications[field][1]
-        revision.modifications[field] = [old_value, new_value]
+        old_value = revision.modifications[field][0].to_s
+        new_value = modifications[field][1].to_s
+        if old_value != new_value
+          revision.modifications[field] = [old_value, new_value]
+        else
+          revision.modifications.delete(field)
+        end
       else
         revision.modifications[field] = modifications[field]
       end
     end
-    revision.save if modifications.length
+    # should probably do this before saving the post!
+    if revision.modifications.empty?
+      revision.destroy
+      @post.version -= 1
+      @post.public_version -= 1
+      @post.save
+    else
+      revision.save
+    end
   end
 
   def post_changes
@@ -410,4 +426,3 @@ class PostRevisor
   end
 
 end
-

spec/components/post_revisor_spec.rb

@@ -5,7 +5,7 @@ describe PostRevisor do
 
   let(:topic) { Fabricate(:topic) }
   let(:newuser) { Fabricate(:newuser) }
-  let(:post_args) { {user: newuser, topic: topic} }
+  let(:post_args) { { user: newuser, topic: topic } }
 
   context 'TopicChanges' do
     let(:topic) { Fabricate(:topic) }
@@ -74,7 +74,8 @@ describe PostRevisor do
 
     describe 'ninja editing' do
       it 'correctly applies edits' do
-        SiteSetting.ninja_edit_window = 1.minute.to_i
+        SiteSetting.stubs(:ninja_edit_window).returns(1.minute)
+
         subject.revise!(post.user, { raw: 'updated body' }, revised_at: post.updated_at + 10.seconds)
         post.reload
 
@@ -84,6 +85,21 @@ describe PostRevisor do
         expect(post.last_version_at).to eq(first_version_at)
         expect(subject.category_changed).to be_blank
       end
+
+      it "doesn't create a new version" do
+        SiteSetting.stubs(:ninja_edit_window).returns(1.minute)
+
+        # making a revision
+        subject.revise!(post.user, { raw: 'updated body' }, revised_at: post.updated_at + SiteSetting.ninja_edit_window + 1.seconds)
+        # "roll back"
+        subject.revise!(post.user, { raw: 'Hello world' }, revised_at: post.updated_at + SiteSetting.ninja_edit_window + 2.seconds)
+
+        post.reload
+
+        expect(post.version).to eq(1)
+        expect(post.public_version).to eq(1)
+        expect(post.revisions.size).to eq(0)
+      end
     end
 
     describe 'revision much later' do