Commit 2b6b33c4 authored by Kassio Borges's avatar Kassio Borges Committed by Alper Akgun

GithubImporter: Thread diff notes

Enable threaded diff notes to be migrated as discussions to GitLab.

Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/336596

MR:
Changelog: added
parent dd2554bc
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment