FIX: Don't publish notifications to MessageBus for inactive users (#15035)

We are pushing /notification-alert/#{user_id} and /notification/#{user_id}
messages to MessageBus from both PostAlerter and User#publish_notification_state.
This can cause memory issues on large sites with many users. This commit
stems the bleeding by only sending these alert messages if the user
in question has been seen in the last 30 days, which eliminates a large
chunk of users on some sites.
This commit is contained in:
Martin Brennan 2021-11-22 13:38:49 +10:00 committed by GitHub
parent 9015183942
commit 9f8ee8f137
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 58 additions and 3 deletions

View File

@ -590,6 +590,8 @@ class User < ActiveRecord::Base
end end
def publish_notifications_state def publish_notifications_state
return if !self.allow_live_notifications?
# publish last notification json with the message so we can apply an update # publish last notification json with the message so we can apply an update
notification = notifications.visible.order('notifications.created_at desc').first notification = notifications.visible.order('notifications.created_at desc').first
json = NotificationSerializer.new(notification).as_json if notification json = NotificationSerializer.new(notification).as_json if notification
@ -693,6 +695,10 @@ class User < ActiveRecord::Base
last_seen_at.present? last_seen_at.present?
end end
def seen_since?(datetime)
seen_before? && last_seen_at >= datetime
end
def create_visit_record!(date, opts = {}) def create_visit_record!(date, opts = {})
user_stat.update_column(:days_visited, user_stat.days_visited + 1) user_stat.update_column(:days_visited, user_stat.days_visited + 1)
user_visits.create!(visited_at: date, posts_read: opts[:posts_read] || 0, mobile: opts[:mobile] || false) user_visits.create!(visited_at: date, posts_read: opts[:posts_read] || 0, mobile: opts[:mobile] || false)
@ -1438,6 +1444,10 @@ class User < ActiveRecord::Base
ShelvedNotification.joins(:notification).where("notifications.user_id = ?", self.id) ShelvedNotification.joins(:notification).where("notifications.user_id = ?", self.id)
end end
def allow_live_notifications?
seen_since?(30.days.ago)
end
protected protected
def badge_grant def badge_grant

View File

@ -28,7 +28,11 @@ class PostAlerter
} }
DiscourseEvent.trigger(:pre_notification_alert, user, payload) DiscourseEvent.trigger(:pre_notification_alert, user, payload)
MessageBus.publish("/notification-alert/#{user.id}", payload, user_ids: [user.id])
if user.allow_live_notifications?
MessageBus.publish("/notification-alert/#{user.id}", payload, user_ids: [user.id])
end
push_notification(user, payload) push_notification(user, payload)
DiscourseEvent.trigger(:post_notification_alert, user, payload) DiscourseEvent.trigger(:post_notification_alert, user, payload)
end end

View File

@ -3,7 +3,7 @@
require 'rails_helper' require 'rails_helper'
describe User do describe User do
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user, last_seen_at: 1.day.ago) }
def user_error_message(*keys) def user_error_message(*keys)
I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join('.')}") I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join('.')}")
@ -1921,6 +1921,18 @@ describe User do
expect(message.data[:unread_private_messages]).to eq(2) expect(message.data[:unread_private_messages]).to eq(2)
expect(message.data[:unread_high_priority_notifications]).to eq(2) expect(message.data[:unread_high_priority_notifications]).to eq(2)
end end
it "does not publish to the /notification channel for users who have not been seen in > 30 days" do
notification = Fabricate(:notification, user: user)
notification2 = Fabricate(:notification, user: user, read: true)
user.update(last_seen_at: 31.days.ago)
message = MessageBus.track_publish("/notification/#{user.id}") do
user.publish_notifications_state
end.first
expect(message).to eq(nil)
end
end end
describe "silenced?" do describe "silenced?" do

View File

@ -848,7 +848,7 @@ describe PostAlerter do
end end
describe "create_notification_alert" do describe "create_notification_alert" do
it "does not nothing for suspended users" do it "does nothing for suspended users" do
evil_trout.update_columns(suspended_till: 1.year.from_now) evil_trout.update_columns(suspended_till: 1.year.from_now)
post = Fabricate(:post) post = Fabricate(:post)
@ -869,6 +869,35 @@ describe PostAlerter do
expect(messages.size).to eq(0) expect(messages.size).to eq(0)
expect(Jobs::PushNotification.jobs.size).to eq(0) expect(Jobs::PushNotification.jobs.size).to eq(0)
end end
it "does not publish to MessageBus /notification-alert if the user has not been seen for > 30 days, but still sends a push notification" do
evil_trout.update_columns(last_seen_at: 31.days.ago)
post = Fabricate(:post)
SiteSetting.allowed_user_api_push_urls = "https://site2.com/push"
UserApiKey.create!(user_id: evil_trout.id,
client_id: "xxx#1",
application_name: "iPhone1",
scopes: ['notifications'].map { |name| UserApiKeyScope.new(name: name) },
push_url: "https://site2.com/push")
events = nil
messages = MessageBus.track_publish do
events = DiscourseEvent.track_events do
PostAlerter.create_notification_alert(
user: evil_trout,
post: post,
notification_type: Notification.types[:custom],
excerpt: "excerpt",
username: "username"
)
end
end
expect(events.size).to eq(2)
expect(messages.size).to eq(0)
expect(Jobs::PushNotification.jobs.size).to eq(1)
end
end end
describe "watching_first_post" do describe "watching_first_post" do