From 3dae3ff3ad75baab8ecafda49e1c556e14144acc Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Thu, 14 Mar 2024 17:22:32 +0530 Subject: [PATCH] feat: Conversation update API for sla_policy_id (#8973) - Add an endpoint for updating conversation attributes (priority / sla_policy_id ) - Swagger spec - minor chores around the conversation API/spec Fixes: https://linear.app/chatwoot/issue/CW-2100/feat-backend-api-to-update-the-sla-of-a-conversation --- .../v1/accounts/conversations_controller.rb | 11 ++++ .../concerns/access_token_auth_helper.rb | 2 +- .../conversations/update.json.jbuilder | 1 + .../partials/_conversation.json.jbuilder | 1 + config/routes.rb | 2 +- .../v1/accounts/conversations_controller.rb | 5 ++ enterprise/app/models/applied_sla.rb | 7 ++ .../enterprise_conversation_concern.rb | 30 +++++++++ enterprise/app/models/sla_policy.rb | 1 + .../app/services/enterprise/action_service.rb | 11 ---- .../accounts/conversations_controller_spec.rb | 64 ++++++++++++++----- .../accounts/conversations_controller_spec.rb | 41 ++++++++++++ spec/enterprise/models/conversation_spec.rb | 62 ++++++++++++++---- .../sla/evaluate_applied_sla_service_spec.rb | 19 ++++-- .../paths/application/conversation/update.yml | 29 +++++++++ swagger/paths/index.yml | 2 + swagger/swagger.json | 58 +++++++++++++++++ 17 files changed, 301 insertions(+), 45 deletions(-) create mode 100644 app/views/api/v1/accounts/conversations/update.json.jbuilder create mode 100644 enterprise/app/controllers/enterprise/api/v1/accounts/conversations_controller.rb create mode 100644 spec/enterprise/controllers/enterprise/api/v1/accounts/conversations_controller_spec.rb create mode 100644 swagger/paths/application/conversation/update.yml diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index 281ff95de..d0d8f6d5b 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -36,6 +36,10 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro end end + def update + @conversation.update!(permitted_update_params) + end + def filter result = ::Conversations::FilterService.new(params.permit!, current_user).perform @conversations = result[:conversations] @@ -110,6 +114,11 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro private + def permitted_update_params + # TODO: Move the other conversation attributes to this method and remove specific endpoints for each attribute + params.permit(:priority) + end + def update_last_seen_on_conversation(last_seen_at, update_assignee) # rubocop:disable Rails/SkipsModelValidations @conversation.update_column(:agent_last_seen_at, last_seen_at) @@ -176,3 +185,5 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro @conversation.assignee_id? && Current.user == @conversation.assignee end end + +Api::V1::Accounts::ConversationsController.prepend_mod_with('Api::V1::Accounts::ConversationsController') diff --git a/app/controllers/concerns/access_token_auth_helper.rb b/app/controllers/concerns/access_token_auth_helper.rb index cd760f7ce..2ee9f9854 100644 --- a/app/controllers/concerns/access_token_auth_helper.rb +++ b/app/controllers/concerns/access_token_auth_helper.rb @@ -1,6 +1,6 @@ module AccessTokenAuthHelper BOT_ACCESSIBLE_ENDPOINTS = { - 'api/v1/accounts/conversations' => %w[toggle_status toggle_priority create], + 'api/v1/accounts/conversations' => %w[toggle_status toggle_priority create update], 'api/v1/accounts/conversations/messages' => ['create'], 'api/v1/accounts/conversations/assignments' => ['create'] }.freeze diff --git a/app/views/api/v1/accounts/conversations/update.json.jbuilder b/app/views/api/v1/accounts/conversations/update.json.jbuilder new file mode 100644 index 000000000..c273dd3c6 --- /dev/null +++ b/app/views/api/v1/accounts/conversations/update.json.jbuilder @@ -0,0 +1 @@ +json.partial! 'api/v1/conversations/partials/conversation', formats: [:json], conversation: @conversation diff --git a/app/views/api/v1/conversations/partials/_conversation.json.jbuilder b/app/views/api/v1/conversations/partials/_conversation.json.jbuilder index cef56fed4..2e90e073a 100644 --- a/app/views/api/v1/conversations/partials/_conversation.json.jbuilder +++ b/app/views/api/v1/conversations/partials/_conversation.json.jbuilder @@ -47,3 +47,4 @@ json.last_non_activity_message conversation.messages.where(account_id: conversat json.last_activity_at conversation.last_activity_at.to_i json.priority conversation.priority json.waiting_since conversation.waiting_since.to_i.to_i +json.sla_policy_id conversation.sla_policy_id diff --git a/config/routes.rb b/config/routes.rb index 90317c7b2..cfa14c854 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -78,7 +78,7 @@ Rails.application.routes.draw do namespace :channels do resource :twilio_channel, only: [:create] end - resources :conversations, only: [:index, :create, :show] do + resources :conversations, only: [:index, :create, :show, :update] do collection do get :meta get :search diff --git a/enterprise/app/controllers/enterprise/api/v1/accounts/conversations_controller.rb b/enterprise/app/controllers/enterprise/api/v1/accounts/conversations_controller.rb new file mode 100644 index 000000000..be8dfbddf --- /dev/null +++ b/enterprise/app/controllers/enterprise/api/v1/accounts/conversations_controller.rb @@ -0,0 +1,5 @@ +module Enterprise::Api::V1::Accounts::ConversationsController + def permitted_update_params + super.merge(params.permit(:sla_policy_id)) + end +end diff --git a/enterprise/app/models/applied_sla.rb b/enterprise/app/models/applied_sla.rb index cb5ca508d..48fd852e4 100644 --- a/enterprise/app/models/applied_sla.rb +++ b/enterprise/app/models/applied_sla.rb @@ -23,6 +23,13 @@ class AppliedSla < ApplicationRecord belongs_to :conversation validates :account_id, uniqueness: { scope: %i[sla_policy_id conversation_id] } + before_validation :ensure_account_id enum sla_status: { active: 0, hit: 1, missed: 2 } + + private + + def ensure_account_id + self.account_id ||= sla_policy&.account_id + end end diff --git a/enterprise/app/models/enterprise/enterprise_conversation_concern.rb b/enterprise/app/models/enterprise/enterprise_conversation_concern.rb index fc39a61a9..721f069dc 100644 --- a/enterprise/app/models/enterprise/enterprise_conversation_concern.rb +++ b/enterprise/app/models/enterprise/enterprise_conversation_concern.rb @@ -3,5 +3,35 @@ module Enterprise::EnterpriseConversationConcern included do belongs_to :sla_policy, optional: true + has_one :applied_sla, dependent: :destroy + before_validation :validate_sla_policy, if: -> { sla_policy_id_changed? } + around_save :ensure_applied_sla_is_created, if: -> { sla_policy_id_changed? } + end + + private + + def validate_sla_policy + # TODO: remove these validations once we figure out how to deal with these cases + if sla_policy_id.nil? && changes[:sla_policy_id].first.present? + errors.add(:sla_policy, 'cannot remove sla policy from conversation') + return + end + + if changes[:sla_policy_id].first.present? + errors.add(:sla_policy, 'conversation already has a different sla') + return + end + + errors.add(:sla_policy, 'sla policy account mismatch') if sla_policy&.account_id != account_id + end + + # handling inside a transaction to ensure applied sla record is also created + def ensure_applied_sla_is_created + ActiveRecord::Base.transaction do + yield + create_applied_sla(sla_policy_id: sla_policy_id) if applied_sla.blank? + end + rescue ActiveRecord::RecordInvalid + raise ActiveRecord::Rollback end end diff --git a/enterprise/app/models/sla_policy.rb b/enterprise/app/models/sla_policy.rb index 647db2cfc..f53f00ed5 100644 --- a/enterprise/app/models/sla_policy.rb +++ b/enterprise/app/models/sla_policy.rb @@ -22,6 +22,7 @@ class SlaPolicy < ApplicationRecord validates :name, presence: true has_many :conversations, dependent: :nullify + has_many :applied_slas, dependent: :destroy def push_event_data { diff --git a/enterprise/app/services/enterprise/action_service.rb b/enterprise/app/services/enterprise/action_service.rb index 1e4165b09..f0c3bbf9f 100644 --- a/enterprise/app/services/enterprise/action_service.rb +++ b/enterprise/app/services/enterprise/action_service.rb @@ -8,16 +8,5 @@ module Enterprise::ActionService Rails.logger.info "SLA:: Adding SLA #{sla_policy.id} to conversation: #{@conversation.id}" @conversation.update!(sla_policy_id: sla_policy.id) - create_applied_sla(sla_policy) - end - - def create_applied_sla(sla_policy) - Rails.logger.info "SLA:: Creating Applied SLA for conversation: #{@conversation.id}" - AppliedSla.create!( - account_id: @conversation.account_id, - sla_policy_id: sla_policy.id, - conversation_id: @conversation.id, - sla_status: 'active' - ) end end diff --git a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb index 72e6603f7..70f5b82ee 100644 --- a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb @@ -210,6 +210,55 @@ RSpec.describe 'Conversations API', type: :request do end end + describe 'PATCH /api/v1/accounts/{account.id}/conversations/:id' do + let(:conversation) { create(:conversation, account: account) } + let(:params) { { priority: 'high' } } + + context 'when it is an unauthenticated user' do + it 'returns unauthorized' do + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated user' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:administrator) { create(:user, account: account, role: :administrator) } + + it 'does not update the conversation if you do not have access to it' do + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params, + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:unauthorized) + end + + it 'updates the conversation if you are an administrator' do + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params, + headers: administrator.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(JSON.parse(response.body, symbolize_names: true)[:priority]).to eq('high') + end + + it 'updates the conversation if you are an agent with access to inbox' do + create(:inbox_member, user: agent, inbox: conversation.inbox) + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params, + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(JSON.parse(response.body, symbolize_names: true)[:priority]).to eq('high') + end + end + end + describe 'POST /api/v1/accounts/{account.id}/conversations' do let(:contact) { create(:contact, account: account) } let(:inbox) { create(:inbox, account: account) } @@ -411,21 +460,6 @@ RSpec.describe 'Conversations API', type: :request do expect(conversation.reload.status).to eq('snoozed') expect(conversation.reload.snoozed_until.to_i).to eq(snoozed_until) end - - # TODO: remove this spec when we remove the condition check in controller - # Added for backwards compatibility for bot status - # remove in next release - # it 'toggles the conversation status to pending status when parameter bot is passed' do - # expect(conversation.status).to eq('open') - - # post "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}/toggle_status", - # headers: agent.create_new_auth_token, - # params: { status: 'bot' }, - # as: :json - - # expect(response).to have_http_status(:success) - # expect(conversation.reload.status).to eq('pending') - # end end context 'when it is an authenticated bot' do diff --git a/spec/enterprise/controllers/enterprise/api/v1/accounts/conversations_controller_spec.rb b/spec/enterprise/controllers/enterprise/api/v1/accounts/conversations_controller_spec.rb new file mode 100644 index 000000000..fb028b76f --- /dev/null +++ b/spec/enterprise/controllers/enterprise/api/v1/accounts/conversations_controller_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe 'Enterprise Conversations API', type: :request do + let(:account) { create(:account) } + let(:admin) { create(:user, account: account, role: :administrator) } + + describe 'PATCH /api/v1/accounts/{account.id}/conversations/:id' do + let(:conversation) { create(:conversation, account: account) } + let(:sla_policy) { create(:sla_policy, account: account) } + let(:params) { { sla_policy_id: sla_policy.id } } + + context 'when it is an authenticated user' do + let(:agent) { create(:user, account: account, role: :agent) } + + before do + create(:inbox_member, user: agent, inbox: conversation.inbox) + end + + it 'updates the conversation if you are an agent with access to inbox' do + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params, + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(JSON.parse(response.body, symbolize_names: true)[:sla_policy_id]).to eq(sla_policy.id) + end + + it 'throws error if conversation already has a different sla' do + conversation.update(sla_policy: create(:sla_policy, account: account)) + patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + params: params, + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:unprocessable_entity) + expect(JSON.parse(response.body, symbolize_names: true)[:message]).to eq('Sla policy conversation already has a different sla') + end + end + end +end diff --git a/spec/enterprise/models/conversation_spec.rb b/spec/enterprise/models/conversation_spec.rb index bb3ced5b3..e9fe811ca 100644 --- a/spec/enterprise/models/conversation_spec.rb +++ b/spec/enterprise/models/conversation_spec.rb @@ -7,10 +7,10 @@ RSpec.describe Conversation, type: :model do describe 'SLA policy updates' do let!(:conversation) { create(:conversation) } - let!(:sla_policy) { create(:sla_policy) } + let!(:sla_policy) { create(:sla_policy, account: conversation.account) } it 'generates an activity message when the SLA policy is updated' do - conversation.update(sla_policy_id: sla_policy.id) + conversation.update!(sla_policy_id: sla_policy.id) perform_enqueued_jobs @@ -21,18 +21,19 @@ RSpec.describe Conversation, type: :model do expect(activity_message.content).to include('added SLA policy') end - it 'generates an activity message when the SLA policy is removed' do - conversation.update(sla_policy_id: sla_policy.id) - conversation.update(sla_policy_id: nil) + # TODO: Reenable this when we let the SLA policy be removed from a conversation + # it 'generates an activity message when the SLA policy is removed' do + # conversation.update!(sla_policy_id: sla_policy.id) + # conversation.update!(sla_policy_id: nil) - perform_enqueued_jobs + # perform_enqueued_jobs - activity_message = conversation.messages.where(message_type: 'activity').last + # activity_message = conversation.messages.where(message_type: 'activity').last - expect(activity_message).not_to be_nil - expect(activity_message.message_type).to eq('activity') - expect(activity_message.content).to include('removed SLA policy') - end + # expect(activity_message).not_to be_nil + # expect(activity_message.message_type).to eq('activity') + # expect(activity_message.content).to include('removed SLA policy') + # end end describe 'conversation sentiments' do @@ -64,4 +65,43 @@ RSpec.describe Conversation, type: :model do expect(sentiments[:label]).to eq('positive') end end + + describe 'sla_policy' do + let(:account) { create(:account) } + let(:conversation) { create(:conversation, account: account) } + let(:sla_policy) { create(:sla_policy, account: account) } + let(:different_account_sla_policy) { create(:sla_policy) } + + context 'when sla_policy is getting updated' do + it 'throws error if sla policy belongs to different account' do + conversation.sla_policy = different_account_sla_policy + expect(conversation.valid?).to be false + expect(conversation.errors[:sla_policy]).to include('sla policy account mismatch') + end + + it 'creates applied sla record if sla policy is present' do + conversation.sla_policy = sla_policy + conversation.save! + expect(conversation.applied_sla.sla_policy_id).to eq(sla_policy.id) + end + end + + context 'when conversation already has a different sla' do + before do + conversation.update(sla_policy: create(:sla_policy, account: account)) + end + + it 'throws error if trying to assing a different sla' do + conversation.sla_policy = sla_policy + expect(conversation.valid?).to be false + expect(conversation.errors[:sla_policy]).to eq(['conversation already has a different sla']) + end + + it 'throws error if trying to set sla to nil' do + conversation.sla_policy = nil + expect(conversation.valid?).to be false + expect(conversation.errors[:sla_policy]).to eq(['cannot remove sla policy from conversation']) + end + end + end end diff --git a/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb b/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb index 17c09cb0e..12cb59d35 100644 --- a/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb +++ b/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb @@ -5,14 +5,21 @@ RSpec.describe Sla::EvaluateAppliedSlaService do let!(:user_1) { create(:user, account: account) } let!(:user_2) { create(:user, account: account) } let!(:admin) { create(:user, account: account, role: :administrator) } - let!(:conversation) { create(:conversation, created_at: 6.hours.ago, assignee: user_1, account: account) } + let!(:sla_policy) do - create(:sla_policy, account: conversation.account, - first_response_time_threshold: nil, - next_response_time_threshold: nil, - resolution_time_threshold: nil) + create(:sla_policy, + account: account, + first_response_time_threshold: nil, + next_response_time_threshold: nil, + resolution_time_threshold: nil) end - let!(:applied_sla) { create(:applied_sla, conversation: conversation, sla_policy: sla_policy, sla_status: 'active') } + let!(:conversation) do + create(:conversation, + created_at: 6.hours.ago, assignee: user_1, + account: sla_policy.account, + sla_policy: sla_policy) + end + let!(:applied_sla) { conversation.applied_sla } describe '#perform - SLA misses' do context 'when first response SLA is missed' do diff --git a/swagger/paths/application/conversation/update.yml b/swagger/paths/application/conversation/update.yml new file mode 100644 index 000000000..3add02635 --- /dev/null +++ b/swagger/paths/application/conversation/update.yml @@ -0,0 +1,29 @@ +tags: + - Conversations +operationId: update-conversation +summary: Update Conversation +description: Update Conversation Attributes +security: + - userApiKey: [] + - agentBotApiKey: [] +parameters: + - name: data + in: body + required: true + schema: + type: object + properties: + priority: + type: string + enum: ["urgent", "high", "medium", "low", "none"] + description: "The priority of the conversation" + sla_policy_id: + type: number + description: "The ID of the SLA policy (Available only in Enterprise edition)" +responses: + 200: + description: Success + 404: + description: Conversation not found + 401: + description: Unauthorized diff --git a/swagger/paths/index.yml b/swagger/paths/index.yml index 2c6dd3f87..6df22d06e 100644 --- a/swagger/paths/index.yml +++ b/swagger/paths/index.yml @@ -339,6 +339,8 @@ - $ref: '#/parameters/conversation_id' get: $ref: ./application/conversation/show.yml + patch: + $ref: ./application/conversation/update.yml /api/v1/accounts/{account_id}/conversations/{conversation_id}/toggle_status: parameters: - $ref: '#/parameters/account_id' diff --git a/swagger/swagger.json b/swagger/swagger.json index ec88a224b..d5f94f730 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -3319,6 +3319,64 @@ "description": "Access denied" } } + }, + "patch": { + "tags": [ + "Conversations" + ], + "operationId": "update-conversation", + "summary": "Update Conversation", + "description": "Update Conversation Attributes", + "security": [ + { + "userApiKey": [ + + ] + }, + { + "agentBotApiKey": [ + + ] + } + ], + "parameters": [ + { + "name": "data", + "in": "body", + "required": true, + "schema": { + "type": "object", + "properties": { + "priority": { + "type": "string", + "enum": [ + "urgent", + "high", + "medium", + "low", + "none" + ], + "description": "The priority of the conversation" + }, + "sla_policy_id": { + "type": "number", + "description": "The ID of the SLA policy (Available only in Enterprise edition)" + } + } + } + } + ], + "responses": { + "200": { + "description": "Success" + }, + "404": { + "description": "Conversation not found" + }, + "401": { + "description": "Unauthorized" + } + } } }, "/api/v1/accounts/{account_id}/conversations/{conversation_id}/toggle_status": {