diff --git a/app/services/import/github/notes/create_service.rb b/app/services/import/github/notes/create_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..79145f4231325994c194e27bcfbdb3407cc8db07 --- /dev/null +++ b/app/services/import/github/notes/create_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Import + module Github + module Notes + class CreateService < ::Notes::CreateService + # Github does not have support to quick actions in notes (like /assign) + # Therefore, when importing notes we skip the quick actions processing + def quick_actions_supported?(_note) + false + end + end + end + end +end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 194c3d7bf7b3bf115e9f05465bae3f9b058584d6..9a0db3bb9aa2e1118e77333d803325d619397142 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -43,7 +43,7 @@ def execute(skip_capture_diff_note_position: false) private def execute_quick_actions(note) - return yield(false) unless quick_actions_service.supported?(note) + return yield(false) unless quick_actions_supported?(note) content, update_params, message = quick_actions_service.execute(note, quick_action_options) only_commands = content.empty? @@ -54,6 +54,10 @@ def execute_quick_actions(note) do_commands(note, update_params, message, only_commands) end + def quick_actions_supported?(note) + quick_actions_service.supported?(note) + end + def quick_actions_service @quick_actions_service ||= QuickActionsService.new(project, current_user) end diff --git a/config/feature_flags/development/github_importer_use_diff_note_with_suggestions.yml b/config/feature_flags/development/github_importer_use_diff_note_with_suggestions.yml new file mode 100644 index 0000000000000000000000000000000000000000..c106d5131ffdceb2214ad5c20e37cfac79625b02 --- /dev/null +++ b/config/feature_flags/development/github_importer_use_diff_note_with_suggestions.yml @@ -0,0 +1,8 @@ +--- +name: github_importer_use_diff_note_with_suggestions +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71765 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344309 +milestone: '14.5' +type: development +group: group::import +default_enabled: false diff --git a/lib/gitlab/github_import/importer/diff_note_importer.rb b/lib/gitlab/github_import/importer/diff_note_importer.rb index 4cfc920e2e34c5d5f3c5c7cd331e0609a59eb73a..eaadcb69fa4ecd999d07e0f2ae4ed1558c23690e 100644 --- a/lib/gitlab/github_import/importer/diff_note_importer.rb +++ b/lib/gitlab/github_import/importer/diff_note_importer.rb @@ -4,58 +4,132 @@ module Gitlab module GithubImport module Importer class DiffNoteImporter - attr_reader :note, :project, :client, :user_finder - - # note - An instance of `Gitlab::GithubImport::Representation::DiffNote`. - # project - An instance of `Project`. - # client - An instance of `Gitlab::GithubImport::Client`. + # note - An instance of `Gitlab::GithubImport::Representation::DiffNote` + # project - An instance of `Project` + # client - An instance of `Gitlab::GithubImport::Client` def initialize(note, project, client) @note = note @project = project @client = client - @user_finder = GithubImport::UserFinder.new(project, client) end def execute - return unless (mr_id = find_merge_request_id) + return if merge_request_id.blank? - author_id, author_found = user_finder.author_id_for(note) + build_author_attributes - note_body = MarkdownText.format(note.note, note.author, author_found) + # Diff notes with suggestions are imported with DiffNote, which is + # slower to import than LegacyDiffNote. Importing DiffNote is slower + # because it cannot use the BulkImporting strategy, which skips + # callbacks and validations. For this reason, notes that don't have + # suggestions are still imported with LegacyDiffNote + if import_with_diff_note? + import_with_diff_note + else + import_with_legacy_diff_note + end + rescue ActiveRecord::InvalidForeignKey => e + # It's possible the project and the issue have been deleted since + # scheduling this job. In this case we'll just skip creating the note + Logger.info( + message: e.message, + github_identifiers: note.github_identifiers + ) + end - attributes = { - discussion_id: Discussion.discussion_id(note), + private + + attr_reader :note, :project, :client, :author_id, :author_found + + def import_with_diff_note? + note.contains_suggestion? && use_diff_note_with_suggestions_enabled? + end + + def use_diff_note_with_suggestions_enabled? + Feature.enabled?( + :github_importer_use_diff_note_with_suggestions, + default_enabled: :yaml + ) + end + + def build_author_attributes + @author_id, @author_found = user_finder.author_id_for(note) + end + + # rubocop:disable Gitlab/BulkInsert + def import_with_legacy_diff_note + log_diff_note_creation('LegacyDiffNote') + # It's possible that during an import we'll insert tens of thousands + # of diff notes. If we were to use the Note/LegacyDiffNote model here + # we'd also have to run additional queries for both validations and + # callbacks, putting a lot of pressure on the database. + # + # To work around this we're using bulk_insert with a single row. This + # 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_id: mr_id, + system: false, + type: 'LegacyDiffNote', + discussion_id: Discussion.discussion_id(note), + noteable_id: merge_request_id, project_id: project.id, author_id: author_id, note: note_body, - system: false, commit_id: note.original_commit_id, line_code: note.line_code, - type: 'LegacyDiffNote', created_at: note.created_at, updated_at: note.updated_at, st_diff: note.diff_hash.to_yaml - } + }]) + end + # rubocop:enabled Gitlab/BulkInsert - # It's possible that during an import we'll insert tens of thousands - # of diff notes. If we were to use the Note/LegacyDiffNote model here - # we'd also have to run additional queries for both validations and - # callbacks, putting a lot of pressure on the database. - # - # To work around this we're using bulk_insert with a single row. This - # 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, [attributes]) # rubocop:disable Gitlab/BulkInsert - rescue ActiveRecord::InvalidForeignKey - # It's possible the project and the issue have been deleted since - # scheduling this job. In this case we'll just skip creating the note. + def import_with_diff_note + log_diff_note_creation('DiffNote') + + ::Import::Github::Notes::CreateService.new(project, author, { + noteable_type: 'MergeRequest', + system: false, + type: 'DiffNote', + noteable_id: merge_request_id, + project_id: project.id, + note: note_body, + commit_id: note.original_commit_id, + created_at: note.created_at, + updated_at: note.updated_at, + position: note.diff_position(merge_request) + }).execute + end + + def note_body + @note_body ||= MarkdownText.format(note.note, note.author, author_found) + end + + def author + @author ||= User.find(author_id) + end + + def merge_request + @merge_request ||= MergeRequest.find(merge_request_id) end # Returns the ID of the merge request this note belongs to. - def find_merge_request_id - GithubImport::IssuableFinder.new(project, note).database_id + def merge_request_id + @merge_request_id ||= GithubImport::IssuableFinder.new(project, note).database_id + end + + def user_finder + @user_finder ||= GithubImport::UserFinder.new(project, client) + end + + def log_diff_note_creation(model) + Logger.info( + project_id: project.id, + importer: self.class.name, + github_identifiers: note.github_identifiers, + model: model + ) end end end diff --git a/lib/gitlab/github_import/representation/diff_note.rb b/lib/gitlab/github_import/representation/diff_note.rb index a3dcd2e380c6ceadd02b599f3cee9d1444c3e1b4..c7dc117032f8d7abe1bfef09938aa5e8e19b8c30 100644 --- a/lib/gitlab/github_import/representation/diff_note.rb +++ b/lib/gitlab/github_import/representation/diff_note.rb @@ -10,8 +10,9 @@ class DiffNote attr_reader :attributes expose_attribute :noteable_type, :noteable_id, :commit_id, :file_path, - :diff_hunk, :author, :note, :created_at, :updated_at, - :original_commit_id, :note_id + :diff_hunk, :author, :created_at, :updated_at, + :original_commit_id, :note_id, :end_line, :start_line, + :side NOTEABLE_ID_REGEX = %r{/pull/(?\d+)}i.freeze @@ -42,7 +43,8 @@ def self.from_api_response(note) updated_at: note.updated_at, note_id: note.id, end_line: note.line, - start_line: note.start_line + start_line: note.start_line, + side: note.side } new(hash) @@ -60,17 +62,31 @@ def self.from_json_hash(raw_hash) # Hash must be Symbols. def initialize(attributes) @attributes = attributes + + @note_formatter = DiffNotes::SuggestionFormatter.new( + note: attributes[:note], + start_line: attributes[:start_line], + end_line: attributes[:end_line] + ) + end + + def contains_suggestion? + @note_formatter.contains_suggestion? + end + + def note + @note_formatter.formatted_note end def line_code diff_line = Gitlab::Diff::Parser.new.parse(diff_hunk.lines).to_a.last - Gitlab::Git - .diff_line_code(file_path, diff_line.new_pos, diff_line.old_pos) + Gitlab::Git.diff_line_code(file_path, diff_line.new_pos, diff_line.old_pos) end # Returns a Hash that can be used to populate `notes.st_diff`, removing # the need for requesting Git data for every diff note. + # Used when importing with LegacyDiffNote def diff_hash { diff: diff_hunk, @@ -85,12 +101,15 @@ def diff_hash } end - def note - @note ||= DiffNotes::SuggestionFormatter.formatted_note_for( - note: attributes[:note], - start_line: attributes[:start_line], - end_line: attributes[:end_line] - ) + # Used when importing with DiffNote + def diff_position(merge_request) + position_params = { + diff_refs: merge_request.diff_refs, + old_path: file_path, + new_path: file_path + } + + Gitlab::Diff::Position.new(position_params.merge(diff_line_params)) end def github_identifiers @@ -100,6 +119,20 @@ def github_identifiers noteable_type: noteable_type } end + + private + + def diff_line_params + if addition? + { new_line: end_line, old_line: nil } + else + { new_line: nil, old_line: end_line } + end + end + + def addition? + side == 'RIGHT' + end end end end diff --git a/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb b/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb index 4e5855ee4cdbdf36f0b9abe342e1bfd315c4bdd4..38b15c4b5bbf898a0dc2f18eb430b9fc69430e97 100644 --- a/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb +++ b/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb @@ -10,30 +10,38 @@ module GithubImport module Representation module DiffNotes class SuggestionFormatter + include Gitlab::Utils::StrongMemoize + # A github suggestion: # - the ```suggestion tag must be the first text of the line # - it might have up to 3 spaces before the ```suggestion tag # - extra text on the ```suggestion tag line will be ignored GITHUB_SUGGESTION = /^\ {,3}(?```suggestion\b).*(?\R)/.freeze - def self.formatted_note_for(...) - new(...).formatted_note - end - def initialize(note:, start_line: nil, end_line: nil) @note = note @start_line = start_line @end_line = end_line end + # Returns a tuple with: + # - a boolean indicating if the note has suggestions + # - the note with the suggestion formatted for Gitlab def formatted_note - if contains_suggestion? - note.gsub( - GITHUB_SUGGESTION, - "\\k:#{suggestion_range}\\k" - ) - else - note + @formatted_note ||= + if contains_suggestion? + note.gsub( + GITHUB_SUGGESTION, + "\\k:#{suggestion_range}\\k" + ) + else + note + end + end + + def contains_suggestion? + strong_memoize(:contain_suggestion) do + note.to_s.match?(GITHUB_SUGGESTION) end end @@ -41,10 +49,6 @@ def formatted_note attr_reader :note, :start_line, :end_line - def contains_suggestion? - note.to_s.match?(GITHUB_SUGGESTION) - end - # Github always saves the comment on the _last_ line of the range. # Therefore, the diff hunk will always be related to lines before # the comment itself. diff --git a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb index 3dc15c7c0599fc0af2107b5c95aba6a93192a5b6..b671b1268510a401713739f3a0e5a12ef73db7a8 100644 --- a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb @@ -2,156 +2,224 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter do - let(:project) { create(:project) } - let(:client) { double(:client) } - let(:user) { create(:user) } - let(:created_at) { Time.new(2017, 1, 1, 12, 00) } - let(:updated_at) { Time.new(2017, 1, 1, 12, 15) } +RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_failures do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } - let(:hunk) do - '@@ -1 +1 @@ + let(:client) { double(:client) } + let(:discussion_id) { 'b0fa404393eeebb4e82becb8104f238812bb1fe6' } + let(:created_at) { Time.new(2017, 1, 1, 12, 00).utc } + let(:updated_at) { Time.new(2017, 1, 1, 12, 15).utc } + let(:note_body) { 'Hello' } + let(:file_path) { 'files/ruby/popen.rb' } + + let(:diff_hunk) do + '@@ -14 +14 @@ -Hello +Hello world' end - let(:note) do + let(:note_representation) do Gitlab::GithubImport::Representation::DiffNote.new( noteable_type: 'MergeRequest', noteable_id: 1, commit_id: '123abc', original_commit_id: 'original123abc', - file_path: 'README.md', - diff_hunk: hunk, - author: Gitlab::GithubImport::Representation::User - .new(id: user.id, login: user.username), - note: 'Hello', + file_path: file_path, + author: Gitlab::GithubImport::Representation::User.new(id: user.id, login: user.username), + note: note_body, created_at: created_at, updated_at: updated_at, - github_id: 1 + start_line: nil, + end_line: 15, + github_id: 1, + diff_hunk: diff_hunk, + side: 'RIGHT' ) end - let(:importer) { described_class.new(note, project, client) } + subject(:importer) { described_class.new(note_representation, project, client) } + + shared_examples 'diff notes without suggestion' do + it 'imports the note as legacy diff note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.author_id).to eq(user.id) + expect(note.commit_id).to eq('original123abc') + expect(note.created_at).to eq(created_at) + expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) + expect(note.discussion_id).to eq(discussion_id) + expect(note.line_code).to eq(note_representation.line_code) + expect(note.note).to eq('Hello') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.noteable_type).to eq('MergeRequest') + expect(note.project_id).to eq(project.id) + expect(note.st_diff).to eq(note_representation.diff_hash) + expect(note.system).to eq(false) + expect(note.type).to eq('LegacyDiffNote') + expect(note.updated_at).to eq(updated_at) + end + + it 'adds a "created by:" note when the author cannot be found' do + stub_user_finder(project.creator_id, false) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.author_id).to eq(project.creator_id) + expect(note.note).to eq("*Created by: #{user.username}*\n\nHello") + end + + it 'does not import the note when a foreign key error is raised' do + stub_user_finder(project.creator_id, false) + + expect(Gitlab::Database.main) + .to receive(:bulk_insert) + .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key') + + expect { subject.execute } + .not_to change(LegacyDiffNote, :count) + end + end describe '#execute' do context 'when the merge request no longer exists' do it 'does not import anything' do - expect(Gitlab::Database.main).not_to receive(:bulk_insert) - - importer.execute + expect { subject.execute } + .to not_change(DiffNote, :count) + .and not_change(LegacyDiffNote, :count) end end context 'when the merge request exists' do - let!(:merge_request) do + let_it_be(:merge_request) do create(:merge_request, source_project: project, target_project: project) end before do - allow(importer) - .to receive(:find_merge_request_id) - .and_return(merge_request.id) - end - - it 'imports the note' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([user.id, true]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .with( - LegacyDiffNote.table_name, - [ - { - discussion_id: anything, - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - project_id: project.id, - author_id: user.id, - note: 'Hello', - system: false, - commit_id: 'original123abc', - line_code: note.line_code, - type: 'LegacyDiffNote', - created_at: created_at, - updated_at: updated_at, - st_diff: note.diff_hash.to_yaml - } - ] - ) - .and_call_original - - importer.execute + expect_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| + expect(finder) + .to receive(:database_id) + .and_return(merge_request.id) + end + + expect(Discussion) + .to receive(:discussion_id) + .and_return(discussion_id) end - it 'imports the note when the author could not be found' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([project.creator_id, false]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .with( - LegacyDiffNote.table_name, - [ - { - discussion_id: anything, - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - project_id: project.id, - author_id: project.creator_id, - note: "*Created by: #{user.username}*\n\nHello", - system: false, - commit_id: 'original123abc', - line_code: note.line_code, - type: 'LegacyDiffNote', - created_at: created_at, - updated_at: updated_at, - st_diff: note.diff_hash.to_yaml - } - ] - ) - .and_call_original - - importer.execute - end - - it 'produces a valid LegacyDiffNote' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([user.id, true]) - - importer.execute - - note = project.notes.diff_notes.take - - expect(note).to be_valid - expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) + context 'when github_importer_use_diff_note_with_suggestions is disabled' do + before do + stub_feature_flags(github_importer_use_diff_note_with_suggestions: false) + end + + it_behaves_like 'diff notes without suggestion' + + context 'when the note has suggestions' do + let(:note_body) do + <<~EOB + Suggestion: + ```suggestion + what do you think to do it like this + ``` + EOB + end + + it 'imports the note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .and not_change(DiffNote, :count) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.note) + .to eq <<~NOTE + Suggestion: + ```suggestion:-0+0 + what do you think to do it like this + ``` + NOTE + end + end end - it 'does not import the note when a foreign key error is raised' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([project.creator_id, false]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key') - - expect { importer.execute }.not_to raise_error + context 'when github_importer_use_diff_note_with_suggestions is enabled' do + before do + stub_feature_flags(github_importer_use_diff_note_with_suggestions: true) + end + + it_behaves_like 'diff notes without suggestion' + + context 'when the note has suggestions' do + let(:note_body) do + <<~EOB + Suggestion: + ```suggestion + what do you think to do it like this + ``` + EOB + end + + it 'imports the note as diff note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(DiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.noteable_type).to eq('MergeRequest') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.project_id).to eq(project.id) + expect(note.author_id).to eq(user.id) + expect(note.system).to eq(false) + expect(note.discussion_id).to eq(discussion_id) + expect(note.commit_id).to eq('original123abc') + expect(note.line_code).to eq(note_representation.line_code) + expect(note.type).to eq('DiffNote') + expect(note.created_at).to eq(created_at) + expect(note.updated_at).to eq(updated_at) + expect(note.position.to_h).to eq({ + base_sha: merge_request.diffs.diff_refs.base_sha, + head_sha: merge_request.diffs.diff_refs.head_sha, + start_sha: merge_request.diffs.diff_refs.start_sha, + new_line: 15, + old_line: nil, + new_path: file_path, + old_path: file_path, + position_type: 'text', + line_range: nil + }) + expect(note.note) + .to eq <<~NOTE + Suggestion: + ```suggestion:-0+0 + what do you think to do it like this + ``` + NOTE + end + end end end end - describe '#find_merge_request_id' do - it 'returns a merge request ID' do - expect_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |instance| - expect(instance).to receive(:database_id).and_return(10) - end - - expect(importer.find_merge_request_id).to eq(10) + def stub_user_finder(user, found) + expect_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + expect(finder) + .to receive(:author_id_for) + .and_return([user, found]) 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 be4fc3cbf16ff4d715bbd398bccf9c111d887606..08b4f806f87505b52fe9ed8a88c2e2abef2cde49 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 @@ -20,6 +20,7 @@ line: 23, start_line: nil, id: 1, + side: 'RIGHT', body: <<~BODY Hello World 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 81722c0eba740caec04d4b5d0d09bed2099b9e1e..53797fe0ff05ee51865c8254dba8299f9b9b0c41 100644 --- a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb +++ b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb @@ -9,16 +9,21 @@ +Hello world' end + let(:note_body) { 'Hello world' } + let(:start_line) { nil } + let(:end_line) { 23 } + let(:user_data) { { 'id' => 4, 'login' => 'alice' } } + let(:side) { 'RIGHT' } let(:created_at) { Time.new(2017, 1, 1, 12, 00) } let(:updated_at) { Time.new(2017, 1, 1, 12, 15) } - shared_examples 'a DiffNote' do + shared_examples 'a DiffNote representation' do it 'returns an instance of DiffNote' do expect(note).to be_an_instance_of(described_class) end context 'the returned DiffNote' do - it 'includes the number of the note' do + it 'includes the number of the merge request' do expect(note.noteable_id).to eq(42) end @@ -30,18 +35,6 @@ expect(note.commit_id).to eq('123abc') end - it 'includes the user details' do - expect(note.author) - .to be_an_instance_of(Gitlab::GithubImport::Representation::User) - - expect(note.author.id).to eq(4) - expect(note.author.login).to eq('alice') - end - - it 'includes the note body' do - expect(note.note).to eq('Hello world') - end - it 'includes the created timestamp' do expect(note.created_at).to eq(created_at) end @@ -57,203 +50,192 @@ it 'returns the noteable type' do expect(note.noteable_type).to eq('MergeRequest') end - end - end - - describe '.from_api_response' do - let(:response) do - double( - :response, - html_url: 'https://github.com/foo/bar/pull/42', - path: 'README.md', - commit_id: '123abc', - original_commit_id: 'original123abc', - diff_hunk: hunk, - user: double(:user, id: 4, login: 'alice'), - body: 'Hello world', - created_at: created_at, - updated_at: updated_at, - line: 23, - start_line: nil, - id: 1 - ) - end - - it_behaves_like 'a DiffNote' do - let(:note) { described_class.from_api_response(response) } - end - it 'does not set the user if the response did not include a user' do - allow(response) - .to receive(:user) - .and_return(nil) - - note = described_class.from_api_response(response) - - expect(note.author).to be_nil - end - - it 'formats a suggestion in the note body' do - allow(response) - .to receive(:body) - .and_return <<~BODY - ```suggestion - Hello World - ``` - BODY - - note = described_class.from_api_response(response) - - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end - end - - describe '.from_json_hash' do - let(:hash) do - { - 'noteable_type' => 'MergeRequest', - 'noteable_id' => 42, - 'file_path' => 'README.md', - 'commit_id' => '123abc', - 'original_commit_id' => 'original123abc', - 'diff_hunk' => hunk, - 'author' => { 'id' => 4, 'login' => 'alice' }, - 'note' => 'Hello world', - 'created_at' => created_at.to_s, - 'updated_at' => updated_at.to_s, - 'note_id' => 1 - } - end - - it_behaves_like 'a DiffNote' do - let(:note) { described_class.from_json_hash(hash) } - end - - it 'does not convert the author if it was not specified' do - hash.delete('author') - - note = described_class.from_json_hash(hash) - - expect(note.author).to be_nil - end + describe '#diff_hash' do + it 'returns a Hash containing the diff details' do + expect(note.diff_hash).to eq( + diff: hunk, + new_path: 'README.md', + old_path: 'README.md', + a_mode: '100644', + b_mode: '100644', + new_file: false + ) + end + end - it 'formats a suggestion in the note body' do - hash['note'] = <<~BODY - ```suggestion - Hello World - ``` - BODY + 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) } + + 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( + base_sha: 'base', + head_sha: 'head', + line_range: nil, + new_line: 23, + new_path: 'README.md', + old_line: nil, + old_path: 'README.md', + position_type: 'text', + start_sha: 'start' + ) + end + end + + context 'when the diff is an deletion' do + let(:side) { 'LEFT' } + + it 'returns a Gitlab::Diff::Position' do + expect(note.diff_position(merge_request).to_h).to eq( + base_sha: 'base', + head_sha: 'head', + line_range: nil, + old_line: 23, + new_path: 'README.md', + new_line: nil, + old_path: 'README.md', + position_type: 'text', + start_sha: 'start' + ) + end + end + end - note = described_class.from_json_hash(hash) + describe '#github_identifiers' do + it 'returns a hash with needed identifiers' do + expect(note.github_identifiers).to eq( + noteable_id: 42, + noteable_type: 'MergeRequest', + note_id: 1 + ) + end + end - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end - end + describe '#line_code' do + it 'generates the proper line code' do + note = described_class.new(diff_hunk: hunk, file_path: 'README.md') - describe '#line_code' do - it 'returns a String' do - note = described_class.new(diff_hunk: hunk, file_path: 'README.md') + expect(note.line_code).to eq('8ec9a00bfd09b3190ac6b22251dbb1aa95a0579d_2_2') + end + end - expect(note.line_code).to be_an_instance_of(String) + describe '#note and #contains_suggestion?' do + it 'includes the note body' do + expect(note.note).to eq('Hello world') + expect(note.contains_suggestion?).to eq(false) + end + + context 'when the note have a suggestion' do + let(:note_body) do + <<~BODY + ```suggestion + Hello World + ``` + BODY + end + + it 'returns the suggestion formatted in the note' do + expect(note.note).to eq <<~BODY + ```suggestion:-0+0 + Hello World + ``` + BODY + expect(note.contains_suggestion?).to eq(true) + end + end + + context 'when the note have a multiline suggestion' do + let(:start_line) { 20 } + let(:end_line) { 23 } + let(:note_body) do + <<~BODY + ```suggestion + Hello World + ``` + BODY + end + + it 'returns the multi-line suggestion formatted in the note' do + expect(note.note).to eq <<~BODY + ```suggestion:-3+0 + Hello World + ``` + BODY + expect(note.contains_suggestion?).to eq(true) + end + end + + describe '#author' do + it 'includes the user details' do + expect(note.author).to be_an_instance_of( + Gitlab::GithubImport::Representation::User + ) + + expect(note.author.id).to eq(4) + expect(note.author.login).to eq('alice') + end + + context 'when the author is empty' do + let(:user_data) { nil } + + it 'does not set the user if the response did not include a user' do + expect(note.author).to be_nil + end + end + end + end end end - describe '#diff_hash' do - it 'returns a Hash containing the diff details' do - note = described_class.from_json_hash( - 'noteable_type' => 'MergeRequest', - 'noteable_id' => 42, - 'file_path' => 'README.md', - 'commit_id' => '123abc', - 'original_commit_id' => 'original123abc', - 'diff_hunk' => hunk, - 'author' => { 'id' => 4, 'login' => 'alice' }, - 'note' => 'Hello world', - 'created_at' => created_at.to_s, - 'updated_at' => updated_at.to_s, - 'note_id' => 1 - ) - - expect(note.diff_hash).to eq( - diff: hunk, - new_path: 'README.md', - old_path: 'README.md', - a_mode: '100644', - b_mode: '100644', - new_file: false - ) - end - end + describe '.from_api_response' do + it_behaves_like 'a DiffNote representation' do + let(:response) do + double( + :response, + id: 1, + html_url: 'https://github.com/foo/bar/pull/42', + path: 'README.md', + commit_id: '123abc', + original_commit_id: 'original123abc', + side: side, + user: user_data && double(:user, user_data), + diff_hunk: hunk, + body: note_body, + created_at: created_at, + updated_at: updated_at, + line: end_line, + start_line: start_line + ) + end - describe '#github_identifiers' do - it 'returns a hash with needed identifiers' do - github_identifiers = { - noteable_id: 42, - noteable_type: 'MergeRequest', - note_id: 1 - } - other_attributes = { something_else: '_something_else_' } - note = described_class.new(github_identifiers.merge(other_attributes)) - - expect(note.github_identifiers).to eq(github_identifiers) + subject(:note) { described_class.from_api_response(response) } end end - describe '#note' do - it 'returns the given note' do - hash = { - 'note': 'simple text' - } - - note = described_class.new(hash) - - expect(note.note).to eq 'simple text' - end - - it 'returns the suggestion formatted in the note' do - hash = { - 'note': <<~BODY - ```suggestion - Hello World - ``` - BODY - } - - note = described_class.new(hash) - - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end + describe '.from_json_hash' do + it_behaves_like 'a DiffNote representation' do + let(:hash) do + { + 'note_id' => 1, + 'noteable_type' => 'MergeRequest', + 'noteable_id' => 42, + 'file_path' => 'README.md', + 'commit_id' => '123abc', + 'original_commit_id' => 'original123abc', + 'side' => side, + 'author' => user_data, + 'diff_hunk' => hunk, + 'note' => note_body, + 'created_at' => created_at.to_s, + 'updated_at' => updated_at.to_s, + 'end_line' => end_line, + 'start_line' => start_line + } + end - it 'returns the multi-line suggestion formatted in the note' do - hash = { - 'start_line': 20, - 'end_line': 23, - 'note': <<~BODY - ```suggestion - Hello World - ``` - BODY - } - - note = described_class.new(hash) - - expect(note.note).to eq <<~BODY - ```suggestion:-3+0 - Hello World - ``` - BODY + subject(:note) { described_class.from_json_hash(hash) } end end end diff --git a/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb index 2ffd5f50d3ba901ccb5132e2133ebc6b2f5b5f6d..bcb8575bdbfe9251337b6826d64ecf03d14165b4 100644 --- a/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb @@ -9,13 +9,19 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'handles nil value for note' do note = nil - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'does not allow over 3 leading spaces for valid suggestion' do @@ -26,7 +32,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'allows up to 3 leading spaces' do @@ -44,7 +53,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'does nothing when there is any text without space after the suggestion tag' do @@ -53,7 +65,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'formats single-line suggestions' do @@ -71,7 +86,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'ignores text after suggestion tag on the same line' do @@ -89,7 +107,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multiple single-line suggestions' do @@ -115,7 +136,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multi-line suggestions' do @@ -133,7 +157,10 @@ ``` BODY - expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected) + note_formatter = described_class.new(note: note, start_line: 6, end_line: 8) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multiple multi-line suggestions' do @@ -159,6 +186,9 @@ ``` BODY - expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected) + note_formatter = described_class.new(note: note, start_line: 6, end_line: 8) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end end diff --git a/spec/services/import/github/notes/create_service_spec.rb b/spec/services/import/github/notes/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..57699def848049d698280649a49e326eae78ffa7 --- /dev/null +++ b/spec/services/import/github/notes/create_service_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::Github::Notes::CreateService do + it 'does not support quick actions' do + project = create(:project, :repository) + user = create(:user) + merge_request = create(:merge_request, source_project: project) + + project.add_maintainer(user) + + note = described_class.new( + project, + user, + note: '/close', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id + ).execute + + expect(note.note).to eq('/close') + expect(note.noteable.closed?).to be(false) + end +end