Browse Source

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
Eugen Rochko 7 months ago
parent
commit
66436d0895
No account linked to committer's email address

+ 10
- 6
app/mailers/notification_mailer.rb View File

@@ -66,16 +66,20 @@ class NotificationMailer < ApplicationMailer
66 66
   end
67 67
 
68 68
   def digest(recipient, **opts)
69
-    @me            = recipient
70
-    @since         = opts[:since] || @me.user.last_emailed_at || @me.user.current_sign_in_at
71
-    @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since)
72
-    @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count
69
+    return if recipient.user.disabled?
70
+
71
+    @me                  = recipient
72
+    @since               = opts[:since] || [@me.user.last_emailed_at, (@me.user.current_sign_in_at + 1.day)].compact.max
73
+    @notifications_count = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).count
73 74
 
74
-    return if @me.user.disabled? || @notifications.empty?
75
+    return if @notifications_count.zero?
76
+
77
+    @notifications = Notification.where(account: @me, activity_type: 'Mention').where('created_at > ?', @since).limit(40)
78
+    @follows_since = Notification.where(account: @me, activity_type: 'Follow').where('created_at > ?', @since).count
75 79
 
76 80
     locale_for_account(@me) do
77 81
       mail to: @me.user.email,
78
-           subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications.size)
82
+           subject: I18n.t(:subject, scope: [:notification_mailer, :digest], count: @notifications_count)
79 83
     end
80 84
   end
81 85
 

+ 3
- 2
app/models/account.rb View File

@@ -85,6 +85,7 @@ class Account < ApplicationRecord
85 85
   scope :silenced, -> { where(silenced: true) }
86 86
   scope :suspended, -> { where(suspended: true) }
87 87
   scope :without_suspended, -> { where(suspended: false) }
88
+  scope :without_silenced, -> { where(silenced: false) }
88 89
   scope :recent, -> { reorder(id: :desc) }
89 90
   scope :bots, -> { where(actor_type: %w(Application Service)) }
90 91
   scope :alphabetic, -> { order(domain: :asc, username: :asc) }
@@ -92,8 +93,8 @@ class Account < ApplicationRecord
92 93
   scope :matches_username, ->(value) { where(arel_table[:username].matches("#{value}%")) }
93 94
   scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) }
94 95
   scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) }
95
-  scope :searchable, -> { where(suspended: false).where(moved_to_account_id: nil) }
96
-  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 }
96
+  scope :searchable, -> { without_suspended.where(moved_to_account_id: nil) }
97
+  scope :discoverable, -> { searchable.without_silenced.where(discoverable: true).joins(:account_stat).where(AccountStat.arel_table[:followers_count].gteq(MIN_FOLLOWERS_DISCOVERY)).by_recent_status }
97 98
   scope :tagged_with, ->(tag) { joins(:accounts_tags).where(accounts_tags: { tag_id: tag }) }
98 99
   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')) }
99 100
   scope :popular, -> { order('account_stats.followers_count desc') }

+ 3
- 1
app/models/user.rb View File

@@ -50,7 +50,7 @@ class User < ApplicationRecord
50 50
   # every day. Raising the duration reduces the amount of expensive
51 51
   # RegenerationWorker jobs that need to be run when those people come
52 52
   # to check their feed
53
-  ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days
53
+  ACTIVE_DURATION = ENV.fetch('USER_ACTIVE_DAYS', 7).to_i.days.freeze
54 54
 
55 55
   devise :two_factor_authenticatable,
56 56
          otp_secret_encryption_key: Rails.configuration.x.otp_secret
@@ -83,9 +83,11 @@ class User < ApplicationRecord
83 83
   scope :moderators, -> { where(moderator: true) }
84 84
   scope :staff, -> { admins.or(moderators) }
85 85
   scope :confirmed, -> { where.not(confirmed_at: nil) }
86
+  scope :enabled, -> { where(disabled: false) }
86 87
   scope :inactive, -> { where(arel_table[:current_sign_in_at].lt(ACTIVE_DURATION.ago)) }
87 88
   scope :active, -> { confirmed.where(arel_table[:current_sign_in_at].gteq(ACTIVE_DURATION.ago)).joins(:account).where(accounts: { suspended: false }) }
88 89
   scope :matches_email, ->(value) { where(arel_table[:email].matches("#{value}%")) }
90
+  scope :emailable, -> { confirmed.enabled.joins(:account).merge(Account.searchable) }
89 91
 
90 92
   before_validation :sanitize_languages
91 93
 

+ 1
- 1
app/views/notification_mailer/digest.html.haml View File

@@ -14,7 +14,7 @@
14 14
                           %tr
15 15
                             %td.column-cell.text-center.padded
16 16
                               %h1= t 'notification_mailer.digest.title'
17
-                              %p.lead= t('notification_mailer.digest.body', since: l(@since.to_date, format: :short), instance: site_hostname)
17
+                              %p.lead= t('notification_mailer.digest.body', since: l((@me.user_current_sign_in_at || @since).to_date, format: :short), instance: site_hostname)
18 18
                               %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 }
19 19
                                 %tbody
20 20
                                   %tr

+ 1
- 1
app/views/notification_mailer/digest.text.erb View File

@@ -1,6 +1,6 @@
1 1
 <%= raw t('application_mailer.salutation', name: display_name(@me)) %>
2 2
 
3
-<%= raw t('notification_mailer.digest.body', since: l(@since), instance: root_url) %>
3
+<%= raw t('notification_mailer.digest.body', since: l(@me.user_current_sign_in_at || @since), instance: root_url) %>
4 4
 <% @notifications.each do |notification| %>
5 5
 
6 6
 * <%= raw t('notification_mailer.digest.mention', name: notification.from_account.acct) %>

+ 6
- 6
app/workers/scheduler/email_scheduler.rb View File

@@ -5,6 +5,9 @@ class Scheduler::EmailScheduler
5 5
 
6 6
   sidekiq_options unique: :until_executed, retry: 0
7 7
 
8
+  FREQUENCY      = 7.days.freeze
9
+  SIGN_IN_OFFSET = 1.day.freeze
10
+
8 11
   def perform
9 12
     eligible_users.reorder(nil).find_each do |user|
10 13
       next unless user.allows_digest_emails?
@@ -15,11 +18,8 @@ class Scheduler::EmailScheduler
15 18
   private
16 19
 
17 20
   def eligible_users
18
-    User.confirmed
19
-        .joins(:account)
20
-        .where(accounts: { silenced: false, suspended: false })
21
-        .where(disabled: false)
22
-        .where('current_sign_in_at < ?', 20.days.ago)
23
-        .where('last_emailed_at IS NULL OR last_emailed_at < ?', 20.days.ago)
21
+    User.emailable
22
+        .where('current_sign_in_at < ?', (FREQUENCY + SIGN_IN_OFFSET).ago)
23
+        .where('last_emailed_at IS NULL OR last_emailed_at < ?', FREQUENCY.ago)
24 24
   end
25 25
 end

+ 1
- 13
spec/mailers/notification_mailer_spec.rb View File

@@ -126,19 +126,7 @@ RSpec.describe NotificationMailer, type: :mailer do
126 126
       end
127 127
     end
128 128
 
129
-    it 'includes activities since the date specified by :since option' do
130
-      receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z')
131
-      mail = NotificationMailer.digest(receiver.account, since: Time.parse('2000-01-01T00:00:00Z'))
132
-      expect(mail.body.encoded).to include 'Jan 01, 2000, 00:00'
133
-    end
134
-
135
-    it 'includes activities since the receiver was last emailed if :since option is unavailable' do
136
-      receiver.update!(last_emailed_at: '2000-02-01T00:00:00Z', current_sign_in_at: '2000-03-01T00:00:00Z')
137
-      mail = NotificationMailer.digest(receiver.account)
138
-      expect(mail.body.encoded).to include 'Feb 01, 2000, 00:00'
139
-    end
140
-
141
-    it 'includes activities since the receiver last signed in if :since option and the last emailed date are unavailable' do
129
+    it 'includes activities since the receiver last signed in' do
142 130
       receiver.update!(last_emailed_at: nil, current_sign_in_at: '2000-03-01T00:00:00Z')
143 131
       mail = NotificationMailer.digest(receiver.account)
144 132
       expect(mail.body.encoded).to include 'Mar 01, 2000, 00:00'

Loading…
Cancel
Save