Revert "Add head commit to pull requests (#56)" (#59)

This reverts commit fbd5154.
Shopify · Aug 26, 2020 · b914d6b · b914d6b
1 parent fbd5154
commit b914d6b
Show file tree

Hide file tree

Showing 23 changed files with 38 additions and 271 deletions.
diff --git a/app/models/shipit/pull_request.rb b/app/models/shipit/pull_request.rb
@@ -6,41 +6,30 @@ class PullRequest < Record
 
     belongs_to :stack
     belongs_to :user
-    belongs_to :head, class_name: 'Shipit::Commit', optional: true
 
     has_many :pull_request_assignments
     has_many :assignees, class_name: :User, through: :pull_request_assignments, source: :user
 
     has_many :pull_request_labels
     has_many :labels, through: :pull_request_labels
 
-    def github_pull_request=(github_pull_request)
-      self.github_id = github_pull_request.id
-      self.number = github_pull_request.number
-      self.api_url = github_pull_request.url
-      self.title = github_pull_request.title
-      self.state = github_pull_request.state
-      self.additions = github_pull_request.additions
-      self.deletions = github_pull_request.deletions
-      self.user = User.find_or_create_by_login!(github_pull_request.user.login)
-      self.assignees = github_pull_request.assignees.map do |github_user|
-        User.find_or_create_by_login!(github_user.login)
-      end
-      self.labels = github_pull_request.labels.map do |github_label|
-        Label.find_or_create_from_github!(github_label)
-      end
-      self.head = find_or_create_commit_from_github_by_sha!(github_pull_request.head.sha)
+    def self.from_github(github_pull_request)
+      new(attributes_from_github(github_pull_request))
     end
 
-    def find_or_create_commit_from_github_by_sha!(sha)
-      if commit = stack.commits.by_sha(sha)
-        commit
-      else
-        github_commit = Shipit.github.api.commit(stack.github_repo_name, sha)
-        stack.commits.create_from_github!(github_commit)
-      end
-    rescue ActiveRecord::RecordNotUnique
-      retry
+    def self.attributes_from_github(github_pull_request)
+      {
+        github_id: github_pull_request.id,
+        number: github_pull_request.number,
+        api_url: github_pull_request.url,
+        title: github_pull_request.title,
+        state: github_pull_request.state,
+        additions: github_pull_request.additions,
+        deletions: github_pull_request.deletions,
+        user: User.find_or_create_by_login!(github_pull_request.user.login),
+        assignees: github_pull_request.assignees.map { |github_user| User.find_or_create_by_login!(github_user.login) },
+        labels: github_pull_request.labels.map { |github_label| Label.find_or_create_from_github!(github_label) },
+      }
     end
   end
 end
diff --git a/app/models/shipit/webhooks/handlers/pull_request/assigned_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/assigned_handler.rb
@@ -16,10 +16,6 @@ class AssignedHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end
@@ -41,7 +37,7 @@ class AssignedHandler < Shipit::Webhooks::Handlers::Handler
           def process
             return unless respond_to_assignee_change?
 
-            pull_request.update(github_pull_request: params.pull_request) if pull_request.present?
+            pull_request.update(pull_request_attributes) if pull_request.present?
           end
 
           private
@@ -67,6 +63,10 @@ def pull_request
           def repository
             Shipit::Repository.from_github_repo_name(params.repository.full_name) || Shipit::NullRepository.new
           end
+
+          def pull_request_attributes
+            Shipit::PullRequest.attributes_from_github(params.pull_request)
+          end
         end
       end
     end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/closed_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/closed_handler.rb
@@ -16,10 +16,6 @@ class ClosedHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/edited_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/edited_handler.rb
@@ -16,10 +16,6 @@ class EditedHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end
@@ -41,7 +37,7 @@ class EditedHandler < Shipit::Webhooks::Handlers::Handler
           def process
             return unless respond_to_pull_request_edited?
 
-            pull_request.update(github_pull_request: params.pull_request) if pull_request.present?
+            pull_request.update_attributes(pull_request_attributes) if pull_request.present?
           end
 
           private
@@ -64,6 +60,10 @@ def repository
             Shipit::Repository.from_github_repo_name(params.repository.full_name) || Shipit::NullRepository.new
           end
 
+          def pull_request_attributes
+            Shipit::PullRequest.attributes_from_github(params.pull_request)
+          end
+
           def respond_to_pull_request_edited?
             params.action == "edited"
           end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/label_capturing_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/label_capturing_handler.rb
@@ -16,10 +16,6 @@ class LabelCapturingHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/labeled_handler.rb
@@ -16,10 +16,6 @@ class LabeledHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/opened_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/opened_handler.rb
@@ -16,10 +16,6 @@ class OpenedHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/reopened_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/reopened_handler.rb
@@ -16,10 +16,6 @@ class ReopenedHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/models/shipit/webhooks/handlers/pull_request/review_stack_adapter.rb b/app/models/shipit/webhooks/handlers/pull_request/review_stack_adapter.rb
@@ -69,20 +69,17 @@ def pr_number
           end
 
           def create!
-            stack = scope.create!(stack_attributes)
-            Shipit::PullRequest.create!(
-              stack: stack,
-              github_pull_request: params.pull_request
-            )
-
+            stack = scope.build(stack_attributes)
+            stack.pull_request = Shipit::PullRequest.from_github(params.pull_request)
+            stack.save!
             Shipit::ReviewStackProvisioningQueue.add(stack)
 
             @stack = stack
           end
 
           def stack_attributes
             {
-              branch: params.pull_request.head.ref,
+              branch: params.pull_request["head"]["ref"],
               environment: environment,
               ignore_ci: false,
               continuous_deployment: false,

diff --git a/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb b/app/models/shipit/webhooks/handlers/pull_request/unlabeled_handler.rb
@@ -16,10 +16,6 @@ class UnlabeledHandler < Shipit::Webhooks::Handlers::Handler
               requires :state, String
               requires :additions, Integer
               requires :deletions, Integer
-              requires :head do
-                requires :sha, String
-                requires :ref, String
-              end
               requires :user do
                 requires :login, String
               end

diff --git a/app/serializers/shipit/pull_request_serializer.rb b/app/serializers/shipit/pull_request_serializer.rb
@@ -6,7 +6,6 @@ class PullRequestSerializer < ActiveModel::Serializer
     include ConditionalAttributes
 
     has_one :user
-    has_one :head, serializer: ShortCommitSerializer
     has_many :assignees, serializer: UserSerializer
 
     attributes :id, :number, :title, :github_id, :additions, :deletions, :state, :html_url

diff --git a/db/migrate/20200823172649_add_head_reference_to_pull_requests.rb b/db/migrate/20200823172649_add_head_reference_to_pull_requests.rb
diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb
@@ -218,8 +218,6 @@
     t.integer "user_id"
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
-    t.integer "head_id"
-    t.index ["head_id"], name: "index_pull_requests_on_head_id"
     t.index ["stack_id", "github_id"], name: "index_pull_requests_on_stack_id_and_github_id", unique: true
     t.index ["stack_id", "number"], name: "index_pull_requests_on_stack_id_and_number", unique: true
     t.index ["stack_id"], name: "index_pull_requests_on_stack_id"

diff --git a/test/fixtures/shipit/commits.yml b/test/fixtures/shipit/commits.yml
@@ -390,7 +390,7 @@ check_deploy_spec_first:
 
 deployable_review_stack_commit:
   id: 801
-  sha: ec26c3e57ca3a959ca5aad62de7213c562f8c821
+  sha: 6d9278037b872fd9a6690523e411ecb3aa181355
   message: "lets go"
   stack: review_stack
   author: walrus

diff --git a/test/fixtures/shipit/pull_requests.yml b/test/fixtures/shipit/pull_requests.yml
@@ -11,7 +11,6 @@ shipit_review:
 review_stack_review:
   stack: review_stack
   number: 100
-  head_id: 801
   title: Review Stack Pull Request
   github_id: 100110001
   api_url: https://api.github.com/repos/shopify/shipit-engine/pulls/100

diff --git a/test/models/shipit/pull_request_test.rb b/test/models/shipit/pull_request_test.rb
@@ -4,7 +4,7 @@
 
 module Shipit
   class PullRequestTest < ActiveSupport::TestCase
-    test "github_pull_request= parses into a a Shipit::PullRequest" do
+    test ".from_github creates a Shipit::PullRequest" do
       github_pull_request = resource(
         {
           url: "https://api.github.com/repos/Codertocat/Hello-World/pulls/2",
@@ -14,9 +14,6 @@ class PullRequestTest < ActiveSupport::TestCase
           additions: 100,
           deletions: 101,
           title: "Update the README with new information.",
-          head: {
-            sha: "ec26c3e57ca3a959ca5aad62de7213c562f8c821",
-          },
           user: {
             login: "Codertocat",
           },
@@ -32,10 +29,8 @@ class PullRequestTest < ActiveSupport::TestCase
           ],
         }
       )
-      stack = shipit_stacks(:review_stack)
-      pull_request = stack.pull_request
 
-      stack.pull_request.github_pull_request = github_pull_request
+      pull_request = PullRequest.from_github(github_pull_request)
 
       assert_equal 279147437, pull_request.github_id
       assert_equal 2, pull_request.number

diff --git a/test/models/shipit/webhooks/handlers/pull_request/closed_handler_test.rb b/test/models/shipit/webhooks/handlers/pull_request/closed_handler_test.rb
@@ -155,36 +155,6 @@ def complete_active_tasks(stack)
             active_tasks.reload
             active_tasks.map(&:complete)
           end
-
-          setup do
-            Shipit.github.api.stubs(:commit)
-              .with("shopify/shipit-engine", "ec26c3e57ca3a959ca5aad62de7213c562f8c821")
-              .returns(
-                resource(
-                  {
-                    sha: "ec26c3e57ca3a959ca5aad62de7213c562f8c821",
-                    commit: {
-                      author: {
-                        name: "Codertocat",
-                        email: "21031067+Codertocat@users.noreply.github.com",
-                        date: "2019-05-15 15:20:30",
-                      },
-                      committer: {
-                        name: "Codertocat",
-                        email: "21031067+Codertocat@users.noreply.github.com",
-                        date: "2019-05-15 15:20:30",
-                      },
-                      message: "Update README.md",
-                    },
-                    stats: {
-                      total: 2,
-                      additions: 1,
-                      deletions: 1,
-                    },
-                  }
-                )
-              )
-          end
         end
       end
     end

diff --git a/test/models/shipit/webhooks/handlers/pull_request/label_capturing_handler_test.rb b/test/models/shipit/webhooks/handlers/pull_request/label_capturing_handler_test.rb
@@ -178,36 +178,6 @@ def assert_does_not_have_label(stack, label_name)
           def environment_for(payload)
             "pr#{payload['number']}"
           end
-
-          setup do
-            Shipit.github.api.stubs(:commit)
-              .with("shopify/shipit-engine", "ec26c3e57ca3a959ca5aad62de7213c562f8c821")
-              .returns(
-                resource(
-                  {
-                    sha: "ec26c3e57ca3a959ca5aad62de7213c562f8c821",
-                    commit: {
-                      author: {
-                        name: "Codertocat",
-                        email: "21031067+Codertocat@users.noreply.github.com",
-                        date: "2019-05-15 15:20:30",
-                      },
-                      committer: {
-                        name: "Codertocat",
-                        email: "21031067+Codertocat@users.noreply.github.com",
-                        date: "2019-05-15 15:20:30",
-                      },
-                      message: "Update README.md",
-                    },
-                    stats: {
-                      total: 2,
-                      additions: 1,
-                      deletions: 1,
-                    },
-                  }
-                )
-              )
-          end
         end
       end
     end