Add grouped_types
parameter to allow clients to restrict which notifications types get grouped (#31594)
This commit is contained in:
parent
662f87dbe9
commit
ad0a28a8bf
5 changed files with 90 additions and 19 deletions
|
@ -42,7 +42,7 @@ class Api::V2Alpha::NotificationsController < Api::BaseController
|
|||
limit = limit_param(DEFAULT_NOTIFICATIONS_COUNT_LIMIT, MAX_NOTIFICATIONS_COUNT_LIMIT)
|
||||
|
||||
with_read_replica do
|
||||
render json: { count: browserable_account_notifications.paginate_groups_by_min_id(limit, min_id: notification_marker&.last_read_id).count }
|
||||
render json: { count: browserable_account_notifications.paginate_groups_by_min_id(limit, min_id: notification_marker&.last_read_id, grouped_types: params[:grouped_types]).count }
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -68,7 +68,7 @@ class Api::V2Alpha::NotificationsController < Api::BaseController
|
|||
MastodonOTELTracer.in_span('Api::V2Alpha::NotificationsController#load_notifications') do
|
||||
notifications = browserable_account_notifications.includes(from_account: [:account_stat, :user]).to_a_grouped_paginated_by_id(
|
||||
limit_param(DEFAULT_NOTIFICATIONS_LIMIT),
|
||||
params_slice(:max_id, :since_id, :min_id)
|
||||
params.slice(:max_id, :since_id, :min_id, :grouped_types).permit(:max_id, :since_id, :min_id, grouped_types: [])
|
||||
)
|
||||
|
||||
Notification.preload_cache_collection_target_statuses(notifications) do |target_statuses|
|
||||
|
@ -92,7 +92,7 @@ class Api::V2Alpha::NotificationsController < Api::BaseController
|
|||
|
||||
def load_grouped_notifications
|
||||
MastodonOTELTracer.in_span('Api::V2Alpha::NotificationsController#load_grouped_notifications') do
|
||||
@notifications.map { |notification| NotificationGroup.from_notification(notification, max_id: @group_metadata.dig(notification.group_key, :max_id)) }
|
||||
@notifications.map { |notification| NotificationGroup.from_notification(notification, max_id: @group_metadata.dig(notification.group_key, :max_id), grouped_types: params[:grouped_types]) }
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -125,11 +125,11 @@ class Api::V2Alpha::NotificationsController < Api::BaseController
|
|||
end
|
||||
|
||||
def browserable_params
|
||||
params.permit(:include_filtered, types: [], exclude_types: [])
|
||||
params.slice(:include_filtered, :types, :exclude_types, :grouped_types).permit(:include_filtered, types: [], exclude_types: [], grouped_types: [])
|
||||
end
|
||||
|
||||
def pagination_params(core_params)
|
||||
params.slice(:limit, :types, :exclude_types, :include_filtered).permit(:limit, :include_filtered, types: [], exclude_types: []).merge(core_params)
|
||||
params.slice(:limit, :include_filtered, :types, :exclude_types, :grouped_types).permit(:limit, :include_filtered, types: [], exclude_types: [], grouped_types: []).merge(core_params)
|
||||
end
|
||||
|
||||
def expand_accounts_param
|
||||
|
|
|
@ -30,6 +30,8 @@ class Notification < ApplicationRecord
|
|||
'Poll' => :poll,
|
||||
}.freeze
|
||||
|
||||
GROUPABLE_NOTIFICATION_TYPES = %i(favourite reblog).freeze
|
||||
|
||||
# Please update app/javascript/api_types/notification.ts if you change this
|
||||
PROPERTIES = {
|
||||
mention: {
|
||||
|
@ -138,17 +140,40 @@ class Notification < ApplicationRecord
|
|||
end
|
||||
end
|
||||
|
||||
def paginate_groups(limit, pagination_order)
|
||||
def paginate_groups(limit, pagination_order, grouped_types: nil)
|
||||
raise ArgumentError unless %i(asc desc).include?(pagination_order)
|
||||
|
||||
query = reorder(id: pagination_order)
|
||||
|
||||
# Ideally `:types` would be a bind rather than part of the SQL itself, but that does not
|
||||
# seem to be possible to do with Rails, considering that the expression would occur in
|
||||
# multiple places, including in a `select`
|
||||
group_key_sql = begin
|
||||
if grouped_types.present?
|
||||
# Normalize `grouped_types` so the number of different SQL query shapes remains small, and
|
||||
# the queries can be analyzed in monitoring/telemetry tools
|
||||
grouped_types = (grouped_types.map(&:to_sym) & GROUPABLE_NOTIFICATION_TYPES).sort
|
||||
|
||||
sanitize_sql_array([<<~SQL.squish, { types: grouped_types }])
|
||||
COALESCE(
|
||||
CASE
|
||||
WHEN notifications.type IN (:types) THEN notifications.group_key
|
||||
ELSE NULL
|
||||
END,
|
||||
'ungrouped-' || notifications.id
|
||||
)
|
||||
SQL
|
||||
else
|
||||
"COALESCE(notifications.group_key, 'ungrouped-' || notifications.id)"
|
||||
end
|
||||
end
|
||||
|
||||
unscoped
|
||||
.with_recursive(
|
||||
grouped_notifications: [
|
||||
# Base case: fetching one notification and annotating it with visited groups
|
||||
query
|
||||
.select('notifications.*', "ARRAY[COALESCE(notifications.group_key, 'ungrouped-' || notifications.id)] AS groups")
|
||||
.select('notifications.*', "ARRAY[#{group_key_sql}] AS groups")
|
||||
.limit(1),
|
||||
# Recursive case, always yielding at most one annotated notification
|
||||
unscoped
|
||||
|
@ -163,12 +188,12 @@ class Notification < ApplicationRecord
|
|||
# Recursive query, using `LATERAL` so we can refer to `wt`
|
||||
query
|
||||
.where(pagination_order == :desc ? 'notifications.id < wt.id' : 'notifications.id > wt.id')
|
||||
.where.not("COALESCE(notifications.group_key, 'ungrouped-' || notifications.id) = ANY(wt.groups)")
|
||||
.where.not("#{group_key_sql} = ANY(wt.groups)")
|
||||
.limit(1)
|
||||
.arel.lateral('notifications'),
|
||||
]
|
||||
)
|
||||
.select('notifications.*', "array_append(wt.groups, COALESCE(notifications.group_key, 'ungrouped-' || notifications.id))"),
|
||||
.select('notifications.*', "array_append(wt.groups, #{group_key_sql}) AS groups"),
|
||||
]
|
||||
)
|
||||
.from('grouped_notifications AS notifications')
|
||||
|
@ -178,28 +203,28 @@ class Notification < ApplicationRecord
|
|||
|
||||
# This returns notifications from the request page, but with at most one notification per group.
|
||||
# Notifications that have no `group_key` each count as a separate group.
|
||||
def paginate_groups_by_max_id(limit, max_id: nil, since_id: nil)
|
||||
def paginate_groups_by_max_id(limit, max_id: nil, since_id: nil, grouped_types: nil)
|
||||
query = reorder(id: :desc)
|
||||
query = query.where(id: ...(max_id.to_i)) if max_id.present?
|
||||
query = query.where(id: (since_id.to_i + 1)...) if since_id.present?
|
||||
query.paginate_groups(limit, :desc)
|
||||
query.paginate_groups(limit, :desc, grouped_types: grouped_types)
|
||||
end
|
||||
|
||||
# Differs from :paginate_groups_by_max_id in that it gives the results immediately following min_id,
|
||||
# whereas since_id gives the items with largest id, but with since_id as a cutoff.
|
||||
# Results will be in ascending order by id.
|
||||
def paginate_groups_by_min_id(limit, max_id: nil, min_id: nil)
|
||||
def paginate_groups_by_min_id(limit, max_id: nil, min_id: nil, grouped_types: nil)
|
||||
query = reorder(id: :asc)
|
||||
query = query.where(id: (min_id.to_i + 1)...) if min_id.present?
|
||||
query = query.where(id: ...(max_id.to_i)) if max_id.present?
|
||||
query.paginate_groups(limit, :asc)
|
||||
query.paginate_groups(limit, :asc, grouped_types: grouped_types)
|
||||
end
|
||||
|
||||
def to_a_grouped_paginated_by_id(limit, options = {})
|
||||
if options[:min_id].present?
|
||||
paginate_groups_by_min_id(limit, min_id: options[:min_id], max_id: options[:max_id]).reverse
|
||||
paginate_groups_by_min_id(limit, min_id: options[:min_id], max_id: options[:max_id], grouped_types: options[:grouped_types]).reverse
|
||||
else
|
||||
paginate_groups_by_max_id(limit, max_id: options[:max_id], since_id: options[:since_id]).to_a
|
||||
paginate_groups_by_max_id(limit, max_id: options[:max_id], since_id: options[:since_id], grouped_types: options[:grouped_types]).to_a
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -6,8 +6,11 @@ class NotificationGroup < ActiveModelSerializers::Model
|
|||
# Try to keep this consistent with `app/javascript/mastodon/models/notification_group.ts`
|
||||
SAMPLE_ACCOUNTS_SIZE = 8
|
||||
|
||||
def self.from_notification(notification, max_id: nil)
|
||||
if notification.group_key.present?
|
||||
def self.from_notification(notification, max_id: nil, grouped_types: nil)
|
||||
grouped_types = grouped_types.presence&.map(&:to_sym) || Notification::GROUPABLE_NOTIFICATION_TYPES
|
||||
groupable = notification.group_key.present? && grouped_types.include?(notification.type)
|
||||
|
||||
if groupable
|
||||
# TODO: caching, and, if caching, preloading
|
||||
scope = notification.account.notifications.where(group_key: notification.group_key)
|
||||
scope = scope.where(id: ..max_id) if max_id.present?
|
||||
|
@ -25,7 +28,7 @@ class NotificationGroup < ActiveModelSerializers::Model
|
|||
|
||||
NotificationGroup.new(
|
||||
notification: notification,
|
||||
group_key: notification.group_key || "ungrouped-#{notification.id}",
|
||||
group_key: groupable ? notification.group_key : "ungrouped-#{notification.id}",
|
||||
sample_accounts: sample_accounts,
|
||||
notifications_count: notifications_count,
|
||||
most_recent_notification_id: most_recent_id
|
||||
|
|
|
@ -237,7 +237,7 @@ class NotifyService < BaseService
|
|||
private
|
||||
|
||||
def notification_group_key
|
||||
return nil if @notification.filtered || %i(favourite reblog).exclude?(@notification.type)
|
||||
return nil if @notification.filtered || Notification::GROUPABLE_NOTIFICATION_TYPES.exclude?(@notification.type)
|
||||
|
||||
type_prefix = "#{@notification.type}-#{@notification.target_status.id}"
|
||||
redis_key = "notif-group/#{@recipient.id}/#{type_prefix}"
|
||||
|
|
|
@ -35,6 +35,17 @@ RSpec.describe 'Notifications' do
|
|||
end
|
||||
end
|
||||
|
||||
context 'with grouped_types parameter' do
|
||||
let(:params) { { grouped_types: %w(reblog) } }
|
||||
|
||||
it 'returns expected notifications count' do
|
||||
subject
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(body_as_json[:count]).to eq 5
|
||||
end
|
||||
end
|
||||
|
||||
context 'with a read marker' do
|
||||
before do
|
||||
id = user.account.notifications.browserable.order(id: :desc).offset(2).first.id
|
||||
|
@ -114,6 +125,38 @@ RSpec.describe 'Notifications' do
|
|||
end
|
||||
end
|
||||
|
||||
context 'with grouped_types param' do
|
||||
let(:params) { { grouped_types: %w(reblog) } }
|
||||
|
||||
it 'returns everything, but does not group favourites' do
|
||||
subject
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(body_as_json[:notification_groups]).to contain_exactly(
|
||||
a_hash_including(
|
||||
type: 'reblog',
|
||||
sample_account_ids: [bob.account_id.to_s]
|
||||
),
|
||||
a_hash_including(
|
||||
type: 'mention',
|
||||
sample_account_ids: [bob.account_id.to_s]
|
||||
),
|
||||
a_hash_including(
|
||||
type: 'favourite',
|
||||
sample_account_ids: [bob.account_id.to_s]
|
||||
),
|
||||
a_hash_including(
|
||||
type: 'favourite',
|
||||
sample_account_ids: [tom.account_id.to_s]
|
||||
),
|
||||
a_hash_including(
|
||||
type: 'follow',
|
||||
sample_account_ids: [bob.account_id.to_s]
|
||||
)
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
context 'with exclude_types param' do
|
||||
let(:params) { { exclude_types: %w(mention) } }
|
||||
|
||||
|
|
Loading…
Reference in a new issue