chore: API improvements (#3637)

- Unique validations for Inbox members and Team member objects
- Move notification processing to Async

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

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
 

app/dispatchers/async_dispatcher.rb

@@ -15,6 +15,7 @@ class AsyncDispatcher < BaseDispatcher
       EventListener.instance,
       HookListener.instance,
       InstallationWebhookListener.instance,
+      NotificationListener.instance,
       WebhookListener.instance
     ]
   end

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

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

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

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

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

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

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
 

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,

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

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