Merge branch '32878-merge-request-from-email' into 'master'

Create merge request from email

Closes #32878

See merge request gitlab-org/gitlab-ce!13817

app/assets/javascripts/issuable_index.js

@@ -28,7 +28,7 @@ export default class IssuableIndex {
         url: $('.incoming-email-token-reset').attr('href'),
         dataType: 'json',
         success(response) {
-          $('#issue_email').val(response.new_issue_address).focus();
+          $('#issuable_email').val(response.new_address).focus();
         },
         beforeSend() {
           $('.incoming-email-token-reset').text('resetting...');

app/assets/stylesheets/pages/issues.scss

@@ -164,12 +164,7 @@ ul.related-merge-requests > li {
   }
 }
 
-.issues-footer {
-  padding-top: $gl-padding;
-  padding-bottom: 37px;
-}
-
-.issue-email-modal-btn {
+.issuable-email-modal-btn {
   padding: 0;
   color: $gl-link-color;
   background-color: transparent;

app/assets/stylesheets/pages/projects.scss

@@ -1210,3 +1210,8 @@ pre.light-well {
     border-color: $border-color;
   }
 }
+
+.issuable-footer {
+  padding-top: $gl-padding;
+  padding-bottom: 37px;
+}

app/controllers/projects_controller.rb

@@ -133,11 +133,11 @@ class ProjectsController < Projects::ApplicationController
     redirect_to edit_project_path(@project), status: 302, alert: ex.message
   end
 
-  def new_issue_address
+  def new_issuable_address
     return render_404 unless Gitlab::IncomingEmail.supports_issue_creation?
 
     current_user.reset_incoming_email_token!
-    render json: { new_issue_address: @project.new_issue_address(current_user) }
+    render json: { new_address: @project.new_issuable_address(current_user, params[:issuable_type]) }
   end
 
   def archive

app/models/project.rb

@@ -752,13 +752,14 @@ class Project < ActiveRecord::Base
     Gitlab::Routing.url_helpers.project_url(self)
   end
 
-  def new_issue_address(author)
+  def new_issuable_address(author, address_type)
     return unless Gitlab::IncomingEmail.supports_issue_creation? && author
 
     author.ensure_incoming_email_token!
 
+    suffix = address_type == 'merge_request' ? '+merge-request' : ''
     Gitlab::IncomingEmail.reply_address(
-      "#{full_path}+#{author.incoming_email_token}")
+      "#{full_path}#{suffix}+#{author.incoming_email_token}")
   end
 
   def build_commit_note(commit)

app/services/merge_requests/build_service.rb

@@ -10,8 +10,12 @@ module MergeRequests
       merge_request.target_branch   = find_target_branch
       merge_request.can_be_created  = branches_valid?
 
-      compare_branches if branches_present?
-      assign_title_and_description if merge_request.can_be_created
+      # compare branches only if branches are valid, otherwise
+      # compare_branches may raise an error
+      if merge_request.can_be_created
+        compare_branches
+        assign_title_and_description
+      end
 
       merge_request
     end

app/services/merge_requests/create_service.rb

@@ -35,6 +35,12 @@ module MergeRequests
       super
     end
 
+    # expose issuable create method so it can be called from email
+    # handler CreateMergeRequestHandler
+    def create(merge_request)
+      super
+    end
+
     private
 
     def update_merge_requests_head_pipeline(merge_request)

app/views/projects/issues/_issue_by_email.html.haml → app/views/projects/_issuable_by_email.html.haml

-.issues-footer.text-center
-  %button.issue-email-modal-btn{ type: "button", data: { toggle: "modal", target: "#issue-email-modal" } }
-    Email a new issue to this project
+- name = issuable_type == 'issue' ? 'issue' : 'merge request'
 
-#issue-email-modal.modal.fade{ tabindex: "-1", role: "dialog" }
+.issuable-footer.text-center
+  %button.issuable-email-modal-btn{ type: "button", data: { toggle: "modal", target: "#issuable-email-modal" } }
+    Email a new #{name} to this project
+
+#issuable-email-modal.modal.fade{ tabindex: "-1", role: "dialog" }
   .modal-dialog{ role: "document" }
     .modal-content
       .modal-header
         %button.close{ type: "button", data: { dismiss: "modal" }, aria: { label: "close" } }
           %span{ aria: { hidden: "true" } }= icon("times")
         %h4.modal-title
-          Create new issue by email
+          Create new #{name} by email
       .modal-body
         %p
-          You can create a new issue inside this project by sending an email to the following email address:
+          You can create a new #{name} inside this project by sending an email to the following email address:
         .email-modal-input-group.input-group
-          = text_field_tag :issue_email, email, class: "monospace js-select-on-focus form-control", readonly: true
+          = text_field_tag :issuable_email, email, class: "monospace js-select-on-focus form-control", readonly: true
           .input-group-btn
-            = clipboard_button(target: '#issue_email')
+            = clipboard_button(target: '#issuable_email')
         %p
-          The subject will be used as the title of the new issue, and the message will be the description.
-
-          = link_to 'Quick actions', help_page_path('user/project/quick_actions'), target: '_blank', tabindex: -1
-          and styling with
-          = link_to 'Markdown', help_page_path('user/markdown'), target: '_blank', tabindex: -1
-          are supported.
-
+          = render 'by_email_description'
         %p
           This is a private email address, generated just for you.
 
-          Anyone who gets ahold of it can create issues as if they were you.
+          Anyone who gets ahold of it can create issues or merge requests as if they were you.
           You should
-          = link_to 'reset it', new_issue_address_project_path(@project), class: 'incoming-email-token-reset'
+          = link_to 'reset it', new_issuable_address_project_path(@project, issuable_type: issuable_type), class: 'incoming-email-token-reset'
           if that ever happens.

app/views/projects/issues/_by_email_description.html.haml

+The subject will be used as the title of the new issue, and the message will be the description.
+
+= link_to 'Quick actions', help_page_path('user/project/quick_actions'), target: '_blank', tabindex: -1
+and styling with
+= link_to 'Markdown', help_page_path('user/markdown'), target: '_blank', tabindex: -1
+are supported.

app/views/projects/issues/index.html.haml

@@ -2,7 +2,7 @@
 - @can_bulk_update = can?(current_user, :admin_issue, @project)
 
 - page_title "Issues"
-- new_issue_email = @project.new_issue_address(current_user)
+- new_issue_email = @project.new_issuable_address(current_user, 'issue')
 
 - content_for :page_specific_javascripts do
   = webpack_bundle_tag 'common_vue'
@@ -25,6 +25,6 @@
     .issues-holder
       = render 'issues'
       - if new_issue_email
-        = render 'issue_by_email', email: new_issue_email
+        = render 'projects/issuable_by_email', email: new_issue_email, issuable_type: 'issue'
 - else
   = render 'shared/empty_states/issues', button_path: new_project_issue_path(@project)

app/views/projects/merge_requests/_by_email_description.html.haml

+The subject will be used as the source branch name for the new merge request and the target branch will be the default branch for the project.

app/views/projects/merge_requests/index.html.haml

@@ -4,6 +4,7 @@
 - new_merge_request_path = project_new_merge_request_path(merge_project) if merge_project
 
 - page_title "Merge Requests"
+- new_merge_request_email = @project.new_issuable_address(current_user, 'merge_request')
 
 - content_for :page_specific_javascripts do
   = webpack_bundle_tag 'common_vue'
@@ -25,5 +26,7 @@
 
     .merge-requests-holder
       = render 'merge_requests'
+      - if new_merge_request_email
+        = render 'projects/issuable_by_email', email: new_merge_request_email, issuable_type: 'merge_request'
 - else
   = render 'shared/empty_states/merge_requests', button_path: new_merge_request_path

app/workers/email_receiver_worker.rb

@@ -39,8 +39,7 @@ class EmailReceiverWorker
         "You are not allowed to perform this action. If you believe this is in error, contact a staff member."
       when Gitlab::Email::NoteableNotFoundError
         "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::InvalidNoteError,
-           Gitlab::Email::InvalidIssueError
+      when Gitlab::Email::InvalidRecordError
         can_retry = true
         e.message
       end

changelogs/unreleased/32878-merge-request-from-email.yml

+---
+title: Allow creation of merge request from email
+merge_request: 13817
+author: janp
+type: added

config/routes/project.rb

@@ -435,7 +435,7 @@ constraints(ProjectUrlConstrainer.new) do
         get :download_export
         get :activity
         get :refs
-        put :new_issue_address
+        put :new_issuable_address
       end
     end
   end

doc/user/project/merge_requests/index.md

@@ -27,7 +27,7 @@ With GitLab merge requests, you can:
 - [Resolve merge conflicts from the UI](#resolve-conflicts)
 - Enable [fast-forward merge requests](#fast-forward-merge-requests)
 - Enable [semi-linear history merge requests](#semi-linear-history-merge-requests) as another security layer to guarantee the pipeline is passing in the target branch
-
+- [Create new merge requests by email](#create_by_email)
 
 With **[GitLab Enterprise Edition][ee]**, you can also:
 
@@ -132,6 +132,14 @@ those conflicts in the GitLab UI.
 
 [Learn more about resolving merge conflicts in the UI.](resolve_conflicts.md)
 
+## Create new merge requests by email
+
+You can create a new merge request by sending an email to a user-specific email
+address. The address can be obtained on the merge requests page by clicking on
+a **Email a new merge request to this project** button.  The subject will be
+used as the source branch name for the new merge request and the target branch
+will be the default branch for the project.
+
 ## Revert changes
 
 GitLab implements Git's powerful feature to revert any commit with introducing

lib/gitlab/email/handler.rb

+require 'gitlab/email/handler/create_merge_request_handler'
 require 'gitlab/email/handler/create_note_handler'
 require 'gitlab/email/handler/create_issue_handler'
 require 'gitlab/email/handler/unsubscribe_handler'
@@ -8,6 +9,7 @@ module Gitlab
       HANDLERS = [
         UnsubscribeHandler,
         CreateNoteHandler,
+        CreateMergeRequestHandler,
         CreateIssueHandler
       ].freeze
 

lib/gitlab/email/handler/create_merge_request_handler.rb

+require 'gitlab/email/handler/base_handler'
+require 'gitlab/email/handler/reply_processing'
+
+module Gitlab
+  module Email
+    module Handler
+      class CreateMergeRequestHandler < BaseHandler
+        include ReplyProcessing
+        attr_reader :project_path, :incoming_email_token
+
+        def initialize(mail, mail_key)
+          super(mail, mail_key)
+          if m = /\A([^\+]*)\+merge-request\+(.*)/.match(mail_key.to_s)
+            @project_path, @incoming_email_token = m.captures
+          end
+        end
+
+        def can_handle?
+          @project_path && @incoming_email_token
+        end
+
+        def execute
+          raise ProjectNotFound unless project
+
+          validate_permission!(:create_merge_request)
+
+          verify_record!(
+            record: create_merge_request,
+            invalid_exception: InvalidMergeRequestError,
+            record_name: 'merge_request')
+        end
+
+        def author
+          @author ||= User.find_by(incoming_email_token: incoming_email_token)
+        end
+
+        def project
+          @project ||= Project.find_by_full_path(project_path)
+        end
+
+        def metrics_params
+          super.merge(project: project&.full_path)
+        end
+
+        private
+
+        def create_merge_request
+          merge_request = MergeRequests::BuildService.new(project, author, merge_request_params).execute
+
+          if merge_request.errors.any?
+            merge_request
+          else
+            MergeRequests::CreateService.new(project, author).create(merge_request)
+          end
+        end
+
+        def merge_request_params
+          {
+            source_project_id: project.id,
+            source_branch: mail.subject,
+            target_project_id: project.id
+          }
+        end
+      end
+    end
+  end
+end

lib/gitlab/email/receiver.rb

@@ -13,8 +13,10 @@ module Gitlab
     UserBlockedError = Class.new(ProcessingError)
     UserNotAuthorizedError = Class.new(ProcessingError)
     NoteableNotFoundError = Class.new(ProcessingError)
-    InvalidNoteError = Class.new(ProcessingError)
-    InvalidIssueError = Class.new(ProcessingError)
+    InvalidRecordError = Class.new(ProcessingError)
+    InvalidNoteError = Class.new(InvalidRecordError)
+    InvalidIssueError = Class.new(InvalidRecordError)
+    InvalidMergeRequestError = Class.new(InvalidRecordError)
     UnknownIncomingEmail = Class.new(ProcessingError)
 
     class Receiver

spec/controllers/projects_controller_spec.rb

@@ -426,11 +426,12 @@ describe ProjectsController do
     end
   end
 
-  describe 'PUT #new_issue_address' do
+  describe 'PUT #new_issuable_address for issue' do
     subject do
-      put :new_issue_address,
+      put :new_issuable_address,
         namespace_id: project.namespace,
-        id: project
+        id: project,
+        issuable_type: 'issue'
       user.reload
     end
 
@@ -449,7 +450,35 @@ describe ProjectsController do
     end
 
     it 'changes projects new issue address' do
-      expect { subject }.to change { project.new_issue_address(user) }
+      expect { subject }.to change { project.new_issuable_address(user, 'issue') }
+    end
+  end
+
+  describe 'PUT #new_issuable_address for merge request' do
+    subject do
+      put :new_issuable_address,
+        namespace_id: project.namespace,
+        id: project,
+        issuable_type: 'merge_request'
+      user.reload
+    end
+
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+      allow(Gitlab.config.incoming_email).to receive(:enabled).and_return(true)
+    end
+
+    it 'has http status 200' do
+      expect(response).to have_http_status(200)
+    end
+
+    it 'changes the user incoming email token' do
+      expect { subject }.to change { user.incoming_email_token }
+    end
+
+    it 'changes projects new merge request address' do
+      expect { subject }.to change { project.new_issuable_address(user, 'merge_request') }
     end
   end
 

spec/features/issues_spec.rb

@@ -365,16 +365,16 @@ describe 'Issues' do
     end
 
     it 'changes incoming email address token', :js do
-      find('.issue-email-modal-btn').click
-      previous_token = find('input#issue_email').value
+      find('.issuable-email-modal-btn').click
+      previous_token = find('input#issuable_email').value
       find('.incoming-email-token-reset').click
 
       wait_for_requests
 
-      expect(page).to have_no_field('issue_email', with: previous_token)
-      new_token = project1.new_issue_address(user.reload)
+      expect(page).to have_no_field('issuable_email', with: previous_token)
+      new_token = project1.new_issuable_address(user.reload, 'issue')
       expect(page).to have_field(
-        'issue_email',
+        'issuable_email',
         with: new_token
       )
     end
@@ -630,8 +630,8 @@ describe 'Issues' do
       end
 
       it 'click the button to show modal for the new email' do
-        page.within '#issue-email-modal' do
-          email = project.new_issue_address(user)
+        page.within '#issuable-email-modal' do
+          email = project.new_issuable_address(user, 'issue')
 
           expect(page).to have_selector("input[value='#{email}']")
         end

spec/fixtures/emails/valid_new_merge_request.eml

+Return-Path: <jake@adventuretime.ooo>
+Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
+Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
+Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
+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+gitlabhq/gitlabhq+merge-request+auth_token@appmail.adventuretime.ooo
+Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+Subject: feature
+Mime-Version: 1.0
+Content-Type: text/plain;
+ charset=ISO-8859-1
+Content-Transfer-Encoding: 7bit
+X-Sieve: CMU Sieve 2.2
+X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
+ 13 Jun 2013 14:03:48 -0700 (PDT)
+X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1

spec/fixtures/emails/valid_new_merge_request_no_subject.eml

+Return-Path: <jake@adventuretime.ooo>
+Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
+Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
+Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
+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+gitlabhq/gitlabhq+merge-request+auth_token@appmail.adventuretime.ooo
+Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+Subject:
+Mime-Version: 1.0
+Content-Type: text/plain;
+ charset=ISO-8859-1
+Content-Transfer-Encoding: 7bit
+X-Sieve: CMU Sieve 2.2
+X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
+ 13 Jun 2013 14:03:48 -0700 (PDT)
+X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1

spec/javascripts/issuable_spec.js

@@ -26,7 +26,7 @@ describe('Issuable', () => {
       document.body.appendChild(element);
 
       const input = document.createElement('input');
-      input.setAttribute('id', 'issue_email');
+      input.setAttribute('id', 'issuable_email');
       document.body.appendChild(input);
 
       Issuable = new IssuableIndex('issue_');

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

+require 'spec_helper'
+require_relative '../email_shared_blocks'
+
+describe Gitlab::Email::Handler::CreateMergeRequestHandler do
+  include_context :email_shared_context
+  it_behaves_like :reply_processing_shared_examples
+
+  before do
+    stub_incoming_email_setting(enabled: true, address: "incoming+%{key}@appmail.adventuretime.ooo")
+    stub_config_setting(host: 'localhost')
+  end
+
+  let(:email_raw) { fixture_file('emails/valid_new_merge_request.eml') }
+  let(:namespace) { create(:namespace, path: 'gitlabhq') }
+
+  # project's git repository is not deleted when project is deleted
+  # between tests. Then tests fail because re-creation of the project with
+  # the same name fails on existing git repository -> skip_disk_validation
+  # ignores repository existence on disk
+  let!(:project)  { create(:project, :public, :repository, skip_disk_validation: true, namespace: namespace, path: 'gitlabhq') }
+  let!(:user) do
+    create(
+      :user,
+      email: 'jake@adventuretime.ooo',
+      incoming_email_token: 'auth_token'
+    )
+  end
+
+  context "as a non-developer" do
+    before do
+      project.add_guest(user)
+    end
+
+    it "raises UserNotAuthorizedError if the user is not a member" do
+      expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
+    end
+  end
+
+  context "as a developer" do
+    before do
+      project.add_developer(user)
+    end
+
+    context "when everything is fine" do
+      it "creates a new merge request" do
+        expect { receiver.execute }.to change { project.merge_requests.count }.by(1)
+        merge_request = project.merge_requests.last
+
+        expect(merge_request.author).to eq(user)
+        expect(merge_request.source_branch).to eq('feature')
+        expect(merge_request.title).to eq('Feature added')
+        expect(merge_request.target_branch).to eq(project.default_branch)
+      end
+    end
+
+    context "something is wrong" do
+      context "when the merge request could not be saved" do
+        before do
+          allow_any_instance_of(MergeRequest).to receive(:save).and_return(false)
+        end
+
+        it "raises an InvalidMergeRequestError" do
+          expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidMergeRequestError)
+        end
+      end
+
+      context "when we can't find the incoming_email_token" do
+        let(:email_raw) { fixture_file("emails/wrong_incoming_email_token.eml") }
+
+        it "raises an UserNotFoundError" do
+          expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotFoundError)
+        end
+      end
+
+      context "when the subject is blank" do
+        let(:email_raw) { fixture_file("emails/valid_new_merge_request_no_subject.eml") }
+
+        it "raises an InvalidMergeRequestError" do
+          expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidMergeRequestError)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/email/handler_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Email::Handler do
+  describe '.for' do
+    it 'picks issue handler if there is not merge request prefix' do
+      expect(described_class.for('email', 'project+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateIssueHandler)
+    end
+
+    it 'picks merge request handler if there is merge request key' do
+      expect(described_class.for('email', 'project+merge-request+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateMergeRequestHandler)
+    end
+
+    it 'returns nil if no handler is found' do
+      expect(described_class.for('email', '')).to be_nil
+    end
+  end
+end

spec/models/project_spec.rb

@@ -451,7 +451,7 @@ describe Project do
     end
   end
 
-  describe "#new_issue_address" do
+  describe "#new_issuable_address" do
     let(:project) { create(:project, path: "somewhere") }
     let(:user) { create(:user) }
 
@@ -463,7 +463,13 @@ describe Project do
       it 'returns the address to create a new issue' do
         address = "p+#{project.full_path}+#{user.incoming_email_token}@gl.ab"
 
-        expect(project.new_issue_address(user)).to eq(address)
+        expect(project.new_issuable_address(user, 'issue')).to eq(address)
+      end
+
+      it 'returns the address to create a new merge request' do
+        address = "p+#{project.full_path}+merge-request+#{user.incoming_email_token}@gl.ab"
+
+        expect(project.new_issuable_address(user, 'merge_request')).to eq(address)
       end
     end
 
@@ -473,7 +479,11 @@ describe Project do
       end
 
       it 'returns nil' do
-        expect(project.new_issue_address(user)).to be_nil
+        expect(project.new_issuable_address(user, 'issue')).to be_nil
+      end
+
+      it 'returns nil' do
+        expect(project.new_issuable_address(user, 'merge_request')).to be_nil
       end
     end
   end

spec/services/merge_requests/build_service_spec.rb

@@ -39,6 +39,7 @@ describe MergeRequests::BuildService do
 
   describe '#execute' do
     it 'calls the compare service with the correct arguments' do
+      allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true)
       expect(CompareService).to receive(:new)
                                   .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch)
                                   .and_call_original