Refactoring and review comments

including verifying the project_slug

lib/gitlab/email/handler/create_issue_handler.rb

@@ -11,16 +11,19 @@ module Gitlab
       class CreateIssueHandler < BaseHandler
         include ReplyProcessing
 
-        HANDLER_REGEX        = /\A.+-(?<project_id>.+)-(?<incoming_email_token>.+)-issue\z/.freeze
+        HANDLER_REGEX        = /\A#{HANDLER_ACTION_BASE_REGEX}-issue\z/.freeze
         HANDLER_REGEX_LEGACY = /\A(?<project_path>[^\+]*)\+(?<incoming_email_token>.*)\z/.freeze
 
         def initialize(mail, mail_key)
           super(mail, mail_key)
 
-          if matched = HANDLER_REGEX.match(mail_key.to_s)
-            @project_id, @incoming_email_token = matched.captures
+          if !mail_key&.include?('/') && (matched = HANDLER_REGEX.match(mail_key.to_s))
+            @project_slug         = matched[:project_slug]
+            @project_id           = matched[:project_id]&.to_i
+            @incoming_email_token = matched[:incoming_email_token]
           elsif matched = HANDLER_REGEX_LEGACY.match(mail_key.to_s)
-            @project_path, @incoming_email_token = matched.captures
+            @project_path         = matched[:project_path]
+            @incoming_email_token = matched[:incoming_email_token]
           end
         end
 
@@ -45,18 +48,8 @@ module Gitlab
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
-        def project
-          @project ||= if project_id
-                         Project.find_by_id(project_id)
-                       else
-                         Project.find_by_full_path(project_path)
-                       end
-        end
-
         private
 
-        attr_reader :project_id, :project_path, :incoming_email_token
-
         def create_issue
           Issues::CreateService.new(
             project,

lib/gitlab/email/handler/create_merge_request_handler.rb

@@ -12,16 +12,19 @@ module Gitlab
       class CreateMergeRequestHandler < BaseHandler
         include ReplyProcessing
 
-        HANDLER_REGEX        = /\A.+-(?<project_id>.+)-(?<incoming_email_token>.+)-merge-request\z/.freeze
+        HANDLER_REGEX        = /\A#{HANDLER_ACTION_BASE_REGEX}-merge-request\z/.freeze
         HANDLER_REGEX_LEGACY = /\A(?<project_path>[^\+]*)\+merge-request\+(?<incoming_email_token>.*)/.freeze
 
         def initialize(mail, mail_key)
           super(mail, mail_key)
 
-          if matched = HANDLER_REGEX.match(mail_key.to_s)
-            @project_id, @incoming_email_token = matched.captures
+          if !mail_key&.include?('/') && (matched = HANDLER_REGEX.match(mail_key.to_s))
+            @project_slug         = matched[:project_slug]
+            @project_id           = matched[:project_id]&.to_i
+            @incoming_email_token = matched[:incoming_email_token]
           elsif matched = HANDLER_REGEX_LEGACY.match(mail_key.to_s)
-            @project_path, @incoming_email_token = matched.captures
+            @project_path         = matched[:project_path]
+            @incoming_email_token = matched[:incoming_email_token]
           end
         end
 
@@ -47,22 +50,12 @@ module Gitlab
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
-        def project
-          @project ||= if project_id
-                         Project.find_by_id(project_id)
-                       else
-                         Project.find_by_full_path(project_path)
-                       end
-        end
-
         def metrics_params
           super.merge(includes_patches: patch_attachments.any?)
         end
 
         private
 
-        attr_reader :project_id, :project_path, :incoming_email_token
-
         def build_merge_request
           MergeRequests::BuildService.new(project, author, merge_request_params).execute
         end

lib/gitlab/email/handler/reply_processing.rb

@@ -6,13 +6,28 @@ module Gitlab
       module ReplyProcessing
         private
 
+        HANDLER_ACTION_BASE_REGEX = /(?<project_slug>.+)-(?<project_id>\d+)-(?<incoming_email_token>.+)/.freeze
+
+        attr_reader :project_id, :project_slug, :project_path, :incoming_email_token
+
         def author
           raise NotImplementedError
         end
 
+        # rubocop:disable Gitlab/ModuleWithInstanceVariables
         def project
-          raise NotImplementedError
+          return @project if instance_variable_defined?(:@project)
+
+          if project_id
+            @project = Project.find_by_id(project_id)
+            @project = nil unless valid_project_slug?(@project)
+          else
+            @project = Project.find_by_full_path(project_path)
+          end
+
+          @project
         end
+        # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
         def message
           @message ||= process_message
@@ -58,6 +73,10 @@ module Gitlab
 
           raise invalid_exception, msg
         end
+
+        def valid_project_slug?(found_project)
+          project_slug == found_project.full_path_slug
+        end
       end
     end
   end

lib/gitlab/email/handler/unsubscribe_handler.rb

@@ -23,7 +23,7 @@ module Gitlab
         end
 
         def can_handle?
-          @reply_token.present?
+          reply_token.present?
         end
 
         def execute

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

@@ -27,9 +27,10 @@ describe Gitlab::Email::Handler::CreateIssueHandler do
     let(:mail) { Mail::Message.new(email_raw) }
 
     it "matches the new format" do
-      handler = described_class.new(mail, "h5bp-html5-boilerplate-#{project.project_id}-#{user.incoming_email_token}-issue")
+      handler = described_class.new(mail, "gitlabhq-gitlabhq-#{project.project_id}-#{user.incoming_email_token}-issue")
 
-      expect(handler.instance_variable_get(:@project_id).to_i).to eq project.project_id
+      expect(handler.instance_variable_get(:@project_id)).to eq project.project_id
+      expect(handler.instance_variable_get(:@project_slug)).to eq project.full_path_slug
       expect(handler.instance_variable_get(:@incoming_email_token)).to eq user.incoming_email_token
       expect(handler.can_handle?).to be_truthy
     end

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

@@ -31,9 +31,10 @@ describe Gitlab::Email::Handler::CreateMergeRequestHandler do
     let(:mail) { Mail::Message.new(email_raw) }
 
     it "matches the new format" do
-      handler = described_class.new(mail, "h5bp-html5-boilerplate-#{project.project_id}-#{user.incoming_email_token}-merge-request")
+      handler = described_class.new(mail, "gitlabhq-gitlabhq-#{project.project_id}-#{user.incoming_email_token}-merge-request")
 
-      expect(handler.instance_variable_get(:@project_id).to_i).to eq project.project_id
+      expect(handler.instance_variable_get(:@project_id)).to eq project.project_id
+      expect(handler.instance_variable_get(:@project_slug)).to eq project.full_path_slug
       expect(handler.instance_variable_get(:@incoming_email_token)).to eq user.incoming_email_token
       expect(handler.can_handle?).to be_truthy
     end

spec/lib/gitlab/email/handler_spec.rb

@@ -19,7 +19,7 @@ describe Gitlab::Email::Handler do
 
   describe 'regexps are set properly' do
     let(:addresses) do
-      %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX} sent_notification_key path-to-project-project_id-user_email_token-merge-request path-to-project-user_email_token-issue) +
+      %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX} sent_notification_key path-to-project-123-user_email_token-merge-request path-to-project-123-user_email_token-issue) +
         %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX_LEGACY} sent_notification_key path/to/project+merge-request+user_email_token path/to/project+user_email_token)
     end