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