FIX: send activity summaries based on "last seen" #27035
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
i18n
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
ZogStriP
force-pushed
the
fix-activity-summaries
branch
from
05e1617
to
a72aa53
Compare
ZogStriP
commented
May 16, 2024
| # 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.
user.user_option.mailing_list_mode is already checking for SiteSetting.disable_mailing_list_mode so I removed that check here.
ZogStriP
commented
May 16, 2024
| 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.
the nil, nil is not required
ZogStriP
commented
May 16, 2024
| 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) |
ZogStriP
commented
May 16, 2024
| .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.
Ensures the digest_after_minutes is not set the 0 which means "never".
ZogStriP
commented
May 16, 2024
| @@ -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.
Use digest_attempted_at instead of last_emailed_at as the "threshold" date for the digest.
ZogStriP
commented
May 16, 2024
| t.first_post.cooked, | ||
| t.first_post, | ||
| ) if t.first_post.present? | ||
| @popular_topics.each do |t| |
ZogStriP
commented
May 16, 2024
| 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.
Extracted the if to its own line so it's easier to read.
ZogStriP
commented
May 16, 2024
| 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 |
ZogStriP
commented
May 16, 2024
| .not_suspended | ||
| .where("created_at > ?", date) | ||
| .count | ||
| .tap { Discourse.redis.setex(key, 1.day, _1) } |
ZogStriP
commented
May 16, 2024
| @@ -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.
Removed "your last visit" since it might not always been based on "last visit"
jjaffeux
reviewed
May 22, 2024
| self.email_digests = true | ||
| end | ||
|
|
||
| self.digest_after_minutes ||= SiteSetting.default_email_digest_frequency.to_i |
There was a problem hiding this comment.
what was the reason for the ||= before?
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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