From 4ad39b05c00792364f4ca5bd9beec79c4a57ab38 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Sat, 28 Apr 2012 23:28:57 +0600 Subject: [PATCH] [refs #90] model PullRequest, tests --- app/controllers/pull_requests_controller.rb | 21 +- app/models/project.rb | 2 +- app/models/pull_request.rb | 55 ++++- app/views/pull_requests/new.html.haml | 38 ++-- config/locales/models/pull_request.en.yml | 7 +- config/locales/models/pull_request.ru.yml | 9 +- .../20120412173938_create_pull_requests.rb | 10 +- db/schema.rb | 10 +- spec/models/pull_request_spec.rb | 55 +++-- vendor/assets/stylesheets/bootstrap.css | 196 ------------------ 10 files changed, 151 insertions(+), 252 deletions(-) diff --git a/app/controllers/pull_requests_controller.rb b/app/controllers/pull_requests_controller.rb index 18f0e0e9f..08285586b 100644 --- a/app/controllers/pull_requests_controller.rb +++ b/app/controllers/pull_requests_controller.rb @@ -2,17 +2,30 @@ class PullRequestsController < ApplicationController before_filter :authenticate_user! load_resource :project - load_and_authorize_resource :pull_request, :through => :project, :find_by => :serial_id + #load_and_authorize_resource :pull_request, :through => :project, :find_by => :serial_id #FIXME Disable for development - def index(status = 200) + def index end def new - @base = @project.default_branch - @head = params[:treeish].presence || @project.default_branch + @pull = PullRequest.default_base_project(@project).pull_requests.new + #@pull.build_issue + @pull.issue = @project.issues.new + @pull.head_project = @project + + @base_ref = @pull.base_project.default_branch + @head_ref = params[:treeish].presence || @pull.head_project.default_branch end def create + @pull = @project.pull_requests.new(params[:pull_request]) # FIXME need validation! + @pull.issue.user, @pull.issue.project = current_user, @pull.base_project + + if @pull.save + render :index #FIXME redirect to show + else + render :new + end end def update diff --git a/app/models/project.rb b/app/models/project.rb index fbe0a80ba..5c59c6bcd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -6,7 +6,7 @@ class Project < ActiveRecord::Base belongs_to :owner, :polymorphic => true, :counter_cache => :own_projects_count has_many :issues, :dependent => :destroy - has_many :pull_requests, :dependent => :destroy + has_many :pull_requests, :dependent => :destroy, :foreign_key => 'base_project_id' has_many :build_lists, :dependent => :destroy has_many :project_imports, :dependent => :destroy diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index a8cfb7047..02863f2ad 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -1,9 +1,15 @@ -class PullRequest < Issue +class PullRequest < ActiveRecord::Base extend StateMachine::MacroMethods # no method state_machine WTF?! - serialize :data #TODO add validates to serialized data scope :needed_checking, where(:state => ['open', 'blocked', 'ready']) + belongs_to :issue, :autosave => true, :dependent => :destroy, :touch => true, :validate => true + belongs_to :base_project, :class_name => 'Project', :foreign_key => 'base_project_id' + belongs_to :head_project, :class_name => 'Project', :foreign_key => 'head_project_id' + delegate :user, :title, :body, :serial_id, :assignee, :to => :issue, :allow_nil => true + accepts_nested_attributes_for :issue + #attr_accessible #FIXME disable for development + state_machine :initial => :open do #after_transition [:ready, :blocked] => [:merged, :closed] do |pull, transition| # FileUtils.rm_rf(pull.path) # What about diff? @@ -62,10 +68,28 @@ class PullRequest < Issue end end + def self.default_base_project(project) + project.is_root? ? project : project.root + end + + def clean #FIXME move to protected + Dir.chdir(path) do + base_project.branches.each {|branch| system 'git', 'checkout', branch.name} + system 'git', 'checkout', base_ref + + base_project.branches.each do |branch| + system 'git', 'branch', '-D', branch.name unless [base_ref, head_ref].include? branch.name + end + base_project.tags.each do |tag| + system 'git', 'tag', '-d', tag.name unless [base_ref, head_ref].include? tag.name + end + end + end + protected def path - filename = [id, project.owner.uname, project.name].join('-') + filename = [id, base_project.owner.uname, base_project.name].join('-') if Rails.env == "production" File.join('/srv/rosa_build/shared/tmp', "pull_requests", filename) else @@ -75,7 +99,7 @@ class PullRequest < Issue def merge clone - %x(cd #{path} && git checkout #{data[:base_branch]} && git merge --no-ff #{data[:head_branch]}) #FIXME need sanitize branch name! + %x(cd #{path} && git checkout #{base_ref} && git merge --no-ff #{head_ref}) #FIXME need sanitize branch name! end def clone @@ -83,18 +107,27 @@ class PullRequest < Issue unless git.exist? FileUtils.mkdir_p(path) - system("git clone --local --no-hardlinks #{project.path} #{path}") + system("git clone --local --no-hardlinks #{base_project.path} #{path}") + if base_project != head_project + Dir.chdir(path) do + system 'git', 'remote', 'add', 'head', head_project.path + end + end end + + clean Dir.chdir(path) do - [data[:base_branch], data[:head_branch]].each do |branch| - system 'git', 'checkout', branch - system 'git', 'pull', 'origin', branch + system 'git', 'checkout', base_ref + system 'git', 'pull', 'origin', base_ref + if base_project == head_project + system 'git', 'checkout', head_ref + system 'git', 'pull', 'origin', head_ref + else + system 'git', 'fetch', 'head', "+#{head_ref}:head_#{head_ref}" end end # TODO catch errors end - def set_serial_id - self.update_attribute :serial_id, self.project.pull_requests.count - end + end diff --git a/app/views/pull_requests/new.html.haml b/app/views/pull_requests/new.html.haml index 532c5a7fe..26a45eb91 100644 --- a/app/views/pull_requests/new.html.haml +++ b/app/views/pull_requests/new.html.haml @@ -1,16 +1,28 @@ -set_meta_tags :title => [title_object(@project), t('.title')] = render :partial => 'projects/submenu' +%h3.bpadding10= raw t '.new', {:base => @base, :head => @head} -=form_for @project.pull_requests.new, :url => project_pull_requests_path do |f| - .row-fluid - .span12 - %h3=raw t '.new', {:base => @base, :head => @head} - .row-fluid - .span6 - .span - %h3 #{t '.base'} #{t '.refs'}: - =text_field_tag :base_ref, @base - .span6 - .span - %h3 #{t '.head'} #{t '.refs'}: - =text_field_tag :head_ref, @head +=form_for @pull, :url => project_pull_requests_path, :html => {:class => 'well well-large'} do |f| + .leftlist=f.label :base_project, t('.base_project'), :class => :label + .rightlist=f.text_field :base_project_id + .both + .leftlist=f.label :base_ref, "#{t '.base_ref'} #{t '.refs'}", :class => :label + .rightlist=text_field_tag :base_ref, @base_ref + .both + .leftlist=f.label :head_project, t('.head_project'), :class => :label + .rightlist=f.text_field :head_project_id + .both + .leftlist=f.label :head_ref, "#{t '.head_ref'} #{t '.refs'}", :class => :label + .rightlist=text_field_tag :head_ref, @head_ref + .both + = f.fields_for :issue do |issue| + .leftlist=issue.label :title, t('activerecord.attributes.issue.title'), :class => :label + .rightlist=issue.text_field :title + .both + .leftlist=issue.label :title, t('activerecord.attributes.issue.body'), :class => :label + .rightlist=issue.text_area :body + .both + .leftlist + \  + .rightlist=f.submit t('.submit'), :class => 'btn btn-primary disabled', 'data-loading-text' => t('layout.processing'), :id => 'create_pull' + .both diff --git a/config/locales/models/pull_request.en.yml b/config/locales/models/pull_request.en.yml index 13a98fc00..7b66f559a 100644 --- a/config/locales/models/pull_request.en.yml +++ b/config/locales/models/pull_request.en.yml @@ -2,6 +2,9 @@ en: pull_requests: new: new: 'Create a pull request into %{base} from %{head}' - base: 'Base' - head: 'Head' + base_ref: 'Base' + head_ref: 'Head' refs: 'branch · tag · commit' + base_project: 'Base project' + head_project: 'Head project' + submit: 'Send pull request' diff --git a/config/locales/models/pull_request.ru.yml b/config/locales/models/pull_request.ru.yml index 87a586b21..fe9db50ed 100644 --- a/config/locales/models/pull_request.ru.yml +++ b/config/locales/models/pull_request.ru.yml @@ -2,6 +2,9 @@ ru: pull_requests: new: new: 'Создать запрос на слияние в %{base} из %{head}' - base: 'База' - head: 'Источник' - refs: 'ветка · тег · коммит' + base_ref: 'База' + head_ref: 'Источник' + refs: 'branch · tag · commit' + base_project: 'Базовый проект' + head_project: 'Проект-источник' + submit: 'Создать запрос на слияние' diff --git a/db/migrate/20120412173938_create_pull_requests.rb b/db/migrate/20120412173938_create_pull_requests.rb index 851272f37..d881370e0 100644 --- a/db/migrate/20120412173938_create_pull_requests.rb +++ b/db/migrate/20120412173938_create_pull_requests.rb @@ -1,7 +1,13 @@ class CreatePullRequests < ActiveRecord::Migration def change - add_column :issues, :type, :string rename_column :issues, :status, :state - add_column :issues, :data, :text, :null => false, :default => 0 + + create_table :pull_requests do |t| + t.integer :issue_id, :null => false + t.integer :base_project_id, :null => false + t.integer :head_project_id, :null => false + t.string :base_ref, :null => false + t.string :head_ref, :null => false + end end end diff --git a/db/schema.rb b/db/schema.rb index 28d92d517..dbc65959b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -146,8 +146,6 @@ ActiveRecord::Schema.define(:version => 20120418100619) do t.integer "user_id" t.datetime "closed_at" t.integer "closed_by" - t.string "type" - t.text "data", :default => "0", :null => false end add_index "issues", ["project_id", "serial_id"], :name => "index_issues_on_project_id_and_serial_id", :unique => true @@ -266,6 +264,14 @@ ActiveRecord::Schema.define(:version => 20120418100619) do add_index "projects", ["owner_id"], :name => "index_projects_on_name_and_owner_id_and_owner_type", :unique => true, :case_sensitive => false + create_table "pull_requests", :force => true do |t| + t.integer "issue_id", :null => false + t.integer "base_project_id", :null => false + t.integer "head_project_id", :null => false + t.string "base_ref", :null => false + t.string "head_ref", :null => false + end + create_table "register_requests", :force => true do |t| t.string "name" t.string "email" diff --git a/spec/models/pull_request_spec.rb b/spec/models/pull_request_spec.rb index 2515b4ed2..28f82e709 100644 --- a/spec/models/pull_request_spec.rb +++ b/spec/models/pull_request_spec.rb @@ -10,6 +10,8 @@ def set_data_for_pull @clone_path = File.join(APP_CONFIG['root_path'], 'repo_clone', @project.id.to_s) FileUtils.mkdir_p(@clone_path) + @other_project = FactoryGirl.create(:project, :owner => @user) + %x(cp -Rf #{Rails.root}/spec/tests.git/* #{@other_project.path}) # maybe FIXME ? any_instance_of(Project, :versions => ['v1.0', 'v2.0']) end @@ -20,10 +22,17 @@ describe PullRequest do stub_rsync_methods @user = FactoryGirl.create(:user) set_data_for_pull - @pull = @project.pull_requests.new(:title => 'test', :body => 'testing') - @pull.user = @user - @pull.data = {:base_branch => 'master', :head_branch => 'non_conflicts'} + @pull = @project.pull_requests.new(:issue_attributes => {:title => 'test', :body => 'testing'}) + @pull.issue.user, @pull.issue.project = @user, @pull.base_project + @pull.base_ref = 'master' + @pull.head_project, @pull.head_ref = @project, 'non_conflicts' @pull.save + + @other_pull = @project.pull_requests.new(:issue_attributes => {:title => 'test_other', :body => 'testing_other'}) + @other_pull.issue.user, @other_pull.issue.project = @user, @other_pull.base_project + @other_pull.base_ref = 'master' + @other_pull.head_project, @other_pull.head_ref = @other_project, 'non_conflicts' + @other_pull.save end it 'master should can be merged with non_conflicts branch' do @@ -32,16 +41,35 @@ describe PullRequest do end it 'master should not be merged with conflicts branch' do - @pull.data[:head_branch] = 'conflicts' + @pull.head_ref = 'conflicts' @pull.check @pull.state.should == 'blocked' end it 'should not be merged when already up-to-date branches' do - @pull.data[:head_branch] = 'master' + @pull.head_ref = 'master' @pull.check @pull.state.should == 'already' end + + context 'for other head project' do + it 'master should can be merged with non_conflicts branch' do + @other_pull.check + @other_pull.state.should == 'ready' + end + + it 'master should not be merged with conflicts branch' do + @pull.head_ref = 'conflicts' + @pull.check + @pull.state.should == 'blocked' + end + + it 'should not be merged when already up-to-date branches' do + @pull.head_ref = 'master' + @pull.check + @pull.state.should == 'already' + end + end end before(:all) do @@ -50,27 +78,18 @@ describe PullRequest do User.delete_all Repository.delete_all FileUtils.rm_rf(APP_CONFIG['root_path']) - # Need for validate_uniqueness_of check - FactoryGirl.create(:pull_request) end - it { should belong_to(:project) } - it { should have_many(:comments)} - - it { should validate_presence_of(:title)} - #it { should validate_uniqueness_of(:serial_id).scoped_to(:project_id) } - it { should validate_presence_of(:body) } - - it { should_not allow_mass_assignment_of(:project) } - it { should_not allow_mass_assignment_of(:project_id) } - it { should_not allow_mass_assignment_of(:user) } - it { should_not allow_mass_assignment_of(:user_id) } + it { should belong_to(:issue) } + it { should belong_to(:base_project) } + it { should belong_to(:head_project) } after(:all) do Platform.delete_all User.delete_all Repository.delete_all FileUtils.rm_rf(APP_CONFIG['root_path']) + #~ FileUtils.rm_rf File.join(Rails.root, "tmp", Rails.env, "pull_requests") end end diff --git a/vendor/assets/stylesheets/bootstrap.css b/vendor/assets/stylesheets/bootstrap.css index 0288a28f9..2df98f666 100644 --- a/vendor/assets/stylesheets/bootstrap.css +++ b/vendor/assets/stylesheets/bootstrap.css @@ -722,199 +722,3 @@ border-color: #ddd #ddd #ddd transparent; *border-left-color: #ffffff; } -.row { - margin-left: -20px; - *zoom: 1; -} -.row:before, -.row:after { - display: table; - content: ""; -} -.row:after { - clear: both; -} -[class*="span"] { - float: left; - margin-left: 20px; -} -.container, -.navbar-fixed-top .container, -.navbar-fixed-bottom .container { - width: 940px; -} -.span12 { - width: 940px; -} -.span11 { - width: 860px; -} -.span10 { - width: 780px; -} -.span9 { - width: 700px; -} -.span8 { - width: 620px; -} -.span7 { - width: 540px; -} -.span6 { - width: 460px; -} -.span5 { - width: 380px; -} -.span4 { - width: 300px; -} -.span3 { - width: 220px; -} -.span2 { - width: 140px; -} -.span1 { - width: 60px; -} -.offset12 { - margin-left: 980px; -} -.offset11 { - margin-left: 900px; -} -.offset10 { - margin-left: 820px; -} -.offset9 { - margin-left: 740px; -} -.offset8 { - margin-left: 660px; -} -.offset7 { - margin-left: 580px; -} -.offset6 { - margin-left: 500px; -} -.offset5 { - margin-left: 420px; -} -.offset4 { - margin-left: 340px; -} -.offset3 { - margin-left: 260px; -} -.offset2 { - margin-left: 180px; -} -.offset1 { - margin-left: 100px; -} -.row-fluid { - width: 100%; - *zoom: 1; -} -.row-fluid:before, -.row-fluid:after { - display: table; - content: ""; -} -.row-fluid:after { - clear: both; -} -.row-fluid > [class*="span"] { - float: left; - margin-left: 2.127659574%; -} -.row-fluid > [class*="span"]:first-child { - margin-left: 0; -} -.row-fluid > .span12 { - width: 99.99999998999999%; -} -.row-fluid > .span11 { - width: 91.489361693%; -} -.row-fluid > .span10 { - width: 82.97872339599999%; -} -.row-fluid > .span9 { - width: 74.468085099%; -} -.row-fluid > .span8 { - width: 65.95744680199999%; -} -.row-fluid > .span7 { - width: 57.446808505%; -} -.row-fluid > .span6 { - width: 48.93617020799999%; -} -.row-fluid > .span5 { - width: 40.425531911%; -} -.row-fluid > .span4 { - width: 31.914893614%; -} -.row-fluid > .span3 { - width: 23.404255317%; -} -.row-fluid > .span2 { - width: 14.89361702%; -} -.row-fluid > .span1 { - width: 6.382978723%; -} -.label { - padding: 1px 4px 2px; - font-size: 10.998px; - font-weight: bold; - line-height: 13px; - color: #ffffff; - vertical-align: middle; - white-space: nowrap; - text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.25); - background-color: #999999; - -webkit-border-radius: 3px; - -moz-border-radius: 3px; - border-radius: 3px; -} -.label:hover { - color: #ffffff; - text-decoration: none; -} -.label-important { - background-color: #b94a48; -} -.label-important:hover { - background-color: #953b39; -} -.label-warning { - background-color: #f89406; -} -.label-warning:hover { - background-color: #c67605; -} -.label-success { - background-color: #468847; -} -.label-success:hover { - background-color: #356635; -} -.label-info { - background-color: #3a87ad; -} -.label-info:hover { - background-color: #2d6987; -} -.label-inverse { - background-color: #333333; -} -.label-inverse:hover { - background-color: #1a1a1a; -}