so we use separate classes to handle different tasks

app/workers/email_receiver_worker.rb

@@ -24,25 +24,25 @@ class EmailReceiverWorker
     reason = nil
 
     case e
-    when Gitlab::Email::Receiver::SentNotificationNotFoundError
+    when Gitlab::Email::SentNotificationNotFoundError
       reason = "We couldn't figure out what the email is in reply to. Please create your comment through the web interface."
-    when Gitlab::Email::Receiver::ProjectNotFound
+    when Gitlab::Email::ProjectNotFound
       reason = "We couldn't find the project. Please check if there's any typo."
-    when Gitlab::Email::Receiver::EmptyEmailError
+    when Gitlab::Email::EmptyEmailError
       can_retry = true
       reason = "It appears that the email is blank. Make sure your reply is at the top of the email, we can't process inline replies."
-    when Gitlab::Email::Receiver::AutoGeneratedEmailError
+    when Gitlab::Email::AutoGeneratedEmailError
       reason = "The email was marked as 'auto generated', which we can't accept. Please create your comment through the web interface."
-    when Gitlab::Email::Receiver::UserNotFoundError
+    when Gitlab::Email::UserNotFoundError
       reason = "We couldn't figure out what user corresponds to the email. Please create your comment through the web interface."
-    when Gitlab::Email::Receiver::UserBlockedError
+    when Gitlab::Email::UserBlockedError
       reason = "Your account has been blocked. If you believe this is in error, contact a staff member."
-    when Gitlab::Email::Receiver::UserNotAuthorizedError
+    when Gitlab::Email::UserNotAuthorizedError
       reason = "You are not allowed to perform this action. If you believe this is in error, contact a staff member."
-    when Gitlab::Email::Receiver::NoteableNotFoundError
+    when Gitlab::Email::NoteableNotFoundError
       reason = "The thread you are replying to no longer exists, perhaps it was deleted? If you believe this is in error, contact a staff member."
-    when Gitlab::Email::Receiver::InvalidNoteError,
-         Gitlab::Email::Receiver::InvalidIssueError
+    when Gitlab::Email::InvalidNoteError,
+         Gitlab::Email::InvalidIssueError
       can_retry = true
       reason = e.message
     else

lib/gitlab/email/handler.rb

+
+module Gitlab
+  module Email
+    class Handler
+      attr_reader :mail, :mail_key
+
+      def initialize(mail, mail_key)
+        @mail = mail
+        @mail_key = mail_key
+      end
+
+      def message
+        @message ||= process_message
+      end
+
+      def author
+        raise NotImplementedError
+      end
+
+      def project
+        raise NotImplementedError
+      end
+
+      private
+      def validate_permission!(permission)
+        raise UserNotFoundError unless author
+        raise UserBlockedError if author.blocked?
+        # TODO: Give project not found error if author cannot read project
+        raise UserNotAuthorizedError unless author.can?(permission, project)
+      end
+
+      def process_message
+        add_attachments(ReplyParser.new(mail).execute.strip)
+      end
+
+      def add_attachments(reply)
+        attachments = Email::AttachmentUploader.new(mail).execute(project)
+
+        reply + attachments.map do |link|
+          "\n\n#{link[:markdown]}"
+        end.join
+      end
+
+      def verify_record(record, exception, error_title)
+        return if record.persisted?
+
+        msg = error_title + record.errors.full_messages.map do |error|
+          "\n\n- #{error}"
+        end.join
+
+        raise exception, msg
+      end
+    end
+  end
+end

lib/gitlab/email/handler/create_issue.rb

+
+require 'gitlab/email/handler'
+
+module Gitlab
+  module Email
+    class Handler
+      class CreateIssue < Handler
+        def can_handle?
+          !!project
+        end
+
+        def execute
+          # Must be private project without access
+          raise ProjectNotFound unless author.can?(:read_project, project)
+
+          validate_permission!(:create_issue)
+          validate_authentication_token!
+
+          verify_record(
+            create_issue,
+            InvalidIssueError,
+            "The issue could not be created for the following reasons:"
+          )
+        end
+
+        def author
+          @author ||= mail.from.find do |email|
+            user = User.find_by_any_email(email)
+            break user if user
+          end
+        end
+
+        def project
+          @project ||= Project.find_with_namespace(project_namespace)
+        end
+
+        private
+        def authentication_token
+          mail_key[/[^\+]+$/]
+        end
+
+        def project_namespace
+          mail_key[/^[^\+]+/]
+        end
+
+        def create_issue
+          Issues::CreateService.new(
+            project,
+            author,
+            title:       mail.subject,
+            description: message
+          ).execute
+        end
+
+        def validate_authentication_token!
+          raise UserNotAuthorizedError unless author.authentication_token ==
+                                                authentication_token
+        end
+      end
+    end
+  end
+end

lib/gitlab/email/handler/create_note.rb

+
+require 'gitlab/email/handler'
+
+module Gitlab
+  module Email
+    class Handler
+      class CreateNote < Handler
+        def can_handle?
+          !!sent_notification
+        end
+
+        def execute
+          raise SentNotificationNotFoundError unless sent_notification
+          raise AutoGeneratedEmailError if mail.header.to_s =~ /auto-(generated|replied)/
+
+          validate_permission!(:create_note)
+
+          raise NoteableNotFoundError unless sent_notification.noteable
+          raise EmptyEmailError if message.blank?
+
+          verify_record(
+            create_note,
+            InvalidNoteError,
+            "The comment could not be created for the following reasons:"
+          )
+        end
+
+        def author
+          sent_notification.recipient
+        end
+
+        def project
+          sent_notification.project
+        end
+
+        def sent_notification
+          @sent_notification ||= SentNotification.for(mail_key)
+        end
+
+        private
+        def create_note
+          Notes::CreateService.new(
+            project,
+            author,
+            note:           message,
+            noteable_type:  sent_notification.noteable_type,
+            noteable_id:    sent_notification.noteable_id,
+            commit_id:      sent_notification.commit_id,
+            line_code:      sent_notification.line_code
+          ).execute
+        end
+      end
+    end
+  end
+end

lib/gitlab/email/receiver.rb

+
+require 'gitlab/email/handler/create_note'
+require 'gitlab/email/handler/create_issue'
+
 # Inspired in great part by Discourse's Email::Receiver
 module Gitlab
   module Email
+    class ProcessingError < StandardError; end
+    class EmailUnparsableError < ProcessingError; end
+    class SentNotificationNotFoundError < ProcessingError; end
+    class ProjectNotFound < ProcessingError; end
+    class EmptyEmailError < ProcessingError; end
+    class AutoGeneratedEmailError < ProcessingError; end
+    class UserNotFoundError < ProcessingError; end
+    class UserBlockedError < ProcessingError; end
+    class UserNotAuthorizedError < ProcessingError; end
+    class NoteableNotFoundError < ProcessingError; end
+    class InvalidNoteError < ProcessingError; end
+    class InvalidIssueError < ProcessingError; end
+
     class Receiver
-      class ProcessingError < StandardError; end
-      class EmailUnparsableError < ProcessingError; end
-      class SentNotificationNotFoundError < ProcessingError; end
-      class ProjectNotFound < ProcessingError; end
-      class EmptyEmailError < ProcessingError; end
-      class AutoGeneratedEmailError < ProcessingError; end
-      class UserNotFoundError < ProcessingError; end
-      class UserBlockedError < ProcessingError; end
-      class UserNotAuthorizedError < ProcessingError; end
-      class NoteableNotFoundError < ProcessingError; end
-      class InvalidNoteError < ProcessingError; end
-      class InvalidIssueError < ProcessingError; end
+      attr_reader :mail
 
       def initialize(raw)
-        @raw = raw
+        raise EmptyEmailError if raw.blank?
+        @mail = build_mail(raw)
       end
 
       def execute
-        raise EmptyEmailError if @raw.blank?
+        mail_key = extract_mail_key
+        raise SentNotificationNotFoundError unless mail_key
 
-        if sent_notification
-          process_create_note
-
-        elsif message_project
-          if message_sender.can?(:read_project, message_project)
-            process_create_issue
-          else # Must be private project without access
-            raise ProjectNotFound
-          end
-
-        elsif reply_key =~ %r{/|\+}
-          # Sent Notification reply_key would not have / or +
+        if handler = find_handler(mail, mail_key)
+          handler.execute
+        elsif mail_key =~ %r{/|\+}
+          # Sent Notification mail_key would not have / or +
           raise ProjectNotFound
         else
           raise SentNotificationNotFoundError
         end
       end
 
-      private
-      def process_create_note
-        raise AutoGeneratedEmailError if message.header.to_s =~ /auto-(generated|replied)/
-
-        author = sent_notification.recipient
-        project = sent_notification.project
-
-        validate_permission!(author, project, :create_note)
-
-        raise NoteableNotFoundError unless sent_notification.noteable
-
-        reply = process_reply(project)
-        raise EmptyEmailError if reply.blank?
-        note = create_note(reply)
-
-        unless note.persisted?
-          msg = "The comment could not be created for the following reasons:"
-          note.errors.full_messages.each do |error|
-            msg << "\n\n- #{error}"
-          end
-
-          raise InvalidNoteError, msg
-        end
-      end
-
-      def process_create_issue
-        validate_permission!(message_sender, message_project, :create_issue)
-        validate_authentication_token!(message_sender)
-
-        issue = Issues::CreateService.new(
-          message_project,
-          message_sender,
-          title:       message.subject,
-          description: process_reply(message_project)
-        ).execute
-
-        unless issue.persisted?
-          msg = "The issue could not be created for the following reasons:"
-          issue.errors.full_messages.each do |error|
-            msg << "\n\n- #{error}"
-          end
-
-          raise InvalidIssueError, msg
-        end
-      end
-
-      def validate_permission!(author, project, permission)
-        raise UserNotFoundError unless author
-        raise UserBlockedError if author.blocked?
-        # TODO: Give project not found error if author cannot read project
-        raise UserNotAuthorizedError unless author.can?(permission, project)
-      end
-
-      def validate_authentication_token!(author)
-        raise UserNotAuthorizedError unless author.authentication_token ==
-                                              authentication_token
-      end
-
-      def message_sender
-        @message_sender ||= message.from.find do |email|
-          user = User.find_by_any_email(email)
-          break user if user
-        end
-      end
-
-      def message_project
-        @message_project ||=
-          Project.find_with_namespace(project_namespace) if reply_key
-      end
-
-      def process_reply(project)
-        reply = ReplyParser.new(message).execute.strip
-
-        add_attachments(reply, project)
-
-        reply
-      end
-
-      def message
-        @message ||= Mail::Message.new(@raw)
-      rescue Encoding::UndefinedConversionError, Encoding::InvalidByteSequenceError => e
+      def build_mail(raw)
+        Mail::Message.new(raw)
+      rescue Encoding::UndefinedConversionError,
+             Encoding::InvalidByteSequenceError => e
         raise EmailUnparsableError, e
       end
 
-      def reply_key
+      def extract_mail_key
         key_from_to_header || key_from_additional_headers
       end
 
-      def authentication_token
-        reply_key[/[^\+]+$/]
-      end
-
-      def project_namespace
-        reply_key[/^[^\+]+/]
-      end
-
       def key_from_to_header
-        key = nil
-        message.to.each do |address|
+        mail.to.find do |address|
           key = Gitlab::IncomingEmail.key_from_address(address)
-          break if key
+          break key if key
         end
-
-        key
       end
 
       def key_from_additional_headers
-        reply_key = nil
-
-        Array(message.references).each do |message_id|
-          reply_key = Gitlab::IncomingEmail.key_from_fallback_reply_message_id(message_id)
-          break if reply_key
+        Array(mail.references).find do |mail_id|
+          key = Gitlab::IncomingEmail.key_from_fallback_reply_mail_id(mail_id)
+          break key if key
         end
-
-        reply_key
       end
 
-      def sent_notification
-        @sent_notification ||= SentNotification.for(reply_key) if reply_key
-      end
-
-      def add_attachments(reply, project)
-        attachments = Email::AttachmentUploader.new(message).execute(project)
-
-        attachments.each do |link|
-          reply << "\n\n#{link[:markdown]}"
+      def find_handler(mail, mail_key)
+        [Handler::CreateNote, Handler::CreateIssue].find do |klass|
+          handler = klass.new(mail, mail_key)
+          break handler if handler.can_handle?
         end
-
-        reply
-      end
-
-      def create_note(reply)
-        Notes::CreateService.new(
-          sent_notification.project,
-          sent_notification.recipient,
-          note:           reply,
-          noteable_type:  sent_notification.noteable_type,
-          noteable_id:    sent_notification.noteable_id,
-          commit_id:      sent_notification.commit_id,
-          line_code:      sent_notification.line_code
-        ).execute
       end
     end
   end

lib/gitlab/incoming_email.rb

 module Gitlab
   module IncomingEmail
     class << self
-      FALLBACK_REPLY_MESSAGE_ID_REGEX = /\Areply\-(.+)@#{Gitlab.config.gitlab.host}\Z/.freeze
+      FALLBACK_REPLY_MAIL_ID_REGEX = /\Areply\-(.+)@#{Gitlab.config.gitlab.host}\Z/.freeze
 
       def enabled?
         config.enabled && config.address
@@ -21,8 +21,8 @@ module Gitlab
         match[1]
       end
 
-      def key_from_fallback_reply_message_id(message_id)
-        match = message_id.match(FALLBACK_REPLY_MESSAGE_ID_REGEX)
+      def key_from_fallback_reply_mail_id(mail_id)
+        match = mail_id.match(FALLBACK_REPLY_MAIL_ID_REGEX)
         return unless match
 
         match[1]

spec/lib/gitlab/email/receiver_spec.rb

@@ -34,7 +34,7 @@ describe Gitlab::Email::Receiver, lib: true do
     let(:email_raw) { fixture_file('emails/valid_reply.eml').gsub(reply_key, "") }
 
     it "raises a SentNotificationNotFoundError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::SentNotificationNotFoundError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::SentNotificationNotFoundError)
     end
   end
 
@@ -42,7 +42,7 @@ describe Gitlab::Email::Receiver, lib: true do
     let(:email_raw) { fixture_file('emails/wrong_reply_key.eml') }
 
     it "raises a SentNotificationNotFoundError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::SentNotificationNotFoundError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::SentNotificationNotFoundError)
     end
   end
 
@@ -50,7 +50,7 @@ describe Gitlab::Email::Receiver, lib: true do
     let(:email_raw) { "" }
 
     it "raises an EmptyEmailError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::EmptyEmailError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::EmptyEmailError)
     end
   end
 
@@ -59,7 +59,7 @@ describe Gitlab::Email::Receiver, lib: true do
     let!(:email_raw) { fixture_file("emails/auto_reply.eml") }
 
     it "raises an AutoGeneratedEmailError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::AutoGeneratedEmailError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::AutoGeneratedEmailError)
     end
   end
 
@@ -69,7 +69,7 @@ describe Gitlab::Email::Receiver, lib: true do
     end
 
     it "raises a UserNotFoundError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::UserNotFoundError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotFoundError)
     end
   end
 
@@ -79,7 +79,7 @@ describe Gitlab::Email::Receiver, lib: true do
     end
 
     it "raises a UserBlockedError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::UserBlockedError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::UserBlockedError)
     end
   end
 
@@ -89,7 +89,7 @@ describe Gitlab::Email::Receiver, lib: true do
     end
 
     it "raises a UserNotAuthorizedError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::UserNotAuthorizedError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
     end
   end
 
@@ -99,7 +99,7 @@ describe Gitlab::Email::Receiver, lib: true do
     end
 
     it "raises a NoteableNotFoundError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::NoteableNotFoundError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::NoteableNotFoundError)
     end
   end
 
@@ -107,7 +107,7 @@ describe Gitlab::Email::Receiver, lib: true do
     let!(:email_raw) { fixture_file("emails/no_content_reply.eml") }
 
     it "raises an EmptyEmailError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::EmptyEmailError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::EmptyEmailError)
     end
   end
 
@@ -117,7 +117,7 @@ describe Gitlab::Email::Receiver, lib: true do
     end
 
     it "raises an InvalidNoteError" do
-      expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::InvalidNoteError)
+      expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidNoteError)
     end
   end
 
@@ -220,7 +220,7 @@ describe Gitlab::Email::Receiver, lib: true do
         end
 
         it "raises an InvalidIssueError" do
-          expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::InvalidIssueError)
+          expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidIssueError)
         end
       end
 
@@ -228,7 +228,7 @@ describe Gitlab::Email::Receiver, lib: true do
         let!(:email_raw) { fixture_file("emails/wrong_authentication_token.eml") }
 
         it "raises an UserNotAuthorizedError" do
-          expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::UserNotAuthorizedError)
+          expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
         end
       end
 
@@ -236,7 +236,7 @@ describe Gitlab::Email::Receiver, lib: true do
         let(:project) { create(:project, :private, namespace: namespace) }
 
         it "raises a ProjectNotFound if the user is not a member" do
-          expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::ProjectNotFound)
+          expect { receiver.execute }.to raise_error(Gitlab::Email::ProjectNotFound)
         end
 
         it "raises a UserNotAuthorizedError if the user has no sufficient permission" do
@@ -245,7 +245,7 @@ describe Gitlab::Email::Receiver, lib: true do
           project.update(group: create(:group))
           project.group.add_guest(user)
 
-          expect { receiver.execute }.to raise_error(Gitlab::Email::Receiver::UserNotAuthorizedError)
+          expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
         end
       end
     end

spec/workers/email_receiver_worker_spec.rb

@@ -17,7 +17,7 @@ describe EmailReceiverWorker do
 
     context "when an error occurs" do
       before do
-        allow_any_instance_of(Gitlab::Email::Receiver).to receive(:execute).and_raise(Gitlab::Email::Receiver::EmptyEmailError)
+        allow_any_instance_of(Gitlab::Email::Receiver).to receive(:execute).and_raise(Gitlab::Email::EmptyEmailError)
       end
 
       it "sends out a rejection email" do