diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index 9eeb9f87b35..57c4893fe21 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 4c26f028cd4..c0aa984089e 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 018fba10251..9f630c477f1 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 53e9bbad790..0e40e9e81ef 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 bf43b41a36f..413ee9c2edd 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 3b24efff08f..0fa1f0b6fe9 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 c081fb5afda..250d1e6d70d 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 681dc96afcc..363fa65ca44 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 8e434f3bd32..d160ed2e518 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 431b7d7f659..ddda0334662 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 64990a3e2e6..9657a80997e 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 00000000000..3f853335809 --- /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 a58c2d16f53..4123b40da14 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 7866206bfee..f1ff985dc9e 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