Merge branch 'kassio/github-importer-thread-diff-notes' into 'master'

GithubImporter: Thread diff notes

See merge request gitlab-org/gitlab!73595

doc/user/project/import/github.md

@@ -25,6 +25,7 @@ The following aspects of a project are imported:
 - Pull request "merged by" information (GitLab.com & 13.7+)
 - Regular issue and pull request comments
 - [Git Large File Storage (LFS) Objects](../../../topics/git/lfs/index.md)
+- Pull request comments replies in discussions ([GitLab.com & 14.5+](https://gitlab.com/gitlab-org/gitlab/-/issues/336596))
 
 References to pull requests and issues are preserved (GitLab.com & 8.7+), and
 each imported repository maintains visibility level unless that [visibility

lib/gitlab/github_import/importer/diff_note_importer.rb

@@ -16,6 +16,9 @@ module Gitlab
         def execute
           return if merge_request_id.blank?
 
+          note.project = project
+          note.merge_request = merge_request
+
           build_author_attributes
 
           # Diff notes with suggestions are imported with DiffNote, which is
@@ -68,10 +71,10 @@ module Gitlab
           # allows us to efficiently insert data (even if it's just 1 row)
           # without having to use all sorts of hacks to disable callbacks.
           Gitlab::Database.main.bulk_insert(LegacyDiffNote.table_name, [{
-            noteable_type: 'MergeRequest',
+            noteable_type: note.noteable_type,
             system: false,
             type: 'LegacyDiffNote',
-            discussion_id: Discussion.discussion_id(note),
+            discussion_id: note.discussion_id,
             noteable_id: merge_request_id,
             project_id: project.id,
             author_id: author_id,
@@ -89,16 +92,17 @@ module Gitlab
           log_diff_note_creation('DiffNote')
 
           ::Import::Github::Notes::CreateService.new(project, author, {
-            noteable_type: 'MergeRequest',
+            noteable_type: note.noteable_type,
             system: false,
             type: 'DiffNote',
             noteable_id: merge_request_id,
             project_id: project.id,
             note: note_body,
+            discussion_id: note.discussion_id,
             commit_id: note.original_commit_id,
             created_at: note.created_at,
             updated_at: note.updated_at,
-            position: note.diff_position(merge_request)
+            position: note.diff_position
           }).execute
         end
 

lib/gitlab/github_import/representation/diff_note.rb

@@ -7,14 +7,14 @@ module Gitlab
         include ToHash
         include ExposeAttribute
 
-        attr_reader :attributes
-
-        expose_attribute :noteable_type, :noteable_id, :commit_id, :file_path,
-                         :diff_hunk, :author, :created_at, :updated_at,
-                         :original_commit_id, :note_id, :end_line, :start_line,
-                         :side
-
+        NOTEABLE_TYPE = 'MergeRequest'
         NOTEABLE_ID_REGEX = %r{/pull/(?<iid>\d+)}i.freeze
+        DISCUSSION_CACHE_KEY = 'github-importer/discussion-id-map/%{project_id}/%{noteable_id}/%{original_note_id}'
+
+        expose_attribute :noteable_id, :commit_id, :file_path,
+          :diff_hunk, :author, :created_at, :updated_at,
+          :original_commit_id, :note_id, :end_line, :start_line,
+          :side, :in_reply_to_id
 
         # Builds a diff note from a GitHub API response.
         #
@@ -31,7 +31,6 @@ module Gitlab
 
           user = Representation::User.from_api_response(note.user) if note.user
           hash = {
-            noteable_type: 'MergeRequest',
             noteable_id: matches[:iid].to_i,
             file_path: note.path,
             commit_id: note.commit_id,
@@ -44,7 +43,8 @@ module Gitlab
             note_id: note.id,
             end_line: note.line,
             start_line: note.start_line,
-            side: note.side
+            side: note.side,
+            in_reply_to_id: note.in_reply_to_id
           }
 
           new(hash)
@@ -58,6 +58,8 @@ module Gitlab
           new(hash)
         end
 
+        attr_accessor :merge_request, :project
+
         # attributes - A Hash containing the raw note details. The keys of this
         #              Hash must be Symbols.
         def initialize(attributes)
@@ -70,6 +72,10 @@ module Gitlab
           )
         end
 
+        def noteable_type
+          NOTEABLE_TYPE
+        end
+
         def contains_suggestion?
           @note_formatter.contains_suggestion?
         end
@@ -102,7 +108,7 @@ module Gitlab
         end
 
         # Used when importing with DiffNote
-        def diff_position(merge_request)
+        def diff_position
           position_params = {
             diff_refs: merge_request.diff_refs,
             old_path: file_path,
@@ -120,8 +126,25 @@ module Gitlab
           }
         end
 
+        def discussion_id
+          if in_reply_to_id.present?
+            current_discussion_id
+          else
+            Discussion.discussion_id(
+              Struct
+              .new(:noteable_id, :noteable_type)
+              .new(merge_request.id, NOTEABLE_TYPE)
+            ).tap do |discussion_id|
+              cache_discussion_id(discussion_id)
+            end
+          end
+        end
+
         private
 
+        # Required by ExposeAttribute
+        attr_reader :attributes
+
         def diff_line_params
           if addition?
             { new_line: end_line, old_line: nil }
@@ -133,6 +156,22 @@ module Gitlab
         def addition?
           side == 'RIGHT'
         end
+
+        def cache_discussion_id(discussion_id)
+          Gitlab::Cache::Import::Caching.write(discussion_id_cache_key(note_id), discussion_id)
+        end
+
+        def current_discussion_id
+          Gitlab::Cache::Import::Caching.read(discussion_id_cache_key(in_reply_to_id))
+        end
+
+        def discussion_id_cache_key(id)
+          DISCUSSION_CACHE_KEY % {
+            project_id: project.id,
+            noteable_id: merge_request.id,
+            original_note_id: id
+          }
+        end
       end
     end
   end

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

@@ -19,6 +19,7 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNotesImporter do
       updated_at: Time.zone.now,
       line: 23,
       start_line: nil,
+      in_reply_to_id: nil,
       id: 1,
       side: 'RIGHT',
       body: <<~BODY

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

@@ -2,16 +2,32 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
+RSpec.describe Gitlab::GithubImport::Representation::DiffNote, :clean_gitlab_redis_shared_state do
   let(:hunk) do
     '@@ -1 +1 @@
     -Hello
     +Hello world'
   end
 
-  let(:note_body) { 'Hello world' }
+  let(:merge_request) do
+    double(
+      :merge_request,
+      id: 54,
+      diff_refs: double(
+        :refs,
+        base_sha: 'base',
+        start_sha: 'start',
+        head_sha: 'head'
+      )
+    )
+  end
+
+  let(:project) { double(:project, id: 836) }
+  let(:note_id) { 1 }
+  let(:in_reply_to_id) { nil }
   let(:start_line) { nil }
   let(:end_line) { 23 }
+  let(:note_body) { 'Hello world' }
   let(:user_data) { { 'id' => 4, 'login' => 'alice' } }
   let(:side) { 'RIGHT' }
   let(:created_at) { Time.new(2017, 1, 1, 12, 00) }
@@ -44,7 +60,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
       end
 
       it 'includes the GitHub ID' do
-        expect(note.note_id).to eq(1)
+        expect(note.note_id).to eq(note_id)
       end
 
       it 'returns the noteable type' do
@@ -65,12 +81,21 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
       end
 
       describe '#diff_position' do
-        let(:diff_refs) { double(:refs, base_sha: 'base', start_sha: 'start', head_sha: 'head') }
-        let(:merge_request) { double(:merge_request, diff_refs: diff_refs) }
+        before do
+          note.merge_request = double(
+            :merge_request,
+            diff_refs: double(
+              :refs,
+              base_sha: 'base',
+              start_sha: 'start',
+              head_sha: 'head'
+            )
+          )
+        end
 
         context 'when the diff is an addition' do
           it 'returns a Gitlab::Diff::Position' do
-            expect(note.diff_position(merge_request).to_h).to eq(
+            expect(note.diff_position.to_h).to eq(
               base_sha: 'base',
               head_sha: 'head',
               line_range: nil,
@@ -88,7 +113,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
           let(:side) { 'LEFT' }
 
           it 'returns a Gitlab::Diff::Position' do
-            expect(note.diff_position(merge_request).to_h).to eq(
+            expect(note.diff_position.to_h).to eq(
               base_sha: 'base',
               head_sha: 'head',
               line_range: nil,
@@ -103,6 +128,47 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
         end
       end
 
+      describe '#discussion_id' do
+        before do
+          note.project = project
+          note.merge_request = merge_request
+        end
+
+        context 'when the note is a reply to a discussion' do
+          it 'uses the cached value as the discussion_id only when responding an existing discussion' do
+            expect(Discussion)
+              .to receive(:discussion_id)
+              .and_return('FIRST_DISCUSSION_ID', 'SECOND_DISCUSSION_ID')
+
+            # Creates the first discussion id and caches its value
+            expect(note.discussion_id)
+              .to eq('FIRST_DISCUSSION_ID')
+
+            reply_note = described_class.from_json_hash(
+              'note_id' => note.note_id + 1,
+              'in_reply_to_id' => note.note_id
+            )
+            reply_note.project = project
+            reply_note.merge_request = merge_request
+
+            # Reading from the cached value
+            expect(reply_note.discussion_id)
+              .to eq('FIRST_DISCUSSION_ID')
+
+            new_discussion_note = described_class.from_json_hash(
+              'note_id' => note.note_id + 2,
+              'in_reply_to_id' => nil
+            )
+            new_discussion_note.project = project
+            new_discussion_note.merge_request = merge_request
+
+            # Because it's a new discussion, it must not use the cached value
+            expect(new_discussion_note.discussion_id)
+              .to eq('SECOND_DISCUSSION_ID')
+          end
+        end
+      end
+
       describe '#github_identifiers' do
         it 'returns a hash with needed identifiers' do
           expect(note.github_identifiers).to eq(
@@ -194,7 +260,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
       let(:response) do
         double(
           :response,
-          id: 1,
+          id: note_id,
           html_url: 'https://github.com/foo/bar/pull/42',
           path: 'README.md',
           commit_id: '123abc',
@@ -206,7 +272,8 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
           created_at: created_at,
           updated_at: updated_at,
           line: end_line,
-          start_line: start_line
+          start_line: start_line,
+          in_reply_to_id: in_reply_to_id
         )
       end
 
@@ -218,7 +285,7 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
     it_behaves_like 'a DiffNote representation' do
       let(:hash) do
         {
-          'note_id' => 1,
+          'note_id' => note_id,
           'noteable_type' => 'MergeRequest',
           'noteable_id' => 42,
           'file_path' => 'README.md',
@@ -231,7 +298,8 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do
           'created_at' => created_at.to_s,
           'updated_at' => updated_at.to_s,
           'end_line' => end_line,
-          'start_line' => start_line
+          'start_line' => start_line,
+          'in_reply_to_id' => in_reply_to_id
         }
       end