Commit d9959427 authored by Izaak Alpert's avatar Izaak Alpert

Style changes from review with @randx

-Some changes around calling origional methods for !for_fork? merge requests. Other changes to follow

Change-Id: I009c716ce2475b9efa3fd07aee9215fca7a1c150
parent 128f2845
......@@ -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
......@@ -124,7 +124,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@target_branches
end
def ci_status
status = project.gitlab_ci_service.commit_status(merge_request.last_commit.sha)
response = {status: status}
......@@ -134,12 +133,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
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 ||= MergeRequest.find_by_id(params[:id])
end
......
module MergeRequestsHelper
def new_mr_path_from_push_event(event)
new_project_merge_request_path(
event.project,
new_mr_from_push_event(event, event.project)
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)
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: target_project.repository.root_ref,
title: event.branch_name.titleize
source_project_id: event.project.id,
target_project_id: target_project.id,
source_branch: event.branch_name,
target_branch: target_project.repository.root_ref,
title: event.branch_name.titleize
}
end
......
......@@ -2,7 +2,7 @@ module Emails
module MergeRequests
def new_merge_request_email(recipient_id, merge_request_id)
@merge_request = MergeRequest.find(merge_request_id)
mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
mail(to: recipient(recipient_id), 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)
......
......@@ -26,8 +26,8 @@ class MergeRequest < ActiveRecord::Base
include Issuable
belongs_to :target_project, :foreign_key => :target_project_id, class_name: "Project"
belongs_to :source_project, :foreign_key => :source_project_id, class_name: "Project"
belongs_to :target_project, foreign_key: :target_project_id, class_name: "Project"
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
......@@ -149,7 +149,11 @@ class MergeRequest < ActiveRecord::Base
end
def unmerged_diffs
diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite
if for_fork?
diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite
else
diffs = target_project.repository.diffs_between(source_branch, target_branch)
end
diffs ||= []
diffs
end
......@@ -172,7 +176,7 @@ class MergeRequest < ActiveRecord::Base
def probably_merged?
unmerged_commits.empty? &&
commits.any? && opened?
commits.any? && opened?
end
def reloaded_commits
......@@ -185,11 +189,15 @@ class MergeRequest < ActiveRecord::Base
end
def unmerged_commits
commits = Gitlab::Satellite::MergeAction.new(self.author, self).commits_between
if for_fork?
commits = Gitlab::Satellite::MergeAction.new(self.author, self).commits_between
else
commits = target_project.repository.commits_between(self.target_branch, self.source_branch)
end
if commits.present?
commits = Commit.decorate(commits).
sort_by(&:created_at).
reverse
sort_by(&:created_at).
reverse
end
commits
end
......
......@@ -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' }
......@@ -42,13 +42,13 @@ class Note < ActiveRecord::Base
# Scopes
scope :for_commit_id, ->(commit_id) { where(noteable_type: "Commit", commit_id: commit_id) }
scope :inline, -> { where("line_code IS NOT NULL") }
scope :not_inline, -> { where(line_code: [nil, '']) }
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, project, author, status)
create({
......@@ -61,8 +61,8 @@ class Note < ActiveRecord::Base
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?
......@@ -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?
......
......@@ -2,8 +2,7 @@ class MergeRequestObserver < ActivityObserver
observe :merge_request
def after_create(merge_request)
event_author_id = merge_request.author_id
if event_author_id
if merge_request.author_id
create_event(merge_request, Event.determine_action(merge_request))
end
......@@ -24,11 +23,11 @@ class MergeRequestObserver < ActivityObserver
return true if merge_request.merge_event
Event.create(
project: merge_request.target_project,
target_id: merge_request.id,
target_type: merge_request.class.name,
action: Event::MERGED,
author_id: merge_request.author_id_of_changes
project: merge_request.target_project,
target_id: merge_request.id,
target_type: merge_request.class.name,
action: Event::MERGED,
author_id: merge_request.author_id_of_changes
)
end
......@@ -41,14 +40,13 @@ class MergeRequestObserver < ActivityObserver
notification.reassigned_merge_request(merge_request, current_user) if merge_request.is_being_reassigned?
end
def create_event(record, status)
Event.create(
project: record.target_project,
target_id: record.id,
target_type: record.class.name,
action: status,
author_id: record.author_id
project: record.target_project,
target_id: record.id,
target_type: record.class.name,
action: status,
author_id: record.author_id
)
end
......
......@@ -80,9 +80,7 @@ class NotificationService
# * project team members with notification level higher then Participating
#
def merge_mr(merge_request)
recipients = reject_muted_users([merge_request.author, merge_request.assignee], merge_request.source_project)
recipients = recipients.concat(reject_muted_users([merge_request.author, merge_request.assignee], merge_request.target_project))
recipients = recipients.concat(project_watchers(merge_request.source_project))
recipients = reject_muted_users([merge_request.author, merge_request.assignee], merge_request.target_project)
recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq
recipients.each do |recipient|
......@@ -104,7 +102,7 @@ class NotificationService
# ignore wall messages
return true unless note.noteable_type.present?
opts = {noteable_type: note.noteable_type, project_id: note.project_id}
opts = { noteable_type: note.noteable_type, project_id: note.project_id }
if note.commit_id.present?
opts.merge!(commit_id: note.commit_id)
......
......@@ -5,4 +5,4 @@
%span= day.stamp("28 Aug, 2010")
.pull-right
%small= pluralize(commits.count, 'commit')
%ul.well-list= render commits, :project => @project
%ul.well-list= render commits, project: @project
= form_for [@source_project, @merge_request], html: { class: "#{controller.action_name}-merge-request form-horizontal" } do |form_helper|
= form_for [@source_project, @merge_request], html: { class: "#{controller.action_name}-merge-request form-horizontal" } do |f|
-if @merge_request.errors.any?
.alert.alert-error
%ul
......@@ -51,9 +51,9 @@
.form-actions
- if @merge_request.new_record?
= form_helper.submit 'Submit merge request', class: "btn btn-create"
= f.submit 'Submit merge request', class: "btn btn-create"
-else
= form_helper.submit 'Save changes', class: "btn btn-save"
= f.submit 'Save changes', class: "btn btn-save"
- if @merge_request.new_record?
= link_to project_merge_requests_path(@source_project), class: "btn btn-cancel" do
Cancel
......
......@@ -8,9 +8,14 @@
= "MERGED"
- else
%span.pull-right
= "#{merge_request.source_project.path_with_namespace}/#{merge_request.source_branch}"
%i.icon-angle-right
= "#{merge_request.target_project.path_with_namespace}/#{merge_request.target_branch}"
- if merge_request.for_fork?
= "#{merge_request.source_project.path_with_namespace}/#{merge_request.source_branch}"
%i.icon-angle-right
= "#{merge_request.target_project.path_with_namespace}/#{merge_request.target_branch}"
- else
= "#{merge_request.source_branch}"
%i.icon-angle-right
= "#{merge_request.target_branch}"
.merge-request-info
- if merge_request.author
authored by #{link_to_member(merge_request.source_project, merge_request.author)}
......
%h3.page-title
= "Merge Request ##{@merge_request.id}:"
&nbsp;
%span.label-project= @merge_request.source_project.path_with_namespace
%span.label-branch= @merge_request.source_branch
&rarr;
%span.label-project= @merge_request.target_project.path_with_namespace
%span.label-branch= @merge_request.target_branch
-if @merge_request.for_fork?
%span.label-project= @merge_request.source_project.path_with_namespace
%span.label-branch= @merge_request.source_branch
&rarr;
%span.label-project= @merge_request.target_project.path_with_namespace
%span.label-branch= @merge_request.target_branch
- else
%span.label-branch= @merge_request.source_branch
&rarr;
%span.label-branch= @merge_request.target_branch
%span.pull-right
- if can?(current_user, :modify_merge_request, @merge_request)
......
......@@ -26,7 +26,10 @@
%span ##{merge_request.id}
%strong.term
= truncate merge_request.title, length: 50
%span.light (#{merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} &rarr; #{merge_request.target_project.name_with_namespace}:#{merge_request.target_branch})
- if merge_request.for_fork?
%span.light (#{merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} &rarr; #{merge_request.target_project.name_with_namespace}:#{merge_request.target_branch})
- else
%span.light (#{merge_request.source_branch} &rarr; #{merge_request.target_branch})
- @issues.each do |issue|
%li
issue:
......
......@@ -2,7 +2,7 @@ class AllowMergesForForks < ActiveRecord::Migration
def self.up
add_column :merge_requests, :target_project_id, :integer, :null => false
MergeRequest.connection.execute("update merge_requests set target_project_id=project_id")
MergeRequest.update_all("target_project_id = project_id")
rename_column :merge_requests, :project_id, :source_project_id
end
......
......@@ -20,7 +20,6 @@ Feature: Project Forked Merge Requests
And I submit the merge request
Then I should see merge request "Merge Request On Forked Project"
@javascript
Scenario: I should see a push widget for forked merge requests
Given project "Forked Shop" has push event
......@@ -39,7 +38,6 @@ Feature: Project Forked Merge Requests
And I click link edit "Merge Request On Forked Project"
Then I see the edit page prefilled for "Merge Request On Forked Project"
@javascript
Scenario: I cannot submit an invalid merge request
Given I visit project "Forked Shop" merge requests page
......
......@@ -109,25 +109,25 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps
@forked_project = Project.find_by_name("Forked Shop")
data = {
before: "0000000000000000000000000000000000000000",
after: "0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e",
ref: "refs/heads/new_design",
user_id: @user.id,
user_name: @user.name,
repository: {
name: @forked_project.name,
url: "localhost/rubinius",
description: "",
homepage: "localhost/rubinius",
private: true
}
before: "0000000000000000000000000000000000000000",
after: "0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e",
ref: "refs/heads/new_design",
user_id: @user.id,
user_name: @user.name,
repository: {
name: @forked_project.name,
url: "localhost/rubinius",
description: "",
homepage: "localhost/rubinius",
private: true
}
}
@event = Event.create(
project: @forked_project,
action: Event::PUSHED,
data: data,
author_id: @user.id
project: @forked_project,
action: Event::PUSHED,
data: data,
author_id: @user.id
)
end
......
module Gitlab
module Satellite
class Action
DEFAULT_OPTIONS = {git_timeout: 30.seconds}
DEFAULT_OPTIONS = { git_timeout: 30.seconds }
attr_accessor :options, :project, :user
......@@ -39,8 +39,8 @@ module Gitlab
def prepare_satellite!(repo)
project.satellite.clear_and_update!
repo.config['user.name']=user.name
repo.config['user.email']=user.email
repo.config['user.name'] = user.name
repo.config['user.email'] = user.email
end
def default_options(options = {})
......
module Gitlab
class SatelliteNotExistError < StandardError;
end
class SatelliteNotExistError < StandardError; end
module Satellite
class Satellite
......@@ -22,9 +21,9 @@ module Gitlab
raise SatelliteNotExistError.new("Satellite doesn't exist")
end
def clear_and_update!
raise_no_satellite unless exists?
File.exists? path
@repo = nil
clear_working_dir!
......@@ -68,7 +67,6 @@ module Gitlab
end
end
def lock_file
create_locks_dir unless File.exists?(lock_files_dir)
File.join(lock_files_dir, "satellite_#{project.id}.lock")
......
......@@ -117,9 +117,9 @@ FactoryGirl.define do
source_branch "stable" # pretend bcf03b5d
st_commits do
[
source_project.repository.commit('bcf03b5d').to_hash,
source_project.repository.commit('bcf03b5d~1').to_hash,
source_project.repository.commit('bcf03b5d~2').to_hash
source_project.repository.commit('bcf03b5d').to_hash,
source_project.repository.commit('bcf03b5d~1').to_hash,
source_project.repository.commit('bcf03b5d~2').to_hash
]
end
st_diffs do
......
require 'spec_helper'
INVALID_FACTORIES = [
:key_with_a_space_in_the_middle,
:invalid_key,
:key_with_a_space_in_the_middle,
:invalid_key,
]
FactoryGirl.factories.map(&:name).each do |factory_name|
next if INVALID_FACTORIES.include?(factory_name)
describe "#{factory_name} factory" do
it 'should be valid' do
build(factory_name).should be_valid
......
......@@ -21,7 +21,6 @@ describe "Application access" do
let(:reporter) { create(:user) }
before do
# full access
project.team << [master, :master]
......
require 'spec_helper'
describe 'Gitlab::Satellite::Action' do
let(:project) { create(:project_with_code) }
let(:user) { create(:user) }
describe '#prepare_satellite!' do
it 'create a repository with a parking branch and one remote: origin' do
......@@ -50,11 +47,8 @@ describe 'Gitlab::Satellite::Action' do
repo.config['user.name'].should ==user.name
repo.config['user.email'].should ==user.email
end
end
describe '#in_locked_and_timed_satellite' do
it 'should make use of a lockfile' do
......
......@@ -11,7 +11,6 @@ describe 'Gitlab::Satellite::MergeAction' do
#these commits are quite close together, itended to make string diffs/format patches small
@close_commit1 = ['2_3_notes_fix', '8470d70da67355c9c009e4401746b1d5410af2e3']
@close_commit2 = ['scss_refactoring', 'f0f14c8eaba69ebddd766498a9d0b0e79becd633']
end
let(:project) { create(:project_with_code) }
......@@ -53,7 +52,6 @@ describe 'Gitlab::Satellite::MergeAction' do
end
end
describe '#format_patch' do
context 'on fork' do
it 'should build a format patch' do
......@@ -76,7 +74,6 @@ describe 'Gitlab::Satellite::MergeAction' do
end
end
describe '#diffs_between_satellite tested against diff_in_satellite' do
def is_a_matching_diff(diff, diffs)
......@@ -109,7 +106,6 @@ describe 'Gitlab::Satellite::MergeAction' do
merge_request.source_branch = @master[0]
diffs = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).diffs_between_satellite
merge_request.target_branch = @close_commit1[0]
merge_request.source_branch = @master[0]
diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).diff_in_satellite
......@@ -119,7 +115,6 @@ describe 'Gitlab::Satellite::MergeAction' do
end
end
describe '#can_be_merged?' do
context 'on fork' do
it 'return true or false depending on if something is mergable' do
......@@ -145,5 +140,4 @@ describe 'Gitlab::Satellite::MergeAction' do
end
end
end
end
\ No newline at end of file
......@@ -90,7 +90,6 @@ describe MergeRequestObserver do
end
end
describe "Merge Request created" do
def self.it_should_be_valid_event
it { @event.should_not be_nil }
......
......@@ -15,8 +15,7 @@ describe API::API do
let!(:users_project) { create(:users_project, user: user, project: project, project_access: UsersProject::MASTER) }
let!(:users_project2) { create(:users_project, user: user3, project: project, project_access: UsersProject::DEVELOPER) }
before {
project.team << [user, :reporter] }
before { project.team << [user, :reporter] }
describe "GET /projects" do
context "when unauthenticated" do
......@@ -48,16 +47,16 @@ describe API::API do
it "should not create new project" do
expect {
post api("/projects", user2), name: 'foo'
}.to change { Project.count }.by(0)
}.to change {Project.count}.by(0)
end
end
it "should create new project without path" do
expect { post api("/projects", user), name: 'foo' }.to change { Project.count }.by(1)
expect { post api("/projects", user), name: 'foo' }.to change {Project.count}.by(1)
end
it "should not create new project without name" do
expect { post api("/projects", user) }.to_not change { Project.count }
expect { post api("/projects", user) }.to_not change {Project.count}
end
it "should return a 400 error if name not given" do
......@@ -91,17 +90,17 @@ describe API::API do
it "should assign attributes to project" do
project = attributes_for(:project, {
description: Faker::Lorem.sentence,
default_branch: 'stable',
issues_enabled: false,
wall_enabled: false,
merge_requests_enabled: false,
wiki_enabled: false
description: Faker::Lorem.sentence,
default_branch: 'stable',
issues_enabled: false,
wall_enabled: false,
merge_requests_enabled: false,
wiki_enabled: false
})
post api("/projects", user), project
project.each_pair do |k, v|
project.each_pair do |k,v|
next if k == :path
json_response[k.to_s].should == v
end
......@@ -127,11 +126,11 @@ describe API::API do
before { admin }
it "should create new project without path" do
expect { post api("/projects/user/#{user.id}", admin), name: 'foo' }.to change { Project.count }.by(1)
expect { post api("/projects/user/#{user.id}", admin), name: 'foo' }.to change {Project.count}.by(1)
end
it "should not create new project without name" do
expect { post api("/projects/user/#{user.id}", admin) }.to_not change { Project.count }
expect { post api("/projects/user/#{user.id}", admin) }.to_not change {Project.count}
end
it "should respond with 201 on success" do
......@@ -146,17 +145,17 @@ describe API::API do
it "should assign attributes to project" do
project = attributes_for(:project, {
description: Faker::Lorem.sentence,
default_branch: 'stable',
issues_enabled: false,
wall_enabled: false,
merge_requests_enabled: false,
wiki_enabled: false
description: Faker::Lorem.sentence,
default_branch: 'stable',
issues_enabled: false,
wall_enabled: false,
merge_requests_enabled: false,
wiki_enabled: false
})
post api("/projects/user/#{user.id}", admin), project
project.each_pair do |k, v|
project.each_pair do |k,v|
next if k == :path
json_response[k.to_s].should == v
end
......@@ -269,7 +268,7 @@ describe API::API do
it "should add user to project team" do
expect {
post api("/projects/#{project.id}/members", user), user_id: user2.id,
access_level: UsersProject::DEVELOPER
access_level: UsersProject::DEVELOPER
}.to change { UsersProject.count }.by(1)
response.status.should == 201
......@@ -279,10 +278,10 @@ describe API::API do
it "should return a 201 status if user is already project member" do
post api("/projects/#{project.id}/members", user), user_id: user2.id,
access_level: UsersProject::DEVELOPER
access_level: UsersProject::DEVELOPER
expect {
post api("/projects/#{project.id}/members", user), user_id: user2.id,
access_level: UsersProject::DEVELOPER
access_level: UsersProject::DEVELOPER
}.not_to change { UsersProject.count }.by(1)
response.status.should == 201
......@@ -413,8 +412,8 @@ describe API::API do
it "should add hook to project" do
expect {
post api("/projects/#{project.id}/hooks", user),
url: "http://example.com"
}.to change { project.hooks.count }.by(1)
url: "http://example.com"
}.to change {project.hooks.count}.by(1)
response.status.should == 201
end
......@@ -432,7 +431,7 @@ describe API::API do
describe "PUT /projects/:id/hooks/:hook_id" do
it "should update an existing project hook" do
put api("/projects/#{project.id}/hooks/#{hook.id}", user),
url: 'http://example.org'
url: 'http://example.org'
response.status.should == 200
json_response['url'].should == 'http://example.org'
end
......@@ -457,7 +456,7 @@ describe API::API do
it "should delete hook from project" do
expect {
delete api("/projects/#{project.id}/hooks/#{hook.id}", user)
}.to change { project.hooks.count }.by(-1)
}.to change {project.hooks.count}.by(-1)