From 438dac99d6afcb5873f5aafee29bf9de2cabb864 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 6 Aug 2024 14:09:35 +0200 Subject: [PATCH] Add option to request partial accounts in grouped notifications API (#31299) --- .../api/v2_alpha/notifications_controller.rb | 25 +++++++++++----- .../grouped_notifications_presenter.rb | 27 +++++++++++++++-- .../dedup_notification_group_serializer.rb | 15 ++++++++++ .../api/v2_alpha/notifications_spec.rb | 30 +++++++++++++++++++ 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v2_alpha/notifications_controller.rb b/app/controllers/api/v2_alpha/notifications_controller.rb index 837499e89..d0205ad6a 100644 --- a/app/controllers/api/v2_alpha/notifications_controller.rb +++ b/app/controllers/api/v2_alpha/notifications_controller.rb @@ -16,10 +16,10 @@ class Api::V2Alpha::NotificationsController < Api::BaseController @group_metadata = load_group_metadata @grouped_notifications = load_grouped_notifications @relationships = StatusRelationshipsPresenter.new(target_statuses_from_notifications, current_user&.account_id) - @sample_accounts = @grouped_notifications.flat_map(&:sample_accounts) + @presenter = GroupedNotificationsPresenter.new(@grouped_notifications, expand_accounts: expand_accounts_param) # Preload associations to avoid N+1s - ActiveRecord::Associations::Preloader.new(records: @sample_accounts, associations: [:account_stat, { user: :role }]).call + ActiveRecord::Associations::Preloader.new(records: @presenter.accounts, associations: [:account_stat, { user: :role }]).call end MastodonOTELTracer.in_span('Api::V2Alpha::NotificationsController#index rendering') do |span| @@ -27,14 +27,14 @@ class Api::V2Alpha::NotificationsController < Api::BaseController span.add_attributes( 'app.notification_grouping.count' => @grouped_notifications.size, - 'app.notification_grouping.sample_account.count' => @sample_accounts.size, - 'app.notification_grouping.sample_account.unique_count' => @sample_accounts.pluck(:id).uniq.size, + 'app.notification_grouping.account.count' => @presenter.accounts.size, + 'app.notification_grouping.partial_account.count' => @presenter.partial_accounts.size, 'app.notification_grouping.status.count' => statuses.size, - 'app.notification_grouping.status.unique_count' => statuses.uniq.size + 'app.notification_grouping.status.unique_count' => statuses.uniq.size, + 'app.notification_grouping.expand_accounts_param' => expand_accounts_param ) - presenter = GroupedNotificationsPresenter.new(@grouped_notifications) - render json: presenter, serializer: REST::DedupNotificationGroupSerializer, relationships: @relationships, group_metadata: @group_metadata + render json: @presenter, serializer: REST::DedupNotificationGroupSerializer, relationships: @relationships, group_metadata: @group_metadata, expand_accounts: expand_accounts_param end end @@ -131,4 +131,15 @@ class Api::V2Alpha::NotificationsController < Api::BaseController def pagination_params(core_params) params.slice(:limit, :types, :exclude_types, :include_filtered).permit(:limit, :include_filtered, types: [], exclude_types: []).merge(core_params) end + + def expand_accounts_param + case params[:expand_accounts] + when nil, 'full' + 'full' + when 'partial_avatars' + 'partial_avatars' + else + raise Mastodon::InvalidParameterError, "Invalid value for 'expand_accounts': '#{params[:expand_accounts]}', allowed values are 'full' and 'partial_avatars'" + end + end end diff --git a/app/presenters/grouped_notifications_presenter.rb b/app/presenters/grouped_notifications_presenter.rb index f01acfd41..9ced1d0c7 100644 --- a/app/presenters/grouped_notifications_presenter.rb +++ b/app/presenters/grouped_notifications_presenter.rb @@ -1,10 +1,11 @@ # frozen_string_literal: true class GroupedNotificationsPresenter < ActiveModelSerializers::Model - def initialize(grouped_notifications) + def initialize(grouped_notifications, options = {}) super() @grouped_notifications = grouped_notifications + @options = options end def notification_groups @@ -16,6 +17,28 @@ class GroupedNotificationsPresenter < ActiveModelSerializers::Model end def accounts - @grouped_notifications.flat_map(&:sample_accounts).uniq(&:id) + @accounts ||= begin + if partial_avatars? + @grouped_notifications.map { |group| group.sample_accounts.first }.uniq(&:id) + else + @grouped_notifications.flat_map(&:sample_accounts).uniq(&:id) + end + end + end + + def partial_accounts + @partial_accounts ||= begin + if partial_avatars? + @grouped_notifications.flat_map { |group| group.sample_accounts[1...] }.uniq(&:id).filter { |account| accounts.exclude?(account) } + else + [] + end + end + end + + private + + def partial_avatars? + @options[:expand_accounts] == 'partial_avatars' end end diff --git a/app/serializers/rest/dedup_notification_group_serializer.rb b/app/serializers/rest/dedup_notification_group_serializer.rb index fd0c6b3c4..4f02505e2 100644 --- a/app/serializers/rest/dedup_notification_group_serializer.rb +++ b/app/serializers/rest/dedup_notification_group_serializer.rb @@ -1,7 +1,22 @@ # frozen_string_literal: true class REST::DedupNotificationGroupSerializer < ActiveModel::Serializer + class PartialAccountSerializer < REST::AccountSerializer + # This is a hack to reset ActiveModel::Serializer internals and only expose the attributes + # we care about. + self._attributes_data = {} + self._reflections = [] + self._links = [] + + attributes :id, :acct, :locked, :bot, :url, :avatar, :avatar_static + end + has_many :accounts, serializer: REST::AccountSerializer + has_many :partial_accounts, serializer: PartialAccountSerializer, if: :return_partial_accounts? has_many :statuses, serializer: REST::StatusSerializer has_many :notification_groups, serializer: REST::NotificationGroupSerializer + + def return_partial_accounts? + instance_options[:expand_accounts] == 'partial_avatars' + end end diff --git a/spec/requests/api/v2_alpha/notifications_spec.rb b/spec/requests/api/v2_alpha/notifications_spec.rb index fc1daef43..94bc7b0ee 100644 --- a/spec/requests/api/v2_alpha/notifications_spec.rb +++ b/spec/requests/api/v2_alpha/notifications_spec.rb @@ -160,6 +160,36 @@ RSpec.describe 'Notifications' do end end + context 'when requesting stripped-down accounts' do + let(:params) { { expand_accounts: 'partial_avatars' } } + + let(:recent_account) { Fabricate(:account) } + + before do + FavouriteService.new.call(recent_account, user.account.statuses.first) + end + + it 'returns an account in "partial_accounts", with the expected keys', :aggregate_failures do + subject + + expect(response).to have_http_status(200) + expect(body_as_json[:partial_accounts].size).to be > 0 + expect(body_as_json[:partial_accounts][0].keys).to contain_exactly(:acct, :avatar, :avatar_static, :bot, :id, :locked, :url) + expect(body_as_json[:partial_accounts].pluck(:id)).to_not include(recent_account.id.to_s) + expect(body_as_json[:accounts].pluck(:id)).to include(recent_account.id.to_s) + end + end + + context 'when passing an invalid value for "expand_accounts"' do + let(:params) { { expand_accounts: 'unknown_foobar' } } + + it 'returns http bad request' do + subject + + expect(response).to have_http_status(400) + end + end + def body_json_types body_as_json[:notification_groups].pluck(:type) end