From 816a84d47d58dcf0a5899da5bf4e8705b9dd115c Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 21 Jul 2025 16:56:49 -0400 Subject: [PATCH 1/6] Map contributions to personal namespace owner For projects imported to personal namespaces from GitHub, immediately map all user contributions to the personal namespace owner when user_mapping_to_personal_namespace_owner feature flag is enabled. --- .../github_import/advance_stage_worker.rb | 5 +- .../stage/finish_import_worker.rb | 7 +- .../github_import/contributions_mapper.rb | 27 ---- .../importer/diff_note_importer.rb | 13 +- .../importer/events/base_importer.rb | 5 +- .../importer/events/changed_assignee.rb | 4 +- .../importer/events/changed_label.rb | 2 +- .../importer/events/changed_milestone.rb | 4 +- .../importer/events/changed_reviewer.rb | 4 +- .../github_import/importer/events/closed.rb | 6 +- .../importer/events/cross_referenced.rb | 4 +- .../github_import/importer/events/merged.rb | 7 +- .../github_import/importer/events/renamed.rb | 2 +- .../github_import/importer/events/reopened.rb | 8 +- .../github_import/importer/issue_importer.rb | 18 +-- .../github_import/importer/note_importer.rb | 7 +- .../importer/protected_branch_importer.rb | 11 +- .../importer/pull_request_importer.rb | 15 +- .../pull_requests/merged_by_importer.rb | 9 +- .../importer/pull_requests/review_importer.rb | 19 +-- .../pull_requests/review_request_importer.rb | 7 +- .../importer/releases_importer.rb | 17 +-- .../push_placeholder_references.rb | 69 --------- lib/gitlab/github_import/settings.rb | 14 ++ lib/gitlab/github_import/user_finder.rb | 37 +++-- lib/import/placeholder_references/pusher.rb | 82 ++++++++-- .../contributions_mapper_spec.rb | 37 ----- .../push_placeholder_references_spec.rb | 144 ------------------ .../lib/gitlab/github_import/settings_spec.rb | 77 ++++++++++ .../gitlab/github_import/user_finder_spec.rb | 66 +++++++- .../advance_stage_worker_spec.rb | 38 ++++- .../stage/finish_import_worker_spec.rb | 57 ++++++- 32 files changed, 404 insertions(+), 418 deletions(-) delete mode 100644 lib/gitlab/github_import/contributions_mapper.rb delete mode 100644 lib/gitlab/github_import/push_placeholder_references.rb delete mode 100644 spec/lib/gitlab/github_import/contributions_mapper_spec.rb delete mode 100644 spec/lib/gitlab/github_import/push_placeholder_references_spec.rb diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index bc85e7a88c9e7f..721276397392c3 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -47,7 +47,10 @@ def proceed_to_next_stage(import_state_jid, next_stage, project_id) project = Project.find_by_id(project_id) import_settings = Gitlab::GithubImport::Settings.new(project) - load_references(project) if import_settings.user_mapping_enabled? + map_to_personal_namespace_owner = project.root_ancestor.user_namespace? && + import_settings.user_mapping_to_personal_namespace_owner_enabled? + + load_references(project) if import_settings.user_mapping_enabled? && !map_to_personal_namespace_owner super end diff --git a/app/workers/gitlab/github_import/stage/finish_import_worker.rb b/app/workers/gitlab/github_import/stage/finish_import_worker.rb index 42054a22c7f627..84f8b73e40f736 100644 --- a/app/workers/gitlab/github_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/github_import/stage/finish_import_worker.rb @@ -27,7 +27,7 @@ def import(_, project) def reference_store_pending? return false unless import_settings(project).user_mapping_enabled? - + return false if map_to_personal_namespace_owner? return false unless placeholder_reference_store.any? ::Import::LoadPlaceholderReferencesWorker.perform_async( @@ -62,6 +62,11 @@ def metrics def placeholder_reference_store @placeholder_reference_store ||= project.placeholder_reference_store end + + def map_to_personal_namespace_owner? + project.root_ancestor.user_namespace? && + import_settings(project).user_mapping_to_personal_namespace_owner_enabled? + end end end end diff --git a/lib/gitlab/github_import/contributions_mapper.rb b/lib/gitlab/github_import/contributions_mapper.rb deleted file mode 100644 index 686957696fb025..00000000000000 --- a/lib/gitlab/github_import/contributions_mapper.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module GithubImport - class ContributionsMapper - def initialize(project) - @project = project - end - - def user_mapper - ::Gitlab::Import::SourceUserMapper.new( - namespace: project.root_ancestor, - source_hostname: project.safe_import_url, - import_type: ::Import::SOURCE_GITHUB - ) - end - - def user_mapping_enabled? - Gitlab::GithubImport::Settings.new(project).user_mapping_enabled? - end - - private - - attr_reader :project - end - end -end diff --git a/lib/gitlab/github_import/importer/diff_note_importer.rb b/lib/gitlab/github_import/importer/diff_note_importer.rb index 8a236186245c41..223a286774825a 100644 --- a/lib/gitlab/github_import/importer/diff_note_importer.rb +++ b/lib/gitlab/github_import/importer/diff_note_importer.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Importer class DiffNoteImporter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher DiffNoteCreationError = Class.new(ActiveRecord::RecordInvalid) @@ -15,7 +15,6 @@ def initialize(note, project, client) @note = note @project = project @client = client - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -43,7 +42,7 @@ def execute private - attr_reader :note, :project, :client, :author_id, :author_found, :mapper + attr_reader :note, :project, :client, :author_id, :author_found def build_author_attributes @author_id, @author_found = user_finder.author_id_for(note) @@ -83,9 +82,7 @@ def import_with_legacy_diff_note ids = ApplicationRecord.legacy_bulk_insert(LegacyDiffNote.table_name, [attributes], return_ids: true) - return unless mapper.user_mapping_enabled? - - push_refs_with_ids(ids, LegacyDiffNote, note[:author]&.id, mapper.user_mapper) + push_references_by_ids(project, ids, LegacyDiffNote, :author_id, note[:author]&.id) end # rubocop:enabled Gitlab/BulkInsert @@ -109,9 +106,7 @@ def import_with_diff_note raise DiffNoteCreationError, record unless record.persisted? - return unless mapper.user_mapping_enabled? - - push_with_record(record, :author_id, note.author&.id, mapper.user_mapper) + push_reference(project, record, :author_id, note.author&.id) end def note_body diff --git a/lib/gitlab/github_import/importer/events/base_importer.rb b/lib/gitlab/github_import/importer/events/base_importer.rb index 8b9fd022e207a2..f4162f8dda613a 100644 --- a/lib/gitlab/github_import/importer/events/base_importer.rb +++ b/lib/gitlab/github_import/importer/events/base_importer.rb @@ -6,7 +6,7 @@ module Importer module Events # Base class for importing issue events during project import from GitHub class BaseImporter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher # project - An instance of `Project`. # client - An instance of `Gitlab::GithubImport::Client`. @@ -14,7 +14,6 @@ def initialize(project, client) @project = project @client = client @user_finder = UserFinder.new(project, client) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end # issue_event - An instance of `Gitlab::GithubImport::Representation::IssueEvent`. @@ -24,7 +23,7 @@ def execute(issue_event) private - attr_reader :project, :user_finder, :client, :mapper + attr_reader :project, :user_finder, :client def author_id(issue_event, author_key: :actor) user_finder.author_id_for(issue_event, author_key: author_key).first diff --git a/lib/gitlab/github_import/importer/events/changed_assignee.rb b/lib/gitlab/github_import/importer/events/changed_assignee.rb index 7a104b32c68b4f..96f004fa23e0e6 100644 --- a/lib/gitlab/github_import/importer/events/changed_assignee.rb +++ b/lib/gitlab/github_import/importer/events/changed_assignee.rb @@ -12,9 +12,7 @@ def execute(issue_event) created_note = create_note(issue_event, note_body, author_id) - return unless mapper.user_mapping_enabled? - - push_with_record(created_note, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_note, :author_id, issue_event[:actor]&.id) end private diff --git a/lib/gitlab/github_import/importer/events/changed_label.rb b/lib/gitlab/github_import/importer/events/changed_label.rb index dda2ca6dd71f93..22f55b6d7945ab 100644 --- a/lib/gitlab/github_import/importer/events/changed_label.rb +++ b/lib/gitlab/github_import/importer/events/changed_label.rb @@ -25,7 +25,7 @@ def create_event(issue_event) return unless mapper.user_mapping_enabled? - push_with_record(created_event, :user_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_event, :user_id, issue_event[:actor]&.id) end def label_finder diff --git a/lib/gitlab/github_import/importer/events/changed_milestone.rb b/lib/gitlab/github_import/importer/events/changed_milestone.rb index 0e73cf0d1654d2..0c3846968d6844 100644 --- a/lib/gitlab/github_import/importer/events/changed_milestone.rb +++ b/lib/gitlab/github_import/importer/events/changed_milestone.rb @@ -32,9 +32,7 @@ def create_event(issue_event) created_event = ResourceMilestoneEvent.create!(attrs) - return unless mapper.user_mapping_enabled? - - push_with_record(created_event, :user_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_event, :user_id, issue_event[:actor]&.id) end def action(event_type) diff --git a/lib/gitlab/github_import/importer/events/changed_reviewer.rb b/lib/gitlab/github_import/importer/events/changed_reviewer.rb index 258172fecc51b6..e8faebf2be8900 100644 --- a/lib/gitlab/github_import/importer/events/changed_reviewer.rb +++ b/lib/gitlab/github_import/importer/events/changed_reviewer.rb @@ -34,9 +34,7 @@ def create_note(issue_event, review_requester_id) imported_from: imported_from ) - return unless mapper.user_mapping_enabled? - - push_with_record(created_note, :author_id, issue_event[:review_requester]&.id, mapper.user_mapper) + push_reference(project, created_note, :author_id, issue_event[:review_requester]&.id) end def parse_body(issue_event) diff --git a/lib/gitlab/github_import/importer/events/closed.rb b/lib/gitlab/github_import/importer/events/closed.rb index 951b305383940c..1a1a4bed963a40 100644 --- a/lib/gitlab/github_import/importer/events/closed.rb +++ b/lib/gitlab/github_import/importer/events/closed.rb @@ -26,9 +26,7 @@ def create_event(issue_event) imported_from: imported_from ) - return unless mapper.user_mapping_enabled? - - push_with_record(created_event, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_event, :author_id, issue_event[:actor]&.id) end def create_state_event(issue_event) @@ -47,7 +45,7 @@ def create_state_event(issue_event) return unless mapper.user_mapping_enabled? - push_with_record(state_event, :user_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, state_event, :user_id, issue_event[:actor]&.id) end end end diff --git a/lib/gitlab/github_import/importer/events/cross_referenced.rb b/lib/gitlab/github_import/importer/events/cross_referenced.rb index 6828ba9f2f9b2d..0369d6ee3de76b 100644 --- a/lib/gitlab/github_import/importer/events/cross_referenced.rb +++ b/lib/gitlab/github_import/importer/events/cross_referenced.rb @@ -46,9 +46,7 @@ def create_note(issue_event, note_body, user_id) imported_from: imported_from ) - return created_note unless mapper.user_mapping_enabled? - - push_with_record(created_note, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_note, :author_id, issue_event[:actor]&.id) created_note end diff --git a/lib/gitlab/github_import/importer/events/merged.rb b/lib/gitlab/github_import/importer/events/merged.rb index 382463f0d7eb87..29dab4530a1b2b 100644 --- a/lib/gitlab/github_import/importer/events/merged.rb +++ b/lib/gitlab/github_import/importer/events/merged.rb @@ -26,9 +26,8 @@ def create_event(issue_event) updated_at: issue_event.created_at, imported_from: imported_from ) - return unless mapper.user_mapping_enabled? - push_with_record(event, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, event, :author_id, issue_event[:actor]&.id) end def create_state_event(issue_event) @@ -45,9 +44,7 @@ def create_state_event(issue_event) state_event = ResourceStateEvent.create!(attrs) - return unless mapper.user_mapping_enabled? - - push_with_record(state_event, :user_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, state_event, :user_id, issue_event[:actor]&.id) end def create_note(issue_event) diff --git a/lib/gitlab/github_import/importer/events/renamed.rb b/lib/gitlab/github_import/importer/events/renamed.rb index 5c84dc7781a4c5..9bc3f737d399fc 100644 --- a/lib/gitlab/github_import/importer/events/renamed.rb +++ b/lib/gitlab/github_import/importer/events/renamed.rb @@ -10,7 +10,7 @@ def execute(issue_event) return unless mapper.user_mapping_enabled? - push_with_record(created_note, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_note, :author_id, issue_event[:actor]&.id) end private diff --git a/lib/gitlab/github_import/importer/events/reopened.rb b/lib/gitlab/github_import/importer/events/reopened.rb index e034f697c76bb8..8391c43cf7f25f 100644 --- a/lib/gitlab/github_import/importer/events/reopened.rb +++ b/lib/gitlab/github_import/importer/events/reopened.rb @@ -26,9 +26,7 @@ def create_event(issue_event) imported_from: imported_from ) - return unless mapper.user_mapping_enabled? - - push_with_record(created_event, :author_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, created_event, :author_id, issue_event[:actor]&.id) end def create_state_event(issue_event) @@ -41,9 +39,7 @@ def create_state_event(issue_event) state_event = ResourceStateEvent.create!(attrs) - return unless mapper.user_mapping_enabled? - - push_with_record(state_event, :user_id, issue_event[:actor]&.id, mapper.user_mapper) + push_reference(project, state_event, :user_id, issue_event[:actor]&.id) end end end diff --git a/lib/gitlab/github_import/importer/issue_importer.rb b/lib/gitlab/github_import/importer/issue_importer.rb index 911fae5bf1ba50..f68ee9e916ffc7 100644 --- a/lib/gitlab/github_import/importer/issue_importer.rb +++ b/lib/gitlab/github_import/importer/issue_importer.rb @@ -4,10 +4,9 @@ module Gitlab module GithubImport module Importer class IssueImporter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher - attr_reader :project, :issue, :client, :user_finder, :milestone_finder, - :issuable_finder, :mapper + attr_reader :project, :issue, :client, :user_finder, :milestone_finder, :issuable_finder # Imports an issue if it's a regular issue and not a pull request. def self.import_if_issue(issue, project, client) @@ -24,7 +23,6 @@ def initialize(issue, project, client) @user_finder = GithubImport::UserFinder.new(project, client) @milestone_finder = MilestoneFinder.new(project) @issuable_finder = GithubImport::IssuableFinder.new(project, issue) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -76,21 +74,17 @@ def create_issue end def push_issue_placeholder_references(new_issue) - return unless mapper.user_mapping_enabled? - - user_mapper = mapper.user_mapper - - push_with_record(new_issue, :author_id, issue.author&.id, user_mapper) + push_reference(project, new_issue, :author_id, issue.author&.id) new_issue.issue_assignees.each do |issue_assignee| github_user_id = issue_assignee_map[issue_assignee.user_id] - push_with_composite_key( + push_reference_with_composite_key( + project, issue_assignee, :user_id, { 'user_id' => issue_assignee.user_id, 'issue_id' => issue_assignee.issue_id }, - github_user_id, - user_mapper + github_user_id ) end end diff --git a/lib/gitlab/github_import/importer/note_importer.rb b/lib/gitlab/github_import/importer/note_importer.rb index 5727a6321f9481..d8858b5bd61aa9 100644 --- a/lib/gitlab/github_import/importer/note_importer.rb +++ b/lib/gitlab/github_import/importer/note_importer.rb @@ -5,9 +5,9 @@ module GithubImport module Importer class NoteImporter include Gitlab::Import::UsernameMentionRewriter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher - attr_reader :note, :project, :client, :user_finder, :mapper + attr_reader :note, :project, :client, :user_finder # note - An instance of `Gitlab::GithubImport::Representation::Note`. # project - An instance of `Project`. @@ -17,7 +17,6 @@ def initialize(note, project, client) @project = project @client = client @user_finder = GithubImport::UserFinder.new(project, client) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -51,7 +50,7 @@ def execute # Gitlab::GithubImport::Importer::NoteAttachmentsImporter. ids = ApplicationRecord.legacy_bulk_insert(Note.table_name, [attributes], return_ids: true) # rubocop:disable Gitlab/BulkInsert - push_refs_with_ids(ids, Note, note[:author]&.id, mapper.user_mapper) if mapper.user_mapping_enabled? + push_references_by_ids(project, ids, Note, :author_id, note[:author]&.id) end # Returns the ID of the issue or merge request to create the note for. diff --git a/lib/gitlab/github_import/importer/protected_branch_importer.rb b/lib/gitlab/github_import/importer/protected_branch_importer.rb index 8daa3795f2a8ce..128296db459509 100644 --- a/lib/gitlab/github_import/importer/protected_branch_importer.rb +++ b/lib/gitlab/github_import/importer/protected_branch_importer.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Importer class ProtectedBranchImporter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher attr_reader :project @@ -21,7 +21,6 @@ def initialize(protected_branch, project, client) @project = project @client = client @user_finder = GithubImport::UserFinder.new(project, client) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) @gitlab_user_id_to_github_user_id = {} end @@ -38,20 +37,18 @@ def execute # Call `#validate!` to pass any validation errors to the error handling in ImportProtectedBranchWorker. imported_protected_branch.validate! - return unless mapper.user_mapping_enabled? - imported_protected_branch.push_access_levels.each do |push_access_level| next unless push_access_level.user_id source_user_identifier = gitlab_user_id_to_github_user_id[push_access_level.user_id] - push_with_record(push_access_level, :user_id, source_user_identifier, mapper.user_mapper) + push_reference(project, push_access_level, :user_id, source_user_identifier) end end private - attr_reader :gitlab_user_id_to_github_user_id, :protected_branch, :mapper, :user_finder + attr_reader :gitlab_user_id_to_github_user_id, :protected_branch, :user_finder def params { @@ -141,7 +138,7 @@ def allowed_to_push_gitlab_user_ids gitlab_user_id_to_github_user_id[gitlab_user_id] = github_user_data.id end - return @allowed_to_push_gitlab_user_ids if mapper.user_mapping_enabled? + return @allowed_to_push_gitlab_user_ids if user_mapping_enabled?(project) && !map_to_personal_namespace_owner? @allowed_to_push_gitlab_user_ids &= project_member_ids end diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index a6db6aad80e4de..4aba28729756bb 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -6,10 +6,10 @@ module Importer class PullRequestImporter include Gitlab::Import::MergeRequestHelpers include Gitlab::Import::UsernameMentionRewriter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher attr_reader :pull_request, :project, :client, :user_finder, - :milestone_finder, :issuable_finder, :mapper + :milestone_finder, :issuable_finder # pull_request - An instance of # `Gitlab::GithubImport::Representation::PullRequest`. @@ -23,7 +23,6 @@ def initialize(pull_request, project, client) @milestone_finder = MilestoneFinder.new(project) @issuable_finder = GithubImport::IssuableFinder.new(project, pull_request) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -34,13 +33,11 @@ def execute set_merge_request_assignees(mr) insert_git_data(mr, already_exists) - if mapper.user_mapping_enabled? - push_with_record(mr, :author_id, pull_request.author&.id, mapper.user_mapper) + push_reference(project, mr, :author_id, pull_request.author&.id) - # we only import one PR assignee - assignee = mr.merge_request_assignees.first - push_with_record(assignee, :user_id, pull_request.assignee&.id, mapper.user_mapper) if assignee - end + # we only import one PR assignee + assignee = mr.merge_request_assignees.first + push_reference(project, assignee, :user_id, pull_request.assignee&.id) if assignee end end diff --git a/lib/gitlab/github_import/importer/pull_requests/merged_by_importer.rb b/lib/gitlab/github_import/importer/pull_requests/merged_by_importer.rb index ca2fabae8ee2ed..9376afb26e6274 100644 --- a/lib/gitlab/github_import/importer/pull_requests/merged_by_importer.rb +++ b/lib/gitlab/github_import/importer/pull_requests/merged_by_importer.rb @@ -5,7 +5,7 @@ module GithubImport module Importer module PullRequests class MergedByImporter - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher # pull_request - An instance of # `Gitlab::GithubImport::Representation::PullRequest` @@ -16,7 +16,6 @@ def initialize(pull_request, project, client) @project = project @client = client @merged_by = pull_request.merged_by - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -26,8 +25,8 @@ def execute metrics_upsert(gitlab_user_id) - if mapper.user_mapping_enabled? - push_with_record(merge_request.metrics, :merged_by_id, merged_by&.id, mapper.user_mapper) + if user_mapping_enabled?(project) && !map_to_personal_namespace_owner?(project) + push_reference(project, merge_request.metrics, :merged_by_id, merged_by&.id) else add_legacy_note! end @@ -35,7 +34,7 @@ def execute private - attr_reader :project, :pull_request, :client, :mapper, :merged_by + attr_reader :project, :pull_request, :client, :merged_by def metrics_upsert(gitlab_user_id) MergeRequest::Metrics.upsert({ diff --git a/lib/gitlab/github_import/importer/pull_requests/review_importer.rb b/lib/gitlab/github_import/importer/pull_requests/review_importer.rb index 5de257c2b34644..9ac29dd8b91fae 100644 --- a/lib/gitlab/github_import/importer/pull_requests/review_importer.rb +++ b/lib/gitlab/github_import/importer/pull_requests/review_importer.rb @@ -6,7 +6,7 @@ module Importer module PullRequests class ReviewImporter include ::Gitlab::Import::MergeRequestHelpers - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher # review - An instance of `Gitlab::GithubImport::Representation::PullRequestReview` # project - An instance of `Project` @@ -17,7 +17,6 @@ def initialize(review, project, client) @client = client @merge_request = project.merge_requests.find_by_iid(review.merge_request_iid) @user_finder = GithubImport::UserFinder.new(project, client) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute(options = {}) @@ -40,7 +39,7 @@ def execute(options = {}) private - attr_reader :review, :merge_request, :project, :client, :mapper, :user_finder + attr_reader :review, :merge_request, :project, :client, :user_finder def add_review_note!(author_id) return if review.note.empty? @@ -74,9 +73,7 @@ def add_note!(author_id, note) note.save! - return unless mapper.user_mapping_enabled? - - push_with_record(note, :author_id, review.author&.id, mapper.user_mapper) + push_reference(project, note, :author_id, review.author&.id) end def note_attributes(author_id, note, extra = {}) @@ -99,18 +96,18 @@ def add_approval!(user_id) approval, approval_system_note = create_approval!(project.id, merge_request.id, user_id, submitted_at) - return unless mapper.user_mapping_enabled? && approval + return unless approval - push_with_record(approval, :user_id, review.author&.id, mapper.user_mapper) - push_with_record(approval_system_note, :author_id, review.author&.id, mapper.user_mapper) + push_reference(project, approval, :user_id, review.author&.id) + push_reference(project, approval_system_note, :author_id, review.author&.id) end def add_reviewer!(user_id) reviewer = create_reviewer!(merge_request.id, user_id, submitted_at) - return unless mapper.user_mapping_enabled? && reviewer + return unless reviewer - push_with_record(reviewer, :user_id, review.author&.id, mapper.user_mapper) + push_reference(project, reviewer, :user_id, review.author&.id) end def submitted_at diff --git a/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb b/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb index 316b81ef892bfa..083ca9dc863fcc 100644 --- a/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb @@ -6,13 +6,12 @@ module Importer module PullRequests class ReviewRequestImporter include Gitlab::Utils::StrongMemoize - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher def initialize(review_request, project, client) @review_request = review_request @project = project @user_finder = UserFinder.new(project, client) - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end def execute @@ -30,14 +29,14 @@ def execute next unless mapper.user_mapping_enabled? - push_with_record(reviewer, :user_id, user&.id, mapper.user_mapper) + push_reference(project, reviewer, :user_id, user&.id) rescue PG::UniqueViolation, ActiveRecord::RecordNotUnique end end private - attr_reader :review_request, :project, :user_finder, :mapper + attr_reader :review_request, :project, :user_finder end end end diff --git a/lib/gitlab/github_import/importer/releases_importer.rb b/lib/gitlab/github_import/importer/releases_importer.rb index 6ce32b74bf827b..2b93c691e2b4d4 100644 --- a/lib/gitlab/github_import/importer/releases_importer.rb +++ b/lib/gitlab/github_import/importer/releases_importer.rb @@ -5,7 +5,7 @@ module GithubImport module Importer class ReleasesImporter include BulkImporting - include Gitlab::GithubImport::PushPlaceholderReferences + include ::Import::PlaceholderReferences::Pusher # rubocop: disable CodeReuse/ActiveRecord def existing_tags @@ -17,10 +17,6 @@ def github_users @github_users ||= [] end - def mapper - @mapper ||= Gitlab::GithubImport::ContributionsMapper.new(project) - end - # Note: if you're going to replace `legacy_bulk_insert` with something that triggers callback # to generate HTML version - you also need to regenerate it in # Gitlab::GithubImport::Importer::NoteAttachmentsImporter. @@ -29,13 +25,10 @@ def execute inserted_ids = bulk_insert(rows) - if mapper.user_mapping_enabled? - inserted_ids.zip(github_users).each do |id, user| - # `id` is the GitLab Release ID we just inserted. - # `user` is the GitHub user object. - release = Release.find_by(id: id, project_id: project.id) # rubocop:disable CodeReuse/ActiveRecord -- required - push_with_record(release, :author_id, user[:id], mapper.user_mapper) - end + inserted_ids.zip(github_users).each do |id, user| + # `id` is the GitLab Release ID we just inserted. + # `user` is the GitHub user object. + push_references_by_ids(project, [id], Release, :author_id, user[:id]) end bulk_insert_failures(validation_errors) if validation_errors.any? diff --git a/lib/gitlab/github_import/push_placeholder_references.rb b/lib/gitlab/github_import/push_placeholder_references.rb deleted file mode 100644 index 77064e91b81c31..00000000000000 --- a/lib/gitlab/github_import/push_placeholder_references.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module GithubImport - # Contains methods to push placeholder references for user contribution mapping - module PushPlaceholderReferences - # Pushes a placeholder reference using .from_record - # Used when the record is available and the reference only requires a numeric key - def push_with_record(record, attribute, source_user_identifier, user_mapper) - return if source_user_identifier.nil? - - source_user = user_mapper.find_source_user(source_user_identifier) - return if source_user.nil? - return if source_user.accepted_status? - - ::Import::PlaceholderReferences::PushService.from_record( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - record: record, - source_user: source_user, - user_reference_column: attribute - ).execute - end - - # Pushes placeholder references for each Note record found via an id look-up using .new - # This is used as Note records are created using legacy_bulk_insert which - # can return the ids of records created, but not the records themselves - def push_refs_with_ids(ids, model, source_user_identifier, user_mapper) - return if source_user_identifier.nil? - - ids.each do |id| - source_user = user_mapper.find_source_user(source_user_identifier) - - next if source_user.nil? - next if source_user.accepted_status? - - ::Import::PlaceholderReferences::PushService.new( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - source_user_id: source_user.id, - source_user_namespace_id: source_user.namespace_id, - model: model, - user_reference_column: :author_id, - numeric_key: id).execute - end - end - - # Pushes a placeholder reference using a composite key. - # This is used when the record requires a composite key for the reference. - def push_with_composite_key(record, attribute, composite_key, source_user_identifier, user_mapper) - return if source_user_identifier.nil? - - source_user = user_mapper.find_source_user(source_user_identifier) - return if source_user.nil? - return if source_user.accepted_status? - - ::Import::PlaceholderReferences::PushService.new( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - source_user_id: source_user.id, - source_user_namespace_id: source_user.namespace_id, - model: record.class, - user_reference_column: attribute, - composite_key: composite_key - ).execute - end - end - end -end diff --git a/lib/gitlab/github_import/settings.rb b/lib/gitlab/github_import/settings.rb index 6f3a7f257c12e3..f708b791ea6f60 100644 --- a/lib/gitlab/github_import/settings.rb +++ b/lib/gitlab/github_import/settings.rb @@ -55,6 +55,7 @@ def write(user_settings) optional_stages: optional_stages, timeout_strategy: user_settings[:timeout_strategy], user_contribution_mapping_enabled: user_contribution_mapping_enabled?, + user_mapping_to_personal_namespace_owner_enabled: user_mapping_to_personal_namespace_owner_flag_enabled?, pagination_limit: user_settings[:pagination_limit] }, credentials: project.import_data&.credentials @@ -75,6 +76,10 @@ def user_mapping_enabled? project.import_data&.data&.dig('user_contribution_mapping_enabled') || false end + def user_mapping_to_personal_namespace_owner_enabled? + project.import_data&.data&.dig('user_mapping_to_personal_namespace_owner_enabled') || false + end + private attr_reader :project @@ -100,6 +105,15 @@ def user_contribution_mapping_enabled? !!flag_by_type end + + def user_mapping_to_personal_namespace_owner_flag_enabled? + return false unless user_contribution_mapping_enabled? + + Feature.enabled?( + :user_mapping_to_personal_namespace_owner, + User.actor_from_id(project.creator_id) + ) + end end end end diff --git a/lib/gitlab/github_import/user_finder.rb b/lib/gitlab/github_import/user_finder.rb index 4b61e5a9b797b6..d56acad559ba61 100644 --- a/lib/gitlab/github_import/user_finder.rb +++ b/lib/gitlab/github_import/user_finder.rb @@ -14,7 +14,7 @@ module GithubImport class UserFinder include Gitlab::ExclusiveLeaseHelpers - attr_reader :project, :client, :mapper + attr_reader :project, :client # The base cache key to use for caching user IDs for a given GitHub user ID. ID_CACHE_KEY = 'github-import/user-finder/user-id/%s' @@ -43,7 +43,6 @@ class UserFinder def initialize(project, client) @project = project @client = client - @mapper = Gitlab::GithubImport::ContributionsMapper.new(project) end # Returns the GitLab user ID of an object's author. @@ -85,20 +84,20 @@ def user_id_for(user, ghost: true) return ghost ? GithubImport.ghost_user_id : nil end - if mapper.user_mapping_enabled? - source_user(user).mapped_user_id - else - find(user[:id], user[:login]) - end + return find(user[:id], user[:login]) unless user_mapping_enabled? + + return project.root_ancestor.owner_id if map_to_personal_namespace_owner? + + source_user(user).mapped_user_id end # Returns the GitLab user ID from placeholder or reassigned_to user. def source_user(user) - source_user = mapper.user_mapper.find_source_user(user[:id]) + source_user = source_user_mapper.find_source_user(user[:id]) return source_user if source_user - mapper.user_mapper.find_or_create_source_user( + source_user_mapper.find_or_create_source_user( source_name: fetch_source_name_from_github(user[:login]), source_username: user[:login], source_user_identifier: user[:id] @@ -107,7 +106,8 @@ def source_user(user) # Returns true if GitLab user has accepted their reassignment status or if UCM is not enabled def source_user_accepted?(user) - return true unless mapper.user_mapping_enabled? + return true unless user_mapping_enabled? + return true if map_to_personal_namespace_owner? source_user(user).accepted_status? end @@ -381,6 +381,23 @@ def log(message, username: nil) message: message ) end + + def source_user_mapper + @user_mapper ||= ::Gitlab::Import::SourceUserMapper.new( + namespace: project.root_ancestor, + source_hostname: project.safe_import_url, + import_type: ::Import::SOURCE_GITHUB + ) + end + + def user_mapping_enabled? + project.import_data.user_mapping_enabled? + end + + def map_to_personal_namespace_owner? + project.root_ancestor.user_namespace? && + project.import_data.user_mapping_to_personal_namespace_owner_enabled? + end end end end diff --git a/lib/import/placeholder_references/pusher.rb b/lib/import/placeholder_references/pusher.rb index f199ffa3cad528..9a37fb8e503918 100644 --- a/lib/import/placeholder_references/pusher.rb +++ b/lib/import/placeholder_references/pusher.rb @@ -4,18 +4,15 @@ module Import module PlaceholderReferences module Pusher def push_reference(project, record, attribute, source_user_identifier) - return unless user_mapping_enabled?(project) - return if map_to_personal_namespace_owner?(project) - return if source_user_identifier.nil? + return if pushing_not_allowed?(project, source_user_identifier) 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. + # 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_source: import_type, import_uid: project.import_state.id, record: record, source_user: source_user, @@ -23,27 +20,80 @@ def push_reference(project, record, attribute, source_user_identifier) ).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) + # This is used for records created using legacy_bulk_insert which can + # return the ids of records created, but not the records themselves + def push_references_by_ids(project, ids, model, attribute, source_user_identifier) + return if pushing_not_allowed?(project, source_user_identifier) + + ids.each do |id| + source_user = source_user_mapper(project).find_source_user(source_user_identifier) + + next if source_user.nil? + next if source_user.accepted_status? + + ::Import::PlaceholderReferences::PushService.new( + import_source: import_type, + import_uid: project.import_state.id, + source_user_id: source_user.id, + source_user_namespace_id: source_user.namespace_id, + model: model, + user_reference_column: attribute, + numeric_key: id).execute + end end - def source_user_mapper(project) - @user_mapper ||= ::Gitlab::Import::SourceUserMapper.new( - namespace: project.root_ancestor, - source_hostname: project.safe_import_url, - import_type: ::Import::SOURCE_BITBUCKET_SERVER - ) + # Pushes a placeholder reference using a composite key. + # This is used when the record requires a composite key for the reference. + def push_reference_with_composite_key(project, record, attribute, composite_key, source_user_identifier) + return if pushing_not_allowed?(project, source_user_identifier) + + 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.new( + import_source: import_type, + import_uid: project.import_state.id, + source_user_id: source_user.id, + source_user_namespace_id: source_user.namespace_id, + model: record.class, + user_reference_column: attribute, + composite_key: composite_key + ).execute end def user_mapping_enabled?(project) - !!project.import_data.user_mapping_enabled? + project.import_data.user_mapping_enabled? end def map_to_personal_namespace_owner?(project) project.root_ancestor.user_namespace? && project.import_data.user_mapping_to_personal_namespace_owner_enabled? end + + private + + def pushing_not_allowed?(project, source_user_identifier) + source_user_identifier.present? && user_mapping_enabled?(project) && !map_to_personal_namespace_owner?(project) + end + + def import_type + @type ||= project.import_type.to_sym + 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.safe_import_url, + import_type: import_type + ) + end end end end diff --git a/spec/lib/gitlab/github_import/contributions_mapper_spec.rb b/spec/lib/gitlab/github_import/contributions_mapper_spec.rb deleted file mode 100644 index e01ab0437db3fa..00000000000000 --- a/spec/lib/gitlab/github_import/contributions_mapper_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::GithubImport::ContributionsMapper, :clean_gitlab_redis_shared_state, feature_category: :importers do - let_it_be(:project) { create(:project, :with_import_url) } - - let(:mapper) { described_class.new(project) } - - let(:user_mapping_enabled) { true } - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: user_mapping_enabled }) - end - - describe '#user_mapper' do - it 'creates an instance of a source user mapper' do - expect(mapper.user_mapper).to be_an_instance_of(::Gitlab::Import::SourceUserMapper) - end - end - - describe '#user_mapping_enabled?' do - context 'when user mapping is enabled' do - it 'returns true' do - expect(mapper.user_mapping_enabled?).to be true - end - end - - context 'when user mapping is disbled' do - let(:user_mapping_enabled) { false } - - it 'returns false' do - expect(mapper.user_mapping_enabled?).to be false - end - end - end -end diff --git a/spec/lib/gitlab/github_import/push_placeholder_references_spec.rb b/spec/lib/gitlab/github_import/push_placeholder_references_spec.rb deleted file mode 100644 index af0764e2ea16f9..00000000000000 --- a/spec/lib/gitlab/github_import/push_placeholder_references_spec.rb +++ /dev/null @@ -1,144 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::GithubImport::PushPlaceholderReferences, feature_category: :importers do - let(:client) { instance_double(Gitlab::GithubImport::Client) } - let_it_be(:source_id) { 'source_identifier' } - let_it_be(:project) { create(:project, :with_import_url) } - let_it_be(:author) { create(:user) } - let_it_be(:import_source_user) do - create( - :import_source_user, - source_user_identifier: source_id, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id, - placeholder_user_id: author.id - ) - end - - let(:github_note) do - Gitlab::GithubImport::Representation::Note.new( - note_id: 100, - noteable_id: 1, - noteable_type: 'Issue', - author: Gitlab::GithubImport::Representation::User.new(id: source_id, login: 'alice'), - note: 'This is my note', - created_at: Time.current, - updated_at: Time.current - ) - end - - let(:gitlab_note) { create(:note) } - let(:user_mapper) { Gitlab::GithubImport::ContributionsMapper.new(project).user_mapper } - - let(:importer) { Gitlab::GithubImport::Importer::NoteImporter.new(github_note, project, client) } - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end - - describe '#push_with_record' do - before do - allow(::Import::PlaceholderReferences::PushService) - .to receive_message_chain(:from_record, :execute) - end - - it 'pushes the reference using .from_record' do - importer.push_with_record(gitlab_note, :author_id, source_id, user_mapper) - - expect(::Import::PlaceholderReferences::PushService) - .to have_received(:from_record) - .with( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - record: gitlab_note, - source_user: import_source_user, - user_reference_column: :author_id) - end - - it 'does not push a reference if source identifier is nil' do - importer.push_with_record(gitlab_note, :author_id, nil, user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:from_record) - end - - it 'does not push a reference if source identifier does not match an existing source user' do - importer.push_with_record(gitlab_note, :author_id, 'unmatched_idenfier', user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:from_record) - end - end - - describe '#push_refs_with_ids' do - before do - allow(::Import::PlaceholderReferences::PushService) - .to receive_message_chain(:new, :execute) - end - - it 'pushes the reference using .new' do - importer.push_refs_with_ids([gitlab_note.id], Note, source_id, user_mapper) - - expect(::Import::PlaceholderReferences::PushService) - .to have_received(:new) - .with( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - source_user_id: import_source_user.id, - source_user_namespace_id: import_source_user.namespace_id, - model: gitlab_note.class, - user_reference_column: :author_id, - numeric_key: gitlab_note.id - ) - end - - it 'does not push a reference if source identifier is nil' do - importer.push_refs_with_ids([gitlab_note.id], Note, nil, user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:new) - end - - it 'does not push a reference if source identifier does not match an existing source user' do - importer.push_refs_with_ids([gitlab_note.id], Note, 'unmatched_idenfier', user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:from_record) - end - end - - describe '#push_with_composite_key' do - let(:composite_key) { { "user_id" => "user_id", "merge_request_id" => "merge_request_id" } } - - before do - allow(::Import::PlaceholderReferences::PushService) - .to receive_message_chain(:new, :execute) - end - - it 'pushes the reference with composite key' do - importer.push_with_composite_key(gitlab_note, :user_id, composite_key, source_id, user_mapper) - - expect(::Import::PlaceholderReferences::PushService) - .to have_received(:new) - .with( - import_source: ::Import::SOURCE_GITHUB, - import_uid: project.import_state.id, - source_user_id: import_source_user.id, - source_user_namespace_id: import_source_user.namespace_id, - model: gitlab_note.class, - user_reference_column: :user_id, - composite_key: composite_key - ) - end - - it 'does not push a reference if source identifier is nil' do - importer.push_with_composite_key(gitlab_note, :user_id, composite_key, nil, user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:new) - end - - it 'does not push a reference if source identifier does not match an existing source user' do - importer.push_with_composite_key(gitlab_note, :user_id, composite_key, 'unmatched_idenfier', user_mapper) - - expect(::Import::PlaceholderReferences::PushService).not_to receive(:from_record) - end - end -end diff --git a/spec/lib/gitlab/github_import/settings_spec.rb b/spec/lib/gitlab/github_import/settings_spec.rb index a024a0aa024b7a..f721bc6f420701 100644 --- a/spec/lib/gitlab/github_import/settings_spec.rb +++ b/spec/lib/gitlab/github_import/settings_spec.rb @@ -72,6 +72,8 @@ .to be true expect(project.import_data.data['pagination_limit']) .to eq(50) + expect(project.import_data.data['user_mapping_to_personal_namespace_owner_enabled']) + .to be true end end @@ -161,4 +163,79 @@ it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false end end + + describe '#user_mapping_to_personal_namespace_owner_enabled?' do + subject do + settings.write(data_input) + settings.user_mapping_to_personal_namespace_owner_enabled? + end + + shared_examples 'when :github_user_mapping is disabled' do |expected_enabled:| + before do + Feature.disable(:github_user_mapping) + end + + it { is_expected.to be(expected_enabled) } + end + + shared_examples 'when :gitea_user_mapping is disabled' do |expected_enabled:| + before do + Feature.disable(:gitea_user_mapping) + end + + it { is_expected.to be(expected_enabled) } + end + + before do + project.build_or_assign_import_data(credentials: { user: 'token' }) + end + + context 'when the project is a GitHub import' do + it { is_expected.to be(true) } + + it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false + it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: true + end + + context 'when the project is a Gitea import' do + before do + project.update!(import_type: ::Import::SOURCE_GITEA.to_s) + end + + it { is_expected.to be(true) } + + it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false + it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: true + end + + context 'when the project does not have an import_type' do + before do + project.update!(import_type: nil) + end + + it { is_expected.to be(false) } + + it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false + it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false + end + + context 'when the project has an import_type without a user mapping flag' do + before do + project.update!(import_type: ::Import::SOURCE_BITBUCKET.to_s) + end + + it { is_expected.to be(false) } + + it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false + it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(user_mapping_to_personal_namespace_owner: false) + end + + it { is_expected.to be(false) } + end + end end diff --git a/spec/lib/gitlab/github_import/user_finder_spec.rb b/spec/lib/gitlab/github_import/user_finder_spec.rb index 4359f9379a1648..299c59e70171ae 100644 --- a/spec/lib/gitlab/github_import/user_finder_spec.rb +++ b/spec/lib/gitlab/github_import/user_finder_spec.rb @@ -3,9 +3,11 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_shared_state, feature_category: :importers do - let_it_be(:project) do + let_it_be(:user_namespace) { create(:namespace) } + let_it_be_with_reload(:project) do create( :project, + :in_group, import_type: 'github', import_url: 'https://github.com/user/repo.git' ) @@ -14,11 +16,15 @@ let(:client) { instance_double(Gitlab::GithubImport::Client) } let(:settings) { Gitlab::GithubImport::Settings.new } let(:user_mapping_enabled) { true } + let(:user_mapping_to_personal_namespace_owner_enabled) { true } subject(:finder) { described_class.new(project, client) } before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: user_mapping_enabled }) + project.build_or_assign_import_data(data: { + user_contribution_mapping_enabled: user_mapping_enabled, + user_mapping_to_personal_namespace_owner_enabled: user_mapping_to_personal_namespace_owner_enabled + }) end describe '#author_id_for' do @@ -136,6 +142,34 @@ expect { finder.user_id_for(user) }.to change { Import::SourceUser.count }.by(1) expect(finder.user_id_for(user)).not_to eq(source_user.mapped_user_id) end + + context 'when the project is imported into a personal namespace' do + before do + project.update!(namespace: user_namespace) + end + + it 'returns the user namespace owner id' do + user = { id: 6, login: 'anything' } + + allow(client).to receive(:user).and_return({ name: 'Source name' }) + + expect { finder.user_id_for(user) }.not_to change { Import::SourceUser.count } + expect(finder.user_id_for(user)).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let(:user_mapping_to_personal_namespace_owner_enabled) { false } + + it 'returns the mapped_user_id of source user' do + user = { id: 6, login: 'anything' } + + allow(client).to receive(:user).and_return({ name: 'Source name' }) + + expect { finder.user_id_for(user) }.to change { Import::SourceUser.count }.by(1) + expect(finder.user_id_for(user)).not_to eq(user_namespace.owner_id) + end + end + end end end @@ -794,6 +828,34 @@ expect(finder.source_user_accepted?(user)).to be(false) end + context 'when the project is imported into a personal namespace' do + before do + project.update!(namespace: user_namespace) + end + + # User contributions are assigned directly to the namespace owner, effectively behaving as accepted source users + it 'returns true' do + expect(finder.source_user_accepted?(user)).to be(true) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let(:user_mapping_to_personal_namespace_owner_enabled) { false } + let!(:source_user) do + create( + :import_source_user, :awaiting_approval, + namespace: user_namespace, + source_hostname: 'https://github.com', + import_type: project.import_type, + source_user_identifier: user[:id] + ) + end + + it 'returns false when the associated source user does not have an accepted status' do + expect(finder.source_user_accepted?(user)).to be(false) + end + end + end + context 'when user contribution mapping is disabled' do let(:user_mapping_enabled) { false } diff --git a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb index cbd41be761c40f..d52a0ee0a08089 100644 --- a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb +++ b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb @@ -10,7 +10,13 @@ end context 'when there are no remaining jobs' do - let_it_be(:project) { create(:project, :import_user_mapping_enabled, import_status: :started, import_jid: '123') } + let_it_be_with_reload(:project) do + create( + :project, :in_group, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled, + import_status: :started, import_jid: '123' + ) + end subject(:worker) { described_class.new } @@ -30,6 +36,36 @@ worker.perform(project.id, { '123' => 2 }, 'finish') end + context 'when importing into a personal namespace' do + before do + project.update!(namespace: create(:namespace)) + end + + it 'does not enqueue LoadPlaceholderReferencesWorker' do + expect(::Import::LoadPlaceholderReferencesWorker).not_to receive(:perform_async) + + worker.perform(project.id, { '123' => 2 }, 'finish') + end + + context 'and user_mapping_to_personal_namespace_owner is disabled' do + before do + allow_next_instance_of(Gitlab::GithubImport::Settings) do |settings| + allow(settings).to receive(:user_mapping_to_personal_namespace_owner_enabled?).and_return(false) + end + end + + it 'enqueues LoadPlaceholderReferencesWorker to save placeholder references' do + expect(::Import::LoadPlaceholderReferencesWorker).to receive(:perform_async).with( + ::Import::SOURCE_GITHUB, + project.import_state.id, + { 'current_user_id' => project.creator_id } + ) + + worker.perform(project.id, { '123' => 2 }, 'finish') + end + end + end + context 'when user contribution mapping is disabled' do before do allow(Gitlab::GithubImport::Settings).to receive_message_chain(:new, :user_mapping_enabled?).and_return(false) diff --git a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb index dea2d61e481bb6..f8a9a741d49a18 100644 --- a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Stage::FinishImportWorker, feature_category: :importers do - let_it_be_with_reload(:project) { create(:project) } + let_it_be_with_reload(:project) do + create(:project, :in_group, :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled) + end + let_it_be(:import_state) { create(:import_state, :started, project: project) } subject(:worker) { described_class.new } @@ -11,10 +14,6 @@ it_behaves_like Gitlab::GithubImport::StageMethods describe '#perform' do - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end - it 'marks the import as finished and reports import statistics' do expect(project).to receive(:after_import) expect_next_instance_of(Gitlab::Import::Metrics) do |instance| @@ -76,6 +75,54 @@ expect(described_class).to have_received(:perform_in).with(30.seconds, project.id) end + + context 'when importing into a personal namespace' do + before do + project.update!(namespace: create(:namespace)) + end + + it 'does not check the reference store and does not re-enqueue' do + allow(described_class).to receive(:perform_in) + allow(worker).to receive_message_chain(:placeholder_reference_store, :any?).and_return(true) + + expect(Import::LoadPlaceholderReferencesWorker).not_to receive(:perform_async) + + worker.import(double(:client), project) + + expect(described_class).not_to have_received(:perform_in) + end + + context 'and user_mapping_to_personal_namespace_owner is disabled' do + before do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'checks the reference store, queues LoadPlaceholderReferencesWorker, and requeues itself' do + allow(described_class).to receive(:perform_in) + allow(worker).to receive_message_chain(:placeholder_reference_store, :any?).and_return(true) + allow(worker).to receive_message_chain(:placeholder_reference_store, :count).and_return(1) + + expect(Import::LoadPlaceholderReferencesWorker).to receive(:perform_async) + + expect(Gitlab::GithubImport::Logger) + .to receive(:info) + .with( + { + message: 'Delaying finalization as placeholder references are pending', + import_stage: 'Gitlab::GithubImport::Stage::FinishImportWorker', + placeholder_store_count: 1, + project_id: project.id + } + ) + + worker.import(double(:client), project) + + expect(described_class).to have_received(:perform_in).with(30.seconds, project.id) + end + end + end end end end -- GitLab From 7d8e79434987cc9ad8b3f88b578abad21e9404e7 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 28 Jul 2025 18:28:16 -0400 Subject: [PATCH 2/6] Updated affected specs with minor fixes --- .../importer/events/changed_label.rb | 2 - .../github_import/importer/events/closed.rb | 2 - .../github_import/importer/events/renamed.rb | 2 - .../importer/protected_branch_importer.rb | 2 +- .../pull_requests/review_request_importer.rb | 2 - lib/import/placeholder_references/pusher.rb | 8 +- spec/factories/projects.rb | 6 + .../importer/diff_note_importer_spec.rb | 297 +++++++++------ .../importer/events/changed_assignee_spec.rb | 131 ++++--- .../importer/events/changed_label_spec.rb | 254 +++++++++++-- .../importer/events/changed_milestone_spec.rb | 177 ++++++--- .../importer/events/changed_reviewer_spec.rb | 152 ++++---- .../importer/events/closed_spec.rb | 193 +++++++--- .../importer/events/cross_referenced_spec.rb | 142 +++++--- .../importer/events/merged_spec.rb | 198 +++++----- .../importer/events/renamed_spec.rb | 136 ++++--- .../importer/issue_importer_spec.rb | 110 ++++-- .../importer/note_importer_spec.rb | 104 +++++- .../protected_branch_importer_spec.rb | 119 +++++-- .../importer/pull_request_importer_spec.rb | 90 +++-- .../pull_requests/merged_by_importer_spec.rb | 72 +++- .../pull_requests/review_importer_spec.rb | 337 +++++++++++------- .../review_request_importer_spec.rb | 63 +++- .../importer/releases_importer_spec.rb | 239 +++++++------ .../helpers/import/user_mapping_helper.rb | 8 +- 25 files changed, 1946 insertions(+), 900 deletions(-) diff --git a/lib/gitlab/github_import/importer/events/changed_label.rb b/lib/gitlab/github_import/importer/events/changed_label.rb index 22f55b6d7945ab..12c366e1dc242e 100644 --- a/lib/gitlab/github_import/importer/events/changed_label.rb +++ b/lib/gitlab/github_import/importer/events/changed_label.rb @@ -23,8 +23,6 @@ def create_event(issue_event) created_event = ResourceLabelEvent.create!(attrs) - return unless mapper.user_mapping_enabled? - push_reference(project, created_event, :user_id, issue_event[:actor]&.id) end diff --git a/lib/gitlab/github_import/importer/events/closed.rb b/lib/gitlab/github_import/importer/events/closed.rb index 1a1a4bed963a40..b6ed36596b79ce 100644 --- a/lib/gitlab/github_import/importer/events/closed.rb +++ b/lib/gitlab/github_import/importer/events/closed.rb @@ -43,8 +43,6 @@ def create_state_event(issue_event) state_event = ResourceStateEvent.create!(attrs) - return unless mapper.user_mapping_enabled? - push_reference(project, state_event, :user_id, issue_event[:actor]&.id) end end diff --git a/lib/gitlab/github_import/importer/events/renamed.rb b/lib/gitlab/github_import/importer/events/renamed.rb index 9bc3f737d399fc..de42a5e05c0520 100644 --- a/lib/gitlab/github_import/importer/events/renamed.rb +++ b/lib/gitlab/github_import/importer/events/renamed.rb @@ -8,8 +8,6 @@ class Renamed < BaseImporter def execute(issue_event) created_note = Note.create!(note_params(issue_event)) - return unless mapper.user_mapping_enabled? - push_reference(project, created_note, :author_id, issue_event[:actor]&.id) end diff --git a/lib/gitlab/github_import/importer/protected_branch_importer.rb b/lib/gitlab/github_import/importer/protected_branch_importer.rb index 128296db459509..332ced61835d4a 100644 --- a/lib/gitlab/github_import/importer/protected_branch_importer.rb +++ b/lib/gitlab/github_import/importer/protected_branch_importer.rb @@ -138,7 +138,7 @@ def allowed_to_push_gitlab_user_ids gitlab_user_id_to_github_user_id[gitlab_user_id] = github_user_data.id end - return @allowed_to_push_gitlab_user_ids if user_mapping_enabled?(project) && !map_to_personal_namespace_owner? + return @allowed_to_push_gitlab_user_ids if user_mapping_enabled?(project) @allowed_to_push_gitlab_user_ids &= project_member_ids end diff --git a/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb b/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb index 083ca9dc863fcc..9aebf66945457e 100644 --- a/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_requests/review_request_importer.rb @@ -27,8 +27,6 @@ def execute created_at: Time.zone.now ) - next unless mapper.user_mapping_enabled? - push_reference(project, reviewer, :user_id, user&.id) rescue PG::UniqueViolation, ActiveRecord::RecordNotUnique end diff --git a/lib/import/placeholder_references/pusher.rb b/lib/import/placeholder_references/pusher.rb index 9a37fb8e503918..fc6b50d8e4a2de 100644 --- a/lib/import/placeholder_references/pusher.rb +++ b/lib/import/placeholder_references/pusher.rb @@ -4,7 +4,7 @@ module Import module PlaceholderReferences module Pusher def push_reference(project, record, attribute, source_user_identifier) - return if pushing_not_allowed?(project, source_user_identifier) + return unless allowed_to_push?(project, source_user_identifier) source_user = source_user_mapper(project).find_source_user(source_user_identifier) @@ -23,7 +23,7 @@ def push_reference(project, record, attribute, source_user_identifier) # This is used for records created using legacy_bulk_insert which can # return the ids of records created, but not the records themselves def push_references_by_ids(project, ids, model, attribute, source_user_identifier) - return if pushing_not_allowed?(project, source_user_identifier) + return unless allowed_to_push?(project, source_user_identifier) ids.each do |id| source_user = source_user_mapper(project).find_source_user(source_user_identifier) @@ -45,7 +45,7 @@ def push_references_by_ids(project, ids, model, attribute, source_user_identifie # Pushes a placeholder reference using a composite key. # This is used when the record requires a composite key for the reference. def push_reference_with_composite_key(project, record, attribute, composite_key, source_user_identifier) - return if pushing_not_allowed?(project, source_user_identifier) + return unless allowed_to_push?(project, source_user_identifier) source_user = source_user_mapper(project).find_source_user(source_user_identifier) @@ -74,7 +74,7 @@ def map_to_personal_namespace_owner?(project) private - def pushing_not_allowed?(project, source_user_identifier) + def allowed_to_push?(project, source_user_identifier) source_user_identifier.present? && user_mapping_enabled?(project) && !map_to_personal_namespace_owner?(project) end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index c9b6c782dfedae..0ab375b1e60ac2 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -227,6 +227,12 @@ import_type { :bitbucket_server } end + trait :github_import do + import_started + import_url { 'https://github.com' } + import_type { :github } + end + trait :jira_dvcs_server do before(:create) do |project| create(:project_feature_usage, :dvcs_server, project: project) diff --git a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb index 71b4f35918719a..f511e2a1055871 100644 --- a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb @@ -3,7 +3,15 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_failures, feature_category: :importers do - let_it_be(:project) { create(:project, :repository, :with_import_url) } + include Import::UserMappingHelper + + let_it_be_with_reload(:project) do + create( + :project, :repository, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:client) { instance_double(Gitlab::GithubImport::Client) } @@ -40,65 +48,13 @@ ) end - subject(:importer) { described_class.new(note_representation, project, client) } - - shared_examples 'diff notes without suggestion' do - it 'imports the note as legacy diff note' do - stub_user_finder(user.id, true) - - expect { subject.execute } - .to change(LegacyDiffNote, :count) - .by(1) - - note = project.notes.diff_notes.take - expect(note).to be_valid - expect(note.imported_from).to eq(::Import::SOURCE_GITHUB.to_s) - expect(note.author_id).to eq(user.id) - expect(note.commit_id).to eq('original123abc') - expect(note.created_at).to eq(created_at) - expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) - expect(note.discussion_id).to eq(discussion_id) - expect(note.line_code).to eq(note_representation.line_code) - expect(note.note).to eq('Hello') - expect(note.noteable_id).to eq(merge_request.id) - expect(note.noteable_type).to eq('MergeRequest') - expect(note.project_id).to eq(project.id) - expect(note.st_diff).to eq(note_representation.diff_hash) - expect(note.system).to eq(false) - expect(note.type).to eq('LegacyDiffNote') - expect(note.updated_at).to eq(updated_at) - end - - it 'adds a "created by:" note when the author cannot be found' do - stub_user_finder(project.creator_id, false) - - expect { subject.execute } - .to change(LegacyDiffNote, :count) - .by(1) + let(:cached_references) { placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) } - note = project.notes.diff_notes.take - expect(note).to be_valid - expect(note.author_id).to eq(project.creator_id) - expect(note.note).to eq("*Created by: #{user.username}*\n\nHello") - end - end + subject(:importer) { described_class.new(note_representation, project, client) } - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end + let_it_be(:source_user) { generate_source_user(project, user.id) } context 'when the merge request no longer exists' do it 'does not import anything' do @@ -123,20 +79,37 @@ end end - it 'pushes placeholder references with ids' do - expect(subject) - .to receive(:push_refs_with_ids) - .with( - array_including(be_an(Integer)), - LegacyDiffNote, - note_representation.author.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - + it 'pushes placeholder references' do subject.execute + + expect(cached_references).to contain_exactly( + ['LegacyDiffNote', instance_of(Integer), 'author_id', source_user.id] + ) end - it_behaves_like 'diff notes without suggestion' + it 'imports the note as legacy diff note' do + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.imported_from).to eq(::Import::SOURCE_GITHUB.to_s) + expect(note.author_id).to eq(source_user.mapped_user_id) + expect(note.commit_id).to eq('original123abc') + expect(note.created_at).to eq(created_at) + expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) + expect(note.discussion_id).to eq(discussion_id) + expect(note.line_code).to eq(note_representation.line_code) + expect(note.note).to eq('Hello') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.noteable_type).to eq('MergeRequest') + expect(note.project_id).to eq(project.id) + expect(note.st_diff).to eq(note_representation.diff_hash) + expect(note.system).to eq(false) + expect(note.type).to eq('LegacyDiffNote') + expect(note.updated_at).to eq(updated_at) + end context 'when the note has suggestions' do let(:note_body) do @@ -148,17 +121,12 @@ EOB end - it 'pushes placeholder references with record' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(DiffNote), - :author_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - + it 'pushes placeholder references' do subject.execute + + expect(cached_references).to contain_exactly( + ['DiffNote', instance_of(Integer), 'author_id', source_user.id] + ) end it 'imports the note as diff note' do @@ -178,7 +146,7 @@ expect(note.noteable_id).to eq(merge_request.id) expect(note.project_id).to eq(project.id) expect(note.namespace_id).to eq(project.project_namespace_id) - expect(note.author_id).to eq(user.id) + expect(note.author_id).to eq(source_user.mapped_user_id) expect(note.system).to eq(false) expect(note.discussion_id).to eq(discussion_id) expect(note.commit_id).to eq('original123abc') @@ -250,6 +218,56 @@ .and not_change(DiffNote, :count) end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + subject.execute + + expect(cached_references).to be_empty + end + + it 'imports the diff note mapped to the personal namespace owner' do + expect { subject.execute } + .to change(DiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note.author_id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, user.id) } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + subject.execute + + expect(cached_references).to contain_exactly( + ['DiffNote', instance_of(Integer), 'author_id', source_user.id] + ) + end + + it 'imports the diff note mapped to the placeholder user' do + expect { subject.execute } + .to change(DiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note.author_id).to eq(source_user.mapped_user_id) + end + end + end end context 'when diff note is invalid' do @@ -259,12 +277,62 @@ expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let_it_be(:project) do + project.update!(namespace: user_namespace) + project + end + + it 'does not push any references' do + subject.execute + + expect(cached_references).to be_empty + end + + it 'imports the legacy diff note mapped to the personal namespace owner' do + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note.author_id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, user.id) } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + subject.execute + + expect(cached_references).to contain_exactly( + ['LegacyDiffNote', instance_of(Integer), 'author_id', source_user.id] + ) + end + + it 'imports the legacy diff note mapped to the placeholder user' do + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note.author_id).to eq(source_user.mapped_user_id) + end + end + end end end context 'when user mapping is disabled' do - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + before_all do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! end context 'when the merge request no longer exists' do @@ -282,7 +350,13 @@ create(:merge_request, source_project: project, target_project: project) end + let(:gitlab_user_id) { user.id } + before do + expect_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + expect(finder).to receive(:find).and_return(gitlab_user_id) + end + expect_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| expect(finder) .to receive(:database_id) @@ -291,14 +365,49 @@ end it 'does not push placeholder references' do - stub_user_finder(user.id, true) + subject.execute - expect(subject).not_to receive(:push_note_refs_with_ids) + expect(cached_references).to be_empty + end - subject.execute + it 'imports the note as legacy diff note' do + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.imported_from).to eq(::Import::SOURCE_GITHUB.to_s) + expect(note.author_id).to eq(user.id) + expect(note.commit_id).to eq('original123abc') + expect(note.created_at).to eq(created_at) + expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) + expect(note.discussion_id).to eq(discussion_id) + expect(note.line_code).to eq(note_representation.line_code) + expect(note.note).to eq('Hello') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.noteable_type).to eq('MergeRequest') + expect(note.project_id).to eq(project.id) + expect(note.st_diff).to eq(note_representation.diff_hash) + expect(note.system).to eq(false) + expect(note.type).to eq('LegacyDiffNote') + expect(note.updated_at).to eq(updated_at) end - it_behaves_like 'diff notes without suggestion' + context 'when the author is not found' do + let(:gitlab_user_id) { nil } + + it 'sets the project creator as the author with a "created by:" note' do + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.author_id).to eq(project.creator_id) + expect(note.note).to eq("*Created by: #{user.username}*\n\nHello") + end + end context 'when the note has suggestions' do let(:note_body) do @@ -310,14 +419,10 @@ EOB end - before do - stub_user_finder(user.id, true) - end - it 'does not push placeholder references' do - expect(subject).not_to receive(:push_with_record) - subject.execute + + expect(cached_references).to be_empty end it 'imports the note as diff note' do @@ -412,8 +517,6 @@ context 'when diff note is invalid' do it 'fails validation' do - stub_user_finder(user.id, true) - expect(note_representation).to receive(:line_code).and_return(nil) expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) @@ -421,13 +524,5 @@ end end end - - def stub_user_finder(user, found) - expect_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - expect(finder) - .to receive(:author_id_for) - .and_return([user, found]) - end - end end end diff --git a/spec/lib/gitlab/github_import/importer/events/changed_assignee_spec.rb b/spec/lib/gitlab/github_import/importer/events/changed_assignee_spec.rb index 0bb14498d6c18d..0a17d796c5fe21 100644 --- a/spec/lib/gitlab/github_import/importer/events/changed_assignee_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/changed_assignee_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedAssignee, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedAssignee, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:author) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -25,19 +33,6 @@ ) end - let(:note_attrs) do - { - noteable_id: issuable.id, - noteable_type: issuable.class.name, - project_id: project.id, - author_id: author.id, - system: true, - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end - let(:expected_system_note_metadata_attrs) do { action: "assignee", @@ -46,6 +41,8 @@ }.stringify_keys end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } + shared_examples 'create expected notes' do it 'creates expected note' do expect { importer.execute(issue_event) }.to change { issuable.notes.count } @@ -98,16 +95,11 @@ let(:event_type) { 'assigned' } it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Note), - :author_id, - issue_event[:actor].id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Note', an_instance_of(Integer), 'author_id', source_user.id] + ]) end end @@ -115,10 +107,9 @@ let(:event_type) { 'assigned' } it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) + + expect(cached_references).to be_empty end end @@ -126,24 +117,23 @@ before do allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(issuable.id) - allow(finder).to receive(:author_id_for).with(issue_event, author_key: :actor).and_return([author.id, true]) end end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: author.id, - source_user_identifier: 1000, - source_username: 'github_author', - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_author_id) { source_user.mapped_user_id } + let(:note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + author_id: mapped_author_id, + project_id: project.id, + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do @@ -157,11 +147,68 @@ it_behaves_like 'process assigned & unassigned events' it_behaves_like 'push a placeholder reference' end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_author_id) { user_namespace.owner_id } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + it_behaves_like 'process assigned & unassigned events' + it_behaves_like 'do not push placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'process assigned & unassigned events' + it_behaves_like 'do not push placeholder reference' + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_author_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + it_behaves_like 'process assigned & unassigned events' + it_behaves_like 'push a placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'process assigned & unassigned events' + it_behaves_like 'push a placeholder reference' + end + end + end end context 'when user mapping is disabled' do + let(:note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + author_id: author.id, + project_id: project.id, + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| allow(finder).to receive(:find).with(1000, 'github_author').and_return(author.id) end diff --git a/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb b/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb index b64435dd661714..2acf72d909ee5c 100644 --- a/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -17,7 +25,7 @@ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => event_type, 'commit_id' => nil, 'label_title' => label_title, @@ -26,14 +34,7 @@ ) end - let(:event_attrs) do - { - user_id: user.id, - label_id: label_id, - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } shared_examples 'new event' do it 'creates a new label event' do @@ -46,25 +47,19 @@ shared_examples 'push placeholder reference' do it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(ResourceLabelEvent), - :user_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['ResourceLabelEvent', an_instance_of(Integer), 'user_id', source_user.id] + ]) end end shared_examples 'do not push placeholder reference' do it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) + + expect(cached_references).to be_empty end end @@ -75,19 +70,15 @@ end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_author_id) { source_user.mapped_user_id } + let(:event_attrs) do + { + user_id: mapped_author_id, + label_id: label_id, + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do @@ -167,14 +158,199 @@ it_behaves_like 'push placeholder reference' end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_author_id) { user_namespace.owner_id } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end + + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end + + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_author_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end + + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end + + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + end + end end context 'when user mapping is disabled' do - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + let(:mapped_author_id) { user.id } + let(:event_attrs) do + { + user_id: mapped_author_id, + label_id: label_id, + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) end end diff --git a/spec/lib/gitlab/github_import/importer/events/changed_milestone_spec.rb b/spec/lib/gitlab/github_import/importer/events/changed_milestone_spec.rb index 2b3bf52bd46e95..077c808afe754f 100644 --- a/spec/lib/gitlab/github_import/importer/events/changed_milestone_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/changed_milestone_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedMilestone, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedMilestone, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -15,7 +23,7 @@ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => event_type, 'commit_id' => nil, 'milestone_title' => milestone_title, @@ -25,16 +33,7 @@ ) end - let(:event_attrs) do - { - user_id: user.id, - milestone_id: milestone.id, - state: 'opened', - created_at: issue_event.created_at, - imported_from: 'github' - - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } shared_examples 'new event' do context 'when a matching milestone exists in GitLab' do @@ -61,16 +60,11 @@ let(:milestone_title) { milestone.title } it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(ResourceMilestoneEvent), - :user_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['ResourceMilestoneEvent', an_instance_of(Integer), 'user_id', source_user.id] + ]) end end @@ -78,10 +72,9 @@ let(:milestone_title) { milestone.title } it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) + + expect(cached_references).to be_empty end end @@ -94,19 +87,16 @@ end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + let(:event_attrs) do + { + user_id: mapped_user_id, + milestone_id: milestone.id, + state: 'opened', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do @@ -146,13 +136,120 @@ it_behaves_like 'push placeholder reference' end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_user_id) { user_namespace.owner_id } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + context 'when importing a milestoned event' do + let(:event_type) { 'milestoned' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when importing demilestoned event' do + let(:event_type) { 'demilestoned' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + context 'when importing a milestoned event' do + let(:event_type) { 'milestoned' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when importing demilestoned event' do + let(:event_type) { 'demilestoned' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder reference' + end + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + context 'when importing a milestoned event' do + let(:event_type) { 'milestoned' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + + context 'when importing demilestoned event' do + let(:event_type) { 'demilestoned' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + context 'when importing a milestoned event' do + let(:event_type) { 'milestoned' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + + context 'when importing demilestoned event' do + let(:event_type) { 'demilestoned' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + it_behaves_like 'push placeholder reference' + end + end + end + end end context 'when user mapping is disabled' do + let(:mapped_user_id) { user.id } + let(:event_attrs) do + { + user_id: mapped_user_id, + milestone_id: milestone.id, + state: 'opened', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) end end diff --git a/spec/lib/gitlab/github_import/importer/events/changed_reviewer_spec.rb b/spec/lib/gitlab/github_import/importer/events/changed_reviewer_spec.rb index 1a8428fa29d3b4..fe8222c9d2ca5e 100644 --- a/spec/lib/gitlab/github_import/importer/events/changed_reviewer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/changed_reviewer_spec.rb @@ -2,41 +2,40 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedReviewer, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedReviewer, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be_with_reload(:requested_reviewer) { create(:user) } let_it_be(:review_requester) { create(:user) } let_it_be(:issuable) { create(:merge_request, source_project: project, target_project: project) } let(:client) { instance_double('Gitlab::GithubImport::Client') } + let(:github_review_requester) { { 'id' => 1000, 'login' => 'github_author' } } + let(:github_requested_reviewer) { { 'id' => 1001, 'login' => 'another_github_user' } } let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => review_requester.id, 'login' => review_requester.username }, + 'actor' => github_review_requester, 'event' => event_type, 'commit_id' => nil, 'created_at' => '2022-04-26 18:30:53 UTC', - 'review_requester' => { 'id' => review_requester.id, 'login' => review_requester.username }, - 'requested_reviewer' => { 'id' => requested_reviewer.id, 'login' => requested_reviewer.username }, + 'review_requester' => github_review_requester, + 'requested_reviewer' => github_requested_reviewer, 'issue' => { 'number' => issuable.iid, pull_request: issuable.is_a?(MergeRequest) } ) end - let(:note_attrs) do - { - noteable_id: issuable.id, - noteable_type: issuable.class.name, - project_id: project.id, - author_id: review_requester.id, - system: true, - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } let(:expected_system_note_metadata_attrs) do { @@ -67,10 +66,8 @@ ) end - context 'when requested_reviewer is nil' do - before do - requested_reviewer.username = nil - end + context 'when requested reviewer is not found on GitHub' do + let(:github_requested_reviewer) { { 'id' => nil, 'login' => nil } } it 'references `@ghost`' do importer.execute(issue_event) @@ -83,7 +80,9 @@ shared_examples 'process review_requested & review_request_removed MR events' do context 'when importing a review_requested event' do let(:event_type) { 'review_requested' } - let(:expected_note_attrs) { note_attrs.merge(note: "requested review from `@#{requested_reviewer.username}`") } + let(:expected_note_attrs) do + note_attrs.merge(note: "requested review from `@#{github_requested_reviewer['login']}`") + end it_behaves_like 'create expected notes' end @@ -91,7 +90,7 @@ context 'when importing a review_request_removed event' do let(:event_type) { 'review_request_removed' } let(:expected_note_attrs) do - note_attrs.merge(note: "removed review request for `@#{requested_reviewer.username}`") + note_attrs.merge(note: "removed review request for `@#{github_requested_reviewer['login']}`") end it_behaves_like 'create expected notes' @@ -100,27 +99,23 @@ shared_examples 'push placeholder reference' do let(:event_type) { 'changed' } - it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Note), - :author_id, - review_requester.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) + it 'pushes the reference' do importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Note', an_instance_of(Integer), 'author_id', source_user.id] + ]) end end shared_examples 'do not push placeholder reference' do let(:event_type) { 'changed' } - it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) + it 'does not push any reference' do importer.execute(issue_event) + + expect(cached_references).to be_empty end end @@ -132,52 +127,81 @@ end context 'when user mapping is enabled' do - let_it_be(:source_user_requester) do - create( - :import_source_user, - placeholder_user_id: review_requester.id, - source_user_identifier: review_requester.id, - source_username: review_requester.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - let_it_be(:source_user_requested) do - create( - :import_source_user, - placeholder_user_id: requested_reviewer.id, - source_user_identifier: requested_reviewer.id, - source_username: requested_reviewer.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let_it_be(:mapped_user_id) { source_user.mapped_user_id } + + let(:note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + project_id: project.id, + author_id: mapped_user_id, + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end let_it_be(:merge_request_reviewer) do create( :merge_request_reviewer, - user_id: requested_reviewer.id, + user_id: mapped_user_id, merge_request_id: issuable.id ) end - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end - it_behaves_like 'process review_requested & review_request_removed MR events' it_behaves_like 'push placeholder reference' + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_user_id) { user_namespace.owner.id } + + before_all do + project.update!(namespace: user_namespace) + end + + it_behaves_like 'process review_requested & review_request_removed MR events' + it_behaves_like 'do not push placeholder reference' + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it_behaves_like 'process review_requested & review_request_removed MR events' + it_behaves_like 'push placeholder reference' + end + end end context 'when user mapping is disabled' do + let(:note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + project_id: project.id, + author_id: review_requester.id, + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(requested_reviewer.id, requested_reviewer.username) - .and_return(requested_reviewer.id) - allow(finder).to receive(:find).with(review_requester.id, review_requester.username) + allow(finder).to receive(:find).with(github_review_requester['id'], github_review_requester['login']) .and_return(review_requester.id) + allow(finder).to receive(:find).with(github_requested_reviewer['id'], github_requested_reviewer['login']) + .and_return(requested_reviewer.id) end end diff --git a/spec/lib/gitlab/github_import/importer/events/closed_spec.rb b/spec/lib/gitlab/github_import/importer/events/closed_spec.rb index b25b051636dd82..d43269e89f437a 100644 --- a/spec/lib/gitlab/github_import/importer/events/closed_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/closed_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::Closed, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::Closed, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -18,7 +26,7 @@ 'id' => 6501124486, 'node_id' => 'CE_lADOHK9fA85If7x0zwAAAAGDf0mG', 'url' => 'https://api.github.com/repos/elhowm/test-import/issues/events/6501124486', - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => 'closed', 'created_at' => created_at.iso8601, 'commit_id' => commit_id, @@ -26,18 +34,7 @@ ) end - let(:expected_event_attrs) do - { - project_id: project.id, - author_id: user.id, - target_id: issuable.id, - target_type: issuable.class.name, - action: 'closed', - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } before do allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| @@ -96,58 +93,44 @@ end shared_examples 'push placeholder references' do - it 'pushes the references' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Event), - :author_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(ResourceStateEvent), - :user_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - + it 'pushes the reference' do importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Event', an_instance_of(Integer), 'author_id', source_user.id], + ['ResourceStateEvent', an_instance_of(Integer), 'user_id', source_user.id] + ]) end end shared_examples 'do not push placeholder references' do - it 'does not push any references' do - expect(subject) - .not_to receive(:push_with_record) - + it 'does not push any reference' do importer.execute(issue_event) + + expect(cached_references).to be_empty end end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + let(:expected_event_attrs) do + { + project_id: project.id, + author_id: mapped_user_id, + target_id: issuable.id, + target_type: issuable.class.name, + action: 'closed', + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, issue_id: issuable.id, state: 'closed', created_at: issue_event.created_at, @@ -163,7 +146,7 @@ let(:issuable) { create(:merge_request, source_project: project, target_project: project) } let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, merge_request_id: issuable.id, state: 'closed', created_at: issue_event.created_at, @@ -174,20 +157,116 @@ it_behaves_like 'new event' it_behaves_like 'push placeholder references' end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_user_id) { user_namespace.owner_id } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + let(:expected_state_event_attrs) do + { + user_id: mapped_user_id, + issue_id: issuable.id, + state: 'closed', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder references' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + let(:expected_state_event_attrs) do + { + user_id: mapped_user_id, + merge_request_id: issuable.id, + state: 'closed', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + + it_behaves_like 'new event' + it_behaves_like 'do not push placeholder references' + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + let(:expected_state_event_attrs) do + { + user_id: mapped_user_id, + issue_id: issuable.id, + state: 'closed', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + + it_behaves_like 'new event' + it_behaves_like 'push placeholder references' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + let(:expected_state_event_attrs) do + { + user_id: mapped_user_id, + merge_request_id: issuable.id, + state: 'closed', + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + + it_behaves_like 'new event' + it_behaves_like 'push placeholder references' + end + end + end end context 'when user mapping is disabled' do + let(:mapped_user_id) { user.id } + let(:expected_event_attrs) do + { + project_id: project.id, + author_id: mapped_user_id, + target_id: issuable.id, + target_type: issuable.class.name, + action: 'closed', + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) end - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) end context 'with Issue' do let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, issue_id: issuable.id, state: 'closed', created_at: issue_event.created_at, @@ -203,7 +282,7 @@ let(:issuable) { create(:merge_request, source_project: project, target_project: project) } let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, merge_request_id: issuable.id, state: 'closed', created_at: issue_event.created_at, diff --git a/spec/lib/gitlab/github_import/importer/events/cross_referenced_spec.rb b/spec/lib/gitlab/github_import/importer/events/cross_referenced_spec.rb index 3cbd091ffe7872..72fd23a27a97a7 100644 --- a/spec/lib/gitlab/github_import/importer/events/cross_referenced_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/cross_referenced_spec.rb @@ -3,10 +3,16 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::CrossReferenced, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } - let_it_be(:user) { create(:user) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:issue_iid) { 999 } @@ -19,7 +25,7 @@ 'id' => 6501124486, 'node_id' => 'CE_lADOHK9fA85If7x0zwAAAAGDf0mG', 'url' => 'https://api.github.com/repos/elhowm/test-import/issues/events/6501124486', - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => 'cross-referenced', 'source' => { 'type' => 'issue', @@ -34,18 +40,7 @@ end let(:pull_request_resource) { nil } - let(:expected_note_attrs) do - { - system: true, - noteable_type: issuable.class.name, - noteable_id: issuable.id, - project_id: project.id, - author_id: user.id, - note: expected_note_body, - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } shared_examples 'import cross-referenced event' do context 'when referenced in other issue' do @@ -69,6 +64,7 @@ it_behaves_like 'internal event tracking' do let(:event) { 'g_project_management_issue_cross_referenced' } let(:subject) { importer.execute(issue_event) } + let(:user) { mapped_user } before do # Trigger g_project_management_issue_created event before executing subject @@ -107,6 +103,12 @@ end context 'when referenced in out of project issue/pull_request' do + before do + allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| + allow(finder).to receive(:database_id).and_return(nil) + end + end + it 'does not create expected note' do importer.execute(issue_event) @@ -124,42 +126,36 @@ end it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Note), - :author_id, - issue_event[:actor].id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Note', an_instance_of(Integer), 'author_id', source_user.id] + ]) end end shared_examples 'do not push placeholder reference' do it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) - end - end - context 'when user_mapping_is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) + expect(cached_references).to be_empty end + end - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + context 'when user mapping is enabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let_it_be(:mapped_user) { source_user.mapped_user } + let(:expected_note_attrs) do + { + system: true, + noteable_type: issuable.class.name, + noteable_id: issuable.id, + project_id: project.id, + author_id: mapped_user.id, + note: expected_note_body, + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do @@ -173,13 +169,71 @@ it_behaves_like 'import cross-referenced event' it_behaves_like 'push a placeholder reference' end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let_it_be(:mapped_user) { user_namespace.owner } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + it_behaves_like 'import cross-referenced event' + it_behaves_like 'do not push placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'import cross-referenced event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let_it_be(:mapped_user) { source_user.mapped_user } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + it_behaves_like 'import cross-referenced event' + it_behaves_like 'push a placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'import cross-referenced event' + it_behaves_like 'push a placeholder reference' + end + end + end end - context 'when user_mapping_is disabled' do + context 'when user mapping is disabled' do + let_it_be(:mapped_user) { create(:user) } + let(:expected_note_attrs) do + { + system: true, + noteable_type: issuable.class.name, + noteable_id: issuable.id, + project_id: project.id, + author_id: mapped_user.id, + note: expected_note_body, + created_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(mapped_user.id) end end diff --git a/spec/lib/gitlab/github_import/importer/events/merged_spec.rb b/spec/lib/gitlab/github_import/importer/events/merged_spec.rb index a76bad65d77ce6..88eaf2632ef557 100644 --- a/spec/lib/gitlab/github_import/importer/events/merged_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/merged_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::Merged, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::Merged, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -18,7 +26,7 @@ 'id' => 6501124486, 'node_id' => 'CE_lADOHK9fA85If7x0zwAAAAGDf0mG', 'url' => 'https://api.github.com/repos/elhowm/test-import/issues/events/6501124486', - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => 'merged', 'created_at' => created_at.iso8601, 'commit_id' => commit_id, @@ -26,6 +34,8 @@ ) end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } + before do allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(merge_request.id) @@ -33,53 +43,27 @@ end shared_examples 'push placeholder references' do - it 'pushes the references' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Event), - :author_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(ResourceStateEvent), - :user_id, - user.id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - + it 'pushes the reference' do importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Event', an_instance_of(Integer), 'author_id', source_user.id], + ['ResourceStateEvent', an_instance_of(Integer), 'user_id', source_user.id], + ['MergeRequest::Metrics', an_instance_of(Integer), 'merged_by_id', source_user.id] + ]) end end - shared_examples 'do not push placeholder references' do - it 'does not push references' do - expect(subject) - .not_to receive(:push_with_record) - + shared_examples 'do not push placeholder reference' do + it 'does not push any reference' do importer.execute(issue_event) + + expect(cached_references).to be_empty end end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end + let_it_be(:source_user) { generate_source_user(project, 1000) } it 'creates expected event and state event' do importer.execute(issue_event) @@ -87,7 +71,7 @@ expect(merge_request.events.count).to eq 1 expect(merge_request.events.first).to have_attributes( project_id: project.id, - author_id: user.id, + author_id: source_user.mapped_user_id, target_id: merge_request.id, target_type: merge_request.class.name, action: 'merged', @@ -98,7 +82,7 @@ expect(merge_request.resource_state_events.count).to eq 1 expect(merge_request.resource_state_events.first).to have_attributes( - user_id: user.id, + user_id: source_user.mapped_user_id, merge_request_id: merge_request.id, state: 'merged', created_at: issue_event.created_at, @@ -144,64 +128,106 @@ end it_behaves_like 'push placeholder references' - end - context 'when user mapping is disabled' do - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) - allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) end - end - it 'creates expected event and state event' do - importer.execute(issue_event) + it 'creates expected event and state event mapped to personal namespace owner' do + importer.execute(issue_event) - expect(merge_request.events.count).to eq 1 - expect(merge_request.events.first).to have_attributes( - project_id: project.id, - author_id: user.id, - target_id: merge_request.id, - target_type: merge_request.class.name, - action: 'merged', - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - ) + expect(merge_request.events.count).to eq 1 + expect(merge_request.events.first.author_id).to eq(user_namespace.owner_id) - expect(merge_request.resource_state_events.count).to eq 1 - expect(merge_request.resource_state_events.first).to have_attributes( - user_id: user.id, - merge_request_id: merge_request.id, - state: 'merged', - created_at: issue_event.created_at, - close_after_error_tracking_resolve: false, - close_auto_resolve_prometheus_alert: false - ) - end + expect(merge_request.resource_state_events.count).to eq 1 + expect(merge_request.resource_state_events.first.user_id).to eq(user_namespace.owner_id) + end + + it_behaves_like 'do not push placeholder reference' - it 'creates a merged by note' do - expect { importer.execute(issue_event) }.to change { Note.count }.by(1) + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } - last_note = merge_request.notes.last - expect(last_note.created_at).to eq(issue_event.created_at) - expect(last_note.author).to eq(merge_request.author) - expect(last_note.note).to eq("*Merged by: #{user.username} at #{issue_event.created_at}*") + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'creates expected event and state event' do + importer.execute(issue_event) + + expect(merge_request.events.count).to eq 1 + expect(merge_request.events.first.author_id).to eq(source_user.mapped_user_id) + + expect(merge_request.resource_state_events.count).to eq 1 + expect(merge_request.resource_state_events.first.user_id).to eq(source_user.mapped_user_id) + end + + it_behaves_like 'push placeholder references' + end end - context 'when commit ID is present' do - let!(:commit) { create(:commit, project: project) } - let(:commit_id) { commit.id } + context 'when user mapping is disabled' do + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) + end + end it 'creates expected event and state event' do importer.execute(issue_event) expect(merge_request.events.count).to eq 1 - state_event = merge_request.resource_state_events.last - expect(state_event.source_commit).to eq commit_id[0..40] + expect(merge_request.events.first).to have_attributes( + project_id: project.id, + author_id: user.id, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: 'merged', + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + ) + + expect(merge_request.resource_state_events.count).to eq 1 + expect(merge_request.resource_state_events.first).to have_attributes( + user_id: user.id, + merge_request_id: merge_request.id, + state: 'merged', + created_at: issue_event.created_at, + close_after_error_tracking_resolve: false, + close_auto_resolve_prometheus_alert: false + ) end - end - it_behaves_like 'do not push placeholder references' + it 'creates a merged by note' do + expect { importer.execute(issue_event) }.to change { Note.count }.by(1) + + last_note = merge_request.notes.last + expect(last_note.created_at).to eq(issue_event.created_at) + expect(last_note.author).to eq(merge_request.author) + expect(last_note.note).to eq("*Merged by: github_author at #{issue_event.created_at}*") + end + + context 'when commit ID is present' do + let!(:commit) { create(:commit, project: project) } + let(:commit_id) { commit.id } + + it 'creates expected event and state event' do + importer.execute(issue_event) + + expect(merge_request.events.count).to eq 1 + state_event = merge_request.resource_state_events.last + expect(state_event.source_commit).to eq commit_id[0..40] + end + end + + it_behaves_like 'do not push placeholder reference' + end end end diff --git a/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb b/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb index 65e3d3b1ad51e9..e4cc60d5a66783 100644 --- a/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb @@ -2,10 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::Renamed, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::Renamed, :clean_gitlab_redis_shared_state, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + let_it_be(:user) { create(:user) } let(:issuable) { create(:issue, project: project) } @@ -14,7 +22,7 @@ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => 'renamed', 'commit_id' => nil, 'created_at' => '2022-04-26 18:30:53 UTC', @@ -24,19 +32,7 @@ ) end - let(:expected_note_attrs) do - { - noteable_id: issuable.id, - noteable_type: issuable.class.name, - project_id: project.id, - author_id: user.id, - note: "changed title from **{-old-} title** to **{+new+} title**", - system: true, - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } let(:expected_system_note_metadata_attrs) do { @@ -77,42 +73,37 @@ shared_examples 'push a placeholder reference' do it 'pushes the reference' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Note), - :author_id, - issue_event[:actor].id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Note', an_instance_of(Integer), 'author_id', source_user.id] + ]) end end shared_examples 'do not push placeholder reference' do it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) + + expect(cached_references).to be_empty end end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + let(:expected_note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + project_id: project.id, + author_id: mapped_user_id, + note: "changed title from **{-old-} title** to **{+new+} title**", + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end context 'with Issue' do @@ -126,13 +117,72 @@ it_behaves_like 'import renamed event' it_behaves_like 'push a placeholder reference' end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:mapped_user_id) { user_namespace.owner_id } + + before_all do + project.update!(namespace: user_namespace) + end + + context 'with Issue' do + it_behaves_like 'import renamed event' + it_behaves_like 'do not push placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'import renamed event' + it_behaves_like 'do not push placeholder reference' + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1000) } + let(:mapped_user_id) { source_user.mapped_user_id } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + context 'with Issue' do + it_behaves_like 'import renamed event' + it_behaves_like 'push a placeholder reference' + end + + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'import renamed event' + it_behaves_like 'push a placeholder reference' + end + end + end end context 'when user mapping is disabled' do + let(:mapped_user_id) { user.id } + let(:expected_note_attrs) do + { + noteable_id: issuable.id, + noteable_type: issuable.class.name, + project_id: project.id, + author_id: mapped_user_id, + note: "changed title from **{-old-} title** to **{+new+} title**", + system: true, + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) end end diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb index 19e5e9717be1af..99f23fe8ec43ba 100644 --- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb @@ -7,7 +7,15 @@ let_it_be(:work_item_type_id) { ::WorkItems::Type.default_issue_type.id } let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, :with_import_url, group: group) } + + let_it_be_with_reload(:project) do + create( + :project, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled, + group: group + ) + end + let_it_be(:milestone) { create(:milestone, project: project) } let(:client) { instance_double(Gitlab::GithubImport::Client) } @@ -36,30 +44,12 @@ ) end - let(:user_mapping_enabled) { true } - let_it_be(:source_user_alice) do - create( - :import_source_user, - source_user_identifier: '4', - source_hostname: project.safe_import_url, - namespace_id: group.id - ) - end - - let_it_be(:source_user_bob) do - create( - :import_source_user, - source_user_identifier: '5', - source_hostname: project.safe_import_url, - namespace_id: group.id - ) - end + let_it_be(:source_user_alice) { generate_source_user(project, '4') } + let_it_be(:source_user_bob) { generate_source_user(project, '5') } - let(:importer) { described_class.new(issue, project, client) } + let(:cached_references) { placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) } - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: user_mapping_enabled }) - end + subject(:importer) { described_class.new(issue, project, client) } describe '.import_if_issue' do it 'imports an issuable if it is a regular issue' do @@ -112,9 +102,8 @@ importer.execute created_issue = Issue.last - user_references = placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) - expect(user_references).to match_array([ + expect(cached_references).to match_array([ ['Issue', created_issue.id, 'author_id', source_user_alice.id], [ 'IssueAssignee', { 'user_id' => source_user_alice.mapped_user_id, 'issue_id' => created_issue.id }, @@ -141,10 +130,79 @@ expect(Issue.last.description).to eq("You can ask `@knejad` by emailing xyz@gitlab.com") end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + importer.execute + + expect(cached_references).to be_empty + end + + it 'imports the issue mapped to the personal namespace owner' do + expect { importer.execute }.to change { Issue.count }.by(1) + + expect(Issue.last).to have_attributes( + iid: 42, + title: 'My Issue', + author_id: user_namespace.owner_id, + assignee_ids: contain_exactly(user_namespace.owner_id) + ) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:namespace_import_user) { create(:namespace_import_user, namespace: user_namespace) } + let_it_be(:source_user_alice) do + generate_source_user(project, '4', placeholder_user: namespace_import_user.import_user) + end + + let_it_be(:source_user_bob) do + generate_source_user(project, '5', placeholder_user: namespace_import_user.import_user) + end + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + importer.execute + + created_issue = Issue.last + + expect(cached_references).to match_array([ + ['Issue', created_issue.id, 'author_id', source_user_alice.id], + [ + 'IssueAssignee', { 'user_id' => namespace_import_user.user_id, 'issue_id' => created_issue.id }, + 'user_id', instance_of(Integer) + ] + ]) + end + + it 'imports the issue mapped to import users' do + expect { importer.execute }.to change { Issue.count }.by(1) + + expect(Issue.last).to have_attributes( + iid: 42, + title: 'My Issue', + author_id: namespace_import_user.user_id, + assignee_ids: contain_exactly(namespace_import_user.user_id) + ) + end + end + end end context 'when user_mapping is not enabled' do - let(:user_mapping_enabled) { false } + before_all do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! + end describe '.import_if_issue' do it 'imports an issuable if it is a regular issue' do diff --git a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb index 463f0a33522f24..7a4c985f8a28df 100644 --- a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb @@ -3,19 +3,19 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::NoteImporter, feature_category: :importers do - let_it_be(:imported_from) { ::Import::HasImportSource::IMPORT_SOURCES[:github] } - let_it_be(:project) { create(:project, :with_import_url) } - let_it_be(:user) { create(:user) } + include Import::UserMappingHelper - let_it_be(:source_user) do - create(:import_source_user, - placeholder_user_id: user.id, - namespace_id: project.root_ancestor.id, - source_user_identifier: '4', - source_hostname: project.safe_import_url + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled ) end + let_it_be(:imported_from) { ::Import::HasImportSource::IMPORT_SOURCES[:github] } + let_it_be(:user) { create(:user) } + let_it_be(:source_user) { generate_source_user(project, '4') } + let(:client) { double(:client) } let(:created_at) { Time.new(2017, 1, 1, 12, 00) } let(:updated_at) { Time.new(2017, 1, 1, 12, 15) } @@ -35,13 +35,8 @@ end let(:importer) { described_class.new(github_note, project, client) } - let(:user_mapping_enabled) { true } - - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: user_mapping_enabled }) - end - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do context 'when the noteable exists' do let!(:issue_row) { create(:issue, project: project, iid: 1) } @@ -51,7 +46,7 @@ .and_return(issue_row.id) end - context 'when user_mapping_enabled is true' do + context 'when user contribution mapping is enabled' do it 'maps the correct user and pushes a reference' do expect(importer.user_finder).to receive(:author_id_for).with(github_note).and_call_original @@ -91,12 +86,83 @@ importer.execute end - end - context 'when user_mapping_enabled is false' do - let(:user_mapping_enabled) { false } + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let(:cached_references) { placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + importer.execute + + expect(cached_references).to be_empty + end + + it 'imports the note mapped to the personal namespace owner' do + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) + .with( + Note.table_name, + [ + { + noteable_type: 'Issue', + noteable_id: issue_row.id, + project_id: project.id, + namespace_id: project.project_namespace_id, + author_id: user_namespace.owner_id, + note: 'This is my note', + discussion_id: match(/\A[0-9a-f]{40}\z/), + system: false, + created_at: created_at, + updated_at: updated_at, + imported_from: imported_from + } + ], + { return_ids: true } + ) + .and_call_original + + importer.execute + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, '4') } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['Note', instance_of(Integer), 'author_id', source_user.id] + ) + end + + it 'imports the note mapped to the placeholder user' do + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) + .with( + Note.table_name, + [a_hash_including(author_id: source_user.mapped_user_id)], + { return_ids: true } + ).and_call_original + + importer.execute + end + end + end + end + context 'when user contribution mapping is disabled' do before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow(importer.user_finder) .to receive(:email_for_github_username) .and_return('alice@alice.com') diff --git a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb index f204328ebbc2f0..da15e5bdd6e7da 100644 --- a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb @@ -5,7 +5,15 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter, feature_category: :importers do include Import::UserMappingHelper - subject(:importer) { described_class.new(github_protected_branch, project, client) } + let_it_be(:namespace_setting) { create(:namespace_settings, default_branch_protection_defaults: {}) } + let_it_be(:group) { create(:group, namespace_settings: namespace_setting) } + let_it_be_with_reload(:project) do + create( + :project, :repository, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled, + group: group + ) + end let(:branch_name) { 'protection' } let(:allow_force_pushes_on_github) { false } @@ -32,17 +40,15 @@ ) end - let(:project) do - create(:project, :repository, :with_import_url, :import_user_mapping_enabled, import_type: Import::SOURCE_GITHUB) - end - let(:client) { instance_double('Gitlab::GithubImport::Client') } + subject(:importer) { described_class.new(github_protected_branch, project, client) } + before do allow(client).to receive(:user).and_return({ name: 'Github user name' }) end - describe '#execute' do + describe '#execute', :clean_gitlab_redis_shared_state do let(:create_service) { instance_double('ProtectedBranches::CreateService') } shared_examples 'create branch protection by the strictest ruleset' do @@ -287,37 +293,85 @@ ]) end + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + importer.execute + + expect(user_references).to be_empty + end + + it 'creates protected branch mapped to the personal namespace owner' do + expect { importer.execute }.to change(ProtectedBranch, :count).by(1) + .and change(ProtectedBranch::PushAccessLevel, :count).by(1) + .and change(ProtectedBranch::MergeAccessLevel, :count).by(1) + + imported_push_access_level = project.protected_branches.first.push_access_levels.first + expect(imported_push_access_level.user_id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + importer.execute + + imported_push_access_level = project.protected_branches.first.push_access_levels.first + source_user = Import::SourceUser.last + + expect(user_references).to match_array([ + ['ProtectedBranch::PushAccessLevel', imported_push_access_level.id, 'user_id', source_user.id] + ]) + end + + it 'creates protected branch mapped to the placeholder user' do + importer.execute + + imported_push_access_level = project.protected_branches.first.push_access_levels.first + source_user = Import::SourceUser.last + + expect(imported_push_access_level.user_id).to eq(source_user.mapped_user_id) + end + end + end + context 'when user mapping is disabled' do - let(:owner_id) { project.owner.id } - let(:owner_username) { project.owner.username } - let(:other_user) { create(:user) } - let(:other_user_id) { other_user.id } - let(:other_user_username) { other_user.username } - let(:allowed_to_push_users) do + let_it_be(:user_1) { create(:user) } + let_it_be(:user_2) { create(:user) } + let_it_be(:allowed_to_push_users) do [ - Gitlab::GithubImport::Representation::User.new(id: owner_id, login: owner_username), - Gitlab::GithubImport::Representation::User.new(id: other_user_id, login: other_user_username) + Gitlab::GithubImport::Representation::User.new(id: user_1.id, login: user_1.username), + Gitlab::GithubImport::Representation::User.new(id: user_2.id, login: user_2.username) ] end before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! end context 'when the users are found on GitLab' do let(:push_access_levels_number) { 2 } let(:push_access_levels_attributes) do [ - { user_id: owner_id, importing: true }, - { user_id: other_user_id, importing: true } + { user_id: user_1.id, importing: true }, + { user_id: user_2.id, importing: true } ] end before do - project.add_member(other_user, :maintainer) + project.add_members([user_1, user_2], :maintainer) allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(owner_id, owner_username).and_return(owner_id) - allow(finder).to receive(:find).with(other_user_id, other_user_username).and_return(other_user_id) + allow(finder).to receive(:find).with(user_1.id, user_1.username).and_return(user_1.id) + allow(finder).to receive(:find).with(user_2.id, user_2.username).and_return(user_2.id) end end @@ -334,14 +388,15 @@ let(:push_access_levels_number) { 1 } let(:push_access_levels_attributes) do [ - { user_id: owner_id, importing: true } + { user_id: user_1.id, importing: true } ] end before do + project.add_member(user_1, :maintainer) allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(owner_id, owner_username).and_return(owner_id) - allow(finder).to receive(:find).with(other_user_id, other_user_username).and_return(other_user_id) + allow(finder).to receive(:find).with(user_1.id, user_1.username).and_return(user_1.id) + allow(finder).to receive(:find).with(user_2.id, user_2.username).and_return(user_2.id) end end @@ -449,10 +504,6 @@ end context 'when Maintainers and Developers can merge' do - before do - merge_access_level.update_column(:access_level, Gitlab::Access::DEVELOPER) - end - let(:gitlab_push_access_level) { push_access_level.access_level } let(:gitlab_merge_access_level) { merge_access_level.access_level } let(:expected_push_access_level) { gitlab_push_access_level } @@ -461,19 +512,25 @@ Gitlab::GithubImport::Importer::ProtectedBranchImporter::GITHUB_DEFAULT_MERGE_ACCESS_LEVEL end + before do + merge_access_level.update_column(:access_level, Gitlab::Access::DEVELOPER) + end + it_behaves_like 'create branch protection by the strictest ruleset' end end context 'when there is no branch protection rule for the role' do - before do - push_access_level.update_column(:user_id, project.owner.id) - merge_access_level.update_column(:user_id, project.owner.id) - end + let_it_be(:project_owner) { create(:user, owner_of: project) } let(:expected_push_access_level) { ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL } let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER } + before do + push_access_level.update_column(:user_id, project_owner.id) + merge_access_level.update_column(:user_id, project_owner.id) + end + it_behaves_like 'create branch protection by the strictest ruleset' end end diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index ac31f638a6391f..505219350a45d7 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -5,33 +5,17 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - let_it_be(:imported_from) { Import::HasImportSource::IMPORT_SOURCES[:github] } - let_it_be(:user_representation_1) { Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice') } - let_it_be(:user_representation_2) { Gitlab::GithubImport::Representation::User.new(id: 5, login: 'bob') } let_it_be_with_reload(:project) do - create(:project, :repository, :with_import_url, :import_user_mapping_enabled, import_type: Import::SOURCE_GITHUB) - end - - let_it_be(:source_user_1) do create( - :import_source_user, - source_user_identifier: user_representation_1.id, - source_hostname: project.safe_import_url, - import_type: Import::SOURCE_GITHUB, - namespace: project.root_ancestor - ) - end - - let_it_be(:source_user_2) do - create( - :import_source_user, - source_user_identifier: user_representation_2.id, - source_hostname: project.safe_import_url, - import_type: Import::SOURCE_GITHUB, - namespace: project.root_ancestor + :project, :repository, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled ) end + let_it_be(:user_representation_1) { Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice') } + let_it_be(:user_representation_2) { Gitlab::GithubImport::Representation::User.new(id: 5, login: 'bob') } + let_it_be(:source_user_1) { generate_source_user(project, user_representation_1.id) } + let_it_be(:source_user_2) { generate_source_user(project, user_representation_2.id) } let_it_be(:user) { create(:user) } let(:client) { instance_double(Gitlab::GithubImport::Client) } @@ -68,6 +52,7 @@ end let(:pull_request) { Gitlab::GithubImport::Representation::PullRequest.new(pull_request_attributes) } + let(:user_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } let(:importer) { described_class.new(pull_request, project, client) } @@ -124,8 +109,6 @@ it 'pushes placeholder references to the store' do importer.execute - - user_references = placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) created_merge_request = MergeRequest.last created_mr_assignee = created_merge_request.merge_request_assignees.first # we only import one PR assignee @@ -135,6 +118,59 @@ ]) end + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + importer.execute + + expect(user_references).to be_empty + end + + it 'imports the pull request mapped to the personal namespace owner' do + expect { importer.execute }.to change { MergeRequest.count }.from(0).to(1) + + created_merge_request = MergeRequest.last + expect(created_merge_request.author_id).to eq(user_namespace.owner_id) + expect(created_merge_request.assignee_ids).to contain_exactly(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user_1) { generate_source_user(project, user_representation_1.id) } + let_it_be(:source_user_2) { generate_source_user(project, user_representation_2.id) } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + importer.execute + + created_merge_request = MergeRequest.last + created_mr_assignee = created_merge_request.merge_request_assignees.first + + expect(user_references).to match_array([ + ['MergeRequest', created_merge_request.id, 'author_id', source_user_1.id], + ['MergeRequestAssignee', created_mr_assignee.id, 'user_id', source_user_2.id] + ]) + end + + it 'imports the pull request mapped to the placeholder users' do + expect { importer.execute }.to change { MergeRequest.count }.from(0).to(1) + + created_merge_request = MergeRequest.last + expect(created_merge_request.author_id).to eq(source_user_1.mapped_user_id) + expect(created_merge_request.assignee_ids).to contain_exactly(source_user_2.mapped_user_id) + end + end + end + context 'when the description is processed for formatting' do let(:description) { "I said to @sam_allen\0 the code should follow @bob's\0 advice. @.ali-ce/group#9?\0" } let(:expected_description) do @@ -230,7 +266,7 @@ end context 'when merge request is open' do - let(:project) { create(:project, :repository, :with_import_url, :import_user_mapping_enabled) } + let(:project) { create(:project, :repository, :in_group, :github_import, :import_user_mapping_enabled) } let(:state) { :opened } before do @@ -331,8 +367,6 @@ it 'does not push any placeholder references' do importer.execute - user_references = placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) - expect(user_references).to be_empty end end @@ -364,8 +398,6 @@ it 'does not push any placeholder references' do importer.execute - user_references = placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) - expect(user_references).to be_empty end end diff --git a/spec/lib/gitlab/github_import/importer/pull_requests/merged_by_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests/merged_by_importer_spec.rb index 5bd19bcc2f1458..9f9ab77cff5386 100644 --- a/spec/lib/gitlab/github_import/importer/pull_requests/merged_by_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_requests/merged_by_importer_spec.rb @@ -5,28 +5,23 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequests::MergedByImporter, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - let_it_be(:project) do - create(:project, :with_import_url, :import_user_mapping_enabled, import_type: Import::SOURCE_GITHUB) - end - - let_it_be(:merge_request) { create(:merged_merge_request, project: project) } - let_it_be(:merger_source_user) do + let_it_be_with_reload(:project) do create( - :import_source_user, - source_user_identifier: 999, - source_hostname: project.safe_import_url, - import_type: Import::SOURCE_GITHUB, - namespace: project.root_ancestor + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled ) end + let_it_be(:merge_request) { create(:merged_merge_request, project: project) } + let_it_be(:merger_source_user) { generate_source_user(project, 999) } + let(:merged_at) { Time.utc(2017, 1, 1, 12) } + let(:merger_user) { { id: 999, login: 'merger' } } + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } let(:client_double) do instance_double(Gitlab::GithubImport::Client, user: { id: 999, login: 'merger', email: 'merger@email.com' }) end - let(:merger_user) { { id: 999, login: 'merger' } } - let(:pull_request) do Gitlab::GithubImport::Representation::PullRequest.from_api_response( { @@ -57,17 +52,62 @@ it 'pushes placeholder references to the store' do subject.execute - user_references = placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) metrics = merge_request.metrics.reload - expect(user_references).to match_array([ + expect(cached_references).to match_array([ ['MergeRequest::Metrics', metrics.id, 'merged_by_id', merger_source_user.id] ]) end + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + subject.execute + + expect(cached_references).to be_empty + end + + it 'imports the merged_by mapped to the personal namespace owner' do + subject.execute + + expect(merge_request.metrics.reload.merged_by_id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:merger_source_user) { generate_source_user(project, 999) } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + subject.execute + + metrics = merge_request.metrics.reload + + expect(cached_references).to match_array([ + ['MergeRequest::Metrics', metrics.id, 'merged_by_id', merger_source_user.id] + ]) + end + + it 'imports the merged_by mapped to the placeholder user' do + subject.execute + + expect(merge_request.metrics.reload.merged_by_id).to eq(merger_source_user.mapped_user_id) + end + end + end + context 'when user mapping is disabled' do before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! end shared_examples 'adds a note referencing the merger user' do diff --git a/spec/lib/gitlab/github_import/importer/pull_requests/review_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests/review_importer_spec.rb index d24b643df82b90..2f7e8f037ba48a 100644 --- a/spec/lib/gitlab/github_import/importer/pull_requests/review_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_requests/review_importer_spec.rb @@ -7,22 +7,16 @@ using RSpec::Parameterized::TableSyntax let_it_be_with_reload(:project) do - create(:project, :with_import_url, :import_user_mapping_enabled, import_type: Import::SOURCE_GITHUB) - end - - let_it_be(:source_user) do create( - :import_source_user, - source_user_identifier: 999, - source_hostname: project.safe_import_url, - import_type: Import::SOURCE_GITHUB, - namespace: project.root_ancestor + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled ) end let_it_be_with_reload(:merge_request) { create(:merge_request, source_project: project) } - let(:submitted_at) { Time.new(2017, 1, 1, 12).utc } + let_it_be(:source_user) { generate_source_user(project, 999) } + let(:submitted_at) { Time.new(2017, 1, 1, 12).utc } let(:client_double) do instance_double( 'Gitlab::GithubImport::Client', @@ -89,51 +83,87 @@ end context 'when user mapping is enabled' do - let_it_be(:author) { source_user.mapped_user } + shared_examples 'imports without pushing placeholder references' do + it 'does not push placeholder references' do + subject.execute - context 'when the review has no note text' do - context 'when the review is "APPROVED"' do - let(:review) { create_review(type: 'APPROVED', note: '') } + expect(user_references).to be_empty + end + end + + shared_examples 'importing a review' do |push_placeholder_references:| + context 'when the review has no note text' do + context 'when the review is "APPROVED"' do + let(:review) { create_review(type: 'APPROVED', note: '') } - it_behaves_like 'imports an approval for the Merge Request' - it_behaves_like 'imports a reviewer for the Merge Request' + it_behaves_like 'imports an approval for the Merge Request' + it_behaves_like 'imports a reviewer for the Merge Request' - it 'creates a note for the review' do - expect { subject.execute }.to change { Note.count }.by(1) + it 'creates a note for the review' do + expect { subject.execute }.to change { Note.count }.by(1) - last_note = merge_request.notes.last - expect(last_note.note).to eq('approved this merge request') - expect(last_note.author).to eq(author) - expect(last_note.created_at).to eq(submitted_at) - expect(last_note.system_note_metadata.action).to eq('approved') - end + last_note = merge_request.notes.last + expect(last_note.note).to eq('approved this merge request') + expect(last_note.author).to eq(author) + expect(last_note.created_at).to eq(submitted_at) + expect(last_note.system_note_metadata.action).to eq('approved') + end - it 'pushes placeholder references for reviewer and system note' do - subject.execute + it 'pushes placeholder references for reviewer and system note', if: push_placeholder_references do + subject.execute - created_approval = merge_request.approvals.last - created_reviewer = merge_request.merge_request_reviewers.last - system_note = merge_request.notes.last + created_approval = merge_request.approvals.last + created_reviewer = merge_request.merge_request_reviewers.last + system_note = merge_request.notes.last - expect(user_references).to match_array([ - ['Approval', created_approval.id, 'user_id', source_user.id], - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], - ['Note', system_note.id, 'author_id', source_user.id] - ]) - end + expect(user_references).to match_array([ + ['Approval', created_approval.id, 'user_id', source_user.id], + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], + ['Note', system_note.id, 'author_id', source_user.id] + ]) + end - context 'when the user already approved the merge request' do - before do - create(:approval, merge_request: merge_request, user: author) + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' end - it 'does not import second approve and note' do - expect { subject.execute } - .to not_change { Note.count } - .and not_change { Approval.count } + context 'when the user already approved the merge request' do + before do + create(:approval, merge_request: merge_request, user: author) + end + + it 'does not import second approve and note' do + expect { subject.execute } + .to not_change { Note.count } + .and not_change { Approval.count } + end + + it 'only pushes placeholder references for reviewer', if: push_placeholder_references do + subject.execute + + created_reviewer = merge_request.merge_request_reviewers.last + + expect(user_references).to match_array([ + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id] + ]) + end + + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end + end + end + + context 'when the review is "COMMENTED"' do + let(:review) { create_review(type: 'COMMENTED', note: '') } + + it_behaves_like 'imports a reviewer for the Merge Request' + + it 'does not create note for the review' do + expect { subject.execute }.not_to change { Note.count } end - it 'only pushes placeholder references for reviewer' do + it 'only pushes placeholder references for reviewer', if: push_placeholder_references do subject.execute created_reviewer = merge_request.merge_request_reviewers.last @@ -142,143 +172,174 @@ ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id] ]) end + + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end end - end - context 'when the review is "COMMENTED"' do - let(:review) { create_review(type: 'COMMENTED', note: '') } + context 'when the review is "CHANGES_REQUESTED"' do + let(:review) { create_review(type: 'CHANGES_REQUESTED', note: '') } + + it_behaves_like 'imports a reviewer for the Merge Request' - it_behaves_like 'imports a reviewer for the Merge Request' + it 'does not create a note for the review' do + expect { subject.execute }.not_to change { Note.count } + end - it 'does not create note for the review' do - expect { subject.execute }.not_to change { Note.count } - end + it 'only pushes placeholder references for reviewer', if: push_placeholder_references do + subject.execute - it 'only pushes placeholder references for reviewer' do - subject.execute + created_reviewer = merge_request.merge_request_reviewers.last - created_reviewer = merge_request.merge_request_reviewers.last + expect(user_references).to match_array([ + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id] + ]) + end - expect(user_references).to match_array([ - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id] - ]) + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end end end - context 'when the review is "CHANGES_REQUESTED"' do - let(:review) { create_review(type: 'CHANGES_REQUESTED', note: '') } + context 'when the review has a note text' do + context 'when the review is "APPROVED"' do + let(:review) { create_review(type: 'APPROVED') } - it_behaves_like 'imports a reviewer for the Merge Request' + it_behaves_like 'imports an approval for the Merge Request' + it_behaves_like 'imports a reviewer for the Merge Request' - it 'does not create a note for the review' do - expect { subject.execute }.not_to change { Note.count } - end + it 'creates a note for the review' do + expect { subject.execute }.to change { Note.count }.by(2) + + note = merge_request.notes.where(system: false).last + expect(note.note).to eq("**Review:** Approved\n\nnote") + expect(note.author).to eq(author) + expect(note.created_at).to eq(submitted_at) - it 'only pushes placeholder references for reviewer' do - subject.execute + system_note = merge_request.notes.where(system: true).last + expect(system_note.note).to eq('approved this merge request') + expect(system_note.author).to eq(author) + expect(system_note.created_at).to eq(submitted_at) + expect(system_note.system_note_metadata.action).to eq('approved') + end + + it 'pushes placeholder references for reviewer, system note, and reviewer note', + if: push_placeholder_references do + subject.execute + + created_approval = merge_request.approvals.last + created_reviewer = merge_request.merge_request_reviewers.last + system_note = merge_request.notes.where(system: true).last + note = merge_request.notes.where(system: false).last - created_reviewer = merge_request.merge_request_reviewers.last + expect(user_references).to match_array([ + ['Approval', created_approval.id, 'user_id', source_user.id], + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], + ['Note', system_note.id, 'author_id', source_user.id], + ['Note', note.id, 'author_id', source_user.id] + ]) + end - expect(user_references).to match_array([ - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id] - ]) + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end end - end - end - context 'when the review has a note text' do - context 'when the review is "APPROVED"' do - let(:review) { create_review(type: 'APPROVED') } + context 'when the review is "COMMENTED"' do + let(:review) { create_review(type: 'COMMENTED') } - it_behaves_like 'imports an approval for the Merge Request' - it_behaves_like 'imports a reviewer for the Merge Request' + it 'creates a note for the review' do + expect { subject.execute } + .to change { Note.count }.by(1) + .and not_change(Approval, :count) - it 'creates a note for the review' do - expect { subject.execute }.to change { Note.count }.by(2) + last_note = merge_request.notes.last - note = merge_request.notes.where(system: false).last - expect(note.note).to eq("**Review:** Approved\n\nnote") - expect(note.author).to eq(author) - expect(note.created_at).to eq(submitted_at) + expect(last_note.note).to eq("**Review:** Commented\n\nnote") + expect(last_note.author).to eq(author) + expect(last_note.created_at).to eq(submitted_at) + end - system_note = merge_request.notes.where(system: true).last - expect(system_note.note).to eq('approved this merge request') - expect(system_note.author).to eq(author) - expect(system_note.created_at).to eq(submitted_at) - expect(system_note.system_note_metadata.action).to eq('approved') - end + it 'pushes placeholder references for reviewer and reviewer note', if: push_placeholder_references do + subject.execute - it 'pushes placeholder references for reviewer, system note, and reviewer note' do - subject.execute + created_reviewer = merge_request.merge_request_reviewers.last + note = merge_request.notes.last - created_approval = merge_request.approvals.last - created_reviewer = merge_request.merge_request_reviewers.last - system_note = merge_request.notes.where(system: true).last - note = merge_request.notes.where(system: false).last + expect(user_references).to match_array([ + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], + ['Note', note.id, 'author_id', source_user.id] + ]) + end - expect(user_references).to match_array([ - ['Approval', created_approval.id, 'user_id', source_user.id], - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], - ['Note', system_note.id, 'author_id', source_user.id], - ['Note', note.id, 'author_id', source_user.id] - ]) + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end end - end - context 'when the review is "COMMENTED"' do - let(:review) { create_review(type: 'COMMENTED') } + context 'when the review is "CHANGES_REQUESTED"' do + let(:review) { create_review(type: 'CHANGES_REQUESTED') } - it 'creates a note for the review' do - expect { subject.execute } - .to change { Note.count }.by(1) - .and not_change(Approval, :count) + it 'creates a note for the review' do + expect { subject.execute } + .to change { Note.count }.by(1) + .and not_change(Approval, :count) - last_note = merge_request.notes.last + last_note = merge_request.notes.last - expect(last_note.note).to eq("**Review:** Commented\n\nnote") - expect(last_note.author).to eq(author) - expect(last_note.created_at).to eq(submitted_at) - end + expect(last_note.note).to eq("**Review:** Changes requested\n\nnote") + expect(last_note.author).to eq(author) + expect(last_note.created_at).to eq(submitted_at) + end + + it 'pushes placeholder references for reviewer and reviewer note', if: push_placeholder_references do + subject.execute - it 'pushes placeholder references for reviewer and reviewer note' do - subject.execute + created_reviewer = merge_request.merge_request_reviewers.last + note = merge_request.notes.last - created_reviewer = merge_request.merge_request_reviewers.last - note = merge_request.notes.last + expect(user_references).to match_array([ + ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], + ['Note', note.id, 'author_id', source_user.id] + ]) + end - expect(user_references).to match_array([ - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], - ['Note', note.id, 'author_id', source_user.id] - ]) + context 'and the author is a real user', unless: push_placeholder_references do + include_examples 'imports without pushing placeholder references' + end end end + end - context 'when the review is "CHANGES_REQUESTED"' do - let(:review) { create_review(type: 'CHANGES_REQUESTED') } + context 'and importing into a group' do + let_it_be(:author) { source_user.mapped_user } - it 'creates a note for the review' do - expect { subject.execute } - .to change { Note.count }.by(1) - .and not_change(Approval, :count) + it_behaves_like 'importing a review', push_placeholder_references: true + end - last_note = merge_request.notes.last + context 'and importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + let_it_be(:author) { user_namespace.owner } # always map to personal namespace owner in this context - expect(last_note.note).to eq("**Review:** Changes requested\n\nnote") - expect(last_note.author).to eq(author) - expect(last_note.created_at).to eq(submitted_at) - end + before_all do + project.update!(namespace: user_namespace) + end - it 'pushes placeholder references for reviewer and reviewer note' do - subject.execute + it_behaves_like 'importing a review', push_placeholder_references: false - created_reviewer = merge_request.merge_request_reviewers.last - note = merge_request.notes.last + context 'and user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 999) } + let_it_be(:author) { source_user.mapped_user } - expect(user_references).to match_array([ - ['MergeRequestReviewer', created_reviewer.id, 'user_id', source_user.id], - ['Note', note.id, 'author_id', source_user.id] - ]) + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! end + + it_behaves_like 'importing a review', push_placeholder_references: true end end end diff --git a/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb index df5d2e34fb6df3..aa0a4b2d861dcf 100644 --- a/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb @@ -5,18 +5,16 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequests::ReviewRequestImporter, :clean_gitlab_redis_shared_state, feature_category: :importers do include Import::UserMappingHelper - let_it_be(:project) { create(:project, :with_import_url, :import_user_mapping_enabled, :in_group) } - let_it_be(:reviewer) { create(:user, username: 'alice') } - let_it_be(:source_user) do + let_it_be_with_reload(:project) do create( - :import_source_user, - source_user_identifier: 1, - source_hostname: project.safe_import_url, - import_type: Import::SOURCE_GITHUB, - namespace: project.root_ancestor + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled ) end + let_it_be(:reviewer) { create(:user, username: 'alice') } + let_it_be(:source_user) { generate_source_user(project, 1) } + let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:merge_request) { create(:merge_request) } let(:review_request) do @@ -61,9 +59,56 @@ ]) end + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + expect { 2.times { importer.execute } }.not_to raise_error + + expect(user_references).to be_empty + end + + it 'imports reviewers mapped to the personal namespace owner' do + expect { 2.times { importer.execute } }.not_to raise_error + + expect(merge_request.reviewers.size).to eq(1) + expect(merge_request.reviewers.last.id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + expect { 2.times { importer.execute } }.not_to raise_error + + created_reviewers = merge_request.merge_request_reviewers.sort_by(&:user_id) + + expect(user_references).to match_array([ + ['MergeRequestReviewer', created_reviewers.first.id, 'user_id', instance_of(Integer)] + ]) + end + + it 'imports reviewers mapped to the import user' do + expect { 2.times { importer.execute } }.not_to raise_error + + reviewers = merge_request.reviewers + expect(reviewers.size).to eq(1) + expect(reviewers).to all(be_import_user) + end + end + end + context 'when user contribution mapping is disabled' do before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| allow(finder).to receive(:find).with(1, reviewer.username).and_return(reviewer.id) allow(finder).to receive(:find).with(2, 'foo').and_return(nil) diff --git a/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb b/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb index 2a2b407901641e..1f77031043c343 100644 --- a/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/releases_importer_spec.rb @@ -3,21 +3,29 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::ReleasesImporter, feature_category: :importers do - let_it_be(:project) { create(:project, :with_import_url) } - let_it_be(:placeholder_user) { create(:user) } - let(:client) { double(:client) } - let(:importer) { described_class.new(project, client) } - let(:github_release_name) { 'Initial Release' } - let(:created_at) { Time.new(2017, 1, 1, 12, 00) } - let(:released_at) { Time.new(2017, 1, 1, 12, 00) } - let(:body) { 'This is my release' } - let(:author) do + include Import::UserMappingHelper + + let_it_be_with_reload(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + + let_it_be(:placeholder_user) { create(:user, :placeholder) } + let_it_be(:author) do { login: 'User A', id: 1 } end + let(:client) { double(:client) } + let(:github_release_name) { 'Initial Release' } + let(:created_at) { Time.new(2017, 1, 1, 12, 00) } + let(:released_at) { Time.new(2017, 1, 1, 12, 00) } + let(:body) { 'This is my release' } + let(:cached_references) { placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) } let(:github_release) do { id: 123456, @@ -30,70 +38,59 @@ } end - context 'when user mapping is enabled' do - let_it_be(:release) { create(:release, project: project) } - - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: placeholder_user.id, - source_user_identifier: 1, - source_username: 'User A', - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) - end + subject(:importer) { described_class.new(project, client) } - before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) - end + context 'when user mapping is enabled', :clean_gitlab_redis_shared_state do + let_it_be(:source_user) { generate_source_user(project, 1, placeholder_user: placeholder_user) } describe '#execute' do - it 'imports the releases in bulk' do # FAILING - allow(importer).to receive_messages(bulk_insert: [release.id], github_users: [author]) - release_hash = { - tag_name: '1.0', - description: 'This is my release', - created_at: created_at, - updated_at: created_at, - released_at: released_at, - author: author - } + before do + allow(importer).to receive(:each_release).and_return([github_release]) + end - expect(importer).to receive(:build_releases).and_return([[release_hash], []]) - expect(importer).to receive(:bulk_insert).with([release_hash]) + it 'imports the releases in bulk' do + expect(importer).to receive(:bulk_insert).and_call_original - expect(importer) - .to receive(:push_with_record) - .with( - an_instance_of(Release), - :author_id, - 1, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) + expect { importer.execute }.to change { Release.count }.by(1) + expect(project.releases.last).to have_attributes( + name: github_release_name, + tag: '1.0', + author_id: source_user.mapped_user_id, + description: body, + created_at: created_at, + updated_at: created_at, + released_at: released_at + ) + end + + it 'pushes placeholder references' do importer.execute + + expect(cached_references).to contain_exactly( + ['Release', project.releases.last.id, 'author_id', source_user.id] + ) end - it 'imports draft releases' do - release_double = { - name: 'Test', - body: 'This is description', - tag_name: '1.0', - description: 'This is my release', - created_at: created_at, - updated_at: created_at, - published_at: nil, - author: author - } + context 'when the release is a draft', :freeze_time do + let(:released_at) { nil } - expect(importer).to receive(:each_release).and_return([release_double]) + it 'imports the release' do + expect { importer.execute }.to change { Release.count }.by(1) - expect { importer.execute }.to change { Release.count }.by(1) + expect(project.releases.last).to have_attributes( + name: github_release_name, + tag: '1.0', + author_id: source_user.mapped_user_id, + description: body, + created_at: created_at, + updated_at: created_at, + released_at: Time.current + ) + end end it 'is idempotent' do - allow(importer).to receive(:each_release).and_return([github_release]) expect { importer.execute }.to change { Release.count }.by(1) expect { importer.execute }.not_to change { Release.count } # Idempotency check end @@ -109,6 +106,50 @@ expect(Release.last.description).to eq("You can ask `@knejad` by emailing xyz@gitlab.com") end end + + context 'when importing into a personal namespace' do + let_it_be(:user_namespace) { create(:namespace) } + + before_all do + project.update!(namespace: user_namespace) + end + + it 'does not push any references' do + importer.execute + + expect(cached_references).to be_empty + end + + it 'imports the release mapped to the personal namespace owner' do + importer.execute + + expect(project.releases.last.author_id).to eq(user_namespace.owner_id) + end + + context 'when user_mapping_to_personal_namespace_owner is disabled' do + let_it_be(:source_user) { generate_source_user(project, 1) } + + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'pushes placeholder references' do + importer.execute + + expect(cached_references).to contain_exactly( + ['Release', project.releases.last.id, 'author_id', source_user.id] + ) + end + + it 'imports the release mapped to the placeholder user' do + importer.execute + + expect(project.releases.last.author_id).to eq(source_user.mapped_user_id) + end + end + end end describe '#build_releases' do @@ -270,59 +311,63 @@ end context 'when user mapping is disabled' do + let_it_be(:user) { create(:user, username: author[:login]) } + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) - end + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! - def stub_email_for_github_username(user_name = 'User A', user_email = 'user@example.com') allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |instance| allow(instance).to receive(:email_for_github_username) - .with(user_name).and_return(user_email) + .with(user.username).and_return(user.email) end end describe '#execute' do before do - stub_email_for_github_username + allow(importer).to receive(:each_release).and_return([github_release]) end it 'imports the releases in bulk' do - release_hash = { - tag_name: '1.0', - description: 'This is my release', + expect(importer).to receive(:bulk_insert).and_call_original + + expect { importer.execute }.to change { Release.count }.by(1) + + expect(project.releases.last).to have_attributes( + name: github_release_name, + tag: '1.0', + author_id: user.id, + description: body, created_at: created_at, updated_at: created_at, released_at: released_at - } - - expect(importer).to receive(:build_releases).and_return([[release_hash], []]) - expect(importer).to receive(:bulk_insert).with([release_hash]) - - expect(importer) - .not_to receive(:push_with_record) + ) + end + it 'does not push placeholder references' do importer.execute + + expect(cached_references).to be_empty end - it 'imports draft releases' do - release_double = { - name: 'Test', - body: 'This is description', - tag_name: '1.0', - description: 'This is my release', - created_at: created_at, - updated_at: created_at, - published_at: nil, - author: author - } + context 'when the release is a draft', :freeze_time do + let(:released_at) { nil } - expect(importer).to receive(:each_release).and_return([release_double]) + it 'imports the release' do + expect { importer.execute }.to change { Release.count }.by(1) - expect { importer.execute }.to change { Release.count }.by(1) + expect(project.releases.last).to have_attributes( + name: github_release_name, + tag: '1.0', + author_id: user.id, + description: body, + created_at: created_at, + updated_at: created_at, + released_at: Time.current + ) + end end it 'is idempotent' do - allow(importer).to receive(:each_release).and_return([github_release]) expect { importer.execute }.to change { Release.count }.by(1) expect { importer.execute }.not_to change { Release.count } # Idempotency check end @@ -341,10 +386,6 @@ def stub_email_for_github_username(user_name = 'User A', user_email = 'user@exam end describe '#build_releases' do - before do - stub_email_for_github_username - end - it 'returns an Array containing release rows' do expect(importer).to receive(:each_release).and_return([github_release]) @@ -417,10 +458,6 @@ def stub_email_for_github_username(user_name = 'User A', user_email = 'user@exam let(:release_hash) { importer.build_attributes(github_release) } context 'the returned Hash' do - before do - stub_email_for_github_username - end - it 'returns the attributes of the release as a Hash' do expect(release_hash).to be_an_instance_of(Hash) end @@ -464,15 +501,12 @@ def stub_email_for_github_username(user_name = 'User A', user_email = 'user@exam context 'author_id attribute' do it 'returns the Gitlab user_id when Github release author is found' do - # Stub user email which matches a Gitlab user. - stub_email_for_github_username('User A', project.users.first.email) - # Disable cache read as the redis cache key can be set by other specs. # https://gitlab.com/gitlab-org/gitlab/-/blob/88bffda004e0aca9c4b9f2de86bdbcc0b49f2bc7/lib/gitlab/github_import/user_finder.rb#L75 # Above line can return different user when read from cache. allow(Gitlab::Cache::Import::Caching).to receive(:read).and_return(nil) - expect(release_hash[:author_id]).to eq(project.users.first.id) + expect(release_hash[:author_id]).to eq(user.id) end it 'returns ghost user when author is empty in Github release' do @@ -486,7 +520,10 @@ def stub_email_for_github_username(user_name = 'User A', user_email = 'user@exam before do # Stub user email which does not match a Gitlab user. - stub_email_for_github_username('octocat', 'octocat@example.com') + allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |instance| + allow(instance).to receive(:email_for_github_username) + .with('octocat').and_return('octocat@example.com') + end end it 'returns project creator as author' do diff --git a/spec/support/helpers/import/user_mapping_helper.rb b/spec/support/helpers/import/user_mapping_helper.rb index 44a088167926dd..a1460122275a40 100644 --- a/spec/support/helpers/import/user_mapping_helper.rb +++ b/spec/support/helpers/import/user_mapping_helper.rb @@ -33,14 +33,18 @@ def placeholder_user_references(import_type, import_uid, limit = 100) # # @param project [Project] # @param identifier [String, Integer] + # @param placholder_user [User] Sets a specific placeholder when given. Otherwise, use factory default. # @return [Import::SourceUser] - def generate_source_user(project, identifier) + def generate_source_user(project, identifier, placeholder_user: nil) + create_properties = placeholder_user.present? ? { placeholder_user: placeholder_user } : {} + create( :import_source_user, source_user_identifier: identifier, source_hostname: project.safe_import_url, import_type: project.import_type, - namespace: project.root_ancestor + namespace: project.root_ancestor, + **create_properties ) end end -- GitLab From 78836a999a1254022faa9d75237160f9d7eb4500 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Tue, 29 Jul 2025 12:37:34 -0400 Subject: [PATCH 3/6] Fixed remaining failing specs --- .../importer/events/commented_spec.rb | 19 ++-- .../importer/events/reopened_spec.rb | 102 +++++++++--------- .../importer/events/reviewed_spec.rb | 25 +++-- .../advance_stage_worker_spec.rb | 3 +- 4 files changed, 78 insertions(+), 71 deletions(-) diff --git a/spec/lib/gitlab/github_import/importer/events/commented_spec.rb b/spec/lib/gitlab/github_import/importer/events/commented_spec.rb index 8db3a59daf4c35..41958dd40bc93e 100644 --- a/spec/lib/gitlab/github_import/importer/events/commented_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/commented_spec.rb @@ -3,10 +3,18 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Commented, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:user) } + let_it_be(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + + let_it_be(:source_user) { generate_source_user(project, 1000) } let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:issuable) { create(:issue, project: project) } @@ -14,7 +22,7 @@ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.new( id: 1196850910, - actor: { id: user.id, login: user.username }, + actor: { id: source_user.source_user_identifier, login: source_user.source_username }, event: 'commented', created_at: '2022-07-27T14:41:11Z', updated_at: '2022-07-27T14:41:11Z', @@ -27,9 +35,6 @@ allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(issuable.id) end - allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) - end end shared_examples 'new note' do @@ -38,7 +43,7 @@ expect(issuable.notes.last).to have_attributes( note: 'This is my note', - author_id: user.id, + author_id: source_user.mapped_user_id, noteable_type: issuable.class.name.to_s, imported_from: 'github' ) diff --git a/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb b/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb index cbe2c8cd2d091f..ae9a8e06634def 100644 --- a/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb @@ -2,10 +2,19 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::Reopened, :aggregate_failures, feature_category: :importers do +RSpec.describe Gitlab::GithubImport::Importer::Events::Reopened, :clean_gitlab_redis_shared_state, :aggregate_failures, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository, :with_import_url) } + let_it_be(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + + let_it_be(:source_user) { generate_source_user(project, 1000) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -17,33 +26,19 @@ 'id' => 6501124486, 'node_id' => 'CE_lADOHK9fA85If7x0zwAAAAGDf0mG', 'url' => 'https://api.github.com/repos/elhowm/test-import/issues/events/6501124486', - 'actor' => { 'id' => user.id, 'login' => user.username }, + 'actor' => { id: 1000, login: 'github_author' }, 'event' => 'reopened', 'created_at' => created_at.iso8601, 'issue' => { 'number' => issuable.iid, pull_request: issuable.is_a?(MergeRequest) } ) end - let(:expected_event_attrs) do - { - project_id: project.id, - author_id: user.id, - target_id: issuable.id, - target_type: issuable.class.name, - action: 'reopened', - created_at: issue_event.created_at, - updated_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - end + let(:cached_references) { placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id) } before do allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(issuable.id) end - allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) - end end shared_examples 'new event' do @@ -85,47 +80,37 @@ shared_examples 'push placeholder references' do it 'pushes the references' do - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(Event), - :author_id, - issue_event[:actor].id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - - expect(subject) - .to receive(:push_with_record) - .with( - an_instance_of(ResourceStateEvent), - :user_id, - issue_event[:actor].id, - an_instance_of(Gitlab::Import::SourceUserMapper) - ) - importer.execute(issue_event) + + expect(cached_references).to match_array([ + ['Event', an_instance_of(Integer), 'author_id', source_user.id], + ['ResourceStateEvent', an_instance_of(Integer), 'user_id', source_user.id] + ]) end end shared_examples 'do not push placeholder references' do it 'does not push any reference' do - expect(subject) - .not_to receive(:push_with_record) - importer.execute(issue_event) + + expect(cached_references).to be_empty end end context 'when user mapping is enabled' do - let_it_be(:source_user) do - create( - :import_source_user, - placeholder_user_id: user.id, - source_user_identifier: user.id, - source_username: user.username, - source_hostname: project.safe_import_url, - namespace_id: project.root_ancestor.id - ) + let(:mapped_user_id) { source_user.mapped_user_id } + + let(:expected_event_attrs) do + { + project_id: project.id, + author_id: mapped_user_id, + target_id: issuable.id, + target_type: issuable.class.name, + action: 'reopened', + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys end before do @@ -135,7 +120,7 @@ context 'with Issue' do let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, issue_id: issuable.id, state: 'reopened', created_at: issue_event.created_at, @@ -151,7 +136,7 @@ let(:issuable) { create(:merge_request, source_project: project, target_project: project) } let(:expected_state_event_attrs) do { - user_id: user.id, + user_id: mapped_user_id, merge_request_id: issuable.id, state: 'reopened', created_at: issue_event.created_at, @@ -165,10 +150,23 @@ end context 'when user mapping is disabled' do + let(:expected_event_attrs) do + { + project_id: project.id, + author_id: user.id, + target_id: issuable.id, + target_type: issuable.class.name, + action: 'reopened', + created_at: issue_event.created_at, + updated_at: issue_event.created_at, + imported_from: 'github' + }.stringify_keys + end + before do - project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }).save! allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) + allow(finder).to receive(:find).with(1000, 'github_author').and_return(user.id) end end diff --git a/spec/lib/gitlab/github_import/importer/events/reviewed_spec.rb b/spec/lib/gitlab/github_import/importer/events/reviewed_spec.rb index 1b3ca686f95624..0a7b56bbc87788 100644 --- a/spec/lib/gitlab/github_import/importer/events/reviewed_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/reviewed_spec.rb @@ -3,10 +3,18 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Reviewed, feature_category: :importers do + include Import::UserMappingHelper + subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:user) } + let_it_be(:project) do + create( + :project, :in_group, :github_import, + :import_user_mapping_enabled, :user_mapping_to_personal_namespace_owner_enabled + ) + end + + let_it_be(:source_user) { generate_source_user(project, 1000) } let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } @@ -14,7 +22,7 @@ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( id: 1196850910, - actor: { id: user.id, login: user.username }, + actor: { id: source_user.source_user_identifier, login: source_user.source_username }, event: 'reviewed', submitted_at: '2022-07-27T14:41:11Z', body: 'This is my review', @@ -29,9 +37,6 @@ allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(merge_request.id) end - allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| - allow(finder).to receive(:find).with(user.id, user.username).and_return(user.id) - end end it 'creates a review note', :aggregate_failures do @@ -39,7 +44,7 @@ last_note = merge_request.notes.last expect(last_note.note).to include("This is my review") - expect(last_note.author).to eq(user) + expect(last_note.author).to eq(source_user.mapped_user) expect(last_note.created_at).to eq(issue_event.submitted_at) expect(last_note.imported_from).to eq('github') end @@ -54,18 +59,18 @@ it 'creates an approval for the Merge Request', :aggregate_failures do expect { importer.execute(issue_event) }.to change { Approval.count }.by(1).and change { Note.count }.by(2) - expect(merge_request.approved_by_users.reload).to include(user) + expect(merge_request.approved_by_users.reload).to include(source_user.mapped_user) expect(merge_request.approvals.last.created_at).to eq(issue_event.submitted_at) note = merge_request.notes.where(system: false).last expect(note.note).to include("This is my review") - expect(note.author).to eq(user) + expect(note.author).to eq(source_user.mapped_user) expect(note.created_at).to eq(issue_event.submitted_at) expect(note.imported_from).to eq('github') system_note = merge_request.notes.where(system: true).last expect(system_note.note).to eq('approved this merge request') - expect(system_note.author).to eq(user) + expect(system_note.author).to eq(source_user.mapped_user) expect(system_note.created_at).to eq(issue_event.submitted_at) expect(system_note.system_note_metadata.action).to eq('approved') end diff --git a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb index d52a0ee0a08089..5d89a0fe370807 100644 --- a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb +++ b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb @@ -57,8 +57,7 @@ it 'enqueues LoadPlaceholderReferencesWorker to save placeholder references' do expect(::Import::LoadPlaceholderReferencesWorker).to receive(:perform_async).with( ::Import::SOURCE_GITHUB, - project.import_state.id, - { 'current_user_id' => project.creator_id } + project.import_state.id ) worker.perform(project.id, { '123' => 2 }, 'finish') -- GitLab From 6a1da2852bbec05a2481a7c755c47c6fbb444f49 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 30 Jul 2025 18:09:26 -0400 Subject: [PATCH 4/6] Strong memoized method based on Duo feedback --- lib/gitlab/github_import/user_finder.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/github_import/user_finder.rb b/lib/gitlab/github_import/user_finder.rb index d56acad559ba61..ea5d160655544f 100644 --- a/lib/gitlab/github_import/user_finder.rb +++ b/lib/gitlab/github_import/user_finder.rb @@ -13,6 +13,7 @@ module GithubImport # the database when most queries are not going to return results anyway. class UserFinder include Gitlab::ExclusiveLeaseHelpers + include Gitlab::Utils::StrongMemoize attr_reader :project, :client @@ -383,12 +384,13 @@ def log(message, username: nil) end def source_user_mapper - @user_mapper ||= ::Gitlab::Import::SourceUserMapper.new( + ::Gitlab::Import::SourceUserMapper.new( namespace: project.root_ancestor, source_hostname: project.safe_import_url, import_type: ::Import::SOURCE_GITHUB ) end + strong_memoize_attr :source_user_mapper def user_mapping_enabled? project.import_data.user_mapping_enabled? -- GitLab From 3c7ae255cd2919931060c0b3d05585f877eaa396 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Tue, 5 Aug 2025 19:27:47 -0400 Subject: [PATCH 5/6] Skip collaborators import in personal namespace --- .../stage/import_collaborators_worker.rb | 16 ++++-- .../stage/import_collaborators_worker_spec.rb | 53 +++++++++++++++++-- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index b78d998db0171b..3664e8a29f83bb 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -13,8 +13,13 @@ class ImportCollaboratorsWorker # rubocop:disable Scalability/IdempotentWorker # client - An instance of Gitlab::GithubImport::Client. # project - An instance of Project. def import(client, project) - return skip_to_next_stage(project) if import_settings(project).disabled?(:collaborators_import) || - !has_push_access?(client, project.import_source) + return move_to_next_stage(project, {}) if import_settings(project).disabled?(:collaborators_import) || + map_contributions_to_personal_namespace_owner?(project) + + unless has_push_access?(client, project.import_source) + log_no_push_access(project) + return move_to_next_stage(project, {}) + end info(project.id, message: 'starting importer', importer: 'Importer::CollaboratorsImporter') @@ -29,7 +34,11 @@ def has_push_access?(client, repo) client.repository(repo).dig(:permissions, :push) end - def skip_to_next_stage(project) + def map_contributions_to_personal_namespace_owner?(project) + project.root_ancestor.user_namespace? && project.import_data.user_mapping_to_personal_namespace_owner_enabled? + end + + def log_no_push_access(project) Gitlab::GithubImport::Logger.warn( log_attributes( project.id, @@ -37,7 +46,6 @@ def skip_to_next_stage(project) importer: 'Importer::CollaboratorsImporter' ) ) - move_to_next_stage(project, {}) end def move_to_next_stage(project, waiters = {}) diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index 440dcfc226913b..4efb2b6e89ea11 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Stage::ImportCollaboratorsWorker, feature_category: :importers do - let_it_be(:project) { create(:project, import_type: ::Import::SOURCE_GITHUB.to_s) } + let_it_be(:project) { create(:project, :in_group, :github_import, :user_mapping_to_personal_namespace_owner_enabled) } let(:settings) { Gitlab::GithubImport::Settings.new(project) } let(:stage_enabled) { true } @@ -18,8 +18,8 @@ let(:push_rights_granted) { true } before do - Feature.disable(:github_user_mapping) - settings.write({ optional_stages: { collaborators_import: stage_enabled } }) + project.build_or_assign_import_data(data: { optional_stages: { collaborators_import: stage_enabled } }).save! + allow(client).to receive(:repository).with(project.import_source) .and_return({ permissions: { push: push_rights_granted } }) end @@ -45,9 +45,16 @@ context 'when user do not have push access for this repo' do let(:push_rights_granted) { false } - it 'skips stage' do + it 'skips stage and logs no push access' do expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) + expect(Gitlab::GithubImport::Logger).to receive(:warn) + .with(a_hash_including( + project_id: project.id, + importer: 'Importer::CollaboratorsImporter', + message: 'no push access rights to fetch collaborators' + )) + expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) .with(project.id, {}, 'issues_and_diff_notes') @@ -69,5 +76,43 @@ worker.import(client, project) end end + + context 'when importing into a personal namespace' do + let_it_be(:project) { create(:project, :github_import, :user_mapping_to_personal_namespace_owner_enabled) } + + it 'skips collaborators import and calls next stage' do + expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) + + expect(Gitlab::GithubImport::AdvanceStageWorker) + .to receive(:perform_async) + .with(project.id, {}, 'issues_and_diff_notes') + + worker.import(client, project) + end + + context 'and user_mapping_to_personal_namespace_owner is disabled' do + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! + end + + it 'imports all collaborators' do + waiter = Gitlab::JobWaiter.new(2, '123') + + expect(Gitlab::GithubImport::Importer::CollaboratorsImporter) + .to receive(:new) + .with(project, client) + .and_return(importer) + expect(importer).to receive(:execute).and_return(waiter) + + expect(Gitlab::GithubImport::AdvanceStageWorker) + .to receive(:perform_async) + .with(project.id, { '123' => 2 }, 'issues_and_diff_notes') + + worker.import(client, project) + end + end + end end end -- GitLab From dc3f10c0a9b94a944c6ad1b85c9b21938e04daab Mon Sep 17 00:00:00 2001 From: Sam Word Date: Thu, 7 Aug 2025 12:06:12 -0400 Subject: [PATCH 6/6] Moved shared logic in workers to import settings --- .../github_import/advance_stage_worker.rb | 7 +++---- .../stage/finish_import_worker.rb | 11 ++++------- .../stage/import_collaborators_worker.rb | 10 ++++------ lib/gitlab/github_import/settings.rb | 4 +++- spec/lib/gitlab/github_import/settings_spec.rb | 18 ++++++++++++++++-- .../github_import/advance_stage_worker_spec.rb | 8 ++++---- 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 721276397392c3..8bac052a72b5d9 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -47,10 +47,9 @@ def proceed_to_next_stage(import_state_jid, next_stage, project_id) project = Project.find_by_id(project_id) import_settings = Gitlab::GithubImport::Settings.new(project) - map_to_personal_namespace_owner = project.root_ancestor.user_namespace? && - import_settings.user_mapping_to_personal_namespace_owner_enabled? - - load_references(project) if import_settings.user_mapping_enabled? && !map_to_personal_namespace_owner + if import_settings.user_mapping_enabled? && !import_settings.map_to_personal_namespace_owner? + load_references(project) + end super end diff --git a/app/workers/gitlab/github_import/stage/finish_import_worker.rb b/app/workers/gitlab/github_import/stage/finish_import_worker.rb index 84f8b73e40f736..735637702b74ac 100644 --- a/app/workers/gitlab/github_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/github_import/stage/finish_import_worker.rb @@ -26,8 +26,10 @@ def import(_, project) attr_reader :project def reference_store_pending? - return false unless import_settings(project).user_mapping_enabled? - return false if map_to_personal_namespace_owner? + import_settings = import_settings(project) + + return false unless import_settings.user_mapping_enabled? + return false if import_settings.map_to_personal_namespace_owner? return false unless placeholder_reference_store.any? ::Import::LoadPlaceholderReferencesWorker.perform_async( @@ -62,11 +64,6 @@ def metrics def placeholder_reference_store @placeholder_reference_store ||= project.placeholder_reference_store end - - def map_to_personal_namespace_owner? - project.root_ancestor.user_namespace? && - import_settings(project).user_mapping_to_personal_namespace_owner_enabled? - end end end end diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index 3664e8a29f83bb..80460d60b7e380 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -13,8 +13,10 @@ class ImportCollaboratorsWorker # rubocop:disable Scalability/IdempotentWorker # client - An instance of Gitlab::GithubImport::Client. # project - An instance of Project. def import(client, project) - return move_to_next_stage(project, {}) if import_settings(project).disabled?(:collaborators_import) || - map_contributions_to_personal_namespace_owner?(project) + import_settings = import_settings(project) + + return move_to_next_stage(project, {}) if import_settings.disabled?(:collaborators_import) || + import_settings.map_to_personal_namespace_owner? unless has_push_access?(client, project.import_source) log_no_push_access(project) @@ -34,10 +36,6 @@ def has_push_access?(client, repo) client.repository(repo).dig(:permissions, :push) end - def map_contributions_to_personal_namespace_owner?(project) - project.root_ancestor.user_namespace? && project.import_data.user_mapping_to_personal_namespace_owner_enabled? - end - def log_no_push_access(project) Gitlab::GithubImport::Logger.warn( log_attributes( diff --git a/lib/gitlab/github_import/settings.rb b/lib/gitlab/github_import/settings.rb index f708b791ea6f60..72933f239017a4 100644 --- a/lib/gitlab/github_import/settings.rb +++ b/lib/gitlab/github_import/settings.rb @@ -76,7 +76,9 @@ def user_mapping_enabled? project.import_data&.data&.dig('user_contribution_mapping_enabled') || false end - def user_mapping_to_personal_namespace_owner_enabled? + def map_to_personal_namespace_owner? + return false unless project.root_ancestor.user_namespace? + project.import_data&.data&.dig('user_mapping_to_personal_namespace_owner_enabled') || false end diff --git a/spec/lib/gitlab/github_import/settings_spec.rb b/spec/lib/gitlab/github_import/settings_spec.rb index f721bc6f420701..e2c0bb95fab149 100644 --- a/spec/lib/gitlab/github_import/settings_spec.rb +++ b/spec/lib/gitlab/github_import/settings_spec.rb @@ -164,10 +164,12 @@ end end - describe '#user_mapping_to_personal_namespace_owner_enabled?' do + describe '#map_to_personal_namespace_owner?' do + let_it_be(:group) { create(:group) } + subject do settings.write(data_input) - settings.user_mapping_to_personal_namespace_owner_enabled? + settings.map_to_personal_namespace_owner? end shared_examples 'when :github_user_mapping is disabled' do |expected_enabled:| @@ -186,6 +188,14 @@ it { is_expected.to be(expected_enabled) } end + shared_examples 'disabled when project is imported into a group' do + before_all do + project.update!(namespace: group) + end + + it { is_expected.to be(false) } + end + before do project.build_or_assign_import_data(credentials: { user: 'token' }) end @@ -195,6 +205,7 @@ it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: true + it_behaves_like 'disabled when project is imported into a group' end context 'when the project is a Gitea import' do @@ -206,6 +217,7 @@ it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: true + it_behaves_like 'disabled when project is imported into a group' end context 'when the project does not have an import_type' do @@ -217,6 +229,7 @@ it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false + it_behaves_like 'disabled when project is imported into a group' end context 'when the project has an import_type without a user mapping flag' do @@ -228,6 +241,7 @@ it_behaves_like 'when :gitea_user_mapping is disabled', expected_enabled: false it_behaves_like 'when :github_user_mapping is disabled', expected_enabled: false + it_behaves_like 'disabled when project is imported into a group' end context 'when the feature flag is disabled' do diff --git a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb index 5d89a0fe370807..3cb39b9d79f97e 100644 --- a/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb +++ b/spec/workers/gitlab/github_import/advance_stage_worker_spec.rb @@ -48,10 +48,10 @@ end context 'and user_mapping_to_personal_namespace_owner is disabled' do - before do - allow_next_instance_of(Gitlab::GithubImport::Settings) do |settings| - allow(settings).to receive(:user_mapping_to_personal_namespace_owner_enabled?).and_return(false) - end + before_all do + project.build_or_assign_import_data( + data: { user_mapping_to_personal_namespace_owner_enabled: false } + ).save! end it 'enqueues LoadPlaceholderReferencesWorker to save placeholder references' do -- GitLab