diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 8248c7543bc..a647d466f79 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -523,6 +523,12 @@ class TopicsController < ApplicationController topic = Topic.find_by(id: params[:topic_id]) + unless pm_has_slots?(topic) + return render_json_error(I18n.t("pm_reached_recipients_limit", + recipients_limit: SiteSetting.max_allowed_message_recipients + )) + end + if topic.private_message? guardian.ensure_can_invite_group_to_private_message!(group, topic) topic.invite_group(current_user, group) @@ -543,6 +549,12 @@ class TopicsController < ApplicationController group_names: params[:group_names] ) + unless pm_has_slots?(topic) + return render_json_error(I18n.t("pm_reached_recipients_limit", + recipients_limit: SiteSetting.max_allowed_message_recipients + )) + end + guardian.ensure_can_invite_to!(topic, groups) group_ids = groups.map(&:id) @@ -880,4 +892,7 @@ class TopicsController < ApplicationController params[:email] end + def pm_has_slots?(pm) + guardian.is_staff? || !pm.reached_recipients_limit? + end end diff --git a/app/models/topic.rb b/app/models/topic.rb index 7a7c2ae0af7..c0bbb021630 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -801,6 +801,11 @@ class Topic < ActiveRecord::Base false end + def reached_recipients_limit? + return false unless private_message? + topic_allowed_users.count + topic_allowed_groups.count >= SiteSetting.max_allowed_message_recipients + end + def invite_group(user, group) TopicAllowedGroup.create!(topic_id: id, group_id: group.id) diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 9cbf1b6383b..8122286406b 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -40,7 +40,7 @@ class TopicUser < ActiveRecord::Base def auto_notification(user_id, topic_id, reason, notification_level) should_change = TopicUser .where(user_id: user_id, topic_id: topic_id) - .where("notifications_reason_id IS NULL OR (notification_level < :min AND notification_level > :max)", min: notification_level, max: notification_levels[:regular]) + .where("notifications_reason_id IS NULL OR (notification_level < :max AND notification_level > :min)", max: notification_level, min: notification_levels[:regular]) .exists? change(user_id, topic_id, notification_level: notification_level, notifications_reason_id: reason) if should_change @@ -243,7 +243,8 @@ class TopicUser < ActiveRecord::Base notification_level = case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) > coalesce(uo.auto_track_topics_after_msecs,:threshold) and - coalesce(uo.auto_track_topics_after_msecs, :threshold) >= 0 then + coalesce(uo.auto_track_topics_after_msecs, :threshold) >= 0 + and t.archetype = 'regular' then :tracking else tu.notification_level diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bf769a1e33d..109d6fc5767 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -263,6 +263,7 @@ en: topic_not_found: "Something has gone wrong. Perhaps this topic was closed or deleted while you were looking at it?" not_accepting_pms: "Sorry, %{username} is not accepting messages at the moment." max_pm_recepients: "Sorry, you can send a message to maximum %{recipients_limit} recipients." + pm_reached_recipients_limit: "Sorry, you can't have more than %{recipients_limit} recipients in a message." just_posted_that: "is too similar to what you recently posted" invalid_characters: "contains invalid characters" diff --git a/lib/guardian.rb b/lib/guardian.rb index 30bf87acd4d..de87f2e9eae 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -272,6 +272,7 @@ class Guardian if object.is_a?(Topic) && object.private_message? return false unless SiteSetting.enable_personal_messages? + return false if object.reached_recipients_limit? && !is_staff? end if object.is_a?(Topic) && object.category diff --git a/lib/post_creator.rb b/lib/post_creator.rb index ddeda46d55a..9d870736ddd 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -531,6 +531,7 @@ class PostCreator if @user.staged TopicUser.auto_notification_for_staging(@user.id, @topic.id, TopicUser.notification_reasons[:auto_watch]) else + return if @topic.private_message? notification_level = @user.user_option.notification_level_when_replying || NotificationLevels.topic_levels[:tracking] TopicUser.auto_notification(@user.id, @topic.id, TopicUser.notification_reasons[:created_post], notification_level) end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 03c4d2c14d6..a99111f77c9 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -568,6 +568,7 @@ describe Guardian do let(:user) { Fabricate(:user, trust_level: TrustLevel[2]) } let!(:pm) { Fabricate(:private_message_topic, user: user) } let(:admin) { Fabricate(:admin) } + let(:moderator) { Fabricate(:moderator) } context "when private messages are disabled" do it "allows an admin to invite to the pm" do @@ -586,6 +587,22 @@ describe Guardian do expect(Guardian.new(user).can_invite_to?(pm)).to be_falsey end end + + context "when PM has receached the maximum number of recipients" do + before do + SiteSetting.max_allowed_message_recipients = 2 + end + + it "doesn't allow a regular user to invite" do + expect(Guardian.new(user).can_invite_to?(pm)).to be_falsey + end + + it "allows staff to invite" do + expect(Guardian.new(admin).can_invite_to?(pm)).to be_truthy + pm.grant_permission_to_user(moderator.email) + expect(Guardian.new(moderator).can_invite_to?(pm)).to be_truthy + end + end end end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 38ddc6ed74d..7499b6a276b 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -1054,6 +1054,22 @@ describe PostCreator do topic_user = TopicUser.find_by(user_id: user.id, topic_id: post.topic_id) expect(topic_user.notification_level).to eq(TopicUser.notification_levels[:regular]) end + + it "user preferences for notification level when replying doesn't affect PMs" do + user.user_option.update!(notification_level_when_replying: 1) + + admin = Fabricate(:admin) + pm = Fabricate(:private_message_topic, user: admin) + + pm.invite(admin, user.username) + PostCreator.create( + user, + topic_id: pm.id, + raw: "this is a test reply 123 123 ;)" + ) + topic_user = TopicUser.find_by(user_id: user.id, topic_id: pm.id) + expect(topic_user.notification_level).to eq(3) + end end describe '#create!' do diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb index ed9de27c3ad..aa333172cfe 100644 --- a/spec/models/topic_user_spec.rb +++ b/spec/models/topic_user_spec.rb @@ -372,6 +372,18 @@ describe TopicUser do TopicUser.update_last_read(new_user, topic, 2, 2, SiteSetting.default_other_auto_track_topics_after_msecs + 1) expect(topic_new_user.notification_level).to eq(TopicUser.notification_levels[:regular]) end + + it 'should not automatically track PMs' do + new_user.user_option.update!(auto_track_topics_after_msecs: 0) + + another_user = Fabricate(:user) + pm = Fabricate(:private_message_topic, user: another_user) + pm.invite(another_user, new_user.username) + + TopicUser.track_visit!(pm.id, new_user.id) + TopicUser.update_last_read(new_user, pm.id, 2, 2, 1000) + expect(TopicUser.get(pm, new_user).notification_level).to eq(TopicUser.notification_levels[:watching]) + end end end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index ece66eef134..ea6638697b6 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2026,6 +2026,37 @@ RSpec.describe TopicsController do expect(response.status).to eq(400) end + describe "when PM has reached maximum allowed numbers of recipients" do + let(:user2) { Fabricate(:user) } + let(:pm) { Fabricate(:private_message_topic, user: user) } + + let(:moderator) { Fabricate(:moderator) } + let(:moderator_pm) { Fabricate(:private_message_topic, user: moderator) } + + before do + SiteSetting.max_allowed_message_recipients = 2 + end + + it "doesn't allow normal users to invite" do + post "/t/#{pm.id}/invite.json", params: { + user: user2.username + } + expect(response.status).to eq(422) + expect(JSON.parse(response.body)["errors"]).to contain_exactly( + I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) + ) + end + + it "allows staff to bypass limits" do + sign_in(moderator) + post "/t/#{moderator_pm.id}/invite.json", params: { + user: user2.username + } + expect(response.status).to eq(200) + expect(moderator_pm.reload.topic_allowed_users.count).to eq(3) + end + end + describe 'when user does not have permission to invite to the topic' do let(:topic) { Fabricate(:private_message_topic) } @@ -2115,6 +2146,37 @@ RSpec.describe TopicsController do expect(pm.allowed_groups.first.id).to eq(admins.id) end end + + context "when PM has reached maximum allowed numbers of recipients" do + let(:group) { Fabricate(:group, messageable_level: 99) } + let(:pm) { Fabricate(:private_message_topic, user: user) } + + let(:moderator) { Fabricate(:moderator) } + let(:moderator_pm) { Fabricate(:private_message_topic, user: moderator) } + + before do + SiteSetting.max_allowed_message_recipients = 2 + end + + it "doesn't allow normal users to invite" do + post "/t/#{pm.id}/invite-group.json", params: { + group: group.name + } + expect(response.status).to eq(422) + expect(JSON.parse(response.body)["errors"]).to contain_exactly( + I18n.t("pm_reached_recipients_limit", recipients_limit: SiteSetting.max_allowed_message_recipients) + ) + end + + it "allows staff to bypass limits" do + sign_in(moderator) + post "/t/#{moderator_pm.id}/invite-group.json", params: { + group: group.name + } + expect(response.status).to eq(200) + expect(moderator_pm.reload.topic_allowed_users.count + moderator_pm.topic_allowed_groups.count).to eq(3) + end + end end describe 'shared drafts' do