From c312d460439f7a5db3f0a3113035ce9baebfb6b8 Mon Sep 17 00:00:00 2001
From: Kassio Borges <kborges@gitlab.com>
Date: Tue, 22 Jun 2021 18:44:51 +0100
Subject: [PATCH] GithubImporter: Avoid failing when review's submitted_at is
 not provided

When a pull request review is not provided, the importer will fall back
to the last updated time of the merge_request.

Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/334214
Changelog: fixed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64645
---
 .../importer/pull_request_review_importer.rb  | 12 ++++++---
 .../representation/pull_request_review.rb     |  2 +-
 .../pull_request_review_importer_spec.rb      | 27 ++++++++++++++-----
 .../pull_request_review_spec.rb               |  6 +++++
 4 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/lib/gitlab/github_import/importer/pull_request_review_importer.rb b/lib/gitlab/github_import/importer/pull_request_review_importer.rb
index f476ee13392fbb..dd5b7c93ced715 100644
--- a/lib/gitlab/github_import/importer/pull_request_review_importer.rb
+++ b/lib/gitlab/github_import/importer/pull_request_review_importer.rb
@@ -69,8 +69,8 @@ def note_attributes(author_id, note, extra = {})
             author_id: author_id,
             note: note,
             system: false,
-            created_at: review.submitted_at,
-            updated_at: review.submitted_at
+            created_at: submitted_at,
+            updated_at: submitted_at
           }.merge(extra)
         end
 
@@ -80,8 +80,8 @@ def add_approval!(user_id)
           approval_attribues = {
             merge_request_id: merge_request.id,
             user_id: user_id,
-            created_at: review.submitted_at,
-            updated_at: review.submitted_at
+            created_at: submitted_at,
+            updated_at: submitted_at
           }
 
           result = ::Approval.insert(
@@ -105,6 +105,10 @@ def add_approval_system_note!(user_id)
 
           Note.create!(attributes)
         end
+
+        def submitted_at
+          @submitted_at ||= (review.submitted_at || merge_request.updated_at)
+        end
       end
     end
   end
diff --git a/lib/gitlab/github_import/representation/pull_request_review.rb b/lib/gitlab/github_import/representation/pull_request_review.rb
index 3205259a1ededa..08b3160fc4cc3a 100644
--- a/lib/gitlab/github_import/representation/pull_request_review.rb
+++ b/lib/gitlab/github_import/representation/pull_request_review.rb
@@ -29,7 +29,7 @@ def self.from_json_hash(raw_hash)
           hash = Representation.symbolize_hash(raw_hash)
 
           hash[:author] &&= Representation::User.from_json_hash(hash[:author])
-          hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone
+          hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone if hash[:submitted_at].present?
 
           new(hash)
         end
diff --git a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb
index fa8b5e6ccf065b..a6da40f47f1c15 100644
--- a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb
@@ -167,6 +167,19 @@
       end
     end
 
+    context 'when the submitted_at is not provided' do
+      let(:review) { create_review(type: 'APPROVED', note: '', submitted_at: nil) }
+
+      it 'creates a note for the review without the author information' do
+        expect { subject.execute }.to change(Note, :count).by(1)
+
+        last_note = merge_request.notes.last
+
+        expect(last_note.created_at)
+          .to be_within(1.second).of(merge_request.updated_at)
+      end
+    end
+
     context 'when the review has a note text' do
       context 'when the review is "APPROVED"' do
         let(:review) { create_review(type: 'APPROVED') }
@@ -215,13 +228,15 @@
     end
   end
 
-  def create_review(type:, note: 'note', author: { id: 999, login: 'author' })
+  def create_review(type:, **extra)
     Gitlab::GithubImport::Representation::PullRequestReview.from_json_hash(
-      merge_request_id: merge_request.id,
-      review_type: type,
-      note: note,
-      submitted_at: submitted_at.to_s,
-      author: author
+      extra.reverse_merge(
+        author: { id: 999, login: 'author' },
+        merge_request_id: merge_request.id,
+        review_type: type,
+        note: 'note',
+        submitted_at: submitted_at.to_s
+      )
     )
   end
 end
diff --git a/spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb b/spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb
index f976345546846a..cad9b13774ef12 100644
--- a/spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb
+++ b/spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb
@@ -68,5 +68,11 @@
 
       expect(review.author).to be_nil
     end
+
+    it 'does not fail when submitted_at is blank' do
+      review = described_class.from_json_hash(hash.except('submitted_at'))
+
+      expect(review.submitted_at).to be_nil
+    end
   end
 end
-- 
GitLab