diff --git a/app/assets/javascripts/discourse/models/composer.js.es6 b/app/assets/javascripts/discourse/models/composer.js.es6 index c81e89e6ed..197f861bd2 100644 --- a/app/assets/javascripts/discourse/models/composer.js.es6 +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index abda4fa6e0..2dd0b9b835 100644 --- a/app/models/topic.rb +++ b/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) diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 802ccad671..c438bacb6f 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/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 diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb index a1836eb8f3..3ed62e3740 100644 --- a/lib/validators/post_validator.rb +++ b/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 diff --git a/spec/components/validators/post_validator_spec.rb b/spec/components/validators/post_validator_spec.rb index e0f984b1e5..25893f34e5 100644 --- a/spec/components/validators/post_validator_spec.rb +++ b/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 diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index e462230efc..4a28dac1c0 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/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 diff --git a/spec/fabricators/topic_allowed_group_fabricator.rb b/spec/fabricators/topic_allowed_group_fabricator.rb new file mode 100644 index 0000000000..e864faf6f9 --- /dev/null +++ b/spec/fabricators/topic_allowed_group_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:topic_allowed_group) do + topic + group +end diff --git a/spec/fabricators/topic_allowed_user_fabricator.rb b/spec/fabricators/topic_allowed_user_fabricator.rb new file mode 100644 index 0000000000..27c08d78b3 --- /dev/null +++ b/spec/fabricators/topic_allowed_user_fabricator.rb @@ -0,0 +1,3 @@ +Fabricator(:topic_allowed_user) do + user +end diff --git a/spec/fabricators/topic_fabricator.rb b/spec/fabricators/topic_fabricator.rb index bd6eff9413..e603a1244f 100644 --- a/spec/fabricators/topic_fabricator.rb +++ b/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 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 8a8db5e174..9db33f78bd 100644 --- a/spec/models/topic_spec.rb +++ b/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 diff --git a/test/javascripts/models/composer-test.js.es6 b/test/javascripts/models/composer-test.js.es6 index d7bcf53af7..039d3a18d5 100644 --- a/test/javascripts/models/composer-test.js.es6 +++ b/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");