[refs #90] fixed errors, add validations & tests

app/controllers/projects/pull_requests_controller.rb

@@ -16,13 +16,21 @@ class Projects::PullRequestsController < Projects::BaseController
 
     @pull.base_ref = @pull.base_project.default_branch
     @pull.head_ref = params[:treeish].presence || @pull.head_project.default_branch
-    @pull.check
-
-    repo = Git::Repository.new(@pull.path)
-    @base_commit = repo.commits(@pull.base_ref).first
-    @head_commit = repo.commits(@pull.head_branch).first
-
-    @diff = Grit::Repo.new(@pull.path).diff @base_commit, @head_commit
+    if @pull.save
+      @pull.check
+      if @pull.state == 'already'
+        record.errors.add(:head_ref, I18n.t('projects.pull_requests.up_to_date', :base_ref => record.base_ref, :head_ref => record.head_ref))
+        flash[:error] = t('flash.pull_request.create_error')
+        flash[:warning] = @pull.errors.full_messages.join('. ')
+      end
+      repo = Git::Repository.new(@pull.path)
+      @base_commit = repo.commits(@pull.base_ref).first
+      @head_commit = repo.commits(@pull.head_branch).first
+      @diff = Grit::Repo.new(@pull.path).diff @base_commit, @head_commit
+    else
+      flash[:error] = t('flash.pull_request.create_error')
+      flash[:warning] = @pull.errors.full_messages.join('. ')
+    end
   end
 
   def create
@@ -33,8 +41,22 @@ class Projects::PullRequestsController < Projects::BaseController
     #@pull.head_ref = params[:head_ref] # FIXME need validation!
 
     if @pull.save
-      render :index #FIXME redirect to show
+      @pull.check
+      puts "!!!!!!!!!!!!!!!!!!!!!!!!"
+      puts "pull state is #{@pull.state}"
+      if @pull.state == 'already'
+        @pull.destroy
+
+        @pull.errors.add(:head_ref, I18n.t('projects.pull_requests.up_to_date', :base_ref => @pull.base_ref, :head_ref => @pull.head_ref))
+        flash[:notice] = t('flash.pull_request.saved')
+        flash[:warning] = @pull.errors.full_messages.join('. ')
+        render :new
+      else
+        redirect_to project_pull_request_path(@project, @pull)
+      end
     else
+      flash[:error] = t('flash.pull_request.save_error')
+      flash[:warning] = @pull.errors.full_messages.join('. ')
       render :new
     end
   end

app/models/pull_request.rb

@@ -8,9 +8,16 @@ class PullRequest < ActiveRecord::Base
   delegate :user, :title, :body, :serial_id, :assignee, :state, :to => :issue, :allow_nil => true
   accepts_nested_attributes_for :issue
   #attr_accessible #FIXME disable for development
-  validate :uniq_merge#, :not_up_to_date
+  validate :uniq_merge
+  validates_each :head_ref, :base_ref do |record, attr, value|
+    project = attr == :head_ref ? record.head_project : record.base_project
+    if !((project.branches + project.tags).map(&:name).include?(value) || project.git_repository.commits.map(&:id).include?(value))
+      record.errors.add attr, I18n.t('projects.pull_requests.wrong_ref')
+    end
+  end
 
   before_create :clean_dir
+  after_destroy :clean_dir
 
   scope :needed_checking, includes(:issue).where(:issues => {:state => ['open', 'blocked', 'ready', 'already']})
 
@@ -20,11 +27,11 @@ class PullRequest < ActiveRecord::Base
     #end
 
     event :ready do
-      transition [:open, :blocked] => :ready
+      transition [:ready, :open, :blocked] => :ready
     end
 
     event :block do
-      transition [:open, :ready] => :blocked
+      transition [:blocked, :open, :ready] => :blocked
     end
 
     event :already do
@@ -151,14 +158,10 @@ class PullRequest < ActiveRecord::Base
 
   def uniq_merge
     if base_project.pull_requests.needed_checking.where(:head_project_id => head_project, :base_ref => base_ref, :head_ref => head_ref).where('pull_requests.id <> :id or :id is null', :id => id).count > 0
-      errors.add(:head_ref, I18n.t('projects.pull_requests.duplicate', :head_ref => head_ref))
+      errors.add(:base_branch, I18n.t('projects.pull_requests.duplicate', :head_ref => head_ref))
     end
   end
 
-  def not_up_to_date
-    errors.add(:head_ref, I18n.t('projects.pull_requests.up_to_date', :base_ref => base_ref, :head_ref => head_ref)) if state == 'already'
-  end
-
   def clean_dir
     FileUtils.rm_rf path
   end

config/locales/models/pull_request.en.yml

@@ -12,9 +12,16 @@ en:
       merge: Merge
       duplicate: 'There is already a pull request for %{head_ref}'
       up_to_date: 'The %{base_ref} branch is already up-to-date with %{head_ref}'
+      wrong_ref: Wrong tree-ish
 
   pull_requests:
     tabs:
       discussion: Discussion
       diff: Diff
       Commits: Commits
+
+  flash:
+    pull_request:
+      new_error: Unable to create pull request
+      saved: Pull request saved
+      save_error: Unable to save pull request

config/locales/models/pull_request.ru.yml

@@ -12,9 +12,16 @@ ru:
       merge: Слияние
       duplicate: 'Уже существует запрос на слияние %{head_ref}'
       up_to_date: 'Ветка %{base_ref} на данный момент уже содержит последние изменения %{head_ref}'
+      wrong_ref: Неправильный tree-ish
 
   pull_requests:
     tabs:
       discussion: Дискуссия
       diff: Diff
       Commits: Коммиты
+
+  flash:
+    pull_request:
+      new_error: Не удалось создать запрос на слияние
+      saved: Запрос на слияние успешно сохранен
+      save_error: Не удалось сохранить запрос на слияние

spec/controllers/projects/pull_requests_controller_spec.rb

@@ -32,7 +32,7 @@ describe Projects::PullRequestsController do
       response.should redirect_to(new_user_session_path)
     end
 
-    it 'should not be able to perform update title and body' do
+    it 'should not update title and body' do
       post :update, @update_params
       PullRequest.joins(:issue).where(:id => @pull.id, :issues => {:title => 'update', :body => 'updating'}).count.should == 0
     end
@@ -49,13 +49,23 @@ describe Projects::PullRequestsController do
     end
 
     it 'should be able to perform update action' do
-      post :update, @update_params
+      put :update, @update_params
       response.should redirect_to(project_pull_request_path(@project, @project.pull_requests.last))
     end
 
-    it 'should be able to perform update title and body' do
-      post :update, @update_params
+    it 'should update title and body' do
+      put :update, @update_params
       PullRequest.joins(:issue).where(:id => @pull.id, :issues => {:title => 'update', :body => 'updating'}).count.should == 1
     end
+
+    it "should not create same pull" do
+      post :create, @create_params.merge({:pull_request => {:issue_attributes => {:title => 'same', :body => 'creating'}, :head_ref => 'non_conflicts', :base_ref => 'master'}})
+      PullRequest.joins(:issue).where(:issues => {:title => 'same', :body => 'creating'}).count.should == 0
+    end
+
+    it "should not create already up-to-date pull" do
+      post :create, @create_params.merge({:pull_request => {:issue_attributes => {:title => 'already', :body => 'creating'}, :base_ref => 'master', :head_ref => 'master'}})
+      PullRequest.joins(:issue).where(:issues => {:title => 'already', :body => 'creating'}).count.should == 0
+    end
   end
 end

spec/models/pull_request_spec.rb

@@ -39,43 +39,43 @@ describe PullRequest do
       @other_pull.save
     end
 
-    it 'master should can be merged with non_conflicts branch' do
+    it 'master should merge with non_conflicts branch' do
       @pull.check
       @pull.state.should == 'ready'
     end
 
-    it 'master should not be merged with conflicts branch' do
+    it 'master should not merge 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
+    it 'should not merge when already up-to-date branches' do
       @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
+      it 'master should merge with non_conflicts branch' do
         @other_pull.check
         @other_pull.state.should == 'ready'
       end
 
-      it 'master should not be merged with conflicts branch' do
+      it 'master should not merge with conflicts branch' do
         @other_pull.head_ref = 'conflicts'
         @other_pull.check
         @other_pull.state.should == 'blocked'
       end
 
-      it 'should not be merged when already up-to-date branches' do
+      it 'should not merge when already up-to-date branches' do
         @other_pull.head_ref = 'master'
         @other_pull.check
         @other_pull.state.should == 'already'
       end
     end
 
-    it "should not be created same pull" do
+    it "should not create same pull" do
       @same_pull = @project.pull_requests.new(:issue_attributes => {:title => 'same', :body => 'testing'})
       @same_pull.issue.user, @same_pull.issue.project = @user, @same_pull.base_project
       @same_pull.base_ref = 'master'
@@ -84,12 +84,22 @@ describe PullRequest do
       @project.pull_requests.joins(:issue).where(:issues => {:title => @same_pull.title}).count.should == 0
     end
 
-    it "should not be created already up-to-date pull" do
-      @wrong_pull = @project.pull_requests.new(:issue_attributes => {:title => 'wrong', :body => 'testing'})
+    it "should not create pull with wrong base ref" do
+      @wrong_pull = @project.pull_requests.new(:issue_attributes => {:title => 'wrong base', :body => 'testing'})
+      @wrong_pull.issue.user, @wrong_pull.issue.project = @user, @wrong_pull.base_project
+      @wrong_pull.base_ref = 'wrong'
+      @wrong_pull.head_project, @wrong_pull.head_ref = @project, 'non_conflicts'
+      @wrong_pull.save
+      @project.pull_requests.joins(:issue).where(:issues => {:title => @wrong_pull.title}).count.should == 0
+    end
+
+    it "should not create pull with wrong head ref" do
+      @wrong_pull = @project.pull_requests.new(:issue_attributes => {:title => 'wrong head', :body => 'testing'})
       @wrong_pull.issue.user, @wrong_pull.issue.project = @user, @wrong_pull.base_project
       @wrong_pull.base_ref = 'master'
-      @wrong_pull.head_project, @wrong_pull.head_ref = @project, 'master'
-      @wrong_pull.save.should be_false
+      @wrong_pull.head_project, @wrong_pull.head_ref = @project, 'wrong'
+      @wrong_pull.save
+      @project.pull_requests.joins(:issue).where(:issues => {:title => @wrong_pull.title}).count.should == 0
     end
   end