-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: send activity summaries based on "last seen" #27035
Conversation
instead of "last emailed" so that people getting email notifications (from a watched topic for example) also get the activity summaries. Context - https://meta.discourse.org/t/activity-summary-not-sent-if-other-emails-are-sent/293040 Internal Ref - t/125582 Improvement over 9588564
05e1617
to
a72aa53
Compare
# no need to log a reason when the mail was already sent via the mailing list job | ||
return nil, nil | ||
# don't catch notifications for users on daily mailing list mode | ||
if user.user_option.mailing_list_mode && user.user_option.mailing_list_mode_frequency > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user.user_option.mailing_list_mode
is already checking for SiteSetting.disable_mailing_list_mode
so I removed that check here.
if !post&.topic&.private_message? && | ||
NOTIFICATIONS_SENT_BY_MAILING_LIST.include?(email_args[:notification_type]) | ||
# no need to log a reason when the mail was already sent via the mailing list job | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the nil, nil
is not required
end | ||
|
||
unless always_email_regular?(user, type) || always_email_private_message?(user, type) | ||
if (notification && notification.read?) || (post && post.seen?(user)) | ||
if notification&.read? || post&.seen?(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easier to read.
.where( | ||
"COALESCE(last_emailed_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * user_options.digest_after_minutes)", | ||
"COALESCE(user_options.digest_after_minutes, ?) > 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensures the digest_after_minutes
is not set the 0
which means "never".
@@ -224,18 +224,18 @@ def digest(user, opts = {}) | |||
build_summary_for(user) | |||
@unsubscribe_key = UnsubscribeKey.create_key_for(@user, UnsubscribeKey::DIGEST_TYPE) | |||
|
|||
min_date = opts[:since] || user.last_emailed_at || user.last_seen_at || 1.month.ago | |||
@since = opts[:since] || user.last_seen_at || user.user_stat&.digest_attempted_at || 1.month.ago |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use digest_attempted_at
instead of last_emailed_at
as the "threshold" date for the digest.
t.first_post.cooked, | ||
t.first_post, | ||
) if t.first_post.present? | ||
@popular_topics.each do |t| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to .map
t.first_post, | ||
) if t.first_post.present? | ||
@popular_topics.each do |t| | ||
next if t.first_post.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted the if
to its own line so it's easier to read.
new_topics_count = Topic.for_digest(user, min_date).count | ||
new_topics_count = Topic.for_digest(user, @since).count | ||
# We used topics from new users instead, so count should match | ||
new_topics_count = topics_for_digest.size if new_topics_count == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlined the if
.not_suspended | ||
.where("created_at > ?", date) | ||
.count | ||
.tap { Discourse.redis.setex(key, 1.day, _1) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.tap
FTW
@@ -4232,7 +4232,7 @@ en: | |||
If you have any questions, [contact our friendly staff](%{base_url}/about). | |||
digest: | |||
why: "A brief summary of %{site_link} since your last visit on %{last_seen_at}" | |||
why: "A brief summary of %{site_link} since %{since}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed "your last visit" since it might not always been based on "last visit"
self.email_digests = true | ||
end | ||
|
||
self.digest_after_minutes ||= SiteSetting.default_email_digest_frequency.to_i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the reason for the ||= before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
instead of "last emailed" so that people getting email notifications (from a watched topic for example) also get the activity summaries.
Context - https://meta.discourse.org/t/activity-summary-not-sent-if-other-emails-are-sent/293040
Internal Ref - t/125582
Improvement over 9588564