Fix last paginated notification group only including data on a single notification (#33271)

app/controllers/api/v2/notifications_controller.rb

@@ -80,10 +80,31 @@ class Api::V2::NotificationsController < Api::BaseController
     return [] if @notifications.empty?
 
     MastodonOTELTracer.in_span('Api::V2::NotificationsController#load_grouped_notifications') do
-      NotificationGroup.from_notifications(@notifications, pagination_range: (@notifications.last.id)..(@notifications.first.id), grouped_types: params[:grouped_types])
+      pagination_range = (@notifications.last.id)..@notifications.first.id
+
+      # If the page is incomplete, we know we are on the last page
+      if incomplete_page?
+        if paginating_up?
+          pagination_range = @notifications.last.id...(params[:max_id]&.to_i)
+        else
+          range_start = params[:since_id]&.to_i
+          range_start += 1 unless range_start.nil?
+          pagination_range = range_start..(@notifications.first.id)
+        end
+      end
+
+      NotificationGroup.from_notifications(@notifications, pagination_range: pagination_range, grouped_types: params[:grouped_types])
     end
   end
 
+  def incomplete_page?
+    @notifications.size < limit_param(DEFAULT_NOTIFICATIONS_LIMIT)
+  end
+
+  def paginating_up?
+    params[:min_id].present?
+  end
+
   def browserable_account_notifications
     current_account.notifications.without_suspended.browserable(
       types: Array(browserable_params[:types]),

app/models/notification_group.rb

@@ -63,21 +63,31 @@ class NotificationGroup < ActiveModelSerializers::Model
         binds = [
           account_id,
           SAMPLE_ACCOUNTS_SIZE,
-          pagination_range.begin,
-          pagination_range.end,
           ActiveRecord::Relation::QueryAttribute.new('group_keys', group_keys, ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Array.new(ActiveModel::Type::String.new)),
+          pagination_range.begin || 0,
         ]
+        binds << pagination_range.end unless pagination_range.end.nil?
+
+        upper_bound_cond = begin
+          if pagination_range.end.nil?
+            ''
+          elsif pagination_range.exclude_end?
+            'AND id < $5'
+          else
+            'AND id <= $5'
+          end
+        end
 
         ActiveRecord::Base.connection.select_all(<<~SQL.squish, 'grouped_notifications', binds).cast_values.to_h { |k, *values| [k, values] }
           SELECT
             groups.group_key,
-            (SELECT id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id <= $4 ORDER BY id DESC LIMIT 1),
+            (SELECT id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key #{upper_bound_cond} ORDER BY id DESC LIMIT 1),
-            array(SELECT from_account_id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id <= $4 ORDER BY id DESC LIMIT $2),
+            array(SELECT from_account_id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key #{upper_bound_cond} ORDER BY id DESC LIMIT $2),
-            (SELECT count(*) FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id <= $4) AS notifications_count,
+            (SELECT count(*) FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key #{upper_bound_cond}) AS notifications_count,
-            (SELECT id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id >= $3 ORDER BY id ASC LIMIT 1) AS min_id,
+            (SELECT id FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id >= $4 ORDER BY id ASC LIMIT 1) AS min_id,
-            (SELECT created_at FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key AND id <= $4 ORDER BY id DESC LIMIT 1)
+            (SELECT created_at FROM notifications WHERE notifications.account_id = $1 AND notifications.group_key = groups.group_key #{upper_bound_cond} ORDER BY id DESC LIMIT 1)
           FROM
-            unnest($5::text[]) AS groups(group_key);
+            unnest($3::text[]) AS groups(group_key);
         SQL
       else
         binds = [

spec/requests/api/v2/notifications_spec.rb

@@ -143,6 +143,55 @@ RSpec.describe 'Notifications' do
       end
     end
 
+    context 'when there are numerous notifications for the same final group' do
+      before do
+        user.account.notifications.destroy_all
+        5.times.each { FavouriteService.new.call(Fabricate(:account), user.account.statuses.first) }
+      end
+
+      context 'with no options' do
+        it 'returns a notification group covering all notifications' do
+          subject
+
+          notification_ids = user.account.notifications.reload.pluck(:id)
+
+          expect(response).to have_http_status(200)
+          expect(response.content_type)
+            .to start_with('application/json')
+          expect(response.parsed_body[:notification_groups]).to contain_exactly(
+            a_hash_including(
+              type: 'favourite',
+              sample_account_ids: have_attributes(size: 5),
+              page_min_id: notification_ids.first.to_s,
+              page_max_id: notification_ids.last.to_s
+            )
+          )
+        end
+      end
+
+      context 'with min_id param' do
+        let(:params) { { min_id: user.account.notifications.reload.first.id - 1 } }
+
+        it 'returns a notification group covering all notifications' do
+          subject
+
+          notification_ids = user.account.notifications.reload.pluck(:id)
+
+          expect(response).to have_http_status(200)
+          expect(response.content_type)
+            .to start_with('application/json')
+          expect(response.parsed_body[:notification_groups]).to contain_exactly(
+            a_hash_including(
+              type: 'favourite',
+              sample_account_ids: have_attributes(size: 5),
+              page_min_id: notification_ids.first.to_s,
+              page_max_id: notification_ids.last.to_s
+            )
+          )
+        end
+      end
+    end
+
     context 'with no options' do
       it 'returns expected notification types', :aggregate_failures do
         subject