mirror of https://github.com/mastodon/mastodon
Add experimental server-side notification grouping (#29889)
parent
db49b0e5e9
commit
974335e414
@ -0,0 +1,91 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class Api::V2Alpha::NotificationsController < Api::BaseController
|
||||||
|
before_action -> { doorkeeper_authorize! :read, :'read:notifications' }, except: [:clear, :dismiss]
|
||||||
|
before_action -> { doorkeeper_authorize! :write, :'write:notifications' }, only: [:clear, :dismiss]
|
||||||
|
before_action :require_user!
|
||||||
|
after_action :insert_pagination_headers, only: :index
|
||||||
|
|
||||||
|
DEFAULT_NOTIFICATIONS_LIMIT = 40
|
||||||
|
|
||||||
|
def index
|
||||||
|
with_read_replica do
|
||||||
|
@notifications = load_notifications
|
||||||
|
@group_metadata = load_group_metadata
|
||||||
|
@relationships = StatusRelationshipsPresenter.new(target_statuses_from_notifications, current_user&.account_id)
|
||||||
|
end
|
||||||
|
|
||||||
|
render json: @notifications.map { |notification| NotificationGroup.from_notification(notification) }, each_serializer: REST::NotificationGroupSerializer, relationships: @relationships, group_metadata: @group_metadata
|
||||||
|
end
|
||||||
|
|
||||||
|
def show
|
||||||
|
@notification = current_account.notifications.without_suspended.find_by!(group_key: params[:id])
|
||||||
|
render json: NotificationGroup.from_notification(@notification), serializer: REST::NotificationGroupSerializer
|
||||||
|
end
|
||||||
|
|
||||||
|
def clear
|
||||||
|
current_account.notifications.delete_all
|
||||||
|
render_empty
|
||||||
|
end
|
||||||
|
|
||||||
|
def dismiss
|
||||||
|
current_account.notifications.where(group_key: params[:id]).destroy_all
|
||||||
|
render_empty
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def load_notifications
|
||||||
|
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)
|
||||||
|
)
|
||||||
|
|
||||||
|
Notification.preload_cache_collection_target_statuses(notifications) do |target_statuses|
|
||||||
|
preload_collection(target_statuses, Status)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def load_group_metadata
|
||||||
|
return {} if @notifications.empty?
|
||||||
|
|
||||||
|
browserable_account_notifications
|
||||||
|
.where(group_key: @notifications.filter_map(&:group_key))
|
||||||
|
.where(id: (@notifications.last.id)..(@notifications.first.id))
|
||||||
|
.group(:group_key)
|
||||||
|
.pluck(:group_key, 'min(notifications.id) as min_id', 'max(notifications.id) as max_id', 'max(notifications.created_at) as latest_notification_at')
|
||||||
|
.to_h { |group_key, min_id, max_id, latest_notification_at| [group_key, { min_id: min_id, max_id: max_id, latest_notification_at: latest_notification_at }] }
|
||||||
|
end
|
||||||
|
|
||||||
|
def browserable_account_notifications
|
||||||
|
current_account.notifications.without_suspended.browserable(
|
||||||
|
types: Array(browserable_params[:types]),
|
||||||
|
exclude_types: Array(browserable_params[:exclude_types]),
|
||||||
|
include_filtered: truthy_param?(:include_filtered)
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
def target_statuses_from_notifications
|
||||||
|
@notifications.filter_map(&:target_status)
|
||||||
|
end
|
||||||
|
|
||||||
|
def next_path
|
||||||
|
api_v2_alpha_notifications_url pagination_params(max_id: pagination_max_id) unless @notifications.empty?
|
||||||
|
end
|
||||||
|
|
||||||
|
def prev_path
|
||||||
|
api_v2_alpha_notifications_url pagination_params(min_id: pagination_since_id) unless @notifications.empty?
|
||||||
|
end
|
||||||
|
|
||||||
|
def pagination_collection
|
||||||
|
@notifications
|
||||||
|
end
|
||||||
|
|
||||||
|
def browserable_params
|
||||||
|
params.permit(:include_filtered, types: [], exclude_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)
|
||||||
|
end
|
||||||
|
end
|
@ -0,0 +1,29 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class NotificationGroup < ActiveModelSerializers::Model
|
||||||
|
attributes :group_key, :sample_accounts, :notifications_count, :notification
|
||||||
|
|
||||||
|
def self.from_notification(notification)
|
||||||
|
if notification.group_key.present?
|
||||||
|
# TODO: caching and preloading
|
||||||
|
sample_accounts = notification.account.notifications.where(group_key: notification.group_key).order(id: :desc).limit(3).map(&:from_account)
|
||||||
|
notifications_count = notification.account.notifications.where(group_key: notification.group_key).count
|
||||||
|
else
|
||||||
|
sample_accounts = [notification.from_account]
|
||||||
|
notifications_count = 1
|
||||||
|
end
|
||||||
|
|
||||||
|
NotificationGroup.new(
|
||||||
|
notification: notification,
|
||||||
|
group_key: notification.group_key || "ungrouped-#{notification.id}",
|
||||||
|
sample_accounts: sample_accounts,
|
||||||
|
notifications_count: notifications_count
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
delegate :type,
|
||||||
|
:target_status,
|
||||||
|
:report,
|
||||||
|
:account_relationship_severance_event,
|
||||||
|
to: :notification, prefix: false
|
||||||
|
end
|
@ -0,0 +1,45 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class REST::NotificationGroupSerializer < ActiveModel::Serializer
|
||||||
|
attributes :group_key, :notifications_count, :type
|
||||||
|
|
||||||
|
attribute :page_min_id, if: :paginated?
|
||||||
|
attribute :page_max_id, if: :paginated?
|
||||||
|
attribute :latest_page_notification_at, if: :paginated?
|
||||||
|
|
||||||
|
has_many :sample_accounts, serializer: REST::AccountSerializer
|
||||||
|
belongs_to :target_status, key: :status, if: :status_type?, serializer: REST::StatusSerializer
|
||||||
|
belongs_to :report, if: :report_type?, serializer: REST::ReportSerializer
|
||||||
|
belongs_to :account_relationship_severance_event, key: :event, if: :relationship_severance_event?, serializer: REST::AccountRelationshipSeveranceEventSerializer
|
||||||
|
|
||||||
|
def status_type?
|
||||||
|
[:favourite, :reblog, :status, :mention, :poll, :update].include?(object.type)
|
||||||
|
end
|
||||||
|
|
||||||
|
def report_type?
|
||||||
|
object.type == :'admin.report'
|
||||||
|
end
|
||||||
|
|
||||||
|
def relationship_severance_event?
|
||||||
|
object.type == :severed_relationships
|
||||||
|
end
|
||||||
|
|
||||||
|
def page_min_id
|
||||||
|
range = instance_options[:group_metadata][object.group_key]
|
||||||
|
range.present? ? range[:min_id].to_s : object.notification.id.to_s
|
||||||
|
end
|
||||||
|
|
||||||
|
def page_max_id
|
||||||
|
range = instance_options[:group_metadata][object.group_key]
|
||||||
|
range.present? ? range[:max_id].to_s : object.notification.id.to_s
|
||||||
|
end
|
||||||
|
|
||||||
|
def latest_page_notification_at
|
||||||
|
range = instance_options[:group_metadata][object.group_key]
|
||||||
|
range.present? ? range[:latest_notification_at] : object.notification.created_at
|
||||||
|
end
|
||||||
|
|
||||||
|
def paginated?
|
||||||
|
instance_options[:group_metadata].present?
|
||||||
|
end
|
||||||
|
end
|
@ -0,0 +1,7 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class AddGroupKeyToNotifications < ActiveRecord::Migration[7.1]
|
||||||
|
def change
|
||||||
|
add_column :notifications, :group_key, :string
|
||||||
|
end
|
||||||
|
end
|
@ -0,0 +1,9 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class AddIndexNotificationsOnAccountIdAndGroupKey < ActiveRecord::Migration[7.1]
|
||||||
|
disable_ddl_transaction!
|
||||||
|
|
||||||
|
def change
|
||||||
|
add_index :notifications, [:account_id, :group_key], algorithm: :concurrently, where: 'group_key IS NOT NULL'
|
||||||
|
end
|
||||||
|
end
|
@ -0,0 +1,65 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
# Add support for writing recursive CTEs in ActiveRecord
|
||||||
|
|
||||||
|
# Initially from Lorin Thwaits (https://github.com/lorint) as per comment:
|
||||||
|
# https://github.com/vlado/activerecord-cte/issues/16#issuecomment-1433043310
|
||||||
|
|
||||||
|
# Modified from the above code to change the signature to
|
||||||
|
# `with_recursive(hash)` and extending CTE hash values to also includes arrays
|
||||||
|
# of values that get turned into UNION ALL expressions.
|
||||||
|
|
||||||
|
# This implementation has been merged in Rails: https://github.com/rails/rails/pull/51601
|
||||||
|
|
||||||
|
module ActiveRecord
|
||||||
|
module QueryMethodsExtensions
|
||||||
|
def with_recursive(*args)
|
||||||
|
@with_is_recursive = true
|
||||||
|
check_if_method_has_arguments!(__callee__, args)
|
||||||
|
spawn.with_recursive!(*args)
|
||||||
|
end
|
||||||
|
|
||||||
|
# Like #with_recursive but modifies the relation in place.
|
||||||
|
def with_recursive!(*args) # :nodoc:
|
||||||
|
self.with_values += args
|
||||||
|
@with_is_recursive = true
|
||||||
|
self
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def build_with(arel)
|
||||||
|
return if with_values.empty?
|
||||||
|
|
||||||
|
with_statements = with_values.map do |with_value|
|
||||||
|
raise ArgumentError, "Unsupported argument type: #{with_value} #{with_value.class}" unless with_value.is_a?(Hash)
|
||||||
|
|
||||||
|
build_with_value_from_hash(with_value)
|
||||||
|
end
|
||||||
|
|
||||||
|
# Was: arel.with(with_statements)
|
||||||
|
@with_is_recursive ? arel.with(:recursive, with_statements) : arel.with(with_statements)
|
||||||
|
end
|
||||||
|
|
||||||
|
def build_with_value_from_hash(hash)
|
||||||
|
hash.map do |name, value|
|
||||||
|
Arel::Nodes::TableAlias.new(build_with_expression_from_value(value), name)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def build_with_expression_from_value(value)
|
||||||
|
case value
|
||||||
|
when Arel::Nodes::SqlLiteral then Arel::Nodes::Grouping.new(value)
|
||||||
|
when ActiveRecord::Relation then value.arel
|
||||||
|
when Arel::SelectManager then value
|
||||||
|
when Array then value.map { |e| build_with_expression_from_value(e) }.reduce { |result, value| Arel::Nodes::UnionAll.new(result, value) }
|
||||||
|
else
|
||||||
|
raise ArgumentError, "Unsupported argument type: `#{value}` #{value.class}"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
ActiveSupport.on_load(:active_record) do
|
||||||
|
ActiveRecord::QueryMethods.prepend(ActiveRecord::QueryMethodsExtensions)
|
||||||
|
end
|
@ -0,0 +1,51 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
# Fix an issue with `LIMIT` ocurring on the left side of a `UNION` causing syntax errors.
|
||||||
|
# See https://github.com/rails/rails/issues/40181
|
||||||
|
|
||||||
|
# The fix has been merged in ActiveRecord: https://github.com/rails/rails/pull/51549
|
||||||
|
# TODO: drop this when available in ActiveRecord
|
||||||
|
|
||||||
|
# rubocop:disable all -- This is a mostly vendored file
|
||||||
|
|
||||||
|
module Arel
|
||||||
|
module Visitors
|
||||||
|
class ToSql
|
||||||
|
private
|
||||||
|
|
||||||
|
def infix_value_with_paren(o, collector, value, suppress_parens = false)
|
||||||
|
collector << "( " unless suppress_parens
|
||||||
|
collector = if o.left.class == o.class
|
||||||
|
infix_value_with_paren(o.left, collector, value, true)
|
||||||
|
else
|
||||||
|
select_parentheses o.left, collector, false # Changed from `visit o.left, collector`
|
||||||
|
end
|
||||||
|
collector << value
|
||||||
|
collector = if o.right.class == o.class
|
||||||
|
infix_value_with_paren(o.right, collector, value, true)
|
||||||
|
else
|
||||||
|
select_parentheses o.right, collector, false # Changed from `visit o.right, collector`
|
||||||
|
end
|
||||||
|
collector << " )" unless suppress_parens
|
||||||
|
collector
|
||||||
|
end
|
||||||
|
|
||||||
|
def select_parentheses(o, collector, always_wrap_selects = true)
|
||||||
|
if o.is_a?(Nodes::SelectStatement) && (always_wrap_selects || require_parentheses?(o))
|
||||||
|
collector << "("
|
||||||
|
visit o, collector
|
||||||
|
collector << ")"
|
||||||
|
collector
|
||||||
|
else
|
||||||
|
visit o, collector
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def require_parentheses?(o)
|
||||||
|
!o.orders.empty? || o.limit || o.offset
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
# rubocop:enable all
|
@ -0,0 +1,161 @@
|
|||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
require 'rails_helper'
|
||||||
|
|
||||||
|
RSpec.describe 'Notifications' do
|
||||||
|
let(:user) { Fabricate(:user, account_attributes: { username: 'alice' }) }
|
||||||
|
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) }
|
||||||
|
let(:scopes) { 'read:notifications write:notifications' }
|
||||||
|
let(:headers) { { 'Authorization' => "Bearer #{token.token}" } }
|
||||||
|
|
||||||
|
describe 'GET /api/v2_alpha/notifications', :sidekiq_inline do
|
||||||
|
subject do
|
||||||
|
get '/api/v2_alpha/notifications', headers: headers, params: params
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:bob) { Fabricate(:user) }
|
||||||
|
let(:tom) { Fabricate(:user) }
|
||||||
|
let(:params) { {} }
|
||||||
|
|
||||||
|
before do
|
||||||
|
first_status = PostStatusService.new.call(user.account, text: 'Test')
|
||||||
|
ReblogService.new.call(bob.account, first_status)
|
||||||
|
mentioning_status = PostStatusService.new.call(bob.account, text: 'Hello @alice')
|
||||||
|
mentioning_status.mentions.first
|
||||||
|
FavouriteService.new.call(bob.account, first_status)
|
||||||
|
FavouriteService.new.call(tom.account, first_status)
|
||||||
|
FollowService.new.call(bob.account, user.account)
|
||||||
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'forbidden for wrong scope', 'write write:notifications'
|
||||||
|
|
||||||
|
context 'with no options' do
|
||||||
|
it 'returns expected notification types', :aggregate_failures do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
expect(body_json_types).to include('reblog', 'mention', 'favourite', 'follow')
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with exclude_types param' do
|
||||||
|
let(:params) { { exclude_types: %w(mention) } }
|
||||||
|
|
||||||
|
it 'returns everything but excluded type', :aggregate_failures do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
expect(body_as_json.size).to_not eq 0
|
||||||
|
expect(body_json_types.uniq).to_not include 'mention'
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with types param' do
|
||||||
|
let(:params) { { types: %w(mention) } }
|
||||||
|
|
||||||
|
it 'returns only requested type', :aggregate_failures do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
expect(body_json_types.uniq).to eq ['mention']
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'with limit param' do
|
||||||
|
let(:params) { { limit: 3 } }
|
||||||
|
|
||||||
|
it 'returns the requested number of notifications paginated', :aggregate_failures do
|
||||||
|
subject
|
||||||
|
|
||||||
|
notifications = user.account.notifications
|
||||||
|
|
||||||
|
expect(body_as_json.size)
|
||||||
|
.to eq(params[:limit])
|
||||||
|
|
||||||
|
expect(response)
|
||||||
|
.to include_pagination_headers(
|
||||||
|
prev: api_v2_alpha_notifications_url(limit: params[:limit], min_id: notifications.last.id),
|
||||||
|
# TODO: one downside of the current approach is that we return the first ID matching the group,
|
||||||
|
# not the last that has been skipped, so pagination is very likely to give overlap
|
||||||
|
next: api_v2_alpha_notifications_url(limit: params[:limit], max_id: notifications[1].id)
|
||||||
|
)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def body_json_types
|
||||||
|
body_as_json.pluck(:type)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe 'GET /api/v2_alpha/notifications/:id' do
|
||||||
|
subject do
|
||||||
|
get "/api/v2_alpha/notifications/#{notification.group_key}", headers: headers
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:notification) { Fabricate(:notification, account: user.account, group_key: 'foobar') }
|
||||||
|
|
||||||
|
it_behaves_like 'forbidden for wrong scope', 'write write:notifications'
|
||||||
|
|
||||||
|
it 'returns http success' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when notification belongs to someone else' do
|
||||||
|
let(:notification) { Fabricate(:notification, group_key: 'foobar') }
|
||||||
|
|
||||||
|
it 'returns http not found' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(404)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe 'POST /api/v2_alpha/notifications/:id/dismiss' do
|
||||||
|
subject do
|
||||||
|
post "/api/v2_alpha/notifications/#{notification.group_key}/dismiss", headers: headers
|
||||||
|
end
|
||||||
|
|
||||||
|
let!(:notification) { Fabricate(:notification, account: user.account, group_key: 'foobar') }
|
||||||
|
|
||||||
|
it_behaves_like 'forbidden for wrong scope', 'read read:notifications'
|
||||||
|
|
||||||
|
it 'destroys the notification' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when notification belongs to someone else' do
|
||||||
|
let(:notification) { Fabricate(:notification) }
|
||||||
|
|
||||||
|
it 'returns http not found' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(response).to have_http_status(404)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe 'POST /api/v2_alpha/notifications/clear' do
|
||||||
|
subject do
|
||||||
|
post '/api/v2_alpha/notifications/clear', headers: headers
|
||||||
|
end
|
||||||
|
|
||||||
|
before do
|
||||||
|
Fabricate(:notification, account: user.account)
|
||||||
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'forbidden for wrong scope', 'read read:notifications'
|
||||||
|
|
||||||
|
it 'clears notifications for the account' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(user.account.reload.notifications).to be_empty
|
||||||
|
expect(response).to have_http_status(200)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
Loading…
Reference in New Issue