Commit 3d7194f0 authored by Izaak Alpert's avatar Izaak Alpert Committed by Izaak Alpert

Merge Request on forked projects

The good:

 - You can do a merge request for a forked commit and it will merge properly (i.e. it does work).
 - Push events take into account merge requests on forked projects
 - Tests around merge_actions now present, spinach, and other rspec tests
 - Satellites now clean themselves up rather then recreate

The questionable:

 - Events only know about target projects
 - Project's merge requests only hold on to MR's where they are the target
 - All operations performed in the satellite

The bad:

  -  Duplication between project's repositories and satellites (e.g. commits_between)

(for reference: http://feedback.gitlab.com/forums/176466-general/suggestions/3456722-merge-requests-between-projects-repos)

Fixes:

Make test repos/satellites only create when needed
-Spinach/Rspec now only initialize test directory, and setup stubs (things that are relatively cheap)
-project_with_code, source_project_with_code, and target_project_with_code now create/destroy their repos individually
-fixed remote removal
-How to merge renders properly
-Update emails to show project/branches
-Edit MR doesn't set target branch
-Fix some failures on editing/creating merge requests, added a test
-Added back a test around merge request observer
-Clean up project_transfer_spec, Remove duplicate enable/disable observers
-Ensure satellite lock files are cleaned up, Attempted to add some testing around these as well
-Signifant speed ups for tests
-Update formatting ordering in notes_on_merge_requests
-Remove wiki schema update
Fixes for search/search results
-Search results was using by_project for a list of projects, updated this to use in_projects
-updated search results to reference the correct (target) project
-udpated search results to print both sides of the merge request

Change-Id: I19407990a0950945cc95d62089cbcc6262dab1a8
parent fd033671
......@@ -415,6 +415,17 @@ img.emoji {
@extend .light-well;
@extend .light;
margin-bottom: 10px;
.label-project {
@include border-radius(4px);
padding: 2px 4px;
border: none;
font-size: 14px;
background: #474D57;
color: #fff;
font-family: $monospace_font;
text-shadow: 0 1px 1px #111;
font-weight: normal;
}
.group-name {
......
......@@ -12,7 +12,7 @@ class FilterContext
def apply_filter items
if params[:project_id]
items = items.where(project_id: params[:project_id])
items = items.by_project(params[:project_id])
end
if params[:search].present?
......@@ -20,12 +20,12 @@ class FilterContext
end
case params[:status]
when 'closed'
items.closed
when 'all'
items
else
items.opened
when 'closed'
items.closed
when 'all'
items
else
items.opened
end
end
end
......@@ -14,7 +14,7 @@ class MergeRequestsLoadContext < BaseContext
end
merge_requests = merge_requests.page(params[:page]).per(20)
merge_requests = merge_requests.includes(:author, :project).order("created_at desc")
merge_requests = merge_requests.includes(:author, :source_project, :target_project).order("created_at desc")
# Filter by specific assignee_id (or lack thereof)?
if params[:assignee_id].present?
......
......@@ -19,7 +19,7 @@ class SearchContext
if params[:search_code].present?
result[:blobs] = project.repository.search_files(query, params[:repository_ref]) unless project.empty_repo?
else
result[:merge_requests] = MergeRequest.where(project_id: project_ids).search(query).limit(10)
result[:merge_requests] = MergeRequest.in_projects(project_ids).search(query).limit(10)
result[:issues] = Issue.where(project_id: project_ids).search(query).limit(10)
result[:wiki_pages] = []
end
......
......@@ -24,8 +24,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
format.html
format.js
format.diff { render text: @merge_request.to_diff }
format.patch { render text: @merge_request.to_patch }
format.diff { render text: @merge_request.to_diff(current_user) }
format.patch { render text: @merge_request.to_patch(current_user) }
end
end
......@@ -33,25 +33,39 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@commit = @merge_request.last_commit
@comments_allowed = @reply_allowed = true
@comments_target = { noteable_type: 'MergeRequest',
noteable_id: @merge_request.id }
@comments_target = {noteable_type: 'MergeRequest',
noteable_id: @merge_request.id}
@line_notes = @merge_request.notes.where("line_code is not null")
end
def new
@merge_request = @project.merge_requests.new(params[:merge_request])
if params[:merge_request] && params[:merge_request][:source_project_id]
@merge_request.source_project = Project.find_by_id(params[:merge_request][:source_project_id])
else
@merge_request.source_project = @project
end
if params[:merge_request] && params[:merge_request][:target_project_id]
@merge_request.target_project = Project.find_by_id(params[:merge_request][:target_project_id])
end
@target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names
@merge_request
end
def edit
@target_branches = @merge_request.target_project.repository.branch_names
end
def create
@merge_request = @project.merge_requests.new(params[:merge_request])
@merge_request.author = current_user
@merge_request.source_project_id = params[:merge_request][:source_project_id].to_i
@merge_request.target_project_id = params[:merge_request][:target_project_id].to_i
@target_branches ||= []
if @merge_request.save
@merge_request.reload_code
redirect_to [@project, @merge_request], notice: 'Merge request was successfully created.'
redirect_to [@merge_request.target_project, @merge_request], notice: 'Merge request was successfully created.'
else
render "new"
end
......@@ -89,22 +103,36 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def branch_from
#This is always source
@commit = @repository.commit(params[:ref])
end
def branch_to
@commit = @repository.commit(params[:ref])
@target_project = selected_target_project
@commit = @target_project.repository.commit(params[:ref])
end
def update_branches
@target_project = selected_target_project
@target_branches = (@target_project.repository.branch_names).unshift("Select branch")
@target_branches
end
def ci_status
status = project.gitlab_ci_service.commit_status(merge_request.last_commit.sha)
response = { status: status }
response = {status: status}
render json: response
end
protected
def selected_target_project
((@project.id.to_s == params[:target_project_id]) || @project.forked_project_link.nil?) ? @project : @project.forked_project_link.forked_from_project
end
def merge_request
@merge_request ||= @project.merge_requests.find(params[:id])
end
......@@ -123,11 +151,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def validates_merge_request
# Show git not found page if target branch doesn't exist
return invalid_mr unless @project.repository.branch_names.include?(@merge_request.target_branch)
return invalid_mr unless @merge_request.target_project.repository.branch_names.include?(@merge_request.target_branch)
# Show git not found page if source branch doesn't exist
# and there is no saved commits between source & target branch
return invalid_mr if !@project.repository.branch_names.include?(@merge_request.source_branch) && @merge_request.commits.blank?
return invalid_mr if !@merge_request.source_project.repository.branch_names.include?(@merge_request.source_branch) && @merge_request.commits.blank?
end
def define_show_vars
......
......@@ -108,8 +108,8 @@ module CommitsHelper
end
end
def commit_to_html commit
escape_javascript(render 'projects/commits/commit', commit: commit)
def commit_to_html commit, project
escape_javascript(render 'projects/commits/commit', commit: commit, project: project) unless commit.nil?
end
def diff_line_content(line)
......
module MergeRequestsHelper
def new_mr_path_from_push_event(event)
new_project_merge_request_path(
event.project,
merge_request: {
event.project,
new_mr_from_push_event(event, event.project)
)
end
def new_mr_path_for_fork_from_push_event(event)
new_project_merge_request_path(
event.project,
new_mr_from_push_event(event, event.project.forked_from_project)
)
end
def new_mr_from_push_event(event, target_project)
return :merge_request => {
source_project_id: event.project.id,
target_project_id: target_project.id,
source_branch: event.branch_name,
target_branch: event.project.repository.root_ref,
target_branch: target_project.repository.root_ref,
title: event.branch_name.titleize
}
)
}
end
def mr_css_classes mr
......@@ -18,6 +32,6 @@ module MergeRequestsHelper
end
def ci_build_details_path merge_request
merge_request.project.gitlab_ci_service.build_page(merge_request.last_commit.sha)
merge_request.source_project.gitlab_ci_service.build_page(merge_request.last_commit.sha)
end
end
......@@ -2,28 +2,65 @@ module Emails
module MergeRequests
def new_merge_request_email(recipient_id, merge_request_id)
@merge_request = MergeRequest.find(merge_request_id)
@project = @merge_request.project
mail(to: recipient(recipient_id), subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
end
def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
@merge_request = MergeRequest.find(merge_request_id)
@previous_assignee = User.find_by_id(previous_assignee_id) if previous_assignee_id
@project = @merge_request.project
mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
end
def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
@merge_request = MergeRequest.find(merge_request_id)
@project = @merge_request.project
@updated_by = User.find updated_by_user_id
mail(to: recipient(recipient_id), subject: subject("Closed merge request !#{@merge_request.id}", @merge_request.title))
end
def merged_merge_request_email(recipient_id, merge_request_id)
@merge_request = MergeRequest.find(merge_request_id)
@project = @merge_request.project
mail(to: recipient(recipient_id), subject: subject("Accepted merge request !#{@merge_request.id}", @merge_request.title))
end
end
# Over rides default behavour to show source/target
# Formats arguments into a String suitable for use as an email subject
#
# extra - Extra Strings to be inserted into the subject
#
# Examples
#
# >> subject('Lorem ipsum')
# => "GitLab Merge Request | Lorem ipsum"
#
# # Automatically inserts Project name:
# Forked MR
# => source project => <Project id: 1, name: "Ruby on Rails", path: "ruby_on_rails", ...>
# => target project => <Project id: 2, name: "My Ror", path: "ruby_on_rails", ...>
# => source branch => source
# => target branch => target
# >> subject('Lorem ipsum')
# => "GitLab Merge Request | Ruby on Rails:source >> My Ror:target | Lorem ipsum "
#
# Non Forked MR
# => source project => <Project id: 1, name: "Ruby on Rails", path: "ruby_on_rails", ...>
# => target project => <Project id: 1, name: "Ruby on Rails", path: "ruby_on_rails", ...>
# => source branch => source
# => target branch => target
# >> subject('Lorem ipsum')
# => "GitLab Merge Request | Ruby on Rails | source >> target | Lorem ipsum "
# # Accepts multiple arguments
# >> subject('Lorem ipsum', 'Dolor sit amet')
# => "GitLab Merge Request | Lorem ipsum | Dolor sit amet"
def subject(*extra)
subject = "GitLab Merge Request |"
if @merge_request.for_fork?
subject << "#{@merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} >> #{@merge_request.target_project.name_with_namespace}:#{merge_request.target_branch}"
else
subject << "#{@merge_request.source_project.name_with_namespace} | #{merge_request.source_branch} >> #{merge_request.target_branch}"
end
subject << " | " + extra.join(' | ') if extra.present?
subject
end
end
......@@ -9,19 +9,14 @@ module Issuable
include Mentionable
included do
belongs_to :project
belongs_to :author, class_name: "User"
belongs_to :assignee, class_name: "User"
belongs_to :milestone
has_many :notes, as: :noteable, dependent: :destroy
validates :project, presence: true
validates :author, presence: true
validates :title, presence: true, length: { within: 0..255 }
scope :opened, -> { with_state(:opened) }
scope :closed, -> { with_state(:closed) }
scope :of_group, ->(group) { where(project_id: group.project_ids) }
scope :assigned_to, ->(u) { where(assignee_id: u.id)}
scope :recent, -> { order("created_at DESC") }
scope :assigned, -> { where("assignee_id IS NOT NULL") }
......
......@@ -17,8 +17,18 @@
#
class Issue < ActiveRecord::Base
include Issuable
belongs_to :project
validates :project, presence: true
scope :of_group, ->(group) { where(project_id: group.project_ids) }
scope :of_user_team, ->(team) { where(project_id: team.project_ids, assignee_id: team.member_ids) }
scope :opened, -> { with_state(:opened) }
scope :closed, -> { with_state(:closed) }
scope :by_project, ->(project_id) {where(project_id:project_id)}
attr_accessible :title, :assignee_id, :position, :description,
:milestone_id, :label_list, :author_id_of_changes,
:state_event
......
......@@ -2,30 +2,37 @@
#
# Table name: merge_requests
#
# id :integer not null, primary key
# target_branch :string(255) not null
# source_branch :string(255) not null
# project_id :integer not null
# author_id :integer
# assignee_id :integer
# title :string(255)
# created_at :datetime
# updated_at :datetime
# st_commits :text(2147483647)
# st_diffs :text(2147483647)
# milestone_id :integer
# state :string(255)
# merge_status :string(255)
# id :integer not null, primary key
# target_project_id :integer not null
# target_branch :string(255) not null
# source_project_id :integer not null
# source_branch :string(255) not null
# author_id :integer
# assignee_id :integer
# title :string(255)
# created_at :datetime
# updated_at :datetime
# st_commits :text(2147483647)
# st_diffs :text(2147483647)
# milestone_id :integer
# state :string(255)
# merge_status :string(255)
#
require Rails.root.join("app/models/commit")
require Rails.root.join("lib/static_model")
class MergeRequest < ActiveRecord::Base
include Issuable
attr_accessible :title, :assignee_id, :target_branch, :source_branch, :milestone_id,
:author_id_of_changes, :state_event
belongs_to :target_project,:foreign_key => :target_project_id, class_name: "Project"
belongs_to :source_project, :foreign_key => :source_project_id,class_name: "Project"
BROKEN_DIFF = "--broken-diff"
attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id,:author_id_of_changes, :state_event
attr_accessor :should_remove_source_branch
......@@ -74,22 +81,29 @@ class MergeRequest < ActiveRecord::Base
serialize :st_commits
serialize :st_diffs
validates :source_project, presence: true
validates :source_branch, presence: true
validates :target_project, presence: true
validates :target_branch, presence: true
validate :validate_branches
validate :validate_branches
scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)",group_project_ids:group.project_ids) }
scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))",team_project_ids:team.project_ids,team_member_ids:team.member_ids) }
scope :opened, -> { with_state(:opened) }
scope :closed, -> { with_state(:closed) }
scope :merged, -> { with_state(:merged) }
scope :by_branch, ->(branch_name) { where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name) }
scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) }
scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) }
scope :by_milestone, ->(milestone) { where(milestone_id: milestone) }
scope :by_project, ->(project_id) { where("source_project_id = :project_id OR target_project_id = :project_id", project_id: project_id) }
scope :in_projects, ->(project_ids) { where("source_project_id in (:project_ids) OR target_project_id in (:project_ids)", project_ids: project_ids) }
# Closed scope for merge request should return
# both merged and closed mr's
scope :closed, -> { with_states(:closed, :merged) }
def validate_branches
if target_branch == source_branch
errors.add :branch_conflict, "You can not use same branch for source and target branches"
if target_project==source_project && target_branch == source_branch
errors.add :branch_conflict, "You can not use same project/branch for source and target"
end
if opened? || reopened?
......@@ -137,7 +151,14 @@ class MergeRequest < ActiveRecord::Base
end
def unmerged_diffs
project.repository.diffs_between(source_branch, target_branch)
#TODO:[IA-8] this needs to be handled better -- logged etc
diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite
if diffs
diffs = diffs.map { |diff| Gitlab::Git::Diff.new(diff) }
else
diffs = []
end
diffs
end
def last_commit
......@@ -145,11 +166,11 @@ class MergeRequest < ActiveRecord::Base
end
def merge_event
self.project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last
self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last
end
def closed_event
self.project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
end
def commits
......@@ -158,24 +179,30 @@ class MergeRequest < ActiveRecord::Base
def probably_merged?
unmerged_commits.empty? &&
commits.any? && opened?
commits.any? && opened?
end
def reloaded_commits
if opened? && unmerged_commits.any?
self.st_commits = dump_commits(unmerged_commits)
save
end
commits
end
def unmerged_commits
self.project.repository.
commits_between(self.target_branch, self.source_branch).
sort_by(&:created_at).
reverse
commits = Gitlab::Satellite::MergeAction.new(self.author,self).commits_between
commits = commits.map{ |commit| Gitlab::Git::Commit.new(commit, nil) }
if commits.present?
commits = Commit.decorate(commits).
sort_by(&:created_at).
reverse
end
commits
end
def merge!(user_id)
self.author_id_of_changes = user_id
self.merge
......@@ -195,25 +222,33 @@ class MergeRequest < ActiveRecord::Base
commit_ids = commits.map(&:id)
Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND commit_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids)
end
# Returns the raw diff for this merge request
#
# see "git diff"
def to_diff
project.repo.git.native(:diff, {timeout: 30, raise: true}, "#{target_branch}...#{source_branch}")
def to_diff(current_user)
Gitlab::Satellite::MergeAction.new(current_user, self).diff_in_satellite
end
# Returns the commit as a series of email patches.
#
# see "git format-patch"
def to_patch
project.repo.git.format_patch({timeout: 30, raise: true, stdout: true}, "#{target_branch}..#{source_branch}")
def to_patch(current_user)
Gitlab::Satellite::MergeAction.new(current_user, self).format_patch
end
def last_commit_short_sha
@last_commit_short_sha ||= last_commit.sha[0..10]
end
def for_fork?
target_project != source_project
end
def disallow_source_branch_removal?
(source_project.root_ref? source_branch) || for_fork?
end
private
def dump_commits(commits)
......
......@@ -32,8 +32,8 @@ class Note < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true
validates :note, :project, presence: true
validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
validates :attachment, file_size: { maximum: 10.megabytes.to_i }
validates :line_code, format: {with: /\A[a-z0-9]+_\d+_\d+\Z/}, allow_blank: true
validates :attachment, file_size: {maximum: 10.megabytes.to_i}
validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' }
validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' }
......@@ -45,24 +45,24 @@ class Note < ActiveRecord::Base
scope :inline, -> { where("line_code IS NOT NULL") }
scope :not_inline, -> { where(line_code: [nil, '']) }
scope :common, ->{ where(noteable_type: ["", nil]) }
scope :fresh, ->{ order("created_at ASC, id ASC") }
scope :inc_author_project, ->{ includes(:project, :author) }
scope :inc_author, ->{ includes(:author) }
scope :common, -> { where(noteable_type: ["", nil]) }
scope :fresh, -> { order("created_at ASC, id ASC") }
scope :inc_author_project, -> { includes(:project, :author) }
scope :inc_author, -> { includes(:author) }
def self.create_status_change_note(noteable, author, status)
def self.create_status_change_note(noteable, project, author, status)
create({
noteable: noteable,
project: noteable.project,
author: author,
note: "_Status changed to #{status}_"
}, without_protection: true)
noteable: noteable,
project: project,
author: author,
note: "_Status changed to #{status}_"
}, without_protection: true)
end
def commit_author
@commit_author ||=
project.users.find_by_email(noteable.author_email) ||
project.users.find_by_name(noteable.author_name)
project.users.find_by_email(noteable.author_email) ||
project.users.find_by_name(noteable.author_name)
rescue
nil
end
......@@ -97,8 +97,8 @@ class Note < ActiveRecord::Base
# otherwise false is returned
def downvote?
votable? && (note.start_with?('-1') ||
note.start_with?(':-1:')
)
note.start_with?(':-1:')
)
end
def for_commit?
......@@ -136,8 +136,8 @@ class Note < ActiveRecord::Base
else
super
end
# Temp fix to prevent app crash
# if note commit id doesn't exist
# Temp fix to prevent app crash
# if note commit id doesn't exist
rescue
nil
end
......@@ -146,8 +146,8 @@ class Note < ActiveRecord::Base
# otherwise false is returned
def upvote?
votable? && (note.start_with?('+1') ||
note.start_with?(':+1:')
)
note.start_with?(':+1:')
)
end
def votable?
......
......@@ -53,7 +53,7 @@ class Project < ActiveRecord::Base
has_many :services, dependent: :destroy
has_many :events, dependent: :destroy
has_many :merge_requests, dependent: :destroy
has_many :merge_requests, dependent: :destroy, foreign_key: "target_project_id"
has_many :issues, dependent: :destroy, order: "state DESC, created_at DESC"
has_many :milestones, dependent: :destroy
has_many :notes, dependent: :destroy
......
class ActivityObserver < BaseObserver
observe :issue, :merge_request, :note, :milestone
observe :issue, :note, :milestone
def after_create(record)
event_author_id = record.author_id
......@@ -13,47 +13,27 @@ class ActivityObserver < BaseObserver
end
if event_author_id
Event.create(
project: record.project,
target_id: record.id,
target_type: record.class.name,
action: Event.determine_action(record),
author_id: event_author_id
)
create_event(record, Event.determine_action(record))
end
end
def after_close(record, transition)
Event.create(
project: record.project,
target_id: record.id,
target_type: record.class.name,
action: Event::CLOSED,
author_id: record.author_id_of_changes
)
create_event(record, Event::CLOSED)
end
def after_reopen(record, transition)
Event.create(
project: record.project,
target_id: record.id,
target_type: record.class.name,
action: Event::REOPENED,
author_id: record.author_id_of_changes
)
create_event(record, Event::REOPENED)
end
def after_merge(record, transition)
# Since MR can be merged via sidekiq
# to prevent event duplication do this check
return true if record.merge_event
protected
def create_event(record, status)
Event.create(
project: record.project,
target_id: record.id,
target_type: record.class.name,
action: Event::MERGED,
author_id: record.author_id_of_changes
project: record.project,
target_id: record.id,
target_type: record.class.name,
action: status,