Code cleanup and test threaded discussions

lib/gitlab/bitbucket_server_import/importer.rb

@@ -83,7 +83,7 @@ module Gitlab
         end
 
         created_branches = restore_branch_shas(shas_to_restore)
-        @temp_branches << created_branches
+        @temp_branches += created_branches
         import_repository unless created_branches.empty?
       end
 
@@ -200,7 +200,7 @@ module Gitlab
 
         inline_comments, pr_comments = comments.partition(&:inline_comment?)
 
-        import_inline_comments(inline_comments.map(&:comment), pull_request, merge_request)
+        import_inline_comments(inline_comments.map(&:comment), merge_request)
         import_standalone_pr_comments(pr_comments.map(&:comment), merge_request)
       end
 
@@ -213,32 +213,27 @@ module Gitlab
         metric.update(merged_by_id: user_id, merged_at: timestamp)
       end
 
-      def import_inline_comments(inline_comments, pull_request, merge_request)
+      def import_inline_comments(inline_comments, merge_request)
         inline_comments.each do |comment|
-          parent = build_diff_note(merge_request, comment)
+          position = build_position(merge_request, comment)
+          parent = build_diff_note(merge_request, comment, position)
 
           next unless parent&.persisted?
 
+          discussion_id = parent.discussion_id
+
           comment.comments.each do |reply|
-            begin
-              attributes = pull_request_comment_attributes(reply)
-              attributes.merge!(
-                position: build_position(merge_request, comment),
-                discussion_id: parent.discussion_id,
-                type: 'DiffNote')
-              merge_request.notes.create!(attributes)
-            rescue StandardError => e
-              errors << { type: :pull_request, id: comment.id, errors: e.message }
-            end
+            build_diff_note(merge_request, reply, position, discussion_id)
           end
         end
       end
 
-      def build_diff_note(merge_request, comment)
+      def build_diff_note(merge_request, comment, position, discussion_id = nil)
         attributes = pull_request_comment_attributes(comment)
         attributes.merge!(
-          position: build_position(merge_request, comment),
+          position: position,
           type: 'DiffNote')
+        attributes[:discussion_id] = discussion_id if discussion_id
 
         merge_request.notes.create!(attributes)
       rescue StandardError => e

spec/lib/gitlab/bitbucket_server_import/importer_spec.rb

@@ -4,6 +4,7 @@ describe Gitlab::BitbucketServerImport::Importer do
   include ImportSpecHelper
 
   let(:project) { create(:project, :repository, import_url: 'http://my-bitbucket') }
+  let(:now) { Time.now.utc.change(usec: 0) }
 
   subject { described_class.new(project) }
 
@@ -63,15 +64,15 @@ describe Gitlab::BitbucketServerImport::Importer do
         comment?: false,
         merge_event?: true,
         committer_email: project.owner.email,
-        merge_timestamp: Time.now.utc.change(usec: 0))
+        merge_timestamp: now)
 
       @pr_note = instance_double(
         BitbucketServer::Representation::Comment,
         note: 'Hello world',
         author_email: 'unknown@gmail.com',
         comments: [],
-        created_at: Time.now.utc.change(usec: 0),
-        updated_at: Time.now.utc.change(usec: 0))
+        created_at: now,
+        updated_at: now)
       @pr_comment = instance_double(
         BitbucketServer::Representation::Activity,
         comment?: true,
@@ -104,10 +105,15 @@ describe Gitlab::BitbucketServerImport::Importer do
       expect(note.updated_at).to eq(@pr_note.created_at)
     end
 
-    it 'imports threaded comments' do
-    end
+    it 'imports threaded discussions' do
+      reply = instance_double(
+        BitbucketServer::Representation::PullRequestComment,
+        author_email: 'someuser@gitlab.com',
+        note: 'I agree',
+        created_at: now,
+        updated_at: now
+      )
 
-    it 'imports diff comments' do
       # https://gitlab.com/gitlab-org/gitlab-test/compare/c1acaa58bbcbc3eafe538cb8274ba387047b69f8...5937ac0a7beb003549fc5fd26fc247ad
       inline_note = instance_double(
         BitbucketServer::Representation::PullRequestComment,
@@ -119,9 +125,9 @@ describe Gitlab::BitbucketServerImport::Importer do
         new_pos: 4,
         note: 'Hello world',
         author_email: 'unknown@gmail.com',
-        comments: [],
-        created_at: Time.now.utc.change(usec: 0),
-        updated_at: Time.now.utc.change(usec: 0))
+        comments: [reply],
+        created_at: now,
+        updated_at: now)
 
       inline_comment = instance_double(
         BitbucketServer::Representation::Activity,
@@ -135,22 +141,31 @@ describe Gitlab::BitbucketServerImport::Importer do
       expect { subject.execute }.to change { MergeRequest.count }.by(1)
 
       merge_request = MergeRequest.first
-      expect(merge_request.notes.count).to eq(1)
-      note = merge_request.notes.first
-
-      expect(note.type).to eq('DiffNote')
-      expect(note.note).to eq(inline_note.note)
-      expect(note.created_at).to eq(inline_note.created_at)
-      expect(note.updated_at).to eq(inline_note.updated_at)
-
-      expect(note.position.base_sha).to eq(inline_note.from_sha)
-      expect(note.position.start_sha).to eq(inline_note.from_sha)
-      expect(note.position.head_sha).to eq(inline_note.to_sha)
-      expect(note.position.old_line).to be_nil
-      expect(note.position.new_line).to eq(inline_note.new_pos)
-    end
-
-    it 'falls back to comments if diff comments' do
+      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.type).to eq('DiffNote')
+      expect(start_note.note).to eq(inline_note.note)
+      expect(start_note.created_at).to eq(inline_note.created_at)
+      expect(start_note.updated_at).to eq(inline_note.updated_at)
+      expect(start_note.position.base_sha).to eq(inline_note.from_sha)
+      expect(start_note.position.start_sha).to eq(inline_note.from_sha)
+      expect(start_note.position.head_sha).to eq(inline_note.to_sha)
+      expect(start_note.position.old_line).to be_nil
+      expect(start_note.position.new_line).to eq(inline_note.new_pos)
+
+      reply_note = notes.last
+      expect(reply_note.note).to eq(reply.note)
+      expect(reply_note.author).to eq(project.owner)
+      expect(reply_note.created_at).to eq(reply.created_at)
+      expect(reply_note.updated_at).to eq(reply.created_at)
+      expect(reply_note.position.base_sha).to eq(inline_note.from_sha)
+      expect(reply_note.position.start_sha).to eq(inline_note.from_sha)
+      expect(reply_note.position.head_sha).to eq(inline_note.to_sha)
+      expect(reply_note.position.old_line).to be_nil
+      expect(reply_note.position.new_line).to eq(inline_note.new_pos)
     end
 
     it 'restores branches of inaccessible SHAs' do