Merge branch 'sdesk-name-use' into 'master'

Use custom user name for service desk emails

See merge request gitlab-org/gitlab!22090

app/mailers/emails/projects.rb

@@ -51,7 +51,7 @@ module Emails
       add_project_headers
       headers['X-GitLab-Author'] = @message.author_username
 
-      mail(from:      sender(@message.author_id, @message.send_from_committer_email?),
+      mail(from:      sender(@message.author_id, send_from_user_email: @message.send_from_committer_email?),
            reply_to:  @message.reply_to,
            subject:   @message.subject)
     end

app/mailers/notify.rb

@@ -59,11 +59,11 @@ class Notify < BaseMailer
 
   # Return an email address that displays the name of the sender.
   # Only the displayed name changes; the actual email address is always the same.
-  def sender(sender_id, send_from_user_email = false)
+  def sender(sender_id, send_from_user_email: false, sender_name: nil)
     return unless sender = User.find(sender_id)
 
     address = default_sender_address
-    address.display_name = sender.name
+    address.display_name = sender_name.presence || sender.name
 
     if send_from_user_email && can_send_from_user_email?(sender)
       address.address = sender.email

ee/app/controllers/projects/service_desk_controller.rb

@@ -10,13 +10,21 @@ class Projects::ServiceDeskController < Projects::ApplicationController
   def update
     Projects::UpdateService.new(project, current_user, { service_desk_enabled: params[:service_desk_enabled] }).execute
 
-    ServiceDeskSetting.update_template_key_for(project: project, issue_template_key: params[:issue_template_key])
+    result = ServiceDeskSettings::UpdateService.new(project, current_user, setting_params).execute
 
-    json_response
+    if result[:status] == :success
+      json_response
+    else
+      render json: { message: result[:message] }, status: :unprocessable_entity
+    end
   end
 
   private
 
+  def setting_params
+    params.permit(:issue_template_key, :outgoing_name)
+  end
+
   def json_response
     respond_to do |format|
       service_desk_settings = project.service_desk_setting
@@ -26,7 +34,8 @@ class Projects::ServiceDeskController < Projects::ApplicationController
           service_desk_address: project.service_desk_address,
           service_desk_enabled: project.service_desk_enabled,
           issue_template_key: service_desk_settings&.issue_template_key,
-          template_file_missing: service_desk_settings&.issue_template_missing?
+          template_file_missing: service_desk_settings&.issue_template_missing?,
+          outgoing_name: service_desk_settings&.outgoing_name
         }
 
       format.json { render json: service_desk_attributes }

ee/app/mailers/emails/service_desk.rb

@@ -11,13 +11,26 @@ module Emails
     def service_desk_thank_you_email(issue_id)
       setup_service_desk_mail(issue_id)
 
-      mail_new_thread(@issue, service_desk_options(@support_bot.id).merge(subject: "Re: #{@issue.title} (##{@issue.iid})"))
+      options = {
+        from: sender(@support_bot.id, send_from_user_email: false, sender_name: @project.service_desk_setting&.outgoing_name),
+        to: @issue.service_desk_reply_to,
+        subject: "Re: #{@issue.title} (##{@issue.iid})"
+      }
+
+      mail_new_thread(@issue, options)
     end
 
     def service_desk_new_note_email(issue_id, note_id)
       @note = Note.find(note_id)
       setup_service_desk_mail(issue_id)
-      mail_answer_thread(@issue, service_desk_options(@note.author_id).merge(subject: "#{@issue.title} (##{@issue.iid})"))
+
+      options = {
+        from: sender(@note.author_id),
+        to: @issue.service_desk_reply_to,
+        subject: "#{@issue.title} (##{@issue.iid})"
+      }
+
+      mail_answer_thread(@issue, options)
     end
 
     private
@@ -29,12 +42,5 @@ module Emails
 
       @sent_notification = SentNotification.record(@issue, @support_bot.id, reply_key)
     end
-
-    def service_desk_options(author_id)
-      {
-        from: sender(author_id),
-        to: @issue.service_desk_reply_to
-      }
-    end
   end
 end

ee/app/models/service_desk_setting.rb

@@ -8,15 +8,6 @@ class ServiceDeskSetting < ApplicationRecord
   validate :valid_issue_template
   validates :outgoing_name, length: { maximum: 255 }, allow_blank: true
 
-  def self.update_template_key_for(project:, issue_template_key:)
-    return unless issue_template_key
-
-    settings = safe_find_or_create_by!(project_id: project.id)
-    settings.update!(issue_template_key: issue_template_key)
-
-    settings
-  end
-
   def issue_template_content
     strong_memoize(:issue_template_content) do
       next unless issue_template_key.present?
@@ -32,7 +23,7 @@ class ServiceDeskSetting < ApplicationRecord
 
   def valid_issue_template
     if issue_template_missing?
-      errors.add(:issue_template_key, "Issue template empty or not found")
+      errors.add(:issue_template_key, "is empty or does not exist")
     end
   end
 end

ee/app/services/service_desk_settings/update_service.rb

+# frozen_string_literal: true
+
+module ServiceDeskSettings
+  class UpdateService < BaseService
+    def execute
+      settings = ServiceDeskSetting.safe_find_or_create_by!(project_id: project.id)
+
+      if settings.update(params)
+        success
+      else
+        error(settings.errors.full_messages.to_sentence)
+      end
+    end
+  end
+end

ee/spec/controllers/projects/service_desk_controller_spec.rb

@@ -3,8 +3,12 @@
 require 'spec_helper'
 
 describe Projects::ServiceDeskController do
-  let(:project) { create(:project_empty_repo, :private, service_desk_enabled: true) }
-  let(:user)    { create(:user) }
+  let_it_be(:project) do
+    create(:project, :private, :custom_repo, service_desk_enabled: true,
+           files: { '.gitlab/issue_templates/service_desk.md' => 'template' })
+  end
+
+  let_it_be(:user) { create(:user) }
 
   before do
     allow(License).to receive(:feature_available?).and_call_original
@@ -40,9 +44,7 @@ describe Projects::ServiceDeskController do
 
     context 'when issue template is present' do
       it 'returns template_file_missing as false' do
-        template_path = '.gitlab/issue_templates/service_desk.md'
-        project.repository.create_file(user, template_path, 'text from template', message: 'message', branch_name: 'master')
-        ServiceDeskSetting.update_template_key_for(project: project, issue_template_key: 'service_desk')
+        create(:service_desk_setting, project: project, issue_template_key: 'service_desk')
 
         get :show, params: { namespace_id: project.namespace.to_param, project_id: project }, format: :json
 
@@ -67,7 +69,9 @@ describe Projects::ServiceDeskController do
     it 'toggles services desk incoming email' do
       project.update!(service_desk_enabled: false)
 
-      put :update, params: { namespace_id: project.namespace.to_param, project_id: project, service_desk_enabled: true }, format: :json
+      put :update, params: { namespace_id: project.namespace.to_param,
+                             project_id: project,
+                             service_desk_enabled: true }, format: :json
 
       expect(json_response["service_desk_address"]).to be_present
       expect(json_response["service_desk_enabled"]).to be_truthy
@@ -75,11 +79,9 @@ describe Projects::ServiceDeskController do
     end
 
     it 'sets issue_template_key' do
-      template_path = '.gitlab/issue_templates/service_desk.md'
-      project.repository.create_file(user, template_path, 'template text', message: 'message', branch_name: 'master')
-      ServiceDeskSetting.update_template_key_for(project: project, issue_template_key: 'service_desk')
-
-      put :update, params: { namespace_id: project.namespace.to_param, project_id: project, issue_template_key: 'service_desk' }, format: :json
+      put :update, params: { namespace_id: project.namespace.to_param,
+                             project_id: project,
+                             issue_template_key: 'service_desk' }, format: :json
 
       settings = project.service_desk_setting
       expect(settings).to be_present
@@ -88,6 +90,15 @@ describe Projects::ServiceDeskController do
       expect(json_response['issue_template_key']).to eq('service_desk')
     end
 
+    it 'returns an error when update of service desk settings fails' do
+      put :update, params: { namespace_id: project.namespace.to_param,
+                             project_id: project,
+                             issue_template_key: 'invalid key' }, format: :json
+
+      expect(response.status).to eq(422)
+      expect(json_response['message']).to eq('Issue template key is empty or does not exist')
+    end
+
     context 'when user cannot admin the project' do
       let(:other_user) { create(:user) }
 

ee/spec/factories/service_desk_settings.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :service_desk_setting do
+    project
+  end
+end

ee/spec/lib/gitlab/email/handler/ee/service_desk_handler_spec.rb

@@ -61,10 +61,12 @@ describe Gitlab::Email::Handler::EE::ServiceDeskHandler do
         end
 
         context 'and template is present' do
+          let_it_be(:settings) { create(:service_desk_setting, project: project) }
+
           def set_template_file(file_name, content)
             file_path = ".gitlab/issue_templates/#{file_name}.md"
             project.repository.create_file(user, file_path, content, message: 'message', branch_name: 'master')
-            ServiceDeskSetting.update_template_key_for(project: project, issue_template_key: file_name)
+            settings.update!(issue_template_key: file_name)
           end
 
           it 'appends template text to issue description' do

ee/spec/mailers/notify_spec.rb

@@ -68,6 +68,12 @@ describe Notify do
         issue.update!(service_desk_reply_to: 'service.desk@example.com')
       end
 
+      def expect_sender(username)
+        sender = subject.header[:from].addrs[0]
+        expect(sender.display_name).to eq(username)
+        expect(sender.address).to eq(gitlab_sender)
+      end
+
       describe 'thank you email' do
         subject { described_class.service_desk_thank_you_email(issue.id) }
 
@@ -83,6 +89,26 @@ describe Notify do
             is_expected.to have_body_text("Thank you for your support request! We are tracking your request as ticket #{issue.to_reference}, and will respond as soon as we can.")
           end
         end
+
+        it 'uses service bot name by default' do
+          expect_sender(User.support_bot.name)
+        end
+
+        context 'when custom outgoing name is set' do
+          let_it_be(:settings) { create(:service_desk_setting, project: project, outgoing_name: 'some custom name') }
+
+          it 'uses custom name in "from" header' do
+            expect_sender('some custom name')
+          end
+        end
+
+        context 'when custom outgoing name is empty' do
+          let_it_be(:settings) { create(:service_desk_setting, project: project, outgoing_name: '') }
+
+          it 'uses service bot name' do
+            expect_sender(User.support_bot.name)
+          end
+        end
       end
 
       describe 'new note email' do
@@ -96,6 +122,10 @@ describe Notify do
           is_expected.to deliver_to('service.desk@example.com')
         end
 
+        it 'uses author\'s name in "from" header' do
+          expect_sender(first_note.author.name)
+        end
+
         it 'has the correct subject and body' do
           aggregate_failures do
             is_expected.to have_referable_subject(issue, include_project: false, reply: true)

ee/spec/models/service_desk_setting_spec.rb

@@ -6,33 +6,26 @@ describe ServiceDeskSetting do
   describe 'validations' do
     it { is_expected.to validate_presence_of(:project_id) }
     it { is_expected.to validate_length_of(:outgoing_name).is_at_most(255) }
-  end
 
-  describe 'associations' do
-    it { is_expected.to belong_to(:project) }
-  end
+    describe '.valid_issue_template' do
+      let_it_be(:project) { create(:project, :custom_repo, files: { '.gitlab/issue_templates/service_desk.md' => 'template' }) }
 
-  describe '.update_template_key_for' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project, :repository) }
+      it 'is not valid if template does not exist' do
+        settings = build(:service_desk_setting, project: project, issue_template_key: 'invalid key')
 
-    context 'when template exists' do
-      it 'updates issue_template_key' do
-        template_path = '.gitlab/issue_templates/service_desk.md'
-        project.repository.create_file(user, template_path, 'Template text', message: 'message', branch_name: 'master')
+        expect(settings).not_to be_valid
+        expect(settings.errors[:issue_template_key].first).to eq('is empty or does not exist')
+      end
 
-        described_class.update_template_key_for(project: project, issue_template_key: 'service_desk')
+      it 'is valid if template exists' do
+        settings = build(:service_desk_setting, project: project, issue_template_key: 'service_desk')
 
-        expect(project.service_desk_setting.issue_template_key).to eq('service_desk')
+        expect(settings).to be_valid
       end
     end
+  end
 
-    context 'when template does not exist' do
-      it 'raises error' do
-        expect do
-          described_class.update_template_key_for(project: project, issue_template_key: 'unknown')
-        end.to raise_error(ActiveRecord::RecordInvalid)
-      end
-    end
+  describe 'associations' do
+    it { is_expected.to belong_to(:project) }
   end
 end

ee/spec/services/service_desk_settings/update_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe ServiceDeskSettings::UpdateService do
+  describe '#execute' do
+    let_it_be(:settings) { create(:service_desk_setting, outgoing_name: 'original name') }
+    let_it_be(:user) { create(:user) }
+
+    context 'with valid params' do
+      let(:params) { { outgoing_name: 'some name' } }
+
+      it 'updates service desk settings' do
+        result = described_class.new(settings.project, user, params).execute
+
+        expect(result[:status]).to eq :success
+        expect(settings.reload.outgoing_name).to eq 'some name'
+      end
+    end
+
+    context 'with invalid params' do
+      let(:params) { { outgoing_name: 'x' * 256 } }
+
+      it 'does not update service desk settings' do
+        result = described_class.new(settings.project, user, params).execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq 'Outgoing name is too long (maximum is 255 characters)'
+        expect(settings.reload.outgoing_name).to eq 'original name'
+      end
+    end
+  end
+end