From e6ffbfb5e7684880e9252f72f9cb00b151c28d10 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 15 Mar 2022 04:11:29 +0100 Subject: [PATCH] Add `types` param to `GET /api/v1/notifications` in REST API (#17767) * Add `types` param to `GET /api/v1/notifications` in REST API * Improve tests --- .../api/v1/notifications_controller.rb | 19 +++-- app/models/notification.rb | 24 ++++-- .../api/v1/notifications_controller_spec.rb | 79 ++++++------------- 3 files changed, 49 insertions(+), 73 deletions(-) diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb index f9d7808398..6d464997ea 100644 --- a/app/controllers/api/v1/notifications_controller.rb +++ b/app/controllers/api/v1/notifications_controller.rb @@ -35,13 +35,18 @@ class Api::V1::NotificationsController < Api::BaseController 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(exclude_types, from_account) + current_account.notifications.without_suspended.browserable( + types: Array(browserable_params[:types]), + exclude_types: Array(browserable_params[:exclude_types]), + from_account_id: browserable_params[:account_id] + ) end def target_statuses_from_notifications @@ -72,17 +77,11 @@ class Api::V1::NotificationsController < Api::BaseController @notifications.first.id end - def exclude_types - val = params.permit(exclude_types: [])[:exclude_types] || [] - val = [val] unless val.is_a?(Enumerable) - val - end - - def from_account - params[:account_id] + def browserable_params + params.permit(:account_id, types: [], exclude_types: []) end def pagination_params(core_params) - params.slice(:limit, :exclude_types).permit(:limit, exclude_types: []).merge(core_params) + params.slice(:limit, :account_id, :types, :exclude_types).permit(:limit, :account_id, types: [], exclude_types: []).merge(core_params) end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 9bf296386a..ba94b54d12 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -63,13 +63,6 @@ class Notification < ApplicationRecord scope :without_suspended, -> { joins(:from_account).merge(Account.without_suspended) } - scope :browserable, ->(exclude_types = [], account_id = nil) { - scope = all - scope = where(from_account_id: account_id) if account_id.present? - scope = scope.where(type: TYPES - exclude_types.map(&:to_sym)) unless exclude_types.empty? - scope - } - def type @type ||= (super || LEGACY_TYPE_CLASS_MAP[activity_type]).to_sym end @@ -90,6 +83,23 @@ class Notification < ApplicationRecord end class << self + def browserable(types: [], exclude_types: [], from_account_id: nil) + requested_types = begin + if types.empty? + TYPES + else + types.map(&:to_sym) & TYPES + end + end + + requested_types -= exclude_types.map(&:to_sym) + + all.tap do |scope| + scope.merge!(where(from_account_id: from_account_id)) if from_account_id.present? + scope.merge!(where(type: requested_types)) unless requested_types.size == TYPES.size + 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] diff --git a/spec/controllers/api/v1/notifications_controller_spec.rb b/spec/controllers/api/v1/notifications_controller_spec.rb index f8df6589f1..46e177c0ec 100644 --- a/spec/controllers/api/v1/notifications_controller_spec.rb +++ b/spec/controllers/api/v1/notifications_controller_spec.rb @@ -70,23 +70,23 @@ RSpec.describe Api::V1::NotificationsController, type: :controller do end it 'includes reblog' do - expect(assigns(:notifications).map(&:activity)).to include(@reblog_of_first_status) + expect(body_as_json.map { |x| x[:type] }).to include 'reblog' end it 'includes mention' do - expect(assigns(:notifications).map(&:activity)).to include(@mention_from_status) + expect(body_as_json.map { |x| x[:type] }).to include 'mention' end it 'includes favourite' do - expect(assigns(:notifications).map(&:activity)).to include(@favourite) + expect(body_as_json.map { |x| x[:type] }).to include 'favourite' end it 'includes follow' do - expect(assigns(:notifications).map(&:activity)).to include(@follow) + expect(body_as_json.map { |x| x[:type] }).to include 'follow' end end - describe 'from specified user' do + describe 'with account_id param' do before do get :index, params: { account_id: third.account.id } end @@ -95,28 +95,12 @@ RSpec.describe Api::V1::NotificationsController, type: :controller do expect(response).to have_http_status(200) end - it 'includes favourite' do - expect(assigns(:notifications).map(&:activity)).to include(@second_favourite) - end - - it 'excludes favourite' do - expect(assigns(:notifications).map(&:activity)).to_not include(@favourite) - end - - it 'excludes mention' do - expect(assigns(:notifications).map(&:activity)).to_not include(@mention_from_status) - end - - it 'excludes reblog' do - expect(assigns(:notifications).map(&:activity)).to_not include(@reblog_of_first_status) - end - - it 'excludes follow' do - expect(assigns(:notifications).map(&:activity)).to_not include(@follow) + it 'returns only notifications from specified user' do + expect(body_as_json.map { |x| x[:account][:id] }.uniq).to eq [third.account.id.to_s] end end - describe 'from nonexistent user' do + describe 'with invalid account_id param' do before do get :index, params: { account_id: 'foo' } end @@ -125,54 +109,37 @@ RSpec.describe Api::V1::NotificationsController, type: :controller do expect(response).to have_http_status(200) end - it 'excludes favourite' do - expect(assigns(:notifications).map(&:activity)).to_not include(@favourite) - end - - it 'excludes second favourite' do - expect(assigns(:notifications).map(&:activity)).to_not include(@second_favourite) - end - - it 'excludes mention' do - expect(assigns(:notifications).map(&:activity)).to_not include(@mention_from_status) - end - - it 'excludes reblog' do - expect(assigns(:notifications).map(&:activity)).to_not include(@reblog_of_first_status) - end - - it 'excludes follow' do - expect(assigns(:notifications).map(&:activity)).to_not include(@follow) + it 'returns nothing' do + expect(body_as_json.size).to eq 0 end end - describe 'with excluded mentions' do + describe 'with excluded_types param' do before do - get :index, params: { exclude_types: ['mention'] } + get :index, params: { exclude_types: %w(mention) } end it 'returns http success' do expect(response).to have_http_status(200) end - it 'includes reblog' do - expect(assigns(:notifications).map(&:activity)).to include(@reblog_of_first_status) - end - - it 'excludes mention' do - expect(assigns(:notifications).map(&:activity)).to_not include(@mention_from_status) + it 'returns everything but excluded type' do + expect(body_as_json.size).to_not eq 0 + expect(body_as_json.map { |x| x[:type] }.uniq).to_not include 'mention' end + end - it 'includes favourite' do - expect(assigns(:notifications).map(&:activity)).to include(@favourite) + describe 'with types param' do + before do + get :index, params: { types: %w(mention) } end - it 'includes third favourite' do - expect(assigns(:notifications).map(&:activity)).to include(@second_favourite) + it 'returns http success' do + expect(response).to have_http_status(200) end - it 'includes follow' do - expect(assigns(:notifications).map(&:activity)).to include(@follow) + it 'returns only requested type' do + expect(body_as_json.map { |x| x[:type] }.uniq).to eq ['mention'] end end end