Merge branch 'patch/fix-merge-request-hook-validation' into 'master'

Validate with Git Hooks before accepting a merge request

We made possible to add some Git Hook based validation to things like commit message or commit author's email.

When we accept a merge request, it actually goes by the same validations an reject the merge if the merge commit does not follow Git Hook's predefined validations.

The problem is that it executes these validations when doing the very hard work of manipulating git repository, and cannot throw a usable and readable error to the user.

We are now executing these validations before scheduling git hard-work operations, and saving the error to display for the end-user.

Fixes #468, #305

See merge request !326

app/controllers/projects/merge_requests_controller.rb

@@ -197,6 +197,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
+    merge_request_service = MergeRequests::MergeService.new(@project, current_user, merge_params)
+    unless merge_request_service.hooks_validation_pass?(@merge_request)
+      @status = :hook_validation_error
+      return
+    end
+
     TodoService.new.merge_merge_request(merge_request, current_user)
 
     @merge_request.update(merge_error: nil)

app/services/merge_requests/merge_service.rb

@@ -28,6 +28,23 @@ module MergeRequests
       end
     end
 
+    def hooks_validation_pass?(merge_request)
+      git_hook = merge_request.project.git_hook
+      return true unless git_hook
+
+      unless git_hook.commit_message_allowed?(params[:commit_message])
+        merge_request.update(merge_error: "Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+        return false
+      end
+
+      unless git_hook.author_email_allowed?(current_user.email)
+        merge_request.update(merge_error: "Commit author's email '#{current_user.email}' does not follow the pattern '#{git_hook.author_email_regex}'")
+        return false
+      end
+
+      true
+    end
+
     private
 
     def commit

app/views/projects/merge_requests/merge.js.haml

@@ -5,6 +5,9 @@
 - when :merge_when_build_succeeds
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}");
+- when :hook_validation_error
+  :plain
+    $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/error'))}");
 - else
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");

app/views/projects/merge_requests/widget/open/_error.html.haml

+%h4
+  = icon('exclamation-triangle')
+  This merge request failed to be merged automatically
+
+%p
+  = @merge_request.merge_error

spec/factories/git_hooks.rb

@@ -2,11 +2,18 @@
 
 FactoryGirl.define do
   factory :git_hook do
-    force_push_regex "MyString"
+    force_push_regex 'feature\/.*'
     deny_delete_tag false
-    delete_branch_regex "MyString"
+    delete_branch_regex 'bug\/.*'
     project
-    commit_message_regex "MyString"
+
+    trait :commit_message do
+      commit_message_regex "(f|F)ixes #\d+.*"
+    end
+
+    trait :author_email do
+      author_email_regex '.*@veryspecificedomain.com'
+    end
 
     factory :git_hook_sample do
       is_sample true

spec/features/merge_requests/merge_with_hooks_validation_spec.rb

+require 'spec_helper'
+
+feature 'Merge With Git Hooks Validation', feature: true, js: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public, git_hook: git_hook) }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04') }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  context 'commit message is invalid' do
+    let(:git_hook) { create(:git_hook, :commit_message) }
+
+    before do
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'displays error message after merge request is clicked' do
+      click_button 'Accept Merge Request'
+
+      expect(page).to have_content('Merge in progress')
+      expect(page).to have_content('This merge request failed to be merged automatically')
+      expect(page).to have_content("Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+    end
+  end
+
+  context 'author email is invalid' do
+    let(:git_hook) { create(:git_hook, :author_email) }
+
+    before do
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'displays error message after merge request is clicked' do
+      click_button 'Accept Merge Request'
+
+      expect(page).to have_content('Merge in progress')
+      expect(page).to have_content('This merge request failed to be merged automatically')
+      expect(page).to have_content("Commit author's email '#{user.email}' does not follow the pattern '#{git_hook.author_email_regex}'")
+    end
+  end
+
+  def visit_merge_request(merge_request)
+    visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
+  end
+end

spec/services/merge_requests/merge_service_spec.rb

@@ -52,4 +52,34 @@ describe MergeRequests::MergeService, services: true do
       end
     end
   end
+
+  describe '#hooks_validation_pass?' do
+    let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
+
+    it 'returns true when valid' do
+      expect(service.hooks_validation_pass?(merge_request)).to be_truthy
+    end
+
+    context 'commit message validation' do
+      before do
+        allow(project).to receive(:git_hook) { build(:git_hook, commit_message_regex: 'unmatched pattern .*') }
+      end
+
+      it 'returns false and saves error when invalid' do
+        expect(service.hooks_validation_pass?(merge_request)).to be_falsey
+        expect(merge_request.merge_error).not_to be_empty
+      end
+    end
+
+    context 'authors email validation' do
+      before do
+        allow(project).to receive(:git_hook) { build(:git_hook, author_email_regex: '.*@unmatchedemaildomain.com') }
+      end
+
+      it 'returns false and saves error when invalid' do
+        expect(service.hooks_validation_pass?(merge_request)).to be_falsey
+        expect(merge_request.merge_error).not_to be_empty
+      end
+    end
+  end
 end