diff --git a/app/controllers/api/v1/accounts/notifications_controller.rb b/app/controllers/api/v1/accounts/notifications_controller.rb index 0eeff5695..3ac0568e3 100644 --- a/app/controllers/api/v1/accounts/notifications_controller.rb +++ b/app/controllers/api/v1/accounts/notifications_controller.rb @@ -7,8 +7,8 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro before_action :set_current_page, only: [:index] def index + @notifications = notification_finder.notifications @unread_count = notification_finder.unread_count - @notifications = notification_finder.perform @count = notification_finder.count end diff --git a/app/finders/notification_finder.rb b/app/finders/notification_finder.rb index 7e5789372..ccfe470a0 100644 --- a/app/finders/notification_finder.rb +++ b/app/finders/notification_finder.rb @@ -10,8 +10,8 @@ class NotificationFinder set_up end - def perform - notifications + def notifications + @notifications.page(current_page).per(RESULTS_PER_PAGE).order(last_activity_at: sort_order) end def unread_count @@ -26,27 +26,31 @@ class NotificationFinder def set_up find_all_notifications - filter_by_read_status - filter_by_status + filter_snoozed_notifications + fitler_read_notifications end def find_all_notifications @notifications = current_user.notifications.where(account_id: @current_account.id) end - def filter_by_status - @notifications = @notifications.where('snoozed_until > ?', DateTime.now.utc) if params[:status] == 'snoozed' + def filter_snoozed_notifications + @notifications = @notifications.where(snoozed_until: nil) unless type_included?('snoozed') end - def filter_by_read_status - @notifications = @notifications.where.not(read_at: nil) if params[:type] == 'read' + def fitler_read_notifications + @notifications = @notifications.where(read_at: nil) unless type_included?('read') + end + + def type_included?(type) + (params[:includes] || []).include?(type) end def current_page params[:page] || 1 end - def notifications - @notifications.page(current_page).per(RESULTS_PER_PAGE).order(last_activity_at: params[:sort_order] || :desc) + def sort_order + params[:sort_order] || :desc end end diff --git a/app/javascript/dashboard/api/notifications.js b/app/javascript/dashboard/api/notifications.js index 65fc937b7..65284623f 100644 --- a/app/javascript/dashboard/api/notifications.js +++ b/app/javascript/dashboard/api/notifications.js @@ -7,12 +7,13 @@ class NotificationsAPI extends ApiClient { } get({ page, status, type, sortOrder }) { + const includesFilter = [status, type].filter(value => !!value); + return axios.get(this.url, { params: { page, - status, - type, sort_order: sortOrder, + includes: includesFilter, }, }); } diff --git a/app/javascript/dashboard/api/specs/notifications.spec.js b/app/javascript/dashboard/api/specs/notifications.spec.js index bc06eaa2b..fe748fe19 100644 --- a/app/javascript/dashboard/api/specs/notifications.spec.js +++ b/app/javascript/dashboard/api/specs/notifications.spec.js @@ -27,20 +27,36 @@ describe('#NotificationAPI', () => { window.axios = originalAxios; }); - it('#get', () => { - notificationsAPI.get({ - page: 1, - status: 'read', - type: 'Conversation', - sortOrder: 'desc', - }); - expect(axiosMock.get).toHaveBeenCalledWith('/api/v1/notifications', { - params: { + describe('#get', () => { + it('generates the API call if both params are available', () => { + notificationsAPI.get({ page: 1, - status: 'read', - type: 'Conversation', - sort_order: 'desc', - }, + status: 'snoozed', + type: 'read', + sortOrder: 'desc', + }); + expect(axiosMock.get).toHaveBeenCalledWith('/api/v1/notifications', { + params: { + page: 1, + sort_order: 'desc', + includes: ['snoozed', 'read'], + }, + }); + }); + + it('generates the API call if one of the params are available', () => { + notificationsAPI.get({ + page: 1, + type: 'read', + sortOrder: 'desc', + }); + expect(axiosMock.get).toHaveBeenCalledWith('/api/v1/notifications', { + params: { + page: 1, + sort_order: 'desc', + includes: ['read'], + }, + }); }); }); diff --git a/app/javascript/dashboard/store/modules/notifications/getters.js b/app/javascript/dashboard/store/modules/notifications/getters.js index b8df2b173..3fc1d9458 100644 --- a/app/javascript/dashboard/store/modules/notifications/getters.js +++ b/app/javascript/dashboard/store/modules/notifications/getters.js @@ -1,4 +1,4 @@ -import { applyInboxPageFilters, sortComparator } from './helpers'; +import { sortComparator } from './helpers'; export const getters = { getNotifications($state) { @@ -6,11 +6,8 @@ export const getters = { }, getFilteredNotifications: $state => filters => { const sortOrder = filters.sortOrder === 'desc' ? 'newest' : 'oldest'; - const filteredNotifications = Object.values($state.records).filter( - notification => applyInboxPageFilters(notification, filters) - ); - const sortedNotifications = filteredNotifications.sort((a, b) => - sortComparator(a, b, sortOrder) + const sortedNotifications = Object.values($state.records).sort((n1, n2) => + sortComparator(n1, n2, sortOrder) ); return sortedNotifications; }, diff --git a/app/javascript/dashboard/store/modules/notifications/helpers.js b/app/javascript/dashboard/store/modules/notifications/helpers.js index 53c05c94e..33b864c43 100644 --- a/app/javascript/dashboard/store/modules/notifications/helpers.js +++ b/app/javascript/dashboard/store/modules/notifications/helpers.js @@ -1,31 +1,3 @@ -export const filterByStatus = (snoozedUntil, filterStatus) => - filterStatus === 'snoozed' ? !!snoozedUntil : !snoozedUntil; - -export const filterByType = (readAt, filterType) => - filterType === 'read' ? !!readAt : !readAt; - -export const filterByTypeAndStatus = ( - readAt, - snoozedUntil, - filterType, - filterStatus -) => { - const shouldFilterByStatus = filterByStatus(snoozedUntil, filterStatus); - const shouldFilterByType = filterByType(readAt, filterType); - return shouldFilterByStatus && shouldFilterByType; -}; - -export const applyInboxPageFilters = (notification, filters) => { - const { status, type } = filters; - const { read_at: readAt, snoozed_until: snoozedUntil } = notification; - - if (status && type) - return filterByTypeAndStatus(readAt, snoozedUntil, type, status); - if (status && !type) return filterByStatus(snoozedUntil, status); - if (!status && type) return filterByType(readAt, type); - return true; -}; - const INBOX_SORT_OPTIONS = { newest: 'desc', oldest: 'asc', diff --git a/app/javascript/dashboard/store/modules/specs/notifications/getters.spec.js b/app/javascript/dashboard/store/modules/specs/notifications/getters.spec.js index d557fca7a..f3ea23cb5 100644 --- a/app/javascript/dashboard/store/modules/specs/notifications/getters.spec.js +++ b/app/javascript/dashboard/store/modules/specs/notifications/getters.spec.js @@ -34,6 +34,8 @@ describe('#getters', () => { sortOrder: 'desc', }; expect(getters.getFilteredNotifications(state)(filters)).toEqual([ + { id: 1, read_at: '2024-02-07T11:42:39.988Z', snoozed_until: null }, + { id: 2, read_at: null, snoozed_until: null }, { id: 3, read_at: '2024-02-07T11:42:39.988Z', diff --git a/app/javascript/dashboard/store/modules/specs/notifications/helpers.spec.js b/app/javascript/dashboard/store/modules/specs/notifications/helpers.spec.js index f5a9a3d9b..6d0338032 100644 --- a/app/javascript/dashboard/store/modules/specs/notifications/helpers.spec.js +++ b/app/javascript/dashboard/store/modules/specs/notifications/helpers.spec.js @@ -1,10 +1,4 @@ -import { - filterByStatus, - filterByType, - filterByTypeAndStatus, - applyInboxPageFilters, - sortComparator, -} from '../../notifications/helpers'; +import { sortComparator } from '../../notifications/helpers'; const notifications = [ { @@ -45,126 +39,6 @@ const notifications = [ }, ]; -describe('#filterByStatus', () => { - it('returns the notifications with snoozed status', () => { - const filters = { status: 'snoozed' }; - notifications.forEach(notification => { - expect( - filterByStatus(notification.snoozed_until, filters.status) - ).toEqual(notification.snoozed_until !== null); - }); - }); - it('returns true if the notification is snoozed', () => { - const filters = { status: 'snoozed' }; - expect( - filterByStatus(notifications[3].snoozed_until, filters.status) - ).toEqual(true); - }); - it('returns false if the notification is not snoozed', () => { - const filters = { status: 'snoozed' }; - expect( - filterByStatus(notifications[2].snoozed_until, filters.status) - ).toEqual(false); - }); -}); - -describe('#filterByType', () => { - it('returns the notifications with read status', () => { - const filters = { type: 'read' }; - notifications.forEach(notification => { - expect(filterByType(notification.read_at, filters.type)).toEqual( - notification.read_at !== null - ); - }); - }); - it('returns true if the notification is read', () => { - const filters = { type: 'read' }; - expect(filterByType(notifications[0].read_at, filters.type)).toEqual(true); - }); - it('returns false if the notification is not read', () => { - const filters = { type: 'read' }; - expect(filterByType(notifications[1].read_at, filters.type)).toEqual(false); - }); -}); - -describe('#filterByTypeAndStatus', () => { - it('returns the notifications with type and status', () => { - const filters = { type: 'read', status: 'snoozed' }; - notifications.forEach(notification => { - expect( - filterByTypeAndStatus( - notification.read_at, - notification.snoozed_until, - filters.type, - filters.status - ) - ).toEqual( - notification.read_at !== null && notification.snoozed_until !== null - ); - }); - }); - it('returns true if the notification is read and snoozed', () => { - const filters = { type: 'read', status: 'snoozed' }; - expect( - filterByTypeAndStatus( - notifications[4].read_at, - notifications[4].snoozed_until, - filters.type, - filters.status - ) - ).toEqual(true); - }); - it('returns false if the notification is not read and snoozed', () => { - const filters = { type: 'read', status: 'snoozed' }; - expect( - filterByTypeAndStatus( - notifications[3].read_at, - notifications[3].snoozed_until, - filters.type, - filters.status - ) - ).toEqual(false); - }); -}); - -describe('#applyInboxPageFilters', () => { - it('returns the notifications with type and status', () => { - const filters = { type: 'read', status: 'snoozed' }; - notifications.forEach(notification => { - expect(applyInboxPageFilters(notification, filters)).toEqual( - filterByTypeAndStatus( - notification.read_at, - notification.snoozed_until, - filters.type, - filters.status - ) - ); - }); - }); - it('returns the notifications with type only', () => { - const filters = { type: 'read', status: null }; - notifications.forEach(notification => { - expect(applyInboxPageFilters(notification, filters)).toEqual( - filterByType(notification.read_at, filters.type) - ); - }); - }); - it('returns the notifications with status only', () => { - const filters = { type: null, status: 'snoozed' }; - notifications.forEach(notification => { - expect(applyInboxPageFilters(notification, filters)).toEqual( - filterByStatus(notification.snoozed_until, filters.status) - ); - }); - }); - it('returns true if there are no filters', () => { - const filters = { type: null, status: null }; - notifications.forEach(notification => { - expect(applyInboxPageFilters(notification, filters)).toEqual(true); - }); - }); -}); - describe('#sortComparator', () => { it('returns the notifications sorted by newest', () => { const sortOrder = 'newest'; diff --git a/spec/factories/notifications.rb b/spec/factories/notifications.rb index 63bae1119..1c3a8fba5 100644 --- a/spec/factories/notifications.rb +++ b/spec/factories/notifications.rb @@ -6,5 +6,15 @@ FactoryBot.define do notification_type { 'conversation_assignment' } user account + read_at { nil } + snoozed_until { nil } + end + + trait :read do + read_at { DateTime.now.utc - 3.days } + end + + trait :snoozed do + snoozed_until { DateTime.now.utc + 3.days } end end diff --git a/spec/finders/notification_finder_spec.rb b/spec/finders/notification_finder_spec.rb index 48a9f2785..962edc297 100644 --- a/spec/finders/notification_finder_spec.rb +++ b/spec/finders/notification_finder_spec.rb @@ -1,103 +1,79 @@ require 'rails_helper' -describe NotificationFinder do - subject(:notification_finder) { described_class.new(user, account, params) } - +RSpec.describe NotificationFinder do let!(:account) { create(:account) } let!(:user) { create(:user, account: account) } + let(:notification_finder) { described_class.new(user, account, params) } before do - create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 1.day, last_activity_at: DateTime.now.utc + 1.day, - read_at: DateTime.now.utc) - create(:notification, account: account, user: user, snoozed_until: DateTime.now.utc + 3.days, updated_at: DateTime.now.utc, - last_activity_at: DateTime.now.utc) - create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 2.days, last_activity_at: DateTime.now.utc + 2.days) - create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 4.days, last_activity_at: DateTime.now.utc + 4.days, - notification_type: :conversation_creation, read_at: DateTime.now.utc) - create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 5.days, last_activity_at: DateTime.now.utc + 5.days, - notification_type: :conversation_mention) - create(:notification, account: account, user: user, updated_at: DateTime.now.utc + 6.days, last_activity_at: DateTime.now.utc + 6.days, - notification_type: :participating_conversation_new_message) + create(:notification, :snoozed, account: account, user: user) + create_list(:notification, 2, :read, account: account, user: user) + create_list(:notification, 3, account: account, user: user) end - describe '#perform' do - context 'when params are empty' do + describe '#notifications' do + subject { notification_finder.notifications } + + context 'with default params (empty)' do let(:params) { {} } - it 'returns all the notifications' do - result = notification_finder.perform - expect(result.length).to be 6 - end - - it 'orders notifications by last activity at' do - result = notification_finder.perform - expect(result.first.last_activity_at).to be > result.last.last_activity_at - end - - it 'returns unread count' do - result = notification_finder.unread_count - expect(result).to be 4 - end - - it 'returns count' do - result = notification_finder.count - expect(result).to be 6 + it 'returns all unread and unsnoozed notifications, ordered by last activity' do + expect(subject.size).to eq(3) + expect(subject).to match_array(subject.sort_by(&:last_activity_at).reverse) end end - context 'when snoozed param is passed' do - let(:params) { { status: 'snoozed' } } + context 'with params including read and snoozed statuses' do + let(:params) { { includes: %w[read snoozed] } } - it 'returns only snoozed notifications' do - result = notification_finder.perform - expect(result.length).to be 1 - end - - it 'returns unread count' do - result = notification_finder.unread_count - expect(result).to be 1 - end - - it 'returns count' do - result = notification_finder.count - expect(result).to be 1 + it 'returns all notifications, including read and snoozed' do + expect(subject.size).to eq(6) end end - context 'when type read param is passed' do - let(:params) { { type: 'read' } } + context 'with params including only read status' do + let(:params) { { includes: ['read'] } } - it 'returns only read notifications' do - result = notification_finder.perform - expect(result.length).to be 2 - end - - it 'returns count' do - result = notification_finder.count - expect(result).to be 2 + it 'returns all notifications expect the snoozed' do + expect(subject.size).to eq(5) end end - context 'when type read and snoozed param is passed' do - let(:params) { { type: 'read', status: 'snoozed' } } + context 'with params including only snoozed status' do + let(:params) { { includes: ['snoozed'] } } - it 'returns only read notifications' do - result = notification_finder.perform - expect(result.length).to be 0 - end - - it 'returns count' do - result = notification_finder.count - expect(result).to be 0 + it 'rreturns all notifications only expect the read' do + expect(subject.size).to eq(4) end end - context 'when sort order is passed' do + context 'with ascending sort order' do let(:params) { { sort_order: :asc } } - it 'returns notifications in ascending order' do - result = notification_finder.perform - expect(result.first.last_activity_at).to be < result.last.last_activity_at + it 'returns notifications in ascending order by last activity' do + expect(subject.first.last_activity_at).to be < subject.last.last_activity_at + end + end + end + + describe 'counts' do + subject { notification_finder } + + context 'without specific filters' do + let(:params) { {} } + + it 'correctly reports unread and total counts' do + expect(subject.unread_count).to eq(3) + expect(subject.count).to eq(3) + end + end + + context 'with filters applied' do + let(:params) { { includes: %w[read snoozed] } } + + it 'adjusts counts based on included statuses' do + expect(subject.unread_count).to eq(4) + expect(subject.count).to eq(6) end end end