use github_html_url equivalent to github_id when searching (#9240)

modules/github_integration/app/models/github_pull_request.rb

@@ -57,14 +57,18 @@ class GithubPullRequest < ApplicationRecord
   validate :validate_labels_schema
 
   scope :without_work_package, -> { left_outer_joins(:work_packages).where(work_packages: { id: nil }) }
-  scope :partial, -> {
+
-    where(body: nil,
+  def self.find_by_github_identifiers(id: nil, url: nil, initialize: false)
-          comments_count: nil,
+    raise ArgumentError, "needs an id or an url" if id.nil? && url.blank?
-          review_comments_count: nil,
+
-          additions_count: nil,
+    found = where(github_id: id).or(where(github_html_url: url)).take
-          deletions_count: nil,
+
-          changed_files_count: nil)
+    if found
-  }
+      found
+    elsif initialize
+      new(github_id: id, github_html_url: url)
+    end
+  end
 
   ##
   # When a PR lives long enough and receives many pushes, the same check (say, a CI test run) can be run multiple times.

modules/github_integration/lib/open_project/github_integration/services/upsert_pull_request.rb

@@ -46,12 +46,9 @@ module OpenProject::GithubIntegration::Services
     private
 
     def find_or_initialize(payload)
-      GithubPullRequest.partial.find_by(
+      GithubPullRequest.find_by_github_identifiers(id: payload.fetch('id'),
-        github_html_url: payload.fetch('html_url')
+                                                   url: payload.fetch('html_url'),
-      ) ||
+                                                   initialize: true)
-        GithubPullRequest.find_or_initialize_by(
-          github_id: payload.fetch('id')
-        )
     end
 
     # Receives the input from the github webhook and translates them

modules/github_integration/spec/models/github_pull_request_spec.rb

@@ -80,69 +80,89 @@ describe GithubPullRequest do
     end
   end
 
-  describe '#partial?' do
+  describe '.find_by_github_identifiers' do
-    context 'when the body is set' do
+    let(:github_id) { 5 }
-      subject { described_class.new(body: 'something').partial? }
+    let(:github_url) { 'https://github.com/opf/openproject/pull/123' }
-
+    let(:pull_request) do
-      it { is_expected.to be_falsey }
+      FactoryBot.create(:github_pull_request,
+                        github_id: github_id,
+                        github_html_url: github_url)
     end
 
-    context 'when the comments_count is set' do
+    context 'when the github_id attribute matches' do
-      subject { described_class.new(comments_count: 5).partial? }
+      it 'finds by github_id' do
-
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id))
-      it { is_expected.to be_falsey }
+          .to eql pull_request
       end
-
-    context 'when the review_comments_count is set' do
-      subject { described_class.new(review_comments_count: 5).partial? }
-
-      it { is_expected.to be_falsey }
     end
 
-    context 'when the additions_count is set' do
+    context 'when the github_html_url attribute matches' do
-      subject { described_class.new(additions_count: 5).partial? }
+      it 'finds by github_html_url' do
+        expect(described_class.find_by_github_identifiers(url: pull_request.github_html_url))
+          .to eql pull_request
+      end
+    end
 
-      it { is_expected.to be_falsey }
+    context 'when the provided github_id does not match' do
+      it 'is nil' do
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1))
+          .to be_nil
+      end
     end
 
-    context 'when the deletions_count is set' do
+    context 'when the provided github_html_url does not match' do
-      subject { described_class.new(deletions_count: 5).partial? }
+      it 'is nil' do
+        expect(described_class.find_by_github_identifiers(url: "#{pull_request.github_html_url}zzzz"))
+          .to be_nil
+      end
+    end
 
-      it { is_expected.to be_falsey }
+    context 'when neither match' do
+      it 'is nil' do
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
+                                                          url: "#{pull_request.github_html_url}zzzz"))
+          .to be_nil
+      end
     end
 
-    context 'when the changed_files_count is set' do
+    context 'when the provided github_html_url does not match but the github_id does' do
-      subject { described_class.new(changed_files_count: 5).partial? }
+      it 'is nil' do
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id,
+                                                          url: "#{pull_request.github_html_url}zzzz"))
+          .to eql pull_request
+      end
+    end
 
-      it { is_expected.to be_falsey }
+    context 'when the provided github_html_url does match but the github_id does not' do
+      it 'is nil' do
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
+                                                          url: pull_request.github_html_url))
+          .to eql pull_request
+      end
     end
 
-    context 'when the all of the above are nil set and the state is open' do
+    context 'when neither match but initialize is true' do
-      subject do
+      subject(:finder) do
-        described_class.new(changed_files_count: nil,
+        described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
-                            body: nil,
+                                                   url: "#{pull_request.github_html_url}zzzz",
-                            comments_count: nil,
+                                                   initialize: true)
-                            review_comments_count: nil,
-                            additions_count: nil,
-                            deletions_count: nil,
-                            state: 'open').partial?
       end
 
-      it { is_expected.to be_truthy }
+      it 'returns a pull reqeust' do
+        expect(finder)
+          .to be_a(described_class)
       end
 
-    context 'when the all of the above are nil set and the state is closed' do
+      it 'returns a new record' do
-      subject do
+        expect(finder)
-        described_class.new(changed_files_count: nil,
+          .to be_new_record
-                            body: nil,
-                            comments_count: nil,
-                            review_comments_count: nil,
-                            additions_count: nil,
-                            deletions_count: nil,
-                            state: 'closed').partial?
       end
 
-      it { is_expected.to be_truthy }
+      it 'has the privided attributes initialized' do
+        expect(finder.attributes.compact)
+          .to eql("github_id" => pull_request.github_id + 1,
+                  "github_html_url" => "#{pull_request.github_html_url}zzzz")
+      end
     end
   end