From 7d35e406ba3637b30c6fe4fb1982658d30a3bb37 Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Fri, 17 Nov 2023 08:22:18 -0600 Subject: [PATCH] DEV: Add support for limit in notifications index w/o recent param (#24423) Currently to use a limit in the notifications index, you have to also pass recent: true as a param. This PR: Adds optional limit param to be used in the notifications query, regardless of the presence of recent Raises the max limit of the response with recent present from 50 -> 60. It is super weird we have a hard-limit of 50 before with recent param, and 60 without the param. --- app/controllers/notifications_controller.rb | 12 +++++++--- .../requests/notifications_controller_spec.rb | 24 ++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index ecf965cf256..471a40bca2c 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -5,7 +5,7 @@ class NotificationsController < ApplicationController before_action :ensure_admin, only: %i[create update destroy] before_action :set_notification, only: %i[update destroy] - INDEX_LIMIT = 50 + INDEX_LIMIT = 60 def index user = @@ -72,6 +72,7 @@ class NotificationsController < ApplicationController render_json_dump(json) else + limit = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT) offset = params[:offset].to_i notifications = @@ -82,7 +83,7 @@ class NotificationsController < ApplicationController notifications = notifications.where(read: false) if params[:filter] == "unread" total_rows = notifications.dup.count - notifications = notifications.offset(offset).limit(60) + notifications = notifications.offset(offset).limit(limit) notifications = Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications) render_json_dump( @@ -90,7 +91,12 @@ class NotificationsController < ApplicationController total_rows_notifications: total_rows, seen_notification_id: user.seen_notification_id, load_more_notifications: - notifications_path(username: user.username, offset: offset + 60, filter: params[:filter]), + notifications_path( + username: user.username, + offset: offset + limit, + limit: limit, + filter: params[:filter], + ), ) end end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index 972ef1c2550..3bb767d6642 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -90,12 +90,34 @@ RSpec.describe NotificationsController do describe "when limit params is invalid" do include_examples "invalid limit params", "/notifications.json", - described_class::INDEX_LIMIT, + described_class::INDEX_LIMIT + 1, params: { recent: true, } end + it "respects limit param and properly bumps offset for load_more_notifications URL" do + 7.times { notification = Fabricate(:notification, user: user) } + + get "/notifications.json", params: { username: user.username, limit: 2 } + expect(response.parsed_body["notifications"].count).to eq(2) + expect(response.parsed_body["load_more_notifications"]).to eq( + "/notifications?limit=2&offset=2&username=#{user.username}", + ) + + # Same as response above but we need .json added before query params. + get "/notifications.json?limit=2&offset=2&username=#{user.username}" + expect(response.parsed_body["load_more_notifications"]).to eq( + "/notifications?limit=2&offset=4&username=#{user.username}", + ) + + # We are seeing that the offset is increasing properly and limit is staying the same + get "/notifications.json?limit=2&offset=4&username=#{user.username}" + expect(response.parsed_body["load_more_notifications"]).to eq( + "/notifications?limit=2&offset=6&username=#{user.username}", + ) + end + it "get notifications with all filters" do notification = Fabricate(:notification, user: user) notification2 = Fabricate(:notification, user: user)