From c88447c11f93c981288e5e85a3197a4d492a6e88 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Fri, 21 Feb 2025 10:58:38 +0530 Subject: [PATCH] feat: invalidate cache after inbox members or team members update (#10869) At the moment, when updating the inbox members, or team members the account cache used for IndexedDB is not invalidated. This can cause inconsistencies in the UI. This PR fixes this by adding explicit invalidation after performing the member changes ### Summary of changes 1. Added a new method `add_members` and `remove_members` to both `team` and `inbox` models. The change was necessary for two reasons - Since the individual `add_member` and `remove_member` is called in a loop, it's wasteful to run the cache invalidation in the method. - Moving the account cache invalidation call in the controller pollutes the controller business logic 2. Updated tests across the board ### More improvements We can make a concern called `Memberable` with usage like `memberable_with :inbox_members`, that can encapsulate the functionality --- Related: https://github.com/chatwoot/chatwoot/issues/10578 --- .../v1/accounts/inbox_members_controller.rb | 8 ++-- .../v1/accounts/team_members_controller.rb | 8 ++-- app/models/inbox.rb | 18 ++++--- app/models/team.rb | 20 ++++++-- .../accounts/inbox_members_controller_spec.rb | 14 +++--- spec/models/inbox_spec.rb | 45 +++++++++++++----- spec/models/team_spec.rb | 47 +++++++++++++++++++ 7 files changed, 124 insertions(+), 36 deletions(-) diff --git a/app/controllers/api/v1/accounts/inbox_members_controller.rb b/app/controllers/api/v1/accounts/inbox_members_controller.rb index 0cb1a0c85..bf8822c27 100644 --- a/app/controllers/api/v1/accounts/inbox_members_controller.rb +++ b/app/controllers/api/v1/accounts/inbox_members_controller.rb @@ -10,7 +10,7 @@ class Api::V1::Accounts::InboxMembersController < Api::V1::Accounts::BaseControl def create authorize @inbox, :create? ActiveRecord::Base.transaction do - agents_to_be_added_ids.map { |user_id| @inbox.add_member(user_id) } + @inbox.add_members(agents_to_be_added_ids) end fetch_updated_agents end @@ -24,7 +24,7 @@ class Api::V1::Accounts::InboxMembersController < Api::V1::Accounts::BaseControl def destroy authorize @inbox, :destroy? ActiveRecord::Base.transaction do - params[:user_ids].map { |user_id| @inbox.remove_member(user_id) } + @inbox.remove_members(params[:user_ids]) end head :ok end @@ -41,8 +41,8 @@ class Api::V1::Accounts::InboxMembersController < Api::V1::Accounts::BaseControl # the missing ones are the agents which are to be deleted from the inbox # the new ones are the agents which are to be added to the inbox ActiveRecord::Base.transaction do - agents_to_be_added_ids.each { |user_id| @inbox.add_member(user_id) } - agents_to_be_removed_ids.each { |user_id| @inbox.remove_member(user_id) } + @inbox.add_members(agents_to_be_added_ids) + @inbox.remove_members(agents_to_be_removed_ids) end end diff --git a/app/controllers/api/v1/accounts/team_members_controller.rb b/app/controllers/api/v1/accounts/team_members_controller.rb index 0243a915a..ffb115a34 100644 --- a/app/controllers/api/v1/accounts/team_members_controller.rb +++ b/app/controllers/api/v1/accounts/team_members_controller.rb @@ -9,14 +9,14 @@ class Api::V1::Accounts::TeamMembersController < Api::V1::Accounts::BaseControll def create ActiveRecord::Base.transaction do - @team_members = members_to_be_added_ids.map { |user_id| @team.add_member(user_id) } + @team_members = @team.add_members(members_to_be_added_ids) end end def update ActiveRecord::Base.transaction do - members_to_be_added_ids.each { |user_id| @team.add_member(user_id) } - members_to_be_removed_ids.each { |user_id| @team.remove_member(user_id) } + @team.add_members(members_to_be_added_ids) + @team.remove_members(members_to_be_removed_ids) end @team_members = @team.members render action: 'create' @@ -24,7 +24,7 @@ class Api::V1::Accounts::TeamMembersController < Api::V1::Accounts::BaseControll def destroy ActiveRecord::Base.transaction do - params[:user_ids].map { |user_id| @team.remove_member(user_id) } + @team.remove_members(params[:user_ids]) end head :ok end diff --git a/app/models/inbox.rb b/app/models/inbox.rb index fe059cb35..2c08172ff 100644 --- a/app/models/inbox.rb +++ b/app/models/inbox.rb @@ -82,14 +82,20 @@ class Inbox < ApplicationRecord scope :order_by_name, -> { order('lower(name) ASC') } - def add_member(user_id) - member = inbox_members.new(user_id: user_id) - member.save! + # Adds multiple members to the inbox + # @param user_ids [Array] Array of user IDs to add as members + # @return [void] + def add_members(user_ids) + inbox_members.create!(user_ids.map { |user_id| { user_id: user_id } }) + update_account_cache end - def remove_member(user_id) - member = inbox_members.find_by!(user_id: user_id) - member.try(:destroy) + # Removes multiple members from the inbox + # @param user_ids [Array] Array of user IDs to remove + # @return [void] + def remove_members(user_ids) + inbox_members.where(user_id: user_ids).destroy_all + update_account_cache end def facebook? diff --git a/app/models/team.rb b/app/models/team.rb index c5935c285..a2b69c7fb 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -31,12 +31,24 @@ class Team < ApplicationRecord self.name = name.downcase if attribute_present?('name') end - def add_member(user_id) - team_members.find_or_create_by(user_id: user_id)&.user + # Adds multiple members to the team + # @param user_ids [Array] Array of user IDs to add as members + # @return [Array] Array of newly added members + def add_members(user_ids) + team_members_to_create = user_ids.map { |user_id| { user_id: user_id } } + created_members = team_members.create(team_members_to_create) + added_users = created_members.filter_map(&:user) + + update_account_cache + added_users end - def remove_member(user_id) - team_members.find_by(user_id: user_id)&.destroy! + # Removes multiple members from the team + # @param user_ids [Array] Array of user IDs to remove + # @return [void] + def remove_members(user_ids) + team_members.where(user_id: user_ids).destroy_all + update_account_cache end def messages diff --git a/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb b/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb index 34633601e..1333d8595 100644 --- a/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb @@ -255,28 +255,30 @@ RSpec.describe 'Inbox Member API', type: :request do expect(response).to have_http_status(:not_found) end - it 'renders error on invalid params' do + it 'ignores invalid params' do params = { inbox_id: inbox.id, user_ids: ['invalid'] } + original_count = inbox.inbox_members&.count delete "/api/v1/accounts/#{account.id}/inbox_members", headers: administrator.create_new_auth_token, params: params, as: :json - expect(response).to have_http_status(:not_found) - expect(response.body).to include('Resource could not be found') + expect(response).to have_http_status(:success) + expect(inbox.inbox_members&.count).to eq(original_count) end - it 'renders error on non member params' do + it 'ignores non member params' do params = { inbox_id: inbox.id, user_ids: [non_member_agent.id] } + original_count = inbox.inbox_members&.count delete "/api/v1/accounts/#{account.id}/inbox_members", headers: administrator.create_new_auth_token, params: params, as: :json - expect(response).to have_http_status(:not_found) - expect(response.body).to include('Resource could not be found') + expect(response).to have_http_status(:success) + expect(inbox.inbox_members&.count).to eq(original_count) end end end diff --git a/spec/models/inbox_spec.rb b/spec/models/inbox_spec.rb index 2a56d1fbc..02d68d77a 100644 --- a/spec/models/inbox_spec.rb +++ b/spec/models/inbox_spec.rb @@ -41,29 +41,50 @@ RSpec.describe Inbox do it_behaves_like 'avatarable' end - describe '#add_member' do + describe '#add_members' do let(:inbox) { FactoryBot.create(:inbox) } - let(:user) { FactoryBot.create(:user) } - it do - expect(inbox.inbox_members.size).to eq(0) + before do + allow(Rails.configuration.dispatcher).to receive(:dispatch) + end - inbox.add_member(user.id) - expect(inbox.reload.inbox_members.size).to eq(1) + it 'handles adds all members and resets cache keys' do + users = FactoryBot.create_list(:user, 3) + inbox.add_members(users.map(&:id)) + expect(inbox.reload.inbox_members.size).to eq(3) + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).at_least(:once) + .with( + 'account.cache_invalidated', + kind_of(Time), + account: inbox.account, + cache_keys: inbox.account.cache_keys + ) end end - describe '#remove_member' do + describe '#remove_members' do let(:inbox) { FactoryBot.create(:inbox) } - let(:user) { FactoryBot.create(:user) } + let(:users) { FactoryBot.create_list(:user, 3) } - before { inbox.add_member(user.id) } + before do + inbox.add_members(users.map(&:id)) + allow(Rails.configuration.dispatcher).to receive(:dispatch) + end - it do - expect(inbox.inbox_members.size).to eq(1) + it 'removes the members and resets cache keys' do + expect(inbox.reload.inbox_members.size).to eq(3) - inbox.remove_member(user.id) + inbox.remove_members(users.map(&:id)) expect(inbox.reload.inbox_members.size).to eq(0) + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).at_least(:once) + .with( + 'account.cache_invalidated', + kind_of(Time), + account: inbox.account, + cache_keys: inbox.account.cache_keys + ) end end diff --git a/spec/models/team_spec.rb b/spec/models/team_spec.rb index e9019e211..cb55dba61 100644 --- a/spec/models/team_spec.rb +++ b/spec/models/team_spec.rb @@ -6,4 +6,51 @@ RSpec.describe Team do it { is_expected.to have_many(:conversations) } it { is_expected.to have_many(:team_members) } end + + describe '#add_members' do + let(:team) { FactoryBot.create(:team) } + + before do + allow(Rails.configuration.dispatcher).to receive(:dispatch) + end + + it 'handles adds all members and resets cache keys' do + users = FactoryBot.create_list(:user, 3) + team.add_members(users.map(&:id)) + expect(team.reload.team_members.size).to eq(3) + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).at_least(:once) + .with( + 'account.cache_invalidated', + kind_of(Time), + account: team.account, + cache_keys: team.account.cache_keys + ) + end + end + + describe '#remove_members' do + let(:team) { FactoryBot.create(:team) } + let(:users) { FactoryBot.create_list(:user, 3) } + + before do + team.add_members(users.map(&:id)) + allow(Rails.configuration.dispatcher).to receive(:dispatch) + end + + it 'removes the members and resets cache keys' do + expect(team.reload.team_members.size).to eq(3) + + team.remove_members(users.map(&:id)) + expect(team.reload.team_members.size).to eq(0) + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).at_least(:once) + .with( + 'account.cache_invalidated', + kind_of(Time), + account: team.account, + cache_keys: team.account.cache_keys + ) + end + end end