fix: Inbox view Read/Snoozed display filters (#8907)

* fix: Notification filters

* Update notification_finder.rb

* Update notification_finder.rb

* Update notification_finder.rb

* fix: spec

* fix: specs

* Update notification_finder.rb

* fix: add more fixes

* Update notification_finder.rb

* fix: specs

* chore: better comments

* chore: removed filtering

* chore: refactoring

* fix: review fixes

* fix: API call

* chore: Minor fix

* Rename spec

* Fix params getting undefined

* Fix finder

---------

Co-authored-by: Sivin Varghese <64252451+iamsivin@users.noreply.github.com>
Co-authored-by: iamsivin <iamsivin@gmail.com>
Co-authored-by: Pranav <pranav@chatwoot.com>

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
 

app/finders/notification_finder.rb

@@ -10,8 +10,8 @@ class NotificationFinder
     set_up
   end
 
-  def perform
+  def notifications
-    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_snoozed_notifications
-    filter_by_status
+    fitler_read_notifications
   end
 
   def find_all_notifications
     @notifications = current_user.notifications.where(account_id: @current_account.id)
   end
 
-  def filter_by_status
+  def filter_snoozed_notifications
-    @notifications = @notifications.where('snoozed_until > ?', DateTime.now.utc) if params[:status] == 'snoozed'
+    @notifications = @notifications.where(snoozed_until: nil) unless type_included?('snoozed')
   end
 
-  def filter_by_read_status
+  def fitler_read_notifications
-    @notifications = @notifications.where.not(read_at: nil) if params[:type] == 'read'
+    @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
+  def sort_order
-    @notifications.page(current_page).per(RESULTS_PER_PAGE).order(last_activity_at: params[:sort_order] || :desc)
+    params[:sort_order] || :desc
   end
 end

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,
       },
     });
   }

app/javascript/dashboard/api/specs/notifications.spec.js

@@ -27,20 +27,36 @@ describe('#NotificationAPI', () => {
       window.axios = originalAxios;
     });
 
-    it('#get', () => {
+    describe('#get', () => {
-      notificationsAPI.get({
+      it('generates the API call if both params are available', () => {
-        page: 1,
+        notificationsAPI.get({
-        status: 'read',
-        type: 'Conversation',
-        sortOrder: 'desc',
-      });
-      expect(axiosMock.get).toHaveBeenCalledWith('/api/v1/notifications', {
-        params: {
           page: 1,
-          status: 'read',
+          status: 'snoozed',
-          type: 'Conversation',
+          type: 'read',
-          sort_order: 'desc',
+          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'],
+          },
+        });
       });
     });
 

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(
+    const sortedNotifications = Object.values($state.records).sort((n1, n2) =>
-      notification => applyInboxPageFilters(notification, filters)
+      sortComparator(n1, n2, sortOrder)
-    );
-    const sortedNotifications = filteredNotifications.sort((a, b) =>
-      sortComparator(a, b, sortOrder)
     );
     return sortedNotifications;
   },

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',

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',

app/javascript/dashboard/store/modules/specs/notifications/helpers.spec.js

@@ -1,10 +1,4 @@
-import {
+import { sortComparator } from '../../notifications/helpers';
-  filterByStatus,
-  filterByType,
-  filterByTypeAndStatus,
-  applyInboxPageFilters,
-  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';

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

spec/finders/notification_finder_spec.rb

@@ -1,103 +1,79 @@
 require 'rails_helper'
 
-describe NotificationFinder do
+RSpec.describe NotificationFinder do
-  subject(:notification_finder) { described_class.new(user, account, params) }
-
   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,
+    create(:notification, :snoozed, account: account, user: user)
-                          read_at: DateTime.now.utc)
+    create_list(:notification, 2, :read, account: account, user: user)
-    create(:notification, account: account, user: user, snoozed_until: DateTime.now.utc + 3.days, updated_at: DateTime.now.utc,
+    create_list(:notification, 3, account: account, user: user)
-                          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)
   end
 
-  describe '#perform' do
+  describe '#notifications' do
-    context 'when params are empty' do
+    subject { notification_finder.notifications }
+
+    context 'with default params (empty)' do
       let(:params) { {} }
 
-      it 'returns all the notifications' do
+      it 'returns all unread and unsnoozed notifications, ordered by last activity' do
-        result = notification_finder.perform
+        expect(subject.size).to eq(3)
-        expect(result.length).to be 6
+        expect(subject).to match_array(subject.sort_by(&:last_activity_at).reverse)
-      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
       end
     end
 
-    context 'when snoozed param is passed' do
+    context 'with params including read and snoozed statuses' do
-      let(:params) { { status: 'snoozed' } }
+      let(:params) { { includes: %w[read snoozed] } }
 
-      it 'returns only snoozed notifications' do
+      it 'returns all notifications, including read and snoozed' do
-        result = notification_finder.perform
+        expect(subject.size).to eq(6)
-        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
       end
     end
 
-    context 'when type read param is passed' do
+    context 'with params including only read status' do
-      let(:params) { { type: 'read' } }
+      let(:params) { { includes: ['read'] } }
 
-      it 'returns only read notifications' do
+      it 'returns all notifications expect the snoozed' do
-        result = notification_finder.perform
+        expect(subject.size).to eq(5)
-        expect(result.length).to be 2
-      end
-
-      it 'returns count' do
-        result = notification_finder.count
-        expect(result).to be 2
       end
     end
 
-    context 'when type read and snoozed param is passed' do
+    context 'with params including only snoozed status' do
-      let(:params) { { type: 'read', status: 'snoozed' } }
+      let(:params) { { includes: ['snoozed'] } }
 
-      it 'returns only read notifications' do
+      it 'rreturns all notifications only expect the read' do
-        result = notification_finder.perform
+        expect(subject.size).to eq(4)
-        expect(result.length).to be 0
-      end
-
-      it 'returns count' do
-        result = notification_finder.count
-        expect(result).to be 0
       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
+      it 'returns notifications in ascending order by last activity' do
-        result = notification_finder.perform
+        expect(subject.first.last_activity_at).to be < subject.last.last_activity_at
-        expect(result.first.last_activity_at).to be < result.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