From 711e1fce0a6f0b883125614aade04d78bb4217ce Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 20 Aug 2024 15:54:08 +0200 Subject: [PATCH] Fix pagination parameters in `GET /api/v2_alpha/notificatins` (#31509) --- app/models/notification.rb | 8 +++--- .../api/v2_alpha/notifications_spec.rb | 28 +++++++++++++++---- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index f1605f0347..ae7c782bed 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -180,8 +180,8 @@ class Notification < ApplicationRecord # 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? + 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) end @@ -190,8 +190,8 @@ class Notification < ApplicationRecord # 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? + 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) end diff --git a/spec/requests/api/v2_alpha/notifications_spec.rb b/spec/requests/api/v2_alpha/notifications_spec.rb index 94bc7b0ee3..a613158aa5 100644 --- a/spec/requests/api/v2_alpha/notifications_spec.rb +++ b/spec/requests/api/v2_alpha/notifications_spec.rb @@ -97,8 +97,7 @@ RSpec.describe 'Notifications' do 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 + PostStatusService.new.call(bob.account, text: 'Hello @alice') FavouriteService.new.call(bob.account, first_status) FavouriteService.new.call(tom.account, first_status) FollowService.new.call(bob.account, user.account) @@ -141,18 +140,37 @@ RSpec.describe 'Notifications' do context 'with limit param' do let(:params) { { limit: 3 } } + let(:notifications) { user.account.notifications.reorder(id: :desc) } it 'returns the requested number of notifications paginated', :aggregate_failures do subject - notifications = user.account.notifications - expect(body_as_json[:notification_groups].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), + prev: api_v2_alpha_notifications_url(limit: params[:limit], min_id: notifications.first.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[3].id) + ) + end + end + + context 'with since_id param' do + let(:params) { { since_id: notifications[2].id } } + let(:notifications) { user.account.notifications.reorder(id: :desc) } + + it 'returns the requested number of notifications paginated', :aggregate_failures do + subject + + expect(body_as_json[:notification_groups].size) + .to eq(2) + + expect(response) + .to include_pagination_headers( + prev: api_v2_alpha_notifications_url(limit: params[:limit], min_id: notifications.first.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)