Maintenance: Improve Mentions error messages and error handling

app/controllers/concerns/creates_ticket_articles.rb

@@ -53,6 +53,7 @@ def article_create(ticket, params)
 
     article = Ticket::Article.new(clean_params)
     article.ticket_id = ticket.id
+    article.check_mentions_raises_error = true
 
     # store dataurl images to store
     attachments_inline = []

app/controllers/tickets_controller.rb

@@ -174,11 +174,13 @@ def create
         end
       end
 
-      # create mentions if given
+      # This mentions handling is used by custom API calls only
+      # Mentions created in UI are handled by Ticket::Article#check_mentions
       if params[:mentions].present?
         authorize!(ticket, :create_mentions?)
+
         Array(params[:mentions]).each do |user_id|
-          Mention.where(mentionable: ticket, user_id: user_id).first_or_create(mentionable: ticket, user_id: user_id)
+          Mention.subscribe! ticket, User.find(user_id)
         end
       end
 

app/models/mention.rb

@@ -66,9 +66,7 @@ def self.subscribed?(object, user)
   # @param user
   # @return Boolean
   def self.subscribe!(object, user)
-    object
-      .mentions
-      .find_or_create_by! user: user
+    object.mentions.create!(user: user) if !subscribed?(object, user)
 
     true
   end

app/models/ticket/article.rb

@@ -63,7 +63,7 @@ class Ticket::Article < ApplicationModel
                              :to,
                              :cc
 
-  attr_accessor :should_clone_inline_attachments
+  attr_accessor :should_clone_inline_attachments, :check_mentions_raises_error
 
   alias should_clone_inline_attachments? should_clone_inline_attachments
 
@@ -357,16 +357,26 @@ def check_mentions
 
     return if mention_user_ids.blank?
 
-    if !TicketPolicy.new(updated_by, ticket).create_mentions?
+    begin
+      Pundit.authorize updated_by, ticket, :create_mentions?
+    rescue Pundit::NotAuthorizedError => e
       return if ApplicationHandleInfo.postmaster?
       return if updated_by.id == 1
+      return if !check_mentions_raises_error
 
-      raise "User #{updated_by_id} has no permission to mention other users!"
+      raise e
     end
 
-    user_ids = User.where(id: mention_user_ids).pluck(:id)
-    user_ids.each do |user_id|
-      Mention.where(mentionable: ticket, user_id: user_id).first_or_create(mentionable: ticket, user_id: user_id)
+    mention_user_ids.each do |user_id|
+      begin
+        Mention.subscribe! ticket, User.find(user_id)
+      rescue ActiveRecord::RecordNotFound, ActiveRecord::RecordInvalid => e
+        next if ApplicationHandleInfo.postmaster?
+        next if updated_by.id == 1
+        next if !check_mentions_raises_error
+
+        raise e
+      end
     end
   end
 

app/policies/ticket_policy.rb

@@ -58,7 +58,9 @@ def agent_update_access?
   end
 
   def create_mentions?
-    agent_read_access?
+    return true if agent_read_access?
+
+    not_authorized __('You have insufficient permissions to mention other users.')
   end
 
   private

app/services/service/ticket/article/create.rb

@@ -11,6 +11,8 @@ def execute(article_data:, ticket:)
     preprocess_article_data(article_data, ticket)
 
     ticket.articles.new(article_data).tap do |article|
+      article.check_mentions_raises_error = true
+
       transform_article(article, attachments_raw, subtype)
 
       article.save!

i18n/zammad.pot

@@ -509,6 +509,10 @@ msgstr ""
 msgid "A list of active import backends that gets scheduled automatically."
 msgstr ""
 
+#: lib/validations/mention_validator.rb
+msgid "A mentioned user has no agent access to this ticket"
+msgstr ""
+
 #: app/assets/javascripts/app/controllers/_plugin/session_taken_over.coffee
 msgid "A new session was created with your account. This session will be stopped to prevent a conflict."
 msgstr ""
@@ -14386,6 +14390,10 @@ msgstr ""
 msgid "You have exceeded the character limit by %s"
 msgstr ""
 
+#: app/policies/ticket_policy.rb
+msgid "You have insufficient permissions to mention other users."
+msgstr ""
+
 #: app/frontend/apps/mobile/entities/organization/composables/useOrganizationDetail.ts
 msgid "You have insufficient rights to view this organization."
 msgstr ""
@@ -15065,10 +15073,6 @@ msgstr ""
 msgid "has changed"
 msgstr ""
 
-#: lib/validations/mention_validator.rb
-msgid "has no agent access to this ticket"
-msgstr ""
-
 #: app/assets/javascripts/app/controllers/_ui_element/_application_selector.coffee
 #: app/assets/javascripts/app/controllers/_ui_element/object_selector.coffee
 msgid "has reached"

lib/validations/mention_validator.rb

@@ -4,6 +4,6 @@ class Validations::MentionValidator < ActiveModel::Validator
   def validate(record)
     return if Mention.mentionable? record.mentionable, record.user
 
-    record.errors.add :user, __('has no agent access to this ticket')
+    record.errors.add :base, __('A mentioned user has no agent access to this ticket')
   end
 end

spec/models/mention_spec.rb

@@ -8,7 +8,10 @@
 
   describe 'validation' do
     it 'does not allow mentions for customers' do
-      expect { create(:mention, mentionable: ticket, user: create(:customer)) }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: User has no agent access to this ticket')
+      record = build(:mention, mentionable: ticket, user: create(:customer))
+      record.save
+
+      expect(record.errors.full_messages).to include('A mentioned user has no agent access to this ticket')
     end
   end
 

spec/models/ticket/article_spec.rb

@@ -523,6 +523,145 @@
         end
       end
     end
+
+    describe '#check_mentions' do
+      def text_blob_with(user)
+        "Lorem ipsum dolor <a data-mention-user-id='#{user.id}'>#{user.fullname}</a>"
+      end
+
+      let(:ticket)              { create(:ticket) }
+      let(:agent_with_access)   { create(:agent, groups: [ticket.group]) }
+      let(:user_without_access) { create(:agent) }
+
+      let(:passing_body) { text_blob_with(agent_with_access) }
+      let(:failing_body) { text_blob_with(user_without_access) }
+      let(:partial_body) { text_blob_with(user_without_access) + text_blob_with(agent_with_access) }
+
+      context 'when created in email parsing' do
+        before { ApplicationHandleInfo.current = 'postmaster' }
+
+        it 'silently ignores mentions if agent cannot mention users' do
+          UserInfo.current_user_id = user_without_access.id
+          record = create(:ticket_article, body: passing_body)
+
+          expect(record).to be_persisted
+          expect(Mention.count).to be_zero
+        end
+
+        it 'silently ignores mentions if given users cannot be mentioned' do
+          UserInfo.current_user_id = agent_with_access.id
+          article = build(:ticket_article, ticket: ticket, body: failing_body)
+          article.save
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'silently saves passing user while failing user is skipped' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = create(:ticket_article, ticket: ticket, body: partial_body)
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(1)
+        end
+
+        it 'mentioned user is added' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          create(:ticket_article, ticket: ticket, body: passing_body)
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(1)
+        end
+      end
+
+      context 'when created with check_mentions_raises_error set to true' do
+        it 'raises an error if agent cannot mention users' do
+          UserInfo.current_user_id = create(:customer).id
+
+          article = build(:ticket_article, ticket: ticket, body: passing_body)
+          article.check_mentions_raises_error = true
+
+          expect { article.save! }
+            .to raise_error(Pundit::NotAuthorizedError)
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'raises an error if given users cannot be mentioned' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = build(:ticket_article, ticket: ticket, body: failing_body)
+          article.check_mentions_raises_error = true
+
+          expect { article.save! }
+            .to raise_error(ActiveRecord::RecordInvalid)
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'raises an error if one if given users cannot be mentioned' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = build(:ticket_article, ticket: ticket, body: partial_body)
+          article.check_mentions_raises_error = true
+
+          expect { article.save! }
+            .to raise_error(ActiveRecord::RecordInvalid)
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'mentioned user is added' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = build(:ticket_article, ticket: ticket, body: passing_body)
+          article.check_mentions_raises_error = true
+          article.save!
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(1)
+        end
+      end
+
+      context 'when created with check_mentions_raises_error set to false' do
+        it 'silently ignores mentions if agent cannot mention users' do
+          UserInfo.current_user_id = create(:customer).id
+
+          article = build(:ticket_article, ticket: ticket, body: failing_body)
+          article.save
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'silently ignores mentions if given users cannot be mentioned' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = build(:ticket_article, ticket: ticket, body: failing_body)
+          article.save
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(0)
+        end
+
+        it 'silently saves passing user while failing user is skipped' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          article = create(:ticket_article, ticket: ticket, body: partial_body)
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(1)
+        end
+
+        it 'mentioned user is added' do
+          UserInfo.current_user_id = agent_with_access.id
+
+          create(:ticket_article, ticket: ticket, body: passing_body)
+
+          expect(article).to be_persisted
+          expect(Mention.count).to eq(1)
+        end
+      end
+    end
   end
 
   describe 'clone attachments' do

spec/requests/ticket/article_spec.rb

@@ -493,6 +493,21 @@
       expect(Mention.where(mentionable: Ticket.last).count).to eq(1)
     end
 
+    it 'does not ticket create with invalid mentions' do
+      params = {
+        title:       'a new ticket #1',
+        group:       'Users',
+        customer_id: customer.id,
+        article:     {
+          body: "some body <a data-mention-user-id=\"#{create(:customer).id}\">customer</a>",
+        }
+      }
+      authenticated_as(agent)
+      post '/api/v1/tickets', params: params, as: :json
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(Mention.count).to eq(0)
+    end
+
     it 'does not ticket create with mentions when customer' do
       params = {
         title:       'a new ticket #1',
@@ -504,7 +519,7 @@
       }
       authenticated_as(customer)
       post '/api/v1/tickets', params: params, as: :json
-      expect(response).to have_http_status(:internal_server_error)
+      expect(response).to have_http_status(:forbidden)
       expect(Mention.count).to eq(0)
     end
   end

spec/requests/ticket_spec.rb

@@ -2200,7 +2200,7 @@
     let(:user2) { create(:agent, groups: [ticket_group]) }
     let(:user3) { create(:agent, groups: [ticket_group]) }
 
-    def new_ticket_with_mentions
+    def new_ticket_with_mentions(*user_ids)
       params = {
         title:    'a new ticket #11',
         group:    ticket_group.name,
@@ -2212,22 +2212,28 @@ def new_ticket_with_mentions
         article:  {
           body: 'some test 123',
         },
-        mentions: [user1.id, user2.id, user3.id]
+        mentions: user_ids
       }
       authenticated_as(agent)
       post '/api/v1/tickets', params: params, as: :json
-      expect(response).to have_http_status(:created)
 
       json_response
     end
 
     it 'create ticket with mentions' do
-      new_ticket_with_mentions
+      new_ticket_with_mentions(user1.id, user2.id, user3.id)
+      expect(response).to have_http_status(:created)
       expect(Mention.count).to eq(3)
     end
 
+    it 'create ticket with one of mentions being invalid' do
+      new_ticket_with_mentions(user1.id, user2.id, create(:customer).id)
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(Mention.count).to eq(0)
+    end
+
     it 'check ticket get' do
-      ticket = new_ticket_with_mentions
+      ticket = new_ticket_with_mentions(user1.id, user2.id, user3.id)
 
       get "/api/v1/tickets/#{ticket['id']}?all=true", params: {}, as: :json
       expect(response).to have_http_status(:ok)