From f1e7bca3c92ea57f69e6ebb19f7fc75f188ab953 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Fri, 3 Feb 2017 16:59:05 -0500 Subject: [PATCH] FEATURE: Warn a user when they're replying to the same user too much --- app/models/user_history.rb | 3 +- config/locales/server.en.yml | 9 ++ config/site_settings.yml | 1 + lib/composer_messages_finder.rb | 74 ++++++++---- .../composer_messages_finder_spec.rb | 108 +++++++++++++++++- 5 files changed, 169 insertions(+), 26 deletions(-) diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 50b616391c3..ee667213c3c 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -61,7 +61,8 @@ class UserHistory < ActiveRecord::Base activate_user: 43, change_readonly_mode: 44, backup_download: 45, - backup_destroy: 46) + backup_destroy: 46, + notified_about_get_a_room: 47) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index b416e043a2c..7abca65861f 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -316,6 +316,13 @@ en: Are you sure you're providing adequate time for other people to share their points of view, too? + get_a_room: | + ### You're replying a lot to the same person + + You've replied %{count} times to that person in a row already. + + Perhaps you're making things too personal, rather than allowing for a community discussion? You could try sending them a message instead. + too_many_replies: | ### You have reached the reply limit for this topic @@ -1359,6 +1366,8 @@ en: sequential_replies_threshold: "Number of posts a user has to make in a row in a topic before being reminded about too many sequential replies." + get_a_room_threshold: "Number of posts a user has to make to the same person in the same topic before being warned." + enable_mobile_theme: "Mobile devices use a mobile-friendly theme, with the ability to switch to the full site. Disable this if you want to use a custom stylesheet that is fully responsive." dominating_topic_minimum_percent: "What percentage of posts a user has to make in a topic before being reminded about overly dominating a topic." diff --git a/config/site_settings.yml b/config/site_settings.yml index 2730226b073..ffc0d70a2a1 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1212,6 +1212,7 @@ uncategorized: # Warnings educate_until_posts: 2 sequential_replies_threshold: 2 + get_a_room_threshold: 4 dominating_topic_minimum_percent: 20 disable_avatar_education_message: false diff --git a/lib/composer_messages_finder.rb b/lib/composer_messages_finder.rb index 347ca208e80..ac2a03d3b25 100644 --- a/lib/composer_messages_finder.rb +++ b/lib/composer_messages_finder.rb @@ -6,13 +6,16 @@ class ComposerMessagesFinder @topic = Topic.find_by(id: details[:topic_id]) if details[:topic_id] end + def self.check_methods + @check_methods ||= instance_methods.find_all {|m| m =~ /^check\_/} + end + def find - check_reviving_old_topic || - check_education_message || - check_new_user_many_replies || - check_avatar_notification || - check_sequential_replies || - check_dominating_topic + self.class.check_methods.each do |m| + msg = send(m) + return msg if msg.present? + end + nil end # Determines whether to show the user education text @@ -79,18 +82,7 @@ class ComposerMessagesFinder # Is a user replying too much in succession? def check_sequential_replies - - # We only care about replies to topics - return unless replying? && @details[:topic_id] && - - # And who have posted enough - (@user.post_count >= SiteSetting.educate_until_posts) && - - # And it's not a message - (@topic.present? && !@topic.private_message?) && - - # And who haven't been notified about sequential replies already - !UserHistory.exists_for_user?(@user, :notified_about_sequential_replies, topic_id: @details[:topic_id]) + return unless educate_reply?(:notified_about_sequential_replies) # Count the topics made by this user in the last day recent_posts_user_ids = Post.where(topic_id: @details[:topic_id]) @@ -118,12 +110,7 @@ class ComposerMessagesFinder end def check_dominating_topic - - # We only care about replies to topics for a user who has posted enough - return unless replying? && - @details[:topic_id] && - (@user.post_count >= SiteSetting.educate_until_posts) && - !UserHistory.exists_for_user?(@user, :notified_about_dominating_topic, topic_id: @details[:topic_id]) + return unless educate_reply?(:notified_about_dominating_topic) return if @topic.blank? || @topic.user_id == @user.id || @@ -149,6 +136,37 @@ class ComposerMessagesFinder } end + def check_get_a_room + return unless educate_reply?(:notified_about_get_a_room) + return unless @details[:post_id].present? + + reply_to_user_id = Post.where(id: @details[:post_id]).pluck(:user_id)[0] + + # Users's last x posts in the topic + last_x_replies = @topic. + posts. + where(user_id: @user.id). + limit(SiteSetting.get_a_room_threshold). + pluck(:reply_to_user_id). + find_all {|uid| uid != @user.id && uid == reply_to_user_id} + + return unless last_x_replies.size == SiteSetting.get_a_room_threshold + + UserHistory.create!(action: UserHistory.actions[:notified_about_get_a_room], + target_user_id: @user.id, + topic_id: @details[:topic_id]) + + { + id: 'get_a_room', + templateName: 'education', + wait_for_typing: true, + extraClass: 'education-message', + body: PrettyText.cook( + I18n.t('education.get_a_room', count: SiteSetting.get_a_room_threshold) + ) + } + end + def check_reviving_old_topic return unless replying? return if @topic.nil? || @@ -167,6 +185,14 @@ class ComposerMessagesFinder private + def educate_reply?(type) + replying? && + @details[:topic_id] && + (@topic.present? && !@topic.private_message?) && + (@user.post_count >= SiteSetting.educate_until_posts) && + !UserHistory.exists_for_user?(@user, type, topic_id: @details[:topic_id]) + end + def creating_topic? @details[:composer_action] == "createTopic" end diff --git a/spec/components/composer_messages_finder_spec.rb b/spec/components/composer_messages_finder_spec.rb index 8bc709e58ad..7e80599ac20 100644 --- a/spec/components/composer_messages_finder_spec.rb +++ b/spec/components/composer_messages_finder_spec.rb @@ -15,6 +15,7 @@ describe ComposerMessagesFinder do finder.expects(:check_sequential_replies).once finder.expects(:check_dominating_topic).once finder.expects(:check_reviving_old_topic).once + finder.expects(:check_get_a_room).once finder.find end @@ -197,7 +198,7 @@ describe ComposerMessagesFinder do expect(finder.check_sequential_replies).to be_blank end - it "doesn't notify in message" do + it "doesn't notify in a message" do Topic.any_instance.expects(:private_message?).returns(true) expect(finder.check_sequential_replies).to be_blank end @@ -303,6 +304,111 @@ describe ComposerMessagesFinder do end + context '.check_get_a_room' do + let(:user) { Fabricate(:user) } + let(:other_user) { Fabricate(:user) } + let(:third_user) { Fabricate(:user) } + let(:topic) { Fabricate(:topic, user: other_user) } + let(:op) { Fabricate(:post, topic_id: topic.id, user: other_user) } + + let!(:other_user_reply) { + Fabricate(:post, topic: topic, user: third_user, reply_to_user_id: op.user_id) + } + + let!(:first_reply) { + Fabricate(:post, topic: topic, user: user, reply_to_user_id: op.user_id) + } + + let!(:second_reply) { + Fabricate(:post, topic: topic, user: user, reply_to_user_id: op.user_id) + } + + before do + SiteSetting.educate_until_posts = 10 + user.stubs(:post_count).returns(11) + SiteSetting.get_a_room_threshold = 2 + end + + it "does not show the message for new topics" do + finder = ComposerMessagesFinder.new(user, composer_action: 'createTopic') + expect(finder.check_get_a_room).to be_blank + end + + it "does not give a message without a topic id" do + expect(ComposerMessagesFinder.new(user, composer_action: 'reply').check_get_a_room).to be_blank + end + + context "reply" do + let(:finder) { ComposerMessagesFinder.new(user, composer_action: 'reply', topic_id: topic.id, post_id: op.id) } + + it "does not give a message to users who are still in the 'education' phase" do + user.stubs(:post_count).returns(9) + expect(finder.check_get_a_room).to be_blank + end + + it "doesn't notify a user it has already notified about sequential replies" do + UserHistory.create!( + action: UserHistory.actions[:notified_about_get_a_room], + target_user_id: user.id, + topic_id: topic.id + ) + expect(finder.check_get_a_room).to be_blank + end + + it "will notify you if it hasn't in the current topic" do + UserHistory.create!( + action: UserHistory.actions[:notified_about_get_a_room], + target_user_id: user.id, + topic_id: topic.id+1 + ) + expect(finder.check_get_a_room).to be_present + end + + it "won't notify you if you haven't had enough posts" do + SiteSetting.get_a_room_threshold = 10 + expect(finder.check_get_a_room).to be_blank + end + + it "doesn't notify you if the posts aren't all to the same person" do + first_reply.update_column(:reply_to_user_id, user.id) + expect(finder.check_get_a_room).to be_blank + end + + it "doesn't notify you of posts to yourself" do + first_reply.update_column(:reply_to_user_id, user.id) + second_reply.update_column(:reply_to_user_id, user.id) + expect(finder.check_get_a_room).to be_blank + end + + it "doesn't notify in a message" do + Topic.any_instance.expects(:private_message?).returns(true) + expect(finder.check_get_a_room).to be_blank + end + + it "doesn't notify when replying to a different user" do + other_finder = ComposerMessagesFinder.new( + user, + composer_action: 'reply', + topic_id: topic.id, + post_id: other_user_reply.id + ) + + expect(other_finder.check_get_a_room).to be_blank + end + + context "success" do + let!(:message) { finder.check_get_a_room } + + it "works as expected" do + expect(message).to be_present + expect(UserHistory.exists_for_user?(user, :notified_about_get_a_room)).to eq(true) + end + end + end + + end + + context '.check_reviving_old_topic' do let(:user) { Fabricate(:user) } let(:topic) { Fabricate(:topic) }