diff --git a/doc/user/project/import/github.md b/doc/user/project/import/github.md index ac4c3580af325e89d5aa7bb2204db509b739d31a..df3029ce5817eb6a5b6de9dedb911de7b3225284 100644 --- a/doc/user/project/import/github.md +++ b/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 diff --git a/lib/gitlab/github_import/importer/diff_note_importer.rb b/lib/gitlab/github_import/importer/diff_note_importer.rb index eaadcb69fa4ecd999d07e0f2ae4ed1558c23690e..fa60b2f1b7db3b1b23b27239ab87695d000c8ff1 100644 --- a/lib/gitlab/github_import/importer/diff_note_importer.rb +++ b/lib/gitlab/github_import/importer/diff_note_importer.rb @@ -16,6 +16,9 @@ def initialize(note, project, client) 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 @@ def import_with_legacy_diff_note # 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 @@ def import_with_diff_note 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 diff --git a/lib/gitlab/github_import/representation/diff_note.rb b/lib/gitlab/github_import/representation/diff_note.rb index c7dc117032f8d7abe1bfef09938aa5e8e19b8c30..fecff0644c2090d452d170b51fd95b1e87e0c14f 100644 --- a/lib/gitlab/github_import/representation/diff_note.rb +++ b/lib/gitlab/github_import/representation/diff_note.rb @@ -7,14 +7,14 @@ class DiffNote 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/(?\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 @@ def self.from_api_response(note) 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 @@ def self.from_api_response(note) 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 @@ def self.from_json_hash(raw_hash) 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 @@ def initialize(attributes) ) end + def noteable_type + NOTEABLE_TYPE + end + def contains_suggestion? @note_formatter.contains_suggestion? end @@ -102,7 +108,7 @@ def diff_hash 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 @@ def github_identifiers } 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 @@ def diff_line_params 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 diff --git a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb index 08b4f806f87505b52fe9ed8a88c2e2abef2cde49..1c7b35ed92832e1360b5adc6c43b500419f5c6b8 100644 --- a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb @@ -19,6 +19,7 @@ updated_at: Time.zone.now, line: 23, start_line: nil, + in_reply_to_id: nil, id: 1, side: 'RIGHT', body: <<~BODY diff --git a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb index 53797fe0ff05ee51865c8254dba8299f9b9b0c41..63834cfdb94ec06b4a360cda5f00a555451277de 100644 --- a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb +++ b/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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ '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