Commit 551946a3 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge pull request #4507 from smashwilson/linking-issues

Linking objects from GFM references
parents 2b36dee6 c8a115c0
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
it 'accesses valid snippets' do
@s0 = create(:project_snippet, project: project)
@s1 = create(:project_snippet, project: project)
@s2 = create(:project_snippet)
subject.analyze "$#{@s0.id}, $999, $#{@s2.id}, $#{@s1.id}"
subject.snippets_for(project).should == [@s0, @s1]
end
it 'accesses valid commits' do
commit = project.repository.commit("master")
subject.analyze "this references commits #{commit.sha[0..6]} and 012345"
extracted = subject.commits_for(project)
extracted.should have(1).item
extracted[0].sha.should == commit.sha
extracted[0].message.should == commit.message
end
end
end
require 'spec_helper'
describe Commit do
let(:commit) { create(:project_with_code).repository.commit }
let(:project) { create :project_with_code }
let(:commit) { project.repository.commit }
describe '#title' do
it "returns no_commit_message when safe_message is blank" do
......@@ -46,4 +47,23 @@ describe Commit do
it { should respond_to(:id) }
it { should respond_to(:to_patch) }
end
describe '#closes_issues' do
let(:issue) { create :issue, project: project }
it 'detects issues that this commit is marked as closing' do
commit.stub(issue_closing_regex: /^([Cc]loses|[Ff]ixes) #\d+/, safe_message: "Fixes ##{issue.iid}")
commit.closes_issues(project).should == [issue]
end
end
it_behaves_like 'a mentionable' do
let(:subject) { commit }
let(:mauthor) { create :user, email: commit.author_email }
let(:backref_text) { "commit #{subject.sha[0..5]}" }
let(:set_mentionable_text) { ->(txt){ subject.stub(safe_message: txt) } }
# Include the subject in the repository stub.
let(:extra_commits) { [subject] }
end
end
......@@ -56,4 +56,10 @@ describe Issue do
Issue.open_for(user).count.should eq 2
end
end
it_behaves_like 'an editable mentionable' do
let(:subject) { create :issue, project: mproject }
let(:backref_text) { "issue ##{subject.iid}" }
let(:set_mentionable_text) { ->(txt){ subject.description = txt } }
end
end
......@@ -43,7 +43,6 @@ describe MergeRequest do
it { should include_module(Issuable) }
end
describe "#mr_and_commit_notes" do
let!(:merge_request) { create(:merge_request) }
......@@ -104,4 +103,34 @@ describe MergeRequest do
end
end
describe 'detection of issues to be closed' do
let(:issue0) { create :issue, project: subject.project }
let(:issue1) { create :issue, project: subject.project }
let(:commit0) { mock('commit0', closes_issues: [issue0]) }
let(:commit1) { mock('commit1', closes_issues: [issue0]) }
let(:commit2) { mock('commit2', closes_issues: [issue1]) }
before do
subject.stub(unmerged_commits: [commit0, commit1, commit2])
end
it 'accesses the set of issues that will be closed on acceptance' do
subject.project.default_branch = subject.target_branch
subject.closes_issues.should == [issue0, issue1].sort_by(&:id)
end
it 'only lists issues as to be closed if it targets the default branch' do
subject.project.default_branch = 'master'
subject.target_branch = 'something-else'
subject.closes_issues.should be_empty
end
end
it_behaves_like 'an editable mentionable' do
let(:subject) { create :merge_request, source_project: mproject, target_project: mproject }
let(:backref_text) { "merge request !#{subject.iid}" }
let(:set_mentionable_text) { ->(txt){ subject.title = txt } }
end
end
......@@ -72,7 +72,6 @@ describe Note do
end
let(:project) { create(:project) }
let(:commit) { project.repository.commit }
describe "Commit notes" do
let!(:note) { create(:note_on_commit, note: "+1 from me") }
......@@ -131,7 +130,7 @@ describe Note do
describe "Merge request notes" do
let!(:note) { create(:note_on_merge_request, note: "+1 from me") }
it "should not be votable" do
it "should be votable" do
note.should be_votable
end
end
......@@ -150,7 +149,7 @@ describe Note do
let(:author) { create(:user) }
let(:status) { 'new_status' }
subject { Note.create_status_change_note(thing, project, author, status) }
subject { Note.create_status_change_note(thing, project, author, status, nil) }
it 'creates and saves a Note' do
should be_a Note
......@@ -161,6 +160,102 @@ describe Note do
its(:project) { should == thing.project }
its(:author) { should == author }
its(:note) { should =~ /Status changed to #{status}/ }
it 'appends a back-reference if a closing mentionable is supplied' do
commit = double('commit', gfm_reference: 'commit 123456')
n = Note.create_status_change_note(thing, project, author, status, commit)
n.note.should =~ /Status changed to #{status} by commit 123456/
end
end
describe '#create_cross_reference_note' do
let(:project) { create(:project_with_code) }
let(:author) { create(:user) }
let(:issue) { create(:issue, project: project) }
let(:mergereq) { create(:merge_request, target_project: project) }
let(:commit) { project.repository.commit }
# Test all of {issue, merge request, commit} in both the referenced and referencing
# roles, to ensure that the correct information can be inferred from any argument.
context 'issue from a merge request' do
subject { Note.create_cross_reference_note(issue, mergereq, author, project) }
it { should be_valid }
its(:noteable) { should == issue }
its(:project) { should == issue.project }
its(:author) { should == author }
its(:note) { should == "_mentioned in merge request !#{mergereq.iid}_" }
end
context 'issue from a commit' do
subject { Note.create_cross_reference_note(issue, commit, author, project) }
it { should be_valid }
its(:noteable) { should == issue }
its(:note) { should == "_mentioned in commit #{commit.sha[0..5]}_" }
end
context 'merge request from an issue' do
subject { Note.create_cross_reference_note(mergereq, issue, author, project) }
it { should be_valid }
its(:noteable) { should == mergereq }
its(:project) { should == mergereq.project }
its(:note) { should == "_mentioned in issue ##{issue.iid}_" }
end
context 'commit from a merge request' do
subject { Note.create_cross_reference_note(commit, mergereq, author, project) }
it { should be_valid }
its(:noteable) { should == commit }
its(:project) { should == project }
its(:note) { should == "_mentioned in merge request !#{mergereq.iid}_" }
end
end
describe '#cross_reference_exists?' do
let(:project) { create :project }
let(:author) { create :user }
let(:issue) { create :issue }
let(:commit0) { double 'commit0', gfm_reference: 'commit 123456' }
let(:commit1) { double 'commit1', gfm_reference: 'commit 654321' }
before do
Note.create_cross_reference_note(issue, commit0, author, project)
end
it 'detects if a mentionable has already been mentioned' do
Note.cross_reference_exists?(issue, commit0).should be_true
end
it 'detects if a mentionable has not already been mentioned' do
Note.cross_reference_exists?(issue, commit1).should be_false
end
end
describe '#system?' do
let(:project) { create(:project) }
let(:issue) { create(:issue, project: project) }
let(:other) { create(:issue, project: project) }
let(:author) { create(:user) }
it 'should recognize user-supplied notes as non-system' do
@note = create(:note_on_issue)
@note.should_not be_system
end
it 'should identify status-change notes as system notes' do
@note = Note.create_status_change_note(issue, project, author, 'closed', nil)
@note.should be_system
end
it 'should identify cross-reference notes as system notes' do
@note = Note.create_cross_reference_note(issue, other, author, project)
@note.should be_system
end
end
describe :authorization do
......@@ -208,4 +303,11 @@ describe Note do
it { @abilities.allowed?(@u3, :admin_note, @p1).should be_false }
end
end
it_behaves_like 'an editable mentionable' do
let(:issue) { create :issue, project: project }
let(:subject) { create :note, noteable: issue, project: project }
let(:backref_text) { issue.gfm_reference }
let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
end
end
......@@ -3,6 +3,8 @@ require 'spec_helper'
describe ActivityObserver do
let(:project) { create(:project) }
before { Thread.current[:current_user] = create(:user) }
def self.it_should_be_valid_event
it { @event.should_not be_nil }
it { @event.project.should == project }
......@@ -34,4 +36,26 @@ describe ActivityObserver do
it { @event.action.should == Event::COMMENTED }
it { @event.target.should == @note }
end
describe "Ignore system notes" do
let(:author) { create(:user) }
let!(:issue) { create(:issue, project: project) }
let!(:other) { create(:issue) }
it "should not create events for status change notes" do
expect do
Note.observers.enable :activity_observer do
Note.create_status_change_note(issue, project, author, 'reopened', nil)
end
end.to_not change { Event.count }
end
it "should not create events for cross-reference notes" do
expect do
Note.observers.enable :activity_observer do
Note.create_cross_reference_note(issue, other, author, issue.project)
end
end.to_not change { Event.count }
end
end
end
......@@ -8,8 +8,9 @@ describe IssueObserver do
before { subject.stub(:current_user).and_return(some_user) }
before { subject.stub(:current_commit).and_return(nil) }
before { subject.stub(notification: mock('NotificationService').as_null_object) }
before { mock_issue.project.stub_chain(:repository, :commit).and_return(nil) }
subject { IssueObserver.instance }
......@@ -19,6 +20,15 @@ describe IssueObserver do
subject.after_create(mock_issue)
end
it 'should create cross-reference notes' do
other_issue = create(:issue)
mock_issue.stub(references: [other_issue])
Note.should_receive(:create_cross_reference_note).with(other_issue, mock_issue,
some_user, mock_issue.project)
subject.after_create(mock_issue)
end
end
context '#after_close' do
......@@ -26,7 +36,7 @@ describe IssueObserver do
before { mock_issue.stub(state: 'closed') }
it 'note is created if the issue is being closed' do
Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed')
Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', nil)
subject.after_close(mock_issue, nil)
end
......@@ -35,13 +45,23 @@ describe IssueObserver do
subject.notification.should_receive(:close_issue).with(mock_issue, some_user)
subject.after_close(mock_issue, nil)
end
it 'appends a mention to the closing commit if one is present' do
commit = double('commit', gfm_reference: 'commit 123456')
subject.stub(current_commit: commit)
Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', commit)
subject.after_close(mock_issue, nil)
end
end
context 'a status "reopened"' do
before { mock_issue.stub(state: 'reopened') }
it 'note is created if the issue is being reopened' do
Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'reopened')
Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'reopened', nil)
subject.after_reopen(mock_issue, nil)
end
end
......@@ -67,5 +87,13 @@ describe IssueObserver do
subject.after_update(mock_issue)
end
end
context 'cross-references' do
it 'notices added references' do
mock_issue.should_receive(:notice_added_references)
subject.after_update(mock_issue)
end
end
end
end
......@@ -5,15 +5,20 @@ describe MergeRequestObserver do
let(:assignee) { create :user }
let(:author) { create :user }
let(:mr_mock) { double(:merge_request, id: 42, assignee: assignee, author: author) }
let(:assigned_mr) { create(:merge_request, assignee: assignee, author: author) }
let(:unassigned_mr) { create(:merge_request, author: author) }
let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author) }
let(:closed_unassigned_mr) { create(:closed_merge_request, author: author) }
let(:assigned_mr) { create(:merge_request, assignee: assignee, author: author, target_project: create(:project)) }
let(:unassigned_mr) { create(:merge_request, author: author, target_project: create(:project)) }
let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author, target_project: create(:project)) }
let(:closed_unassigned_mr) { create(:closed_merge_request, author: author, target_project: create(:project)) }
before { subject.stub(:current_user).and_return(some_user) }
before { subject.stub(notification: mock('NotificationService').as_null_object) }
before { mr_mock.stub(:author_id) }
before { mr_mock.stub(:target_project) }
before { mr_mock.stub(:source_project) }
before { mr_mock.stub(:project) }
before { mr_mock.stub(:create_cross_references!).and_return(true) }
before { Repository.any_instance.stub(commit: nil) }
before(:each) { enable_observers }
after(:each) { disable_observers }
......@@ -24,11 +29,20 @@ describe MergeRequestObserver do
subject.should_receive(:notification)
subject.after_create(mr_mock)
end
it 'creates cross-reference notes' do
project = create :project
mr_mock.stub(title: "this mr references !#{assigned_mr.id}", project: project)
mr_mock.should_receive(:create_cross_references!).with(project, some_user)
subject.after_create(mr_mock)
end
end
context '#after_update' do
before(:each) do
mr_mock.stub(:is_being_reassigned?).and_return(false)
mr_mock.stub(:notice_added_references)
end
it 'is called when a merge request is changed' do
......@@ -41,6 +55,12 @@ describe MergeRequestObserver do
end
end
it 'checks for new references' do
mr_mock.should_receive(:notice_added_references)
subject.after_update(mr_mock)
end
context 'a notification' do
it 'is sent if the merge request is being reassigned' do
mr_mock.should_receive(:is_being_reassigned?).and_return(true)
......@@ -61,13 +81,13 @@ describe MergeRequestObserver do
context '#after_close' do
context 'a status "closed"' do
it 'note is created if the merge request is being closed' do
Note.should_receive(:create_status_change_note).with(assigned_mr, assigned_mr.target_project, some_user, 'closed')
Note.should_receive(:create_status_change_note).with(assigned_mr, assigned_mr.target_project, some_user, 'closed', nil)
assigned_mr.close
end
it 'notification is delivered only to author if the merge request is being closed' do
Note.should_receive(:create_status_change_note).with(unassigned_mr, unassigned_mr.target_project, some_user, 'closed')
Note.should_receive(:create_status_change_note).with(unassigned_mr, unassigned_mr.target_project, some_user, 'closed', nil)
unassigned_mr.close
end
......@@ -77,13 +97,13 @@ describe MergeRequestObserver do
context '#after_reopen' do
context 'a status "reopened"' do
it 'note is created if the merge request is being reopened' do
Note.should_receive(:create_status_change_note).with(closed_assigned_mr, closed_assigned_mr.target_project, some_user, 'reopened')
Note.should_receive(:create_status_change_note).with(closed_assigned_mr, closed_assigned_mr.target_project, some_user, 'reopened', nil)
closed_assigned_mr.reopen
end
it 'notification is delivered only to author if the merge request is being reopened' do
Note.should_receive(:create_status_change_note).with(closed_unassigned_mr, closed_unassigned_mr.target_project, some_user, 'reopened')
Note.should_receive(:create_status_change_note).with(closed_unassigned_mr, closed_unassigned_mr.target_project, some_user, 'reopened', nil)
closed_unassigned_mr.reopen
end
......
......@@ -5,9 +5,9 @@ describe NoteObserver do
before { subject.stub(notification: mock('NotificationService').as_null_object) }
let(:team_without_author) { (1..2).map { |n| double :user, id: n } }
let(:note) { double(:note).as_null_object }
describe '#after_create' do
let(:note) { double :note }
it 'is called after a note is created' do
subject.should_receive :after_create
......@@ -22,5 +22,35 @@ describe NoteObserver do
subject.after_create(note)
end
it 'creates cross-reference notes as appropriate' do
@p = create(:project)
@referenced = create(:issue, project: @p)
@referencer = create(:issue, project: @p)
@author = create(:user)
Note.should_receive(:create_cross_reference_note).with(@referenced, @referencer, @author, @p)
Note.observers.enable :note_observer do
create(:note, project: @p, author: @author, noteable: @referencer,
note: "Duplicate of ##{@referenced.iid}")
end
end
it "doesn't cross-reference system notes" do
Note.should_receive(:create_cross_reference_note).once
Note.observers.enable :note_observer do
Note.create_cross_reference_note(create(:issue), create(:issue))
end
end
end
describe '#after_update' do
it 'checks for new cross-references' do
note.should_receive(:notice_added_references)
subject.after_update(note)
end
end
end
......@@ -6,6 +6,7 @@ describe GitPushService do
let (:service) { GitPushService.new }
before do
@blankrev = '0000000000000000000000000000000000000000'
@oldrev = 'b98a310def241a6fd9c9a9a3e7934c48e498fe81'
@newrev = 'b19a04f53caeebf4fe5ec2327cb83e9253dc91bb'
@ref = 'refs/heads/master'
......@@ -98,7 +99,7 @@ describe GitPushService do
it "when pushing a branch for the first time" do
@project_hook.should_not_receive(:execute)
service.execute(project, user, '00000000000000000000000000000000', 'newrev', 'refs/heads/master')
service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
end
it "when pushing tags" do
......@@ -107,5 +108,107 @@ describe GitPushService do
end
end
end
describe "cross-reference notes" do
let(:issue) { create :issue, project: project }
let(:commit_author) { create :user }
let(:commit) { project.repository.commit }
before do
commit.stub({
safe_message: "this commit \n mentions ##{issue.id}",
references: [issue],
author_name: commit_author.name,
author_email: commit_author.email
})
project.repository.stub(commits_between: [commit])
end
it "creates a note if a pushed commit mentions an issue" do
Note.should_receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
service.execute(project, user, @oldrev, @newrev, @ref)
end
it "only creates a cross-reference note if one doesn't already exist" do
Note.create_cross_reference_note(issue, commit, user, project)
Note.should_not_receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
service.execute(project, user, @oldrev, @newrev, @ref)
end
it "defaults to the pushing user if the commit's author is not known" do
commit.stub(author_name: 'unknown name', author_email: 'unknown@email.com')
Note.should_receive(:create_cross_reference_note).with(issue, commit, user, project)
service.execute(project, user, @oldrev, @newrev, @ref)
end
it "finds references in the first push to a non-default branch" do
project.repository.stub(:commits_between).with(@blankrev, @newrev).and_return([])
project.repository.stub(:commits_between).with("master", @newrev).and_return([commit])
Note.should_receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
service.execute(project, user, @blankrev, @newrev, 'refs/heads/other')
end
it "finds references in the first push to a default branch" do
project.repository.stub(:commits_between).with(@blankrev, @newrev).and_return([])
project.repository.stub(:commits).with(@newrev).and_return([commit])
Note.should_receive(:create_cross_reference_note).with(issue, commit, commit_author, project)
service.execute(project, user, @blankrev, @newrev, 'refs/heads/master')
end
end
describe "closing issues from pushed commits" do
let(:issue) { create :issue, project: project }
let(:other_issue) { create :issue, project: project }
let(:commit_author) { create :user }
let(:closing_commit) { project.repository.commit }
before do
closing_commit.stub({
issue_closing_regex: /^([Cc]loses|[Ff]ixes) #\d+/,
safe_message: "this is some work.\n\ncloses ##{issue.iid}",
author_name: commit_author.name,
author_email: commit_author.email
})
project.repository.stub(commits_between: [closing_commit])
end
it "closes issues with commit messages" do
service.execute(project, user, @oldrev, @newrev, @ref)
Issue.find(issue.id).should be_closed
end
it "passes the closing commit as a thread-local" do
service.execute(project, user, @oldrev, @newrev, @ref)
Thread.current[:current_commit].should == closing_commit
end
it "doesn't create cross-reference notes for a closing reference" do
expect {
service.execute(project, user, @oldrev, @newrev, @ref)
}.not_to change { Note.where(project_id: project.id, system: true).count }
end
it "doesn't close issues when pushed to non-default branches" do
project.stub(default_branch: 'durf')
# The push still shouldn't create cross-reference notes.
expect {
service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
}.not_to change { Note.where(project_id: project.id, system: true).count }
Issue.find(issue.id).should be_opened
end
end
end
......@@ -18,6 +18,7 @@ module LoginHelpers
fill_in "user_login", with: user.email
fill_in "user_password", with: "123456"
click_button "Sign in"
Thread.current[:current_user] = user
end
def logout
......
# Specifications for behavior common to all Mentionable implementations.
# Requires a shared context containing:
# - let(:subject) { "the mentionable implementation" }
# - let(:backref_text) { "the way that +subject+ should refer to itself in backreferences " }
# - let(:set_mentionable_text) { lambda { |txt| "block that assigns txt to the subject's mentionable_text" } }
def common_mentionable_setup
# Avoid name collisions with let(:project) or let(:author) in the surrounding scope.
let(:mproject) { create :project }
let(:mauthor) { subject.author }
let(:mentioned_issue) { create :issue, project: mproject }
let(:other_issue) { create :issue, project: mproject }
let(:mentioned_mr) { create :merge_request, target_project: mproject, source_branch: 'different' }
let(:mentioned_commit) { mock('commit', sha: '1234567890abcdef').as_null_object }
# Override to add known commits to the repository stub.
let(:extra_commits) { [] }
# A string that mentions each of the +mentioned_.*+ objects above. Mentionables should add a self-reference
# to this string and place it in their +mentionable_text+.
let(:ref_string) do
"mentions ##{mentioned_issue.iid} twice ##{mentioned_issue.iid}, !#{mentioned_mr.iid}, " +
"#{mentioned_commit.sha[0..5]} and itself as #{backref_text}"
end
before do
# Wire the project's repository to return the mentioned commit, and +nil+ for any
# unrecognized commits.
commitmap = { '123456' => mentioned_commit }
extra_commits.each { |c| commitmap[c.sha[0..5]] = c }
repo = mock('repository')
repo.stub(:commit) { |sha| commitmap[sha] }
mproject.stub(repository: repo)
set_mentionable_text.call(ref_string)
end
end
shared_examples 'a mentionable' do
common_mentionable_setup
it 'generates a descriptive back-reference' do
subject.gfm_reference.should == backref_text
end
it "extracts references from its reference property" do
# De-duplicate and omit itself
refs = subject.references(mproject)
refs.should have(3).items
refs.should include(mentioned_issue)
refs.should include(mentioned_mr)
refs.should include(mentioned_commit)
end
it 'creates cross-reference notes' do
[mentioned_issue, mentioned_mr, mentioned_commit].each do |referenced|
Note.should_receive(:create_cross_reference_note).with(referenced, subject.local_reference, mauthor, mproject)
end
subject.create_cross_references!(mproject, mauthor)
end
it 'detects existing cross-references' do
Note.create_cross_reference_note(mentioned_issue, subject.local_reference, mauthor, mproject)
subject.has_mentioned?(mentioned_issue).should be_true
subject.has_mentioned?(mentioned_mr).should be_false
end
end
shared_examples 'an editable mentionable' do
common_mentionable_setup
it_behaves_like 'a mentionable'
it 'creates new cross-reference notes when the mentionable text is edited' do
new_text = "this text still mentions ##{mentioned_issue.iid} and #{mentioned_commit.sha[0..5]}, " +
"but now it mentions ##{other_issue.iid}, too."
[mentioned_issue, mentioned_commit].each do |oldref|
Note.should_not_receive(:create_cross_reference_note).with(oldref, subject.local_reference,
mauthor, mproject)
end
Note.should_receive(:create_cross_reference_note).with(other_issue, subject.local_reference, mauthor, mproject)
subject.save
set_mentionable_text.call(new_text)
subject.notice_added_references(mproject, mauthor)
end
end
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