diff options
author | Stan Hu <stanhu@gmail.com> | 2018-08-07 13:53:25 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-08-07 14:34:25 -0700 |
commit | 8c467b917537a87ae95c2fb5d3464ed4fe7e102f (patch) | |
tree | 73f0a01b196409839e94bcdaf3b95c4afa1a3bc7 | |
parent | f3b36ac1171f6d170d008c52a0a324a438f3e886 (diff) | |
download | gitlab-ce-8c467b917537a87ae95c2fb5d3464ed4fe7e102f.tar.gz |
Fix Bitbucket Cloud importer omitting replies
Inline diff comments did not have the proper position, so even though
they had line codes the merge request validation would fail. Now
we cache the line position for each parent comment and use that.
Closes #50052
-rw-r--r-- | changelogs/unreleased/sh-fix-bitbucket-cloud-importer-replies.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/bitbucket_import/importer.rb | 23 | ||||
-rw-r--r-- | spec/lib/gitlab/bitbucket_import/importer_spec.rb | 75 |
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 |