IntenseWebs/discourse
3
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-22 08:57:10 -06:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
https://meta.discourse.org/t/daily-updates-option-for-mailing-list-mode/45029/14?u=tgxworld
This commit is contained in:
Guo Xiang Tan 2017-05-05 12:11:20 +08:00
parent 441e0059af
commit 71a266b673
14 changed files with 17 additions and 425 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/assets/javascripts/discourse/controllers/preferences/emails.js.es6
View File

@ -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}
];

29
app/jobs/scheduled/enqueue_mailing_list_emails.rb
View File

@ -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

33
app/mailers/user_notifications.rb
View File

@ -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

1
app/models/mailing_list_mode_site_setting.rb
View File

@ -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 }
]

2
app/models/user_option.rb
View File

@ -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
#

79
app/views/user_notifications/mailing_list.html.erb
View File

@ -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>

33
app/views/user_notifications/mailing_list.text.erb
View File

@ -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 %>

1
config/locales/client.en.yml
View File

@ -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)"

9
config/locales/server.en.yml
View File

@ -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"

11
db/migrate/20170505035229_migrate_mailing_list_daily_updates_users_to_daily_summary.rb Normal file
View File

@ -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

142
spec/jobs/enqueue_mailing_list_emails_spec.rb
View File

@ -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

5
spec/jobs/notify_mailing_list_subscribers_spec.rb
View File

@ -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"

88
spec/mailers/user_notifications_spec.rb
View File

@ -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) }

8
spec/models/mailing_list_mode_site_setting_spec.rb
View File

@ -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