diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb index 0d4de385f5eee9f821e652c56300eba4e0e8330e..99f4adbe317477eb8def7cda3ad04e9f9b66441f 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -10,6 +10,7 @@ def initialize(project, hash) @project = project @formatter = Gitlab::ImportFormatter.new @user_finder = UserFinder.new(project) + @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) # Object should behave as a object so we can remove object.is_a?(Hash) check # This will be fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/412328 @@ -19,10 +20,6 @@ def initialize(project, hash) def execute log_info(import_stage: 'import_pull_request', message: 'starting', iid: object[:iid]) - description = '' - description += author_line - description += object[:description] if object[:description] - attributes = { iid: object[:iid], title: object[:title], @@ -49,7 +46,19 @@ def execute private - attr_reader :object, :project, :formatter, :user_finder + attr_reader :object, :project, :formatter, :user_finder, :mentions_converter + + def description + description = '' + description += author_line + description += object[:description] if object[:description] + + if Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) + description = mentions_converter.convert(description) + end + + description + end def author_line return '' if user_finder.uid(object) diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb index d58f7cec8ff5161f73457b152e8467b05ab02ce8..19e5cdcbdc27a4da38640ace7506849565c046d2 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb @@ -11,6 +11,7 @@ def initialize(project, hash) @project = project @user_finder = UserFinder.new(project) @formatter = Gitlab::ImportFormatter.new + @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) @object = hash.with_indifferent_access end @@ -43,7 +44,7 @@ def execute private - attr_reader :object, :project, :formatter, :user_finder + attr_reader :object, :project, :formatter, :user_finder, :mentions_converter def import_data_valid? project.import_data&.credentials && project.import_data&.data @@ -192,12 +193,18 @@ def pull_request_comment_attributes(comment) note = "*By #{comment.author_username} (#{comment.author_email})*\n\n" end + comment_note = if Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) + mentions_converter.convert(comment.note) + else + comment.note + end + note += # Provide some context for replying if comment.parent_comment - "> #{comment.parent_comment.note.truncate(80)}\n\n#{comment.note}" + "> #{comment.parent_comment.note.truncate(80)}\n\n#{comment_note}" else - comment.note + comment_note end { diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb index f8d0521afb26118a6749f9136f68373c76f88321..32bea5942d43107dd2140c5f268e703be8400c14 100644 --- a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -5,7 +5,7 @@ module BitbucketServerImport module Importers class UsersImporter include Loggable - include UserCaching + include UserFromMention BATCH_SIZE = 100 @@ -47,7 +47,7 @@ def cache_users(users) hash[cache_key] = user.email end - ::Gitlab::Cache::Import::Caching.write_multiple(users_hash) + cache_multiple(users_hash) end def client diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb new file mode 100644 index 0000000000000000000000000000000000000000..8b1eeb6e0078736d141d3c12f2502cfd74da41b1 --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + class MentionsConverter + include UserFromMention + + MENTIONS_REGEX = User.reference_pattern + MENTION_PLACEHOLDER = '~GITLAB_MENTION_PLACEHOLDER~' + + attr_reader :project_id + + def initialize(project_id) + @project_id = project_id + end + + def convert(text) + replace_mentions(text.dup) + end + + private + + def replace_mentions(text) + mentions = text.scan(MENTIONS_REGEX).flatten + altered_mentions = [] + + mentions.each do |mention| + user = user_from_cache(mention) + + if user + altered_mentions << ["@#{mention}", "#{MENTION_PLACEHOLDER}#{user.username}"] + next + end + + altered_mentions << ["@#{mention}", "`#{MENTION_PLACEHOLDER}#{mention}`"] + end + + altered_mentions.each do |original_mention, altered_mention| + text.sub!(original_mention, altered_mention) + end + + text.gsub(MENTION_PLACEHOLDER, '@') + end + end + end +end diff --git a/lib/gitlab/bitbucket_server_import/user_caching.rb b/lib/gitlab/bitbucket_server_import/user_caching.rb deleted file mode 100644 index 0f0169122c513d7b302ac3b8dab59dc220af4560..0000000000000000000000000000000000000000 --- a/lib/gitlab/bitbucket_server_import/user_caching.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BitbucketServerImport - module UserCaching - SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' - - def source_user_cache_key(project_id, username) - format(SOURCE_USER_CACHE_KEY, project_id, username) - end - end - end -end diff --git a/lib/gitlab/bitbucket_server_import/user_from_mention.rb b/lib/gitlab/bitbucket_server_import/user_from_mention.rb new file mode 100644 index 0000000000000000000000000000000000000000..907db24576014cf5f0b9e5e971145bd68f5080a1 --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/user_from_mention.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + module UserFromMention + SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' + + def user_from_cache(mention) + cached_email = read(mention) + + return unless cached_email + + find_user(cached_email) + end + + def cache_multiple(hash) + ::Gitlab::Cache::Import::Caching.write_multiple(hash, timeout: timeout) + end + + def source_user_cache_key(project_id, username) + format(SOURCE_USER_CACHE_KEY, project_id, username) + end + + private + + def read(mention) + ::Gitlab::Cache::Import::Caching.read(source_user_cache_key(project_id, mention)) + end + + def find_user(email) + User.find_by_any_email(email, confirmed: true) + end + + def timeout + ::Gitlab::Cache::Import::Caching::LONGER_TIMEOUT + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb index 1ae68f9efb8610c9ae97f7f0d5a92cf1008c2f8b..eeb2f9c8000d428618be6baddab5f5d532551f06 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb @@ -18,6 +18,8 @@ it 'imports the merge request correctly' do expect_next(Gitlab::Import::MergeRequestCreator, project).to receive(:execute).and_call_original expect_next(Gitlab::BitbucketServerImport::UserFinder, project).to receive(:author_id).and_call_original + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project.id).to receive(:convert).and_call_original + expect { importer.execute }.to change { MergeRequest.count }.by(1) merge_request = project.merge_requests.find_by_iid(pull_request.iid) @@ -34,6 +36,18 @@ ) end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project.id).not_to receive(:convert) + + importer.execute + end + end + context 'when the `bitbucket_server_user_mapping_by_username` flag is disabled' do before do stub_feature_flags(bitbucket_server_user_mapping_by_username: false) diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb index 914ebefdb8fc2e4452ff3222f77c0214005a4944..7b662c1a2c7daea7de2ea07ffe0258a5c96eca5a 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb @@ -17,6 +17,7 @@ let_it_be(:pull_request_data) { Gitlab::Json.parse(fixture_file('importers/bitbucket_server/pull_request.json')) } let_it_be(:pull_request) { BitbucketServer::Representation::PullRequest.new(pull_request_data) } let_it_be(:note_author) { create(:user, username: 'note_author', email: 'note_author@example.org') } + let(:mentions_converter) { Gitlab::BitbucketServerImport::MentionsConverter.new(project) } let!(:pull_request_author) do create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') @@ -79,6 +80,10 @@ def expect_log(stage:, message:) .to receive(:info).with(include(import_stage: stage, message: message)) end + before do + allow(Gitlab::BitbucketServerImport::MentionsConverter).to receive(:new).and_return(mentions_converter) + end + subject(:importer) { described_class.new(project.reload, pull_request.to_hash) } describe '#execute' do @@ -113,6 +118,8 @@ def expect_log(stage:, message:) end it 'imports the stand alone comments' do + expect(mentions_converter).to receive(:convert).and_call_original + expect { subject.execute }.to change { Note.count }.by(1) expect(merge_request.notes.count).to eq(1) @@ -124,6 +131,66 @@ def expect_log(stage:, message:) ) end + context 'when the author is not found' do + before do + allow_next_instance_of(Gitlab::BitbucketServerImport::UserFinder) do |user_finder| + allow(user_finder).to receive(:uid).and_return(nil) + end + end + + it 'adds a note with the author username and email' do + subject.execute + + expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") + end + end + + context 'when the note has a parent note' do + let(:pr_note) do + instance_double( + BitbucketServer::Representation::Comment, + note: 'Note', + author_email: note_author.email, + author_username: note_author.username, + comments: [], + created_at: now, + updated_at: now, + parent_comment: pr_parent_note + ) + end + + let(:pr_parent_note) do + instance_double( + BitbucketServer::Representation::Comment, + note: 'Parent note', + author_email: note_author.email, + author_username: note_author.username, + comments: [], + created_at: now, + updated_at: now, + parent_comment: nil + ) + end + + it 'adds the parent note before the actual note' do + subject.execute + + expect(Note.first.note).to include("> #{pr_parent_note.note}\n\n") + end + end + + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect(mentions_converter).not_to receive(:convert) + + subject.execute + end + end + it 'logs its progress' do expect_log(stage: 'import_standalone_pr_comments', message: 'starting') expect_log(stage: 'import_standalone_pr_comments', message: 'finished') @@ -181,6 +248,8 @@ def expect_log(stage:, message:) end it 'imports the threaded discussion' do + expect(mentions_converter).to receive(:convert).and_call_original.twice + expect { subject.execute }.to change { Note.count }.by(2) expect(merge_request.discussions.count).to eq(1) @@ -204,6 +273,18 @@ def expect_log(stage:, message:) expect(reply_note.position.new_line).to eq(pr_inline_note.new_pos) end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect(mentions_converter).not_to receive(:convert) + + subject.execute + end + end + it 'logs its progress' do expect_log(stage: 'import_inline_comments', message: 'starting') expect_log(stage: 'import_inline_comments', message: 'finished') diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..46800c924c9e19ab8678bad4e7f4f5c276af858e --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::MentionsConverter, :clean_gitlab_redis_cache, feature_category: :importers do + let(:project_id) { 12 } + let(:text) { 'text without @ mentions' } + let(:source_user_cache_prefix) { "bitbucket_server/project/#{project_id}/source/username" } + + subject(:converted_text) { described_class.new(project_id).convert(text) } + + describe '#convert' do + context 'when the text has no mentions' do + it 'does not change the text' do + expect(converted_text).to eq(text) + end + end + + context 'when the text has a mention' do + let(:text) { 'mentioning @john' } + + context 'when the mention has matching cached email' do + before do + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john", 'john@example.com') + end + + context 'when a user with the email does not exist on gitlab' do + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `@john`') + end + end + + context 'when a user with the same email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + it "replaces the mention with the user's username" do + expect(converted_text).to eq('mentioning @johndoe') + end + end + + context 'when a user with the same username but not email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'john') } + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `@john`') + end + end + end + + context 'when there is cached email but not for the mentioned username' do + before do + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/jane", 'jane@example.com') + end + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `@john`') + end + + context 'when a user with the same email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'jane', email: 'jane@example.com') } + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `@john`') + end + end + end + + context 'when the mention has digits, underscores, uppercase and hyphens' do + let(:text) { '@john_DOE-123' } + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + before do + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john_DOE-123", 'john@example.com') + end + + it "replaces the mention with the user's username" do + expect(converted_text).to eq('@johndoe') + end + end + + context 'when the mention has emails' do + let(:text) { "@john's email is john@gmail.com and @jane's email is info@jane." } + + it 'does not alter the emails' do + expect(converted_text).to eq("`@john`'s email is john@gmail.com and `@jane`'s email is info@jane.") + end + end + + context 'when no emails are cached' do + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `@john`') + end + end + end + + context 'when the text has multiple mentions' do + let(:text) { "@john, @jane-doe and @johndoe123 with \n@john again on a newline" } + + context 'if none of the mentions have matching cached emails and users' do + it 'puts every mention in backticks' do + expect(converted_text).to eq("`@john`, `@jane-doe` and `@johndoe123` with \n`@john` again on a newline") + end + end + + context 'if one of the mentions have matching user' do + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + before do + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john", 'john@example.com') + end + + it 'replaces all mentions with the username and puts rest of mentions in backticks' do + expect(converted_text).to eq("@johndoe, `@jane-doe` and `@johndoe123` with \n@johndoe again on a newline") + end + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..73f9cde832286e43176fdfac2352953bd36631c4 --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::UserFromMention, :clean_gitlab_redis_cache, feature_category: :importers do + let(:project_id) { 11 } + let(:username) { '@johndoe' } + let(:email) { 'john@gmail.com' } + let(:hash) { { key: 'value' } } + let(:cache_key) { "bitbucket_server/project/#{project_id}/source/username/#{username}" } + + let(:example) do + Class.new do + include Gitlab::BitbucketServerImport::UserFromMention + + def initialize(project_id) + @project_id = project_id + end + + attr_reader :project_id + + def foo(mention) + user_from_cache(mention) + end + + def bar(hash) + cache_multiple(hash) + end + end + end + + subject(:example_class) { example.new(project_id) } + + describe '#user_from_cache' do + it 'returns nil if the cache is empty' do + expect(example_class.foo(username)).to be_nil + end + + context 'when the username and email is cached' do + before do + ::Gitlab::Cache::Import::Caching.write(cache_key, email) + end + + context 'if a user with the email does not exist' do + it 'returns nil' do + expect(example_class.foo(username)).to be_nil + end + end + + context 'if a user with the email exists' do + let!(:user) { create(:user, email: email) } + + it 'returns the user' do + expect(example_class.foo(username)).to eq(user) + end + end + end + end + + describe '#cache_multiple' do + it 'calls write_multiple with the hash' do + expect(Gitlab::Cache::Import::Caching).to receive(:write_multiple).with(hash, timeout: 72.hours) + + example_class.bar(hash) + end + end +end