From feffe23cc5fd7dffa68137b156910ee831ad6aa2 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Sat, 21 May 2016 06:17:54 -0700 Subject: [PATCH] FEATURE: More granular mailing list mode (#4068) * Rearrange frontend to account for mailing list mode * Allow update of user preference for mailing list frequency * Add mailing list frequency estimate * Simplify frequency estimate; disable activity summary for mailing list mode * Remove combined updates * Add specs for enqueue mailing list mode job * Write mailing list method for mailer * Fix linting error * Account for stale topics * Add translations for default mailing list setting * One query for mailing list topics * Fix failing spec * WIP * Flesh out html template * First pass at text-based mailing list summary * Add user avatar * Properly format posts for mailing list * Move make_all_links_absolute into Email::Styles * Apply first_seen_at to user * Send mailing list email summary hourly based on first_seen_at * Branch and test cleanup * Use existing mailing list mode estimate * Fix failing specs --- .../discourse/controllers/preferences.js.es6 | 18 +++ .../components/preference-checkbox.hbs | 2 +- .../discourse/templates/user/preferences.hbs | 31 ++++- app/helpers/user_notifications_helper.rb | 4 +- .../notify_mailing_list_subscribers.rb | 2 +- .../scheduled/enqueue_mailing_list_emails.rb | 27 ++++ app/mailers/user_notifications.rb | 42 ++++-- app/models/mailing_list_mode_site_setting.rb | 19 +++ app/models/post.rb | 8 ++ app/models/user.rb | 1 + app/models/user_option.rb | 1 + app/serializers/user_option_serializer.rb | 1 + app/services/user_updater.rb | 1 + .../user_notifications/mailing_list.html.erb | 76 +++++++++++ .../user_notifications/mailing_list.text.erb | 31 +++++ config/locales/client.en.yml | 12 +- config/locales/server.en.yml | 13 ++ config/site_settings.yml | 3 + ...9073132_add_mailing_list_mode_frequency.rb | 5 + .../20160326001747_add_user_first_visit.rb | 5 + lib/email/styles.rb | 12 ++ lib/pretty_text.rb | 27 +--- spec/components/pretty_text_spec.rb | 65 ++++----- spec/jobs/enqueue_mailing_list_emails_spec.rb | 128 ++++++++++++++++++ .../notify_mailing_list_subscribers_spec.rb | 9 +- spec/mailers/user_notifications_spec.rb | 63 +++++++++ .../mailing_list_mode_site_setting_spec.rb | 17 +++ spec/models/user_spec.rb | 23 ++++ 28 files changed, 567 insertions(+), 79 deletions(-) create mode 100644 app/jobs/scheduled/enqueue_mailing_list_emails.rb create mode 100644 app/models/mailing_list_mode_site_setting.rb create mode 100644 app/views/user_notifications/mailing_list.html.erb create mode 100644 app/views/user_notifications/mailing_list.text.erb create mode 100644 db/migrate/20160309073132_add_mailing_list_mode_frequency.rb create mode 100644 db/migrate/20160326001747_add_user_first_visit.rb create mode 100644 spec/jobs/enqueue_mailing_list_emails_spec.rb create mode 100644 spec/models/mailing_list_mode_site_setting_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/preferences.js.es6 b/app/assets/javascripts/discourse/controllers/preferences.js.es6 index 6ccc5bb22..733268a16 100644 --- a/app/assets/javascripts/discourse/controllers/preferences.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences.js.es6 @@ -62,6 +62,24 @@ export default Ember.Controller.extend(CanCheckEmails, { return this.siteSettings.available_locales.split('|').map(s => ({ name: s, value: s })); }, + @computed() + frequencyEstimate() { + var estimate = this.get('model.mailing_list_posts_per_day'); + if (!estimate || estimate < 2) { + return I18n.t('user.mailing_list_mode.few_per_day'); + } else { + return I18n.t('user.mailing_list_mode.many_per_day', { dailyEmailEstimate: estimate }); + } + }, + + @computed() + mailingListModeOptions() { + return [ + {name: I18n.t('user.mailing_list_mode.daily'), value: 0}, + {name: this.get('frequencyEstimate'), value: 1} + ]; + }, + previousRepliesOptions: [ {name: I18n.t('user.email_previous_replies.always'), value: 0}, {name: I18n.t('user.email_previous_replies.unless_emailed'), value: 1}, diff --git a/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs b/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs index b47200bf5..f7931d591 100644 --- a/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs +++ b/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs @@ -1,4 +1,4 @@ diff --git a/app/assets/javascripts/discourse/templates/user/preferences.hbs b/app/assets/javascripts/discourse/templates/user/preferences.hbs index 644b9a84b..9c14f6b45 100644 --- a/app/assets/javascripts/discourse/templates/user/preferences.hbs +++ b/app/assets/javascripts/discourse/templates/user/preferences.hbs @@ -185,9 +185,6 @@ {{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}} - {{#unless siteSettings.disable_mailing_list_mode}} - {{preference-checkbox warning="checkMailingList" labelKey="user.mailing_list_mode" checked=model.user_option.mailing_list_mode}} - {{/unless}} {{preference-checkbox labelKey="user.email_always" checked=model.user_option.email_always}} {{#unless model.user_option.email_always}}
@@ -198,11 +195,33 @@ {{/if}}
{{/unless}} - - - +
+ + {{#if canReceiveDigest}} + {{preference-checkbox labelKey="user.email_digests.title" disabled=model.user_option.mailing_list_mode checked=model.user_option.email_digests}} + {{#if model.user_option.email_digests}} +
+ {{combo-box valueAttribute="value" content=digestFrequencies value=model.user_option.digest_after_minutes}} +
+ {{/if}} + {{/if}} +
+ + {{#unless siteSettings.disable_mailing_list_mode}} +
+ + {{preference-checkbox labelKey="user.mailing_list_mode.enabled" warning="checkMailingList" checked=model.user_option.mailing_list_mode}} +
{{{i18n 'user.mailing_list_mode.instructions'}}}
+ {{#if model.user_option.mailing_list_mode}} +
+ {{combo-box valueAttribute="value" content=mailingListModeOptions value=model.user_option.mailing_list_mode_frequency}} +
+ {{/if}} +
+ {{/unless}} +
{{desktop-notification-config}} diff --git a/app/helpers/user_notifications_helper.rb b/app/helpers/user_notifications_helper.rb index 4c088836d..39553ffca 100644 --- a/app/helpers/user_notifications_helper.rb +++ b/app/helpers/user_notifications_helper.rb @@ -65,9 +65,9 @@ module UserNotificationsHelper normalize_name(post.user.name) != normalize_name(post.user.username) end - def format_for_email(post, use_excerpt) + def format_for_email(post, use_excerpt, style: nil) html = use_excerpt ? post.excerpt : post.cooked - PrettyText.format_for_email(html, post).html_safe + PrettyText.format_for_email(html, post, style: style).html_safe end end diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 38cb079a3..bbae4bb8b 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}) + .where(user_options: {mailing_list_mode: true, mailing_list_mode_frequency: 1}) .where('NOT EXISTS( SELECT 1 FROM topic_users tu diff --git a/app/jobs/scheduled/enqueue_mailing_list_emails.rb b/app/jobs/scheduled/enqueue_mailing_list_emails.rb new file mode 100644 index 000000000..241726492 --- /dev/null +++ b/app/jobs/scheduled/enqueue_mailing_list_emails.rb @@ -0,0 +1,27 @@ +module Jobs + + class EnqueueMailingListEmails < Jobs::Scheduled + every 1.hour + + def execute(args) + return if SiteSetting.disable_mailing_list_mode? + target_user_ids.each do |user_id| + Jobs.enqueue(:user_email, type: :mailing_list, user_id: user_id) + end + end + + def target_user_ids + # Users who want to receive daily mailing list emails + User.real + .not_suspended + .joins(:user_option) + .where(active: true, staged: false, user_options: {mailing_list_mode: true, mailing_list_mode_frequency: 0}) + .where("#{!SiteSetting.must_approve_users?} OR approved OR moderator OR admin") + .where("date_part('hour', first_seen_at) = date_part('hour', CURRENT_TIMESTAMP)") # where the hour of first_seen_at is the same as the current hour + .where("COALESCE(first_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - '23 HOURS'::INTERVAL") # don't send unless you've been around for a day already + .pluck(:id) + end + + end + +end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 4b9ffa272..a1b5361f0 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -73,16 +73,27 @@ class UserNotifications < ActionMailer::Base end end - def digest(user, opts={}) - @user = user - @base_url = Discourse.base_url + def mailing_list(user, opts={}) + @since = opts[:since] || 1.day.ago + @since_formatted = short_date(@since) + @new_topic_posts = Post.mailing_list_new_topics(user, @since).group_by(&:topic) || {} + @existing_topic_posts = Post.mailing_list_updates(user, @since).group_by(&:topic) || {} + @posts_by_topic = @new_topic_posts.merge @existing_topic_posts + return unless @posts_by_topic.present? + + build_summary_for(user) + build_email @user.email, + from_alias: I18n.t('user_notifications.mailing_list.from', site_name: SiteSetting.title), + subject: I18n.t('user_notifications.mailing_list.subject_template', + site_name: @site_name, + date: @date) + end + + def digest(user, opts={}) + build_summary_for(user) min_date = opts[:since] || @user.last_emailed_at || @user.last_seen_at || 1.month.ago - @site_name = SiteSetting.email_prefix.presence || SiteSetting.title - - @header_color = ColorScheme.hex_for_name('header_background') - @anchor_color = ColorScheme.hex_for_name('tertiary') @last_seen_at = short_date(@user.last_seen_at || @user.created_at) # A list of topics to show the user @@ -106,10 +117,8 @@ class UserNotifications < ActionMailer::Base end @featured_topics, @new_topics = @featured_topics[0..4], @featured_topics[5..-1] - @markdown_linker = MarkdownLinker.new(Discourse.base_url) - @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) - build_email user.email, + build_email @user.email, from_alias: I18n.t('user_notifications.digest.from', site_name: SiteSetting.title), subject: I18n.t('user_notifications.digest.subject_template', site_name: @site_name, @@ -396,4 +405,17 @@ class UserNotifications < ActionMailer::Base build_email(user.email, email_opts) end + + private + + def build_summary_for(user) + @user = user + @date = short_date(Time.now) + @base_url = Discourse.base_url + @site_name = SiteSetting.email_prefix.presence || SiteSetting.title + @header_color = ColorScheme.hex_for_name('header_background') + @anchor_color = ColorScheme.hex_for_name('tertiary') + @markdown_linker = MarkdownLinker.new(@base_url) + @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) + end end diff --git a/app/models/mailing_list_mode_site_setting.rb b/app/models/mailing_list_mode_site_setting.rb new file mode 100644 index 000000000..b2a8291d0 --- /dev/null +++ b/app/models/mailing_list_mode_site_setting.rb @@ -0,0 +1,19 @@ +require_dependency 'enum_site_setting' + +class MailingListModeSiteSetting < EnumSiteSetting + def self.valid_value?(val) + val.to_i.to_s == val.to_s && + values.any? { |v| v[:value] == val.to_i } + end + + def self.values + @values ||= [ + { name: 'user.mailing_list_mode.daily', value: 0 }, + { name: 'user.mailing_list_mode.individual', value: 1 } + ] + end + + def self.translate_names? + true + end +end diff --git a/app/models/post.rb b/app/models/post.rb index 981a0ba92..65a1e7fcb 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -65,6 +65,14 @@ class Post < ActiveRecord::Base scope :with_topic_subtype, ->(subtype) { joins(:topic).where('topics.subtype = ?', subtype) } scope :visible, -> { joins(:topic).where('topics.visible = true').where(hidden: false) } scope :secured, lambda { |guardian| where('posts.post_type in (?)', Topic.visible_post_types(guardian && guardian.user))} + scope :for_mailing_list, ->(user, since) { + created_since(since) + .joins(:topic) + .where(topic: Topic.for_digest(user, 100.years.ago)) # we want all topics with new content, regardless when they were created + .order('posts.created_at ASC') + } + scope :mailing_list_new_topics, ->(user, since) { for_mailing_list(user, since).where('topics.created_at > ?', since) } + scope :mailing_list_updates, ->(user, since) { for_mailing_list(user, since).where('topics.created_at <= ?', since) } delegate :username, to: :user diff --git a/app/models/user.rb b/app/models/user.rb index 8d68c337b..a410a7116 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -476,6 +476,7 @@ class User < ActiveRecord::Base update_previous_visit(now) # using update_column to avoid the AR transaction update_column(:last_seen_at, now) + update_column(:first_seen_at, now) unless self.first_seen_at end def self.gravatar_template(email) diff --git a/app/models/user_option.rb b/app/models/user_option.rb index d986a2885..ad4ac23a4 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -24,6 +24,7 @@ class UserOption < ActiveRecord::Base def set_defaults self.email_always = SiteSetting.default_email_always self.mailing_list_mode = SiteSetting.default_email_mailing_list_mode + self.mailing_list_mode_frequency = SiteSetting.default_email_mailing_list_mode_frequency self.email_direct = SiteSetting.default_email_direct self.automatically_unpin_topics = SiteSetting.default_topics_automatic_unpin self.email_private_messages = SiteSetting.default_email_private_messages diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index 591b07e45..a6ea26da1 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -2,6 +2,7 @@ class UserOptionSerializer < ApplicationSerializer attributes :user_id, :email_always, :mailing_list_mode, + :mailing_list_mode_frequency, :email_digests, :email_private_messages, :email_direct, diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 747ce7d15..fa9c57736 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -9,6 +9,7 @@ class UserUpdater OPTION_ATTR = [ :email_always, :mailing_list_mode, + :mailing_list_mode_frequency, :email_digests, :email_direct, :email_private_messages, diff --git a/app/views/user_notifications/mailing_list.html.erb b/app/views/user_notifications/mailing_list.html.erb new file mode 100644 index 000000000..efd6ac3df --- /dev/null +++ b/app/views/user_notifications/mailing_list.html.erb @@ -0,0 +1,76 @@ +
+ + + + + + + +
+ + <% if logo_url.blank? %> + <%= SiteSetting.title %> + <% else %> + + <% end %> +
+
+ <%= raw(t 'user_notifications.mailing_list.why', site_link: html_site_link(@anchor_color), date: @since_formatted) %> +
+
+ +
+

<%= t('user_notifications.mailing_list.new_topics') %>

+
    + <% @new_topic_posts.keys.each do |topic| %> +
  • + <%= raw format_topic_title(topic.title) %> + <%= @new_topic_posts[topic].length %> + <%= category_badge(topic.category, inline_style: true, absolute_url: true) %> +
    • + <% end %> +
+

<%= t('user_notifications.mailing_list.topic_updates') %>

+
    + <% @existing_topic_posts.keys.each do |topic| %> +
  • + <%= raw format_topic_title(topic.title) %> + <%= @existing_topic_posts[topic].length %> + <%= category_badge(topic.category, inline_style: true, absolute_url: true) %> +
    • + <% end %> +
+
+ + <% @posts_by_topic.keys.each do |topic| %> + + + + + + + + + + <% end %> +
+

+ <%= raw format_topic_title(topic.title) %> +

+
+ <% @posts_by_topic[topic].each do |post| %> +
+ +

+ '><%= post.user.name || post.user.username %> + - + <%= I18n.l(post.created_at, format: :long) %> +

+ <%= raw format_for_email(post, false, style: :notification) %> +
+
+ <% end %> + <%= t('user_notifications.mailing_list.view_this_topic') %> +
+ <%= t('user_notifications.mailing_list.back_to_top') %> +
diff --git a/app/views/user_notifications/mailing_list.text.erb b/app/views/user_notifications/mailing_list.text.erb new file mode 100644 index 000000000..5208a9f08 --- /dev/null +++ b/app/views/user_notifications/mailing_list.text.erb @@ -0,0 +1,31 @@ +<%- site_link = raw(@markdown_linker.create(@site_name, '/')) %> +<%= raw(t 'user_notifications.mailing_list.why', site_link: site_link, date: @since_formatted) %> + +<%- if @new_topic_posts.keys.present? -%> + ### <%= t 'user_notifications.mailing_list.new_topics' %> + <%- @new_topic_posts.keys.each do |topic| %> + <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + <%- end -%> +<%- end -%> + +<%- if @existing_topic_posts.keys.present? -%> + ### <%= t 'user_notifications.mailing_list.new_topics' %> + <%- @existing_topic_posts.keys.each do |topic| -%> + <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + <%= @existing_topic_posts[topic].length %> + <%- end -%> +<%- end -%> + +-------------------------------------------------------------------------------- + +<%- @posts_by_topic.keys.each do |topic| %> + ### <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + + <%- @posts_by_topic[topic].each do |post| -%> + <%= post.user.name || post.user.username %> - <%= post.created_at %> +-------------------------------------------------------------------------------- + <%= post.raw %> + + + <%- end -%> +<%- end %> diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 80c13dabd..5a4c386f4 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -504,7 +504,17 @@ en: suspended_notice: "This user is suspended until {{date}}." suspended_reason: "Reason: " github_profile: "Github" - mailing_list_mode: "Send me an email for every new post (unless I mute the topic or category)" + email_activity_summary: "Activity Summary" + mailing_list_mode: + label: "Mailing list mode" + enabled: "Enable mailing list mode." + instructions: | + This setting overrides the activity summary.
+ 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 0c73c7d85..663ea6a9b 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 fc59c0589..3f993bb21 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 000000000..dcdce07c4 --- /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 000000000..50e000ab0 --- /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 a82c8459f..25076ab72 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 bfdcbbcd9..5ff3c054f 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 63e2eca47..d66de2138 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 000000000..820ed1e81 --- /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 f63b4225c..75e32073d 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 00ceb74a3..c1f0c6f06 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 000000000..d8ccc7e4a --- /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 92e12e9e7..63a0dcfb2 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) }