Merge 31e3cac2c9 into 65093c619f

app/controllers/api/v2_alpha/notifications_controller.rb

@@ -0,0 +1,79 @@
+# 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
+      @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
+  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|
+      cache_collection(target_statuses, Status)
+    end
+  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

app/models/notification.rb

@@ -13,6 +13,7 @@
 #  from_account_id :bigint(8)        not null
 #  type            :string
 #  filtered        :boolean          default(FALSE), not null
+#  group_key       :string
 #
 
 class Notification < ApplicationRecord
@@ -136,6 +137,67 @@ class Notification < ApplicationRecord
       end
     end
 
+    # 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)
+      query = reorder(id: :desc)
+      query = query.where(id: ...max_id) if max_id.present?
+      query = query.where(id: (since_id + 1)...) if since_id.present?
+
+      unscoped
+        .with_recursive(
+          grouped_notifications: [
+            query
+              .select('notifications.*', "ARRAY[COALESCE(notifications.group_key, 'ungrouped-' || notifications.id)] groups")
+              .limit(1),
+            query
+              .joins('CROSS JOIN grouped_notifications')
+              .where('notifications.id < grouped_notifications.id')
+              .where.not("COALESCE(notifications.group_key, 'ungrouped-' || notifications.id) = ANY(grouped_notifications.groups)")
+              .select('notifications.*', "array_append(grouped_notifications.groups, COALESCE(notifications.group_key, 'ungrouped-' || notifications.id))")
+              .limit(1),
+          ]
+        )
+        .from('grouped_notifications AS notifications')
+        .order(id: :desc)
+        .limit(limit)
+    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)
+      query = reorder(id: :asc)
+      query = query.where(id: (min_id + 1)...) if min_id.present?
+      query = query.where(id: ...max_id) if max_id.present?
+
+      unscoped
+        .with_recursive(
+          grouped_notifications: [
+            query
+              .select('notifications.*', "ARRAY[COALESCE(notifications.group_key, 'ungrouped-' || notifications.id)] groups")
+              .limit(1),
+            query
+              .joins('CROSS JOIN grouped_notifications')
+              .where('notifications.id > grouped_notifications.id')
+              .where.not("COALESCE(notifications.group_key, 'ungrouped-' || notifications.id) = ANY(grouped_notifications.groups)")
+              .select('notifications.*', "array_append(grouped_notifications.groups, COALESCE(notifications.group_key, 'ungrouped-' || notifications.id))")
+              .limit(1),
+          ]
+        )
+        .from('grouped_notifications AS notifications')
+        .order(id: :asc)
+        .limit(limit)
+    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
+      else
+        paginate_groups_by_max_id(limit, max_id: options[:max_id], since_id: options[:since_id]).to_a
+      end
+    end
+
     def preload_cache_collection_target_statuses(notifications, &_block)
       notifications.group_by(&:type).each do |type, grouped_notifications|
         associations = TARGET_STATUS_INCLUDES_BY_TYPE[type]

app/models/notification_group.rb

@@ -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

app/serializers/rest/notification_group_serializer.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class REST::NotificationGroupSerializer < ActiveModel::Serializer
+  attributes :group_key, :notifications_count, :type
+
+  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
+
+  # TODO: most recent and/or oldest notification ID
+end

app/services/notify_service.rb

@@ -3,6 +3,9 @@
 class NotifyService < BaseService
   include Redisable
 
+  MAXIMUM_GROUP_SPAN_HOURS = 12
+  MAXIMUM_GROUP_GAP_TIME = 4.hours.to_i
+
   NON_EMAIL_TYPES = %i(
     admin.report
     admin.sign_up
@@ -179,6 +182,7 @@ class NotifyService < BaseService
     return if dismiss?
 
     @notification.filtered = filter?
+    @notification.group_key = notification_group_key
     @notification.save!
 
     # It's possible the underlying activity has been deleted
@@ -198,6 +202,24 @@ class NotifyService < BaseService
 
   private
 
+  def notification_group_key
+    return nil if @notification.filtered || %i(favourite reblog).exclude?(@notification.type)
+
+    type_prefix = "#{@notification.type}-#{@notification.target_status.id}"
+    redis_key   = "notif-group/#{@recipient.id}/#{type_prefix}"
+    hour_bucket = @notification.activity.created_at.utc.to_i / 1.hour.to_i
+
+    # Reuse previous group if it does not span too large an amount of time
+    previous_bucket = redis.get(redis_key).to_i
+    hour_bucket = previous_bucket if hour_bucket < previous_bucket + MAXIMUM_GROUP_SPAN_HOURS
+
+    # Do not track groups past a given inactivity time
+    # We do not concern ourselves with race conditions since we use hour buckets
+    redis.set(redis_key, hour_bucket, ex: MAXIMUM_GROUP_GAP_TIME)
+
+    "#{type_prefix}-#{hour_bucket}"
+  end
+
   def dismiss?
     DismissCondition.new(@notification).dismiss?
   end

config/application.rb

@@ -49,6 +49,8 @@ require_relative '../lib/webpacker/helper_extensions'
 require_relative '../lib/rails/engine_extensions'
 require_relative '../lib/active_record/database_tasks_extensions'
 require_relative '../lib/active_record/batches'
+require_relative '../lib/active_record/with_recursive'
+require_relative '../lib/arel/union_parenthesizing'
 require_relative '../lib/simple_navigation/item_extensions'
 
 Dotenv::Rails.load

config/routes/api.rb

@@ -327,6 +327,18 @@ namespace :api, format: false do
     end
   end
 
+  namespace :v2_alpha do
+    resources :notifications, only: [:index, :show] do
+      collection do
+        post :clear
+      end
+
+      member do
+        post :dismiss
+      end
+    end
+  end
+
   namespace :web do
     resource :settings, only: [:update]
     resources :embeds, only: [:show]

db/migrate/20240410095755_add_group_key_to_notifications.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddGroupKeyToNotifications < ActiveRecord::Migration[7.1]
+  def change
+    add_column :notifications, :group_key, :string
+  end
+end

db/migrate/20240423123807_add_index_notifications_on_account_id_and_group_key.rb

@@ -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

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do
+ActiveRecord::Schema[7.1].define(version: 2024_04_23_123807) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -724,6 +724,8 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do
     t.bigint "from_account_id", null: false
     t.string "type"
     t.boolean "filtered", default: false, null: false
+    t.string "group_key"
+    t.index ["account_id", "group_key"], name: "index_notifications_on_account_id_and_group_key", where: "(group_key IS NOT NULL)"
     t.index ["account_id", "id", "type"], name: "index_notifications_on_account_id_and_id_and_type", order: { id: :desc }
     t.index ["account_id", "id", "type"], name: "index_notifications_on_filtered", order: { id: :desc }, where: "(filtered = false)"
     t.index ["activity_id", "activity_type"], name: "index_notifications_on_activity_id_and_activity_type"

lib/active_record/with_recursive.rb

@@ -0,0 +1,63 @@
+# 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.
+
+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

lib/arel/union_parenthesizing.rb

@@ -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

spec/models/notification_spec.rb

@@ -151,6 +151,66 @@ RSpec.describe Notification do
     end
   end
 
+  describe '.paginate_groups_by_max_id' do
+    let(:account) { Fabricate(:account) }
+
+    let!(:notifications) do
+      ['group-1', 'group-1', nil, 'group-2', nil, 'group-1', 'group-2', 'group-1']
+        .map { |group_key| Fabricate(:notification, account: account, group_key: group_key) }
+    end
+
+    context 'without since_id or max_id' do
+      it 'returns the most recent notifications, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_max_id(4).pluck(:id))
+          .to eq [notifications[7].id, notifications[6].id, notifications[4].id, notifications[2].id]
+      end
+    end
+
+    context 'with since_id' do
+      it 'returns the most recent notifications, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_max_id(4, since_id: notifications[4].id).pluck(:id))
+          .to eq [notifications[7].id, notifications[6].id]
+      end
+    end
+
+    context 'with max_id' do
+      it 'returns the most recent notifications after max_id, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_max_id(4, max_id: notifications[7].id).pluck(:id))
+          .to eq [notifications[6].id, notifications[5].id, notifications[4].id, notifications[2].id]
+      end
+    end
+  end
+
+  describe '.paginate_groups_by_min_id' do
+    let(:account) { Fabricate(:account) }
+
+    let!(:notifications) do
+      ['group-1', 'group-1', nil, 'group-2', nil, 'group-1', 'group-2', 'group-1']
+        .map { |group_key| Fabricate(:notification, account: account, group_key: group_key) }
+    end
+
+    context 'without min_id or max_id' do
+      it 'returns the oldest notifications, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_min_id(4).pluck(:id))
+          .to eq [notifications[0].id, notifications[2].id, notifications[3].id, notifications[4].id]
+      end
+    end
+
+    context 'with max_id' do
+      it 'returns the oldest notifications, stopping at max_id, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_min_id(4, max_id: notifications[4].id).pluck(:id))
+          .to eq [notifications[0].id, notifications[2].id, notifications[3].id]
+      end
+    end
+
+    context 'with min_id' do
+      it 'returns the most oldest notifications after min_id, only keeping one notification per group' do
+        expect(described_class.without_suspended.paginate_groups_by_min_id(4, min_id: notifications[0].id).pluck(:id))
+          .to eq [notifications[1].id, notifications[2].id, notifications[3].id, notifications[4].id]
+      end
+    end
+  end
+
   describe '.preload_cache_collection_target_statuses' do
     subject do
       described_class.preload_cache_collection_target_statuses(notifications) do |target_statuses|

spec/requests/api/v2_alpha/notifications_spec.rb

@@ -0,0 +1,164 @@
+# 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'
+        expect(body_json_types).to include 'mention'
+        expect(body_json_types).to include 'favourite'
+        expect(body_json_types).to include '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