Commit 0a645009 authored by Sean McGivern's avatar Sean McGivern

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
parents 8f5cf205 3e3d6b53
/* 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 */ /* global MergeRequestTabs */
/*= require jquery.waitforimages */ /*= require jquery.waitforimages */
...@@ -27,6 +27,7 @@ ...@@ -27,6 +27,7 @@
// Prevent duplicate event bindings // Prevent duplicate event bindings
this.disableTaskList(); this.disableTaskList();
this.initMRBtnListeners(); this.initMRBtnListeners();
this.initCommitMessageListeners();
if ($("a.btn-close").length) { if ($("a.btn-close").length) {
this.initTaskList(); this.initTaskList();
} }
...@@ -108,6 +109,26 @@ ...@@ -108,6 +109,26 @@
// note so that we can re-use its form here // 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; return MergeRequest;
})(); })();
......
...@@ -59,6 +59,10 @@ module MergeRequestsHelper ...@@ -59,6 +59,10 @@ module MergeRequestsHelper
@mr_closes_issues ||= @merge_request.closes_issues @mr_closes_issues ||= @merge_request.closes_issues
end 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) def mr_change_branches_path(merge_request)
new_namespace_project_merge_request_path( new_namespace_project_merge_request_path(
@project.namespace, @project, @project.namespace, @project,
......
...@@ -568,6 +568,19 @@ class MergeRequest < ActiveRecord::Base ...@@ -568,6 +568,19 @@ class MergeRequest < ActiveRecord::Base
end end
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 def target_project_path
if target_project if target_project
target_project.path_with_namespace target_project.path_with_namespace
...@@ -612,13 +625,24 @@ class MergeRequest < ActiveRecord::Base ...@@ -612,13 +625,24 @@ class MergeRequest < ActiveRecord::Base
self.target_project.repository.branch_names.include?(self.target_branch) self.target_project.repository.branch_names.include?(self.target_branch)
end end
def merge_commit_message def merge_commit_message(include_description: false)
message = "Merge branch '#{source_branch}' into '#{target_branch}'\n\n" closes_issues_references = closes_issues.map do |issue|
message << "#{title}\n\n" issue.to_reference(target_project)
message << "#{description}\n\n" if description.present? 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 << "See merge request #{to_reference}"
message message.join("\n\n")
end end
def reset_merge_when_build_succeeds def reset_merge_when_build_succeeds
......
...@@ -30,11 +30,18 @@ ...@@ -30,11 +30,18 @@
- elsif @merge_request.can_be_merged? || resolved_conflicts - elsif @merge_request.can_be_merged? || resolved_conflicts
= render 'projects/merge_requests/widget/open/accept' = 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 .mr-widget-footer
%span %span
%i.fa.fa-check = icon('check')
- if mr_closes_issues.present?
Accepting this merge request will close #{"issue".pluralize(mr_closes_issues.size)} Accepting this merge request will close #{"issue".pluralize(mr_closes_issues.size)}
= succeed '.' do = succeed '.' do
!= markdown issues_sentence(mr_closes_issues), pipeline: :gfm, author: @merge_request.author != markdown issues_sentence(mr_closes_issues), pipeline: :gfm, author: @merge_request.author
= mr_assign_issues_link = 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.
...@@ -41,6 +41,8 @@ ...@@ -41,6 +41,8 @@
Modify commit message Modify commit message
.js-toggle-content.hide.prepend-top-default .js-toggle-content.hide.prepend-top-default
= render 'shared/commit_message_container', params: params, = 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, text: @merge_request.merge_commit_message,
rows: 14, hint: true rows: 14, hint: true
......
.form-group.commit_message-group .form-group.commit_message-group
- nonce = SecureRandom.hex - nonce = SecureRandom.hex
- descriptions = local_assigns.slice(:message_with_description, :message_without_description)
= label_tag "commit_message-#{nonce}", class: 'control-label' do = label_tag "commit_message-#{nonce}", class: 'control-label' do
Commit message Commit message
.col-sm-10 .col-sm-10
...@@ -8,9 +9,17 @@ ...@@ -8,9 +9,17 @@
= text_area_tag 'commit_message', = text_area_tag 'commit_message',
(params[:commit_message] || local_assigns[:text] || local_assigns[:placeholder]), (params[:commit_message] || local_assigns[:text] || local_assigns[:placeholder]),
class: 'form-control js-commit-message', placeholder: local_assigns[:placeholder], class: 'form-control js-commit-message', placeholder: local_assigns[:placeholder],
data: descriptions,
required: true, rows: (local_assigns[:rows] || 3), required: true, rows: (local_assigns[:rows] || 3),
id: "commit_message-#{nonce}" id: "commit_message-#{nonce}"
- if local_assigns[:hint] - if local_assigns[:hint]
%p.hint %p.hint
Try to keep the first line under 52 characters Try to keep the first line under 52 characters
and the others under 72. 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
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
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
...@@ -252,7 +252,7 @@ describe MergeRequest, models: true do ...@@ -252,7 +252,7 @@ describe MergeRequest, models: true do
end end
end end
describe 'detection of issues to be closed' do describe '#closes_issues' do
let(:issue0) { create :issue, project: subject.project } let(:issue0) { create :issue, project: subject.project }
let(:issue1) { create :issue, project: subject.project } let(:issue1) { create :issue, project: subject.project }
...@@ -280,14 +280,19 @@ describe MergeRequest, models: true do ...@@ -280,14 +280,19 @@ describe MergeRequest, models: true do
expect(subject.closes_issues).to be_empty expect(subject.closes_issues).to be_empty
end 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). allow(subject.project).to receive(:default_branch).
and_return(subject.target_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
end end
...@@ -410,11 +415,17 @@ describe MergeRequest, models: true do ...@@ -410,11 +415,17 @@ describe MergeRequest, models: true do
.to match("Remove all technical debt\n\n") .to match("Remove all technical debt\n\n")
end end
it 'includes its description in the body' do it 'includes its closed issues in the body' do
request = build(:merge_request, description: 'By removing all code') issue = create(:issue, project: subject.project)
expect(request.merge_commit_message) subject.project.team << [subject.author, :developer]
.to match("By removing all code\n\n") 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 end
it 'includes its reference in the body' do it 'includes its reference in the body' do
...@@ -429,6 +440,20 @@ describe MergeRequest, models: true do ...@@ -429,6 +440,20 @@ describe MergeRequest, models: true do
expect(request.merge_commit_message).not_to match("Title\n\n\n\n") expect(request.merge_commit_message).not_to match("Title\n\n\n\n")
end 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 end
describe "#reset_merge_when_build_succeeds" do describe "#reset_merge_when_build_succeeds" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment