Merge pull request #403 from Fryguy/pr_mergeability_checker_to_commit…

…_range

Change PrMergeabilityChecker to be queued by BranchMergeabilityChecker.

app/models/branch.rb

@@ -15,6 +15,7 @@ class Branch < ActiveRecord::Base
   after_initialize(:unless => :commit_uri) { self.commit_uri = self.class.github_commit_uri(repo.try(:name)) }
 
   scope :regular_branches, -> { where(:pull_request => [false, nil]) }
+  scope :pr_branches,      -> { where(:pull_request => true) }
 
   def self.with_branch_or_pr_number(n)
     n = MinigitService.pr_branch(n) if n.kind_of?(Fixnum)
@@ -65,6 +66,10 @@ def pr_number
     MinigitService.pr_number(name) if pull_request?
   end
 
+  def fq_pr_number
+    "#{fq_repo_name}##{pr_number}" if pull_request?
+  end
+
   def pr_title_tags
     pr_title.to_s.match(/^(?:\s*\[\w+\])+/).to_s.gsub("[", " [").split.map { |s| s[1...-1] }
   end

app/models/repo.rb

@@ -62,15 +62,23 @@ def with_travis_service
   end
 
   def branch_names
-    branches.collect(&:name)
+    branches.pluck(:name)
+  end
+
+  def regular_branches
+    branches.regular_branches
+  end
+
+  def regular_branch_names
+    regular_branches.pluck(:name)
   end
 
   def pr_branches
-    branches.select(&:pull_request?)
+    branches.pr_branches
   end
 
   def pr_branch_names
-    pr_branches.collect(&:name)
+    pr_branches.pluck(:name)
   end
 
   # @param expected [Array<Hash>] The desired state of the PR branches.

app/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker.rb

@@ -0,0 +1,19 @@
+class CommitMonitorHandlers::CommitRange::BranchMergeabilityChecker
+  include Sidekiq::Worker
+  sidekiq_options :queue => :miq_bot
+
+  include BranchWorkerMixin
+
+  def self.handled_branch_modes
+    [:regular]
+  end
+
+  def perform(branch_id, _new_commits)
+    return unless find_branch(branch_id, :regular)
+
+    repo.pr_branches.where(:merge_target => branch.name).each do |pr|
+      logger.info("Queueing PrMergeabilityChecker for PR #{pr.fq_pr_number}.")
+      PrMergeabilityChecker.perform_async(pr.id)
+    end
+  end
+end

app/workers/commit_monitor_handlers/branch/pr_mergeability_checker.rb → app/workers/pr_mergeability_checker.rb

@@ -1,17 +1,14 @@
-class CommitMonitorHandlers::Branch::PrMergeabilityChecker
+class PrMergeabilityChecker
   include Sidekiq::Worker
   sidekiq_options :queue => :miq_bot
 
   include BranchWorkerMixin
 
   LABEL = "unmergeable".freeze
 
-  def self.handled_branch_modes
-    [:pr]
-  end
-
   def perform(branch_id)
     return unless find_branch(branch_id, :pr)
+    logger.info("Determining mergeability of PR #{branch.fq_pr_number}.")
 
     process_mergeability
   end
@@ -38,7 +35,7 @@ def process_mergeability
   end
 
   def write_to_github
-    logger.info("Updating PR #{branch.pr_number} with mergability comment.")
+    logger.info("Updating PR #{branch.fq_pr_number} with mergability comment.")
 
     GithubService.add_comment(
       fq_repo_name,
@@ -48,13 +45,13 @@ def write_to_github
   end
 
   def apply_label
-    logger.info("Updating PR #{branch.pr_number} with label #{LABEL.inspect}.")
+    logger.info("Updating PR #{branch.fq_pr_number} with label #{LABEL.inspect}.")
 
     GithubService.add_labels_to_an_issue(fq_repo_name, branch.pr_number, [LABEL])
   end
 
   def remove_label
-    logger.info("Updating PR #{branch.pr_number} my removing label #{LABEL.inspect}.")
+    logger.info("Updating PR #{branch.fq_pr_number} removing label #{LABEL.inspect}.")
     GithubService.remove_label(fq_repo_name, branch.pr_number, LABEL)
   rescue Octokit::NotFound
     # This label is not currently applied, skip

spec/factories/branch.rb

@@ -5,14 +5,14 @@
     sequence(:name) { |n| "branch_#{n}" }
     commit_uri      { "https://example.com/#{repo.name}/commit/$commit" }
     last_commit     { SecureRandom.hex(40) }
-    merge_target    "master"
 
     repo
   end
 
   factory :pr_branch, :parent => :branch do
     sequence(:name)     { |n| "prs/#{n}/head" }
     sequence(:pr_title) { |n| "PR title #{n}" }
+    merge_target        "master"
 
     pull_request true
   end

spec/models/branch_spec.rb

@@ -110,6 +110,19 @@
     end
   end
 
+  context "#fq_pr_number" do
+    it "on pr branch" do
+      branch.name = "pr/133"
+      branch.pull_request = true
+
+      expect(branch.fq_pr_number).to eq "test-user/test-repo#133"
+    end
+
+    it "on regular branch" do
+      expect(branch.fq_pr_number).to be_nil
+    end
+  end
+
   describe "#pr_title_tags" do
     it "with a nil pr_title" do
       branch.pr_title = nil

spec/models/repo_spec.rb

@@ -91,20 +91,38 @@
     expect(repo.branch_names).to match_array([branch1.name, branch2.name])
   end
 
+  it "#regular_branches" do
+    repo.save!
+    _pr_branch      = create(:branch, :repo => repo, :pull_request => true)
+    regular_branch1 = create(:branch, :repo => repo, :pull_request => false)
+    regular_branch2 = create(:branch, :repo => repo, :pull_request => false)
+
+    expect(repo.regular_branches).to match_array([regular_branch1, regular_branch2])
+  end
+
+  it "#regular_branch_names" do
+    repo.save!
+    _pr_branch      = create(:branch, :repo => repo, :pull_request => true)
+    regular_branch1 = create(:branch, :repo => repo, :pull_request => false)
+    regular_branch2 = create(:branch, :repo => repo, :pull_request => false)
+
+    expect(repo.regular_branch_names).to match_array([regular_branch1.name, regular_branch2.name])
+  end
+
   it "#pr_branches" do
     repo.save!
-    pr_branch1     = create(:branch, :repo => repo, :pull_request => true)
-    pr_branch2     = create(:branch, :repo => repo, :pull_request => true)
-    _non_pr_branch = create(:branch, :repo => repo, :pull_request => false)
+    pr_branch1      = create(:branch, :repo => repo, :pull_request => true)
+    pr_branch2      = create(:branch, :repo => repo, :pull_request => true)
+    _regular_branch = create(:branch, :repo => repo, :pull_request => false)
 
     expect(repo.pr_branches).to match_array([pr_branch1, pr_branch2])
   end
 
   it "#pr_branch_names" do
     repo.save!
-    pr_branch1     = create(:branch, :repo => repo, :pull_request => true)
-    pr_branch2     = create(:branch, :repo => repo, :pull_request => true)
-    _non_pr_branch = create(:branch, :repo => repo, :pull_request => false)
+    pr_branch1      = create(:branch, :repo => repo, :pull_request => true)
+    pr_branch2      = create(:branch, :repo => repo, :pull_request => true)
+    _regular_branch = create(:branch, :repo => repo, :pull_request => false)
 
     expect(repo.pr_branch_names).to match_array([pr_branch1.name, pr_branch2.name])
   end

spec/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker_spec.rb

@@ -0,0 +1,13 @@
+describe CommitMonitorHandlers::CommitRange::BranchMergeabilityChecker do
+  let!(:branch)     { create(:branch) }
+  let!(:pr_branch)  { create(:pr_branch, :repo => branch.repo, :merge_target => branch.name) }
+  let!(:pr_branch2) { create(:pr_branch, :repo => branch.repo) }
+
+  before { stub_sidekiq_logger }
+
+  it "queues up PrMergeabilityChecker for PRs targeting this branch" do
+    expect(PrMergeabilityChecker).to receive(:perform_async).once.with(pr_branch.id)
+
+    described_class.new.perform(branch.id, ["abcde123"])
+  end
+end

spec/workers/commit_monitor_handlers/branch/pr_mergeability_checker_spec.rb → spec/workers/pr_mergeability_checker_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe CommitMonitorHandlers::Branch::PrMergeabilityChecker do
+describe PrMergeabilityChecker do
   before(:each) do
     stub_sidekiq_logger
   end