Merge pull request #4852 from tgxworld/pull_the_plug_on_daily_mailing_list

Remove daily mailing mode option as it doesn't scale.

app/assets/javascripts/discourse/controllers/preferences/emails.js.es6

@@ -30,7 +30,6 @@ export default Ember.Controller.extend(PreferencesTabController, {
   @computed()
   mailingListModeOptions() {
     return [
-      {name: I18n.t('user.mailing_list_mode.daily'), value: 0},
       {name: this.get('frequencyEstimate'), value: 1},
       {name: I18n.t('user.mailing_list_mode.individual_no_echo'), value: 2}
     ];

app/jobs/scheduled/enqueue_mailing_list_emails.rb

@@ -1,29 +0,0 @@
-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
-          .activated
-          .not_suspended
-          .not_blocked
-          .joins(:user_option)
-          .where(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

app/mailers/user_notifications.rb

@@ -75,39 +75,6 @@ class UserNotifications < ActionMailer::Base
     end
   end
 
-  # This is the "mailing list summary email"
-  def mailing_list(user, opts={})
-    @since = opts[:since] || 1.day.ago
-    @since_formatted = short_date(@since)
-
-    topics = Topic
-      .joins(:posts)
-      .includes(:posts)
-      .for_digest(user, 100.years.ago)
-      .where("posts.created_at > ?", @since)
-
-    unless user.staff?
-      topics = topics.where("posts.post_type <> ?", Post.types[:whisper])
-    end
-
-    @new_topics = topics.where("topics.created_at > ?", @since).uniq
-    @existing_topics = topics.where("topics.created_at <= ?", @since).uniq
-    @topics = topics.uniq
-
-    return if @topics.empty?
-
-    build_summary_for(user)
-    opts = {
-      from_alias: I18n.t('user_notifications.mailing_list.from', site_name: SiteSetting.title),
-      subject: I18n.t('user_notifications.mailing_list.subject_template', email_prefix: @email_prefix, date: @date),
-      mailing_list_mode: true,
-      add_unsubscribe_link: true,
-      unsubscribe_url: "#{Discourse.base_url}/email/unsubscribe/#{@unsubscribe_key}",
-    }
-
-    apply_notification_styles(build_email(@user.email, opts))
-  end
-
   def digest(user, opts={})
     build_summary_for(user)
     min_date = opts[:since] || user.last_emailed_at || user.last_seen_at || 1.month.ago

app/models/mailing_list_mode_site_setting.rb

@@ -8,7 +8,6 @@ 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_no_echo', value:  2 }
     ]

app/models/user_option.rb

@@ -155,8 +155,8 @@ end
 #  email_previous_replies           :integer          default(2), not null
 #  email_in_reply_to                :boolean          default(TRUE), not null
 #  like_notification_frequency      :integer          default(1), not null
+#  mailing_list_mode_frequency      :integer          default(1), not null
 #  include_tl0_in_digests           :boolean          default(FALSE)
-#  mailing_list_mode_frequency      :integer          default(0), not null
 #
 # Indexes
 #

app/views/user_notifications/mailing_list.html.erb

@@ -1,79 +0,0 @@
-<div id="top"></div>
-<table style='width: 100%; border: 20px solid #eee;' cellspacing='0' cellpadding='0'>
-  <thead>
-    <td style='padding: 10px 10px; background-color: #<%= @header_bgcolor %>;'>
-      <a href='<%= Discourse.base_url %>' style='color: #<%= @anchor_color %>'>
-        <% if logo_url.blank? %>
-          <%= SiteSetting.title %>
-        <% else %>
-          <img src='<%= logo_url %>' style='max-height: 35px; min-height: 35px; height: 35px;' class='site-logo'></a>
-        <% end %>
-        <br />
-        <div style='padding-top: 10px;'>
-          <%= raw(t 'user_notifications.mailing_list.why', site_link: html_site_link(@anchor_color), date: @since_formatted) %>
-        </div>
-        <hr />
-      </a>
-    </td>
-  </thead>
-  <tr>
-    <td>
-      <h4 style="padding: 0 25px; margin: 10px 0;"><%= t('user_notifications.mailing_list.new_topics') %></h3>
-      <ul>
-        <% @new_topics.each do |topic| %>
-          <%= mailing_list_topic(topic, topic.posts.length) %>
-        <% end %>
-      </ul>
-      <h4 style="padding: 0 25px; margin: 10px 0;"><%= t('user_notifications.mailing_list.topic_updates') %></h3>
-      <ul>
-        <% @existing_topics.each do |topic| %>
-          <%= mailing_list_topic(topic, topic.posts.length) %>
-        <% end %>
-      </ul>
-    </td>
-  </tr>
-</table>
-<table style='width: 100%; background: #eee;' cellspacing='0' cellpadding='0'>
-  <% @topics.each do |topic| %>
-    <tr>
-      <td>
-        <h3 style='padding: 20px 20px 10px; margin: 0;'>
-          <%= email_topic_link(topic) %>
-        </h3>
-      </td>
-    </tr>
-    <%- unless SiteSetting.private_email? %>
-      <tr>
-        <td style='border-left: 20px solid #eee; border-right: 20px solid #eee; border-bottom: 10px solid #eee; padding: 10px 10px; background: #fff;'>
-          <% topic.posts.each do |post| %>
-            <div>
-              <img style="float: left; width: 20px; padding-right: 5px;" src="<%= post.user.small_avatar_url %>" title="<%= post.user.username%>">
-              <p style="font-size: 15px; color: #888">
-                <a href='<%= "#{Discourse.base_url}/u/#{post.user.username}" %>'>
-                  <%- if show_username_on_post(post) %>
-                    <%= post.user.username %>
-                  <% end %>
-
-                  <%- if show_name_on_post(post) %>
-                    - <%= post.user.name %>
-                  <% end %>
-                </a>
-
-                <span> - </span>
-                <span><%= I18n.l(post.created_at, format: :long) %></span>
-              </p>
-              <%= raw format_for_email(post, false) %>
-              <hr />
-            </div>
-          <% end %>
-          <a style='font-size: 12px; float: right;' href='<%= Discourse.base_url + topic.relative_url %>'><%= t('user_notifications.mailing_list.view_this_topic') %></a>
-        </td>
-      </tr>
-      <tr>
-        <td style='padding: 0 20px; font-size: 12px;'>
-          <a href="#top"><%= t('user_notifications.mailing_list.back_to_top') %></a>
-        </td>
-      </tr>
-    <% end %>
-  <% end %>
-</table>

app/views/user_notifications/mailing_list.text.erb

@@ -1,33 +0,0 @@
-<%- site_link = raw(@markdown_linker.create(@site_name, '/')) %>
-<%= raw(t 'user_notifications.mailing_list.why', site_link: site_link, date: @since_formatted) %>
-
-<%- if @new_topics.present? -%>
-  ### <%= t 'user_notifications.mailing_list.new_topics' %>
-  <%- @new_topics.each do |topic| %>
-    <%= mailing_list_topic_text(topic) %>
-  <%- end -%>
-<%- end -%>
-
-<%- if @existing_topics.present? -%>
-  ### <%= t 'user_notifications.mailing_list.new_topics' %>
-  <%- @existing_topics.each do |topic| -%>
-    <%= mailing_list_topic_text(topic) %>
-    <%= topic.posts.length %>
-  <%- end -%>
-<%- end -%>
-
---------------------------------------------------------------------------------
-
-<%- unless SiteSetting.private_email? %>
-  <%- @topics.each do |topic| %>
-    ### <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %>
-
-    <%- topic.posts.each do |post| -%>
-      <%= post.user.name || post.user.username %> - <%= post.created_at %>
-  --------------------------------------------------------------------------------
-      <%= post.raw %>
-
-
-    <%- end -%>
-  <%- end %>
-<%- end %>

config/locales/client.en.yml

@@ -599,7 +599,6 @@ en:
         instructions: |
           This setting overrides the activity summary.<br />
           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)"

config/locales/server.en.yml

@@ -2594,15 +2594,6 @@ en:
           above_footer: ''
           below_footer: ''
 
-    mailing_list:
-      why: "All activity on %{site_link} for %{date}"
-      subject_template: "[%{email_prefix}] 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:
       title: "Forgot Password"
       subject_template: "[%{email_prefix}] Password reset"

db/migrate/20170505035229_migrate_mailing_list_daily_updates_users_to_daily_summary.rb

@@ -0,0 +1,11 @@
+class MigrateMailingListDailyUpdatesUsersToDailySummary < ActiveRecord::Migration
+  def change
+    change_column_default :user_options, :mailing_list_mode_frequency, 1
+
+    UserOption.exec_sql(<<~SQL)
+    UPDATE user_options
+    SET digest_after_minutes = 1440, email_digests = 't', mailing_list_mode = 'f'
+    WHERE mailing_list_mode_frequency = 0 AND mailing_list_mode
+    SQL
+  end
+end

spec/jobs/enqueue_mailing_list_emails_spec.rb

@@ -1,142 +0,0 @@
-require 'rails_helper'
-require_dependency 'jobs/base'
-
-describe Jobs::EnqueueMailingListEmails do
-
-  describe '#target_users' do
-
-    context 'unapproved users' do
-      let!(:unapproved_user) { Fabricate(:active_user, approved: false, first_seen_at: 24.hours.ago) }
-
-      before do
-        @original_value = SiteSetting.must_approve_users
-        SiteSetting.must_approve_users = true
-      end
-
-      after do
-        SiteSetting.must_approve_users = @original_value
-      end
-
-      it 'should enqueue the right emails' do
-        unapproved_user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0)
-        expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false)
-
-        # As a moderator
-        unapproved_user.update_column(:moderator, true)
-        expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
-
-        # As an admin
-        unapproved_user.update_attributes(admin: true, moderator: false)
-        expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
-
-        # As an approved user
-        unapproved_user.update_attributes(admin: false, moderator: false, approved: true )
-        expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
-      end
-    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 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
-      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

spec/jobs/notify_mailing_list_subscribers_spec.rb

@@ -79,11 +79,6 @@ describe Jobs::NotifyMailingListSubscribers do
         include_examples "no emails"
       end
 
-      context "to an user who has frequency set to 'daily'" do
-        before { mailing_list_user.user_option.update(mailing_list_mode_frequency: 0) }
-        include_examples "no emails"
-      end
-
       context "to an user who has frequency set to 'always'" do
         before { mailing_list_user.user_option.update(mailing_list_mode_frequency: 1) }
         include_examples "one email"

spec/mailers/user_notifications_spec.rb

@@ -79,94 +79,6 @@ 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
-        expect(subject.header["List-Unsubscribe"].to_s).to match(/\/email\/unsubscribe\/\h{64}/)
-      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
-
-      it "excludes whispers" do
-        new_post_in_old_topic
-        whisper = Fabricate(:post, topic: old_topic, created_at: 1.hour.ago, raw: "This is secret", post_type: Post.types[:whisper])
-        expect(subject.html_part.body.to_s).to include old_topic.title
-        expect(subject.html_part.body.to_s).to_not include whisper.cooked
-      end
-
-      it "includes whispers for staff" do
-        user.admin = true; user.save!
-        new_post_in_old_topic
-        whisper = Fabricate(:post, topic: old_topic, created_at: 1.hour.ago, raw: "This is secret", post_type: Post.types[:whisper])
-        expect(subject.html_part.body.to_s).to include old_topic.title
-        expect(subject.html_part.body.to_s).to include whisper.cooked
-      end
-
-      it "hides details for private email" do
-        SiteSetting.private_email = true
-
-        expect(subject.html_part.body.to_s).not_to include(topic.title)
-        expect(subject.html_part.body.to_s).not_to include(topic.slug)
-        expect(subject.text_part.body.to_s).not_to include(topic.title)
-        expect(subject.text_part.body.to_s).not_to include(topic.slug)
-      end
-    end
-  end
-
   describe '.digest' do
 
     subject { UserNotifications.digest(user) }

spec/models/mailing_list_mode_site_setting_spec.rb

@@ -3,15 +3,17 @@ require 'rails_helper'
 describe MailingListModeSiteSetting do
   describe 'valid_value?' do
     it 'returns true for a valid value as an int' do
-      expect(MailingListModeSiteSetting.valid_value?(0)).to eq(true)
+      expect(MailingListModeSiteSetting.valid_value?(1)).to eq(true)
     end
 
     it 'returns true for a valid value as a string' do
-      expect(MailingListModeSiteSetting.valid_value?('0')).to eq(true)
+      expect(MailingListModeSiteSetting.valid_value?('1')).to eq(true)
     end
 
     it 'returns false for an out of range value' do
-      expect(MailingListModeSiteSetting.valid_value?(3)).to eq(false)
+      [0, 3].each do |value|
+        expect(MailingListModeSiteSetting.valid_value?(value)).to eq(false)
+      end
     end
   end
 end