|
+
+ <% if logo_url.blank? %>
+ <%= SiteSetting.title %>
+ <% else %>
+ +
+ <%= raw(t 'user_notifications.mailing_list.why', site_link: html_site_link(@anchor_color), date: @since_formatted) %>
+
+ + + |
+
+
+ <%= t('user_notifications.mailing_list.new_topics') %>
+
|
+
+ + <%= raw format_topic_title(topic.title) %> ++ |
+
|
+ <% @posts_by_topic[topic].each do |post| %>
+
+
+ <% end %>
+ <%= t('user_notifications.mailing_list.view_this_topic') %>
+ + '><%= post.user.name || post.user.username %> + - + <%= I18n.l(post.created_at, format: :long) %> + + <%= raw format_for_email(post, false, style: :notification) %> ++ |
+
| + <%= t('user_notifications.mailing_list.back_to_top') %> + | +
+ Muted topics and categories are not included in these emails. + daily: "Send daily updates." + individual: "Send an email for every new post." + 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 (less than 2 per day)." watched_categories: "Watched" watched_categories_instructions: "You will automatically watch all new topics in these categories. You will be notified of all new posts and topics, and a count of new posts will also appear next to the topic." tracked_categories: "Tracked" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0c73c7d8570..663ea6a9b46 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -28,6 +28,7 @@ en: short_no_year: "%B %-d" # Format directives: http://ruby-doc.org/core-2.2.0/Time.html#method-i-strftime date_only: "%B %-d, %Y" + long: "%B %-d, %Y, %l:%M%P" date: # Do not remove the brackets and commas and do not translate the first month name. It should be "null". month_names: @@ -35,6 +36,8 @@ en: <<: *datetime_formats time: <<: *datetime_formats + am: "am" + pm: "pm" title: "Discourse" topics: "Topics" @@ -1300,6 +1303,7 @@ en: default_email_private_messages: "Send an email when someone messages the user by default." default_email_direct: "Send an email when someone quotes/replies to/mentions or invites the user by default." default_email_mailing_list_mode: "Send an email for every new post by default." + default_email_mailing_list_mode_frequency: "Users who enable mailing list mode will receive emails this often by default." disable_mailing_list_mode: "Disallow users from enabling mailing list mode." 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." @@ -2278,6 +2282,15 @@ en: more_topics: "There were %{new_topics_since_seen} other new topics." more_topics_category: "More new topics:" + mailing_list: + why: "All activity on %{site_link} for %{date}" + subject_template: "[%{site_name}] Summary for %{date}" + unsubscribe: "This summary is sent daily due to mailing list mode being enabled. To unsubscribe %{unsubscribe_link}." + from: "%{site_name} summary" + new_topics: "New topics" + topic_updates: "Topic updates" + view_this_topic: "View this topic" + back_to_top: "Back to top" forgot_password: subject_template: "[%{site_name}] Password reset" text_body_template: | diff --git a/config/site_settings.yml b/config/site_settings.yml index fc59c058972..3f993bb21da 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1128,6 +1128,9 @@ user_preferences: default_email_private_messages: true default_email_direct: true default_email_mailing_list_mode: false + default_email_mailing_list_mode_frequency: + enum: 'MailingListModeSiteSetting' + default: 1 disable_mailing_list_mode: default: false client: true diff --git a/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb b/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb new file mode 100644 index 00000000000..dcdce07c402 --- /dev/null +++ b/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb @@ -0,0 +1,5 @@ +class AddMailingListModeFrequency < ActiveRecord::Migration + def change + add_column :user_options, :mailing_list_mode_frequency, :integer, default: 0, null: false + end +end diff --git a/db/migrate/20160326001747_add_user_first_visit.rb b/db/migrate/20160326001747_add_user_first_visit.rb new file mode 100644 index 00000000000..50e000ab00c --- /dev/null +++ b/db/migrate/20160326001747_add_user_first_visit.rb @@ -0,0 +1,5 @@ +class AddUserFirstVisit < ActiveRecord::Migration + def change + add_column :users, :first_seen_at, :datetime + end +end diff --git a/lib/email/styles.rb b/lib/email/styles.rb index a82c8459f30..25076ab72f0 100644 --- a/lib/email/styles.rb +++ b/lib/email/styles.rb @@ -90,6 +90,7 @@ module Email style('.rtl', 'direction: rtl;') style('td.body', 'padding-top:5px;', colspan: "2") style('.whisper td.body', 'font-style: italic; color: #9c9c9c;') + style('.lightbox-wrapper .meta', 'display: none') correct_first_body_margin correct_footer_style reset_tables @@ -186,6 +187,17 @@ module Email @fragment.to_s end + def make_all_links_absolute + site_uri = URI(Discourse.base_url) + @fragment.css("a").each do |link| + begin + link["href"] = "#{site_uri}#{link['href']}" unless URI(link["href"].to_s).host.present? + rescue URI::InvalidURIError, URI::InvalidComponentError + # leave it + end + end + end + private def replace_relative_urls diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index bfdcbbcd92c..5ff3c054f10 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -386,31 +386,16 @@ module PrettyText fragment.to_html end - # Given a Nokogiri doc, convert all links to absolute - def self.make_all_links_absolute(doc) - site_uri = nil - doc.css("a").each do |link| - href = link["href"].to_s - begin - uri = URI(href) - site_uri ||= URI(Discourse.base_url) - link["href"] = "#{site_uri}#{link['href']}" unless uri.host.present? - rescue URI::InvalidURIError, URI::InvalidComponentError - # leave it - end - end - end - def self.strip_image_wrapping(doc) doc.css(".lightbox-wrapper .meta").remove end - def self.format_for_email(html, post = nil) - doc = Nokogiri::HTML.fragment(html) - DiscourseEvent.trigger(:reduce_cooked, doc, post) - make_all_links_absolute(doc) - strip_image_wrapping(doc) - doc.to_html + def self.format_for_email(html, post = nil, style: nil) + Email::Styles.new(html, style: style).tap do |doc| + doc.make_all_links_absolute + doc.send :"format_#{style}" if style + DiscourseEvent.trigger(:reduce_cooked, doc, post) + end.to_html end protected diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index 63e2eca471f..d66de21387c 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -286,41 +286,6 @@ HTML end end - describe "make_all_links_absolute" do - let(:base_url) { "http://baseurl.net" } - - def make_abs_string(html) - doc = Nokogiri::HTML.fragment(html) - described_class.make_all_links_absolute(doc) - doc.to_html - end - - before do - Discourse.stubs(:base_url).returns(base_url) - end - - it "adds base url to relative links" do - html = "
@wiseguy, @trollol what do you guys think?
" - output = make_abs_string(html) - expect(output).to eq("@wiseguy, @trollol what do you guys think?
") - end - - it "doesn't change external absolute links" do - html = "Check out this guy.
" - expect(make_abs_string(html)).to eq(html) - end - - it "doesn't change internal absolute links" do - html = "Check out this guy.
" - expect(make_abs_string(html)).to eq(html) - end - - it "can tolerate invalid URLs" do - html = "Check out this guy.
" - expect { make_abs_string(html) }.to_not raise_error - end - end - describe "strip_image_wrapping" do def strip_image_wrapping(html) doc = Nokogiri::HTML.fragment(html) @@ -339,8 +304,36 @@ HTML end describe 'format_for_email' do + let(:base_url) { "http://baseurl.net" } + let(:post) { Fabricate(:post) } + + before do + Discourse.stubs(:base_url).returns(base_url) + end + it 'does not crash' do - PrettyText.format_for_email('test') + PrettyText.format_for_email('test', post) + end + + it "adds base url to relative links" do + html = "@wiseguy, @trollol what do you guys think?
" + output = described_class.format_for_email(html, post) + expect(output).to eq("@wiseguy, @trollol what do you guys think?
") + end + + it "doesn't change external absolute links" do + html = "Check out this guy.
" + expect(described_class.format_for_email(html, post)).to eq(html) + end + + it "doesn't change internal absolute links" do + html = "Check out this guy.
" + expect(described_class.format_for_email(html, post)).to eq(html) + end + + it "can tolerate invalid URLs" do + html = "Check out this guy.
" + expect { described_class.format_for_email(html, post) }.to_not raise_error end end diff --git a/spec/jobs/enqueue_mailing_list_emails_spec.rb b/spec/jobs/enqueue_mailing_list_emails_spec.rb new file mode 100644 index 00000000000..820ed1e812e --- /dev/null +++ b/spec/jobs/enqueue_mailing_list_emails_spec.rb @@ -0,0 +1,128 @@ +require 'rails_helper' +require_dependency 'jobs/base' + +describe Jobs::EnqueueMailingListEmails do + + describe '#target_users' do + + context 'unapproved users' do + Given!(:unapproved_user) { Fabricate(:active_user, approved: false, first_seen_at: 24.hours.ago) } + When do + SiteSetting.must_approve_users = true + unapproved_user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + end + Then { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false) } + + # As a moderator + And { unapproved_user.update_column(:moderator, true) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + + # As an admin + And { unapproved_user.update_attributes(admin: true, moderator: false) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + + # As an approved user + And { unapproved_user.update_attributes(admin: false, moderator: false, approved: true ) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + end + + context 'staged users' do + let!(:staged_user) { Fabricate(:active_user, staged: true, last_emailed_at: 1.year.ago, last_seen_at: 1.year.ago) } + + it "doesn't return staged users" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(staged_user.id)).to eq(false) + end + end + + context "inactive user" do + let!(:inactive_user) { Fabricate(:user, active: false) } + + it "doesn't return users who have been emailed recently" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(inactive_user.id)).to eq(false) + end + end + + context "suspended user" do + let!(:suspended_user) { Fabricate(:user, suspended_till: 1.week.from_now, suspended_at: 1.day.ago) } + + it "doesn't return users who are suspended" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(suspended_user.id)).to eq(false) + end + end + + context 'users with mailing list mode on' do + let(:user) { Fabricate(:active_user, first_seen_at: 24.hours.ago) } + let(:user_option) { user.user_option } + subject { Jobs::EnqueueMailingListEmails.new.target_user_ids } + before do + user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + end + + it "returns a user whose first_seen_at matches the current hour" do + expect(subject).to include user.id + end + + it "returns a user seen multiple days ago" do + user.update(first_seen_at: 72.hours.ago) + expect(subject).to include user.id + end + + it "doesn't return a user who has never been seen" do + user.update(first_seen_at: nil) + expect(subject).to_not include user.id + end + + it "doesn't return users with mailing list mode off" do + user_option.update(mailing_list_mode: false) + expect(subject).to_not include user.id + end + + it "doesn't return users with mailing list mode set to 'individual'" do + user_option.update(mailing_list_mode_frequency: 1) + 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 + end + + it "doesn't return a user who was first seen today" do + user.update(first_seen_at: 2.minutes.ago) + expect(subject).to_not include user.id + end + end + + end + + describe '#execute' do + + let(:user) { Fabricate(:user) } + + context "mailing list emails are enabled" do + before do + Jobs::EnqueueMailingListEmails.any_instance.expects(:target_user_ids).returns([user.id]) + end + + it "enqueues the mailing list email job" do + Jobs.expects(:enqueue).with(:user_email, type: :mailing_list, user_id: user.id) + Jobs::EnqueueMailingListEmails.new.execute({}) + end + end + + context "mailing list emails are disabled" do + before do + Jobs::EnqueueMailingListEmails.any_instance.expects(:target_user_ids).never + end + + it "does not enqueue the mailing list email job" do + SiteSetting.disable_mailing_list_mode = true + Jobs.expects(:enqueue).with(:user_email, type: :mailing_list, user_id: user.id).never + Jobs::EnqueueMailingListEmails.new.execute({}) + end + end + + end + + +end diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index f63b4225ccc..75e32073d99 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -34,10 +34,17 @@ describe Jobs::NotifyMailingListSubscribers do context "with a valid post" do let!(:post) { Fabricate(:post, user: user) } - it "sends the email to the user" do + 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 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 diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 00ceb74a3af..c1f0c6f065a 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -76,6 +76,69 @@ describe UserNotifications do end + describe '.mailing_list' do + subject { UserNotifications.mailing_list(user) } + + context "without new posts" do + it "doesn't send the email" do + expect(subject.to).to be_blank + end + end + + context "with new posts" do + let(:user) { Fabricate(:user) } + let(:topic) { Fabricate(:topic, user: user) } + let!(:new_post) { Fabricate(:post, topic: topic, created_at: 2.hours.ago, raw: "Feel the Bern") } + let!(:old_post) { Fabricate(:post, topic: topic, created_at: 25.hours.ago, raw: "Make America Great Again") } + let(:old_topic) { Fabricate(:topic, user: user, created_at: 10.days.ago) } + let(:new_post_in_old_topic) { Fabricate(:post, topic: old_topic, created_at: 2.hours.ago, raw: "Yes We Can") } + let(:stale_post) { Fabricate(:post, topic: old_topic, created_at: 2.days.ago, raw: "A New American Century") } + + it "works" do + expect(subject.to).to eq([user.email]) + expect(subject.subject).to be_present + expect(subject.from).to eq([SiteSetting.notification_email]) + expect(subject.html_part.body.to_s).to include topic.title + expect(subject.text_part.body.to_s).to be_present + end + + it "includes posts less than 24 hours old" do + expect(subject.html_part.body.to_s).to include new_post.cooked + end + + it "does not include posts older than 24 hours old" do + expect(subject.html_part.body.to_s).to_not include old_post.cooked + end + + it "includes topics created over 24 hours ago which have new posts" do + new_post_in_old_topic + expect(subject.html_part.body.to_s).to include old_topic.title + expect(subject.html_part.body.to_s).to include new_post_in_old_topic.cooked + expect(subject.html_part.body.to_s).to_not include stale_post.cooked + end + + it "includes multiple topics" do + new_post_in_old_topic + expect(subject.html_part.body.to_s).to include topic.title + expect(subject.html_part.body.to_s).to include old_topic.title + end + + it "does not include topics not updated for the past 24 hours" do + stale_post + expect(subject.html_part.body.to_s).to_not include old_topic.title + expect(subject.html_part.body.to_s).to_not include stale_post.cooked + end + + it "includes email_prefix in email subject instead of site title" do + SiteSetting.email_prefix = "Try Discourse" + SiteSetting.title = "Discourse Meta" + + expect(subject.subject).to match(/Try Discourse/) + expect(subject.subject).not_to match(/Discourse Meta/) + end + end + end + describe '.digest' do subject { UserNotifications.digest(user) } diff --git a/spec/models/mailing_list_mode_site_setting_spec.rb b/spec/models/mailing_list_mode_site_setting_spec.rb new file mode 100644 index 00000000000..d8ccc7e4aa1 --- /dev/null +++ b/spec/models/mailing_list_mode_site_setting_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +describe MailingListModeSiteSetting do + describe 'valid_value?' do + it 'returns true for a valid value as an int' do + expect(DigestEmailSiteSetting.valid_value?(0)).to eq true + end + + it 'returns false for a valid value as a string' do + expect(DigestEmailSiteSetting.valid_value?('0')).to eq true + end + + it 'returns false for an out of range value' do + expect(DigestEmailSiteSetting.valid_value?(3)).to eq false + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 92e12e9e71e..63a0dcfb2a2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -596,6 +596,29 @@ describe User do end + describe "update_last_seen!" do + let (:user) { Fabricate(:user) } + let!(:first_visit_date) { Time.zone.now } + let!(:second_visit_date) { 2.hours.from_now } + + it "should update the last seen value" do + expect(user.last_seen_at).to eq nil + user.update_last_seen!(first_visit_date) + expect(user.reload.last_seen_at).to be_within_one_second_of(first_visit_date) + end + + it "should update the first seen value if it doesn't exist" do + user.update_last_seen!(first_visit_date) + expect(user.reload.first_seen_at).to be_within_one_second_of(first_visit_date) + end + + it "should not update the first seen value if it doesn't exist" do + user.update_last_seen!(first_visit_date) + user.update_last_seen!(second_visit_date) + expect(user.reload.first_seen_at).to be_within_one_second_of(first_visit_date) + end + end + describe "last_seen_at" do let(:user) { Fabricate(:user) }