From 1be8c4d8abd0160dd68aae639ab8055f53ac4d5e Mon Sep 17 00:00:00 2001 From: George Koltsov Date: Wed, 11 Sep 2024 18:40:53 +0100 Subject: [PATCH 1/7] Add placeholder user mapping to bitbucket server importer --- .../wip/bitbucket_server_user_mapping.yml | 9 +++ .../representation/pull_request.rb | 8 ++- .../importers/pull_request_importer.rb | 63 ++++++++++++++++++- .../bitbucket_server_import/user_finder.rb | 31 ++++++++- 4 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 config/feature_flags/wip/bitbucket_server_user_mapping.yml 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 00000000000000..e6addc08b7b26e --- /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/groups/gitlab-org/-/epics/tbd +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/tbd +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/tbd +milestone: '17.5' +group: group::import and integrate +type: wip +default_enabled: false diff --git a/lib/bitbucket_server/representation/pull_request.rb b/lib/bitbucket_server/representation/pull_request.rb index 996a10318f5e17..c1922d6e5d86d6 100644 --- a/lib/bitbucket_server/representation/pull_request.rb +++ b/lib/bitbucket_server/representation/pull_request.rb @@ -22,7 +22,13 @@ def description end def reviewers - raw['reviewers'] + raw['reviewers'].map do |reviewer| + reviewer.merge( + 'author_username' => reviewer.dig('user', 'slug'), + 'author_email' => reviewer.dig('user', 'emailAddress'), + 'author' => reviewer.dig('user', 'name') + ) + end end def iid 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 198f0ed35615cf..a237d89a01af44 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -15,6 +15,7 @@ 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 + @original_users_map = {}.compare_by_identity end def execute @@ -32,7 +33,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] @@ -45,6 +46,8 @@ def execute # Create refs/merge-requests/iid/head reference for the merge request merge_request.fetch_ref! + push_placeholder_references(merge_request) if merge_request.persisted? + log_info(import_stage: 'import_pull_request', message: 'finished', iid: object[:iid]) end @@ -70,11 +73,17 @@ def author_line formatter.author_line(object[:author]) end + def author_id(object) + user_finder.author_id(object) + 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) + if placeholder_user_mapping_enabled? + user_finder.placeholder_user_id(reviewer) + elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) user_finder.find_user_id(by: :username, value: reviewer.dig('user', 'slug')) else user_finder.find_user_id(by: :email, value: reviewer.dig('user', 'emailAddress')) @@ -82,6 +91,11 @@ def reviewers end end + def placeholder_user_mapping_enabled? + Feature.enabled?(:importer_user_mapping, project.creator) && + Feature.enabled?(:bitbucket_server_user_mapping, project.creator) + end + def source_branch_sha source_branch_sha = project.repository.commit(object[:source_branch_sha])&.sha @@ -89,6 +103,51 @@ def source_branch_sha project.repository.find_commits_by_message(object[:source_branch_sha])&.first&.sha end + + def push_placeholder_references(merge_request) + return unless placeholder_user_mapping_enabled? + + push_merge_request_reference(merge_request) + push_reviewer_references(merge_request) + end + + def push_merge_request_reference(merge_request) + source_user = user_finder.source_user(object[:author_email]) + + return if source_user.accepted_status? && + merge_request.author_id == source_user.reassign_to_user_id + + ::Import::PlaceholderReferences::PushService.from_record( + import_source: ::Import::SOURCE_BITBUCKET_SERVER, + import_uid: project.import_state.id, + record: merge_request, + source_user: source_user, + user_reference_column: :author_id + ).execute + end + + def push_reviewer_references(merge_request) + mr_reviewers = merge_request.merge_request_reviewers + source_users = ::Import::SourceUser.find_by_placeholder_user_id(mr_reviewers.map(&:user_id)) + + return unless mr_reviewers.any? + return unless source_users.any? + + mr_reviewers.each do |mr_reviewer| + source_user = source_users.find { |user| user.placeholder_user_id == mr_reviewer.user_id } + + next unless source_user.present? + next if source_user.accepted_status? && mr_reviewer.user_id == source_user.reassign_to_user_id + + ::Import::PlaceholderReferences::PushService.from_record( + import_source: ::Import::SOURCE_BITBUCKET_SERVER, + import_uid: project.import_state.id, + record: mr_reviewer, + source_user: source_user, + user_reference_column: :user_id + ).execute + end + end end end end diff --git a/lib/gitlab/bitbucket_server_import/user_finder.rb b/lib/gitlab/bitbucket_server_import/user_finder.rb index fec0af16013d2f..4fd55ab23fd0b1 100644 --- a/lib/gitlab/bitbucket_server_import/user_finder.rb +++ b/lib/gitlab/bitbucket_server_import/user_finder.rb @@ -16,7 +16,11 @@ def initialize(project) end def author_id(object) - uid(object) || project.creator_id + if placeholder_user_mapping_enabled? + placeholder_user_id(object) + else + uid(object) || project.creator_id + end end # Object should behave as a object so we can remove object.is_a?(Hash) check @@ -51,6 +55,18 @@ def find_user_id(by:, value:) end end + def placeholder_user_id(object) + source_user_mapper.find_or_create_source_user( + source_name: object[:author], + source_username: object[:author_username], + source_user_identifier: object[:author_email] + ).mapped_user_id + end + + def source_user(identifier) + source_user_mapper.find_source_user(identifier) + end + private def cache @@ -60,6 +76,19 @@ def cache def build_cache_key(by, value) format(CACHE_KEY, project_id: project.id, by: by, value: value) 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_data.credentials[:base_uri] + ) + end + + def placeholder_user_mapping_enabled? + Feature.enabled?(:importer_user_mapping, project.creator) && + Feature.enabled?(:bitbucket_server_user_mapping, project.creator) + end end end end -- GitLab From f36256b7336aebd094e36d5187a65bd3613d33e4 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Fri, 20 Sep 2024 15:19:24 +0100 Subject: [PATCH 2/7] Add placeholder user mapping to bitbucket server [jnutt additions] --- .../wip/bitbucket_server_user_mapping.yml | 2 +- .../representation/pull_request.rb | 8 +- .../importers/pull_request_importer.rb | 92 +++++------- .../pull_request_notes/approved_event.rb | 15 +- .../pull_request_notes/base_importer.rb | 1 + .../base_note_diff_importer.rb | 29 +++- .../pull_request_notes/declined_event.rb | 32 +++- .../pull_request_notes/merge_event.rb | 15 +- .../pull_request_notes/standalone_notes.rb | 6 +- .../importers/pull_request_notes_importer.rb | 53 +++++-- .../project_creator.rb | 7 +- .../bitbucket_server_import/user_finder.rb | 45 +++--- lib/gitlab/import/merge_request_helpers.rb | 2 + lib/import/placeholder_references/pusher.rb | 42 ++++++ spec/factories/projects.rb | 6 + .../importers/pull_request_importer_spec.rb | 49 ++++++- .../pull_request_notes/approved_event_spec.rb | 137 ++++++++++-------- .../pull_request_notes/declined_event_spec.rb | 28 +++- .../pull_request_notes/inline_spec.rb | 27 +++- .../pull_request_notes/merge_event_spec.rb | 28 +++- .../standalone_notes_spec.rb | 27 +++- .../pull_request_notes_importer_spec.rb | 125 ++++++++++++++++ .../project_creator_spec.rb | 121 ++++++++++++++++ .../user_finder_spec.rb | 72 +++++++-- .../import/merge_request_helpers_spec.rb | 31 ++++ 25 files changed, 802 insertions(+), 198 deletions(-) create mode 100644 lib/import/placeholder_references/pusher.rb create mode 100644 spec/lib/gitlab/bitbucket_server_import/project_creator_spec.rb diff --git a/config/feature_flags/wip/bitbucket_server_user_mapping.yml b/config/feature_flags/wip/bitbucket_server_user_mapping.yml index e6addc08b7b26e..a8360c5eb83287 100644 --- a/config/feature_flags/wip/bitbucket_server_user_mapping.yml +++ b/config/feature_flags/wip/bitbucket_server_user_mapping.yml @@ -3,7 +3,7 @@ name: bitbucket_server_user_mapping feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/tbd introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/tbd rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/tbd -milestone: '17.5' +milestone: '17.7' group: group::import and integrate type: wip default_enabled: false diff --git a/lib/bitbucket_server/representation/pull_request.rb b/lib/bitbucket_server/representation/pull_request.rb index c1922d6e5d86d6..996a10318f5e17 100644 --- a/lib/bitbucket_server/representation/pull_request.rb +++ b/lib/bitbucket_server/representation/pull_request.rb @@ -22,13 +22,7 @@ def description end def reviewers - raw['reviewers'].map do |reviewer| - reviewer.merge( - 'author_username' => reviewer.dig('user', 'slug'), - 'author_email' => reviewer.dig('user', 'emailAddress'), - 'author' => reviewer.dig('user', 'name') - ) - end + raw['reviewers'] end def iid 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 a237d89a01af44..e0cab6d03fb93d 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,7 +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 - @original_users_map = {}.compare_by_identity + + @reviewer_references = {} end def execute @@ -42,12 +44,12 @@ 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! - push_placeholder_references(merge_request) if merge_request.persisted? - log_info(import_stage: 'import_pull_request', message: 'finished', iid: object[:iid]) end @@ -60,42 +62,54 @@ 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(object) - user_finder.author_id(object) + 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 placeholder_user_mapping_enabled? - user_finder.placeholder_user_id(reviewer) + 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.dig('user', 'slug')) + 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 - def placeholder_user_mapping_enabled? - Feature.enabled?(:importer_user_mapping, project.creator) && - Feature.enabled?(:bitbucket_server_user_mapping, project.creator) - end - def source_branch_sha source_branch_sha = project.repository.commit(object[:source_branch_sha])&.sha @@ -104,48 +118,10 @@ def source_branch_sha project.repository.find_commits_by_message(object[:source_branch_sha])&.first&.sha end - def push_placeholder_references(merge_request) - return unless placeholder_user_mapping_enabled? - - push_merge_request_reference(merge_request) - push_reviewer_references(merge_request) - end - - def push_merge_request_reference(merge_request) - source_user = user_finder.source_user(object[:author_email]) - - return if source_user.accepted_status? && - merge_request.author_id == source_user.reassign_to_user_id - - ::Import::PlaceholderReferences::PushService.from_record( - import_source: ::Import::SOURCE_BITBUCKET_SERVER, - import_uid: project.import_state.id, - record: merge_request, - source_user: source_user, - user_reference_column: :author_id - ).execute - end - def push_reviewer_references(merge_request) mr_reviewers = merge_request.merge_request_reviewers - source_users = ::Import::SourceUser.find_by_placeholder_user_id(mr_reviewers.map(&:user_id)) - - return unless mr_reviewers.any? - return unless source_users.any? - mr_reviewers.each do |mr_reviewer| - source_user = source_users.find { |user| user.placeholder_user_id == mr_reviewer.user_id } - - next unless source_user.present? - next if source_user.accepted_status? && mr_reviewer.user_id == source_user.reassign_to_user_id - - ::Import::PlaceholderReferences::PushService.from_record( - import_source: ::Import::SOURCE_BITBUCKET_SERVER, - import_uid: project.import_state.id, - record: mr_reviewer, - source_user: source_user, - user_reference_column: :user_id - ).execute + push_reference(project, mr_reviewer, :user_id, @reviewer_references[mr_reviewer.user_id]) 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 632377229cba97..46de3450650f57 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 c9924229f4b7f2..fc9f1d81b9bf3f 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 a171b57e21afda..16e017d94fb6e7 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,29 @@ 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]) 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 7a3ec390518334..e0b7e43ab95efe 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 @@ -6,6 +6,7 @@ module Importers module PullRequestNotes class DeclinedEvent < BaseImporter include ::Gitlab::Import::MergeRequestHelpers + include ::Import::PlaceholderReferences::Pusher def execute(declined_event) log_info( @@ -15,7 +16,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 +43,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 +60,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 fe60a1dbf36960..1e0b28b4c64c71 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 fadddabd9edab9..a8b0fa26bc8fd7 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 ef6715db3bef41..36b4d1a1931ce1 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,8 @@ 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) end def build_position(merge_request, pr_comment) @@ -171,10 +193,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 +214,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 @@ -222,6 +246,17 @@ 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 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 2833b6fa2007f6..5b4fcaa2c13637 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 4fd55ab23fd0b1..aee28a947775e9 100644 --- a/lib/gitlab/bitbucket_server_import/user_finder.rb +++ b/lib/gitlab/bitbucket_server_import/user_finder.rb @@ -16,19 +16,17 @@ def initialize(project) end def author_id(object) - if placeholder_user_mapping_enabled? - placeholder_user_id(object) - else - uid(object) || project.creator_id - end + uid(object) || project.creator_id end # 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) @@ -55,18 +53,6 @@ def find_user_id(by:, value:) end end - def placeholder_user_id(object) - source_user_mapper.find_or_create_source_user( - source_name: object[:author], - source_username: object[:author_username], - source_user_identifier: object[:author_email] - ).mapped_user_id - end - - def source_user(identifier) - source_user_mapper.find_source_user(identifier) - end - private def cache @@ -77,18 +63,25 @@ 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_data.credentials[:base_uri] + source_hostname: project.import_url ) end - - def placeholder_user_mapping_enabled? - Feature.enabled?(:importer_user_mapping, project.creator) && - Feature.enabled?(:bitbucket_server_user_mapping, project.creator) - end end end end diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb index 778757ab390a05..491178ccfa35bd 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 00000000000000..e1f428e5f188c5 --- /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 ca4ce5af91a158..f335fc45d37a65 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 24b867c475cab9..d0c5b142357b94 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 @@ -2,10 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestImporter, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestImporter, :clean_gitlab_redis_shared_state, feature_category: :importers do include AfterNextHelpers + include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } 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') } @@ -68,9 +69,49 @@ end end - context 'when the `bitbucket_server_user_mapping_by_username` flag is disabled' do + context 'when placeholder user mapping enabled?' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import, + :import_user_mapping_enabled + ) + end + + it 'maps the contribution to the correct user' do + importer.execute + + merge_request = project.merge_requests.last + + expect(merge_request.author).to be_a(User) + end + + it 'excludes the author tag line' do + importer.execute + + merge_request = project.merge_requests.last + expect(merge_request.description).to eq('Test') + 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', instance_of(Integer)], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], + ['MergeRequest', instance_of(Integer), 'author_id', instance_of(Integer)] + ) + end + end + + context 'when `bitbucket_server_user_mapping_by_username` & `bitbucket_server_user_mapping` flags are disabled' do before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) + stub_feature_flags( + bitbucket_server_convert_mentions_to_users: false, + bitbucket_server_user_mapping: false + ) end it 'imports reviewers correctly' do 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 c7d68f01449a98..c4bddcde50f851 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 @@ -2,14 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + let_it_be(: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(:merge_request) { create(:merge_request, source_project: project) } @@ -22,6 +19,7 @@ let(:approved_event) do { id: 4, + approver_name: pull_request_author.name, approver_username: pull_request_author.username, approver_email: pull_request_author.email, created_at: now @@ -37,6 +35,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 'creates placeholder references', :aggregate_failures 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', instance_of(Integer)], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], + ['Note', instance_of(Integer), 'author_id', instance_of(Integer)] + ) + 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 +54,48 @@ def expect_log(stage:, message:, iid:, event_id:) approval = merge_request.approvals.first - expect(approval.user).to eq(pull_request_author) + author = ::Import::SourceUser.find_source_user( + source_user_identifier: pull_request_author.username, + namespace: project.root_namespace, + source_hostname: project.import_url, + import_type: project.import_type + ).placeholder_user + + expect(approval.user).to eq(author) 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).to eq(author) 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.id) 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( - stage: 'import_approved_event', - message: 'skipped due to missing user', - iid: merge_request.iid, - event_id: 4 - ) + 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) - 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 } + importer.execute(approved_event) + end - expect(merge_request.approvals).to be_empty + context 'when user contribution mapping is disabled' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import + ) 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) + let_it_be(:merge_request) { create(:merge_request, source_project: project) } - 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(: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 +112,46 @@ 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 'does not create the reviewer record' do - expect { importer.execute(approved_event) }.not_to change { merge_request.reviewers.count } - end - end + it 'finds the user based on email' do + importer.execute(approved_event) - 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) + approval = merge_request.approvals.first - importer.execute(approved_event) + 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 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 e10e263cf20fb2..582075ce8e08cf 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 @@ -2,7 +2,9 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + let_it_be(:project) do create(:project, :repository, :import_started, import_data_attributes: { @@ -22,6 +24,7 @@ let(:declined_event) do { id: 7, + decliner_name: decliner_author.name, decliner_username: decliner_author.username, decliner_email: decliner_author.email, created_at: now @@ -37,6 +40,29 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute', :clean_gitlab_redis_shared_state do + context 'when user contribution mapping is enabled' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import, + :import_user_mapping_enabled + ) + end + + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + it 'creates placeholder reference', :aggregate_failures 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', instance_of(Integer)], + ['MergeRequest::Metrics', instance_of(Integer), 'latest_closed_by_id', instance_of(Integer)] + ) + end + end + it 'imports the declined event' do expect { importer.execute(declined_event) } .to change { merge_request.events.count }.from(0).to(1) 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 52a09c5e3b327a..b662bc6cc041ec 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 @@ -2,7 +2,9 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + let_it_be(:project) do create(:project, :repository, :import_started, import_data_attributes: { @@ -63,6 +65,29 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do + context 'when user contribution mapping is enabled' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import, + :import_user_mapping_enabled + ) + end + + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + it 'creates placeholder reference', :aggregate_failures 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', instance_of(Integer)], + ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)] + ) + end + end + it 'imports the threaded discussion' do expect(mentions_converter).to receive(:convert).and_call_original.twice 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 d9fc56062bab8d..a1b7f7c7d306c7 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 @@ -2,7 +2,9 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + let_it_be(:project) do create(:project, :repository, :import_started, import_data_attributes: { @@ -22,6 +24,8 @@ let_it_be(:merge_event) do { id: 3, + committer_user: pull_request_author.name, + committer_username: pull_request_author.username, committer_email: pull_request_author.email, merge_timestamp: now, merge_commit: '12345678' @@ -37,6 +41,28 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do + context 'when user contribution mapping is enabled' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import, + :import_user_mapping_enabled + ) + end + + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + it 'creates placeholder references', :aggregate_failures 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', instance_of(Integer)] + ) + end + end + it 'imports the merge event' do importer.execute(merge_event) 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 94a901f2d3b750..eca16afcb62383 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 @@ -2,7 +2,9 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + let_it_be(:project) do create(:project, :repository, :import_started, import_data_attributes: { @@ -21,6 +23,7 @@ { id: 5, note: 'Hello world', + author_name: note_author.name, author_email: note_author.email, author_username: note_author.username, comments: [], @@ -43,6 +46,28 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do + context 'when user contribution mapping is enabled' do + let_it_be(:project) do + create( + :project, + :repository, + :bitbucket_server_import, + :import_user_mapping_enabled + ) + end + + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + it 'creates 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', instance_of(Integer)] + ) + end + end + it 'imports the stand alone comments' do expect(mentions_converter).to receive(:convert).and_call_original 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 ee1b53b566fa86..8c4f98685c93e7 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,6 +4,7 @@ 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, @@ -30,6 +31,8 @@ comment?: false, merge_event?: true, approved_event?: false, + committer_name: pull_request_author.name, + committer_username: pull_request_author.username, committer_email: pull_request_author.email, merge_timestamp: now, merge_commit: '12345678' @@ -43,6 +46,7 @@ comment?: false, merge_event?: false, approved_event?: true, + approver_name: pull_request_author.name, approver_username: pull_request_author.username, approver_email: pull_request_author.email, created_at: now @@ -53,6 +57,7 @@ instance_double( BitbucketServer::Representation::Comment, note: 'Hello world', + author_name: note_author.name, author_email: note_author.email, author_username: note_author.username, comments: [], @@ -397,6 +402,126 @@ def expect_log(stage:, message:) end end end + + context 'when user contribution mapping is enabled' do + 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, iid: pull_request.iid, source_project: project) } + let(:cached_references) do + placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) + end + + context 'when importing standalone comments' do + before do + 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', instance_of(Integer)] + ) + 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 + + let(:reply) do + instance_double( + BitbucketServer::Representation::PullRequestComment, + author_name: reply_author.name, + author_email: reply_author.email, + author_username: reply_author.username, + 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.name, + author_email: inline_note_author.email, + author_username: inline_note_author.username, + 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 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)], + ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)] + ) + end + end + + context 'when PR has a merge event' do + before do + allow_next(BitbucketServer::Client).to receive(:activities).and_return([merge_event]) + end + + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ["MergeRequest::Metrics", instance_of(Integer), "merged_by_id", instance_of(Integer)] + ) + end + end + + context 'when PR has an approved event' do + before do + allow_next(BitbucketServer::Client).to receive(:activities).and_return([approved_event]) + end + + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['Approval', instance_of(Integer), 'user_id', instance_of(Integer)], + ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], + ['Note', instance_of(Integer), 'author_id', instance_of(Integer)] + ) + end + end + end end shared_examples 'import is skipped' do 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 00000000000000..9ea6a4719ee16a --- /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 78d258bd4445b5..394371d3698a27 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb @@ -6,23 +6,58 @@ let_it_be(:user) { create(:user) } let(:created_id) { 1 } - let(:project) { build_stubbed(:project, creator_id: created_id, id: 1) } + let(:user_contribution_mapping_enabled) { false } + let(:project) do + build_stubbed(:project, creator_id: created_id, id: 1, import_data_attributes: { + data: { 'user_contribution_mapping_enabled' => user_contribution_mapping_enabled } + }) + end + + 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 } + context 'when project has user contribution mapping enabled' do + let(:user_contribution_mapping_enabled) { true } + let(:source_user) { build_stubbed(:import_source_user, :completed) } + + 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 - expect(user_finder).to receive(:uid).with(object).and_return(10) - expect(user_finder.author_id(object)).to eq(10) + it 'returns the mapped user' do + expect( + user_finder.author_id(user_representation) + ).to eq(source_user.mapped_user.id) + end end - context 'when corresponding user does not exist' do - it 'fallsback to project creator_id' do - object = { author_email: 'unknown' } + context 'when `bitbucket_server_user_mapping` is disabled' do + before do + stub_feature_flags(bitbucket_server_user_mapping: false) + end + + 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) + end + + context 'when corresponding user does not exist' do + it 'falls back to project creator_id' do + object = { author_email: 'unknown' } - expect(user_finder.author_id(object)).to eq(created_id) + expect(user_finder.author_id(object)).to eq(created_id) + end end end end @@ -36,7 +71,24 @@ end end - context 'when provided object is a representation Object' do + context 'when project has user contribution mapping enabled' do + let(:user_contribution_mapping_enabled) { true } + let(:source_user) { build_stubbed(:import_source_user, :completed) } + + 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 representation 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 + + context 'when provided object is a Comment representation object' do it 'maps to a existing user with the same username' do object = instance_double(BitbucketServer::Representation::Comment, author_username: user.username) diff --git a/spec/lib/gitlab/import/merge_request_helpers_spec.rb b/spec/lib/gitlab/import/merge_request_helpers_spec.rb index d38eca9d52a73c..75f8085735c61d 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 -- GitLab From 85e87983e7c4786ba4b2dc9a744553cef692bfe9 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Tue, 26 Nov 2024 22:42:24 +0000 Subject: [PATCH 3/7] Spec updates for new default --- .../importers/pull_request_importer_spec.rb | 81 +++--- .../pull_request_notes/approved_event_spec.rb | 28 +- .../pull_request_notes/declined_event_spec.rb | 117 ++++----- .../pull_request_notes/inline_spec.rb | 56 ++-- .../pull_request_notes/merge_event_spec.rb | 43 +-- .../standalone_notes_spec.rb | 36 +-- .../pull_request_notes_importer_spec.rb | 246 +++++++----------- .../user_finder_spec.rb | 168 ++++++------ .../helpers/import/user_mapping_helper.rb | 16 ++ 9 files changed, 323 insertions(+), 468 deletions(-) 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 d0c5b142357b94..2ef04a16907386 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 @@ -6,9 +6,14 @@ include AfterNextHelpers include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + + let_it_be(:author) { create(:user, username: 'username') } + let_it_be(:author_source_user) { generate_source_user(project, author) } let_it_be(:reviewer_1) { create(:user, username: 'john_smith', email: 'john@smith.com') } + let_it_be(:reviewer_1_source_user) { generate_source_user(project, reviewer_1) } let_it_be(:reviewer_2) { create(:user, username: 'jane_doe', email: 'jane@doe.com') } + let_it_be(:reviewer_2_source_user) { generate_source_user(project, reviewer_2) } 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) } @@ -19,8 +24,6 @@ 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) @@ -31,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: [reviewer_1.id, reviewer_2.id], state: pull_request.state, - author_id: project.creator_id, - description: "*Created by: #{pull_request.author}*\n\n#{pull_request.description}", + author_id: author.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') @@ -69,43 +83,6 @@ end end - context 'when placeholder user mapping enabled?' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import, - :import_user_mapping_enabled - ) - end - - it 'maps the contribution to the correct user' do - importer.execute - - merge_request = project.merge_requests.last - - expect(merge_request.author).to be_a(User) - end - - it 'excludes the author tag line' do - importer.execute - - merge_request = project.merge_requests.last - expect(merge_request.description).to eq('Test') - 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', instance_of(Integer)], - ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], - ['MergeRequest', instance_of(Integer), 'author_id', instance_of(Integer)] - ) - end - end - context 'when `bitbucket_server_user_mapping_by_username` & `bitbucket_server_user_mapping` flags are disabled' do before do stub_feature_flags( @@ -193,5 +170,23 @@ importer.execute end + + context 'when user contribution mapping is disabled' do + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } + + 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 + 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 c4bddcde50f851..da25b3dcd5901f 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 @@ -5,17 +5,16 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - let_it_be(:project) do - create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) - end - + 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!(:pull_request_author) do + let_it_be(:pull_request_author) do create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') end + let_it_be(:source_user) { generate_source_user(project, pull_request_author) } + let(:approved_event) do { id: 4, @@ -40,9 +39,9 @@ def expect_log(stage:, message:, iid:, event_id:) 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', instance_of(Integer)], - ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], - ['Note', instance_of(Integer), 'author_id', instance_of(Integer)] + ['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 @@ -54,26 +53,19 @@ def expect_log(stage:, message:, iid:, event_id:) approval = merge_request.approvals.first - author = ::Import::SourceUser.find_source_user( - source_user_identifier: pull_request_author.username, - namespace: project.root_namespace, - source_hostname: project.import_url, - import_type: project.import_type - ).placeholder_user - - expect(approval.user).to eq(author) + expect(approval.user).to eq(pull_request_author) 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(author) + expect(note.author).to eq(pull_request_author) expect(note.system).to be_truthy expect(note.created_at).to eq(now) reviewer = merge_request.reviewers.first - expect(reviewer.id).to eq(author.id) + expect(reviewer.id).to eq(pull_request_author.id) end it 'logs its progress' do 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 582075ce8e08cf..a8e937397ad56b 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 @@ -5,21 +5,11 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - 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(: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!(:decliner_author) do - create(:user, username: 'decliner_author', email: 'decliner_author@example.org') - end + let_it_be(:decliner_author) { create(:user, username: 'decliner_author', email: 'decliner_author@example.org') } + let_it_be(:source_user) { generate_source_user(project, decliner_author) } let(:declined_event) do { @@ -27,7 +17,7 @@ decliner_name: decliner_author.name, decliner_username: decliner_author.username, decliner_email: decliner_author.email, - created_at: now + created_at: Time.now.utc.change(usec: 0) } end @@ -40,27 +30,14 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute', :clean_gitlab_redis_shared_state do - context 'when user contribution mapping is enabled' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import, - :import_user_mapping_enabled - ) - end - - let_it_be(:merge_request) { create(:merge_request, source_project: project) } - - it 'creates placeholder reference', :aggregate_failures do - importer.execute(declined_event) + it 'creates placeholder reference' 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', instance_of(Integer)], - ['MergeRequest::Metrics', instance_of(Integer), 'latest_closed_by_id', instance_of(Integer)] - ) - end + 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 @@ -73,56 +50,62 @@ def expect_log(stage:, message:, iid:, event_id:) expect(metrics.latest_closed_at).to eq(declined_event[:created_at]) event = merge_request.events.first + expect(event.author_id).to eq(decliner_author.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 - before do - stub_feature_flags(bitbucket_server_user_mapping_by_username: false) - 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) + + importer.execute(declined_event) + end + + context 'when user contribution mapping is disabled' do + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } - 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) - - importer.execute(declined_event) - 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 b662bc6cc041ec..83cacc76a3a5fb 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 @@ -5,23 +5,20 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - 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(: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(: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(:reply_source_user) { generate_source_user(project, reply_author) } + let_it_be(:inline_note_author) do create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') end + let_it_be(:note_source_user) { generate_source_user(project, inline_note_author) } + let(:reply) do { author_email: reply_author.email, @@ -65,27 +62,14 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do - context 'when user contribution mapping is enabled' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import, - :import_user_mapping_enabled - ) - end - - let_it_be(:merge_request) { create(:merge_request, source_project: project) } - - it 'creates placeholder reference', :aggregate_failures do - importer.execute(pr_inline_comment) + it 'creates placeholder reference', :aggregate_failures 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', instance_of(Integer)], - ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)] - ) - end + 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 @@ -168,13 +152,15 @@ 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 + 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) + importer.execute(pr_inline_comment) + end 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 a1b7f7c7d306c7..8098928706e80e 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 @@ -5,15 +5,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - 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(: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) } @@ -21,6 +13,8 @@ create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') end + let_it_be(:source_user) { generate_source_user(project, pull_request_author) } + let_it_be(:merge_event) do { id: 3, @@ -41,35 +35,22 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do - context 'when user contribution mapping is enabled' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import, - :import_user_mapping_enabled - ) - end - - let_it_be(:merge_request) { create(:merge_request, source_project: project) } - - it 'creates placeholder references', :aggregate_failures do - importer.execute(merge_event) + it 'creates placeholder references', :aggregate_failures 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', instance_of(Integer)] - ) - end + 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).to eq(pull_request_author) + expect(metrics.merged_at).to eq(merge_event[:merge_timestamp]) expect(merge_request.merge_commit_sha).to eq(merge_event[:merge_commit]) 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 eca16afcb62383..7fc1cbb5438a0a 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 @@ -5,18 +5,11 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - 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(: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(:source_user) { generate_source_user(project, note_author) } let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } let(:pr_comment) do @@ -46,26 +39,13 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } describe '#execute' do - context 'when user contribution mapping is enabled' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import, - :import_user_mapping_enabled - ) - end - - let_it_be(:merge_request) { create(:merge_request, source_project: project) } + it 'creates placeholder reference' do + importer.execute(pr_comment) - it 'creates 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', instance_of(Integer)] - ) - end + 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 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 8c4f98685c93e7..e13f90a32f21d6 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 @@ -6,24 +6,19 @@ 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' } - } - ) - end - + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } 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(:note_source_user) { generate_source_user(project, note_author) } 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!(:author_source_user) { generate_source_user(project, pull_request_author) } + let(:merge_event) do instance_double( BitbucketServer::Representation::Activity, @@ -79,6 +74,10 @@ 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) @@ -122,10 +121,18 @@ 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] + ) + end + it 'imports the stand alone comments' do expect(mentions_converter).to receive(:convert).and_call_original - expect { subject.execute }.to change { Note.count }.by(1) + expect { importer.execute }.to change { Note.count }.by(1) expect(merge_request.notes.count).to eq(1) expect(merge_request.notes.first).to have_attributes( @@ -145,7 +152,7 @@ def expect_log(stage:, message:) end it 'adds a note with the author username and email' do - subject.execute + importer.execute expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") end @@ -156,6 +163,7 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Note', + author_name: note_author.name, author_email: note_author.email, author_username: note_author.username, comments: [], @@ -169,6 +177,7 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Parent note', + author_name: note_author.name, author_email: note_author.email, author_username: note_author.username, comments: [], @@ -179,7 +188,7 @@ 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 @@ -193,7 +202,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 @@ -207,13 +216,18 @@ def expect_log(stage:, message:) 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(:reply_source_user) { generate_source_user(project, reply_author) } + let_it_be(:inline_note_author) do create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') end + let_it_be(:note_source_user) { generate_source_user(project, inline_note_author) } + let(:reply) do instance_double( BitbucketServer::Representation::PullRequestComment, + author_name: reply_author.name, author_email: reply_author.email, author_username: reply_author.username, note: 'I agree', @@ -232,6 +246,7 @@ def expect_log(stage:, message:) old_pos: nil, new_pos: 4, note: 'Hello world', + author_name: inline_note_author.name, author_email: inline_note_author.email, author_username: inline_note_author.username, comments: [reply], @@ -256,7 +271,7 @@ def expect_log(stage:, message:) 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) @@ -281,6 +296,15 @@ 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 the `bitbucket_server_convert_mentions_to_users` flag is disabled' do before do stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) @@ -289,7 +313,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 @@ -315,6 +339,14 @@ def expect_log(stage:, message:) 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 @@ -345,20 +377,21 @@ def expect_log(stage:, message:) expect(reviewer.id).to eq(pull_request_author.id) end + it 'pushes placeholder references' do + importer.execute + + 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 '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) @@ -373,12 +406,8 @@ def expect_log(stage:, message:) 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 + context 'when user contribution mapping is disabled' do + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } it 'does not set an approver' do expect { importer.execute } @@ -388,6 +417,23 @@ def expect_log(stage:, message:) expect(merge_request.approvals).to be_empty 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 end @@ -402,126 +448,6 @@ def expect_log(stage:, message:) end end end - - context 'when user contribution mapping is enabled' do - 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, iid: pull_request.iid, source_project: project) } - let(:cached_references) do - placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) - end - - context 'when importing standalone comments' do - before do - 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', instance_of(Integer)] - ) - 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 - - let(:reply) do - instance_double( - BitbucketServer::Representation::PullRequestComment, - author_name: reply_author.name, - author_email: reply_author.email, - author_username: reply_author.username, - 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.name, - author_email: inline_note_author.email, - author_username: inline_note_author.username, - 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 'pushes placeholder references' do - importer.execute - - expect(cached_references).to contain_exactly( - ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)], - ['DiffNote', instance_of(Integer), 'author_id', instance_of(Integer)] - ) - end - end - - context 'when PR has a merge event' do - before do - allow_next(BitbucketServer::Client).to receive(:activities).and_return([merge_event]) - end - - it 'pushes placeholder references' do - importer.execute - - expect(cached_references).to contain_exactly( - ["MergeRequest::Metrics", instance_of(Integer), "merged_by_id", instance_of(Integer)] - ) - end - end - - context 'when PR has an approved event' do - before do - allow_next(BitbucketServer::Client).to receive(:activities).and_return([approved_event]) - end - - it 'pushes placeholder references' do - importer.execute - - expect(cached_references).to contain_exactly( - ['Approval', instance_of(Integer), 'user_id', instance_of(Integer)], - ['MergeRequestReviewer', instance_of(Integer), 'user_id', instance_of(Integer)], - ['Note', instance_of(Integer), 'author_id', instance_of(Integer)] - ) - end - end - end end shared_examples 'import is skipped' do @@ -542,18 +468,26 @@ 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' 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 394371d3698a27..8f1d7ff735957c 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb @@ -5,14 +5,9 @@ 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(:user_contribution_mapping_enabled) { false } - let(:project) do - build_stubbed(:project, creator_id: created_id, id: 1, import_data_attributes: { - data: { 'user_contribution_mapping_enabled' => user_contribution_mapping_enabled } - }) - end + let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + let(:source_user) { build_stubbed(:import_source_user, :completed) } let(:user_representation) do { username: user.username, @@ -23,28 +18,74 @@ subject(:user_finder) { described_class.new(project) } describe '#author_id' do - context 'when project has user contribution mapping enabled' do - let(:user_contribution_mapping_enabled) { true } - let(:source_user) { build_stubbed(:import_source_user, :completed) } + 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 - 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 + it 'returns the mapped user' do + expect( + user_finder.author_id(user_representation) + ).to eq(source_user.mapped_user.id) + end + end + + 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 - it 'returns the mapped user' do - expect( - user_finder.author_id(user_representation) - ).to eq(source_user.mapped_user.id) + 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_id).to be_nil + end end end - context 'when `bitbucket_server_user_mapping` is disabled' do - before do - stub_feature_flags(bitbucket_server_user_mapping: false) + 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 + + 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 user contribution mapping is disabled' do + let(:created_id) { 1 } + let(:project) { build_stubbed(:project, :repository, :bitbucket_server_import, creator_id: created_id) } + + describe '#author_id' do it 'calls uid method' do object = { author_username: user.username } @@ -60,47 +101,30 @@ end end 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 } - expect(user_finder.uid(object)).to eq(user.id) - end - end - - context 'when project has user contribution mapping enabled' do - let(:user_contribution_mapping_enabled) { true } - let(:source_user) { build_stubbed(:import_source_user, :completed) } + 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 } - before do - allow_next_instance_of(Gitlab::Import::SourceUserMapper) do |isum| - allow(isum).to receive(:find_or_create_source_user).and_return(source_user) + expect(user_finder.uid(object)).to eq(user.id) end end - it 'takes a user representation and finds the mapped user ID' do - user_id = user_finder.uid(user_representation) + context 'when provided object is a Comment 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_id).to eq(source_user.mapped_user.id) - end - end - - context 'when provided object is a Comment 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) + expect(user_finder.uid(object)).to eq(user.id) + end end - end - context 'when corresponding user does not exist' do - it 'returns nil' do - object = { author_username: 'unknown' } + context 'when corresponding user does not exist' do + it 'returns nil' do + object = { author_username: 'unknown' } - expect(user_finder.uid(object)).to eq(nil) + expect(user_finder.uid(object)).to be_nil + end end end @@ -129,43 +153,7 @@ it 'returns nil' do object = { author_email: 'unknown' } - expect(user_finder.uid(object)).to eq(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 - - 2.times do - user_id = user_finder.find_user_id(by: :email, value: 'nobody@example.com') - - expect(user_id).to be_nil - 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) - - 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 - - 2.times do - user_id = user_finder.find_user_id(by: :username, value: user.username) - - expect(user_id).to eq(user.id) + expect(user_finder.uid(object)).to be_nil end end end diff --git a/spec/support/helpers/import/user_mapping_helper.rb b/spec/support/helpers/import/user_mapping_helper.rb index 49a80d39580c7d..fea64461c0ecd4 100644 --- a/spec/support/helpers/import/user_mapping_helper.rb +++ b/spec/support/helpers/import/user_mapping_helper.rb @@ -28,5 +28,21 @@ 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 that returns a provided user as a placeholder + # + # @param project [Project] + # @param user [User] + # @return [Import::SourceUser] + def generate_source_user(project, user) + create( + :import_source_user, + source_user_identifier: user.username, + placeholder_user: user, + source_hostname: project.import_url, + import_type: project.import_type, + namespace: project.root_ancestor + ) + end end end -- GitLab From 40206ef70f6aa4222bba4d51ce34b236e168f576 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Wed, 27 Nov 2024 19:56:26 +0000 Subject: [PATCH 4/7] Add ID to user representations as unique-er ID --- .../wip/bitbucket_server_user_mapping.yml | 4 +- .../representation/activity.rb | 15 ++ .../representation/comment.rb | 5 + .../representation/pull_request.rb | 5 + lib/bitbucket_server/representation/user.rb | 4 + .../importers/pull_request_importer.rb | 6 +- .../pull_request_notes/approved_event.rb | 7 +- .../base_note_diff_importer.rb | 6 +- .../pull_request_notes/declined_event.rb | 6 +- .../pull_request_notes/merge_event.rb | 3 +- .../pull_request_notes/standalone_notes.rb | 4 +- .../importers/pull_request_notes_importer.rb | 19 +- .../bitbucket_server_import/user_finder.rb | 4 +- .../representation/activity_spec.rb | 6 + .../representation/comment_spec.rb | 5 + .../representation/pull_request_spec.rb | 5 + .../representation/user_spec.rb | 7 +- .../importers/pull_request_importer_spec.rb | 103 +++++--- .../pull_request_notes/approved_event_spec.rb | 68 +++-- .../pull_request_notes/declined_event_spec.rb | 44 ++-- .../pull_request_notes/inline_spec.rb | 92 ++++--- .../pull_request_notes/merge_event_spec.rb | 48 +++- .../standalone_notes_spec.rb | 93 ++++--- .../pull_request_notes_importer_spec.rb | 242 +++++++++--------- .../user_finder_spec.rb | 64 +++-- .../helpers/import/user_mapping_helper.rb | 9 +- 26 files changed, 531 insertions(+), 343 deletions(-) diff --git a/config/feature_flags/wip/bitbucket_server_user_mapping.yml b/config/feature_flags/wip/bitbucket_server_user_mapping.yml index a8360c5eb83287..e50e3e44c12fd0 100644 --- a/config/feature_flags/wip/bitbucket_server_user_mapping.yml +++ b/config/feature_flags/wip/bitbucket_server_user_mapping.yml @@ -1,7 +1,7 @@ --- name: bitbucket_server_user_mapping -feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/tbd -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/tbd +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 diff --git a/lib/bitbucket_server/representation/activity.rb b/lib/bitbucket_server/representation/activity.rb index df77fd0d026d30..687acbf67127a9 100644 --- a/lib/bitbucket_server/representation/activity.rb +++ b/lib/bitbucket_server/representation/activity.rb @@ -31,6 +31,10 @@ def merge_event? action == 'MERGED' end + def committer_id + commit.dig('committer', 'id') + end + def committer_user commit.dig('committer', 'displayName') end @@ -61,6 +65,10 @@ def approved_event? action == 'APPROVED' end + def approver_id + raw.dig('user', 'id') + end + def approver_name raw.dig('user', 'displayName') end @@ -77,6 +85,10 @@ def declined_event? action == 'DECLINED' end + def decliner_id + raw.dig('user', 'id') + end + def decliner_name raw.dig('user', 'displayName') end @@ -96,15 +108,18 @@ def created_at def to_hash { id: id, + committer_id: committer_id, committer_name: committer_user, committer_user: committer_user, committer_username: committer_username, committer_email: committer_email, merge_timestamp: merge_timestamp, merge_commit: merge_commit, + approver_id: approver_id, approver_name: approver_name, approver_username: approver_username, approver_email: approver_email, + decliner_id: decliner_id, decliner_name: decliner_name, decliner_username: decliner_username, decliner_email: decliner_email, diff --git a/lib/bitbucket_server/representation/comment.rb b/lib/bitbucket_server/representation/comment.rb index d2b0a42b9e6086..6dfe8d3d781372 100644 --- a/lib/bitbucket_server/representation/comment.rb +++ b/lib/bitbucket_server/representation/comment.rb @@ -37,6 +37,10 @@ def id raw_comment['id'] end + def author_id + author['id'] + end + def author_name author['displayName'] end @@ -85,6 +89,7 @@ def to_hash { id: id, + author_id: author_id, author_name: author_name, author_email: author_email, author_username: author_username, diff --git a/lib/bitbucket_server/representation/pull_request.rb b/lib/bitbucket_server/representation/pull_request.rb index 996a10318f5e17..8ac976a96c7272 100644 --- a/lib/bitbucket_server/representation/pull_request.rb +++ b/lib/bitbucket_server/representation/pull_request.rb @@ -3,6 +3,10 @@ module BitbucketServer module Representation class PullRequest < Representation::Base + def author_id + raw.dig('author', 'user', 'id') + end + def author raw.dig('author', 'user', 'name') end @@ -79,6 +83,7 @@ def target_branch_sha def to_hash { iid: iid, + author_id: author_id, author: author, author_email: author_email, author_username: author_username, diff --git a/lib/bitbucket_server/representation/user.rb b/lib/bitbucket_server/representation/user.rb index 433baec1c42f96..28d766439e1b3d 100644 --- a/lib/bitbucket_server/representation/user.rb +++ b/lib/bitbucket_server/representation/user.rb @@ -3,6 +3,10 @@ module BitbucketServer module Representation class User < Representation::Base + def id + user['id'] + end + def email user['emailAddress'] end 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 e0cab6d03fb93d..b1864306e2a0d2 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -44,7 +44,7 @@ def execute creator = Gitlab::Import::MergeRequestCreator.new(project) merge_request = creator.execute(attributes) - push_reference(project, merge_request, :author_id, object[:author_username]) + push_reference(project, merge_request, :author_id, object[:author_id]) push_reviewer_references(merge_request) # Create refs/merge-requests/iid/head reference for the merge request @@ -81,6 +81,7 @@ def author_line def author_id(pull_request_data) if user_mapping_enabled?(project) user_finder.author_id( + id: pull_request_data['author_id'], username: pull_request_data['author_username'], display_name: pull_request_data['author'] ) @@ -95,11 +96,12 @@ def reviewers object[:reviewers].filter_map do |reviewer_data| if user_mapping_enabled?(project) uid = user_finder.uid( + id: reviewer_data.dig('user', 'id'), username: reviewer_data.dig('user', 'slug'), display_name: reviewer_data.dig('user', 'displayName') ) - @reviewer_references[uid] = reviewer_data.dig('user', 'slug') + @reviewer_references[uid] = reviewer_data.dig('user', 'id') uid elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) 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 46de3450650f57..8593e2cdef8267 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 @@ -17,6 +17,7 @@ def execute(approved_event) user_id = if user_mapping_enabled?(project) user_finder.uid( + id: approved_event[:approver_id], username: approved_event[:approver_username], display_name: approved_event[:approver_name] ) @@ -40,11 +41,11 @@ def execute(approved_event) submitted_at = approved_event[:created_at] || merge_request[:updated_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]) + push_reference(project, approval, :user_id, approved_event[:approver_id]) + push_reference(project, approval_note, :author_id, approved_event[:approver_id]) reviewer = create_reviewer!(merge_request.id, user_id, submitted_at) - push_reference(project, reviewer, :user_id, approved_event[:approver_username]) if reviewer + push_reference(project, reviewer, :user_id, approved_event[:approver_id]) if reviewer log_info( import_stage: 'import_approved_event', 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 16e017d94fb6e7..21ab6cb3a544da 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,7 +28,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]) + push_reference(project, note, :author_id, comment[:author_id]) return note end @@ -84,6 +84,7 @@ def pull_request_comment_attributes(comment) def author(comment) if user_mapping_enabled?(project) user_finder.uid( + id: comment[:author_id], username: comment[:author_username], display_name: comment[:author_name] ) @@ -103,7 +104,8 @@ def create_basic_fallback_note(merge_request, comment, position) attributes[:note] = note_text note = merge_request.notes.create!(attributes) - push_reference(project, note, :author_id, comment[author_username]) + push_reference(project, note, :author_id, comment[:author_id]) + 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 e0b7e43ab95efe..adc16c6733a7c5 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 @@ -6,7 +6,6 @@ module Importers module PullRequestNotes class DeclinedEvent < BaseImporter include ::Gitlab::Import::MergeRequestHelpers - include ::Import::PlaceholderReferences::Pusher def execute(declined_event) log_info( @@ -18,6 +17,7 @@ def execute(declined_event) user_id = if user_mapping_enabled?(project) user_finder.uid( + id: declined_event[:decliner_id], username: declined_event[:decliner_username], display_name: declined_event[:decliner_name] ) @@ -45,13 +45,13 @@ def execute(declined_event) SystemNoteService.change_status(merge_request, merge_request.target_project, user, 'closed', nil) event = record_event(user_id) - push_reference(project, event, :author_id, declined_event[:decliner_username]) + push_reference(project, event, :author_id, declined_event[:decliner_id]) 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]) + push_reference(project, metric, :latest_closed_by_id, declined_event[:decliner_id]) log_info( import_stage: 'import_declined_event', 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 1e0b28b4c64c71..a84246c2d433e5 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 @@ -17,6 +17,7 @@ def execute(merge_event) user_id = if user_mapping_enabled?(project) user_finder.uid( + id: merge_event[:committer_id], username: merge_event[:committer_username], display_name: merge_event[:committer_user] ) @@ -29,7 +30,7 @@ def execute(merge_event) timestamp = merge_event[:merge_timestamp] merge_request.update({ merge_commit_sha: merge_event[:merge_commit] }) metric = create_merge_request_metrics(merged_by_id: user_id, merged_at: timestamp) - push_reference(project, metric, :merged_by_id, merge_event[:committer_username]) + push_reference(project, metric, :merged_by_id, merge_event[:committer_id]) 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 a8b0fa26bc8fd7..82e23a232bf9ed 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 @@ -14,11 +14,11 @@ def execute(comment) ) note = merge_request.notes.create!(pull_request_comment_attributes(comment)) - push_reference(note.project, note, :author_id, comment[:author_username]) + push_reference(note.project, note, :author_id, comment[:author_id]) comment[:comments].each do |reply| note = merge_request.notes.create!(pull_request_comment_attributes(reply)) - push_reference(note.project, note, :author_id, reply[:author_username]) + push_reference(note.project, note, :author_id, reply[:author_id]) 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 36b4d1a1931ce1..a067da93e6420f 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 @@ -59,6 +59,7 @@ def import_merge_event(merge_request, merge_event) user_id = if user_mapping_enabled?(project) user_finder.uid( + id: merge_event.committer_id, username: merge_event.committer_username, display_name: merge_event.committer_name ) @@ -73,7 +74,7 @@ def import_merge_event(merge_request, merge_event) 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) + push_reference(project, metric, :merged_by_id, merge_event.committer_id) log_info(import_stage: 'import_merge_event', message: 'finished', iid: merge_request.iid) end @@ -89,6 +90,7 @@ def import_approved_event(merge_request, approved_event) user_id = if user_mapping_enabled?(project) user_finder.uid( + id: approved_event.approver_id, username: approved_event.approver_username, display_name: approved_event.approver_name ) @@ -103,11 +105,11 @@ def import_approved_event(merge_request, approved_event) submitted_at = approved_event.created_at || merge_request.updated_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) + push_reference(project, approval, :user_id, approved_event.approver_id) + push_reference(project, approval_note, :author_id, approved_event.approver_id) reviewer = create_reviewer!(merge_request.id, user_id, submitted_at) - push_reference(project, reviewer, :user_id, approved_event.approver_username) if reviewer + push_reference(project, reviewer, :user_id, approved_event.approver_id) if reviewer log_info( import_stage: 'import_approved_event', @@ -145,7 +147,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) + push_reference(project, note, :author_id, comment.author_id) return note end @@ -174,7 +176,7 @@ def create_fallback_diff_note(merge_request, comment, position) attributes[:note] = note note = merge_request.notes.create!(attributes) - push_reference(project, note, :author_id, comment.author_username) + push_reference(project, note, :author_id, comment.author_id) end def build_position(merge_request, pr_comment) @@ -194,11 +196,11 @@ def import_standalone_pr_comments(pr_comments, merge_request) pr_comments.each do |comment| note = merge_request.notes.create!(pull_request_comment_attributes(comment)) - push_reference(project, note, :author_id, comment.author_username) + push_reference(project, note, :author_id, comment.author_id) comment.comments.each do |replies| note = merge_request.notes.create!(pull_request_comment_attributes(replies)) - push_reference(project, note, :author_id, comment.author_username) + push_reference(project, note, :author_id, comment.author_id) end rescue StandardError => e Gitlab::ErrorTracking.log_exception( @@ -249,6 +251,7 @@ def pull_request_comment_attributes(comment) def author(comment) if user_mapping_enabled?(project) user_finder.uid( + id: comment.author_id, username: comment.author_username, display_name: comment.author_name ) diff --git a/lib/gitlab/bitbucket_server_import/user_finder.rb b/lib/gitlab/bitbucket_server_import/user_finder.rb index aee28a947775e9..f1f9f69752f89e 100644 --- a/lib/gitlab/bitbucket_server_import/user_finder.rb +++ b/lib/gitlab/bitbucket_server_import/user_finder.rb @@ -25,7 +25,7 @@ def uid(object) # 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 + 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 @@ -69,7 +69,7 @@ def user_mapping_enabled?(project) def source_user_for_author(user_data) source_user_mapper.find_or_create_source_user( - source_user_identifier: user_data[:username], + source_user_identifier: user_data[:id], source_name: user_data[:display_name], source_username: user_data[:username] ) diff --git a/spec/lib/bitbucket_server/representation/activity_spec.rb b/spec/lib/bitbucket_server/representation/activity_spec.rb index 32899050a04a6f..fa436e4296077c 100644 --- a/spec/lib/bitbucket_server/representation/activity_spec.rb +++ b/spec/lib/bitbucket_server/representation/activity_spec.rb @@ -48,6 +48,7 @@ it { expect(subject.id).to eq(7) } it { expect(subject.comment?).to be_falsey } it { expect(subject.inline_comment?).to be_falsey } + it { expect(subject.committer_id).to eq(1) } it { expect(subject.committer_user).to eq('root') } it { expect(subject.committer_name).to eq('root') } it { expect(subject.committer_username).to eq('slug') } @@ -61,6 +62,7 @@ expect(subject.to_hash).to match( a_hash_including( id: 7, + committer_id: 1, committer_user: 'root', committer_name: 'root', committer_username: 'slug', @@ -80,6 +82,7 @@ it { expect(subject.inline_comment?).to be_falsey } it { expect(subject.merge_event?).to be_falsey } it { expect(subject.approved_event?).to be_truthy } + it { expect(subject.approver_id).to eq(1) } it { expect(subject.approver_name).to eq('root') } it { expect(subject.approver_username).to eq('slug') } it { expect(subject.approver_email).to eq('test.user@example.com') } @@ -90,6 +93,7 @@ expect(subject.to_hash).to match( a_hash_including( id: 15, + approver_id: 1, approver_name: 'root', approver_username: 'slug', approver_email: 'test.user@example.com' @@ -107,6 +111,7 @@ it { expect(subject.inline_comment?).to be_falsey } it { expect(subject.merge_event?).to be_falsey } it { expect(subject.declined_event?).to be_truthy } + it { expect(subject.decliner_id).to eq(1) } it { expect(subject.decliner_name).to eq('root') } it { expect(subject.decliner_username).to eq('slug') } it { expect(subject.decliner_email).to eq('test.user@example.com') } @@ -117,6 +122,7 @@ expect(subject.to_hash).to match( a_hash_including( id: 18, + decliner_id: 1, decliner_name: 'root', decliner_username: 'slug', decliner_email: 'test.user@example.com' diff --git a/spec/lib/bitbucket_server/representation/comment_spec.rb b/spec/lib/bitbucket_server/representation/comment_spec.rb index 7e32da8a7f7b0a..cceb9240a5bb48 100644 --- a/spec/lib/bitbucket_server/representation/comment_spec.rb +++ b/spec/lib/bitbucket_server/representation/comment_spec.rb @@ -12,6 +12,10 @@ it { expect(comment_representation.id).to eq(9) } end + describe '#author_id' do + it { expect(comment_representation.author_id).to eq(1) } + end + describe '#author_name' do it { expect(comment_representation.author_name).to eq('root') } end @@ -87,6 +91,7 @@ expect(comment_representation.to_hash).to match( a_hash_including( id: 9, + author_id: 1, author_name: 'root', author_email: 'test.user@example.com', author_username: 'username', diff --git a/spec/lib/bitbucket_server/representation/pull_request_spec.rb b/spec/lib/bitbucket_server/representation/pull_request_spec.rb index 2d67dd88b241ef..c558404000df7e 100644 --- a/spec/lib/bitbucket_server/representation/pull_request_spec.rb +++ b/spec/lib/bitbucket_server/representation/pull_request_spec.rb @@ -7,6 +7,10 @@ subject { described_class.new(sample_data) } + describe '#author_id' do + it { expect(subject.author_id).to eq(1) } + end + describe '#author' do it { expect(subject.author).to eq('root') } end @@ -126,6 +130,7 @@ it do expect(subject.to_hash).to match( a_hash_including( + author_id: 1, author_email: "joe.montana@49ers.com", author_username: "username", author: "root", diff --git a/spec/lib/bitbucket_server/representation/user_spec.rb b/spec/lib/bitbucket_server/representation/user_spec.rb index 32470e3a12ffc2..68ad20da455f83 100644 --- a/spec/lib/bitbucket_server/representation/user_spec.rb +++ b/spec/lib/bitbucket_server/representation/user_spec.rb @@ -3,12 +3,17 @@ require 'spec_helper' RSpec.describe BitbucketServer::Representation::User, feature_category: :importers do + let(:id) { 'id' } let(:email) { 'test@email.com' } let(:username) { 'test_user' } - let(:sample_data) { { 'user' => { 'emailAddress' => email, 'slug' => username } } } + let(:sample_data) { { 'user' => { 'id' => id, 'emailAddress' => email, 'slug' => username } } } subject(:user) { described_class.new(sample_data) } + describe '#id' do + it { expect(user.id).to eq(id) } + end + describe '#email' do it { expect(user.email).to eq(email) } end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb index 2ef04a16907386..5ce4555dbc3942 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 @@ -2,25 +2,25 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestImporter, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestImporter, feature_category: :importers do include AfterNextHelpers include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) + end - let_it_be(:author) { create(:user, username: 'username') } - let_it_be(:author_source_user) { generate_source_user(project, author) } - let_it_be(:reviewer_1) { create(:user, username: 'john_smith', email: 'john@smith.com') } - let_it_be(:reviewer_1_source_user) { generate_source_user(project, reviewer_1) } - let_it_be(:reviewer_2) { create(:user, username: 'jane_doe', email: 'jane@doe.com') } - let_it_be(:reviewer_2_source_user) { generate_source_user(project, reviewer_2) } + # Identifiers taken from importers/bitbucket_server/pull_request.json + let_it_be(:author_source_user) { generate_source_user(project, 1) } + let_it_be(:reviewer_1_source_user) { generate_source_user(project, 2) } + let_it_be(:reviewer_2_source_user) { generate_source_user(project, 3) } 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 @@ -34,9 +34,9 @@ title: pull_request.title, source_branch: 'root/CODE_OF_CONDUCTmd-1530600625006', target_branch: 'master', - reviewer_ids: [reviewer_1.id, reviewer_2.id], + reviewer_ids: [reviewer_1_source_user.mapped_user_id, reviewer_2_source_user.mapped_user_id], state: pull_request.state, - author_id: author.id, + author_id: author_source_user.mapped_user_id, description: pull_request.description, imported_from: 'bitbucket_server' ) @@ -71,35 +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 `bitbucket_server_user_mapping_by_username` & `bitbucket_server_user_mapping` flags are disabled' do - before do - stub_feature_flags( - bitbucket_server_convert_mentions_to_users: false, - bitbucket_server_user_mapping: 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) @@ -172,7 +143,13 @@ end context 'when user contribution mapping is disabled' do - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } + let_it_be(:author) { create(:user, username: 'username') } + 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| @@ -187,6 +164,50 @@ 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 `bitbucket_server_user_mapping_by_username` & `bitbucket_server_user_mapping` flags are disabled' do + before do + stub_feature_flags( + 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).to eq(author) + 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 da25b3dcd5901f..68770591469e99 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 @@ -2,29 +2,28 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::ApprovedEvent, feature_category: :importers do 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') + let_it_be_with_reload(:project) do + create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) end - let_it_be(:source_user) { generate_source_user(project, pull_request_author) } - - let(:approved_event) do + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:now) { Time.now.utc.change(usec: 0) } + let_it_be(:approved_event) do { id: 4, - approver_name: pull_request_author.name, - approver_username: pull_request_author.username, - approver_email: pull_request_author.email, + approver_id: 123, + 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_id]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -34,7 +33,7 @@ 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 'creates placeholder references', :aggregate_failures do + it 'pushes placeholder references' do importer.execute(approved_event) cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) @@ -53,19 +52,19 @@ 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 it 'logs its progress' do @@ -76,15 +75,29 @@ def expect_log(stage:, message:, iid:, event_id:) end context 'when user contribution mapping is disabled' do - let_it_be(:project) do - create( - :project, - :repository, - :bitbucket_server_import - ) + 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 - let_it_be(:merge_request) { create(:merge_request, source_project: project) } + 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) + + approval = merge_request.approvals.first + expect(approval.user_id).to eq(pull_request_author.id) + + note = merge_request.notes.first + expect(note.author_id).to eq(pull_request_author.id) + + reviewer = merge_request.reviewers.first + expect(reviewer.id).to eq(pull_request_author.id) + end context 'when a user with a matching username does not exist' do let(:approved_event) { super().merge(approver_username: 'another_username') } @@ -144,6 +157,13 @@ def expect_log(stage:, message:, iid:, event_id:) end end end + + it 'does not push placeholder references' do + 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 a8e937397ad56b..1b005a05b40cb2 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 @@ -2,25 +2,27 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::DeclinedEvent, feature_category: :importers do 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(:decliner_author) { create(:user, username: 'decliner_author', email: 'decliner_author@example.org') } - let_it_be(:source_user) { generate_source_user(project, decliner_author) } + 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_name: decliner_author.name, - decliner_username: decliner_author.username, - decliner_email: decliner_author.email, + decliner_id: 123, + 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_id]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -30,7 +32,7 @@ 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 'creates placeholder reference' do + it 'pushes placeholder references' do importer.execute(declined_event) cached_references = placeholder_user_references(::Import::SOURCE_BITBUCKET_SERVER, project.import_state.id) @@ -46,11 +48,11 @@ def expect_log(stage:, message:, iid:, event_id:) .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(decliner_author.id) + 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 @@ -65,8 +67,11 @@ def expect_log(stage:, message:, iid:, event_id:) end context 'when user contribution mapping is disabled' do - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } - let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:decliner_author) { create(:user, username: 'decliner_author', email: 'decliner_author@example.org') } + + 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 @@ -101,11 +106,18 @@ def expect_log(stage:, message:, iid:, event_id:) expect { importer.execute(another_user_event) } .to not_change { merge_request.events.count } - .and not_change { merge_request.resource_state_events.count } + .and not_change { merge_request.resource_state_events.count } expect(merge_request.metrics.reload.latest_closed_by).to be_nil end end + + it 'does not push placeholder references' do + 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 83cacc76a3a5fb..9f6a6dbdef0818 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 @@ -2,27 +2,21 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::Inline, feature_category: :importers do include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + 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(:reply_source_user) { generate_source_user(project, reply_author) } - - let_it_be(:inline_note_author) do - create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') - end - - let_it_be(:note_source_user) { generate_source_user(project, inline_note_author) } - - let(:reply) do + let_it_be(:reply) do { - author_email: reply_author.email, - author_username: reply_author.username, + author_id: 123, + author_email: 'reply_author@example.org', + author_username: 'reply_author', note: 'I agree', created_at: now, updated_at: now, @@ -30,7 +24,7 @@ } end - let(:pr_inline_comment) do + let_it_be(:pr_inline_comment) do { id: 7, file_type: 'ADDED', @@ -40,8 +34,9 @@ old_pos: nil, new_pos: 4, note: 'Hello world', - author_email: inline_note_author.email, - author_username: inline_note_author.username, + author_id: 234, + author_email: 'inline_note_author@example.org', + author_username: 'inline_note_author', comments: [reply], created_at: now, updated_at: now, @@ -49,6 +44,9 @@ } end + let_it_be(:reply_source_user) { generate_source_user(project, reply[:author_id]) } + let_it_be(:note_source_user) { generate_source_user(project, pr_inline_comment[:author_id]) } + before do allow(Gitlab::Import::MentionsConverter).to receive(:new).and_return(mentions_converter) end @@ -61,8 +59,8 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do - it 'creates placeholder reference', :aggregate_failures 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) @@ -87,11 +85,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 @@ -107,22 +105,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 @@ -153,15 +140,46 @@ def expect_log(stage:, message:, iid:, comment_id:) end 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')) + .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 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 8098928706e80e..94d83daadc6772 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 @@ -2,30 +2,26 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::MergeEvent, feature_category: :importers do 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(:source_user) { generate_source_user(project, pull_request_author) } - let_it_be(:merge_event) do { id: 3, - committer_user: pull_request_author.name, - committer_username: pull_request_author.username, - committer_email: pull_request_author.email, + committer_id: 123, + 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_id]) } + def expect_log(stage:, message:, iid:, event_id:) allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original expect(Gitlab::BitbucketServerImport::Logger) @@ -34,8 +30,8 @@ def expect_log(stage:, message:, iid:, event_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do - it 'creates placeholder references', :aggregate_failures 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) @@ -49,7 +45,7 @@ def expect_log(stage:, message:, iid:, event_id:) metrics = merge_request.metrics.reload - expect(metrics.merged_by).to eq(pull_request_author) + 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 @@ -60,5 +56,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 7fc1cbb5438a0a..5bb34abc9cb662 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 @@ -2,30 +2,35 @@ require 'spec_helper' -RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, :clean_gitlab_redis_shared_state, feature_category: :importers do +RSpec.describe Gitlab::BitbucketServerImport::Importers::PullRequestNotes::StandaloneNotes, feature_category: :importers do 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(:source_user) { generate_source_user(project, note_author) } let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } + let_it_be(:author_details) do + { + author_id: 123, + 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_name: note_author.name, - 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_id]) } + before do allow(Gitlab::Import::MentionsConverter).to receive(:new).and_return(mentions_converter) end @@ -38,8 +43,8 @@ def expect_log(stage:, message:, iid:, comment_id:) subject(:importer) { described_class.new(project, merge_request) } - describe '#execute' do - it 'creates placeholder reference' 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) @@ -56,7 +61,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' @@ -68,28 +73,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 @@ -100,14 +101,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' @@ -115,33 +116,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 @@ -181,5 +166,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 e13f90a32f21d6..bdc3a6097ee671 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 @@ -6,19 +6,15 @@ include AfterNextHelpers include Import::UserMappingHelper - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + let_it_be_with_reload(:project) do + 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(:note_source_user) { generate_source_user(project, note_author) } + 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!(:author_source_user) { generate_source_user(project, pull_request_author) } - let(:merge_event) do instance_double( BitbucketServer::Representation::Activity, @@ -26,9 +22,10 @@ comment?: false, merge_event?: true, approved_event?: false, - committer_name: pull_request_author.name, - committer_username: pull_request_author.username, - committer_email: pull_request_author.email, + committer_id: 123, + committer_name: 'Pull Request Author', + committer_username: 'pull_request_author', + committer_email: 'pull_request_author@example.com', merge_timestamp: now, merge_commit: '12345678' ) @@ -41,9 +38,10 @@ comment?: false, merge_event?: false, approved_event?: true, - approver_name: pull_request_author.name, - approver_username: pull_request_author.username, - approver_email: pull_request_author.email, + approver_id: 123, + approver_name: 'Pull Request Author', + approver_username: 'pull_request_author', + approver_email: 'pull_request_author@example.org', created_at: now ) end @@ -52,9 +50,10 @@ instance_double( BitbucketServer::Representation::Comment, note: 'Hello world', - author_name: note_author.name, - author_email: note_author.email, - author_username: note_author.username, + author_id: 234, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -71,6 +70,9 @@ comment: pr_note) end + let!(:author_source_user) { generate_source_user(project, merge_event.committer_id) } + let!(:note_source_user) { generate_source_user(project, pr_note.author_id) } + let_it_be(:sample) { RepoHelpers.sample_compare } let_it_be(:now) { Time.now.utc.change(usec: 0) } @@ -90,8 +92,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 @@ -104,9 +110,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([]) @@ -137,35 +141,22 @@ def expect_log(stage:, message:) expect(merge_request.notes.count).to eq(1) expect(merge_request.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 - importer.execute - - expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") - end - end - context 'when the note has a parent note' do let(:pr_note) do instance_double( BitbucketServer::Representation::Comment, note: 'Note', - author_name: note_author.name, - author_email: note_author.email, - author_username: note_author.username, + author_id: 234, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -177,9 +168,10 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Parent note', - author_name: note_author.name, - author_email: note_author.email, - author_username: note_author.username, + author_id: 234, + author_name: 'Note Author', + author_email: 'note_author@example.org', + author_username: 'note_author', comments: [], created_at: now, updated_at: now, @@ -215,21 +207,13 @@ def expect_log(stage:, message:) 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(:reply_source_user) { generate_source_user(project, reply_author) } - - let_it_be(:inline_note_author) do - create(:user, username: 'inline_note_author', email: 'inline_note_author@example.org') - end - - let_it_be(:note_source_user) { generate_source_user(project, inline_note_author) } - let(:reply) do instance_double( BitbucketServer::Representation::PullRequestComment, - author_name: reply_author.name, - author_email: reply_author.email, - author_username: reply_author.username, + author_id: 345, + author_name: 'Reply Author', + author_email: 'reply_author@example.org', + author_username: 'reply_author', note: 'I agree', created_at: now, updated_at: now, @@ -246,9 +230,10 @@ def expect_log(stage:, message:) old_pos: nil, new_pos: 4, note: 'Hello world', - author_name: inline_note_author.name, - author_email: inline_note_author.email, - author_username: inline_note_author.username, + author_id: 456, + 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, @@ -264,6 +249,9 @@ def expect_log(stage:, message:) comment: pr_inline_note) end + let_it_be(:reply_source_user) { generate_source_user(project, 345) } + let_it_be(:note_source_user) { generate_source_user(project, 456) } + before do allow_next(BitbucketServer::Client).to receive(:activities).and_return([pr_inline_comment]) end @@ -283,12 +271,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 @@ -335,7 +323,7 @@ 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 @@ -362,19 +350,19 @@ 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 it 'pushes placeholder references' do @@ -387,59 +375,9 @@ def expect_log(stage:, message:) ) end - context 'when a user with a matching username does not exist' do - before do - pull_request_author.update!(username: 'another_username') - 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 user contribution mapping is disabled' do - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import) } - - 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 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 - 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 @@ -492,5 +430,77 @@ def expect_log(stage:, message:) 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 + 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 8f1d7ff735957c..2973a5a3a8b6b9 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb @@ -5,11 +5,14 @@ RSpec.describe Gitlab::BitbucketServerImport::UserFinder, :clean_gitlab_redis_shared_state, feature_category: :importers do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :repository, :bitbucket_server_import, :import_user_mapping_enabled) } + 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 { + id: user.id, username: user.username, display_name: user.name } @@ -45,45 +48,46 @@ 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 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') + 2.times do + user_id = user_finder.find_user_id(by: :email, value: 'nobody@example.com') - expect(user_id).to be_nil + expect(user_id).to be_nil + end 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 + 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) + 2.times do + user_id = user_finder.find_user_id(by: :email, value: user.email) - expect(user_id).to eq(user.id) + expect(user_id).to eq(user.id) + end end - end - it 'caches and returns the user ID by username' do - expect(User).to receive(:find_by_username).once.and_call_original + 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) + 2.times do + user_id = user_finder.find_user_id(by: :username, value: user.username) - expect(user_id).to eq(user.id) + expect(user_id).to eq(user.id) + end end end end - end - - context 'when user contribution mapping is disabled' do - let(:created_id) { 1 } - let(:project) { build_stubbed(:project, :repository, :bitbucket_server_import, creator_id: created_id) } describe '#author_id' do it 'calls uid method' do @@ -94,10 +98,14 @@ end 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(created_id) + expect(user_finder.author_id(object)).to eq(123) end end end @@ -111,7 +119,7 @@ end end - context 'when provided object is a Comment representation object' do + 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) diff --git a/spec/support/helpers/import/user_mapping_helper.rb b/spec/support/helpers/import/user_mapping_helper.rb index fea64461c0ecd4..ab29be03654838 100644 --- a/spec/support/helpers/import/user_mapping_helper.rb +++ b/spec/support/helpers/import/user_mapping_helper.rb @@ -29,16 +29,15 @@ def placeholder_user_references(import_type, import_uid, limit = 100) end end - # Generate a source user that returns a provided user as a placeholder + # Generate a source user with a provided project and identifier # # @param project [Project] - # @param user [User] + # @param identifier [String, Integer] # @return [Import::SourceUser] - def generate_source_user(project, user) + def generate_source_user(project, identifier) create( :import_source_user, - source_user_identifier: user.username, - placeholder_user: user, + source_user_identifier: identifier, source_hostname: project.import_url, import_type: project.import_type, namespace: project.root_ancestor -- GitLab From 3bc1bdc19b03ef74f2e0f3ebe2cb8fe465b77e90 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Thu, 28 Nov 2024 15:52:47 +0000 Subject: [PATCH 5/7] Revert back to using the slug --- .../representation/activity.rb | 15 --------------- .../representation/comment.rb | 5 ----- .../representation/pull_request.rb | 5 ----- lib/bitbucket_server/representation/user.rb | 4 ---- .../importers/pull_request_importer.rb | 6 ++---- .../pull_request_notes/approved_event.rb | 7 +++---- .../base_note_diff_importer.rb | 5 ++--- .../pull_request_notes/declined_event.rb | 5 ++--- .../pull_request_notes/merge_event.rb | 3 +-- .../pull_request_notes/standalone_notes.rb | 4 ++-- .../importers/pull_request_notes_importer.rb | 19 ++++++++----------- .../bitbucket_server_import/user_finder.rb | 4 ++-- .../representation/activity_spec.rb | 6 ------ .../representation/comment_spec.rb | 5 ----- .../representation/pull_request_spec.rb | 5 ----- .../representation/user_spec.rb | 7 +------ .../importers/pull_request_importer_spec.rb | 6 +++--- .../pull_request_notes/approved_event_spec.rb | 3 +-- .../pull_request_notes/declined_event_spec.rb | 3 +-- .../pull_request_notes/inline_spec.rb | 6 ++---- .../pull_request_notes/merge_event_spec.rb | 3 +-- .../standalone_notes_spec.rb | 3 +-- .../pull_request_notes_importer_spec.rb | 15 ++++----------- .../user_finder_spec.rb | 1 - 24 files changed, 36 insertions(+), 109 deletions(-) diff --git a/lib/bitbucket_server/representation/activity.rb b/lib/bitbucket_server/representation/activity.rb index 687acbf67127a9..df77fd0d026d30 100644 --- a/lib/bitbucket_server/representation/activity.rb +++ b/lib/bitbucket_server/representation/activity.rb @@ -31,10 +31,6 @@ def merge_event? action == 'MERGED' end - def committer_id - commit.dig('committer', 'id') - end - def committer_user commit.dig('committer', 'displayName') end @@ -65,10 +61,6 @@ def approved_event? action == 'APPROVED' end - def approver_id - raw.dig('user', 'id') - end - def approver_name raw.dig('user', 'displayName') end @@ -85,10 +77,6 @@ def declined_event? action == 'DECLINED' end - def decliner_id - raw.dig('user', 'id') - end - def decliner_name raw.dig('user', 'displayName') end @@ -108,18 +96,15 @@ def created_at def to_hash { id: id, - committer_id: committer_id, committer_name: committer_user, committer_user: committer_user, committer_username: committer_username, committer_email: committer_email, merge_timestamp: merge_timestamp, merge_commit: merge_commit, - approver_id: approver_id, approver_name: approver_name, approver_username: approver_username, approver_email: approver_email, - decliner_id: decliner_id, decliner_name: decliner_name, decliner_username: decliner_username, decliner_email: decliner_email, diff --git a/lib/bitbucket_server/representation/comment.rb b/lib/bitbucket_server/representation/comment.rb index 6dfe8d3d781372..d2b0a42b9e6086 100644 --- a/lib/bitbucket_server/representation/comment.rb +++ b/lib/bitbucket_server/representation/comment.rb @@ -37,10 +37,6 @@ def id raw_comment['id'] end - def author_id - author['id'] - end - def author_name author['displayName'] end @@ -89,7 +85,6 @@ def to_hash { id: id, - author_id: author_id, author_name: author_name, author_email: author_email, author_username: author_username, diff --git a/lib/bitbucket_server/representation/pull_request.rb b/lib/bitbucket_server/representation/pull_request.rb index 8ac976a96c7272..996a10318f5e17 100644 --- a/lib/bitbucket_server/representation/pull_request.rb +++ b/lib/bitbucket_server/representation/pull_request.rb @@ -3,10 +3,6 @@ module BitbucketServer module Representation class PullRequest < Representation::Base - def author_id - raw.dig('author', 'user', 'id') - end - def author raw.dig('author', 'user', 'name') end @@ -83,7 +79,6 @@ def target_branch_sha def to_hash { iid: iid, - author_id: author_id, author: author, author_email: author_email, author_username: author_username, diff --git a/lib/bitbucket_server/representation/user.rb b/lib/bitbucket_server/representation/user.rb index 28d766439e1b3d..433baec1c42f96 100644 --- a/lib/bitbucket_server/representation/user.rb +++ b/lib/bitbucket_server/representation/user.rb @@ -3,10 +3,6 @@ module BitbucketServer module Representation class User < Representation::Base - def id - user['id'] - end - def email user['emailAddress'] end 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 b1864306e2a0d2..e0cab6d03fb93d 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -44,7 +44,7 @@ def execute creator = Gitlab::Import::MergeRequestCreator.new(project) merge_request = creator.execute(attributes) - push_reference(project, merge_request, :author_id, object[:author_id]) + 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 @@ -81,7 +81,6 @@ def author_line def author_id(pull_request_data) if user_mapping_enabled?(project) user_finder.author_id( - id: pull_request_data['author_id'], username: pull_request_data['author_username'], display_name: pull_request_data['author'] ) @@ -96,12 +95,11 @@ def reviewers object[:reviewers].filter_map do |reviewer_data| if user_mapping_enabled?(project) uid = user_finder.uid( - id: reviewer_data.dig('user', 'id'), username: reviewer_data.dig('user', 'slug'), display_name: reviewer_data.dig('user', 'displayName') ) - @reviewer_references[uid] = reviewer_data.dig('user', 'id') + @reviewer_references[uid] = reviewer_data.dig('user', 'slug') uid elsif Feature.enabled?(:bitbucket_server_user_mapping_by_username, project, type: :ops) 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 8593e2cdef8267..46de3450650f57 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 @@ -17,7 +17,6 @@ def execute(approved_event) user_id = if user_mapping_enabled?(project) user_finder.uid( - id: approved_event[:approver_id], username: approved_event[:approver_username], display_name: approved_event[:approver_name] ) @@ -41,11 +40,11 @@ def execute(approved_event) submitted_at = approved_event[:created_at] || merge_request[:updated_at] approval, approval_note = create_approval!(project.id, merge_request.id, user_id, submitted_at) - push_reference(project, approval, :user_id, approved_event[:approver_id]) - push_reference(project, approval_note, :author_id, approved_event[:approver_id]) + 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_id]) if reviewer + 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_note_diff_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes/base_note_diff_importer.rb index 21ab6cb3a544da..0e73959882f224 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,7 +28,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_id]) + push_reference(project, note, :author_id, comment[:author_username]) return note end @@ -84,7 +84,6 @@ def pull_request_comment_attributes(comment) def author(comment) if user_mapping_enabled?(project) user_finder.uid( - id: comment[:author_id], username: comment[:author_username], display_name: comment[:author_name] ) @@ -104,7 +103,7 @@ def create_basic_fallback_note(merge_request, comment, position) attributes[:note] = note_text note = merge_request.notes.create!(attributes) - push_reference(project, note, :author_id, comment[:author_id]) + push_reference(project, note, :author_id, comment[:author_username]) note 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 adc16c6733a7c5..c8f9bec3fcbf00 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 @@ -17,7 +17,6 @@ def execute(declined_event) user_id = if user_mapping_enabled?(project) user_finder.uid( - id: declined_event[:decliner_id], username: declined_event[:decliner_username], display_name: declined_event[:decliner_name] ) @@ -45,13 +44,13 @@ def execute(declined_event) SystemNoteService.change_status(merge_request, merge_request.target_project, user, 'closed', nil) event = record_event(user_id) - push_reference(project, event, :author_id, declined_event[:decliner_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_id]) + push_reference(project, metric, :latest_closed_by_id, declined_event[:decliner_username]) log_info( import_stage: 'import_declined_event', 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 a84246c2d433e5..1e0b28b4c64c71 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 @@ -17,7 +17,6 @@ def execute(merge_event) user_id = if user_mapping_enabled?(project) user_finder.uid( - id: merge_event[:committer_id], username: merge_event[:committer_username], display_name: merge_event[:committer_user] ) @@ -30,7 +29,7 @@ def execute(merge_event) timestamp = merge_event[:merge_timestamp] merge_request.update({ merge_commit_sha: merge_event[:merge_commit] }) metric = create_merge_request_metrics(merged_by_id: user_id, merged_at: timestamp) - push_reference(project, metric, :merged_by_id, merge_event[:committer_id]) + 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 82e23a232bf9ed..a8b0fa26bc8fd7 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 @@ -14,11 +14,11 @@ def execute(comment) ) note = merge_request.notes.create!(pull_request_comment_attributes(comment)) - push_reference(note.project, note, :author_id, comment[:author_id]) + push_reference(note.project, note, :author_id, comment[:author_username]) comment[:comments].each do |reply| note = merge_request.notes.create!(pull_request_comment_attributes(reply)) - push_reference(note.project, note, :author_id, reply[:author_id]) + 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 a067da93e6420f..36b4d1a1931ce1 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 @@ -59,7 +59,6 @@ def import_merge_event(merge_request, merge_event) user_id = if user_mapping_enabled?(project) user_finder.uid( - id: merge_event.committer_id, username: merge_event.committer_username, display_name: merge_event.committer_name ) @@ -74,7 +73,7 @@ def import_merge_event(merge_request, merge_event) 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_id) + 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 @@ -90,7 +89,6 @@ def import_approved_event(merge_request, approved_event) user_id = if user_mapping_enabled?(project) user_finder.uid( - id: approved_event.approver_id, username: approved_event.approver_username, display_name: approved_event.approver_name ) @@ -105,11 +103,11 @@ def import_approved_event(merge_request, approved_event) submitted_at = approved_event.created_at || merge_request.updated_at approval, approval_note = create_approval!(project.id, merge_request.id, user_id, submitted_at) - push_reference(project, approval, :user_id, approved_event.approver_id) - push_reference(project, approval_note, :author_id, approved_event.approver_id) + 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_id) if reviewer + push_reference(project, reviewer, :user_id, approved_event.approver_username) if reviewer log_info( import_stage: 'import_approved_event', @@ -147,7 +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_id) + push_reference(project, note, :author_id, comment.author_username) return note end @@ -176,7 +174,7 @@ def create_fallback_diff_note(merge_request, comment, position) attributes[:note] = note note = merge_request.notes.create!(attributes) - push_reference(project, note, :author_id, comment.author_id) + push_reference(project, note, :author_id, comment.author_username) end def build_position(merge_request, pr_comment) @@ -196,11 +194,11 @@ def import_standalone_pr_comments(pr_comments, merge_request) pr_comments.each do |comment| note = merge_request.notes.create!(pull_request_comment_attributes(comment)) - push_reference(project, note, :author_id, comment.author_id) + push_reference(project, note, :author_id, comment.author_username) comment.comments.each do |replies| note = merge_request.notes.create!(pull_request_comment_attributes(replies)) - push_reference(project, note, :author_id, comment.author_id) + push_reference(project, note, :author_id, comment.author_username) end rescue StandardError => e Gitlab::ErrorTracking.log_exception( @@ -251,7 +249,6 @@ def pull_request_comment_attributes(comment) def author(comment) if user_mapping_enabled?(project) user_finder.uid( - id: comment.author_id, username: comment.author_username, display_name: comment.author_name ) diff --git a/lib/gitlab/bitbucket_server_import/user_finder.rb b/lib/gitlab/bitbucket_server_import/user_finder.rb index f1f9f69752f89e..b3031d48112546 100644 --- a/lib/gitlab/bitbucket_server_import/user_finder.rb +++ b/lib/gitlab/bitbucket_server_import/user_finder.rb @@ -64,12 +64,12 @@ def build_cache_key(by, value) end def user_mapping_enabled?(project) - !!project.import_data&.user_mapping_enabled? + !!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[:id], + source_user_identifier: user_data[:username], source_name: user_data[:display_name], source_username: user_data[:username] ) diff --git a/spec/lib/bitbucket_server/representation/activity_spec.rb b/spec/lib/bitbucket_server/representation/activity_spec.rb index fa436e4296077c..32899050a04a6f 100644 --- a/spec/lib/bitbucket_server/representation/activity_spec.rb +++ b/spec/lib/bitbucket_server/representation/activity_spec.rb @@ -48,7 +48,6 @@ it { expect(subject.id).to eq(7) } it { expect(subject.comment?).to be_falsey } it { expect(subject.inline_comment?).to be_falsey } - it { expect(subject.committer_id).to eq(1) } it { expect(subject.committer_user).to eq('root') } it { expect(subject.committer_name).to eq('root') } it { expect(subject.committer_username).to eq('slug') } @@ -62,7 +61,6 @@ expect(subject.to_hash).to match( a_hash_including( id: 7, - committer_id: 1, committer_user: 'root', committer_name: 'root', committer_username: 'slug', @@ -82,7 +80,6 @@ it { expect(subject.inline_comment?).to be_falsey } it { expect(subject.merge_event?).to be_falsey } it { expect(subject.approved_event?).to be_truthy } - it { expect(subject.approver_id).to eq(1) } it { expect(subject.approver_name).to eq('root') } it { expect(subject.approver_username).to eq('slug') } it { expect(subject.approver_email).to eq('test.user@example.com') } @@ -93,7 +90,6 @@ expect(subject.to_hash).to match( a_hash_including( id: 15, - approver_id: 1, approver_name: 'root', approver_username: 'slug', approver_email: 'test.user@example.com' @@ -111,7 +107,6 @@ it { expect(subject.inline_comment?).to be_falsey } it { expect(subject.merge_event?).to be_falsey } it { expect(subject.declined_event?).to be_truthy } - it { expect(subject.decliner_id).to eq(1) } it { expect(subject.decliner_name).to eq('root') } it { expect(subject.decliner_username).to eq('slug') } it { expect(subject.decliner_email).to eq('test.user@example.com') } @@ -122,7 +117,6 @@ expect(subject.to_hash).to match( a_hash_including( id: 18, - decliner_id: 1, decliner_name: 'root', decliner_username: 'slug', decliner_email: 'test.user@example.com' diff --git a/spec/lib/bitbucket_server/representation/comment_spec.rb b/spec/lib/bitbucket_server/representation/comment_spec.rb index cceb9240a5bb48..7e32da8a7f7b0a 100644 --- a/spec/lib/bitbucket_server/representation/comment_spec.rb +++ b/spec/lib/bitbucket_server/representation/comment_spec.rb @@ -12,10 +12,6 @@ it { expect(comment_representation.id).to eq(9) } end - describe '#author_id' do - it { expect(comment_representation.author_id).to eq(1) } - end - describe '#author_name' do it { expect(comment_representation.author_name).to eq('root') } end @@ -91,7 +87,6 @@ expect(comment_representation.to_hash).to match( a_hash_including( id: 9, - author_id: 1, author_name: 'root', author_email: 'test.user@example.com', author_username: 'username', diff --git a/spec/lib/bitbucket_server/representation/pull_request_spec.rb b/spec/lib/bitbucket_server/representation/pull_request_spec.rb index c558404000df7e..2d67dd88b241ef 100644 --- a/spec/lib/bitbucket_server/representation/pull_request_spec.rb +++ b/spec/lib/bitbucket_server/representation/pull_request_spec.rb @@ -7,10 +7,6 @@ subject { described_class.new(sample_data) } - describe '#author_id' do - it { expect(subject.author_id).to eq(1) } - end - describe '#author' do it { expect(subject.author).to eq('root') } end @@ -130,7 +126,6 @@ it do expect(subject.to_hash).to match( a_hash_including( - author_id: 1, author_email: "joe.montana@49ers.com", author_username: "username", author: "root", diff --git a/spec/lib/bitbucket_server/representation/user_spec.rb b/spec/lib/bitbucket_server/representation/user_spec.rb index 68ad20da455f83..32470e3a12ffc2 100644 --- a/spec/lib/bitbucket_server/representation/user_spec.rb +++ b/spec/lib/bitbucket_server/representation/user_spec.rb @@ -3,17 +3,12 @@ require 'spec_helper' RSpec.describe BitbucketServer::Representation::User, feature_category: :importers do - let(:id) { 'id' } let(:email) { 'test@email.com' } let(:username) { 'test_user' } - let(:sample_data) { { 'user' => { 'id' => id, 'emailAddress' => email, 'slug' => username } } } + let(:sample_data) { { 'user' => { 'emailAddress' => email, 'slug' => username } } } subject(:user) { described_class.new(sample_data) } - describe '#id' do - it { expect(user.id).to eq(id) } - end - describe '#email' do it { expect(user.email).to eq(email) } end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb index 5ce4555dbc3942..3dc4f20d73c526 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 @@ -11,9 +11,9 @@ end # Identifiers taken from importers/bitbucket_server/pull_request.json - let_it_be(:author_source_user) { generate_source_user(project, 1) } - let_it_be(:reviewer_1_source_user) { generate_source_user(project, 2) } - let_it_be(:reviewer_2_source_user) { generate_source_user(project, 3) } + 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) } 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 68770591469e99..37e929c19623b6 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 @@ -14,7 +14,6 @@ let_it_be(:approved_event) do { id: 4, - approver_id: 123, approver_name: 'John Approvals', approver_username: 'pull_request_author', approver_email: 'pull_request_author@example.org', @@ -22,7 +21,7 @@ } end - let_it_be(:source_user) { generate_source_user(project, approved_event[:approver_id]) } + 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 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 1b005a05b40cb2..4cc6f844e58307 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 @@ -13,7 +13,6 @@ let_it_be(:declined_event) do { id: 7, - decliner_id: 123, decliner_name: 'John Rejections', decliner_username: 'decliner_author', decliner_email: 'decliner_author@example.org', @@ -21,7 +20,7 @@ } end - let_it_be(:source_user) { generate_source_user(project, declined_event[:decliner_id]) } + 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 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 9f6a6dbdef0818..467dcc52f86bad 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 @@ -14,7 +14,6 @@ let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } let_it_be(:reply) do { - author_id: 123, author_email: 'reply_author@example.org', author_username: 'reply_author', note: 'I agree', @@ -34,7 +33,6 @@ old_pos: nil, new_pos: 4, note: 'Hello world', - author_id: 234, author_email: 'inline_note_author@example.org', author_username: 'inline_note_author', comments: [reply], @@ -44,8 +42,8 @@ } end - let_it_be(:reply_source_user) { generate_source_user(project, reply[:author_id]) } - let_it_be(:note_source_user) { generate_source_user(project, pr_inline_comment[:author_id]) } + 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) 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 94d83daadc6772..ef3bc4146a24e7 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 @@ -11,7 +11,6 @@ let_it_be(:merge_event) do { id: 3, - committer_id: 123, committer_user: 'John Merges', committer_username: 'pull_request_author', committer_email: 'pull_request_author@example.org', @@ -20,7 +19,7 @@ } end - let_it_be(:source_user) { generate_source_user(project, merge_event[:committer_id]) } + 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 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 5bb34abc9cb662..01fd4843f03886 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 @@ -11,7 +11,6 @@ let_it_be(:mentions_converter) { Gitlab::Import::MentionsConverter.new('bitbucket_server', project) } let_it_be(:author_details) do { - author_id: 123, author_name: 'John Notes', author_username: 'note_author', author_email: 'note_author@example.org' @@ -29,7 +28,7 @@ }.merge(author_details) end - let_it_be(:source_user) { generate_source_user(project, pr_comment[:author_id]) } + 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) 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 bdc3a6097ee671..09ef38b1e30f62 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 @@ -22,7 +22,6 @@ comment?: false, merge_event?: true, approved_event?: false, - committer_id: 123, committer_name: 'Pull Request Author', committer_username: 'pull_request_author', committer_email: 'pull_request_author@example.com', @@ -38,7 +37,6 @@ comment?: false, merge_event?: false, approved_event?: true, - approver_id: 123, approver_name: 'Pull Request Author', approver_username: 'pull_request_author', approver_email: 'pull_request_author@example.org', @@ -50,7 +48,6 @@ instance_double( BitbucketServer::Representation::Comment, note: 'Hello world', - author_id: 234, author_name: 'Note Author', author_email: 'note_author@example.org', author_username: 'note_author', @@ -70,8 +67,8 @@ comment: pr_note) end - let!(:author_source_user) { generate_source_user(project, merge_event.committer_id) } - let!(:note_source_user) { generate_source_user(project, pr_note.author_id) } + 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) } @@ -153,7 +150,6 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Note', - author_id: 234, author_name: 'Note Author', author_email: 'note_author@example.org', author_username: 'note_author', @@ -168,7 +164,6 @@ def expect_log(stage:, message:) instance_double( BitbucketServer::Representation::Comment, note: 'Parent note', - author_id: 234, author_name: 'Note Author', author_email: 'note_author@example.org', author_username: 'note_author', @@ -210,7 +205,6 @@ def expect_log(stage:, message:) let(:reply) do instance_double( BitbucketServer::Representation::PullRequestComment, - author_id: 345, author_name: 'Reply Author', author_email: 'reply_author@example.org', author_username: 'reply_author', @@ -230,7 +224,6 @@ def expect_log(stage:, message:) old_pos: nil, new_pos: 4, note: 'Hello world', - author_id: 456, author_name: 'Inline Note Author', author_email: 'inline_note_author@example.org', author_username: 'inline_note_author', @@ -249,8 +242,8 @@ def expect_log(stage:, message:) comment: pr_inline_note) end - let_it_be(:reply_source_user) { generate_source_user(project, 345) } - let_it_be(:note_source_user) { generate_source_user(project, 456) } + 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]) 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 2973a5a3a8b6b9..63f0e26f8e40ef 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_finder_spec.rb @@ -12,7 +12,6 @@ let(:source_user) { build_stubbed(:import_source_user, :completed) } let(:user_representation) do { - id: user.id, username: user.username, display_name: user.name } -- GitLab From 9654305544d5c047fc3652eb4ae68e74b774b058 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Tue, 3 Dec 2024 08:38:43 +0000 Subject: [PATCH 6/7] Fix flaky spec --- .../importers/pull_request_importer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3dc4f20d73c526..a90ec39731b5d3 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 @@ -34,7 +34,7 @@ title: pull_request.title, source_branch: 'root/CODE_OF_CONDUCTmd-1530600625006', target_branch: 'master', - reviewer_ids: [reviewer_1_source_user.mapped_user_id, reviewer_2_source_user.mapped_user_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: author_source_user.mapped_user_id, description: pull_request.description, -- GitLab From 6c87bada41115dab202e2122b14b009208d726e2 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Tue, 3 Dec 2024 11:19:03 +0000 Subject: [PATCH 7/7] Fix undercoverage issues --- .../importers/pull_request_notes_importer.rb | 8 +- .../importers/pull_request_importer_spec.rb | 6 +- .../pull_request_notes_importer_spec.rb | 170 +++++++++++++++++- 3 files changed, 172 insertions(+), 12 deletions(-) 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 36b4d1a1931ce1..ad00a8926e4ae5 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 @@ -175,6 +175,7 @@ def create_fallback_diff_note(merge_request, comment, position) attributes[:note] = note note = merge_request.notes.create!(attributes) push_reference(project, note, :author_id, comment.author_username) + note end def build_position(merge_request, pr_comment) @@ -222,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 @@ -246,6 +247,11 @@ 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( 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 a90ec39731b5d3..3dd55e9cfc082f 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 @@ -143,7 +143,6 @@ end context 'when user contribution mapping is disabled' do - let_it_be(:author) { create(:user, username: 'username') } 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') } @@ -177,9 +176,10 @@ end end - context 'when `bitbucket_server_user_mapping_by_username` & `bitbucket_server_user_mapping` flags are disabled' do + 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 ) @@ -190,7 +190,7 @@ merge_request = project.merge_requests.find_by_iid(pull_request.iid) - expect(merge_request.author).to eq(author) + expect(merge_request.author_id).to eq(project.creator_id) end it 'imports reviewers correctly' do 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 09ef38b1e30f62..a97f42e121e70e 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 @@ -47,10 +47,24 @@ let(:pr_note) do instance_double( BitbucketServer::Representation::Comment, + id: 456, note: 'Hello world', 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, @@ -126,23 +140,31 @@ def expect_log(stage:, message:) 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], + ["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 { importer.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_id: note_source_user.mapped_user_id, created_at: pr_note.created_at, updated_at: pr_note.created_at, imported_from: 'bitbucket_server' ) + + 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 @@ -181,6 +203,26 @@ def expect_log(stage:, message:) 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) @@ -201,7 +243,7 @@ def expect_log(stage:, message:) end end - context 'when PR has threaded discussion' do + context 'when PR has threaded inline discussion' do let(:reply) do instance_double( BitbucketServer::Representation::PullRequestComment, @@ -217,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, @@ -250,8 +293,6 @@ def expect_log(stage:, message:) end it 'imports the threaded discussion' do - expect(mentions_converter).to receive(:convert).and_call_original.twice - expect { importer.execute }.to change { Note.count }.by(2) expect(merge_request.discussions.count).to eq(1) @@ -286,6 +327,55 @@ def expect_log(stage:, message:) ) 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) @@ -493,6 +583,70 @@ def expect_log(stage:, message:) 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 -- GitLab