From aee943486aa002250d1fdae3769f8fa3f87392f8 Mon Sep 17 00:00:00 2001 From: Kiril Staikov Date: Wed, 5 Oct 2016 19:28:58 -0400 Subject: [PATCH] FEATURE:'No Echo' option for mailing list mode. Mailing list mode now includes the 'no echo' option: to only receive emails of posts not created by you. If you reply to an email thread in mailing list mode, your reply will not then be echoed back to you in a duplicate email by the system. --- .../discourse/controllers/preferences.js.es6 | 3 +- .../notify_mailing_list_subscribers.rb | 7 ++++- app/jobs/regular/user_email.rb | 2 +- app/models/mailing_list_mode_site_setting.rb | 3 +- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + spec/jobs/enqueue_mailing_list_emails_spec.rb | 5 ++++ .../notify_mailing_list_subscribers_spec.rb | 30 ++++++++++++++++++- spec/jobs/user_email_spec.rb | 15 ++++++++++ 9 files changed, 62 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/preferences.js.es6 b/app/assets/javascripts/discourse/controllers/preferences.js.es6 index b94d993ed1..f9077e6e43 100644 --- a/app/assets/javascripts/discourse/controllers/preferences.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences.js.es6 @@ -79,7 +79,8 @@ export default Ember.Controller.extend(CanCheckEmails, { mailingListModeOptions() { return [ {name: I18n.t('user.mailing_list_mode.daily'), value: 0}, - {name: this.get('frequencyEstimate'), value: 1} + {name: this.get('frequencyEstimate'), value: 1}, + {name: I18n.t('user.mailing_list_mode.individual_no_echo'), value: 2} ]; }, diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 6eed13a420..ae5e333e7e 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -16,7 +16,7 @@ module Jobs users = User.activated.not_blocked.not_suspended.real .joins(:user_option) - .where(user_options: {mailing_list_mode: true, mailing_list_mode_frequency: 1}) + .where('user_options.mailing_list_mode AND user_options.mailing_list_mode_frequency > 0') .where('NOT EXISTS( SELECT 1 FROM topic_users tu @@ -46,6 +46,11 @@ module Jobs next end + if (user.id == post.user_id) && (user.user_option.mailing_list_mode_frequency == 2) + skip(user.email, user.id, post.id, I18n.t('email_log.no_echo_mailing_list_mode')) + next + end + begin if message = UserNotifications.mailing_list_notify(user, post) EmailLog.unique_email_per_post(post, user) do diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 50109e2a68..de1d318f4f 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -103,7 +103,7 @@ module Jobs end if user.user_option.mailing_list_mode? && - user.user_option.mailing_list_mode_frequency == 1 && # don't catch notifications for users on daily mailing list mode + user.user_option.mailing_list_mode_frequency > 0 && # don't catch notifications for users on daily mailing list mode (!post.try(:topic).try(:private_message?)) && NOTIFICATIONS_SENT_BY_MAILING_LIST.include?(email_args[:notification_type]) # no need to log a reason when the mail was already sent via the mailing list job diff --git a/app/models/mailing_list_mode_site_setting.rb b/app/models/mailing_list_mode_site_setting.rb index b2a8291d00..9a8241c1f0 100644 --- a/app/models/mailing_list_mode_site_setting.rb +++ b/app/models/mailing_list_mode_site_setting.rb @@ -9,7 +9,8 @@ class MailingListModeSiteSetting < EnumSiteSetting def self.values @values ||= [ { name: 'user.mailing_list_mode.daily', value: 0 }, - { name: 'user.mailing_list_mode.individual', value: 1 } + { name: 'user.mailing_list_mode.individual', value: 1 }, + { name: 'user.mailing_list_mode.individual_no_echo', value: 2 } ] end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index fe210e4bb3..df618cac2b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -535,6 +535,7 @@ en: Muted topics and categories are not included in these emails. daily: "Send daily updates" individual: "Send an email for every new post" + individual_no_echo: "Send an email for every new post except my own" many_per_day: "Send me an email for every new post (about {{dailyEmailEstimate}} per day)" few_per_day: "Send me an email for every new post (about 2 per day)" tag_settings: "Tags" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index fd7ebcc9b6..3ae8f8fb67 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2614,6 +2614,7 @@ en: message_to_blank: "message.to is blank" text_part_body_blank: "text_part.body is blank" body_blank: "body is blank" + no_echo_mailing_list_mode: "Mailing list notifications disabled for user's own posts" color_schemes: base_theme_name: "Base" diff --git a/spec/jobs/enqueue_mailing_list_emails_spec.rb b/spec/jobs/enqueue_mailing_list_emails_spec.rb index 820ed1e812..3ee9ebe941 100644 --- a/spec/jobs/enqueue_mailing_list_emails_spec.rb +++ b/spec/jobs/enqueue_mailing_list_emails_spec.rb @@ -82,6 +82,11 @@ describe Jobs::EnqueueMailingListEmails do expect(subject).to_not include user.id end + it "doesn't return users with mailing list mode set to 'individual_excluding_own'" do + user_option.update(mailing_list_mode_frequency: 2) + expect(subject).to_not include user.id + end + it "doesn't return a user who has received the mailing list summary earlier" do user.update(first_seen_at: 5.hours.ago) expect(subject).to_not include user.id diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index 899d4aedd9..92a97409b9 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -47,9 +47,15 @@ describe Jobs::NotifyMailingListSubscribers do end end - context "with a valid post" do + context "with a valid post authored by same user" do let!(:post) { Fabricate(:post, user: user) } + it "doesn't send the email to the user if the frequency is set to 'always with no echo'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 2) + UserNotifications.expects(:mailing_list_notify).never + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end + it "sends the email to the user if the frequency is set to 'always'" do user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 1) UserNotifications.expects(:mailing_list_notify).with(user, post).once @@ -63,6 +69,28 @@ describe Jobs::NotifyMailingListSubscribers do end end + context "with a valid post created by someone other than the user" do + let!(:post) { Fabricate(:post, user: user, user_id: 'different') } + + it "sends the email to the user if the frequency is set to 'always with no echo'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 2) + UserNotifications.expects(:mailing_list_notify).once + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end + + it "sends the email to the user if the frequency is set to 'always'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 1) + UserNotifications.expects(:mailing_list_notify).once + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end + + it "does not send the email to the user if the frequency is set to 'daily'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + UserNotifications.expects(:mailing_list_notify).never + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end + end + context "with a deleted post" do let!(:post) { Fabricate(:post, user: user, deleted_at: Time.now) } diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index f99a403b4e..a1af812c12 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -240,6 +240,21 @@ describe Jobs::UserEmail do Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted", post_id: post.id) end + it "doesn't send the mail if the user is using individual mailing list mode with no echo" do + Email::Sender.any_instance.expects(:send).never + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 2) + # sometimes, we pass the notification_id + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id) + # other times, we only pass the type of notification + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted", post_id: post.id) + # When post is nil + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted") + # When post does not have a topic + post = Fabricate(:post) + post.topic.destroy + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted", post_id: post.id) + end + it "doesn't send the email if the post has been user deleted" do Email::Sender.any_instance.expects(:send).never post.update_column(:user_deleted, true)