From 58776a6e9e9f16261997f0616cc2d9027df6fa37 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Wed, 18 Apr 2012 23:08:53 +0600 Subject: [PATCH] [refs #90] fixing pull methods --- Gemfile | 1 + Gemfile.lock | 1 + app/models/activity_feed_observer.rb | 1 + app/models/issue.rb | 4 +- app/models/pull_request.rb | 61 ++++++++++++++----- .../20120412173938_create_pull_requests.rb | 1 + spec/factories/pull_request.rb | 10 +++ spec/models/pull_request_spec.rb | 45 +++++++++++++- 8 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 spec/factories/pull_request.rb diff --git a/Gemfile b/Gemfile index a11e9337c..11334fd09 100644 --- a/Gemfile +++ b/Gemfile @@ -47,6 +47,7 @@ gem "haml-rails", '~> 0.3.4' gem 'ruby-haml-js' gem 'rails-backbone' gem 'jquery-rails', '~> 2.0.1' +gem 'state_machine', :require => 'state_machine/core' group :assets do gem 'sass-rails', '~> 3.2.5' diff --git a/Gemfile.lock b/Gemfile.lock index 786cb8c3c..acf73b5eb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -372,6 +372,7 @@ DEPENDENCIES sass-rails (~> 3.2.5) shotgun shoulda + state_machine therubyracer (~> 0.10.0) therubyrhino (~> 1.73.1) trinidad (~> 1.0.2) diff --git a/app/models/activity_feed_observer.rb b/app/models/activity_feed_observer.rb index 023c1e9f6..8d27619bf 100644 --- a/app/models/activity_feed_observer.rb +++ b/app/models/activity_feed_observer.rb @@ -90,6 +90,7 @@ class ActivityFeedObserver < ActiveRecord::Observer :change_type => change_type, :user_email => record.project.git_repository.repo.log(branch_name, nil).first.author.email, :project_owner => record.project.owner.uname} options.merge!({:user_id => first_commiter.id, :user_name => first_commiter.name}) if first_commiter + record.project.pull_requests.needed_checking.each {|pull| pull.check} end record.project.owner_and_admin_ids.each do |recipient| diff --git a/app/models/issue.rb b/app/models/issue.rb index 11f6d9849..a37de6b75 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -21,8 +21,8 @@ class Issue < ActiveRecord::Base attr_accessible :labelings_attributes, :title, :body, :assignee_id accepts_nested_attributes_for :labelings, :allow_destroy => true - scope :opened, where(:status => 'open', :closed_by => nil, :closed_at => nil) - scope :closed, where(:status => 'closed').where("closed_by is not null and closed_at is not null") + scope :opened, where(:status => 'open') + scope :closed, where(:status => 'closed') def assign_uname assignee.uname if assignee diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index 29f721796..24599e243 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -1,48 +1,78 @@ class PullRequest < Issue + extend StateMachine::MacroMethods # no method state_machine WTF?! serialize :data + #TODO add validates to serialized data + scope :needed_checking, where(:state => ['open', 'blocked', 'ready']) state_machine :initial => :open do + #after_transition [:ready, :blocked] => [:merged, :closed] do |pull, transition| + # FileUtils.rm_rf(pull.path) # What about diff? + #end - event :open do - transition :closed => :open - transition :blocked => :open, :if => lambda {|pull| pull.can_merge?} + event :ready do + transition [:open, :blocked] => :ready end event :block do - transition :open => :blocked + transition [:open, :ready] => :block end - event :merge do - transition :open => :merged, :if => lambda {|pull| pull.can_merge?} - transition :open => :blocked, :if => lambda {|pull| !pull.can_merge?} + event :merging do + transition :ready => :merged end event :close do - transition [:open, :blocked] => :closed + transition [:open, :ready, :blocked] => :closed + end + + event :reopen do + transition :closed => :open end end def can_merge? - !merge + state == 'ready' + end + + def check + if ret = merge + system("cd #{path} && git reset --hard HEAD^") # remove merge commit + ready + else + system("cd #{path} && git reset --hard HEAD") + block + end + end + + def merge!(who) + return false unless can_merge? + Dir.chdir(path) do + system "git config user.name \"#{who.uname}\" && git config user.email \"#{who.email}\"" + if merge + merging + system("git push origin HEAD") + end + end end protected def path + filename = [id, project.owner.uname, project.name].join('-') if Rails.env == "production" - File.join('/srv/rosa_build/shared/tmp', "pull_requests", [id, project.owner.uname, project.name].join('-')) + File.join('/srv/rosa_build/shared/tmp', "pull_requests", filename) else - File.join(Rails.root, "tmp", Rails.env, "pull_requests", [id, project.owner.uname, project.name].join('-')) + File.join(Rails.root, "tmp", Rails.env, "pull_requests", filename) end end def merge clone - system "cd #{path} && git checkout #{data.base_branch} && git merge #{data.head_branch} && git reset --hard #{data.head_branch}" + system("cd #{path} && git checkout #{data[:base_branch]} && git merge --no-ff #{data[:head_branch]}") end def clone - git = Git.new(path) + git = Grit::Git.new(path) unless git.exist? FileUtils.mkdir_p(path) @@ -51,7 +81,7 @@ class PullRequest < Issue end else Dir.chdir(path) do - [data.base_branch, data.head_branch].each do |branch| + [data[:base_branch], data[:head_branch]].each do |branch| system "git checkout #{branch} && git pull origin #{branch}" end end @@ -59,4 +89,7 @@ class PullRequest < Issue # TODO catch errors end + def set_serial_id + self.update_attribute :serial_id, self.project.pull_requests.count + end end diff --git a/db/migrate/20120412173938_create_pull_requests.rb b/db/migrate/20120412173938_create_pull_requests.rb index 52c3af17d..8b8b15fdc 100644 --- a/db/migrate/20120412173938_create_pull_requests.rb +++ b/db/migrate/20120412173938_create_pull_requests.rb @@ -1,6 +1,7 @@ class CreatePullRequests < ActiveRecord::Migration def change add_column :issues, :type, :string + add_column :issues, :state, :string add_column :issues, :data, :text, :null => false, :default => 0 end end diff --git a/spec/factories/pull_request.rb b/spec/factories/pull_request.rb new file mode 100644 index 000000000..f016b8a6a --- /dev/null +++ b/spec/factories/pull_request.rb @@ -0,0 +1,10 @@ +# -*- encoding : utf-8 -*- +FactoryGirl.define do + factory :pull_request do + title { FactoryGirl.generate(:string) } + body { FactoryGirl.generate(:string) } + association :project, :factory => :project + association :user, :factory => :user + association :assignee, :factory => :user + end +end diff --git a/spec/models/pull_request_spec.rb b/spec/models/pull_request_spec.rb index 7b11ec0a5..74d7791b1 100644 --- a/spec/models/pull_request_spec.rb +++ b/spec/models/pull_request_spec.rb @@ -1,5 +1,48 @@ +# -*- encoding : utf-8 -*- require 'spec_helper' describe PullRequest do - pending "add some examples to (or delete) #{__FILE__}" + + #~ context 'when create with same owner that platform' do + #~ before (:each) do + #~ stub_rsync_methods + #~ @platform = FactoryGirl.create(:platform) + #~ @params = {:name => 'tst_platform', :description => 'test platform'} + #~ end + + #~ it 'it should increase Repository.count by 1' do + #~ rep = Repository.create(@params) {|r| r.platform = @platform} + #~ @platform.repositories.count.should eql(1) + #~ end + #~ end + + before(:all) do + stub_rsync_methods + Platform.delete_all + 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) } + + after(:all) do + Platform.delete_all + User.delete_all + Repository.delete_all + FileUtils.rm_rf(APP_CONFIG['root_path']) + end + end