From c478ffc6620cdc83c2a7e830b05b7cb7354663f3 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Thu, 1 Apr 2021 06:46:18 +0530
Subject: [PATCH] FIX: post merging was failing silently (#12566)

https://meta.discourse.org/t/merging-very-long-posts-removes-them/183597
---
 .../javascripts/discourse/app/models/post.js    |  2 +-
 app/controllers/posts_controller.rb             |  2 ++
 config/locales/server.en.yml                    |  1 +
 lib/post_merger.rb                              | 15 +++++++++++----
 lib/validators/stripped_length_validator.rb     | 17 ++++++++++-------
 spec/components/post_merger_spec.rb             | 13 +++++++++++++
 6 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js
index 4c3a671ddff..77e1967f2ed 100644
--- a/app/assets/javascripts/discourse/app/models/post.js
+++ b/app/assets/javascripts/discourse/app/models/post.js
@@ -400,7 +400,7 @@ Post.reopenClass({
     return ajax("/posts/merge_posts", {
       type: "PUT",
       data: { post_ids },
-    });
+    }).catch(popupAjaxError);
   },
 
   loadRevision(postId, version) {
diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index 7123851c98e..981bd706211 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -363,6 +363,8 @@ class PostsController < ApplicationController
     raise Discourse::InvalidParameters.new(:post_ids) if posts.pluck(:id) == params[:post_ids]
     PostMerger.new(current_user, posts).merge
     render body: nil
+  rescue PostMerger::CannotMergeError => e
+    render_json_error(e.message)
   end
 
   # Direct replies to this post
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index e6b66b1d42a..83426bbf769 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2382,6 +2382,7 @@ en:
     errors:
       different_topics: "Posts belonging to different topics cannot be merged."
       different_users: "Posts belonging to different users cannot be merged."
+      max_post_length: "Posts cannot be merged because the combined post length is more than allowed."
 
   move_posts:
     new_topic_moderator_post:
diff --git a/lib/post_merger.rb b/lib/post_merger.rb
index e80a2ac8c09..1cfabc3fbbe 100644
--- a/lib/post_merger.rb
+++ b/lib/post_merger.rb
@@ -24,14 +24,14 @@ class PostMerger
     post_content = posts.map(&:raw)
     post = posts.pop
 
+    merged_post_raw = post_content.join("\n\n")
     changes = {
-      raw: post_content.join("\n\n"),
+      raw: merged_post_raw,
       edit_reason: I18n.t("merge_posts.edit_reason", count: posts.length, username: @user.username)
     }
 
-    revisor = PostRevisor.new(post, post.topic)
-
-    revisor.revise!(@user, changes) do
+    ensure_max_post_length!(merged_post_raw)
+    PostRevisor.new(post, post.topic).revise!(@user, changes) do
       posts.each { |p| PostDestroyer.new(@user, p).destroy }
     end
   end
@@ -57,4 +57,11 @@ class PostMerger
   def ensure_staff_user!(guardian)
     raise Discourse::InvalidAccess unless guardian.is_staff?
   end
+
+  def ensure_max_post_length!(raw)
+    value = StrippedLengthValidator.get_sanitized_value(raw)
+    if value.length > SiteSetting.max_post_length
+      raise CannotMergeError.new(I18n.t("merge_posts.errors.max_post_length"))
+    end
+  end
 end
diff --git a/lib/validators/stripped_length_validator.rb b/lib/validators/stripped_length_validator.rb
index de736197771..b2ff7f82efd 100644
--- a/lib/validators/stripped_length_validator.rb
+++ b/lib/validators/stripped_length_validator.rb
@@ -3,13 +3,7 @@
 class StrippedLengthValidator < ActiveModel::EachValidator
   def self.validate(record, attribute, value, range)
     if !value.nil?
-      value = value.dup
-      value.gsub!(/<!--(.*?)-->/, '') # strip HTML comments
-      value.gsub!(/:\w+(:\w+)?:/, "X") # replace emojis with a single character
-      value.gsub!(/\.{2,}/, '…') # replace multiple ... with …
-      value.gsub!(/\,{2,}/, ',') # replace multiple ,,, with ,
-      value.strip!
-
+      value = get_sanitized_value(value)
       record.errors.add attribute, (I18n.t('errors.messages.too_short', count: range.begin)) if value.length < range.begin
       record.errors.add attribute, (I18n.t('errors.messages.too_long_validation', max: range.end, length: value.length)) if value.length > range.end
     else
@@ -22,4 +16,13 @@ class StrippedLengthValidator < ActiveModel::EachValidator
     range = options[:in].lambda? ? options[:in].call : options[:in]
     self.class.validate(record, attribute, value, range)
   end
+
+  def self.get_sanitized_value(value)
+    value = value.dup
+    value.gsub!(/<!--(.*?)-->/, '') # strip HTML comments
+    value.gsub!(/:\w+(:\w+)?:/, "X") # replace emojis with a single character
+    value.gsub!(/\.{2,}/, '…') # replace multiple ... with …
+    value.gsub!(/\,{2,}/, ',') # replace multiple ,,, with ,
+    value.strip
+  end
 end
diff --git a/spec/components/post_merger_spec.rb b/spec/components/post_merger_spec.rb
index 08a7ffe1f87..be1b97a4d1c 100644
--- a/spec/components/post_merger_spec.rb
+++ b/spec/components/post_merger_spec.rb
@@ -66,5 +66,18 @@ describe PostMerger do
         PostMerger::CannotMergeError, I18n.t("merge_posts.errors.different_users")
       )
     end
+
+    it "should not allow posts with length greater than max_post_length" do
+      SiteSetting.max_post_length = 60
+
+      reply1 = create_post(topic: topic, raw: 'The first reply', post_number: 2, user: user)
+      reply2 = create_post(topic: topic, raw: "The second reply\nSecond line", post_number: 3, user: user)
+      reply3 = create_post(topic: topic, raw: 'The third reply', post_number: 4, user: user)
+      replies = [reply3, reply2, reply1]
+
+      expect { PostMerger.new(admin, replies).merge }.to raise_error(
+        PostMerger::CannotMergeError, I18n.t("merge_posts.errors.max_post_length")
+      )
+    end
   end
 end