Merge pull request #903 from chrishunt/chrishunt/extract-post-move

Extract post move

app/models/post.rb

@@ -317,6 +317,10 @@ class Post < ActiveRecord::Base
     result
   end
 
+  def is_first_post?
+    post_number == 1
+  end
+
   def is_flagged?
     post_actions.where(post_action_type_id: PostActionType.flag_types.values, deleted_at: nil).count != 0
   end

app/models/post_mover.rb

@@ -0,0 +1,117 @@
+class PostMover
+  attr_reader :original_topic, :destination_topic, :user, :post_ids
+
+  def initialize(original_topic, user, post_ids)
+    @original_topic = original_topic
+    @user = user
+    @post_ids = post_ids
+  end
+
+  def to_topic(id)
+    Topic.transaction do
+      move_posts_to Topic.find_by_id(id)
+    end
+  end
+
+  def to_new_topic(title)
+    Topic.transaction do
+      move_posts_to Topic.create!(
+        user: user,
+        title: title,
+        category: original_topic.category
+      )
+    end
+  end
+
+  private
+
+  def move_posts_to(topic)
+    Guardian.new(user).ensure_can_see! topic
+    @destination_topic = topic
+
+    move_posts_to_destination_topic
+    destination_topic
+  end
+
+  def move_posts_to_destination_topic
+    move_each_post
+    notify_users_that_posts_have_moved
+    update_statistics
+  end
+
+  def move_each_post
+    with_max_post_number do |max_post_number|
+      posts.each_with_index do |post, offset|
+        post.is_first_post? ? copy(post) : move(post, offset + max_post_number)
+      end
+    end
+  end
+
+  def copy(post)
+    PostCreator.create(
+      post.user,
+      raw: post.raw,
+      topic_id: destination_topic.id,
+      acting_user: user
+    )
+  end
+
+  def move(post, post_number)
+    @first_post_number_moved ||= post.post_number
+
+    Post.update_all(
+      [
+        ['post_number = :post_number',
+         'topic_id    = :topic_id',
+         'sort_order  = :post_number'
+        ].join(', '),
+        post_number: post_number,
+        topic_id: destination_topic.id
+      ],
+      id: post.id,
+      topic_id: original_topic.id
+    )
+  end
+
+  def update_statistics
+    destination_topic.update_statistics
+    original_topic.update_statistics
+  end
+
+  def notify_users_that_posts_have_moved
+    enqueue_notification_job
+    create_moderator_post_in_original_topic
+  end
+
+  def enqueue_notification_job
+    Jobs.enqueue(
+      :notify_moved_posts,
+      post_ids: post_ids,
+      moved_by_id: user.id
+    )
+  end
+
+  def create_moderator_post_in_original_topic
+    original_topic.add_moderator_post(
+      user,
+      I18n.t(
+        "move_posts.moderator_post",
+        count: post_ids.count,
+        topic_link: "[#{destination_topic.title}](#{destination_topic.url})"
+      ),
+      post_number: @first_post_number_moved
+    )
+  end
+
+  def with_max_post_number
+    yield destination_topic.max_post_number + 1
+  end
+
+  def posts
+    @posts ||= begin
+      Post.where(id: post_ids).order(:created_at).tap do |posts|
+        raise Discourse::InvalidParameters.new(:post_ids) if posts.empty?
+      end
+    end
+  end
+end

app/models/topic.rb

@@ -447,70 +447,18 @@ class Topic < ActiveRecord::Base
     invite
   end
 
-  def move_posts_to_topic(moved_by, post_ids, destination_topic)
-    to_move = posts.where(id: post_ids).order(:created_at)
-    raise Discourse::InvalidParameters.new(:post_ids) if to_move.blank?
-
-    first_post_number = nil
-    Topic.transaction do
-      # Find the max post number in the topic
-      max_post_number = destination_topic.posts.maximum(:post_number) || 0
-
-      to_move.each_with_index do |post, i|
-        if post.post_number == 1
-          # We have a special case for the OP, we copy it instead of deleting it.
-          result = PostCreator.new(post.user,
-                                  raw: post.raw,
-                                  topic_id: destination_topic.id,
-                                  acting_user: moved_by).create
-        else
-          first_post_number ||= post.post_number
-          # Move the post and raise an error if it couldn't be moved
-          row_count = Post.update_all ["post_number = :post_number, topic_id = :topic_id, sort_order = :post_number", post_number: max_post_number+i+1, topic_id: destination_topic.id], id: post.id, topic_id: id
-          raise Discourse::InvalidParameters.new(:post_ids) if row_count == 0
-        end
-      end
-    end
-
-
-    first_post_number
+  def max_post_number
+    posts.maximum(:post_number).to_i
   end
 
   def move_posts(moved_by, post_ids, opts)
+    post_mover = PostMover.new(self, moved_by, post_ids)
 
-    topic = nil
-    first_post_number = nil
-
-    if opts[:title].present?
-      # If we're moving to a new topic...
-      Topic.transaction do
-        topic = Topic.create(user: moved_by, title: opts[:title], category: category)
-        first_post_number = move_posts_to_topic(moved_by, post_ids, topic)
+    if opts[:destination_topic_id]
+      post_mover.to_topic opts[:destination_topic_id]
+    elsif opts[:title]
+      post_mover.to_new_topic opts[:title]
     end
-
-    elsif opts[:destination_topic_id].present?
-      # If we're moving to an existing topic...
-
-      topic = Topic.where(id: opts[:destination_topic_id]).first
-      Guardian.new(moved_by).ensure_can_see!(topic)
-      first_post_number = move_posts_to_topic(moved_by, post_ids, topic)
-
-    end
-
-    # Add a moderator post explaining that the post was moved
-    if topic.present?
-      topic_url = "#{Discourse.base_url}#{topic.relative_url}"
-      topic_link = "[#{topic.title}](#{topic_url})"
-
-      add_moderator_post(moved_by, I18n.t("move_posts.moderator_post", count: post_ids.size, topic_link: topic_link), post_number: first_post_number)
-      Jobs.enqueue(:notify_moved_posts, post_ids: post_ids, moved_by_id: moved_by.id)
-
-      topic.update_statistics
-      update_statistics
-    end
-
-
-    topic
   end
 
   # Updates the denormalized statistics of a topic including featured posters. They shouldn't
@@ -678,6 +626,10 @@ class Topic < ActiveRecord::Base
     url
   end
 
+  def url(post_number = nil)
+    self.class.url id, slug, post_number
+  end
+
   def relative_url(post_number=nil)
     url = "/t/#{slug}/#{id}"
     url << "/#{post_number}" if post_number.to_i > 1

spec/models/topic_spec.rb

@@ -204,10 +204,15 @@ describe Topic do
         lambda { topic.move_posts(user, [1003], title: "new testing topic name") }.should raise_error(Discourse::InvalidParameters)
       end
 
-      it "raises an error if no posts were moved" do
-        lambda { topic.move_posts(user, [], title: "new testing topic name") }.should raise_error(Discourse::InvalidParameters)
-      end
+      it "raises an error and does not create a topic if no posts were moved" do
+        Topic.count.tap do |original_topic_count|
+          lambda {
+            topic.move_posts(user, [], title: "new testing topic name")
+          }.should raise_error(Discourse::InvalidParameters)
 
+          expect(Topic.count).to eq original_topic_count
+        end
+      end
     end
 
     context "successfully moved" do