Merge branch 'sh-fix-bitbucket-cloud-importer-replies' into 'master'

Fix Bitbucket Cloud importer omitting replies

Closes #50052

See merge request gitlab-org/gitlab-ce!21076

3 files changed, 93 insertions, 10 deletions

diff --git a/changelogs/unreleased/sh-fix-bitbucket-cloud-importer-replies.yml b/changelogs/unreleased/sh-fix-bitbucket-cloud-importer-replies.yml
new file mode 100644
index 00000000000..3f7044833f1
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-bitbucket-cloud-importer-replies.yml
@@ -0,0 +1,5 @@
+---
+title: Fix Bitbucket Cloud importer omitting replies
+merge_request: 21076
+author:
+type: fixed
diff --git a/lib/gitlab/bitbucket_import/importer.rb b/lib/gitlab/bitbucket_import/importer.rb
index f3999e690fa..fa0186c854c 100644
--- a/lib/gitlab/bitbucket_import/importer.rb
+++ b/lib/gitlab/bitbucket_import/importer.rb
@@ -188,7 +188,8 @@ module Gitlab
       end
 
       def import_inline_comments(inline_comments, pull_request, merge_request)
-        line_code_map = {}
+        position_map = {}
+        discussion_map = {}
 
         children, parents = inline_comments.partition(&:has_parent?)
 
@@ -196,22 +197,28 @@ module Gitlab
         # relationships. We assume that the child can appear in any order in
         # the JSON.
         parents.each do |comment|
-          line_code_map[comment.iid] = generate_line_code(comment)
+          position_map[comment.iid] = build_position(merge_request, comment)
         end
 
         children.each do |comment|
-          line_code_map[comment.iid] = line_code_map.fetch(comment.parent_id, nil)
+          position_map[comment.iid] = position_map.fetch(comment.parent_id, nil)
         end
 
         inline_comments.each do |comment|
           begin
             attributes = pull_request_comment_attributes(comment)
+            attributes[:discussion_id] = discussion_map[comment.parent_id] if comment.has_parent?
+
             attributes.merge!(
-              position: build_position(merge_request, comment),
-              line_code: line_code_map.fetch(comment.iid),
+              position: position_map[comment.iid],
               type: 'DiffNote')
 
-            merge_request.notes.create!(attributes)
+            note = merge_request.notes.create!(attributes)
+
+            # We can't store a discussion ID until a note is created, so if
+            # replies are created before the parent the discussion ID won't be
+            # linked properly.
+            discussion_map[comment.iid] = note.discussion_id
           rescue StandardError => e
             errors << { type: :pull_request, iid: comment.iid, errors: e.message }
           end
@@ -240,10 +247,6 @@ module Gitlab
         end
       end
 
-      def generate_line_code(pr_comment)
-        Gitlab::Git.diff_line_code(pr_comment.file_path, pr_comment.new_pos, pr_comment.old_pos)
-      end
-
       def pull_request_comment_attributes(comment)
         {
           project: project,
diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
index 05c232d22cf..7a681bc6610 100644
--- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb
+++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
@@ -69,6 +69,7 @@ describe Gitlab::BitbucketImport::Importer do
   let(:project) do
     create(
       :project,
+      :repository,
       import_source: project_identifier,
       import_url: "https://bitbucket.org/#{project_identifier}.git",
       import_data_attributes: { credentials: data }
@@ -85,10 +86,84 @@ describe Gitlab::BitbucketImport::Importer do
     }
   end
 
+  let(:sample) { RepoHelpers.sample_compare }
+
   before do
     allow(importer).to receive(:gitlab_shell) { gitlab_shell }
   end
 
+  subject { described_class.new(project) }
+
+  describe '#import_pull_requests' do
+    before do
+      allow(subject).to receive(:import_wiki)
+      allow(subject).to receive(:import_issues)
+
+      pull_request = instance_double(
+        Bitbucket::Representation::PullRequest,
+        iid: 10,
+        source_branch_sha: sample.commits.last,
+        source_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.source_branch,
+        target_branch_sha: sample.commits.first,
+        target_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.target_branch,
+        title: 'This is a title',
+        description: 'This is a test pull request',
+        state: 'merged',
+        author: 'other',
+        created_at: Time.now,
+        updated_at: Time.now)
+
+      # https://gitlab.com/gitlab-org/gitlab-test/compare/c1acaa58bbcbc3eafe538cb8274ba387047b69f8...5937ac0a7beb003549fc5fd26fc247ad
+      @inline_note = instance_double(
+        Bitbucket::Representation::PullRequestComment,
+        iid: 2,
+        file_path: '.gitmodules',
+        old_pos: nil,
+        new_pos: 4,
+        note: 'Hello world',
+        author: 'root',
+        created_at: Time.now,
+        updated_at: Time.now,
+        inline?: true,
+        has_parent?: false)
+
+      @reply = instance_double(
+        Bitbucket::Representation::PullRequestComment,
+        iid: 3,
+        file_path: '.gitmodules',
+        note: 'Hello world',
+        author: 'root',
+        created_at: Time.now,
+        updated_at: Time.now,
+        inline?: true,
+        has_parent?: true,
+        parent_id: 2)
+
+      comments = [@inline_note, @reply]
+
+      allow(subject.client).to receive(:repo)
+      allow(subject.client).to receive(:pull_requests).and_return([pull_request])
+      allow(subject.client).to receive(:pull_request_comments).with(anything, pull_request.iid).and_return(comments)
+    end
+
+    it 'imports threaded discussions' do
+      expect { subject.execute }.to change { MergeRequest.count }.by(1)
+
+      merge_request = MergeRequest.first
+      expect(merge_request.notes.count).to eq(2)
+      expect(merge_request.notes.map(&:discussion_id).uniq.count).to eq(1)
+
+      notes = merge_request.notes.order(:id).to_a
+      start_note = notes.first
+      expect(start_note).to be_a(DiffNote)
+      expect(start_note.note).to eq(@inline_note.note)
+
+      reply_note = notes.last
+      expect(reply_note).to be_a(DiffNote)
+      expect(reply_note.note).to eq(@reply.note)
+    end
+  end
+
   context 'issues statuses' do
     before do
       # HACK: Bitbucket::Representation.const_get('Issue') seems to return ::Issue without this