From 3835e16cf73a5f14e452daf5b301f6d6458d52c2 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Wed, 19 Apr 2017 16:16:27 -0400 Subject: [PATCH] FIX: New implementation of the "notify about flag after" setting. Only notify about new flags since the last notification. Send a private message to staff. Mention the 3 most active moderators in the message so they get notification emails. --- app/jobs/scheduled/pending_flags_reminder.rb | 42 +++++++++++++++++-- config/locales/server.en.yml | 2 +- spec/jobs/pending_flags_reminder_spec.rb | 24 +++++++++-- .../pending_queued_posts_reminder_spec.rb | 4 +- 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/app/jobs/scheduled/pending_flags_reminder.rb b/app/jobs/scheduled/pending_flags_reminder.rb index 94fb480ef17..1b8080a049c 100644 --- a/app/jobs/scheduled/pending_flags_reminder.rb +++ b/app/jobs/scheduled/pending_flags_reminder.rb @@ -4,12 +4,15 @@ module Jobs class PendingFlagsReminder < Jobs::Scheduled - every 1.day + every 1.hour def execute(args) if SiteSetting.notify_about_flags_after > 0 && - PostAction.flagged_posts_count > 0 && - FlagQuery.flagged_post_actions('active').where('post_actions.created_at < ?', SiteSetting.notify_about_flags_after.to_i.hours.ago).pluck(:id).count > 0 + PostAction.flagged_posts_count > 0 && + flag_ids.size > 0 && last_notified_id.to_i < flag_ids.max + + mentions = active_moderator_usernames.size > 0 ? + "@#{active_moderator_usernames.join(', @')} " : "" PostCreator.create( Discourse.system_user, @@ -17,9 +20,40 @@ module Jobs archetype: Archetype.private_message, subtype: TopicSubtype.system_message, title: I18n.t('flags_reminder.subject_template', { count: PostAction.flagged_posts_count }), - raw: I18n.t('flags_reminder.flags_were_submitted', { count: SiteSetting.notify_about_flags_after }) + raw: mentions + I18n.t('flags_reminder.flags_were_submitted', { count: SiteSetting.notify_about_flags_after }) ) + + self.last_notified_id = flag_ids.max end + + true + end + + def flag_ids + @_flag_ids ||= FlagQuery.flagged_post_actions('active') + .where('post_actions.created_at < ?', SiteSetting.notify_about_flags_after.to_i.hours.ago) + .pluck(:id) + end + + def last_notified_id + (i = $redis.get(self.class.last_notified_key)) && i.to_i + end + + def last_notified_id=(arg) + $redis.set(self.class.last_notified_key, arg) + end + + def self.last_notified_key + "last_notified_pending_flag_id" + end + + def active_moderator_usernames + @_active_moderator_usernames ||= + User.where(moderator: true) + .human_users + .order('last_seen_at DESC') + .limit(3) + .pluck(:username) end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 940db10b9f9..40617b68e0f 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1446,7 +1446,7 @@ en: embed_username_required: "The username for topic creation is required." embed_whitelist_selector: "CSS selector for elements that are allowed in embeds." embed_blacklist_selector: "CSS selector for elements that are removed from embeds." - notify_about_flags_after: "If there are flags that haven't been handled after this many hours, send an email to the contact_email. Set to 0 to disable." + notify_about_flags_after: "If there are flags that haven't been handled after this many hours, send a private message to staff. Set to 0 to disable." show_create_topics_notice: "If the site has fewer than 5 public topics, show a notice asking admins to create some topics." delete_drafts_older_than_n_days: Delete drafts older than (n) days. diff --git a/spec/jobs/pending_flags_reminder_spec.rb b/spec/jobs/pending_flags_reminder_spec.rb index e73038c0fb2..25f6eb673b0 100644 --- a/spec/jobs/pending_flags_reminder_spec.rb +++ b/spec/jobs/pending_flags_reminder_spec.rb @@ -4,15 +4,22 @@ describe Jobs::PendingFlagsReminder do context "notify_about_flags_after is 0" do before { SiteSetting.stubs(:notify_about_flags_after).returns(0) } - it "never emails" do + it "never notifies" do PostAction.stubs(:flagged_posts_count).returns(1) - Email::Sender.any_instance.expects(:send).never + PostCreator.expects(:create).never described_class.new.execute({}) end end context "notify_about_flags_after is 48" do - before { SiteSetting.stubs(:notify_about_flags_after).returns(48) } + before do + SiteSetting.notify_about_flags_after = 48 + $redis.del described_class.last_notified_key + end + + after do + $redis.del described_class.last_notified_key + end it "doesn't send message when flags are less than 48 hours old" do Fabricate(:flag, created_at: 47.hours.ago) @@ -27,5 +34,16 @@ describe Jobs::PendingFlagsReminder do PostCreator.expects(:create).once.returns(true) described_class.new.execute({}) end + + it "doesn't send a message if there are no new flags older than 48 hours old" do + old_flag = Fabricate(:flag, created_at: 50.hours.ago) + new_flag = Fabricate(:flag, created_at: 47.hours.ago) + PostAction.stubs(:flagged_posts_count).returns(2) + job = described_class.new + job.last_notified_id = old_flag.id + PostCreator.expects(:create).never + job.execute({}) + expect(job.last_notified_id).to eq(old_flag.id) + end end end diff --git a/spec/jobs/pending_queued_posts_reminder_spec.rb b/spec/jobs/pending_queued_posts_reminder_spec.rb index dd182686da0..fe5a1c79563 100644 --- a/spec/jobs/pending_queued_posts_reminder_spec.rb +++ b/spec/jobs/pending_queued_posts_reminder_spec.rb @@ -12,7 +12,9 @@ describe Jobs::PendingQueuedPostReminder do end context "notify_about_queued_posts_after is 24" do - before { SiteSetting.stubs(:notify_about_queued_posts_after).returns(24) } + before do + SiteSetting.notify_about_queued_posts_after = 24 + end it "doesn't email if there are no queued posts" do described_class.any_instance.stubs(:should_notify_ids).returns([])