From ad0a28a8bfb8b527eae11cdc29829d87b07db53c Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 29 Aug 2024 14:39:07 +0200 Subject: [PATCH] Add `grouped_types` parameter to allow clients to restrict which notifications types get grouped (#31594) --- .../api/v2_alpha/notifications_controller.rb | 10 ++--- app/models/notification.rb | 45 ++++++++++++++----- app/models/notification_group.rb | 9 ++-- app/services/notify_service.rb | 2 +- .../api/v2_alpha/notifications_spec.rb | 43 ++++++++++++++++++ 5 files changed, 90 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/v2_alpha/notifications_controller.rb b/app/controllers/api/v2_alpha/notifications_controller.rb index d0205ad6af..a9f4ac02a0 100644 --- a/app/controllers/api/v2_alpha/notifications_controller.rb +++ b/app/controllers/api/v2_alpha/notifications_controller.rb @@ -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 diff --git a/app/models/notification.rb b/app/models/notification.rb index ae7c782bed..44a43d2ece 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -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 diff --git a/app/models/notification_group.rb b/app/models/notification_group.rb index 223945f07b..12294f24ec 100644 --- a/app/models/notification_group.rb +++ b/app/models/notification_group.rb @@ -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 diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index 695f4153c6..97eee05487 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -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}" diff --git a/spec/requests/api/v2_alpha/notifications_spec.rb b/spec/requests/api/v2_alpha/notifications_spec.rb index a613158aa5..9d9eb34ebd 100644 --- a/spec/requests/api/v2_alpha/notifications_spec.rb +++ b/spec/requests/api/v2_alpha/notifications_spec.rb @@ -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) } }