Merge pull request #714 from warpc/90-fix_orphan_pulls

[refs #90] fixed orphan pulls

app/controllers/projects/pull_requests_controller.rb

@@ -151,5 +151,7 @@ class Projects::PullRequestsController < Projects::BaseController
     @pull.from_project = @project
     @pull.to_ref = (pull_params[:to_ref].presence if pull_params) || @pull.to_project.default_branch
     @pull.from_ref = params[:treeish].presence || (pull_params[:from_ref].presence if pull_params) || @pull.from_project.default_branch
+    @pull.from_project_owner_uname = @pull.from_project.owner.uname
+    @pull.from_project_name = @pull.from_project.name
   end
 end

app/helpers/pull_request_helper.rb

@@ -27,20 +27,27 @@ module PullRequestHelper
    #{show_ref pull, 'from'}</span> \
    #{t 'into'} <span class='label-bootstrap label-info font14'> \
    #{show_ref pull, 'to'}</span>"
-    str << " #{t 'by'} #{link_to pull.user.uname, user_path(pull.user)}" if pull.persisted?
+    str << " #{t 'by'} #{link_to pull.user.uname, user_path(pull.user)}" if pull.user# pull.persisted?
     str.html_safe
   end
 
   #helper for helpers
   def show_ref pull, which, limit = 30
     project, ref = pull.send("#{which}_project"), pull.send("#{which}_ref")
-    link_to "#{project.owner.uname.truncate limit}/#{project.name.truncate limit}: #{ref.truncate limit}", ref_path(project, ref)
+    fullname = if which == 'to'
+                 "#{project.owner.uname.truncate limit}/#{project.name.truncate limit}"
+               elsif which == 'from'
+                 "#{pull.from_project_owner_uname.truncate limit}/#{pull.from_project_name.truncate limit}"
+               end
+    link_to "#{fullname}: #{ref.truncate limit}", ref_path(project, ref)
   end
 
   def ref_path project, ref
-    return tree_path(project, ref) if project.repo.branches_and_tags.map(&:name).include? ref
-    return commit_path(project, ref) if project.repo.commit ref
-    '#'
+    if project && project.repo.branches_and_tags.map(&:name).include?(ref)
+      tree_path(project, ref)
+    else
+      '#'
+    end
   end
 
   def ref_selector_options(project, current)

app/models/pull_request.rb

@@ -6,12 +6,14 @@ class PullRequest < ActiveRecord::Base
   delegate :user, :user_id, :title, :body, :serial_id, :assignee, :status, :to_param,
     :created_at, :updated_at, :comments, :status=, :to => :issue, :allow_nil => true
 
-  validate :uniq_merge
+  validates :from_project, :to_project, :presence => true
+  validate :uniq_merge, :if => Proc.new { |pull| pull.to_project.present? }
   validates_each :from_ref, :to_ref do |record, attr, value|
     check_ref record, attr, value
   end
 
   before_create :clean_dir
+  before_create :set_add_data
   after_destroy :clean_dir
 
   accepts_nested_attributes_for :issue
@@ -82,12 +84,12 @@ class PullRequest < ActiveRecord::Base
   end
 
   def path
-    filename = [id, from_project.owner.uname, from_project.name].compact.join('-')
+    filename = [id, from_project_owner_uname, from_project_name].compact.join('-')
     File.join(APP_CONFIG['git_path'], 'pull_requests', to_project.owner.uname, to_project.name, filename)
   end
 
   def from_branch
-    if to_project != from_project
+    if to_project_id != from_project_id
       "head_#{from_ref}"
     else
       from_ref
@@ -138,11 +140,19 @@ class PullRequest < ActiveRecord::Base
 
   def self.check_ref(record, attr, value)
     project = attr == :from_ref ? record.from_project : record.to_project
-    record.errors.add attr, I18n.t('projects.pull_requests.wrong_ref') unless project.repo.branches_and_tags.map(&:name).include?(value)
+    return if project.blank?
+    if record.to_project.repo.commits.count > 0
+      record.errors.add attr, I18n.t('projects.pull_requests.wrong_ref') unless project.repo.branches_and_tags.map(&:name).include?(value)
+    else
+      record.errors.add attr, I18n.t('projects.pull_requests.empty_repo')
+    end
   end
 
   def uniq_merge
-    if to_project.pull_requests.needed_checking.where(:from_project_id => from_project, :to_ref => to_ref, :from_ref => from_ref).where('pull_requests.id <> :id or :id is null', :id => id).count > 0
+    if to_project && to_project.pull_requests.needed_checking
+         .where(:from_project_id => from_project_id,
+                :to_ref => to_ref, :from_ref => from_ref)
+         .where('pull_requests.id <> :id or :id is null', :id => id).count > 0
       errors.add(:base_branch, I18n.t('projects.pull_requests.duplicate', :from_ref => from_ref))
     end
   end
@@ -160,7 +170,7 @@ class PullRequest < ActiveRecord::Base
 
   def merge
     clone
-    message = "Merge pull request ##{serial_id} from #{from_project.name_with_owner}:#{from_ref}\r\n #{title}"
+    message = "Merge pull request ##{serial_id} from #{from_project_owner_uname}/#{from_project_name}:#{from_ref}\r\n #{title}"
     %x(cd #{path} && git checkout #{to_ref} && git merge --no-ff #{from_branch} -m '#{message}')
   end
 
@@ -183,7 +193,7 @@ class PullRequest < ActiveRecord::Base
     Dir.chdir(path) do
       system 'git', 'checkout', to_ref
       system 'git', 'pull',  'origin', to_ref
-      if to_project == from_project
+      if to_project_id == from_project_id
         system 'git', 'checkout', from_ref
         system 'git', 'pull', 'origin', from_ref
       else
@@ -219,4 +229,9 @@ class PullRequest < ActiveRecord::Base
       end
     end
   end
+
+  def set_add_data
+    self.from_project_owner_uname = from_project.owner.uname
+    self.from_project_name = from_project.name
+  end
 end

app/views/projects/pull_requests/show.html.haml

@@ -1,5 +1,5 @@
 -ar = 'activerecord.attributes.pull_requests'
--set_meta_tags :title => [title_object(@project), t('.title', :name => @pull.title.truncate(40), :user => @pull.user.uname)]
+-set_meta_tags :title => [title_object(@project), t('.title', :name => @pull.title.truncate(40), :user => @pull.user.try(:uname))]
 = render :partial => 'submenu'
 %h3.bpadding10
   =pull_status_label @pull

config/locales/models/pull_request.en.yml

@@ -15,6 +15,7 @@ en:
       duplicate: 'There is already a pull request for %{from_ref}'
       up_to_date: 'The %{to_ref} branch is already up-to-date with %{from_ref}'
       wrong_ref: Wrong branch or tag
+      empty_repo: Empty git repository
       blocked: This pull request cannot be automatically merged.
       ready: This pull request can be automatically merged.
       merged: |

config/locales/models/pull_request.ru.yml

@@ -15,6 +15,8 @@ ru:
       duplicate: 'Уже существует пул реквест %{from_ref}'
       up_to_date: 'Ветка %{to_ref} на данный момент уже содержит последние изменения %{from_ref}'
       wrong_ref: Неправильная ветка или тег
+      empty_repo: Пустой гит-репозиторий
+
       blocked: Невозможно автоматически смержить данный пул реквест.
       ready: Данный пул реквест можно смержить автоматически.
       merged: |

db/migrate/20121027084602_add_helpers_columns_to_pull_request.rb

@@ -0,0 +1,14 @@
+class AddHelpersColumnsToPullRequest < ActiveRecord::Migration
+  def up
+    add_column :pull_requests, :from_project_owner_uname, :string
+    add_column :pull_requests, :from_project_name, :string
+    # includes generate error "undefined method `repo' for nil:NilClass"
+    # update not orphan pulls. For other need execute a task project:fix_orphan_pulls
+    PullRequest.joins(:from_project).each {|pull| pull.from_project_name = pull.from_project.name}
+  end
+
+  def down
+    remove_column :pull_requests, :from_project_owner_uname
+    remove_column :pull_requests, :from_project_name
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20121005100158) do
+ActiveRecord::Schema.define(:version => 20121027084602) do
 
   create_table "activity_feeds", :force => true do |t|
     t.integer  "user_id",    :null => false
@@ -347,11 +347,13 @@ ActiveRecord::Schema.define(:version => 20121005100158) do
   add_index "projects", ["owner_id", "name", "owner_type"], :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 "to_project_id",   :null => false
-    t.integer "from_project_id", :null => false
-    t.string  "to_ref",          :null => false
-    t.string  "from_ref",        :null => false
+    t.integer "issue_id",                 :null => false
+    t.integer "to_project_id",            :null => false
+    t.integer "from_project_id",          :null => false
+    t.string  "to_ref",                   :null => false
+    t.string  "from_ref",                 :null => false
+    t.string  "from_project_owner_uname"
+    t.string  "from_project_name"
   end
 
   add_index "pull_requests", ["from_project_id"], :name => "index_pull_requests_on_head_project_id"

lib/tasks/fix_orphan_pulls.rake

@@ -0,0 +1,21 @@
+namespace :project do
+  desc 'Fix pull requests where was delete the "from project"'
+  task :fix_orphan_pulls => :environment do
+    projects = Project.where('ancestry IS NOT NULL')
+    say "Pull requests total count is #{PullRequest.count}"
+    PullRequest.all.each_with_index do |pull, ind|
+      say "Check pull with id:#{pull.id} (#{ind+1}/#{PullRequest.count})"
+      unless pull.from_project.present?
+        print '  its orphan! updating...'
+        parent_path = File.join(APP_CONFIG['git_path'], 'pull_requests', pull.to_project.owner.uname, pull.to_project.name)
+        Dir.chdir(parent_path) do
+          # Get an owner and project name from the pull dir
+          elements = Dir["#{pull.id}-*"].first.split '-' rescue []
+          pull.from_project_owner_uname, pull.from_project_name = elements[1], elements[2]
+          say pull.save(:validate => false) ? 'success' : 'fail!'
+        end
+      end
+    end
+  end
+end
+