From 820a435af83fd0fffe0429541e657a3756f13bc0 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Fri, 26 Feb 2016 00:05:40 +1100 Subject: [PATCH] FEATURE: add "email in-reply-to user option" We no longer include previous replies as "context", instead we include and excerpt of the post being replied to at the bottom of notifications, this information was previously missing. Users may opt in to emailing previous replies if they wish or opt out of "in-reply-to" which makes sense in some email clients that are smarter about displaying a tree of replies. --- .../javascripts/discourse/models/user.js.es6 | 1 + .../discourse/templates/user/preferences.hbs | 1 + app/mailers/user_notifications.rb | 12 +++-- app/models/user_option.rb | 1 + app/serializers/user_option_serializer.rb | 3 +- app/services/user_updater.rb | 9 ++-- app/views/email/_post.html.erb | 4 +- app/views/email/notification.html.erb | 19 ++++--- config/locales/client.en.yml | 3 +- config/locales/server.en.yml | 3 ++ config/site_settings.yml | 4 +- ...6_add_email_in_reply_to_to_user_options.rb | 11 ++++ spec/mailers/user_notifications_spec.rb | 53 ++++++++++++++----- spec/services/user_updater_spec.rb | 4 +- 14 files changed, 92 insertions(+), 36 deletions(-) create mode 100644 db/migrate/20160225095306_add_email_in_reply_to_to_user_options.rb diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index 9eeb9f87b..57c4893fe 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -158,6 +158,7 @@ const User = RestModel.extend({ 'external_links_in_new_tab', 'email_digests', 'email_direct', + 'email_in_reply_to', 'email_private_messages', 'email_previous_replies', 'dynamic_favicon', diff --git a/app/assets/javascripts/discourse/templates/user/preferences.hbs b/app/assets/javascripts/discourse/templates/user/preferences.hbs index 4c26f028c..c0aa98408 100644 --- a/app/assets/javascripts/discourse/templates/user/preferences.hbs +++ b/app/assets/javascripts/discourse/templates/user/preferences.hbs @@ -180,6 +180,7 @@ {{combo-box valueAttribute="value" content=previousRepliesOptions value=model.user_option.email_previous_replies}} + {{preference-checkbox labelKey="user.email_in_reply_to" checked=model.user_option.email_in_reply_to}} {{preference-checkbox labelKey="user.email_private_messages" checked=model.user_option.email_private_messages}} {{preference-checkbox labelKey="user.email_direct" checked=model.user_option.email_direct}} {{preference-checkbox labelKey="user.mailing_list_mode" checked=model.user_option.mailing_list_mode}} diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 018fba102..9f630c477 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -193,9 +193,9 @@ class UserNotifications < ActionMailer::Base include UserNotificationsHelper end - def self.get_context_posts(post, topic_user) - user_option = topic_user.try(:user).try(:user_option) - if user_option && (user_option.email_previous_replies == UserOption.previous_replies_type[:never]) + def self.get_context_posts(post, topic_user, user) + + if user.user_option.email_previous_replies == UserOption.previous_replies_type[:never] return [] end @@ -210,7 +210,7 @@ class UserNotifications < ActionMailer::Base .order('created_at desc') .limit(SiteSetting.email_posts_context) - if topic_user && topic_user.last_emailed_post_number && user_option.try(:email_previous_replies) == UserOption.previous_replies_type[:unless_emailed] + if topic_user && topic_user.last_emailed_post_number && user.user_option.email_previous_replies == UserOption.previous_replies_type[:unless_emailed] context_posts = context_posts.where("post_number > ?", topic_user.last_emailed_post_number) end @@ -280,7 +280,7 @@ class UserNotifications < ActionMailer::Base context = "" tu = TopicUser.get(post.topic_id, user) - context_posts = self.class.get_context_posts(post, tu) + context_posts = self.class.get_context_posts(post, tu, user) # make .present? cheaper context_posts = context_posts.to_a @@ -296,11 +296,13 @@ class UserNotifications < ActionMailer::Base if opts[:use_template_html] topic_excerpt = post.excerpt.gsub("\n", " ") if post.is_first_post? && post.excerpt else + in_reply_to_post = post.reply_to_post if user.user_option.email_in_reply_to html = UserNotificationRenderer.new(Rails.configuration.paths["app/views"]).render( template: 'email/notification', format: :html, locals: { context_posts: context_posts, post: post, + in_reply_to_post: in_reply_to_post, classes: RTL.new(user).css_class } ) diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 53e9bbad7..0e40e9e81 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -16,6 +16,7 @@ class UserOption < ActiveRecord::Base self.automatically_unpin_topics = SiteSetting.default_topics_automatic_unpin self.email_private_messages = SiteSetting.default_email_private_messages self.email_previous_replies = SiteSetting.default_email_previous_replies + self.email_in_reply_to = SiteSetting.default_email_in_reply_to self.enable_quoting = SiteSetting.default_other_enable_quoting self.external_links_in_new_tab = SiteSetting.default_other_external_links_in_new_tab diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index bf43b41a3..413ee9c2e 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -14,7 +14,8 @@ class UserOptionSerializer < ApplicationSerializer :edit_history_public, :auto_track_topics_after_msecs, :new_topic_duration_minutes, - :email_previous_replies + :email_previous_replies, + :email_in_reply_to def include_edit_history_public? diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 3b24efff0..0fa1f0b6f 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -21,7 +21,8 @@ class UserUpdater :digest_after_days, :new_topic_duration_minutes, :auto_track_topics_after_msecs, - :email_previous_replies + :email_previous_replies, + :email_in_reply_to ] def initialize(actor, user) @@ -53,10 +54,10 @@ class UserUpdater save_options = false - OPTION_ATTR.each do |attribute| - if attributes[attribute].present? - save_options = true + OPTION_ATTR.each do |attribute| + if attributes.key?(attribute) + save_options = true if [true,false].include?(user.user_option.send(attribute)) val = attributes[attribute].to_s == 'true' diff --git a/app/views/email/_post.html.erb b/app/views/email/_post.html.erb index c081fb5af..250d1e6d7 100644 --- a/app/views/email/_post.html.erb +++ b/app/views/email/_post.html.erb @@ -1,4 +1,4 @@ - +
- +
@@ -23,7 +23,7 @@
<%= format_for_email(post.cooked) %><%= format_for_email(use_excerpt ? post.excerpt : post.cooked) %>
diff --git a/app/views/email/notification.html.erb b/app/views/email/notification.html.erb index 681dc96af..363fa65ca 100644 --- a/app/views/email/notification.html.erb +++ b/app/views/email/notification.html.erb @@ -2,17 +2,22 @@
%{header_instructions}
- <%= render partial: 'email/post', locals: { post: post } %> + <%= render partial: 'email/post', locals: { post: post, use_excerpt: false } %> + + <% if in_reply_to_post.present? || context_posts.present? %> +
%{respond_instructions}
+
+ <% end %> + + <% if in_reply_to_post.present? %> +

<%= t "user_notifications.in_reply_to" %>

+ <%= render partial: 'email/post', locals: { post: in_reply_to_post, use_excerpt: true} %> + <% end %> <% if context_posts.present? %> -
%{respond_instructions}
- -
-

<%= t "user_notifications.previous_discussion" %>

- <% context_posts.each do |p| %> - <%= render partial: 'email/post', locals: { post: p } %> + <%= render partial: 'email/post', locals: { post: p, use_excerpt: false } %> <% end %> <% end %> diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 8e434f3bd..d160ed2e5 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -628,7 +628,7 @@ en: website: "Web Site" email_settings: "Email" email_previous_replies: - title: "Include previous replies" + title: "Include previous replies at the bottom of emails" unless_emailed: "unless previously sent" always: "always" never: "never" @@ -639,6 +639,7 @@ en: weekly: "weekly" every_two_weeks: "every two weeks" + email_in_reply_to: "Include an excerpt of replied to post in emails" email_direct: "Send me an email when someone quotes me, replies to my post, mentions my @username, or invites me to a topic" email_private_messages: "Send me an email when someone messages me" email_always: "Send me email notifications even when I am active on the site" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 431b7d7f6..ddda03346 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1243,6 +1243,8 @@ en: default_email_always: "Send an email notification even when the user is active by default." default_email_previous_replies: "Include previous replies in emails by default." + default_email_in_reply_to: "Include excerpt of replied to post in emails by default." + default_other_new_topic_duration_minutes: "Global default condition for which a topic is considered new." default_other_auto_track_topics_after_msecs: "Global default time before a topic is automatically tracked." default_other_external_links_in_new_tab: "Open external links in a new tab by default." @@ -1995,6 +1997,7 @@ en: user_notifications: previous_discussion: "Previous Replies" + in_reply_to: "In Reply To" unsubscribe: title: "Unsubscribe" description: "Not interested in getting these emails? No problem! Click below to unsubscribe instantly:" diff --git a/config/site_settings.yml b/config/site_settings.yml index 64990a3e2..9657a8099 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1063,7 +1063,9 @@ user_preferences: default_email_always: false default_email_previous_replies: enum: 'PreviousRepliesSiteSetting' - default: 1 + default: 2 + default_email_in_reply_to: + default: true default_other_new_topic_duration_minutes: enum: 'NewTopicDurationSiteSetting' diff --git a/db/migrate/20160225095306_add_email_in_reply_to_to_user_options.rb b/db/migrate/20160225095306_add_email_in_reply_to_to_user_options.rb new file mode 100644 index 000000000..3f8533358 --- /dev/null +++ b/db/migrate/20160225095306_add_email_in_reply_to_to_user_options.rb @@ -0,0 +1,11 @@ +class AddEmailInReplyToToUserOptions < ActiveRecord::Migration + def up + add_column :user_options, :email_in_reply_to, :boolean, null: false, default: true + change_column :user_options, :email_previous_replies, :integer, default: 2, null: false + execute 'UPDATE user_options SET email_previous_replies = 2' + end + + def down + remove_column :user_options, :email_in_reply_to + end +end diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index a58c2d16f..4123b40da 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -15,11 +15,16 @@ describe UserNotifications do _post7 = Fabricate(:post, topic: post1.topic, post_type: Post.types[:whisper]) last = Fabricate(:post, topic: post1.topic) + post1.user.user_option.email_previous_replies = UserOption.previous_replies_type[:always] + # default is only post #1 - expect(UserNotifications.get_context_posts(last, nil).count).to eq(1) + expect(UserNotifications.get_context_posts(last, nil, post1.user).count).to eq(1) # staff members can also see the whisper - tu = TopicUser.new(topic: post1.topic, user: build(:moderator)) - expect(UserNotifications.get_context_posts(last, tu).count).to eq(2) + moderator = build(:moderator) + moderator.user_option = UserOption.new + moderator.user_option.email_previous_replies = UserOption.previous_replies_type[:always] + tu = TopicUser.new(topic: post1.topic, user: moderator) + expect(UserNotifications.get_context_posts(last, tu, tu.user).count).to eq(2) end it "allows users to control context" do @@ -32,13 +37,15 @@ describe UserNotifications do topic_user = TopicUser.find_by(user_id: user.id, topic_id: post1.topic_id) # to avoid reloads after update_columns user = topic_user.user - expect(UserNotifications.get_context_posts(post3, topic_user).count).to eq(1) + user.user_option.update_columns(email_previous_replies: UserOption.previous_replies_type[:unless_emailed]) + + expect(UserNotifications.get_context_posts(post3, topic_user, user).count).to eq(1) user.user_option.update_columns(email_previous_replies: UserOption.previous_replies_type[:never]) - expect(UserNotifications.get_context_posts(post3, topic_user).count).to eq(0) + expect(UserNotifications.get_context_posts(post3, topic_user, user).count).to eq(0) user.user_option.update_columns(email_previous_replies: UserOption.previous_replies_type[:always]) - expect(UserNotifications.get_context_posts(post3, topic_user).count).to eq(2) + expect(UserNotifications.get_context_posts(post3, topic_user, user).count).to eq(2) end end @@ -110,12 +117,14 @@ describe UserNotifications do let(:response_by_user) { Fabricate(:user, name: "John Doe") } let(:category) { Fabricate(:category, name: 'India') } let(:topic) { Fabricate(:topic, category: category) } - let(:post) { Fabricate(:post, topic: topic) } - let(:response) { Fabricate(:post, topic: post.topic, user: response_by_user)} + let(:post) { Fabricate(:post, topic: topic, raw: 'This is My super duper cool topic') } + let(:response) { Fabricate(:post, reply_to_post_number: 1, topic: post.topic, user: response_by_user)} let(:user) { Fabricate(:user) } let(:notification) { Fabricate(:notification, user: user) } it 'generates a correct email' do + + # Fabricator is not fabricating this ... SiteSetting.enable_names = true SiteSetting.display_name_on_posts = true mail = UserNotifications.user_replied(response.user, @@ -123,25 +132,41 @@ describe UserNotifications do notification_type: notification.notification_type, notification_data_hash: notification.data_hash ) - # from should include full user name expect(mail[:from].display_names).to eql(['John Doe']) # subject should include category name expect(mail.subject).to match(/India/) + mail_html = mail.html_part.to_s + + expect(mail_html.scan(/My super duper cool topic/).count).to eq(1) + expect(mail_html.scan(/In Reply To/).count).to eq(1) + # 2 "visit topic" link - expect(mail.html_part.to_s.scan(/Visit Topic/).count).to eq(2) + expect(mail_html.scan(/Visit Topic/).count).to eq(2) # 2 respond to links cause we have 1 context post - expect(mail.html_part.to_s.scan(/to respond/).count).to eq(2) + expect(mail_html.scan(/to respond/).count).to eq(2) # 1 unsubscribe - expect(mail.html_part.to_s.scan(/To unsubscribe/).count).to eq(1) + expect(mail_html.scan(/To unsubscribe/).count).to eq(1) # side effect, topic user is updated with post number tu = TopicUser.get(post.topic_id, response.user) expect(tu.last_emailed_post_number).to eq(response.post_number) + + + # no In Reply To if user opts out + response.user.user_option.email_in_reply_to = false + mail = UserNotifications.user_replied(response.user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) + + + expect(mail.html_part.to_s.scan(/In Reply To/).count).to eq(0) end end @@ -169,8 +194,8 @@ describe UserNotifications do # subject should not include category name expect(mail.subject).not_to match(/Uncategorized/) - # 2 respond to links cause we have 1 context post - expect(mail.html_part.to_s.scan(/to respond/).count).to eq(2) + # 1 respond to links as no context by default + expect(mail.html_part.to_s.scan(/to respond/).count).to eq(1) # 1 unsubscribe link expect(mail.html_part.to_s.scan(/To unsubscribe/).count).to eq(1) diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index 7866206bf..f1ff985dc 100644 --- a/spec/services/user_updater_spec.rb +++ b/spec/services/user_updater_spec.rb @@ -48,7 +48,8 @@ describe UserUpdater do mailing_list_mode: true, digest_after_days: "8", new_topic_duration_minutes: 100, - auto_track_topics_after_msecs: 101 + auto_track_topics_after_msecs: 101, + email_in_reply_to: false ) user.reload @@ -58,6 +59,7 @@ describe UserUpdater do expect(user.user_option.digest_after_days).to eq 8 expect(user.user_option.new_topic_duration_minutes).to eq 100 expect(user.user_option.auto_track_topics_after_msecs).to eq 101 + expect(user.user_option.email_in_reply_to).to eq false end context 'when update succeeds' do