Merge branch 'issue_2029' into 'master'

Simplify service desk project email

Closes #2029

See merge request !1668

app/models/ee/project.rb

@@ -14,8 +14,6 @@ module EE
 
       delegate :actual_shared_runners_minutes_limit,
         :shared_runners_minutes_used?, to: :namespace
-
-      before_validation :auto_refresh_service_desk_key
     end
 
     def shared_runners_available?
@@ -29,16 +27,10 @@ module EE
     def service_desk_address
       return nil unless service_desk_available?
 
-      refresh_service_desk_key! if service_desk_mail_key.blank?
-
-      from = "service_desk+#{service_desk_mail_key}"
-      ::Gitlab::IncomingEmail.reply_address(from)
-    end
-
-    def refresh_service_desk_key!
-      return unless service_desk_available?
+      config = ::Gitlab.config.incoming_email
+      wildcard = ::Gitlab::IncomingEmail::WILDCARD_PLACEHOLDER
 
-      self.service_desk_mail_key = SentNotification.reply_key
+      config.address&.gsub(wildcard, full_path)
     end
 
     private
@@ -47,11 +39,5 @@ module EE
       @service_desk_available ||=
         EE::Gitlab::ServiceDesk.enabled? && service_desk_enabled?
     end
-
-    def auto_refresh_service_desk_key
-      if service_desk_mail_key.blank? || service_desk_enabled_changed?
-        refresh_service_desk_key!
-      end
-    end
   end
 end

db/migrate/20170420200123_remove_service_desk_mail_key_from_projects.rb

+class RemoveServiceDeskMailKeyFromProjects < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    remove_column :projects, :service_desk_mail_key, :string
+  end
+end

db/schema.rb

@@ -1111,7 +1111,6 @@ ActiveRecord::Schema.define(version: 20170421113144) do
     t.boolean "printing_merge_request_link_enabled", default: true, null: false
     t.string "import_jid"
     t.boolean "service_desk_enabled"
-    t.string "service_desk_mail_key"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
@@ -1127,7 +1126,6 @@ ActiveRecord::Schema.define(version: 20170421113144) do
   add_index "projects", ["path"], name: "index_projects_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
   add_index "projects", ["pending_delete"], name: "index_projects_on_pending_delete", using: :btree
   add_index "projects", ["runners_token"], name: "index_projects_on_runners_token", using: :btree
-  add_index "projects", ["service_desk_mail_key"], name: "index_projects_on_service_desk_mail_key", unique: true, using: :btree
   add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree
   add_index "projects", ["sync_time"], name: "index_projects_on_sync_time", using: :btree
   add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree

lib/gitlab/email/handler/ee/service_desk_handler.rb

@@ -19,20 +19,17 @@ module Gitlab
           private
 
           def service_desk_key
-            @service_desk_key ||=
-              begin
-                mail_key =~ /\Aservice_desk[+](\w+)\z/
-                $1
-              end
+            return unless mail_key.include?("/")
+
+            mail_key
           end
 
           def project
             return @project if instance_variable_defined?(:@project)
 
-            @project = Project.find_by(
-              service_desk_enabled: true,
-              service_desk_mail_key: service_desk_key
-            )
+            @project =
+              Project.where(service_desk_enabled: true)
+                .find_by_full_path(service_desk_key)
           end
 
           def create_issue!

spec/controllers/projects/service_desk_controller_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Projects::ServiceDeskController do
   let(:project) { create(:project_empty_repo, :private) }
-  let(:user)    { create(:user, admin: true) }
+  let(:user)    { create(:user) }
 
   before do
     allow_any_instance_of(License).to receive(:add_on?).and_call_original
@@ -17,6 +17,7 @@ describe Projects::ServiceDeskController do
       get :show, namespace_id: project.namespace.to_param, project_id: project, format: :json
 
       body = JSON.parse(response.body)
+
       expect(body["service_desk_address"]).to match(/\A[^@]+@[^@]+\z/)
       expect(body["service_desk_enabled"]).to be_truthy
       expect(response.status).to eq(200)
@@ -38,15 +39,13 @@ describe Projects::ServiceDeskController do
 
   describe 'PUT service desk properties' do
     it 'toggles services desk incoming email' do
-      project.update(service_desk_enabled: true)
-      old_address = project.service_desk_address
       project.update(service_desk_enabled: false)
 
       put :update, namespace_id: project.namespace.to_param, project_id: project, service_desk_enabled: true, format: :json
 
       body = JSON.parse(response.body)
+
       expect(body["service_desk_address"]).to be_present
-      expect(body["service_desk_address"]).not_to eq(old_address)
       expect(body["service_desk_enabled"]).to be_truthy
       expect(response.status).to eq(200)
     end

spec/fixtures/emails/service_desk.eml

@@ -5,7 +5,7 @@ Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <incom
 Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
 Date: Thu, 13 Jun 2013 17:03:48 -0400
 From: Jake the Dog <jake@adventuretime.ooo>
-To: incoming+service_desk+somemailkey@appmail.adventuretime.ooo
+To: incoming+email/test@appmail.adventuretime.ooo
 Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
 Subject: The message subject! @all
 Mime-Version: 1.0

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

@@ -9,12 +9,12 @@ describe Gitlab::Email::Handler::EE::ServiceDeskHandler do
   end
 
   let(:email_raw) { fixture_file('emails/service_desk.eml') }
-  let(:project) { create(:project, :public) }
+  let(:namespace) { create(:namespace, name: "email") }
+  let(:project) { create(:project, :public, namespace: namespace, path: "test") }
 
   context 'when service desk is enabled' do
     before do
       project.update(service_desk_enabled: true)
-      project.update(service_desk_mail_key: 'somemailkey')
 
       allow(Notify).to receive(:service_desk_thank_you_email)
         .with(kind_of(Integer)).and_return(double(deliver_later!: true))
@@ -68,10 +68,7 @@ describe Gitlab::Email::Handler::EE::ServiceDeskHandler do
 
   context 'when service desk is not enabled' do
     before do
-      project.update_attributes(
-        service_desk_enabled: false,
-        service_desk_mail_key: 'somemailkey',
-      )
+      project.update_attributes(service_desk_enabled: false)
     end
 
     it 'bounces the email' do

spec/lib/gitlab/email/handler_spec.rb

@@ -16,14 +16,14 @@ describe Gitlab::Email::Handler, lib: true do
         allow_any_instance_of(License).to receive(:add_on?).and_call_original
         allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk').and_return(true)
 
-        expect(handler_for('emails/service_desk.eml', 'service_desk+auth_token')).to be_instance_of(Gitlab::Email::Handler::EE::ServiceDeskHandler)
+        expect(handler_for('emails/service_desk.eml', 'service_desk+some/project')).to be_instance_of(Gitlab::Email::Handler::EE::ServiceDeskHandler)
       end
 
       it 'uses the create issue handler when Service Desk is disabled' do
         allow_any_instance_of(License).to receive(:add_on?).and_call_original
         allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk').and_return(false)
 
-        expect(handler_for('emails/service_desk.eml', 'service_desk+auth_token')).to be_instance_of(Gitlab::Email::Handler::CreateIssueHandler)
+        expect(handler_for('emails/service_desk.eml', 'service_desk+some/project')).to be_instance_of(Gitlab::Email::Handler::CreateIssueHandler)
       end
     end
 

spec/models/ee/project_spec.rb

@@ -116,37 +116,17 @@ describe Project, models: true do
     end
   end
 
-  describe '#regenerate_service_desk_key' do
+  describe '#service_desk_address' do
+    let(:project) { create(:empty_project, service_desk_enabled: true) }
+
     before do
       allow_any_instance_of(License).to receive(:add_on?).and_call_original
       allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk') { true }
+      allow(Gitlab.config.incoming_email).to receive(:address).and_return("test+%{key}@mail.com")
     end
 
-    subject { create(:project) }
-
-    it 'leaves it blank by default' do
-      expect(subject.service_desk_mail_key).to be_blank
-    end
-
-    it 'updates when enabled' do
-      subject.service_desk_enabled = true
-      subject.validate
-      expect(subject.service_desk_mail_key).not_to be_blank
-    end
-
-    it 'changes when enabled' do
-      subject.update!(service_desk_mail_key: '12345')
-      subject.service_desk_enabled = true
-      expect { subject.validate }.to change { subject.service_desk_mail_key }
-    end
-
-    it 'ensures mail key is never nil when enabled' do
-      subject.update!(service_desk_enabled: true)
-
-      expect { subject.update!(service_desk_mail_key: nil) }
-        .to change { subject.service_desk_mail_key }
-
-      expect(subject.service_desk_mail_key).not_to be_blank
+    it 'uses project full path as service desk address key' do
+      expect(project.service_desk_address).to eq("test+#{project.full_path}@mail.com")
     end
   end
 end