diff --git a/config/feature_flags/wip/bitbucket_server_user_mapping.yml b/config/feature_flags/wip/bitbucket_server_user_mapping.yml new file mode 100644 index 0000000000000000000000000000000000000000..e50e3e44c12fd0c46fae58271d396cd3a591b20c --- /dev/null +++ b/config/feature_flags/wip/bitbucket_server_user_mapping.yml @@ -0,0 +1,9 @@ +--- +name: bitbucket_server_user_mapping +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/466356 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165855 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/tbd +milestone: '17.7' +group: group::import and integrate +type: wip +default_enabled: false 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 198f0ed35615cf5bc78720f8b69c7ad5f2c16f79..e0cab6d03fb93d424eff04dfdb16cd4525650615 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -5,6 +5,7 @@ module BitbucketServerImport module Importers class PullRequestImporter include Loggable + include ::Import::PlaceholderReferences::Pusher def initialize(project, hash) @project = project @@ -15,6 +16,8 @@ def initialize(project, hash) # 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 @object = hash.with_indifferent_access + + @reviewer_references = {} end def execute @@ -32,7 +35,7 @@ def execute target_branch: Gitlab::Git.ref_name(object[:target_branch_name]), target_branch_sha: object[:target_branch_sha], state_id: MergeRequest.available_states[object[:state]], - author_id: user_finder.author_id(object), + author_id: author_id(object), created_at: object[:created_at], updated_at: object[:updated_at], imported_from: ::Import::HasImportSource::IMPORT_SOURCES[:bitbucket_server] @@ -41,6 +44,8 @@ def execute creator = Gitlab::Import::MergeRequestCreator.new(project) merge_request = creator.execute(attributes) + push_reference(project, merge_request, :author_id, object[:author_username]) + push_reviewer_references(merge_request) # Create refs/merge-requests/iid/head reference for the merge request merge_request.fetch_ref! @@ -57,27 +62,50 @@ def 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 = mentions_converter.convert(description) if convert_mentions? description end + def convert_mentions? + Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) && + !user_mapping_enabled?(project) + end + def author_line - return '' if user_finder.uid(object) + return '' if user_mapping_enabled?(project) || user_finder.uid(object) formatter.author_line(object[:author]) end + def author_id(pull_request_data) + if user_mapping_enabled?(project) + user_finder.author_id( + username: pull_request_data['author_username'], + display_name: pull_request_data['author'] + ) + else + user_finder.author_id(pull_request_data) + end + end + def reviewers return [] unless object[:reviewers].present? - object[:reviewers].filter_map do |reviewer| - if Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) - user_finder.find_user_id(by: :username, value: reviewer.dig('user', 'slug')) + object[:reviewers].filter_map do |reviewer_data| + if user_mapping_enabled?(project) + uid = user_finder.uid( + username: reviewer_data.dig('user', 'slug'), + display_name: reviewer_data.dig('user', 'displayName') + ) + + @reviewer_references[uid] = reviewer_data.dig('user', 'slug') + + uid + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) + user_finder.find_user_id(by: :username, value: reviewer_data.dig('user', 'slug')) else - user_finder.find_user_id(by: :email, value: reviewer.dig('user', 'emailAddress')) + user_finder.find_user_id(by: :email, value: reviewer_data.dig('user', 'emailAddress')) end end end @@ -89,6 +117,13 @@ def source_branch_sha project.repository.find_commits_by_message(object[:source_branch_sha])&.first&.sha end + + def push_reviewer_references(merge_request) + mr_reviewers = merge_request.merge_request_reviewers + mr_reviewers.each do |mr_reviewer| + push_reference(project, mr_reviewer, :user_id, @reviewer_references[mr_reviewer.user_id]) + end + end end end end diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event.rb index 632377229cba97bdb4da3bb5aea4cbfe67802174..46de3450650f575917809038fe21b27610565a58 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event.rb @@ -15,7 +15,12 @@ def execute(approved_event) event_id: approved_event[:id] ) - user_id = if Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) + user_id = if user_mapping_enabled?(project) + user_finder.uid( + username: approved_event[:approver_username], + display_name: approved_event[:approver_name] + ) + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) user_finder.find_user_id(by: :username, value: approved_event[:approver_username]) else user_finder.find_user_id(by: :email, value: approved_event[:approver_email]) @@ -34,8 +39,12 @@ def execute(approved_event) submitted_at = approved_event[:created_at] || merge_request[:updated_at] - create_approval!(project.id, merge_request.id, user_id, submitted_at) - create_reviewer!(merge_request.id, user_id, submitted_at) + approval, approval_note = create_approval!(project.id, merge_request.id, user_id, submitted_at) + push_reference(project, approval, :user_id, approved_event[:approver_username]) + push_reference(project, approval_note, :author_id, approved_event[:approver_username]) + + reviewer = create_reviewer!(merge_request.id, user_id, submitted_at) + push_reference(project, reviewer, :user_id, approved_event[:approver_username]) if reviewer log_info( import_stage: 'import_approved_event', diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_importer.rb index c9924229f4b7f2774f79b9d6046435c3d880d5ea..fc9f1d81b9bf3ff59689ba9976f5deeb2672e0fc 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_importer.rb @@ -7,6 +7,7 @@ module PullRequestNotes # Base class for importing pull request notes during project import from Bitbucket Server class BaseImporter include Loggable + include ::Import::PlaceholderReferences::Pusher # @param project [Project] # @param merge_request [MergeRequest] diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_note_diff_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_note_diff_importer.rb index a171b57e21afdab21aef6a900c766d282d18add5..0e73959882f2247b2c6fa79a7621af12710ecf6b 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_note_diff_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_note_diff_importer.rb @@ -28,6 +28,8 @@ def create_diff_note(merge_request, comment, position, discussion_id = nil) if note.valid? note.save + push_reference(project, note, :author_id, comment[:author_username]) + return note end @@ -52,7 +54,7 @@ def create_diff_note(merge_request, comment, position, discussion_id = nil) end def pull_request_comment_attributes(comment) - author = user_finder.uid(comment) + author = author(comment) note = '' unless author @@ -79,16 +81,30 @@ def pull_request_comment_attributes(comment) } end + def author(comment) + if user_mapping_enabled?(project) + user_finder.uid( + username: comment[:author_username], + display_name: comment[:author_name] + ) + else + user_finder.uid(comment) + end + end + def create_basic_fallback_note(merge_request, comment, position) attributes = pull_request_comment_attributes(comment) - note = "*Comment on" + note_text = "*Comment on" + + note_text += " #{position.old_path}:#{position.old_line} -->" if position.old_line + note_text += " #{position.new_path}:#{position.new_line}" if position.new_line + note_text += "*\n\n#{comment[:note]}" - note += " #{position.old_path}:#{position.old_line} -->" if position.old_line - note += " #{position.new_path}:#{position.new_line}" if position.new_line - note += "*\n\n#{comment[:note]}" + attributes[:note] = note_text - attributes[:note] = note - merge_request.notes.create!(attributes) + note = merge_request.notes.create!(attributes) + push_reference(project, note, :author_id, comment[:author_username]) + note end end end diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event.rb index 7a3ec39051833498190f24df4612605f3837fc12..c8f9bec3fcbf000625bb9e2bd55f4e0099b4968a 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event.rb @@ -15,7 +15,12 @@ def execute(declined_event) event_id: declined_event[:id] ) - user_id = if Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) + user_id = if user_mapping_enabled?(project) + user_finder.uid( + username: declined_event[:decliner_username], + display_name: declined_event[:decliner_name] + ) + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) user_finder.find_user_id(by: :username, value: declined_event[:decliner_username]) else user_finder.find_user_id(by: :email, value: declined_event[:decliner_email]) @@ -37,8 +42,15 @@ def execute(declined_event) user = User.new(id: user_id) SystemNoteService.change_status(merge_request, merge_request.target_project, user, 'closed', nil) - EventCreateService.new.close_mr(merge_request, user) - create_merge_request_metrics(latest_closed_by_id: user_id, latest_closed_at: declined_event[:created_at]) + + event = record_event(user_id) + push_reference(project, event, :author_id, declined_event[:decliner_username]) + + metric = create_merge_request_metrics( + latest_closed_by_id: user_id, + latest_closed_at: declined_event[:created_at] + ) + push_reference(project, metric, :latest_closed_by_id, declined_event[:decliner_username]) log_info( import_stage: 'import_declined_event', @@ -47,6 +59,19 @@ def execute(declined_event) event_id: declined_event[:id] ) end + + private + + def record_event(user_id) + Event.create!( + project_id: project.id, + author_id: user_id, + action: 'closed', + target_type: 'MergeRequest', + target_id: merge_request.id, + imported_from: ::Import::HasImportSource::IMPORT_SOURCES[:bitbucket_server] + ) + end end end end diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event.rb index fe60a1dbf36960d045e7d9674226d09180f4a7c4..1e0b28b4c64c71fdaf0a5fc5e104a71365726306 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event.rb @@ -15,12 +15,21 @@ def execute(merge_event) event_id: merge_event[:id] ) - committer = merge_event[:committer_email] + user_id = if user_mapping_enabled?(project) + user_finder.uid( + username: merge_event[:committer_username], + display_name: merge_event[:committer_user] + ) + else + user_finder.find_user_id(by: :email, value: merge_event[:committer_email]) + end + + user_id ||= project.creator_id - user_id = user_finder.find_user_id(by: :email, value: committer) || project.creator_id timestamp = merge_event[:merge_timestamp] merge_request.update({ merge_commit_sha: merge_event[:merge_commit] }) - create_merge_request_metrics(merged_by_id: user_id, merged_at: timestamp) + metric = create_merge_request_metrics(merged_by_id: user_id, merged_at: timestamp) + push_reference(project, metric, :merged_by_id, merge_event[:committer_username]) log_info( import_stage: 'import_merge_event', diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes.rb index fadddabd9edab9a8fcddaf1c4b4c1c5d5dcbb2d1..a8b0fa26bc8fd7a57ba5a21ebae5d1ff5580769e 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes.rb @@ -13,10 +13,12 @@ def execute(comment) comment_id: comment[:id] ) - merge_request.notes.create!(pull_request_comment_attributes(comment)) + note = merge_request.notes.create!(pull_request_comment_attributes(comment)) + push_reference(note.project, note, :author_id, comment[:author_username]) comment[:comments].each do |reply| - merge_request.notes.create!(pull_request_comment_attributes(reply)) + note = merge_request.notes.create!(pull_request_comment_attributes(reply)) + push_reference(note.project, note, :author_id, reply[:author_username]) end rescue StandardError => e Gitlab::ErrorTracking.log_exception( 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 ef6715db3bef41f97780129c66779193c9317e97..ad00a8926e4ae5e478e9b3278c52993d857008b5 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 @@ -6,6 +6,7 @@ module Importers class PullRequestNotesImporter include ::Gitlab::Import::MergeRequestHelpers include Loggable + include ::Import::PlaceholderReferences::Pusher def initialize(project, hash) @project = project @@ -56,13 +57,23 @@ def import_data_valid? def import_merge_event(merge_request, merge_event) log_info(import_stage: 'import_merge_event', message: 'starting', iid: merge_request.iid) - committer = merge_event.committer_email + user_id = if user_mapping_enabled?(project) + user_finder.uid( + username: merge_event.committer_username, + display_name: merge_event.committer_name + ) + else + user_finder.find_user_id(by: :email, value: merge_event.committer_email) + end + + user_id ||= project.creator_id - user_id = user_finder.find_user_id(by: :email, value: committer) || project.creator_id timestamp = merge_event.merge_timestamp merge_request.update({ merge_commit_sha: merge_event.merge_commit }) + metric = MergeRequest::Metrics.find_or_initialize_by(merge_request: merge_request) metric.update(merged_by_id: user_id, merged_at: timestamp) + push_reference(project, metric, :merged_by_id, merge_event.committer_username) log_info(import_stage: 'import_merge_event', message: 'finished', iid: merge_request.iid) end @@ -76,7 +87,12 @@ def import_approved_event(merge_request, approved_event) event_id: approved_event.id ) - user_id = if Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) + user_id = if user_mapping_enabled?(project) + user_finder.uid( + username: approved_event.approver_username, + display_name: approved_event.approver_name + ) + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) user_finder.find_user_id(by: :username, value: approved_event.approver_username) else user_finder.find_user_id(by: :email, value: approved_event.approver_email) @@ -86,8 +102,12 @@ def import_approved_event(merge_request, approved_event) submitted_at = approved_event.created_at || merge_request.updated_at - create_approval!(project.id, merge_request.id, user_id, submitted_at) - create_reviewer!(merge_request.id, user_id, submitted_at) + approval, approval_note = create_approval!(project.id, merge_request.id, user_id, submitted_at) + push_reference(project, approval, :user_id, approved_event.approver_username) + push_reference(project, approval_note, :author_id, approved_event.approver_username) + + reviewer = create_reviewer!(merge_request.id, user_id, submitted_at) + push_reference(project, reviewer, :user_id, approved_event.approver_username) if reviewer log_info( import_stage: 'import_approved_event', @@ -125,6 +145,7 @@ def create_diff_note(merge_request, comment, position, discussion_id = nil) if note.valid? note.save + push_reference(project, note, :author_id, comment.author_username) return note end @@ -152,7 +173,9 @@ def create_fallback_diff_note(merge_request, comment, position) note += "*\n\n#{comment.note}" attributes[:note] = note - merge_request.notes.create!(attributes) + note = merge_request.notes.create!(attributes) + push_reference(project, note, :author_id, comment.author_username) + note end def build_position(merge_request, pr_comment) @@ -171,10 +194,12 @@ def import_standalone_pr_comments(pr_comments, merge_request) log_info(import_stage: 'import_standalone_pr_comments', message: 'starting', iid: merge_request.iid) pr_comments.each do |comment| - merge_request.notes.create!(pull_request_comment_attributes(comment)) + note = merge_request.notes.create!(pull_request_comment_attributes(comment)) + push_reference(project, note, :author_id, comment.author_username) comment.comments.each do |replies| - merge_request.notes.create!(pull_request_comment_attributes(replies)) + note = merge_request.notes.create!(pull_request_comment_attributes(replies)) + push_reference(project, note, :author_id, comment.author_username) end rescue StandardError => e Gitlab::ErrorTracking.log_exception( @@ -190,7 +215,7 @@ def import_standalone_pr_comments(pr_comments, merge_request) end def pull_request_comment_attributes(comment) - author = user_finder.uid(comment) + author = author(comment) note = '' unless author @@ -198,7 +223,7 @@ 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) + comment_note = if convert_mentions? mentions_converter.convert(comment.note) else comment.note @@ -222,6 +247,22 @@ def pull_request_comment_attributes(comment) } end + def convert_mentions? + Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) && + !user_mapping_enabled?(project) + end + + def author(comment) + if user_mapping_enabled?(project) + user_finder.uid( + username: comment.author_username, + display_name: comment.author_name + ) + else + user_finder.uid(comment) + end + end + def client BitbucketServer::Client.new(project.import_data.credentials) end diff --git a/lib/gitlab/bitbucket_server_import/project_creator.rb b/lib/gitlab/bitbucket_server_import/project_creator.rb index 2833b6fa2007f63333e5bb8a299aee80c8ae867d..5b4fcaa2c13637d3e36a9a26bd4c63a2cf419301 100644 --- a/lib/gitlab/bitbucket_server_import/project_creator.rb +++ b/lib/gitlab/bitbucket_server_import/project_creator.rb @@ -20,6 +20,10 @@ def execute bitbucket_server_notes_separate_worker_enabled = Feature.enabled?(:bitbucket_server_notes_separate_worker, current_user) + user_contribution_mapping_enabled = + Feature.enabled?(:importer_user_mapping, current_user) && + Feature.enabled?(:bitbucket_server_user_mapping, current_user) + ::Projects::CreateService.new( current_user, name: name, @@ -37,7 +41,8 @@ def execute project_key: project_key, repo_slug: repo_slug, timeout_strategy: timeout_strategy, - bitbucket_server_notes_separate_worker: bitbucket_server_notes_separate_worker_enabled + bitbucket_server_notes_separate_worker: bitbucket_server_notes_separate_worker_enabled, + user_contribution_mapping_enabled: user_contribution_mapping_enabled } }, skip_wiki: true diff --git a/lib/gitlab/bitbucket_server_import/user_finder.rb b/lib/gitlab/bitbucket_server_import/user_finder.rb index fec0af16013d2f7685c6996505bbc37240cf1f71..b3031d4811254626177c516d6ac3bf898fc9c0c9 100644 --- a/lib/gitlab/bitbucket_server_import/user_finder.rb +++ b/lib/gitlab/bitbucket_server_import/user_finder.rb @@ -22,9 +22,11 @@ def author_id(object) # 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 def uid(object) - # We want this to only match either username or email depending on the flag state. - # There should be no fall-through. - if Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) + # We want this to only match either placeholder, username, or email + # depending on the flag state. There should be no fall-through. + if user_mapping_enabled?(project) + source_user_for_author(object).mapped_user_id + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) find_user_id(by: :username, value: object.is_a?(Hash) ? object[:author_username] : object.author_username) else find_user_id(by: :email, value: object.is_a?(Hash) ? object[:author_email] : object.author_email) @@ -60,6 +62,26 @@ def cache def build_cache_key(by, value) format(CACHE_KEY, project_id: project.id, by: by, value: value) end + + def user_mapping_enabled?(project) + !!project.import_data.user_mapping_enabled? + end + + def source_user_for_author(user_data) + source_user_mapper.find_or_create_source_user( + source_user_identifier: user_data[:username], + source_name: user_data[:display_name], + source_username: user_data[:username] + ) + end + + def source_user_mapper + @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( + namespace: project.root_ancestor, + import_type: ::Import::SOURCE_BITBUCKET_SERVER, + source_hostname: project.import_url + ) + end end end end diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb index 778757ab390a05560d79cfdb0ef14dd62d7a5250..491178ccfa35bd81de4018114597ac7c287f6b7f 100644 --- a/lib/gitlab/import/merge_request_helpers.rb +++ b/lib/gitlab/import/merge_request_helpers.rb @@ -6,9 +6,11 @@ module MergeRequestHelpers include DatabaseHelpers # @param attributes [Hash] + # @return MergeRequest::Metrics def create_merge_request_metrics(attributes) metric = MergeRequest::Metrics.find_or_initialize_by(merge_request: merge_request) # rubocop: disable CodeReuse/ActiveRecord -- no need to move this to ActiveRecord model metric.update(attributes) + metric end # rubocop: disable CodeReuse/ActiveRecord diff --git a/lib/import/placeholder_references/pusher.rb b/lib/import/placeholder_references/pusher.rb new file mode 100644 index 0000000000000000000000000000000000000000..e1f428e5f188c58f4a51cefa63bb16b47928f3ca --- /dev/null +++ b/lib/import/placeholder_references/pusher.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module Import + module PlaceholderReferences + module Pusher + def push_reference(project, record, attribute, source_user_identifier) + return unless user_mapping_enabled?(project) + + source_user = source_user_mapper(project).find_source_user(source_user_identifier) + + # Do not create a reference if the object is already associated + # with a real user. + return if source_user_mapped_to_human?(record, attribute, source_user) + + ::Import::PlaceholderReferences::PushService.from_record( + import_source: ::Import::SOURCE_BITBUCKET_SERVER, + import_uid: project.import_state.id, + record: record, + source_user: source_user, + user_reference_column: attribute + ).execute + end + + def source_user_mapped_to_human?(record, attribute, source_user) + source_user.nil? || + (source_user.accepted_status? && record[attribute] == source_user.reassign_to_user_id) + end + + def source_user_mapper(project) + @user_mapper ||= ::Gitlab::Import::SourceUserMapper.new( + namespace: project.root_ancestor, + source_hostname: project.import_url, + import_type: ::Import::SOURCE_BITBUCKET_SERVER + ) + end + + def user_mapping_enabled?(project) + !!project.import_data.user_mapping_enabled? + end + end + end +end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index ca4ce5af91a158e7b8ba06a2dfbf64e9892fd0dc..f335fc45d37a65e4f88df9d7f71024e31bc5a2c6 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -205,6 +205,12 @@ import_status { :canceled } end + trait :bitbucket_server_import do + import_started + import_url { 'https://bitbucket.example.com' } + import_type { :bitbucket_server } + end + trait :jira_dvcs_server do before(:create) do |project| create(:project_feature_usage, :dvcs_server, project: project) 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 24b867c475cab90a23f659bda4f69c40d564e602..3dd55e9cfc082f1c16cc8cbcf3718d19f6e7c68d 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 @@ -4,22 +4,26 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestImporter, feature_category: :importers do include AfterNextHelpers + include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository) } - let_it_be(:reviewer_1) { create(:user, username: 'john_smith', email: 'john@smith.com') } - let_it_be(:reviewer_2) { create(:user, username: 'jane_doe', email: 'jane@doe.com') } + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) + end + + # Identifiers taken from importers/bitbucket_server/pull_request.json + let_it_be(:author_source_user) { generate_source_user(project, 'username') } + let_it_be(:reviewer_1_source_user) { generate_source_user(project, 'john_smith') } + let_it_be(:reviewer_2_source_user) { generate_source_user(project, 'jane_doe') } let(:pull_request_data) { Gitlab::Json.parse(fixture_file('importers/bitbucket_server/pull_request.json')) } let(:pull_request) { BitbucketServer::Representation::PullRequest.new(pull_request_data) } subject(:importer) { described_class.new(project, pull_request.to_hash) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do 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::Import::MentionsConverter, 'bitbucket_server', - project).to receive(:convert).and_call_original expect { importer.execute }.to change { MergeRequest.count }.by(1) @@ -30,14 +34,25 @@ title: pull_request.title, source_branch: 'root/CODE_OF_CONDUCTmd-1530600625006', target_branch: 'master', - reviewer_ids: match_array([reviewer_1.id, reviewer_2.id]), + reviewer_ids: an_array_matching([reviewer_1_source_user.mapped_user_id, reviewer_2_source_user.mapped_user_id]), state: pull_request.state, - author_id: project.creator_id, - description: "*Created by: #{pull_request.author}*\n\n#{pull_request.description}", + author_id: author_source_user.mapped_user_id, + description: pull_request.description, imported_from: 'bitbucket_server' ) end + it 'pushes placeholder references', :aggregate_failures do + importer.execute + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['MergeRequestReviewer', instance_of(Integer), 'user_id', reviewer_1_source_user.id], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', reviewer_2_source_user.id], + ['MergeRequest', instance_of(Integer), 'author_id', author_source_user.id] + ) + end + describe 'refs/merge-requests/:iid/head creation' do before do project.repository.create_branch(pull_request.source_branch_name, 'master') @@ -56,32 +71,6 @@ 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_next(Gitlab::Import::MentionsConverter, 'bitbucket_server', project).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) - end - - it 'imports reviewers correctly' do - importer.execute - - merge_request = project.merge_requests.find_by_iid(pull_request.iid) - - expect(merge_request.reviewer_ids).to match_array([reviewer_1.id, reviewer_2.id]) - end - end - describe 'merge request diff head_commit_sha' do before do allow(pull_request).to receive(:source_branch_sha).and_return(source_branch_sha) @@ -152,5 +141,73 @@ importer.execute end + + context 'when user contribution mapping is disabled' do + let_it_be(:reviewer_1) { create(:user, username: 'john_smith', email: 'john@smith.com') } + let_it_be(:reviewer_2) { create(:user, username: 'jane_doe', email: 'jane@doe.com') } + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end + + it 'annotates the description with the source username when no matching user is found' do + allow_next_instance_of(Gitlab::BitbucketServerImport::UserFinder) do |finder| + allow(finder).to receive(:uid).and_return(nil) + end + + importer.execute + + merge_request = project.merge_requests.find_by_iid(pull_request.iid) + + expect(merge_request).to have_attributes( + description: "*Created by: #{pull_request.author}*\n\n#{pull_request.description}" + ) + 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::Import::MentionsConverter, 'bitbucket_server', project).not_to receive(:convert) + + importer.execute + end + end + + context 'when alternate UCM flags are disabled' do + before do + stub_feature_flags( + bitbucket_server_user_mapping_by_username: false, + bitbucket_server_convert_mentions_to_users: false, + bitbucket_server_user_mapping: false + ) + end + + it 'assigns the MR author' do + importer.execute + + merge_request = project.merge_requests.find_by_iid(pull_request.iid) + + expect(merge_request.author_id).to eq(project.creator_id) + end + + it 'imports reviewers correctly' do + importer.execute + + merge_request = project.merge_requests.find_by_iid(pull_request.iid) + + expect(merge_request.reviewer_ids).to match_array([reviewer_1.id, reviewer_2.id]) + end + end + + it 'does not push placeholder references' do + importer.execute + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + end + end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event_spec.rb index c7d68f01449a98b6e84f6871bafb61e95bc6dad2..37e929c19623b6c80ebecc4a00ae8372589e8bf6 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/approved_event_spec.rb @@ -3,31 +3,26 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, feature_category: :importers do - let_it_be(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) + include Import::UserMappingHelper + + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) end let_it_be(:merge_request) { create(:merge_request, source_project: project) } let_it_be(:now) { Time.now.utc.change(usec: 0) } - - let!(:pull_request_author) do - create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') - end - - let(:approved_event) do + let_it_be(:approved_event) do { id: 4, - approver_username: pull_request_author.username, - approver_email: pull_request_author.email, + approver_name: 'John Approvals', + approver_username: 'pull_request_author', + approver_email: 'pull_request_author@example.org', created_at: now } end + let_it_be(:source_user) { generate_source_user(project, approved_event[:approver_username]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -37,6 +32,17 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute', :clean_gitlab_redis_shared_state do + it 'pushes placeholder references' do + importer.execute(approved_event) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['Approval', instance_of(Integer), 'user_id', source_user.id], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', source_user.id], + ['Note', instance_of(Integer), 'author_id', source_user.id] + ) + end + it 'creates the approval, reviewer and approval note' do expect { importer.execute(approved_event) } .to change { merge_request.approvals.count }.from(0).to(1) @@ -45,61 +51,55 @@ def expect_log(stage:, message:, iid:, event_id:) approval = merge_request.approvals.first - expect(approval.user).to eq(pull_request_author) + expect(approval.user_id).to eq(source_user.mapped_user_id) expect(approval.created_at).to eq(now) note = merge_request.notes.first expect(note.note).to eq('approved this merge request') - expect(note.author).to eq(pull_request_author) + expect(note.author_id).to eq(source_user.mapped_user_id) expect(note.system).to be_truthy expect(note.created_at).to eq(now) reviewer = merge_request.reviewers.first - expect(reviewer.id).to eq(pull_request_author.id) + expect(reviewer.id).to eq(source_user.mapped_user_id) end - context 'when a user with a matching username does not exist' do - let(:approved_event) { super().merge(approver_username: 'another_username') } + it 'logs its progress' do + expect_log(stage: 'import_approved_event', message: 'starting', iid: merge_request.iid, event_id: 4) + expect_log(stage: 'import_approved_event', message: 'finished', iid: merge_request.iid, event_id: 4) - it 'does not set an approver' do - expect_log( - stage: 'import_approved_event', - message: 'skipped due to missing user', - iid: merge_request.iid, - event_id: 4 - ) + importer.execute(approved_event) + end - expect { importer.execute(approved_event) } - .to not_change { merge_request.approvals.count } - .and not_change { merge_request.notes.count } - .and not_change { merge_request.reviewers.count } + context 'when user contribution mapping is disabled' do + let_it_be(:pull_request_author) do + create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') + end - expect(merge_request.approvals).to be_empty + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! end - context 'when bitbucket_server_user_mapping_by_username flag is disabled' do - before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) - end + it 'creates the approval, reviewer and approval note' do + expect { importer.execute(approved_event) } + .to change { merge_request.approvals.count }.from(0).to(1) + .and change { merge_request.notes.count }.from(0).to(1) + .and change { merge_request.reviewers.count }.from(0).to(1) - it 'finds the user based on email' do - importer.execute(approved_event) + approval = merge_request.approvals.first + expect(approval.user_id).to eq(pull_request_author.id) - approval = merge_request.approvals.first + note = merge_request.notes.first + expect(note.author_id).to eq(pull_request_author.id) - expect(approval.user).to eq(pull_request_author) - end + reviewer = merge_request.reviewers.first + expect(reviewer.id).to eq(pull_request_author.id) end - context 'when no users match email or username' do - let(:approved_event) do - super().merge( - approver_username: 'another_username', - approver_email: 'anotheremail@example.com' - ) - end + context 'when a user with a matching username does not exist' do + let(:approved_event) { super().merge(approver_username: 'another_username') } it 'does not set an approver' do expect_log( @@ -116,25 +116,53 @@ def expect_log(stage:, message:, iid:, event_id:) expect(merge_request.approvals).to be_empty end - end - end - context 'if the reviewer already existed' do - before do - merge_request.reviewers = [pull_request_author] - merge_request.save! - end + context 'when bitbucket_server_user_mapping_by_username flag is disabled' do + before do + stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + end + + it 'finds the user based on email' do + importer.execute(approved_event) + + approval = merge_request.approvals.first - it 'does not create the reviewer record' do - expect { importer.execute(approved_event) }.not_to change { merge_request.reviewers.count } + expect(approval.user).to eq(pull_request_author) + end + end + + context 'when no users match email or username' do + let(:approved_event) do + super().merge( + approver_username: 'another_username', + approver_email: 'anotheremail@example.com' + ) + end + + it 'does not set an approver' do + expect_log( + stage: 'import_approved_event', + message: 'skipped due to missing user', + iid: merge_request.iid, + event_id: 4 + ) + + expect { importer.execute(approved_event) } + .to not_change { merge_request.approvals.count } + .and not_change { merge_request.notes.count } + .and not_change { merge_request.reviewers.count } + + expect(merge_request.approvals).to be_empty + end + end end - end - it 'logs its progress' do - expect_log(stage: 'import_approved_event', message: 'starting', iid: merge_request.iid, event_id: 4) - expect_log(stage: 'import_approved_event', message: 'finished', iid: merge_request.iid, event_id: 4) + it 'does not push placeholder references' do + importer.execute(approved_event) - importer.execute(approved_event) + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + end end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event_spec.rb index e10e263cf20fb2c19edc8d1081d7427917e98007..4cc6f844e5830711ae4e545beac64f8e75dbef6b 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/declined_event_spec.rb @@ -3,31 +3,25 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, feature_category: :importers do - let_it_be(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) - end - - let_it_be(:merge_request) { create(:merge_request, source_project: project) } - let_it_be(:now) { Time.now.utc.change(usec: 0) } + include Import::UserMappingHelper - let!(:decliner_author) do - create(:user, username: 'decliner_author', email: 'decliner_author@example.org') + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) end - let(:declined_event) do + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:declined_event) do { id: 7, - decliner_username: decliner_author.username, - decliner_email: decliner_author.email, - created_at: now + decliner_name: 'John Rejections', + decliner_username: 'decliner_author', + decliner_email: 'decliner_author@example.org', + created_at: Time.now.utc.change(usec: 0) } end + let_it_be(:source_user) { generate_source_user(project, declined_event[:decliner_username]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -37,66 +31,92 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute', :clean_gitlab_redis_shared_state do + it 'pushes placeholder references' do + importer.execute(declined_event) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['Event', instance_of(Integer), 'author_id', source_user.id], + ['MergeRequest::Metrics', instance_of(Integer), 'latest_closed_by_id', source_user.id] + ) + end + it 'imports the declined event' do expect { importer.execute(declined_event) } .to change { merge_request.events.count }.from(0).to(1) .and change { merge_request.resource_state_events.count }.from(0).to(1) metrics = merge_request.metrics.reload - expect(metrics.latest_closed_by).to eq(decliner_author) + expect(metrics.latest_closed_by_id).to eq(source_user.mapped_user_id) expect(metrics.latest_closed_at).to eq(declined_event[:created_at]) event = merge_request.events.first + expect(event.author_id).to eq(source_user.mapped_user_id) expect(event.action).to eq('closed') resource_state_event = merge_request.resource_state_events.first expect(resource_state_event.state).to eq('closed') end - context 'when bitbucket_server_user_mapping_by_username flag is disabled' do + it 'logs its progress' do + expect_log(stage: 'import_declined_event', message: 'starting', iid: merge_request.iid, event_id: 7) + expect_log(stage: 'import_declined_event', message: 'finished', iid: merge_request.iid, event_id: 7) + + importer.execute(declined_event) + end + + context 'when user contribution mapping is disabled' do + let_it_be(:decliner_author) { create(:user, username: 'decliner_author', email: 'decliner_author@example.org') } + before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! end - context 'when a user with a matching username does not exist' do - let(:another_username_event) do - declined_event.merge(decliner_username: 'another_username') + context 'when bitbucket_server_user_mapping_by_username flag is disabled' do + before do + stub_feature_flags(bitbucket_server_user_mapping_by_username: false) end - it 'finds the user based on email' do - importer.execute(another_username_event) + context 'when a user with a matching username does not exist' do + let(:another_username_event) do + declined_event.merge(decliner_username: 'another_username') + end + + it 'finds the user based on email' do + importer.execute(another_username_event) - expect(merge_request.metrics.reload.latest_closed_by).to eq(decliner_author) + expect(merge_request.metrics.reload.latest_closed_by).to eq(decliner_author) + end end end - end - context 'when no users match email or username' do - let(:another_user_event) do - declined_event.merge(decliner_username: 'another_username', decliner_email: 'another_email@example.org') - end + context 'when no users match email or username' do + let(:another_user_event) do + declined_event.merge(decliner_username: 'another_username', decliner_email: 'another_email@example.org') + end - it 'does not set a decliner' do - expect_log( - stage: 'import_declined_event', - message: 'skipped due to missing user', - iid: merge_request.iid, - event_id: 7 - ) + it 'does not set a decliner' do + expect_log( + stage: 'import_declined_event', + message: 'skipped due to missing user', + iid: merge_request.iid, + event_id: 7 + ) - expect { importer.execute(another_user_event) } - .to not_change { merge_request.events.count } - .and not_change { merge_request.resource_state_events.count } + expect { importer.execute(another_user_event) } + .to not_change { merge_request.events.count } + .and not_change { merge_request.resource_state_events.count } - expect(merge_request.metrics.reload.latest_closed_by).to be_nil + expect(merge_request.metrics.reload.latest_closed_by).to be_nil + end end - end - it 'logs its progress' do - expect_log(stage: 'import_declined_event', message: 'starting', iid: merge_request.iid, event_id: 7) - expect_log(stage: 'import_declined_event', message: 'finished', iid: merge_request.iid, event_id: 7) + it 'does not push placeholder references' do + importer.execute(declined_event) - importer.execute(declined_event) + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + end end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/inline_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/inline_spec.rb index 52a09c5e3b327af5ff87add61cdb4eff4e5ae6a7..467dcc52f86bad5323df7f0ebd8d18a6d98cb3f8 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/inline_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/inline_spec.rb @@ -3,27 +3,19 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, feature_category: :importers do - let_it_be(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) + include Import::UserMappingHelper + + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) end let_it_be(:merge_request) { create(:merge_request, source_project: project) } let_it_be(:now) { Time.now.utc.change(usec: 0) } let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } - let_it_be(:reply_author) { create(:user, username: 'reply_author', email: 'reply_author@example.org') } - let_it_be(:inline_note_author) do - create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') - end - - let(:reply) do + let_it_be(:reply) do { - author_email: reply_author.email, - author_username: reply_author.username, + author_email: 'reply_author@example.org', + author_username: 'reply_author', note: 'I agree', created_at: now, updated_at: now, @@ -31,7 +23,7 @@ } end - let(:pr_inline_comment) do + let_it_be(:pr_inline_comment) do { id: 7, file_type: 'ADDED', @@ -41,8 +33,8 @@ old_pos: nil, new_pos: 4, note: 'Hello world', - author_email: inline_note_author.email, - author_username: inline_note_author.username, + author_email: 'inline_note_author@example.org', + author_username: 'inline_note_author', comments: [reply], created_at: now, updated_at: now, @@ -50,6 +42,9 @@ } end + let_it_be(:reply_source_user) { generate_source_user(project, reply[:author_username]) } + let_it_be(:note_source_user) { generate_source_user(project, pr_inline_comment[:author_username]) } + before do allow(Gitlab::Import::MentionsConverter).to receive(:new).and_return(mentions_converter) end @@ -62,7 +57,17 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do + it 'pushes placeholder references' do + importer.execute(pr_inline_comment) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['DiffNote', instance_of(Integer), 'author_id', note_source_user.id], + ['DiffNote', instance_of(Integer), 'author_id', reply_source_user.id] + ) + end + it 'imports the threaded discussion' do expect(mentions_converter).to receive(:convert).and_call_original.twice @@ -78,11 +83,11 @@ def expect_log(stage:, message:, iid:, comment_id:) expect(start_note.updated_at).to eq(pr_inline_comment[:updated_at]) expect(start_note.position.old_line).to be_nil expect(start_note.position.new_line).to eq(pr_inline_comment[:new_pos]) - expect(start_note.author).to eq(inline_note_author) + expect(start_note.author_id).to eq(note_source_user.mapped_user_id) reply_note = notes.last expect(reply_note.note).to eq(reply[:note]) - expect(reply_note.author).to eq(reply_author) + expect(reply_note.author_id).to eq(reply_source_user.mapped_user_id) expect(reply_note.created_at).to eq(reply[:created_at]) expect(reply_note.updated_at).to eq(reply[:created_at]) expect(reply_note.position.old_line).to be_nil @@ -98,22 +103,11 @@ def expect_log(stage:, message:, iid:, comment_id:) context 'when note is invalid' do let(:invalid_comment) do - { - id: 7, - file_type: 'ADDED', - from_sha: 'c5f4288162e2e6218180779c7f6ac1735bb56eab', - to_sha: 'a4c2164330f2549f67c13f36a93884cf66e976be', - file_path: '.gitmodules', + pr_inline_comment.merge( old_pos: 3, - new_pos: 4, note: '', - author_email: inline_note_author.email, - author_username: inline_note_author.username, - comments: [], - created_at: now, - updated_at: now, - parent_comment_note: nil - } + comments: [] + ) end it 'fallback to basic note' do @@ -143,13 +137,46 @@ def expect_log(stage:, message:, iid:, comment_id:) end end - context 'when converting mention is failed' do - it 'logs its exception' do - expect(mentions_converter).to receive(:convert).and_raise(StandardError) - expect(Gitlab::ErrorTracking).to receive(:log_exception) - .with(StandardError, include(import_stage: 'create_diff_note')) + context 'when user contribution mapping is disabled' do + let_it_be(:reply_author) { create(:user, username: 'reply_author', email: 'reply_author@example.org') } + let_it_be(:inline_note_author) do + create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end + + it 'imports the threaded discussion' do + expect(mentions_converter).to receive(:convert).and_call_original.twice + + expect { importer.execute(pr_inline_comment) }.to change { Note.count }.by(2) + + expect(merge_request.discussions.count).to eq(1) + + notes = merge_request.notes.order(:id).to_a + start_note = notes.first + expect(start_note.author_id).to eq(inline_note_author.id) + reply_note = notes.last + expect(reply_note.author_id).to eq(reply_author.id) + end + + context 'when converting mention is failed' do + it 'logs its exception' do + expect(mentions_converter).to receive(:convert).and_raise(StandardError) + expect(Gitlab::ErrorTracking).to receive(:log_exception) + .with(StandardError, include(import_stage: 'create_diff_note')) + + importer.execute(pr_inline_comment) + end + end + + it 'does not push placeholder references' do importer.execute(pr_inline_comment) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event_spec.rb index d9fc56062bab8d90b893e73bcf85d6b8551fc672..ef3bc4146a24e74d93de438af3fd157819b6bba3 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/merge_event_spec.rb @@ -3,31 +3,24 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, feature_category: :importers do - let_it_be(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) - end + include Import::UserMappingHelper + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } let_it_be(:merge_request) { create(:merge_request, source_project: project) } let_it_be(:now) { Time.now.utc.change(usec: 0) } - - let_it_be(:pull_request_author) do - create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') - end - let_it_be(:merge_event) do { id: 3, - committer_email: pull_request_author.email, + committer_user: 'John Merges', + committer_username: 'pull_request_author', + committer_email: 'pull_request_author@example.org', merge_timestamp: now, merge_commit: '12345678' } end + let_it_be(:source_user) { generate_source_user(project, merge_event[:committer_username]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -36,14 +29,23 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do + it 'pushes placeholder references' do + importer.execute(merge_event) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['MergeRequest::Metrics', instance_of(Integer), 'merged_by_id', source_user.id] + ) + end + it 'imports the merge event' do importer.execute(merge_event) - merge_request.reload + metrics = merge_request.metrics.reload - expect(merge_request.metrics.merged_by).to eq(pull_request_author) - expect(merge_request.metrics.merged_at).to eq(merge_event[:merge_timestamp]) + expect(metrics.merged_by_id).to eq(source_user.mapped_user_id) + expect(metrics.merged_at).to eq(merge_event[:merge_timestamp]) expect(merge_request.merge_commit_sha).to eq(merge_event[:merge_commit]) end @@ -53,5 +55,29 @@ def expect_log(stage:, message:, iid:, event_id:) importer.execute(merge_event) end + + context 'when user contribution mapping is disabled' do + let_it_be(:pull_request_author) do + create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end + + it 'imports the merge event' do + importer.execute(merge_event) + + metrics = merge_request.metrics.reload + expect(metrics.merged_by_id).to eq(pull_request_author.id) + end + + it 'does not push placeholder references' do + importer.execute(merge_event) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + end + end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes_spec.rb index 94a901f2d3b7508023492bf7cbaab0bd841e83d2..01fd4843f038863d6f189a3b0185ffa6ff717860 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/standalone_notes_spec.rb @@ -3,33 +3,33 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, feature_category: :importers do - let_it_be(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) - end + include Import::UserMappingHelper + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } let_it_be(:merge_request) { create(:merge_request, source_project: project) } let_it_be(:now) { Time.now.utc.change(usec: 0) } - let_it_be(:note_author) { create(:user, username: 'note_author', email: 'note_author@example.org') } let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } + let_it_be(:author_details) do + { + author_name: 'John Notes', + author_username: 'note_author', + author_email: 'note_author@example.org' + } + end - let(:pr_comment) do + let_it_be(:pr_comment) do { id: 5, note: 'Hello world', - author_email: note_author.email, - author_username: note_author.username, comments: [], created_at: now, updated_at: now, parent_comment_note: nil - } + }.merge(author_details) end + let_it_be(:source_user) { generate_source_user(project, pr_comment[:author_username]) } + before do allow(Gitlab::Import::MentionsConverter).to receive(:new).and_return(mentions_converter) end @@ -42,7 +42,16 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do + it 'pushes placeholder reference' do + importer.execute(pr_comment) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to contain_exactly( + ['Note', instance_of(Integer), 'author_id', source_user.id] + ) + end + it 'imports the stand alone comments' do expect(mentions_converter).to receive(:convert).and_call_original @@ -51,7 +60,7 @@ def expect_log(stage:, message:, iid:, comment_id:) expect(merge_request.notes.count).to eq(1) expect(merge_request.notes.first).to have_attributes( note: end_with(pr_comment[:note]), - author: note_author, + author_id: source_user.mapped_user_id, created_at: pr_comment[:created_at], updated_at: pr_comment[:created_at], imported_from: 'bitbucket_server' @@ -63,28 +72,24 @@ def expect_log(stage:, message:, iid:, comment_id:) { id: 6, note: 'Foo bar', - author_email: note_author.email, - author_username: note_author.username, comments: [], created_at: now, updated_at: now, parent_comment_note: nil, imported_from: 'bitbucket_server' - } + }.merge(author_details) end let(:pr_comment) do { id: 5, note: 'Hello world', - author_email: note_author.email, - author_username: note_author.username, comments: [pr_comment_extra], created_at: now, updated_at: now, parent_comment_note: nil, imported_from: 'bitbucket_server' - } + }.merge(author_details) end it 'imports multiple comments' do @@ -95,14 +100,14 @@ def expect_log(stage:, message:, iid:, comment_id:) expect(merge_request.notes.count).to eq(2) expect(merge_request.notes.first).to have_attributes( note: end_with(pr_comment[:note]), - author: note_author, + author_id: source_user.mapped_user_id, created_at: pr_comment[:created_at], updated_at: pr_comment[:created_at], imported_from: 'bitbucket_server' ) expect(merge_request.notes.last).to have_attributes( note: end_with(pr_comment_extra[:note]), - author: note_author, + author_id: source_user.mapped_user_id, created_at: pr_comment_extra[:created_at], updated_at: pr_comment_extra[:created_at], imported_from: 'bitbucket_server' @@ -110,33 +115,17 @@ def expect_log(stage:, message:, iid:, comment_id:) end 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 - importer.execute(pr_comment) - - 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_comment) do { id: 5, note: 'Note', - author_email: note_author.email, - author_username: note_author.username, comments: [], created_at: now, updated_at: now, parent_comment_note: 'Parent note', imported_from: 'bitbucket_server' - } + }.merge(author_details) end it 'adds the parent note before the actual note' do @@ -176,5 +165,41 @@ def expect_log(stage:, message:, iid:, comment_id:) importer.execute(pr_comment) end end + + context 'when user contribution mapping is disabled' do + let_it_be(:note_author) { create(:user, username: 'note_author', email: 'note_author@example.org') } + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end + + it 'imports the merge event' do + expect { importer.execute(pr_comment) }.to change { Note.count }.by(1) + expect(merge_request.notes.first).to have_attributes( + author_id: note_author.id + ) + end + + it 'does not push placeholder references' do + importer.execute(pr_comment) + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + 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 + importer.execute(pr_comment) + + expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") + end + end + end end end 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 ee1b53b566fa869a9b6ed4acbdff351a8e70f5e6..a97f42e121e70ebc8f73bc78b0dd16b94568d790 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 @@ -4,25 +4,17 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotesImporter, feature_category: :importers do include AfterNextHelpers + include Import::UserMappingHelper let_it_be_with_reload(:project) do - create(:project, :repository, :import_started, - import_data_attributes: { - data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, - credentials: { 'token' => 'token' } - } - ) + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) end 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_it_be_with_reload(:merge_request) { create(:merge_request, iid: pull_request.iid, source_project: project) } let(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } - let!(:pull_request_author) do - create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') - end - let(:merge_event) do instance_double( BitbucketServer::Representation::Activity, @@ -30,7 +22,9 @@ comment?: false, merge_event?: true, approved_event?: false, - committer_email: pull_request_author.email, + committer_name: 'Pull Request Author', + committer_username: 'pull_request_author', + committer_email: 'pull_request_author@example.com', merge_timestamp: now, merge_commit: '12345678' ) @@ -43,8 +37,9 @@ comment?: false, merge_event?: false, approved_event?: true, - approver_username: pull_request_author.username, - approver_email: pull_request_author.email, + approver_name: 'Pull Request Author', + approver_username: 'pull_request_author', + approver_email: 'pull_request_author@example.org', created_at: now ) end @@ -52,9 +47,24 @@ let(:pr_note) do instance_double( BitbucketServer::Representation::Comment, + id: 456, note: 'Hello world', - author_email: note_author.email, - author_username: note_author.username, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', + comments: [pr_note_reply], + created_at: now, + updated_at: now, + parent_comment: nil) + end + + let(:pr_note_reply) do + instance_double( + BitbucketServer::Representation::Comment, + note: 'Yes, absolutely.', + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -71,9 +81,16 @@ comment: pr_note) end + let!(:author_source_user) { generate_source_user(project, merge_event.committer_username) } + let!(:note_source_user) { generate_source_user(project, pr_note.author_username) } + let_it_be(:sample) { RepoHelpers.sample_compare } let_it_be(:now) { Time.now.utc.change(usec: 0) } + let(:cached_references) do + placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + end + def expect_log(stage:, message:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -86,8 +103,12 @@ def expect_log(stage:, message:) subject(:importer) { described_class.new(project.reload, pull_request.to_hash) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do context 'when a matching merge request is not found' do + before do + merge_request.update!(iid: merge_request.iid + 1) + end + it 'does nothing' do expect { importer.execute }.not_to change { Note.count } end @@ -100,9 +121,7 @@ def expect_log(stage:, message:) end end - context 'when a matching merge request is found', :clean_gitlab_redis_shared_state do - let_it_be(:merge_request) { create(:merge_request, iid: pull_request.iid, source_project: project) } - + context 'when a matching merge request is found' do it 'logs its progress' do allow_next(BitbucketServer::Client).to receive(:activities).and_return([]) @@ -117,33 +136,35 @@ def expect_log(stage:, message:) allow_next(BitbucketServer::Client).to receive(:activities).and_return([pr_comment]) end + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['Note', instance_of(Integer), 'author_id', note_source_user.id], + ["Note", instance_of(Integer), "author_id", note_source_user.id] + ) + end + it 'imports the stand alone comments' do - expect(mentions_converter).to receive(:convert).and_call_original + expect { importer.execute }.to change { Note.count }.by(2) - expect { subject.execute }.to change { Note.count }.by(1) + notes = merge_request.notes.order(:id) - expect(merge_request.notes.count).to eq(1) - expect(merge_request.notes.first).to have_attributes( + expect(notes.first).to have_attributes( note: end_with(pr_note.note), - author: note_author, + author_id: note_source_user.mapped_user_id, created_at: pr_note.created_at, updated_at: pr_note.created_at, imported_from: 'bitbucket_server' ) - 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 + expect(notes.last).to have_attributes( + note: end_with(pr_note_reply.note), + author_id: note_source_user.mapped_user_id, + created_at: pr_note_reply.created_at, + updated_at: pr_note_reply.created_at, + imported_from: 'bitbucket_server' + ) end context 'when the note has a parent note' do @@ -151,8 +172,9 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Note', - author_email: note_author.email, - author_username: note_author.username, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -164,8 +186,9 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Parent note', - author_email: note_author.email, - author_username: note_author.username, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -174,12 +197,32 @@ def expect_log(stage:, message:) end it 'adds the parent note before the actual note' do - subject.execute + importer.execute expect(Note.first.note).to include("> #{pr_parent_note.note}\n\n") end end + context 'when an exception is raised during comment creation' do + before do + allow(importer).to receive(:pull_request_comment_attributes).and_raise(exception) + end + + let(:exception) { StandardError.new('something went wrong') } + + it 'logs the error' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + exception, + import_stage: 'import_standalone_pr_comments', + comment_id: pr_note.id, + error: exception.message, + merge_request_id: merge_request.id + ) + + importer.execute + 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) @@ -188,7 +231,7 @@ def expect_log(stage:, message:) it 'does not convert mentions' do expect(mentions_converter).not_to receive(:convert) - subject.execute + importer.execute end end @@ -200,17 +243,13 @@ def expect_log(stage:, message:) end end - context 'when PR has threaded discussion' do - let_it_be(:reply_author) { create(:user, username: 'reply_author', email: 'reply_author@example.org') } - let_it_be(:inline_note_author) do - create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') - end - + context 'when PR has threaded inline discussion' do let(:reply) do instance_double( BitbucketServer::Representation::PullRequestComment, - author_email: reply_author.email, - author_username: reply_author.username, + author_name: 'Reply Author', + author_email: 'reply_author@example.org', + author_username: 'reply_author', note: 'I agree', created_at: now, updated_at: now, @@ -220,6 +259,7 @@ def expect_log(stage:, message:) let(:pr_inline_note) do instance_double( BitbucketServer::Representation::PullRequestComment, + id: 123, file_type: 'ADDED', from_sha: pull_request.target_branch_sha, to_sha: pull_request.source_branch_sha, @@ -227,8 +267,9 @@ def expect_log(stage:, message:) old_pos: nil, new_pos: 4, note: 'Hello world', - author_email: inline_note_author.email, - author_username: inline_note_author.username, + author_name: 'Inline Note Author', + author_email: 'inline_note_author@example.org', + author_username: 'inline_note_author', comments: [reply], created_at: now, updated_at: now, @@ -244,14 +285,15 @@ def expect_log(stage:, message:) comment: pr_inline_note) end + let_it_be(:reply_source_user) { generate_source_user(project, 'reply_author') } + let_it_be(:note_source_user) { generate_source_user(project, 'inline_note_author') } + before do allow_next(BitbucketServer::Client).to receive(:activities).and_return([pr_inline_comment]) 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 { importer.execute }.to change { Note.count }.by(2) expect(merge_request.discussions.count).to eq(1) @@ -263,12 +305,12 @@ def expect_log(stage:, message:) expect(start_note.updated_at).to eq(pr_inline_note.updated_at) expect(start_note.position.old_line).to be_nil expect(start_note.position.new_line).to eq(pr_inline_note.new_pos) - expect(start_note.author).to eq(inline_note_author) + expect(start_note.author_id).to eq(note_source_user.mapped_user_id) expect(start_note.imported_from).to eq('bitbucket_server') reply_note = notes.last expect(reply_note.note).to eq(reply.note) - expect(reply_note.author).to eq(reply_author) + expect(reply_note.author_id).to eq(reply_source_user.mapped_user_id) expect(reply_note.created_at).to eq(reply.created_at) expect(reply_note.updated_at).to eq(reply.created_at) expect(reply_note.position.old_line).to be_nil @@ -276,6 +318,64 @@ def expect_log(stage:, message:) expect(reply_note.imported_from).to eq('bitbucket_server') end + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['DiffNote', instance_of(Integer), 'author_id', reply_source_user.id], + ['DiffNote', instance_of(Integer), 'author_id', note_source_user.id] + ) + end + + context 'when a diff note is invalid' do + let(:pr_inline_note) do + instance_double( + BitbucketServer::Representation::PullRequestComment, + file_type: 'ADDED', + from_sha: pull_request.target_branch_sha, + to_sha: pull_request.source_branch_sha, + file_path: '.gitmodules', + old_pos: 3, + new_pos: nil, + note: 'Hello world', + author_name: 'Inline Note Author', + author_email: 'inline_note_author@example.org', + author_username: 'inline_note_author', + comments: [], + created_at: now, + updated_at: now, + parent_comment: nil) + end + + it 'creates a fallback diff note' do + importer.execute + + notes = merge_request.notes.order(:id).to_a + note = notes.first + + expect(note.note).to eq("*Comment on .gitmodules:3 -->*\n\nHello world") + end + end + + context 'when an exception is raised during DiffNote creation' do + before do + allow(importer).to receive(:pull_request_comment_attributes).and_raise(exception) + end + + let(:exception) { StandardError.new('something went wrong') } + + it 'logs the error' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + exception, + import_stage: 'create_diff_note', + comment_id: 123, + error: exception.message + ) + + importer.execute + 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) @@ -284,7 +384,7 @@ def expect_log(stage:, message:) it 'does not convert mentions' do expect(mentions_converter).not_to receive(:convert) - subject.execute + importer.execute end end @@ -306,10 +406,18 @@ def expect_log(stage:, message:) merge_request.reload - expect(merge_request.metrics.merged_by).to eq(pull_request_author) + expect(merge_request.metrics.merged_by_id).to eq(author_source_user.mapped_user_id) expect(merge_request.metrics.merged_at).to eq(merge_event.merge_timestamp) expect(merge_request.merge_commit_sha).to eq(merge_event.merge_commit) end + + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ["MergeRequest::Metrics", instance_of(Integer), "merged_by_id", author_source_user.id] + ) + end end context 'when PR has an approved event' do @@ -325,70 +433,34 @@ def expect_log(stage:, message:) approval = merge_request.approvals.first - expect(approval.user).to eq(pull_request_author) + expect(approval.user_id).to eq(author_source_user.mapped_user_id) expect(approval.created_at).to eq(now) note = merge_request.notes.first expect(note.note).to eq('approved this merge request') - expect(note.author).to eq(pull_request_author) + expect(note.author_id).to eq(author_source_user.mapped_user_id) expect(note.system).to be_truthy expect(note.created_at).to eq(now) reviewer = merge_request.reviewers.first - expect(reviewer.id).to eq(pull_request_author.id) + expect(reviewer.id).to eq(author_source_user.mapped_user_id) end - context 'when a user with a matching username does not exist' do - before do - pull_request_author.update!(username: 'another_username') - end - - it 'does not set an approver' do - expect { importer.execute } - .to not_change { merge_request.approvals.count } - .and not_change { merge_request.notes.count } - .and not_change { merge_request.reviewers.count } - - expect(merge_request.approvals).to be_empty - end - - context 'when bitbucket_server_user_mapping_by_username flag is disabled' do - before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) - end - - it 'finds the user based on email' do - importer.execute - - approval = merge_request.approvals.first - - expect(approval.user).to eq(pull_request_author) - end - end - - context 'when no users match email or username' do - let_it_be(:another_author) { create(:user) } - - before do - pull_request_author.destroy! - end - - it 'does not set an approver' do - expect { importer.execute } - .to not_change { merge_request.approvals.count } - .and not_change { merge_request.notes.count } - .and not_change { merge_request.reviewers.count } + it 'pushes placeholder references' do + importer.execute - expect(merge_request.approvals).to be_empty - end - end + expect(cached_references).to contain_exactly( + ['Approval', instance_of(Integer), 'user_id', author_source_user.id], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', author_source_user.id], + ['Note', instance_of(Integer), 'author_id', author_source_user.id] + ) end - context 'if the reviewer already existed' do + context 'if the reviewer is already assigned to the MR' do before do - merge_request.reviewers = [pull_request_author] + merge_request.reviewers = [author_source_user.mapped_user] merge_request.save! end @@ -417,21 +489,165 @@ def expect_log(stage:, message:) end context 'when the import data does not have credentials' do - before do - project.import_data.credentials = nil - project.import_data.save! + let_it_be(:project) do + create(:project, :repository, :bitbucket_server_import, + import_data_attributes: { + data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, + credentials: nil + } + ) end include_examples 'import is skipped' end context 'when the import data does not have data' do - before do - project.import_data.data = nil - project.import_data.save! + let_it_be(:project) do + create(:project, :repository, :bitbucket_server_import, + import_data_attributes: { + data: nil, + credentials: { 'token' => 'token' } + } + ) end include_examples 'import is skipped' end + + context 'when user contribution mapping is disabled' do + let!(:note_author) { create(:user, username: 'note_author', email: 'note_author@example.org') } + let!(:pull_request_author) do + create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + allow_next(BitbucketServer::Client).to receive(:activities).and_return([approved_event]) + end + + it 'does not push placeholder references' do + importer.execute + + cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + expect(cached_references).to be_empty + end + + context 'when the author is not found' do + before do + allow_next(BitbucketServer::Client).to receive(:activities).and_return([pr_comment]) + + 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 + importer.execute + + expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") + end + end + + context 'when bitbucket_server_user_mapping_by_username flag is disabled' do + before do + stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + end + + context 'when a user with a matching username does not exist' do + before do + pull_request_author.update!(username: 'another_username') + end + + it 'finds the user based on email' do + importer.execute + + approval = merge_request.approvals.first + + expect(approval.user).to eq(pull_request_author) + end + + context 'when no users match email or username' do + let_it_be(:another_author) { create(:user) } + + before do + pull_request_author.destroy! + end + + it 'does not set an approver' do + expect { importer.execute } + .to not_change { merge_request.approvals.count } + .and not_change { merge_request.notes.count } + .and not_change { merge_request.reviewers.count } + + expect(merge_request.approvals).to be_empty + end + end + end + + context 'when importing merge events' do + before do + allow_next(BitbucketServer::Client).to receive(:activities).and_return([merge_event]) + end + + it 'attributes the merge event to the project creator' do + importer.execute + + expect(merge_request.metrics.merged_by_id).to eq(project.creator_id) + end + end + + context 'when PR has threaded discussion' do + let(:reply) do + instance_double( + BitbucketServer::Representation::PullRequestComment, + author_name: 'Reply Author', + author_email: 'reply_author@example.org', + author_username: 'reply_author', + note: 'I agree', + created_at: now, + updated_at: now, + parent_comment: nil) + end + + let(:pr_inline_note) do + instance_double( + BitbucketServer::Representation::PullRequestComment, + file_type: 'ADDED', + from_sha: pull_request.target_branch_sha, + to_sha: pull_request.source_branch_sha, + file_path: '.gitmodules', + old_pos: nil, + new_pos: 4, + note: 'Hello world', + author_name: 'Inline Note Author', + author_email: 'inline_note_author@example.org', + author_username: 'inline_note_author', + comments: [reply], + created_at: now, + updated_at: now, + parent_comment: nil) + end + + let(:pr_inline_comment) do + instance_double( + BitbucketServer::Representation::Activity, + comment?: true, + inline_comment?: true, + merge_event?: false, + comment: pr_inline_note) + end + + before do + allow_next(BitbucketServer::Client).to receive(:activities).and_return([pr_inline_comment]) + end + + it 'attributes the comments to the project creator' do + importer.execute + + expect(merge_request.notes.collect(&:author_id)).to match_array([project.creator_id, project.creator_id]) + end + end + end + end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/project_creator_spec.rb b/spec/lib/gitlab/bitbucket_server_import/project_creator_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9ea6a4719ee16a7e2b2d740bf83dba6aeda1b0bc --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/project_creator_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::ProjectCreator, feature_category: :importers do + let(:project_key) { 'TEST' } + let(:repo_slug) { 'my-repo' } + let(:name) { 'Test Project' } + let(:namespace) { create(:group) } + let(:current_user) { create(:user) } + let(:session_data) { { 'token' => 'abc123' } } + let(:timeout_strategy) { 'default' } + + let(:repo_data) do + { + 'description' => 'Test repo', + 'project' => { + 'public' => true + }, + 'links' => { + 'self' => [ + { + 'href' => 'http://localhost/brows', + 'name' => 'http' + } + ], + 'clone' => [ + { + 'href' => 'http://localhost/clone', + 'name' => 'http' + } + ] + } + } + end + + let(:repo) do + BitbucketServer::Representation::Repo.new(repo_data) + end + + subject(:creator) do + described_class.new( + project_key, + repo_slug, + repo, + name, + namespace, + current_user, + session_data, + timeout_strategy + ) + end + + describe '#execute' do + let_it_be(:project) { create(:project) } + let(:service) { instance_double(Projects::CreateService) } + + before do + allow(Projects::CreateService).to receive(:new).and_return(service) + allow(service).to receive(:execute).and_return(project) + end + + it 'passes the arguments to Project::CreateService' do + expected_params = { + name: name, + path: name, + description: repo.description, + namespace_id: namespace.id, + organization_id: namespace.organization_id, + visibility_level: repo.visibility_level, + import_type: 'bitbucket_server', + import_source: repo.browse_url, + import_url: repo.clone_url, + import_data: { + credentials: session_data, + data: { + project_key: project_key, + repo_slug: repo_slug, + timeout_strategy: timeout_strategy, + bitbucket_server_notes_separate_worker: true, + user_contribution_mapping_enabled: true + } + }, + skip_wiki: true + } + + expect(Projects::CreateService).to receive(:new) + .with(current_user, expected_params) + + creator.execute + end + + context 'when feature flags are disabled' do + before do + stub_feature_flags(bitbucket_server_notes_separate_worker: false) + stub_feature_flags(importer_user_mapping: false) + stub_feature_flags(bitbucket_server_user_mapping: false) + end + + it 'disables these options in the import_data' do + expected_params = { + import_data: { + credentials: session_data, + data: { + project_key: project_key, + repo_slug: repo_slug, + timeout_strategy: timeout_strategy, + bitbucket_server_notes_separate_worker: false, + user_contribution_mapping_enabled: false + } + } + } + + expect(Projects::CreateService).to receive(:new) + .with(current_user, a_hash_including(expected_params)) + + creator.execute + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb index 78d258bd4445b5c22e0d1dac43f39742a00feabb..63f0e26f8e40ef148ee4dabf91e7fec52765e823 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb @@ -5,69 +5,122 @@ RSpec.describe Gitlab::BitbucketServerImport::UserFinder, :clean_gitlab_redis_shared_state, feature_category: :importers do let_it_be(:user) { create(:user) } - let(:created_id) { 1 } - let(:project) { build_stubbed(:project, creator_id: created_id, id: 1) } + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) + end + + let(:source_user) { build_stubbed(:import_source_user, :completed) } + let(:user_representation) do + { + username: user.username, + display_name: user.name + } + end subject(:user_finder) { described_class.new(project) } describe '#author_id' do - it 'calls uid method' do - object = { author_username: user.username } - - expect(user_finder).to receive(:uid).with(object).and_return(10) - expect(user_finder.author_id(object)).to eq(10) + before do + allow_next_instance_of(Gitlab::Import::SourceUserMapper) do |isum| + allow(isum).to receive(:find_or_create_source_user).and_return(source_user) + end end - context 'when corresponding user does not exist' do - it 'fallsback to project creator_id' do - object = { author_email: 'unknown' } + it 'returns the mapped user' do + expect( + user_finder.author_id(user_representation) + ).to eq(source_user.mapped_user.id) + end + end - expect(user_finder.author_id(object)).to eq(created_id) + describe '#uid' do + before do + allow_next_instance_of(Gitlab::Import::SourceUserMapper) do |isum| + allow(isum).to receive(:find_or_create_source_user).and_return(source_user) end end + + it 'takes a user data hash and finds the mapped user ID' do + user_id = user_finder.uid(user_representation) + + expect(user_id).to eq(source_user.mapped_user.id) + end end - describe '#uid' do - context 'when provided object is a Hash' do - it 'maps to an existing user with the same username' do - object = { author_username: user.username } + context 'when user contribution mapping is disabled' do + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end + + describe '#find_user_id' do + context 'when user cannot be found' do + it 'caches and returns nil' do + expect(User).to receive(:find_by_any_email).once.and_call_original + + 2.times do + user_id = user_finder.find_user_id(by: :email, value: 'nobody@example.com') - expect(user_finder.uid(object)).to eq(user.id) + expect(user_id).to be_nil + end + end end - end - context 'when provided object is a representation Object' do - it 'maps to a existing user with the same username' do - object = instance_double(BitbucketServer::Representation::Comment, author_username: user.username) + context 'when user can be found' do + it 'caches and returns the user ID by email' do + expect(User).to receive(:find_by_any_email).once.and_call_original + + 2.times do + user_id = user_finder.find_user_id(by: :email, value: user.email) + + expect(user_id).to eq(user.id) + end + end + + it 'caches and returns the user ID by username' do + expect(User).to receive(:find_by_username).once.and_call_original - expect(user_finder.uid(object)).to eq(user.id) + 2.times do + user_id = user_finder.find_user_id(by: :username, value: user.username) + + expect(user_id).to eq(user.id) + end + end end end - context 'when corresponding user does not exist' do - it 'returns nil' do - object = { author_username: 'unknown' } + describe '#author_id' do + it 'calls uid method' do + object = { author_username: user.username } - expect(user_finder.uid(object)).to eq(nil) + expect(user_finder).to receive(:uid).with(object).and_return(10) + expect(user_finder.author_id(object)).to eq(10) end - end - context 'when bitbucket_server_user_mapping_by_username is disabled' do - before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + context 'when corresponding user does not exist' do + before do + project.update!(creator_id: 123) + end + + it 'falls back to project creator_id' do + object = { author_email: 'unknown' } + + expect(user_finder.author_id(object)).to eq(123) + end end + end + describe '#uid' do context 'when provided object is a Hash' do - it 'maps to an existing user with the same email' do - object = { author_email: user.email } + it 'maps to an existing user with the same username' do + object = { author_username: user.username } expect(user_finder.uid(object)).to eq(user.id) end end - context 'when provided object is a representation Object' do - it 'maps to an existing user with the same email' do - object = instance_double(BitbucketServer::Representation::Comment, author_email: user.email) + context 'when provided object is a representation object' do + it 'maps to a existing user with the same username' do + object = instance_double(BitbucketServer::Representation::Comment, author_username: user.username) expect(user_finder.uid(object)).to eq(user.id) end @@ -75,45 +128,39 @@ context 'when corresponding user does not exist' do it 'returns nil' do - object = { author_email: 'unknown' } + object = { author_username: 'unknown' } - expect(user_finder.uid(object)).to eq(nil) + expect(user_finder.uid(object)).to be_nil end end end - end - describe '#find_user_id' do - context 'when user cannot be found' do - it 'caches and returns nil' do - expect(User).to receive(:find_by_any_email).once.and_call_original + context 'when bitbucket_server_user_mapping_by_username is disabled' do + before do + stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + end - 2.times do - user_id = user_finder.find_user_id(by: :email, value: 'nobody@example.com') + context 'when provided object is a Hash' do + it 'maps to an existing user with the same email' do + object = { author_email: user.email } - expect(user_id).to be_nil + expect(user_finder.uid(object)).to eq(user.id) end end - end - context 'when user can be found' do - it 'caches and returns the user ID by email' do - expect(User).to receive(:find_by_any_email).once.and_call_original - - 2.times do - user_id = user_finder.find_user_id(by: :email, value: user.email) + context 'when provided object is a representation Object' do + it 'maps to an existing user with the same email' do + object = instance_double(BitbucketServer::Representation::Comment, author_email: user.email) - expect(user_id).to eq(user.id) + expect(user_finder.uid(object)).to eq(user.id) end end - it 'caches and returns the user ID by username' do - expect(User).to receive(:find_by_username).once.and_call_original - - 2.times do - user_id = user_finder.find_user_id(by: :username, value: user.username) + context 'when corresponding user does not exist' do + it 'returns nil' do + object = { author_email: 'unknown' } - expect(user_id).to eq(user.id) + expect(user_finder.uid(object)).to be_nil end end end diff --git a/spec/lib/gitlab/import/merge_request_helpers_spec.rb b/spec/lib/gitlab/import/merge_request_helpers_spec.rb index d38eca9d52a73ce1442ec8104289cde84d97b2d4..75f8085735c61da867c9c8eb51104d432da51450 100644 --- a/spec/lib/gitlab/import/merge_request_helpers_spec.rb +++ b/spec/lib/gitlab/import/merge_request_helpers_spec.rb @@ -119,4 +119,35 @@ expect(note.system_note_metadata).to have_attributes(action: 'approved') end end + + describe '.create_merge_request_metrics' do + let(:attributes) do + { + merged_by_id: user.id, + merged_at: Time.current - 1.hour, + latest_closed_by_id: user.id, + latest_closed_at: Time.current + 1.hour + } + end + + subject(:metric) { helper.create_merge_request_metrics(attributes) } + + before do + allow(helper).to receive(:merge_request).and_return(merge_request) + end + + it 'returns a metric with the provided attributes' do + expect(metric).to have_attributes(attributes) + end + + it 'creates a metric if none currently exists' do + merge_request.metrics.destroy! + + expect { metric }.to change { MergeRequest::Metrics.count }.from(0).to(1) + end + + it 'updates the existing record if one already exists' do + expect { metric }.not_to change { MergeRequest::Metrics.count } + end + end end diff --git a/spec/support/helpers/import/user_mapping_helper.rb b/spec/support/helpers/import/user_mapping_helper.rb index 49a80d39580c7d3d0fc80b3ddff2eb1591a7672e..ab29be036548388ae11f4fd5976a68f8278331b3 100644 --- a/spec/support/helpers/import/user_mapping_helper.rb +++ b/spec/support/helpers/import/user_mapping_helper.rb @@ -28,5 +28,20 @@ def placeholder_user_references(import_type, import_uid, limit = 100) [item.model, key, item.user_reference_column, item.source_user_id] end end + + # Generate a source user with a provided project and identifier + # + # @param project [Project] + # @param identifier [String, Integer] + # @return [Import::SourceUser] + def generate_source_user(project, identifier) + create( + :import_source_user, + source_user_identifier: identifier, + source_hostname: project.import_url, + import_type: project.import_type, + namespace: project.root_ancestor + ) + end end end