From 66436d08959998be20c6c6bf631177d8c1f3e0d1 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 2 Jan 2019 10:47:32 +0100 Subject: [PATCH] Improve e-mail digest (#9689) - Reduce time-to-digest from 20 to 7 days - Fetch mentions starting from +1 day since last login - Fix case when last login is more recent than last e-mail - Do not render all mentions, only 40, but show number in subject - Do not send digest to moved accounts - Do send digest to silenced accounts --- app/mailers/notification_mailer.rb | 16 ++++++++++------ app/models/account.rb | 5 +++-- app/models/user.rb | 4 +++- app/views/notification_mailer/digest.html.haml | 2 +- app/views/notification_mailer/digest.text.erb | 2 +- app/workers/scheduler/email_scheduler.rb | 12 ++++++------ spec/mailers/notification_mailer_spec.rb | 14 +------------- 7 files changed, 25 insertions(+), 30 deletions(-) diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index b458442962..66fa337c1f 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -66,16 +66,20 @@ class NotificationMailer < ApplicationMailer end def digest(recipient, **opts) - @me = recipient - @since = opts[:since] || @me.user.last_emailed_at || @me.user.current_sign_in_at - @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since) - @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count + return if recipient.user.disabled? - return if @me.user.disabled? || @notifications.empty? + @me = recipient + @since = opts[:since] || [@me.user.last_emailed_at, (@me.user.current_sign_in_at + 1.day)].compact.max + @notifications_count = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).count + + return if @notifications_count.zero? + + @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).limit(40) + @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count locale_for_account(@me) do mail to: @me.user.email, - subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications.size) + subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications_count) end end diff --git a/app/models/account.rb b/app/models/account.rb index f354bc29b8..97beb416aa 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -85,6 +85,7 @@ class Account < ApplicationRecord scope :silenced, -> { where(silenced: true) } scope :suspended, -> { where(suspended: true) } scope :without_suspended, -> { where(suspended: false) } + scope :without_silenced, -> { where(silenced: false) } scope :recent, -> { reorder(id: :desc) } scope :bots, -> { where(actor_type: %w(Application Service)) } scope :alphabetic, -> { order(domain: :asc, username: :asc) } @@ -92,8 +93,8 @@ class Account < ApplicationRecord scope :matches_username, ->(value) { where(arel_table[:username].matches("#{value}%")) } scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) } scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } - scope :searchable, -> { where(suspended: false).where(moved_to_account_id: nil) } - scope :discoverable, -> { searchable.where(silenced: false).where(discoverable: true).joins(:account_stat).where(AccountStat.arel_table[:followers_count].gteq(MIN_FOLLOWERS_DISCOVERY)).by_recent_status } + scope :searchable, -> { without_suspended.where(moved_to_account_id: nil) } + scope :discoverable, -> { searchable.without_silenced.where(discoverable: true).joins(:account_stat).where(AccountStat.arel_table[:followers_count].gteq(MIN_FOLLOWERS_DISCOVERY)).by_recent_status } scope :tagged_with, ->(tag) { joins(:accounts_tags).where(accounts_tags: { tag_id: tag }) } scope :by_recent_status, -> { order(Arel.sql('(case when account_stats.last_status_at is null then 1 else 0 end) asc, account_stats.last_status_at desc')) } scope :popular, -> { order('account_stats.followers_count desc') } diff --git a/app/models/user.rb b/app/models/user.rb index 1684b9bea7..8b374c1825 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,7 +50,7 @@ class User < ApplicationRecord # every day. Raising the duration reduces the amount of expensive # RegenerationWorker jobs that need to be run when those people come # to check their feed - ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days + ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days.freeze devise :two_factor_authenticatable, otp_secret_encryption_key: Rails.configuration.x.otp_secret @@ -83,9 +83,11 @@ class User < ApplicationRecord scope :moderators, -> { where(moderator: true) } scope :staff, -> { admins.or(moderators) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :enabled, -> { where(disabled: false) } scope :inactive, -> { where(arel_table[:current_sign_in_at].lt(ACTIVE_DURATION.ago)) } scope :active, -> { confirmed.where(arel_table[:current_sign_in_at].gteq(ACTIVE_DURATION.ago)).joins(:account).where(accounts: { suspended: false }) } scope :matches_email, ->(value) { where(arel_table[:email].matches("#{value}%")) } + scope :emailable, -> { confirmed.enabled.joins(:account).merge(Account.searchable) } before_validation :sanitize_languages diff --git a/app/views/notification_mailer/digest.html.haml b/app/views/notification_mailer/digest.html.haml index 10e44f8dde..a94ace228d 100644 --- a/app/views/notification_mailer/digest.html.haml +++ b/app/views/notification_mailer/digest.html.haml @@ -14,7 +14,7 @@ %tr %td.column-cell.text-center.padded %h1= t 'notification_mailer.digest.title' - %p.lead= t('notification_mailer.digest.body', since: l(@since.to_date, format: :short), instance: site_hostname) + %p.lead= t('notification_mailer.digest.body', since: l((@me.user_current_sign_in_at || @since).to_date, format: :short), instance: site_hostname) %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } %tbody %tr diff --git a/app/views/notification_mailer/digest.text.erb b/app/views/notification_mailer/digest.text.erb index e0d1f9b8b9..b2c85a9e3d 100644 --- a/app/views/notification_mailer/digest.text.erb +++ b/app/views/notification_mailer/digest.text.erb @@ -1,6 +1,6 @@ <%= raw t('application_mailer.salutation', name: display_name(@me)) %> -<%= raw t('notification_mailer.digest.body', since: l(@since), instance: root_url) %> +<%= raw t('notification_mailer.digest.body', since: l(@me.user_current_sign_in_at || @since), instance: root_url) %> <% @notifications.each do |notification| %> * <%= raw t('notification_mailer.digest.mention', name: notification.from_account.acct) %> diff --git a/app/workers/scheduler/email_scheduler.rb b/app/workers/scheduler/email_scheduler.rb index 24ec89b298..1eeeee412d 100644 --- a/app/workers/scheduler/email_scheduler.rb +++ b/app/workers/scheduler/email_scheduler.rb @@ -5,6 +5,9 @@ class Scheduler::EmailScheduler sidekiq_options unique: :until_executed, retry: 0 + FREQUENCY = 7.days.freeze + SIGN_IN_OFFSET = 1.day.freeze + def perform eligible_users.reorder(nil).find_each do |user| next unless user.allows_digest_emails? @@ -15,11 +18,8 @@ class Scheduler::EmailScheduler private def eligible_users - User.confirmed - .joins(:account) - .where(accounts: { silenced: false, suspended: false }) - .where(disabled: false) - .where('current_sign_in_at < ?', 20.days.ago) - .where('last_emailed_at IS NULL OR last_emailed_at < ?', 20.days.ago) + User.emailable + .where('current_sign_in_at < ?', (FREQUENCY + SIGN_IN_OFFSET).ago) + .where('last_emailed_at IS NULL OR last_emailed_at < ?', FREQUENCY.ago) end end diff --git a/spec/mailers/notification_mailer_spec.rb b/spec/mailers/notification_mailer_spec.rb index 1be01e8baa..38916b54f1 100644 --- a/spec/mailers/notification_mailer_spec.rb +++ b/spec/mailers/notification_mailer_spec.rb @@ -126,19 +126,7 @@ RSpec.describe NotificationMailer, type: :mailer do end end - it 'includes activities since the date specified by :since option' do - receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z') - mail = NotificationMailer.digest(receiver.account, since: Time.parse('2000-01-01T00:00:00Z')) - expect(mail.body.encoded).to include 'Jan 01, 2000, 00:00' - end - - it 'includes activities since the receiver was last emailed if :since option is unavailable' do - receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z') - mail = NotificationMailer.digest(receiver.account) - expect(mail.body.encoded).to include 'Feb 01, 2000, 00:00' - end - - it 'includes activities since the receiver last signed in if :since option and the last emailed date are unavailable' do + it 'includes activities since the receiver last signed in' do receiver.update!(last_emailed_at: nil, current_sign_in_at: '2000-03-01T00:00:00Z') mail = NotificationMailer.digest(receiver.account) expect(mail.body.encoded).to include 'Mar 01, 2000, 00:00'