Merge branch...

Merge branch 'kassio/github-importer-avoid-failing-when-review-submitted-at-is-not-provided' into 'master'

GithubImporter: Avoid failing when review's submitted_at is not provided

See merge request gitlab-org/gitlab!64645

lib/gitlab/github_import/importer/pull_request_review_importer.rb

@@ -69,8 +69,8 @@ module Gitlab
             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 @@ module Gitlab
           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 @@ module Gitlab
 
           Note.create!(attributes)
         end
+
+        def submitted_at
+          @submitted_at ||= (review.submitted_at || merge_request.updated_at)
+        end
       end
     end
   end

lib/gitlab/github_import/representation/pull_request_review.rb

@@ -29,7 +29,7 @@ module Gitlab
           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

spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb

@@ -167,6 +167,19 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
       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 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
     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

spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb

@@ -68,5 +68,11 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
 
       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