Fall back to the global GitHub id when the URL lookup misses

A URL miss with an id hit is the same pull request after a repository
rename or transfer; the upsert then writes the new URL back. GitLab gets
no such fallback because its ids repeat per project.

Author: akabiru

modules/github_integration/app/models/github_pull_request.rb

@@ -63,7 +63,11 @@ class GithubPullRequest < ApplicationRecord
   def self.find_by_github_identifiers(url:, id: nil, initialize: false)
     raise ArgumentError, "needs an url" if url.blank?
 
-    found = find_by(github_html_url: url)
+    # The URL matches across every payload shape (comment payloads carry no
+    # pull request id). The id fallback covers repository renames: GitHub ids
+    # are globally unique, so a URL miss with an id hit is the same pull
+    # request under a new URL, which the next upsert writes back.
+    found = find_by(github_html_url: url) || (find_by(github_id: id) if id)
     found || (new(github_id: id, github_html_url: url) if initialize)
   end
 

modules/github_integration/spec/lib/open_project/github_integration/services/upsert_pull_request_spec.rb

@@ -126,6 +126,17 @@
     end
   end
 
+  context "when the repository was renamed and only the id still matches" do
+    let!(:github_pull_request) do
+      create(:github_pull_request, github_id:, github_html_url: "https://github.com/test_user/old_repo_name")
+    end
+
+    it "updates the existing record and stores the new url instead of duplicating" do
+      expect { upsert }.not_to change(GithubPullRequest, :count)
+      expect(github_pull_request.reload.github_html_url).to eql github_html_url
+    end
+  end
+
   context "when a partial github pull request with that html_url already exists" do
     let(:github_pull_request) do
       create(:github_pull_request,

modules/github_integration/spec/models/github_pull_request_spec.rb

@@ -104,13 +104,34 @@
     end
 
     context "when the url does not match" do
-      it "returns nothing even when the id matches" do
+      it "falls back to the id so renamed repositories keep their records" 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
+
+      it "returns nothing when the id does not match either" 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 url matches one record and the id another" do
+      let!(:other_pull_request) do
+        create(:github_pull_request,
+               github_id: github_id + 1,
+               github_html_url: "#{github_url}4")
+      end
+
+      before { pull_request }
+
+      it "prefers the url match" do
+        expect(described_class.find_by_github_identifiers(id: other_pull_request.github_id, url: github_url))
+          .to eql pull_request
+      end
+    end
+
     context "when the url does not match but initialize is true" do
       subject(:finder) do
         described_class.find_by_github_identifiers(id: pull_request.github_id + 1,