Merge pull request #4843 from tgxworld/disable_post_min_length_for_pm

FEATURE: Disable minimum post length check when in PM with non human …

app/assets/javascripts/discourse/models/composer.js.es6

@@ -284,13 +284,14 @@ const Composer = RestModel.extend({
 
     @property minimumTitleLength
   **/
-  minimumTitleLength: function() {
-    if (this.get('privateMessage')) {
+  @computed('privateMessage')
+  minimumTitleLength(privateMessage) {
+    if (privateMessage) {
       return this.siteSettings.min_private_message_title_length;
     } else {
       return this.siteSettings.min_topic_title_length;
     }
-  }.property('privateMessage'),
+  },
 
   @computed('minimumPostLength', 'replyLength', 'canEditTopicFeaturedLink')
   missingReplyCharacters(minimumPostLength, replyLength, canEditTopicFeaturedLink) {
@@ -304,16 +305,19 @@ const Composer = RestModel.extend({
 
     @property minimumPostLength
   **/
-  minimumPostLength: function() {
-    if( this.get('privateMessage') ) {
+  @computed('privateMessage', 'topicFirstPost', 'topic.pm_with_non_human_user')
+  minimumPostLength(privateMessage, topicFirstPost, pmWithNonHumanUser) {
+    if (pmWithNonHumanUser) {
+      return 1;
+    } else if (privateMessage) {
       return this.siteSettings.min_private_message_post_length;
-    } else if (this.get('topicFirstPost')) {
+    } else if (topicFirstPost) {
       // first post (topic body)
       return this.siteSettings.min_first_post_length;
     } else {
       return this.siteSettings.min_post_length;
     }
-  }.property('privateMessage', 'topicFirstPost'),
+  },
 
   /**
     Computes the length of the title minus non-significant whitespaces

app/models/topic.rb

@@ -1187,6 +1187,15 @@ SQL
     private_topic
   end
 
+  def pm_with_non_human_user?
+    Topic.private_messages
+      .joins("LEFT JOIN topic_allowed_groups ON topics.id = topic_allowed_groups.topic_id")
+      .where("topic_allowed_groups.topic_id IS NULL")
+      .where("topics.id = ?", self.id)
+      .where("(SELECT COUNT(*) FROM topic_allowed_users WHERE topic_allowed_users.topic_id = ? AND topic_allowed_users.user_id > 0) = 1", self.id)
+      .exists?
+  end
+
   private
 
   def update_category_topic_count_by(num)

app/serializers/topic_view_serializer.rb

@@ -34,7 +34,8 @@ class TopicViewSerializer < ApplicationSerializer
                         :word_count,
                         :deleted_at,
                         :pending_posts_count,
-                        :user_id
+                        :user_id,
+                        :pm_with_non_human_user?
 
   attributes :draft,
              :draft_key,
@@ -71,7 +72,7 @@ class TopicViewSerializer < ApplicationSerializer
       last_poster: BasicUserSerializer.new(topic.last_poster, scope: scope, root: false)
     }
 
-    if object.topic.private_message?
+    if private_message?(topic)
       allowed_user_ids = Set.new
 
       result[:allowed_groups] = object.topic.allowed_groups.map do |group|
@@ -129,7 +130,7 @@ class TopicViewSerializer < ApplicationSerializer
   end
 
   def is_warning
-    object.topic.private_message? && object.topic.subtype == TopicSubtype.moderator_warning
+    private_message?(object.topic) && object.topic.subtype == TopicSubtype.moderator_warning
   end
 
   def include_is_warning?
@@ -149,7 +150,7 @@ class TopicViewSerializer < ApplicationSerializer
   end
 
   def include_message_archived?
-    object.topic.private_message?
+    private_message?(object.topic)
   end
 
   def message_archived
@@ -274,4 +275,14 @@ class TopicViewSerializer < ApplicationSerializer
     gsub_emoji_to_unicode(object.topic.title)
   end
 
+  def include_pm_with_non_human_user?
+    private_message?(object.topic)
+  end
+
+  private
+
+    def private_message?(topic)
+      @private_message ||= topic.private_message?
+    end
+
 end

lib/validators/post_validator.rb

@@ -30,7 +30,7 @@ class Validators::PostValidator < ActiveModel::Validator
   end
 
   def post_body_validator(post)
-    return if options[:skip_post_body]
+    return if options[:skip_post_body] || post.topic&.pm_with_non_human_user?
     stripped_length(post)
     raw_quality(post)
   end

spec/components/validators/post_validator_spec.rb

@@ -5,13 +5,41 @@ describe Validators::PostValidator do
   let(:post) { build(:post) }
   let(:validator) { Validators::PostValidator.new({}) }
 
-  context "when empty raw can bypass post body validation" do
-    let(:validator) { Validators::PostValidator.new(skip_post_body: true) }
-
-    it "should be allowed for empty raw based on site setting" do
+  context "#post_body_validator" do
+    it 'should not allow a post with an empty raw' do
       post.raw = ""
       validator.post_body_validator(post)
-      expect(post.errors).to be_empty
+      expect(post.errors).to_not be_empty
+    end
+
+    context "when empty raw can bypass validation" do
+      let(:validator) { Validators::PostValidator.new(skip_post_body: true) }
+
+      it "should be allowed for empty raw based on site setting" do
+        post.raw = ""
+        validator.post_body_validator(post)
+        expect(post.errors).to be_empty
+      end
+    end
+
+    describe "when post's topic is a PM between a human and a non human user" do
+      let(:robot) { Fabricate(:user, id: -3) }
+      let(:user) { Fabricate(:user) }
+
+      let(:topic) do
+        Fabricate(:private_message_topic, topic_allowed_users: [
+          Fabricate.build(:topic_allowed_user, user: robot),
+          Fabricate.build(:topic_allowed_user, user: user)
+        ])
+      end
+
+      it 'should allow a post with an empty raw' do
+        post = Fabricate.build(:post, topic: topic)
+        post.raw = ""
+        validator.post_body_validator(post)
+
+        expect(post.errors).to be_empty
+      end
     end
   end
 

spec/fabricators/post_fabricator.rb

@@ -126,8 +126,8 @@ Fabricator(:private_message_post, from: :post) do
       created_at: attrs[:created_at],
       subtype: TopicSubtype.user_to_user,
       topic_allowed_users: [
-        Fabricate.build(:topic_allowed_user, user_id: attrs[:user].id),
-        Fabricate.build(:topic_allowed_user, user_id: Fabricate(:user).id)
+        Fabricate.build(:topic_allowed_user, user: attrs[:user]),
+        Fabricate.build(:topic_allowed_user, user: Fabricate(:user))
       ]
     )
   end

spec/fabricators/topic_allowed_group_fabricator.rb

@@ -0,0 +1,4 @@
+Fabricator(:topic_allowed_group) do
+  topic
+  group
+end

spec/fabricators/topic_allowed_user_fabricator.rb

@@ -0,0 +1,3 @@
+Fabricator(:topic_allowed_user) do
+  user
+end

spec/fabricators/topic_fabricator.rb

@@ -12,9 +12,6 @@ Fabricator(:closed_topic, from: :topic) do
   closed true
 end
 
-Fabricator(:topic_allowed_user) do
-end
-
 Fabricator(:banner_topic, from: :topic) do
   archetype Archetype.banner
 end
@@ -25,7 +22,7 @@ Fabricator(:private_message_topic, from: :topic) do
   title { sequence(:title) { |i| "This is a private message #{i}" } }
   archetype "private_message"
   topic_allowed_users{|t| [
-    Fabricate.build(:topic_allowed_user, user_id: t[:user].id),
-    Fabricate.build(:topic_allowed_user, user_id: Fabricate(:coding_horror).id)
+    Fabricate.build(:topic_allowed_user, user: t[:user]),
+    Fabricate.build(:topic_allowed_user, user: Fabricate(:coding_horror))
   ]}
 end

spec/models/topic_spec.rb

@@ -1867,4 +1867,54 @@ describe Topic do
       expect(Topic.with_no_response_total).to eq(1)
     end
   end
+
+  describe '#pm_with_non_human_user?' do
+    let(:robot) { Fabricate(:user, id: -3) }
+    let(:user) { Fabricate(:user) }
+
+    let(:topic) do
+      Fabricate(:private_message_topic, topic_allowed_users: [
+        Fabricate.build(:topic_allowed_user, user: robot),
+        Fabricate.build(:topic_allowed_user, user: user)
+      ])
+    end
+
+    describe 'when PM is between a human and a non human user' do
+      it 'should return true' do
+        expect(topic.pm_with_non_human_user?).to be(true)
+      end
+    end
+
+    describe 'when PM contains 2 human users and a non human user' do
+      it 'should return false' do
+        Fabricate(:topic_allowed_user, topic: topic, user: Fabricate(:user))
+
+        expect(topic.pm_with_non_human_user?).to be(false)
+      end
+    end
+
+    describe 'when PM only contains a user' do
+      it 'should return true' do
+        topic.topic_allowed_users.first.destroy!
+
+        expect(topic.reload.pm_with_non_human_user?).to be(true)
+      end
+    end
+
+    describe 'when PM contains a group' do
+      it 'should return false' do
+        Fabricate(:topic_allowed_group, topic: topic)
+
+        expect(topic.pm_with_non_human_user?).to be(false)
+      end
+    end
+
+    describe 'when topic is not a PM' do
+      it 'should return false' do
+        topic.convert_to_public_topic(Fabricate(:admin))
+
+        expect(topic.pm_with_non_human_user?).to be(false)
+      end
+    end
+  end
 end

test/javascripts/models/composer-test.js.es6

@@ -160,6 +160,14 @@ test("Title length for private messages", function() {
   ok(composer.get('titleLengthValid'), "in the range is okay");
 });
 
+test("Post length for private messages with non human users", function() {
+  const composer = createComposer({
+    topic: Ember.Object.create({ pm_with_non_human_user: true })
+  });
+
+  equal(composer.get('minimumPostLength'), 1);
+});
+
 test('editingFirstPost', function() {
   const composer = createComposer();
   ok(!composer.get('editingFirstPost'), "it's false by default");