feat: Conversation update API for sla_policy_id (chatwoot#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

app/controllers/api/v1/accounts/conversations_controller.rb

@@ -36,6 +36,10 @@ def create
     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 @@ def custom_attributes
 
   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 @@ def assignee?
     @conversation.assignee_id? && Current.user == @conversation.assignee
   end
 end
+
+Api::V1::Accounts::ConversationsController.prepend_mod_with('Api::V1::Accounts::ConversationsController')

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

app/views/api/v1/accounts/conversations/update.json.jbuilder

@@ -0,0 +1 @@
+json.partial! 'api/v1/conversations/partials/conversation', formats: [:json], conversation: @conversation

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

config/routes.rb

@@ -78,7 +78,7 @@
           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

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

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

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

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
     {

enterprise/app/services/enterprise/action_service.rb

@@ -8,16 +8,5 @@ def add_sla(sla_policy_id)
 
     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

spec/controllers/api/v1/accounts/conversations_controller_spec.rb

@@ -210,6 +210,55 @@
     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 @@
         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

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

spec/enterprise/models/conversation_spec.rb

@@ -7,10 +7,10 @@
 
   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 @@
       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 @@
       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

spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb

@@ -5,14 +5,21 @@
   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!(:conversation) do
+    create(:conversation,
+           created_at: 6.hours.ago, assignee: user_1,
+           account: sla_policy.account,
+           sla_policy: sla_policy)
   end
-  let!(:applied_sla) { create(:applied_sla, conversation: conversation, sla_policy: sla_policy, sla_status: 'active') }
+  let!(:applied_sla) { conversation.applied_sla }
 
   describe '#perform - SLA misses' do
     context 'when first response SLA is missed' do

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