Commit c8a115c0 authored by ash wilson's avatar ash wilson Committed by Ash Wilson

Link issues from comments and automatically close them

Any mention of Issues, MergeRequests, or Commits via GitLab-flavored markdown
references in descriptions, titles, or attached Notes creates a back-reference
Note that links to the original referencer. Furthermore, pushing commits with
commit messages that match a (configurable) regexp to a project's default
branch will close any issues mentioned by GFM in the matched closing phrase.
If accepting a merge request would close any Issues in this way, a banner is
appended to the merge request's main panel to indicate this.
parent 2b36dee6
v 6.1.0
- Link issues, merge requests, and commits when they reference each other with GFM
- Close issues automatically when pushing commits with a special message
v 6.0.0
- Feature: Replace teams with group membership
We introduce group membership in 6.0 as a replacement for teams.
......@@ -54,7 +58,7 @@ v 5.4.0
- Notify mentioned users with email
v 5.3.0
- Refactored services
- Refactored services
- Campfire service added
- HipChat service added
- Fixed bug with LDAP + git over http
......
......@@ -3,6 +3,7 @@ require 'gitlab/satellite/satellite'
class Projects::MergeRequestsController < Projects::ApplicationController
before_filter :module_enabled
before_filter :merge_request, only: [:edit, :update, :show, :commits, :diffs, :automerge, :automerge_check, :ci_status]
before_filter :closes_issues, only: [:edit, :update, :show, :commits, :diffs]
before_filter :validates_merge_request, only: [:show, :diffs]
before_filter :define_show_vars, only: [:show, :diffs]
......@@ -135,6 +136,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@merge_request ||= @project.merge_requests.find_by_iid!(params[:id])
end
def closes_issues
@closes_issues ||= @merge_request.closes_issues
end
def authorize_modify_merge_request!
return render_404 unless can?(current_user, :modify_merge_request, @merge_request)
end
......
......@@ -2,6 +2,9 @@ class Commit
include ActiveModel::Conversion
include StaticModel
extend ActiveModel::Naming
include Mentionable
attr_mentionable :safe_message
# Safe amount of files with diffs in one commit to render
# Used to prevent 500 error on huge commits by suppressing diff
......@@ -65,6 +68,29 @@ class Commit
end
end
# Regular expression that identifies commit message clauses that trigger issue closing.
def issue_closing_regex
@issue_closing_regex ||= Regexp.new(Gitlab.config.gitlab.issue_closing_pattern)
end
# Discover issues should be closed when this commit is pushed to a project's
# default branch.
def closes_issues project
md = issue_closing_regex.match(safe_message)
if md
extractor = Gitlab::ReferenceExtractor.new
extractor.analyze(md[0])
extractor.issues_for(project)
else
[]
end
end
# Mentionable override.
def gfm_reference
"commit #{sha[0..5]}"
end
def method_missing(m, *args, &block)
@raw.send(m, *args, &block)
end
......
# == Mentionable concern
#
# Contains common functionality shared between Issues and Notes
# Contains functionality related to objects that can mention Users, Issues, MergeRequests, or Commits by
# GFM references.
#
# Used by Issue, Note
# Used by Issue, Note, MergeRequest, and Commit.
#
module Mentionable
extend ActiveSupport::Concern
module ClassMethods
# Indicate which attributes of the Mentionable to search for GFM references.
def attr_mentionable *attrs
mentionable_attrs.concat(attrs.map(&:to_s))
end
# Accessor for attributes marked mentionable.
def mentionable_attrs
@mentionable_attrs ||= []
end
end
# Generate a GFM back-reference that will construct a link back to this Mentionable when rendered. Must
# be overridden if this model object can be referenced directly by GFM notation.
def gfm_reference
raise NotImplementedError.new("#{self.class} does not implement #gfm_reference")
end
# Construct a String that contains possible GFM references.
def mentionable_text
self.class.mentionable_attrs.map { |attr| send(attr) || '' }.join
end
# The GFM reference to this Mentionable, which shouldn't be included in its #references.
def local_reference
self
end
# Determine whether or not a cross-reference Note has already been created between this Mentionable and
# the specified target.
def has_mentioned? target
Note.cross_reference_exists?(target, local_reference)
end
def mentioned_users
users = []
return users if mentionable_text.blank?
......@@ -24,14 +59,39 @@ module Mentionable
users.uniq
end
def mentionable_text
if self.class == Issue
description
elsif self.class == Note
note
else
nil
# Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
def references p = project, text = mentionable_text
return [] if text.blank?
ext = Gitlab::ReferenceExtractor.new
ext.analyze(text)
(ext.issues_for(p) + ext.merge_requests_for(p) + ext.commits_for(p)).uniq - [local_reference]
end
# Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+.
def create_cross_references! p = project, a = author, without = []
refs = references(p) - without
refs.each do |ref|
Note.create_cross_reference_note(ref, local_reference, a, p)
end
end
# If the mentionable_text field is about to change, locate any *added* references and create cross references for
# them. Invoke from an observer's #before_save implementation.
def notice_added_references p = project, a = author
ch = changed_attributes
original, mentionable_changed = "", false
self.class.mentionable_attrs.each do |attr|
if ch[attr]
original << ch[attr]
mentionable_changed = true
end
end
# Only proceed if the saved changes actually include a chance to an attr_mentionable field.
return unless mentionable_changed
preexisting = references(p, original)
create_cross_references!(p, a, preexisting)
end
end
......@@ -32,6 +32,7 @@ class Issue < ActiveRecord::Base
attr_accessible :title, :assignee_id, :position, :description,
:milestone_id, :label_list, :author_id_of_changes,
:state_event
attr_mentionable :title, :description
acts_as_taggable_on :labels
......@@ -56,4 +57,10 @@ class Issue < ActiveRecord::Base
# Both open and reopened issues should be listed as opened
scope :opened, -> { with_state(:opened, :reopened) }
# Mentionable overrides.
def gfm_reference
"issue ##{iid}"
end
end
......@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project"
attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :author_id_of_changes, :state_event
attr_mentionable :title
attr_accessor :should_remove_source_branch
......@@ -255,6 +255,20 @@ class MergeRequest < ActiveRecord::Base
target_project
end
# Return the set of issues that will be closed if this merge request is accepted.
def closes_issues
if target_branch == project.default_branch
unmerged_commits.map { |c| c.closes_issues(project) }.flatten.uniq.sort_by(&:id)
else
[]
end
end
# Mentionable override.
def gfm_reference
"merge request !#{iid}"
end
private
def dump_commits(commits)
......
......@@ -24,6 +24,7 @@ class Note < ActiveRecord::Base
attr_accessible :note, :noteable, :noteable_id, :noteable_type, :project_id,
:attachment, :line_code, :commit_id
attr_mentionable :note
belongs_to :project
belongs_to :noteable, polymorphic: true
......@@ -54,15 +55,36 @@ class Note < ActiveRecord::Base
serialize :st_diff
before_create :set_diff, if: ->(n) { n.line_code.present? }
def self.create_status_change_note(noteable, project, author, status)
def self.create_status_change_note(noteable, project, author, status, source)
body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
create({
noteable: noteable,
project: project,
author: author,
note: body,
system: true
}, without_protection: true)
end
# +noteable+ was referenced from +mentioner+, by including GFM in either +mentioner+'s description or an associated Note.
# Create a system Note associated with +noteable+ with a GFM back-reference to +mentioner+.
def self.create_cross_reference_note(noteable, mentioner, author, project)
create({
noteable: noteable,
commit_id: (noteable.sha if noteable.respond_to? :sha),
project: project,
author: author,
note: "_Status changed to #{status}_"
note: "_mentioned in #{mentioner.gfm_reference}_",
system: true
}, without_protection: true)
end
# Determine whether or not a cross-reference note already exists.
def self.cross_reference_exists?(noteable, mentioner)
where(noteable_id: noteable.id, system: true, note: "_mentioned in #{mentioner.gfm_reference}_").any?
end
def commit_author
@commit_author ||=
project.users.find_by_email(noteable.author_email) ||
......@@ -191,6 +213,16 @@ class Note < ActiveRecord::Base
for_issue? || (for_merge_request? && !for_diff_line?)
end
# Mentionable override.
def gfm_reference
noteable.gfm_reference
end
# Mentionable override.
def local_reference
noteable
end
def noteable_type_name
if noteable_type.present?
noteable_type.downcase
......
......@@ -18,6 +18,7 @@ class Repository
end
def commit(id = nil)
return nil unless raw_repository
commit = Gitlab::Git::Commit.find(raw_repository, id)
commit = Commit.new(commit) if commit
commit
......
......@@ -5,8 +5,8 @@ class ActivityObserver < BaseObserver
event_author_id = record.author_id
if record.kind_of?(Note)
# Skip system status notes like 'status changed to close'
return true if record.note.include?("_Status changed to ")
# Skip system notes, like status changes and cross-references.
return true if record.system?
# Skip wall notes to prevent spamming of dashboard
return true if record.noteable_type.blank?
......
......@@ -10,4 +10,8 @@ class BaseObserver < ActiveRecord::Observer
def current_user
Thread.current[:current_user]
end
def current_commit
Thread.current[:current_commit]
end
end
class IssueObserver < BaseObserver
def after_create(issue)
notification.new_issue(issue, current_user)
issue.create_cross_references!(issue.project, current_user)
end
def after_close(issue, transition)
......@@ -17,12 +19,14 @@ class IssueObserver < BaseObserver
if issue.is_being_reassigned?
notification.reassigned_issue(issue, current_user)
end
issue.notice_added_references(issue.project, current_user)
end
protected
# Create issue note with service comment like 'Status changed to closed'
def create_note(issue)
Note.create_status_change_note(issue, issue.project, current_user, issue.state)
Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
end
end
......@@ -7,11 +7,13 @@ class MergeRequestObserver < ActivityObserver
end
notification.new_merge_request(merge_request, current_user)
merge_request.create_cross_references!(merge_request.project, current_user)
end
def after_close(merge_request, transition)
create_event(merge_request, Event::CLOSED)
Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state)
Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
notification.close_mr(merge_request, current_user)
end
......@@ -33,11 +35,13 @@ class MergeRequestObserver < ActivityObserver
def after_reopen(merge_request, transition)
create_event(merge_request, Event::REOPENED)
Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state)
Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
end
def after_update(merge_request)
notification.reassigned_merge_request(merge_request, current_user) if merge_request.is_being_reassigned?
merge_request.notice_added_references(merge_request.project, current_user)
end
def create_event(record, status)
......
class NoteObserver < BaseObserver
def after_create(note)
notification.new_note(note)
unless note.system?
# Create a cross-reference note if this Note contains GFM that names an
# issue, merge request, or commit.
note.references.each do |mentioned|
Note.create_cross_reference_note(mentioned, note.noteable, note.author, note.project)
end
end
end
def after_update(note)
note.notice_added_references(note.project, current_user)
end
end
class GitPushService
attr_accessor :project, :user, :push_data
attr_accessor :project, :user, :push_data, :push_commits
# This method will be called after each git update
# and only if the provided user and project is present in GitLab.
#
# All callbacks for post receive action should be placed here.
#
# Now this method do next:
# 1. Ensure project satellite exists
# 2. Update merge requests
# 3. Execute project web hooks
# 4. Execute project services
# 5. Create Push Event
# Next, this method:
# 1. Creates the push event
# 2. Ensures that the project satellite exists
# 3. Updates merge requests
# 4. Recognizes cross-references from commit messages
# 5. Executes the project's web hooks
# 6. Executes the project's services
#
def execute(project, user, oldrev, newrev, ref)
@project, @user = project, user
# Collect data for this git push
@push_commits = project.repository.commits_between(oldrev, newrev)
@push_data = post_receive_data(oldrev, newrev, ref)
create_push_event
......@@ -25,11 +27,27 @@ class GitPushService
project.discover_default_branch
project.repository.expire_cache
if push_to_branch?(ref, oldrev)
if push_to_existing_branch?(ref, oldrev)
project.update_merge_requests(oldrev, newrev, ref, @user)
process_commit_messages(ref)
project.execute_hooks(@push_data.dup)
project.execute_services(@push_data.dup)
end
if push_to_new_branch?(ref, oldrev)
# Re-find the pushed commits.
if is_default_branch?(ref)
# Initial push to the default branch. Take the full history of that branch as "newly pushed".
@push_commits = project.repository.commits(newrev)
else
# Use the pushed commits that aren't reachable by the default branch
# as a heuristic. This may include more commits than are actually pushed, but
# that shouldn't matter because we check for existing cross-references later.
@push_commits = project.repository.commits_between(project.default_branch, newrev)
end
process_commit_messages(ref)
end
end
# This method provide a sample data
......@@ -45,7 +63,7 @@ class GitPushService
protected
def create_push_event
Event.create(
Event.create!(
project: project,
action: Event::PUSHED,
data: push_data,
......@@ -53,6 +71,36 @@ class GitPushService
)
end
# Extract any GFM references from the pushed commit messages. If the configured issue-closing regex is matched,
# close the referenced Issue. Create cross-reference Notes corresponding to any other referenced Mentionables.
def process_commit_messages ref
is_default_branch = is_default_branch?(ref)
@push_commits.each do |commit|
# Close issues if these commits were pushed to the project's default branch and the commit message matches the
# closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to
# a different branch.
issues_to_close = commit.closes_issues(project)
author = commit_user(commit)
if !issues_to_close.empty? && is_default_branch
Thread.current[:current_user] = author
Thread.current[:current_commit] = commit
issues_to_close.each { |i| i.close && i.save }
end
# Create cross-reference notes for any other references. Omit any issues that were referenced in an
# issue-closing phrase, or have already been mentioned from this commit (probably from this commit
# being pushed to a different branch).
refs = commit.references(project) - issues_to_close
refs.reject! { |r| commit.has_mentioned?(r) }
refs.each do |r|
Note.create_cross_reference_note(r, commit, author, project)
end
end
end
# Produce a hash of post-receive data
#
# data = {
......@@ -72,8 +120,6 @@ class GitPushService
# }
#
def post_receive_data(oldrev, newrev, ref)
push_commits = project.repository.commits_between(oldrev, newrev)
# Total commits count
push_commits_count = push_commits.size
......@@ -116,10 +162,26 @@ class GitPushService
data
end
def push_to_branch? ref, oldrev
def push_to_existing_branch? ref, oldrev
ref_parts = ref.split('/')
# Return if this is not a push to a branch (e.g. new commits)
!(ref_parts[1] !~ /heads/ || oldrev == "00000000000000000000000000000000")
ref_parts[1] =~ /heads/ && oldrev != "0000000000000000000000000000000000000000"
end
def push_to_new_branch? ref, oldrev
ref_parts = ref.split('/')
ref_parts[1] =~ /heads/ && oldrev == "0000000000000000000000000000000000000000"
end
def is_default_branch? ref
ref == "refs/heads/#{project.default_branch}"
end
def commit_user commit
User.where(email: commit.author_email).first ||
User.where(name: commit.author_name).first ||
user
end
end
......@@ -33,4 +33,10 @@
%i.icon-ok
Merged by #{link_to_member(@project, @merge_request.merge_event.author)}
#{time_ago_in_words(@merge_request.merge_event.created_at)} ago.
- if !@closes_issues.empty? && @merge_request.opened?
.ui-box-bottom.alert-info
%span
%i.icon-ok
Accepting this merge request will close #{@closes_issues.size == 1 ? 'issue' : 'issues'}
= succeed '.' do
!= gfm(@closes_issues.map { |i| "##{i.iid}" }.to_sentence)
......@@ -46,6 +46,11 @@ production: &base
## Users management
# signup_enabled: true # default: false - Account passwords are not sent via the email if signup is enabled.
## Automatic issue closing
# If a commit message matches this regular express, all issues referenced from the matched text will be closed
# if it's pushed to a project's default branch.
# issue_closing_pattern: "^([Cc]loses|[Ff]ixes) +#\d+"
## Default project features settings
default_projects_features:
issues: true
......
......@@ -68,6 +68,7 @@ rescue ArgumentError # no user configured
end
Settings.gitlab['signup_enabled'] ||= false
Settings.gitlab['username_changing_enabled'] = true if Settings.gitlab['username_changing_enabled'].nil?
Settings.gitlab['issue_closing_pattern'] = '^([Cc]loses|[Ff]ixes) #(\d+)' if Settings.gitlab['issue_closing_pattern'].nil?
Settings.gitlab['default_projects_features'] ||= {}
Settings.gitlab.default_projects_features['issues'] = true if Settings.gitlab.default_projects_features['issues'].nil?
Settings.gitlab.default_projects_features['merge_requests'] = true if Settings.gitlab.default_projects_features['merge_requests'].nil?
......
class AddSystemToNotes < ActiveRecord::Migration
class Note < ActiveRecord::Base
end
def up
add_column :notes, :system, :boolean, default: false, null: false
Note.reset_column_information
Note.update_all(system: false)
Note.where("note like '_status changed to%'").update_all(system: true)
end
def down
remove_column :notes, :system
end
end
......@@ -152,6 +152,7 @@ ActiveRecord::Schema.define(:version => 20130821090531) do
t.string "commit_id"
t.integer "noteable_id"
t.text "st_diff"
t.boolean "system", :default => false, :null => false
end
add_index "notes", ["author_id"], :name => "index_notes_on_author_id"
......
module Gitlab
# Extract possible GFM references from an arbitrary String for further processing.
class ReferenceExtractor
attr_accessor :users, :issues, :merge_requests, :snippets, :commits
include Markdown
def initialize
@users, @issues, @merge_requests, @snippets, @commits = [], [], [], [], []
end
def analyze string
parse_references(string.dup)
end
# Given a valid project, resolve the extracted identifiers of the requested type to
# model objects.
def users_for project
users.map do |identifier|
project.users.where(username: identifier).first
end.reject(&:nil?)
end
def issues_for project
issues.map do |identifier|
project.issues.where(iid: identifier).first
end.reject(&:nil?)
end
def merge_requests_for project
merge_requests.map do |identifier|
project.merge_requests.where(iid: identifier).first
end.reject(&:nil?)
end
def snippets_for project
snippets.map do |identifier|
project.snippets.where(id: identifier).first
end.reject(&:nil?)
end
def commits_for project
repo = project.repository
return [] if repo.nil?
commits.map do |identifier|
repo.commit(identifier)
end.reject(&:nil?)
end
private
def reference_link type, identifier
# Append identifier to the appropriate collection.
send("#{type}s") << identifier
end
end
end
require 'spec_helper'
describe Gitlab::ReferenceExtractor do
it 'extracts username references' do
subject.analyze "this contains a @user reference"
subject.users.should == ["user"]
end
it 'extracts issue references' do
subject.analyze "this one talks about issue #1234"
subject.issues.should == ["1234"]
end
it 'extracts merge request references' do
subject.analyze "and here's !43, a merge request"
subject.merge_requests.should == ["43"]
end
it 'extracts snippet ids' do
subject.analyze "snippets like $12 get extracted as well"
subject.snippets.should == ["12"]
end
it 'extracts commit shas' do
subject.analyze "commit shas 98cf0ae3 are pulled out as Strings"
subject.commits.should == ["98cf0ae3"]
end
it 'extracts multiple references and preserves their order' do
subject.analyze "@me and @you both care about this"
subject.users.should == ["me", "you"]
end
it 'leaves the original note unmodified' do
text = "issue #123 is just the worst, @user"
subject.analyze text
text.should == "issue #123 is just the worst, @user"
end
it 'handles all possible kinds of references' do
accessors = Gitlab::Markdown::TYPES.map { |t| "#{t}s".to_sym }
subject.should respond_to(*accessors)
end
context 'with a project' do
let(:project) { create(:project_with_code) }
it 'accesses valid user objects on the project team' do
@u_foo = create(:user, username: 'foo')
@u_bar = create(:user, username: 'bar')
create(:user, username: 'offteam')
project.team << [@u_foo, :reporter]
project.team << [@u_bar, :guest]
subject.analyze "@foo, @baduser, @bar, and @offteam"
subject.users_for(project).should == [@u_foo, @u_bar]
end
it 'accesses valid issue objects' do
@i0 = create(:issue, project: project)
@i1 = create(:issue, project: project)
subject.analyze "##{@i0.iid}, ##{@i1.iid}, and #999."
subject.issues_for(project).should == [@i0, @i1]
end
it 'accesses valid merge requests' do
@m0 = create(:merge_request, source_project: project, target_project: project, source_branch: 'aaa')
@m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'bbb')
subject.analyze "!999, !#{@m1.iid}, and !#{@m0.iid}."
subject.merge_requests_for(project).should == [@m1, @m0]
end