Merge branch 'cleaner-merge-commit-messages' into 'master'

Cleaner merge commit messages

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23462

See merge request !7722

9 files changed, 242 insertions, 21 deletions

diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js
index 70f9a8d1955..244c2f6746c 100644
--- a/app/assets/javascripts/merge_request.js
+++ b/app/assets/javascripts/merge_request.js
@@ -1,4 +1,4 @@
-/* eslint-disable func-names, space-before-function-paren, no-var, space-before-blocks, prefer-rest-params, wrap-iife, quotes, no-underscore-dangle, one-var, one-var-declaration-per-line, consistent-return, dot-notation, quote-props, comma-dangle, object-shorthand, padded-blocks, max-len */
+/* eslint-disable func-names, space-before-function-paren, no-var, space-before-blocks, prefer-rest-params, wrap-iife, quotes, no-underscore-dangle, one-var, one-var-declaration-per-line, consistent-return, dot-notation, quote-props, comma-dangle, object-shorthand, padded-blocks, max-len, prefer-arrow-callback */
 /* global MergeRequestTabs */
 
 /*= require jquery.waitforimages */
@@ -27,6 +27,7 @@
       // Prevent duplicate event bindings
       this.disableTaskList();
       this.initMRBtnListeners();
+      this.initCommitMessageListeners();
       if ($("a.btn-close").length) {
         this.initTaskList();
       }
@@ -108,6 +109,26 @@
     // note so that we can re-use its form here
     };
 
+    MergeRequest.prototype.initCommitMessageListeners = function() {
+      var textarea = $('textarea.js-commit-message');
+
+      $('a.js-with-description-link').on('click', function(e) {
+        e.preventDefault();
+
+        textarea.val(textarea.data('messageWithDescription'));
+        $('p.js-with-description-hint').hide();
+        $('p.js-without-description-hint').show();
+      });
+
+      $('a.js-without-description-link').on('click', function(e) {
+        e.preventDefault();
+
+        textarea.val(textarea.data('messageWithoutDescription'));
+        $('p.js-with-description-hint').show();
+        $('p.js-without-description-hint').hide();
+      });
+    };
+
     return MergeRequest;
 
   })();
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index a6659ea2fd1..20218775659 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -59,6 +59,10 @@ module MergeRequestsHelper
     @mr_closes_issues ||= @merge_request.closes_issues
   end
 
+  def mr_issues_mentioned_but_not_closing
+    @mr_issues_mentioned_but_not_closing ||= @merge_request.issues_mentioned_but_not_closing
+  end
+
   def mr_change_branches_path(merge_request)
     new_namespace_project_merge_request_path(
       @project.namespace, @project,
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b73d7acefea..b7c775777c7 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -568,6 +568,19 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def issues_mentioned_but_not_closing(current_user = self.author)
+    return [] unless target_branch == project.default_branch
+
+    ext = Gitlab::ReferenceExtractor.new(project, current_user)
+    ext.analyze(description)
+
+    issues = ext.issues
+    closing_issues = Gitlab::ClosingIssueExtractor.new(project, current_user).
+      closed_by_message(description)
+
+    issues - closing_issues
+  end
+
   def target_project_path
     if target_project
       target_project.path_with_namespace
@@ -612,13 +625,24 @@ class MergeRequest < ActiveRecord::Base
     self.target_project.repository.branch_names.include?(self.target_branch)
   end
 
-  def merge_commit_message
-    message = "Merge branch '#{source_branch}' into '#{target_branch}'\n\n"
-    message << "#{title}\n\n"
-    message << "#{description}\n\n" if description.present?
+  def merge_commit_message(include_description: false)
+    closes_issues_references = closes_issues.map do |issue|
+      issue.to_reference(target_project)
+    end
+
+    message = [
+      "Merge branch '#{source_branch}' into '#{target_branch}'",
+      title
+    ]
+
+    if !include_description && closes_issues_references.present?
+      message << "Closes #{closes_issues_references.to_sentence}"
+    end
+
+    message << "#{description}" if include_description && description.present?
     message << "See merge request #{to_reference}"
 
-    message
+    message.join("\n\n")
   end
 
   def reset_merge_when_build_succeeds
diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml
index eee711dc5af..f4aa1609a1b 100644
--- a/app/views/projects/merge_requests/widget/_open.html.haml
+++ b/app/views/projects/merge_requests/widget/_open.html.haml
@@ -30,11 +30,18 @@
     - elsif @merge_request.can_be_merged? || resolved_conflicts
       = render 'projects/merge_requests/widget/open/accept'
 
-  - if mr_closes_issues.present?
+  - if mr_closes_issues.present? || mr_issues_mentioned_but_not_closing.present?
     .mr-widget-footer
       %span
-        %i.fa.fa-check
-        Accepting this merge request will close #{"issue".pluralize(mr_closes_issues.size)}
-        = succeed '.' do
-          != markdown issues_sentence(mr_closes_issues), pipeline: :gfm, author: @merge_request.author
-        = mr_assign_issues_link
+        = icon('check')
+        - if mr_closes_issues.present?
+          Accepting this merge request will close #{"issue".pluralize(mr_closes_issues.size)}
+          = succeed '.' do
+            != markdown issues_sentence(mr_closes_issues), pipeline: :gfm, author: @merge_request.author
+          = mr_assign_issues_link
+        - if mr_issues_mentioned_but_not_closing.present?
+          #{"Issue".pluralize(mr_issues_mentioned_but_not_closing.size)}
+          != markdown issues_sentence(mr_issues_mentioned_but_not_closing), pipeline: :gfm, author: @merge_request.author
+          #{mr_issues_mentioned_but_not_closing.size > 1 ? 'are' : 'is'} mentioned but will not closed.
+         
+
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index 435fe835fae..d6f7f23533c 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -41,6 +41,8 @@
           Modify commit message
     .js-toggle-content.hide.prepend-top-default
       = render 'shared/commit_message_container', params: params,
+          message_with_description: @merge_request.merge_commit_message(include_description: true),
+          message_without_description: @merge_request.merge_commit_message,
           text: @merge_request.merge_commit_message,
           rows: 14, hint: true
 
diff --git a/app/views/shared/_commit_message_container.html.haml b/app/views/shared/_commit_message_container.html.haml
index 0a38327baa2..c196bc06b17 100644
--- a/app/views/shared/_commit_message_container.html.haml
+++ b/app/views/shared/_commit_message_container.html.haml
@@ -1,5 +1,6 @@
 .form-group.commit_message-group
   - nonce = SecureRandom.hex
+  - descriptions = local_assigns.slice(:message_with_description, :message_without_description)
   = label_tag "commit_message-#{nonce}", class: 'control-label' do
     Commit message
   .col-sm-10
@@ -8,9 +9,17 @@
       = text_area_tag 'commit_message',
           (params[:commit_message] || local_assigns[:text] || local_assigns[:placeholder]),
           class: 'form-control js-commit-message', placeholder: local_assigns[:placeholder],
+          data: descriptions,
           required: true, rows: (local_assigns[:rows] || 3),
           id: "commit_message-#{nonce}"
     - if local_assigns[:hint]
       %p.hint
         Try to keep the first line under 52 characters
         and the others under 72.
+    - if descriptions.present?
+      %p.hint.js-with-description-hint
+        = link_to "#", class: "js-with-description-link" do
+          Include description in commit message
+      %p.hint.js-without-description-hint.hide
+        = link_to "#", class: "js-without-description-link" do
+          Don't include description in commit message
diff --git a/spec/features/merge_requests/closes_issues_spec.rb b/spec/features/merge_requests/closes_issues_spec.rb
new file mode 100644
index 00000000000..dc32c8f7373
--- /dev/null
+++ b/spec/features/merge_requests/closes_issues_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+feature 'Merge Request closing issues message', feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public) }
+  let(:issue_1) { create(:issue, project: project)}
+  let(:issue_2) { create(:issue, project: project)}
+  let(:merge_request) do
+    create(
+      :merge_request,
+      :simple,
+      source_project: project,
+      description: merge_request_description
+    )
+  end
+  let(:merge_request_description) { 'Merge Request Description' }
+
+  before do
+    project.team << [user, :master]
+
+    login_as user
+
+    visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+  end
+
+  context 'not closing or mentioning any issue' do
+    it 'does not display closing issue message' do
+      expect(page).not_to have_css('.mr-widget-footer')
+    end
+  end
+
+  context 'closing issues but not mentioning any other issue' do
+    let(:merge_request_description) { "Description\n\nclosing #{issue_1.to_reference}, #{issue_2.to_reference}" }
+
+    it 'does not display closing issue message' do
+      expect(page).to have_content("Accepting this merge request will close issues #{issue_1.to_reference} and #{issue_2.to_reference}")
+    end
+  end
+
+  context 'mentioning issues but not closing them' do
+    let(:merge_request_description) { "Description\n\nRefers to #{issue_1.to_reference} and #{issue_2.to_reference}" }
+
+    it 'does not display closing issue message' do
+      expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but will not closed.")
+    end
+  end
+
+  context 'closing some issues and mentioning, but not closing, others' do
+    let(:merge_request_description) { "Description\n\ncloses #{issue_1.to_reference}\n\n refers to #{issue_2.to_reference}" }
+
+    it 'does not display closing issue message' do
+      expect(page).to have_content("Accepting this merge request will close issue #{issue_1.to_reference}. Issue #{issue_2.to_reference} is mentioned but will not closed.")
+    end
+  end
+end
diff --git a/spec/features/merge_requests/merge_commit_message_toggle_spec.rb b/spec/features/merge_requests/merge_commit_message_toggle_spec.rb
new file mode 100644
index 00000000000..3dbe26cddb0
--- /dev/null
+++ b/spec/features/merge_requests/merge_commit_message_toggle_spec.rb
@@ -0,0 +1,74 @@
+require 'spec_helper'
+
+feature 'Clicking toggle commit message link', feature: true, js: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public) }
+  let(:issue_1) { create(:issue, project: project)}
+  let(:issue_2) { create(:issue, project: project)}
+  let(:merge_request) do
+    create(
+      :merge_request,
+      :simple,
+      source_project: project,
+      description: "Description\n\nclosing #{issue_1.to_reference}, #{issue_2.to_reference}"
+    )
+  end
+  let(:textbox) { page.find(:css, '.js-commit-message', visible: false) }
+  let(:include_link) { page.find(:css, '.js-with-description-link', visible: false) }
+  let(:do_not_include_link) { page.find(:css, '.js-without-description-link', visible: false) }
+  let(:default_message) do
+    [
+      "Merge branch 'feature' into 'master'",
+      merge_request.title,
+      "Closes #{issue_1.to_reference} and #{issue_2.to_reference}",
+      "See merge request #{merge_request.to_reference}"
+    ].join("\n\n")
+  end
+  let(:message_with_description) do
+    [
+      "Merge branch 'feature' into 'master'",
+      merge_request.title,
+      merge_request.description,
+      "See merge request #{merge_request.to_reference}"
+    ].join("\n\n")
+  end
+
+  before do
+    project.team << [user, :master]
+
+    login_as user
+
+    visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+    expect(textbox).not_to be_visible
+    click_link "Modify commit message"
+    expect(textbox).to be_visible
+  end
+
+  it "toggles commit message between message with description and without description " do
+    expect(textbox.value).to eq(default_message)
+
+    click_link "Include description in commit message"
+
+    expect(textbox.value).to eq(message_with_description)
+
+    click_link "Don't include description in commit message"
+
+    expect(textbox.value).to eq(default_message)
+  end
+
+  it "toggles link between 'Include description' and 'Don't include description'" do
+    expect(include_link).to be_visible
+    expect(do_not_include_link).not_to be_visible
+
+    click_link "Include description in commit message"
+
+    expect(include_link).not_to be_visible
+    expect(do_not_include_link).to be_visible
+
+    click_link "Don't include description in commit message"
+
+    expect(include_link).to be_visible
+    expect(do_not_include_link).not_to be_visible
+  end
+end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 1b71d00eb8f..5da00a8636a 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -252,7 +252,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe 'detection of issues to be closed' do
+  describe '#closes_issues' do
     let(:issue0) { create :issue, project: subject.project }
     let(:issue1) { create :issue, project: subject.project }
 
@@ -280,14 +280,19 @@ describe MergeRequest, models: true do
 
       expect(subject.closes_issues).to be_empty
     end
+  end
+
+  describe '#issues_mentioned_but_not_closing' do
+    it 'detects issues mentioned in description but not closed' do
+      mentioned_issue = create(:issue, project: subject.project)
+
+      subject.project.team << [subject.author, :developer]
+      subject.description = "Is related to #{mentioned_issue.to_reference}"
 
-    it 'detects issues mentioned in the description' do
-      issue2 = create(:issue, project: subject.project)
-      subject.description = "Closes #{issue2.to_reference}"
       allow(subject.project).to receive(:default_branch).
         and_return(subject.target_branch)
 
-      expect(subject.closes_issues).to include(issue2)
+      expect(subject.issues_mentioned_but_not_closing).to match_array([mentioned_issue])
     end
   end
 
@@ -410,11 +415,17 @@ describe MergeRequest, models: true do
         .to match("Remove all technical debt\n\n")
     end
 
-    it 'includes its description in the body' do
-      request = build(:merge_request, description: 'By removing all code')
+    it 'includes its closed issues in the body' do
+      issue = create(:issue, project: subject.project)
 
-      expect(request.merge_commit_message)
-        .to match("By removing all code\n\n")
+      subject.project.team << [subject.author, :developer]
+      subject.description = "This issue Closes #{issue.to_reference}"
+
+      allow(subject.project).to receive(:default_branch).
+        and_return(subject.target_branch)
+
+      expect(subject.merge_commit_message)
+        .to match("Closes #{issue.to_reference}")
     end
 
     it 'includes its reference in the body' do
@@ -429,6 +440,20 @@ describe MergeRequest, models: true do
 
       expect(request.merge_commit_message).not_to match("Title\n\n\n\n")
     end
+
+    it 'includes its description in the body' do
+      request = build(:merge_request, description: 'By removing all code')
+
+      expect(request.merge_commit_message(include_description: true))
+        .to match("By removing all code\n\n")
+    end
+
+    it 'does not includes its description in the body' do
+      request = build(:merge_request, description: 'By removing all code')
+
+      expect(request.merge_commit_message)
+        .not_to match("By removing all code\n\n")
+    end
   end
 
   describe "#reset_merge_when_build_succeeds" do