From 009abc1948d499e15155d7d0049e6cf24774d35e Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Tue, 21 Dec 2021 22:48:01 +0530 Subject: [PATCH] chore: API improvements (#3637) - Unique validations for Inbox members and Team member objects - Move notification processing to Async --- .../v1/accounts/inbox_members_controller.rb | 4 ++-- .../v1/accounts/team_members_controller.rb | 2 +- app/dispatchers/async_dispatcher.rb | 1 + app/dispatchers/sync_dispatcher.rb | 2 +- .../inboxes/fetch_imap_email_inboxes_job.rb | 2 +- .../concerns/activity_message_handler.rb | 14 +++++++------- app/models/inbox_member.rb | 4 +++- app/models/team_member.rb | 1 + ...25545_add_unique_index_on_inbox_members.rb | 19 +++++++++++++++++++ db/schema.rb | 3 ++- .../accounts/inbox_members_controller_spec.rb | 2 +- .../accounts/team_members_controller_spec.rb | 11 +++++++---- .../concerns/assignment_handler_shared.rb | 14 ++------------ 13 files changed, 48 insertions(+), 31 deletions(-) create mode 100644 db/migrate/20211221125545_add_unique_index_on_inbox_members.rb diff --git a/app/controllers/api/v1/accounts/inbox_members_controller.rb b/app/controllers/api/v1/accounts/inbox_members_controller.rb index 591aa5637..22726a855 100644 --- a/app/controllers/api/v1/accounts/inbox_members_controller.rb +++ b/app/controllers/api/v1/accounts/inbox_members_controller.rb @@ -1,11 +1,11 @@ class Api::V1::Accounts::InboxMembersController < Api::V1::Accounts::BaseController before_action :fetch_inbox - before_action :current_agents_ids, only: [:update] + before_action :current_agents_ids, only: [:create, :update] def create authorize @inbox, :create? ActiveRecord::Base.transaction do - params[:user_ids].map { |user_id| @inbox.add_member(user_id) } + agents_to_be_added_ids.map { |user_id| @inbox.add_member(user_id) } end fetch_updated_agents end diff --git a/app/controllers/api/v1/accounts/team_members_controller.rb b/app/controllers/api/v1/accounts/team_members_controller.rb index d7e17a687..19a46b607 100644 --- a/app/controllers/api/v1/accounts/team_members_controller.rb +++ b/app/controllers/api/v1/accounts/team_members_controller.rb @@ -8,7 +8,7 @@ class Api::V1::Accounts::TeamMembersController < Api::V1::Accounts::BaseControll def create ActiveRecord::Base.transaction do - @team_members = params[:user_ids].map { |user_id| @team.add_member(user_id) } + @team_members = members_to_be_added_ids.map { |user_id| @team.add_member(user_id) } end end diff --git a/app/dispatchers/async_dispatcher.rb b/app/dispatchers/async_dispatcher.rb index 1cbb56747..232dcff77 100644 --- a/app/dispatchers/async_dispatcher.rb +++ b/app/dispatchers/async_dispatcher.rb @@ -15,6 +15,7 @@ class AsyncDispatcher < BaseDispatcher EventListener.instance, HookListener.instance, InstallationWebhookListener.instance, + NotificationListener.instance, WebhookListener.instance ] end diff --git a/app/dispatchers/sync_dispatcher.rb b/app/dispatchers/sync_dispatcher.rb index 9f7adc02c..509a42727 100644 --- a/app/dispatchers/sync_dispatcher.rb +++ b/app/dispatchers/sync_dispatcher.rb @@ -5,6 +5,6 @@ class SyncDispatcher < BaseDispatcher end def listeners - [ActionCableListener.instance, AgentBotListener.instance, NotificationListener.instance] + [ActionCableListener.instance, AgentBotListener.instance] end end diff --git a/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb b/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb index ec0ca7d21..c9b847575 100644 --- a/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb +++ b/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb @@ -3,7 +3,7 @@ class Inboxes::FetchImapEmailInboxesJob < ApplicationJob def perform Inbox.where(channel_type: 'Channel::Email').all.each do |inbox| - Inboxes::FetchImapEmailsJob.perform_later(inbox.channel) if inbox.channel.imap_enabled + ::Inboxes::FetchImapEmailsJob.perform_later(inbox.channel) if inbox.channel.imap_enabled end end end diff --git a/app/models/concerns/activity_message_handler.rb b/app/models/concerns/activity_message_handler.rb index 90eb2f52d..03a488e4c 100644 --- a/app/models/concerns/activity_message_handler.rb +++ b/app/models/concerns/activity_message_handler.rb @@ -24,7 +24,7 @@ module ActivityMessageHandler I18n.t('conversations.activity.status.auto_resolved', duration: auto_resolve_duration) end - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def create_label_added(user_name, labels = []) @@ -33,7 +33,7 @@ module ActivityMessageHandler params = { user_name: user_name, labels: labels.join(', ') } content = I18n.t('conversations.activity.labels.added', **params) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def create_label_removed(user_name, labels = []) @@ -42,7 +42,7 @@ module ActivityMessageHandler params = { user_name: user_name, labels: labels.join(', ') } content = I18n.t('conversations.activity.labels.removed', **params) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def create_muted_message @@ -51,7 +51,7 @@ module ActivityMessageHandler params = { user_name: Current.user.name } content = I18n.t('conversations.activity.muted', **params) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def create_unmuted_message @@ -60,7 +60,7 @@ module ActivityMessageHandler params = { user_name: Current.user.name } content = I18n.t('conversations.activity.unmuted', **params) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def generate_team_change_activity_key @@ -82,7 +82,7 @@ module ActivityMessageHandler params[:team_name] = generate_team_name_for_activity if key == 'removed' content = I18n.t("conversations.activity.team.#{key}", **params) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end def generate_assignee_change_activity_content(user_name) @@ -96,6 +96,6 @@ module ActivityMessageHandler return unless user_name content = generate_assignee_change_activity_content(user_name) - Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content + ::Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content end end diff --git a/app/models/inbox_member.rb b/app/models/inbox_member.rb index 4326b0a8f..d0b5640d8 100644 --- a/app/models/inbox_member.rb +++ b/app/models/inbox_member.rb @@ -10,12 +10,14 @@ # # Indexes # -# index_inbox_members_on_inbox_id (inbox_id) +# index_inbox_members_on_inbox_id (inbox_id) +# index_inbox_members_on_inbox_id_and_user_id (inbox_id,user_id) UNIQUE # class InboxMember < ApplicationRecord validates :inbox_id, presence: true validates :user_id, presence: true + validates :user_id, uniqueness: { scope: :inbox_id } belongs_to :user belongs_to :inbox diff --git a/app/models/team_member.rb b/app/models/team_member.rb index 309d0e8e7..d73cd7c09 100644 --- a/app/models/team_member.rb +++ b/app/models/team_member.rb @@ -22,4 +22,5 @@ class TeamMember < ApplicationRecord belongs_to :user belongs_to :team + validates :user_id, uniqueness: { scope: :team_id } end diff --git a/db/migrate/20211221125545_add_unique_index_on_inbox_members.rb b/db/migrate/20211221125545_add_unique_index_on_inbox_members.rb new file mode 100644 index 000000000..0af4219e7 --- /dev/null +++ b/db/migrate/20211221125545_add_unique_index_on_inbox_members.rb @@ -0,0 +1,19 @@ +# ref: https://dev.to/nodefiend/rails-migration-adding-a-unique-index-and-deleting-duplicates-5cde + +class AddUniqueIndexOnInboxMembers < ActiveRecord::Migration[6.1] + def up + # partioning the duplicate records and then removing where more than one row is found + ActiveRecord::Base.connection.execute(' + DELETE FROM inbox_members WHERE id IN (SELECT id from ( + SELECT id, user_id, inbox_id, ROW_NUMBER() OVER w AS rnum FROM inbox_members WINDOW w AS ( + PARTITION BY inbox_id, user_id ORDER BY id + ) + ) t WHERE t.rnum > 1) + ') + add_index :inbox_members, [:inbox_id, :user_id], unique: true + end + + def down + remove_index :inbox_members, [:inbox_id, :user_id] + end +end diff --git a/db/schema.rb b/db/schema.rb index 52b7cd861..1429612cd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_12_19_031453) do +ActiveRecord::Schema.define(version: 2021_12_21_125545) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" @@ -429,6 +429,7 @@ ActiveRecord::Schema.define(version: 2021_12_19_031453) do t.integer "inbox_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index ["inbox_id", "user_id"], name: "index_inbox_members_on_inbox_id_and_user_id", unique: true t.index ["inbox_id"], name: "index_inbox_members_on_inbox_id" end 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 0bf5eba5a..b19035e77 100644 --- a/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/inbox_members_controller_spec.rb @@ -81,7 +81,7 @@ RSpec.describe 'Inbox Member API', type: :request do end it 'add inbox members' do - params = { inbox_id: inbox.id, user_ids: [agent_to_add.id] } + params = { inbox_id: inbox.id, user_ids: [old_agent.id, agent_to_add.id] } post "/api/v1/accounts/#{account.id}/inbox_members", headers: administrator.create_new_auth_token, diff --git a/spec/controllers/api/v1/accounts/team_members_controller_spec.rb b/spec/controllers/api/v1/accounts/team_members_controller_spec.rb index 6067ae1fb..967b8e79c 100644 --- a/spec/controllers/api/v1/accounts/team_members_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/team_members_controller_spec.rb @@ -55,6 +55,8 @@ RSpec.describe 'Team Members API', type: :request do it 'add a new team members when its administrator' do user_ids = (1..5).map { create(:user, account: account, role: :agent).id } params = { user_ids: user_ids } + # have a team member added already + create(:team_member, team: team, user: User.find(user_ids.first)) post "/api/v1/accounts/#{account.id}/teams/#{team.id}/team_members", params: params, @@ -63,7 +65,7 @@ RSpec.describe 'Team Members API', type: :request do expect(response).to have_http_status(:success) json_response = JSON.parse(response.body) - expect(json_response.count).to eq(user_ids.count) + expect(json_response.count).to eq(user_ids.count - 1) end end end @@ -93,15 +95,16 @@ RSpec.describe 'Team Members API', type: :request do it 'destroys the team members when its administrator' do user_ids = (1..5).map { create(:user, account: account, role: :agent).id } - params = { user_ids: user_ids } + user_ids.each { |id| create(:team_member, team: team, user: User.find(id)) } + params = { user_ids: user_ids.first(3) } - delete "/api/v1/accounts/#{account.id}/teams/#{team.id}", + delete "/api/v1/accounts/#{account.id}/teams/#{team.id}/team_members", params: params, headers: administrator.create_new_auth_token, as: :json expect(response).to have_http_status(:success) - expect(team.team_members.count).to eq(0) + expect(team.team_members.count).to eq(2) end end end diff --git a/spec/models/concerns/assignment_handler_shared.rb b/spec/models/concerns/assignment_handler_shared.rb index 485d01411..5c74b81d2 100644 --- a/spec/models/concerns/assignment_handler_shared.rb +++ b/spec/models/concerns/assignment_handler_shared.rb @@ -78,19 +78,9 @@ shared_examples_for 'assignment_handler' do expect(conversation.reload.assignee).to eq(agent) end - it 'creates a new notification for the agent' do + it 'dispaches assignee changed event' do + expect(EventDispatcherJob).to(have_been_enqueued.at_least(:once).with('assignee.changed', anything, anything)) expect(update_assignee).to eq(true) - expect(agent.notifications.count).to eq(1) - end - - it 'does not create assignment notification if notification setting is turned off' do - notification_setting = agent.notification_settings.first - notification_setting.unselect_all_email_flags - notification_setting.unselect_all_push_flags - notification_setting.save! - - expect(update_assignee).to eq(true) - expect(agent.notifications.count).to eq(0) end context 'when agent is current user' do