From bd84e48794e9d3ffbe31ca7ec4e57a140ad5dffd Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 12 Nov 2024 06:25:57 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../javascripts/issuable/popover/index.js | 2 +- app/assets/javascripts/lib/graphql.js | 3 + .../organizations/users/components/app.vue | 116 ++++++--- .../users/components/users_view.vue | 43 +++- .../queries/organization_users.query.graphql | 5 +- ...nization_users_is_last_owner.query.graphql | 17 ++ .../groups/work_items_controller.rb | 1 - app/controllers/projects/issues_controller.rb | 1 - .../projects/work_items_controller.rb | 1 - .../development/comment_tooltips.yml | 8 - data/deprecations/17-0-workflow-templates.yml | 2 +- db/docs/vulnerability_statistics.yml | 2 +- doc/ci/yaml/index.md | 3 +- doc/update/deprecations.md | 36 +-- doc/user/markdown.md | 12 +- doc/user/organization/index.md | 18 +- doc/user/project/import/github.md | 3 + .../importer/events/base_importer.rb | 11 +- .../importer/events/changed_assignee.rb | 17 +- .../importer/events/changed_label.rb | 6 +- .../importer/events/changed_milestone.rb | 6 +- .../importer/events/changed_reviewer.rb | 17 +- .../github_import/importer/events/closed.rb | 12 +- .../importer/events/cross_referenced.rb | 8 +- .../github_import/importer/events/merged.rb | 11 +- .../github_import/importer/events/renamed.rb | 6 +- .../github_import/importer/events/reopened.rb | 12 +- lib/tasks/gitlab/keep_around.rake | 10 +- locale/gitlab.pot | 3 + qa/gdk/gdk.yml | 2 +- scripts/internal_events/server.rb | 6 +- spec/frontend/issuable/popover/index_spec.js | 8 - .../users/components/app_spec.js | 104 +++++++- .../users/components/users_view_spec.js | 52 ++++ .../frontend/organizations/users/mock_data.js | 3 +- .../importer/events/changed_assignee_spec.rb | 94 ++++++-- .../importer/events/changed_label_spec.rb | 228 ++++++++++++++---- .../importer/events/changed_milestone_spec.rb | 145 +++++++++-- .../importer/events/changed_reviewer_spec.rb | 100 +++++++- .../importer/events/closed_spec.rb | 141 +++++++++-- .../importer/events/cross_referenced_spec.rb | 89 ++++++- .../importer/events/merged_spec.rb | 186 +++++++++++--- .../importer/events/renamed_spec.rb | 79 +++++- .../importer/events/reopened_spec.rb | 138 +++++++++-- spec/scripts/internal_events/server_spec.rb | 42 ++-- spec/tasks/gitlab/keep_around_rake_spec.rb | 40 ++- 46 files changed, 1477 insertions(+), 372 deletions(-) create mode 100644 app/assets/javascripts/organizations/users/graphql/queries/organization_users_is_last_owner.query.graphql delete mode 100644 config/feature_flags/development/comment_tooltips.yml diff --git a/app/assets/javascripts/issuable/popover/index.js b/app/assets/javascripts/issuable/popover/index.js index 40ac1717fdf..8574299ce93 100644 --- a/app/assets/javascripts/issuable/popover/index.js +++ b/app/assets/javascripts/issuable/popover/index.js @@ -29,7 +29,7 @@ function isCommentPopover(target) { const targetUrl = new URL(target.href); const noteId = targetUrl.hash; - return window?.gon?.features?.commentTooltips && noteId && noteId.startsWith('#note_'); + return noteId && noteId.startsWith('#note_'); } export function handleCommentPopoverMount({ target, apolloProvider }) { diff --git a/app/assets/javascripts/lib/graphql.js b/app/assets/javascripts/lib/graphql.js index b0134b3278e..d3e2d418579 100644 --- a/app/assets/javascripts/lib/graphql.js +++ b/app/assets/javascripts/lib/graphql.js @@ -81,6 +81,9 @@ export const typePolicies = { ComplianceFrameworkConnection: { merge: true, }, + OrganizationUserConnection: { + merge: true, + }, }; export const stripWhitespaceFromQuery = (url, path) => { diff --git a/app/assets/javascripts/organizations/users/components/app.vue b/app/assets/javascripts/organizations/users/components/app.vue index 48b1a2c2599..92b0827c6e1 100644 --- a/app/assets/javascripts/organizations/users/components/app.vue +++ b/app/assets/javascripts/organizations/users/components/app.vue @@ -1,8 +1,12 @@ @@ -114,14 +132,21 @@ export default { :column-widths="$options.usersTable.columnWidths" >
diff --git a/app/assets/javascripts/organizations/users/graphql/queries/organization_users.query.graphql b/app/assets/javascripts/organizations/users/graphql/queries/organization_users.query.graphql index 37b1a25b909..979fc38842f 100644 --- a/app/assets/javascripts/organizations/users/graphql/queries/organization_users.query.graphql +++ b/app/assets/javascripts/organizations/users/graphql/queries/organization_users.query.graphql @@ -4,8 +4,8 @@ query getOrganizationUsers( $id: OrganizationsOrganizationID! $first: Int $last: Int - $before: String! - $after: String! + $before: String + $after: String ) { organization(id: $id) { id @@ -28,6 +28,7 @@ query getOrganizationUsers( accessLevel { stringValue } + isLastOwner } pageInfo { ...PageInfo diff --git a/app/assets/javascripts/organizations/users/graphql/queries/organization_users_is_last_owner.query.graphql b/app/assets/javascripts/organizations/users/graphql/queries/organization_users_is_last_owner.query.graphql new file mode 100644 index 00000000000..e040843714e --- /dev/null +++ b/app/assets/javascripts/organizations/users/graphql/queries/organization_users_is_last_owner.query.graphql @@ -0,0 +1,17 @@ +query getOrganizationUsersIsLastOwner( + $id: OrganizationsOrganizationID! + $first: Int + $last: Int + $before: String + $after: String +) { + organization(id: $id) { + id + organizationUsers(first: $first, last: $last, before: $before, after: $after) { + nodes { + id + isLastOwner + } + } + } +} diff --git a/app/controllers/groups/work_items_controller.rb b/app/controllers/groups/work_items_controller.rb index db4d6da0297..e0caa66a7f8 100644 --- a/app/controllers/groups/work_items_controller.rb +++ b/app/controllers/groups/work_items_controller.rb @@ -5,7 +5,6 @@ module Groups feature_category :team_planning before_action do - push_frontend_feature_flag(:comment_tooltips) push_frontend_feature_flag(:notifications_todos_buttons) push_force_frontend_feature_flag(:work_items, group&.work_items_feature_flag_enabled?) push_force_frontend_feature_flag(:work_items_beta, group&.work_items_beta_feature_flag_enabled?) diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 67d61544843..113ff93a710 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -49,7 +49,6 @@ class Projects::IssuesController < Projects::ApplicationController push_frontend_feature_flag(:service_desk_ticket) push_frontend_feature_flag(:issues_list_drawer, project) push_frontend_feature_flag(:notifications_todos_buttons, current_user) - push_frontend_feature_flag(:comment_tooltips, current_user) push_force_frontend_feature_flag(:glql_integration, project&.glql_integration_feature_flag_enabled?) end diff --git a/app/controllers/projects/work_items_controller.rb b/app/controllers/projects/work_items_controller.rb index 4f82e6a4cf5..7fcb1e5e5f0 100644 --- a/app/controllers/projects/work_items_controller.rb +++ b/app/controllers/projects/work_items_controller.rb @@ -14,7 +14,6 @@ class Projects::WorkItemsController < Projects::ApplicationController push_force_frontend_feature_flag(:work_items_alpha, project&.work_items_alpha_feature_flag_enabled?) push_force_frontend_feature_flag(:glql_integration, project&.glql_integration_feature_flag_enabled?) push_frontend_feature_flag(:namespace_level_work_items, project&.group) - push_frontend_feature_flag(:comment_tooltips) end feature_category :team_planning diff --git a/config/feature_flags/development/comment_tooltips.yml b/config/feature_flags/development/comment_tooltips.yml deleted file mode 100644 index eb530808622..00000000000 --- a/config/feature_flags/development/comment_tooltips.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: comment_tooltips -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/158020 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/476395 -milestone: '17.3' -type: development -group: group::project management -default_enabled: false diff --git a/data/deprecations/17-0-workflow-templates.yml b/data/deprecations/17-0-workflow-templates.yml index 1877c8e231b..0cd0a2b5fe4 100644 --- a/data/deprecations/17-0-workflow-templates.yml +++ b/data/deprecations/17-0-workflow-templates.yml @@ -18,7 +18,7 @@ - title: "`workflow:rules` templates" # The milestones for the deprecation announcement, and the removal. - removal_milestone: "18.0" + removal_milestone: "19.0" announcement_milestone: "17.0" # Change breaking_change to false if needed. breaking_change: true diff --git a/db/docs/vulnerability_statistics.yml b/db/docs/vulnerability_statistics.yml index f804a74ae14..9b28b2efed5 100644 --- a/db/docs/vulnerability_statistics.yml +++ b/db/docs/vulnerability_statistics.yml @@ -7,7 +7,7 @@ feature_categories: description: Stores pre-calculated vulnerability statistics for projects introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34289 milestone: '13.2' -gitlab_schema: gitlab_main_cell +gitlab_schema: gitlab_sec allow_cross_foreign_keys: - gitlab_main_clusterwide sharding_key: diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 789bfe6c98d..375a4693166 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -1488,7 +1488,8 @@ job: > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145206) in GitLab 16.11. -Use `artifacts:access` to determine who can access the job artifacts. +Use `artifacts:access` to determine who can access the job artifacts from the GitLab UI +or API. This option does not prevent you from forwarding artifacts to downstream pipelines. You cannot use [`artifacts:public`](#artifactspublic) and `artifacts:access` in the same job. diff --git a/doc/update/deprecations.md b/doc/update/deprecations.md index 61c5f8f97b6..440ec63cace 100644 --- a/doc/update/deprecations.md +++ b/doc/update/deprecations.md @@ -146,6 +146,24 @@ to [add the `ci:` section](https://docs.gitlab.com/ee/install/installation.html# Omnibus, the Helm chart, and Operator will handle this configuration automatically from GitLab 16.0 onwards. +
+ +
+ +### `workflow:rules` templates + +
+ +- Announced in GitLab 17.0 +- Removal in GitLab 19.0 ([breaking change](https://docs.gitlab.com/ee/update/terminology.html#breaking-change)) +- To discuss this change or learn more, see the [deprecation issue](https://gitlab.com/gitlab-org/gitlab/-/issues/456394). + +
+ +The [`workflow:rules`](https://docs.gitlab.com/ee/ci/yaml/workflow.html#workflowrules-templates) templates are deprecated and no longer recommended for use. Using these templates greatly limits the flexibility of your pipelines and makes it hard to use new `workflow` features. + +This is one small step towards moving away from CI/CD templates in preference of [CI/CD components](https://docs.gitlab.com/ee/ci/components/). You can search the [CI/CD Catalog](https://docs.gitlab.com/ee/ci/components/#cicd-catalog) for a replacement, or [add `workflow:rules`](https://docs.gitlab.com/ee/ci/yaml/workflow.html) to your pipeline explicitly. +
@@ -1133,24 +1151,6 @@ We encourage GitLab administrators to switch to the webhook delivery method for [Issue 393157](https://gitlab.com/gitlab-org/gitlab/-/issues/393157) tracks improving email ingestion in general. We hope this will simplify infrastructure setup and add several improvements to how you manage GitLab in the near future. - - -
- -### `workflow:rules` templates - -
- -- Announced in GitLab 17.0 -- Removal in GitLab 18.0 ([breaking change](https://docs.gitlab.com/ee/update/terminology.html#breaking-change)) -- To discuss this change or learn more, see the [deprecation issue](https://gitlab.com/gitlab-org/gitlab/-/issues/456394). - -
- -The [`workflow:rules`](https://docs.gitlab.com/ee/ci/yaml/workflow.html#workflowrules-templates) templates are deprecated and no longer recommended for use. Using these templates greatly limits the flexibility of your pipelines and makes it hard to use new `workflow` features. - -This is one small step towards moving away from CI/CD templates in preference of [CI/CD components](https://docs.gitlab.com/ee/ci/components/). You can search the [CI/CD Catalog](https://docs.gitlab.com/ee/ci/components/#cicd-catalog) for a replacement, or [add `workflow:rules`](https://docs.gitlab.com/ee/ci/yaml/workflow.html) to your pipeline explicitly. -
diff --git a/doc/user/markdown.md b/doc/user/markdown.md index 77687585b60..54f7c418af3 100644 --- a/doc/user/markdown.md +++ b/doc/user/markdown.md @@ -2055,17 +2055,9 @@ references refresh. ### Show comment preview when hovering on a link > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/29663) in GitLab 17.3 [with a flag](../administration/feature_flags.md) named `comment_tooltips`. Disabled by default. +> - Feature flag removed in GitLab 17.6 -FLAG: -The availability of this feature is controlled by a feature flag. -For more information, see the history. -This feature is available for testing, but not ready for production use. - -When this feature is enabled, hovering over a link to a comment shows the author and -part of the comment. - -When this feature is disabled, hovering over a link to a comment shows information about the item, -such as issue or epic. +Hovering over a link to a comment shows the author and first line of the comment. ### Embedding Observability dashboards diff --git a/doc/user/organization/index.md b/doc/user/organization/index.md index a5e54477bfb..e043c0bf4c8 100644 --- a/doc/user/organization/index.md +++ b/doc/user/organization/index.md @@ -108,10 +108,26 @@ To switch organizations: 1. Select the [**Visibility level**](../public_access.md) of the group. 1. Select **Create group**. -## Manage users +## View users + +1. On the left sidebar, select **Organizations** and find the organization you want to view. +1. Select **Manage > Users**. + +## Change a user's role + +Prerequisites: + +- You must have the Owner role for the organization. + +To change a user's role: 1. On the left sidebar, select **Organizations** and find the organization you want to manage. 1. Select **Manage > Users**. +1. Find the user whose role you want to update. +1. From the **Organization role** dropdown list, select a role. + +NOTE: +If you cannot select from the **Organization role** dropdown list, this user is the organization's only Owner. To change this user's role, first assign the Owner role to another user. ## Supported Markdown for Organization description diff --git a/doc/user/project/import/github.md b/doc/user/project/import/github.md index c8d3a295ecb..b2d1955d5f6 100644 --- a/doc/user/project/import/github.md +++ b/doc/user/project/import/github.md @@ -84,6 +84,9 @@ If the above requirements are not met, the importer can't map the particular use describing that non-existent users were added as reviewers and approvers. However, the actual reviewer status and approval are not applied to the merge request in GitLab. +[In GitLab 17.5 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/477553), GitLab adds backticks to username mentions in issues, merge requests, and notes. +These backticks prevent linking to an incorrect user with the same username on the GitLab instance. + ## Known issues - GitHub pull request comments (known as diff notes in GitLab) created before 2017 are imported in separate threads. diff --git a/lib/gitlab/github_import/importer/events/base_importer.rb b/lib/gitlab/github_import/importer/events/base_importer.rb index f8d601daf4d..534e6adcbb7 100644 --- a/lib/gitlab/github_import/importer/events/base_importer.rb +++ b/lib/gitlab/github_import/importer/events/base_importer.rb @@ -6,12 +6,15 @@ module Gitlab module Events # Base class for importing issue events during project import from GitHub class BaseImporter + include Gitlab::GithubImport::PushPlaceholderReferences + # project - An instance of `Project`. # client - An instance of `Gitlab::GithubImport::Client`. 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`. @@ -21,7 +24,7 @@ module Gitlab private - attr_reader :project, :user_finder, :client + attr_reader :project, :user_finder, :client, :mapper def author_id(issue_event, author_key: :actor) user_finder.author_id_for(issue_event, author_key: author_key).first @@ -45,11 +48,7 @@ module Gitlab end def imported_from - ::Import::HasImportSource::IMPORT_SOURCES[:github] - end - - def import_settings - @import_settings ||= Gitlab::GithubImport::Settings.new(project) + ::Import::SOURCE_GITHUB end end end diff --git a/lib/gitlab/github_import/importer/events/changed_assignee.rb b/lib/gitlab/github_import/importer/events/changed_assignee.rb index cf51ea05b8b..2dcd809d300 100644 --- a/lib/gitlab/github_import/importer/events/changed_assignee.rb +++ b/lib/gitlab/github_import/importer/events/changed_assignee.rb @@ -6,12 +6,15 @@ module Gitlab module Events class ChangedAssignee < BaseImporter def execute(issue_event) - assignee_id = author_id(issue_event, author_key: :assignee) author_id = author_id(issue_event, author_key: :actor) - note_body = parse_body(issue_event, assignee_id) + note_body = parse_body(issue_event) - create_note(issue_event, note_body, author_id) + 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) end private @@ -38,13 +41,11 @@ module Gitlab ) end - def parse_body(issue_event, assignee_id) - assignee = User.find(assignee_id).to_reference - + def parse_body(issue_event) if issue_event.event == 'unassigned' - "#{SystemNotes::IssuablesService.issuable_events[:unassigned]} #{assignee}" + "#{SystemNotes::IssuablesService.issuable_events[:unassigned]} `@#{issue_event[:assignee].login}`" else - "#{SystemNotes::IssuablesService.issuable_events[:assigned]} #{assignee}" + "#{SystemNotes::IssuablesService.issuable_events[:assigned]} `@#{issue_event[:assignee].login}`" end end end diff --git a/lib/gitlab/github_import/importer/events/changed_label.rb b/lib/gitlab/github_import/importer/events/changed_label.rb index 2d5f4dd4936..e4786238cb6 100644 --- a/lib/gitlab/github_import/importer/events/changed_label.rb +++ b/lib/gitlab/github_import/importer/events/changed_label.rb @@ -21,7 +21,11 @@ module Gitlab imported_from: imported_from }.merge(resource_event_belongs_to(issue_event)) - ResourceLabelEvent.create!(attrs) + created_event = ResourceLabelEvent.create!(attrs) + + return unless mapper.user_mapping_enabled? + + push_with_record(created_event, :user_id, issue_event[:actor].id, mapper.user_mapper) 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 0a029a21ba0..ca40385c2b2 100644 --- a/lib/gitlab/github_import/importer/events/changed_milestone.rb +++ b/lib/gitlab/github_import/importer/events/changed_milestone.rb @@ -30,7 +30,11 @@ module Gitlab imported_from: imported_from }.merge(resource_event_belongs_to(issue_event)) - ResourceMilestoneEvent.create!(attrs) + 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) 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 2f07a9241c4..1174a00ed96 100644 --- a/lib/gitlab/github_import/importer/events/changed_reviewer.rb +++ b/lib/gitlab/github_import/importer/events/changed_reviewer.rb @@ -6,10 +6,9 @@ module Gitlab module Events class ChangedReviewer < BaseImporter def execute(issue_event) - requested_reviewer_id = author_id(issue_event, author_key: :requested_reviewer) review_requester_id = author_id(issue_event, author_key: :review_requester) - note_body = parse_body(issue_event, requested_reviewer_id) + note_body = parse_body(issue_event) create_note(issue_event, note_body, review_requester_id) end @@ -17,7 +16,7 @@ module Gitlab private def create_note(issue_event, note_body, review_requester_id) - Note.create!( + created_note = Note.create!( importing: true, system: true, noteable_type: issuable_type(issue_event), @@ -36,17 +35,19 @@ module Gitlab updated_at: issue_event.created_at, 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) end - def parse_body(issue_event, requested_reviewer_id) - requested_reviewer = User.find(requested_reviewer_id).to_reference - + def parse_body(issue_event) if issue_event.event == 'review_request_removed' "#{SystemNotes::IssuablesService.issuable_events[:review_request_removed]} " \ - "#{requested_reviewer}" + "`@#{issue_event[:requested_reviewer].login}`" else "#{SystemNotes::IssuablesService.issuable_events[:review_requested]} " \ - "#{requested_reviewer}" + "`@#{issue_event[:requested_reviewer].login}`" end end end diff --git a/lib/gitlab/github_import/importer/events/closed.rb b/lib/gitlab/github_import/importer/events/closed.rb index 9cfe7a570f7..f99d8e58e23 100644 --- a/lib/gitlab/github_import/importer/events/closed.rb +++ b/lib/gitlab/github_import/importer/events/closed.rb @@ -13,7 +13,7 @@ module Gitlab private def create_event(issue_event) - Event.create!( + created_event = Event.create!( project_id: project.id, author_id: author_id(issue_event), action: 'closed', @@ -23,6 +23,10 @@ module Gitlab updated_at: issue_event.created_at, imported_from: imported_from ) + + return unless mapper.user_mapping_enabled? + + push_with_record(created_event, :author_id, issue_event[:actor].id, mapper.user_mapper) end def create_state_event(issue_event) @@ -37,7 +41,11 @@ module Gitlab imported_from: imported_from }.merge(resource_event_belongs_to(issue_event)) - ResourceStateEvent.create!(attrs) + 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) 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 a640e83ade2..f9393a75d78 100644 --- a/lib/gitlab/github_import/importer/events/cross_referenced.rb +++ b/lib/gitlab/github_import/importer/events/cross_referenced.rb @@ -33,7 +33,7 @@ module Gitlab end def create_note(issue_event, note_body, user_id) - Note.create!( + created_note = Note.create!( importing: true, system: true, noteable_type: issuable_type(issue_event), @@ -45,6 +45,12 @@ module Gitlab created_at: issue_event.created_at, 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) + + created_note end def mentioned_in_type(issue_event) diff --git a/lib/gitlab/github_import/importer/events/merged.rb b/lib/gitlab/github_import/importer/events/merged.rb index 9fb2c71b7a8..bc04ab4cd48 100644 --- a/lib/gitlab/github_import/importer/events/merged.rb +++ b/lib/gitlab/github_import/importer/events/merged.rb @@ -14,7 +14,7 @@ module Gitlab private def create_event(issue_event) - Event.create!( + event = Event.create!( project_id: project.id, author_id: author_id(issue_event), action: 'merged', @@ -24,6 +24,9 @@ module Gitlab 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) end def create_state_event(issue_event) @@ -38,7 +41,11 @@ module Gitlab imported_from: imported_from }.merge(resource_event_belongs_to(issue_event)) - ResourceStateEvent.create!(attrs) + 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) 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 6aa7b3cd15c..a1ebb1308e1 100644 --- a/lib/gitlab/github_import/importer/events/renamed.rb +++ b/lib/gitlab/github_import/importer/events/renamed.rb @@ -6,7 +6,11 @@ module Gitlab module Events class Renamed < BaseImporter def execute(issue_event) - Note.create!(note_params(issue_event)) + created_note = Note.create!(note_params(issue_event)) + + return unless mapper.user_mapping_enabled? + + push_with_record(created_note, :author_id, issue_event[:actor].id, mapper.user_mapper) end private diff --git a/lib/gitlab/github_import/importer/events/reopened.rb b/lib/gitlab/github_import/importer/events/reopened.rb index 903b4fd0ae8..5721b2dc5e5 100644 --- a/lib/gitlab/github_import/importer/events/reopened.rb +++ b/lib/gitlab/github_import/importer/events/reopened.rb @@ -13,7 +13,7 @@ module Gitlab private def create_event(issue_event) - Event.create!( + created_event = Event.create!( project_id: project.id, author_id: author_id(issue_event), action: 'reopened', @@ -23,6 +23,10 @@ module Gitlab updated_at: issue_event.created_at, imported_from: imported_from ) + + return unless mapper.user_mapping_enabled? + + push_with_record(created_event, :author_id, issue_event[:actor].id, mapper.user_mapper) end def create_state_event(issue_event) @@ -33,7 +37,11 @@ module Gitlab imported_from: imported_from }.merge(resource_event_belongs_to(issue_event)) - ResourceStateEvent.create!(attrs) + 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) end end end diff --git a/lib/tasks/gitlab/keep_around.rake b/lib/tasks/gitlab/keep_around.rake index c0f067e812a..419203f7588 100644 --- a/lib/tasks/gitlab/keep_around.rake +++ b/lib/tasks/gitlab/keep_around.rake @@ -47,7 +47,7 @@ namespace :gitlab do def add_merge_request_shas(project, csv) logger.info "Checking merge request shas..." - merge_requests = MergeRequest.from_and_to_forks(project).select(:id, :merge_commit_sha) + merge_requests = MergeRequest.of_projects(project).select(:id, :merge_commit_sha) merge_requests.find_each do |merge_request| add_match(csv, merge_request.merge_commit_sha) end @@ -55,12 +55,12 @@ namespace :gitlab do def add_merge_request_diff_shas(project, csv) logger.info "Checking merge request diff shas..." - merge_requests = MergeRequest.from_and_to_forks(project) merge_request_diffs = MergeRequestDiff - .joins(:merge_request).merge(merge_requests) - .select(:id, :start_commit_sha, :head_commit_sha) + .joins(:merge_request).merge(MergeRequest.of_projects([project, project.forked_from_project].compact)) + .select(:id, :start_commit_sha, :head_commit_sha, :diff_type) + merge_request_diffs.find_each do |diff| + next if diff.merge_head? - merge_request_diffs.where.not(diff_type: :merge_head).find_each do |diff| add_match(csv, diff.start_commit_sha) add_match(csv, diff.head_commit_sha) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6d60f3fcb14..42b14a7ab30 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38576,6 +38576,9 @@ msgstr "" msgid "Organization|Organizations" msgstr "" +msgid "Organization|Organizations must have at least one owner." +msgstr "" + msgid "Organization|Organizations must have at least one owner. To delete the user, first assign a new owner to %{lastLink}." msgid_plural "Organization|Organizations must have at least one owner. To delete the user, first assign a new owner to %{links} %{boldStart}and%{boldEnd} %{lastLink}." msgstr[0] "" diff --git a/qa/gdk/gdk.yml b/qa/gdk/gdk.yml index dae965cead7..860d0b7ec78 100644 --- a/qa/gdk/gdk.yml +++ b/qa/gdk/gdk.yml @@ -32,4 +32,4 @@ tracer: enabled: false # https://gitlab.com/gitlab-org/gitlab/-/issues/471172 gitlab_http_router: - enabled: true + enabled: false diff --git a/scripts/internal_events/server.rb b/scripts/internal_events/server.rb index bc3bd3f2e5b..0f51c8ef6f5 100644 --- a/scripts/internal_events/server.rb +++ b/scripts/internal_events/server.rb @@ -60,9 +60,9 @@ class Server se_category: query['se_ca'], se_action: query['se_ac'], collector_tstamp: query['dtm'], - label: query['se_la'], - property: query['se_pr'], - value: query['se_va'], + se_label: query['se_la'], + se_property: query['se_pr'], + se_value: query['se_va'], contexts: (JSON.parse(Base64.decode64(query['cx'])) if query['cx']) }, rawEvent: { parameters: query } diff --git a/spec/frontend/issuable/popover/index_spec.js b/spec/frontend/issuable/popover/index_spec.js index 91421912f6a..ac364ce4642 100644 --- a/spec/frontend/issuable/popover/index_spec.js +++ b/spec/frontend/issuable/popover/index_spec.js @@ -99,14 +99,6 @@ describe('initIssuablePopovers', () => { }); describe('comment tooltips', () => { - beforeEach(() => { - window.gon = { - features: { - commentTooltips: true, - }, - }; - }); - it('calls popover mount function for comments', async () => { jest.spyOn(popover, 'handleIssuablePopoverMount').mockImplementation(jest.fn()); diff --git a/spec/frontend/organizations/users/components/app_spec.js b/spec/frontend/organizations/users/components/app_spec.js index 1249a1e97c4..c7348186646 100644 --- a/spec/frontend/organizations/users/components/app_spec.js +++ b/spec/frontend/organizations/users/components/app_spec.js @@ -4,15 +4,21 @@ import organizationUsersResponse from 'test_fixtures/graphql/organizations/organ import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; +import { useMockLocationHelper } from 'helpers/mock_window_location_helper'; import { createAlert } from '~/alert'; import organizationUsersQuery from '~/organizations/users/graphql/queries/organization_users.query.graphql'; +import organizationUsersIsLastOwnerQuery from '~/organizations/users/graphql/queries/organization_users_is_last_owner.query.graphql'; import OrganizationsUsersApp from '~/organizations/users/components/app.vue'; import OrganizationsUsersView from '~/organizations/users/components/users_view.vue'; import { ORGANIZATION_USERS_PER_PAGE } from '~/organizations/users/constants'; +import * as Sentry from '~/sentry/sentry_browser_wrapper'; import { pageInfoMultiplePages, pageInfoEmpty } from 'jest/organizations/mock_data'; import { MOCK_USERS_FORMATTED } from '../mock_data'; jest.mock('~/alert'); +jest.mock('~/sentry/sentry_browser_wrapper'); + +useMockLocationHelper(); Vue.use(VueApollo); @@ -53,12 +59,32 @@ const successfulResponseHandlerNoResults = jest.fn().mockResolvedValue({ const errorResponseHandler = jest.fn().mockRejectedValue(mockError); const loadingResponseHandler = jest.fn().mockReturnValue(new Promise(() => {})); +const successfulIsLastOwnerResponseHandler = jest.fn().mockResolvedValue({ + data: { + organization: { + ...organizationUsersResponse.data.organization, + organizationUsers: { + // eslint-disable-next-line no-underscore-dangle + __typename: organizationUsersResponse.data.organization.organizationUsers.__typename, + nodes: nodes.map((node) => ({ id: node.id, isLastOwner: node.isLastOwner })), + }, + }, + }, +}); + describe('OrganizationsUsersApp', () => { let wrapper; let mockApollo; + let apolloCache; - const createComponent = ({ handler = successfulResponseHandler } = {}) => { - mockApollo = createMockApollo([[organizationUsersQuery, handler]]); + const createComponent = ({ + handler = successfulResponseHandler, + isLastOwnerHandler = successfulIsLastOwnerResponseHandler, + } = {}) => { + mockApollo = createMockApollo([ + [organizationUsersQuery, handler], + [organizationUsersIsLastOwnerQuery, isLastOwnerHandler], + ]); wrapper = shallowMountExtended(OrganizationsUsersApp, { apolloProvider: mockApollo, @@ -66,10 +92,16 @@ describe('OrganizationsUsersApp', () => { organizationGid, }, }); + + apolloCache = mockApollo.defaultClient.cache; + jest.spyOn(apolloCache, 'identify').mockImplementation((object) => object.id); + jest.spyOn(apolloCache, 'evict'); + jest.spyOn(apolloCache, 'gc'); }; afterEach(() => { mockApollo = null; + apolloCache = null; }); const findOrganizationUsersView = () => wrapper.findComponent(OrganizationsUsersView); @@ -138,9 +170,77 @@ describe('OrganizationsUsersApp', () => { id: organizationGid, before: pageInfoMultiplePages.startCursor, after: '', + first: null, + last: ORGANIZATION_USERS_PER_PAGE, + }); + }); + }); + + describe('when role-change event is emitted', () => { + it('calls isLastOwner query with correct variables', async () => { + createComponent(); + await waitForPromises(); + findOrganizationUsersView().vm.$emit('role-change'); + await waitForPromises(); + + expect(successfulIsLastOwnerResponseHandler).toHaveBeenCalledWith({ + id: organizationGid, + before: '', + after: '', first: ORGANIZATION_USERS_PER_PAGE, last: null, }); }); + + describe('when isLastOwner query is successful', () => { + beforeEach(async () => { + createComponent({ + handler: successfulResponseHandlerMultiplePages, + }); + await waitForPromises(); + findOrganizationUsersView().vm.$emit('next'); + await waitForPromises(); + findOrganizationUsersView().vm.$emit('role-change'); + await waitForPromises(); + }); + + it('evicts cache for all pages except current one', () => { + expect(apolloCache.evict).toHaveBeenCalledWith({ + id: organizationGid, + fieldName: 'organizationUsers', + args: { + before: '', + after: '', + first: ORGANIZATION_USERS_PER_PAGE, + last: null, + }, + }); + }); + + it('calls gc on apollo cache', () => { + expect(apolloCache.gc).toHaveBeenCalled(); + }); + }); + + describe('when isLastOwner query is not successful', () => { + const error = new Error(); + + beforeEach(async () => { + createComponent({ + isLastOwnerHandler: jest.fn().mockRejectedValueOnce(error), + }); + await waitForPromises(); + findOrganizationUsersView().vm.$emit('role-change'); + await waitForPromises(); + }); + + it('reloads the page to get fresh user list', () => { + expect(window.location.reload).toHaveBeenCalled(); + }); + + it('reports error to sentry', () => { + expect(Sentry.captureException).toHaveBeenCalledWith(error); + }); + }); }); }); diff --git a/spec/frontend/organizations/users/components/users_view_spec.js b/spec/frontend/organizations/users/components/users_view_spec.js index a68fba100ad..bc2635c38d5 100644 --- a/spec/frontend/organizations/users/components/users_view_spec.js +++ b/spec/frontend/organizations/users/components/users_view_spec.js @@ -10,6 +10,7 @@ import organizationUserUpdateMutation from '~/organizations/users/graphql/mutati import createMockApollo from 'helpers/mock_apollo_helper'; import { createAlert } from '~/alert'; import waitForPromises from 'helpers/wait_for_promises'; +import { createMockDirective, getBinding } from 'helpers/vue_mock_directive'; import { pageInfoMultiplePages } from 'jest/organizations/mock_data'; import { MOCK_PATHS, MOCK_USERS_FORMATTED } from '../mock_data'; @@ -43,6 +44,9 @@ describe('UsersView', () => { show: mockToastShow, }, }, + directives: { + GlTooltip: createMockDirective('gl-tooltip'), + }, }); }; @@ -113,6 +117,45 @@ describe('UsersView', () => { }, ], selected: MOCK_USERS_FORMATTED[0].accessLevel.stringValue, + disabled: false, + }); + }); + + it('does not render tooltip', () => { + createComponent(); + + const tooltipContainer = findListbox().element.parentNode; + const tooltip = getBinding(tooltipContainer, 'gl-tooltip'); + + expect(tooltip.value.disabled).toBe(true); + expect(tooltipContainer.getAttribute('tabindex')).toBe(null); + }); + + describe('when user is last owner of organization', () => { + const [firstUser] = MOCK_USERS_FORMATTED; + + beforeEach(() => { + createComponent({ + propsData: { + loading: false, + users: [{ ...firstUser, isLastOwner: true }], + }, + }); + }); + + it('renders listbox as disabled', () => { + expect(findListbox().props('disabled')).toBe(true); + }); + + it('renders tooltip and makes element focusable', () => { + const tooltipContainer = findListbox().element.parentNode; + const tooltip = getBinding(tooltipContainer, 'gl-tooltip'); + + expect(tooltip.value).toEqual({ + title: 'Organizations must have at least one owner.', + disabled: false, + }); + expect(tooltipContainer.getAttribute('tabindex')).toBe('0'); }); }); @@ -152,6 +195,15 @@ describe('UsersView', () => { expect(mockToastShow).toHaveBeenCalledWith('Organization role was updated successfully.'); }); + it('emits role-change event when GraphQL mutation is successful', async () => { + createComponent(); + listboxSelectOwner(); + + await waitForPromises(); + + expect(wrapper.emitted('role-change')).toEqual([[]]); + }); + it('calls createAlert when GraphQL mutation has validation error', async () => { const errorResponseHandler = jest .fn() diff --git a/spec/frontend/organizations/users/mock_data.js b/spec/frontend/organizations/users/mock_data.js index dc85a5678bc..8b20908f2c7 100644 --- a/spec/frontend/organizations/users/mock_data.js +++ b/spec/frontend/organizations/users/mock_data.js @@ -14,7 +14,7 @@ export const MOCK_PATHS = { }; export const MOCK_USERS_FORMATTED = users.map( - ({ id, badges, user, accessLevel, userPermissions }) => { + ({ id, badges, user, accessLevel, userPermissions, isLastOwner }) => { return { ...user, gid: id, @@ -23,6 +23,7 @@ export const MOCK_USERS_FORMATTED = users.map( accessLevel, userPermissions, email: user.publicEmail, + isLastOwner, }; }, ); 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 136eae1aa78..e959b141710 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 @@ -5,9 +5,8 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedAssignee, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:author) { create(:user) } - let_it_be(:assignee) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:issuable) { create(:issue, project: project) } @@ -15,11 +14,11 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedAssignee, feature_ let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => author.id, 'login' => author.username }, + 'actor' => { 'id' => 1000, 'login' => 'github_author' }, 'event' => event_type, 'commit_id' => nil, 'created_at' => '2022-04-26 18:30:53 UTC', - 'assignee' => { 'id' => assignee.id, 'login' => assignee.username }, + 'assignee' => { 'id' => 2000, 'login' => 'github_assignee' }, 'issue' => { 'number' => issuable.iid, pull_request: issuable.is_a?(MergeRequest) } ) end @@ -70,38 +69,103 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedAssignee, feature_ shared_examples 'process assigned & unassigned events' do context 'when importing an assigned event' do let(:event_type) { 'assigned' } - let(:expected_note_attrs) { note_attrs.merge(note: "assigned to @#{assignee.username}") } + let(:expected_note_attrs) { note_attrs.merge(note: "assigned to `@github_assignee`") } it_behaves_like 'create expected notes' end context 'when importing an unassigned event' do let(:event_type) { 'unassigned' } - let(:expected_note_attrs) { note_attrs.merge(note: "unassigned @#{assignee.username}") } + let(:expected_note_attrs) { note_attrs.merge(note: "unassigned `@github_assignee`") } it_behaves_like 'create expected notes' end end + shared_examples 'push a placeholder reference' do + 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) + end + end + + shared_examples 'do not push placeholder reference' do + let(:event_type) { 'assigned' } + + it 'does not push any reference' do + expect(subject) + .not_to receive(:push_with_record) + + importer.execute(issue_event) + end + end + describe '#execute' do 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(author.id, author.username).and_return(author.id) - allow(finder).to receive(:find).with(assignee.id, assignee.username).and_return(assignee.id) + allow(finder).to receive(:author_id_for).with(issue_event, author_key: :actor).and_return([author.id, true]) end end - context 'with Issue' do - it_behaves_like 'process assigned & unassigned events' + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 - context 'with MergeRequest' do - let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + 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(1000, 'github_author').and_return(author.id) + end + end - it_behaves_like 'process assigned & unassigned events' + 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 end end 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 f3f6f27613c..5090a2e0748 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 @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -44,84 +44,216 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel, feature_cat end end + 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) + 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 + 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 - 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) + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 a labeled event' do + 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' - 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) + 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 - 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') } + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end - it_behaves_like 'new event' - 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(: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 - 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') } + context 'when user mapping is disabled' do + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: false }) - it_behaves_like 'new event' + 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 - context 'when importing event without associated label' do - before do - allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + 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 - 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') } + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end - it_behaves_like 'new event' + 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 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 7e2863818e4..c69e70526f2 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 @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedMilestone, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -57,48 +57,141 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedMilestone, feature end end + shared_examples 'push placeholder reference' do + 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) + end + end + + shared_examples 'do not push placeholder reference' do + let(:milestone_title) { milestone.title } + + it 'does not push any reference' do + expect(subject) + .not_to receive(:push_with_record) + + importer.execute(issue_event) + end + end + describe '#execute' do before do allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(milestone.id) 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 + + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 - 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' + 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) + end end - context 'when importing demilestoned event' do - let(:event_type) { 'demilestoned' } - let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + 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' - end - end + it_behaves_like 'new 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) } + context 'when importing demilestoned event' do + let(:event_type) { 'demilestoned' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } - 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 'new event' + it_behaves_like 'do not push placeholder reference' + end 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') } + context 'with MergeRequest' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project) } - it_behaves_like 'new event' + 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 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 22406961c8e..392d49d37e4 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,20 +2,20 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedReviewer do +RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedReviewer, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(: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(:issuable) { create(:merge_request, source_project: project, target_project: project) } let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( 'id' => 6501124486, - 'actor' => { 'id' => 4, 'login' => 'alice' }, + 'actor' => { 'id' => review_requester.id, 'login' => review_requester.username }, 'event' => event_type, 'commit_id' => nil, 'created_at' => '2022-04-26 18:30:53 UTC', @@ -71,32 +71,106 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedReviewer do 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) { note_attrs.merge(note: "requested review from `@#{requested_reviewer.username}`") } it_behaves_like 'create expected notes' end context 'when importing a review_request_removed event' do let(:event_type) { 'review_request_removed' } - let(:expected_note_attrs) { note_attrs.merge(note: "removed review request for @#{requested_reviewer.username}") } + let(:expected_note_attrs) do + note_attrs.merge(note: "removed review request for `@#{requested_reviewer.username}`") + end it_behaves_like 'create expected notes' end end + 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) + ) + + importer.execute(issue_event) + 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) + + importer.execute(issue_event) + end + end + describe '#execute' do 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(requested_reviewer.id, requested_reviewer.username) - .and_return(requested_reviewer.id) - allow(finder).to receive(:find).with(review_requester.id, review_requester.username) - .and_return(review_requester.id) - end end - it_behaves_like 'process review_requested & review_request_removed MR events' + 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.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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + let_it_be(:merge_request_reviewer) do + create( + :merge_request_reviewer, + user_id: requested_reviewer.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' + 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(requested_reviewer.id, requested_reviewer.username) + .and_return(requested_reviewer.id) + allow(finder).to receive(:find).with(review_requester.id, review_requester.username) + .and_return(review_requester.id) + end + end + + it_behaves_like 'process review_requested & review_request_removed MR events' + it_behaves_like 'do not push placeholder reference' + end 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 9d8d05bea0d..820cf3e279e 100644 --- a/spec/lib/gitlab/github_import/importer/events/closed_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/closed_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Closed, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -42,9 +42,6 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Closed, feature_category: 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 @@ -74,32 +71,124 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Closed, feature_category: end end - context 'with Issue' do - let(:expected_state_event_attrs) do - { - user_id: user.id, - issue_id: issuable.id, - state: 'closed', - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - 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) + ) - it_behaves_like 'new event' + expect(subject) + .to receive(:push_with_record) + .with( + an_instance_of(ResourceStateEvent), + :user_id, + user.id, + an_instance_of(Gitlab::Import::SourceUserMapper) + ) + + importer.execute(issue_event) + end end - context 'with MergeRequest' do - let(:issuable) { create(:merge_request, source_project: project, target_project: project) } - let(:expected_state_event_attrs) do - { - user_id: user.id, - merge_request_id: issuable.id, - state: 'closed', - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys + shared_examples 'do not push placeholder references' do + it 'does not push any references' 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.import_url, + namespace_id: project.root_ancestor.id + ) end - it_behaves_like 'new event' + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + end + + context 'with Issue' do + let(:expected_state_event_attrs) do + { + user_id: 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: 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 + + context 'when user mapping is disabled' do + before do + allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + allow(finder).to receive(:find).with(user.id, user.username).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, + 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: 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 end end 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 262b0673e8a..a1f3549f5c9 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 @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::CrossReferenced, :clean_gitlab_redis_shared_state, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -56,9 +56,6 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::CrossReferenced, :clean_g allow(finder).to receive(:database_id).and_return(referenced_in.iid) 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 it 'creates expected note' do @@ -92,9 +89,6 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::CrossReferenced, :clean_g allow(finder).to receive(:database_id).and_return(referenced_in.iid) 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 it 'creates expected note' do @@ -121,13 +115,84 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::CrossReferenced, :clean_g end end - context 'with Issue' do - it_behaves_like 'import cross-referenced event' + shared_examples 'push a placeholder reference' do + before do + allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| + allow(finder).to receive(:database_id).and_return(referenced_in.iid) + allow(finder).to receive(:database_id).and_return(issuable.id) + end + 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) + end end - context 'with MergeRequest' do - let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + shared_examples 'do not push placeholder reference' do + it 'does not push any reference' do + expect(subject) + .not_to receive(:push_with_record) - it_behaves_like 'import cross-referenced event' + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 + + 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) + end + 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 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 68c64d8fba6..7a7a0fb3267 100644 --- a/spec/lib/gitlab/github_import/importer/events/merged_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/merged_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Merged, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -29,56 +29,164 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Merged, feature_category: 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 + + 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) + ) + + importer.execute(issue_event) end end - it 'creates expected event and state event' do - importer.execute(issue_event) + shared_examples 'do not push placeholder references' do + it 'does not push references' do + expect(subject) + .not_to receive(:push_with_record) - 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.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 - ) + importer.execute(issue_event) + end end - it 'creates a merged by note' do - expect { importer.execute(issue_event) }.to change { Note.count }.by(1) + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end - last_note = merge_request.notes.last - expect(last_note.created_at).to eq(issue_event.created_at) - expect(last_note.author).to eq(project.owner) - expect(last_note.note).to eq("*Merged by: #{user.username} at #{issue_event.created_at}*") - end - - context 'when commit ID is present' do - let!(:commit) { create(:commit, project: project) } - let(:commit_id) { commit.id } + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 + + 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: #{user.username} 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 '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) + end + 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).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 + + 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: #{user.username} 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 references' 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 a3f2b19c976..2dcd30bc209 100644 --- a/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/renamed_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Renamed, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:issuable) { create(:issue, project: project) } @@ -51,9 +51,6 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Renamed, feature_category 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 'import renamed event' do @@ -78,14 +75,78 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Renamed, feature_category end end - context 'with Issue' do - it_behaves_like 'import renamed event' + 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) + end end - context 'with MergeRequest' do - let(:issuable) { create(:merge_request, source_project: project, target_project: project) } + shared_examples 'do not push placeholder reference' do + it 'does not push any reference' do + expect(subject) + .not_to receive(:push_with_record) - it_behaves_like 'import renamed event' + 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.import_url, + namespace_id: project.root_ancestor.id + ) + end + + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + 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 + + 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) + end + 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 end end end 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 8370c903bc2..a887699622f 100644 --- a/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/reopened_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::Events::Reopened, :aggregate_failures, feature_category: :importers do subject(:importer) { described_class.new(project, client) } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository, :with_import_url) } let_it_be(:user) { create(:user) } let(:client) { instance_double('Gitlab::GithubImport::Client') } @@ -59,32 +59,124 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::Reopened, :aggregate_fail end end - context 'with Issue' do - let(:expected_state_event_attrs) do - { - user_id: user.id, - issue_id: issuable.id, - state: 'reopened', - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys - 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, + issue_event[:actor].id, + an_instance_of(Gitlab::Import::SourceUserMapper) + ) - it_behaves_like 'new event' + 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) + end end - context 'with MergeRequest' do - let(:issuable) { create(:merge_request, source_project: project, target_project: project) } - let(:expected_state_event_attrs) do - { - user_id: user.id, - merge_request_id: issuable.id, - state: 'reopened', - created_at: issue_event.created_at, - imported_from: 'github' - }.stringify_keys + 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) + 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.import_url, + namespace_id: project.root_ancestor.id + ) end - it_behaves_like 'new event' + before do + project.build_or_assign_import_data(data: { user_contribution_mapping_enabled: true }) + end + + context 'with Issue' do + let(:expected_state_event_attrs) do + { + user_id: user.id, + issue_id: issuable.id, + state: 'reopened', + 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: user.id, + merge_request_id: issuable.id, + state: 'reopened', + 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 + + 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) + end + end + + context 'with Issue' do + let(:expected_state_event_attrs) do + { + user_id: user.id, + issue_id: issuable.id, + state: 'reopened', + 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: user.id, + merge_request_id: issuable.id, + state: 'reopened', + 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 end end diff --git a/spec/scripts/internal_events/server_spec.rb b/spec/scripts/internal_events/server_spec.rb index 8b0761dc859..993e4a1e5d7 100644 --- a/spec/scripts/internal_events/server_spec.rb +++ b/spec/scripts/internal_events/server_spec.rb @@ -37,9 +37,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'InternalEventTracking', se_action: 'g_project_management_issue_created', collector_tstamp: '1727475117074', - label: nil, - property: nil, - value: nil, + se_label: nil, + se_property: nil, + se_value: nil, contexts: Gitlab::Json.parse(context) }, rawEvent: { parameters: Rack::Utils.parse_query(query_params) } @@ -62,9 +62,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'category', se_action: 'super_action_thing', collector_tstamp: '1727476712646', - label: nil, - property: nil, - value: nil, + se_label: nil, + se_property: nil, + se_value: nil, contexts: nil }, rawEvent: { parameters: Rack::Utils.parse_query(query_params) } @@ -90,9 +90,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'projects:blob:show', se_action: 'click_blame_control_on_blob_page', collector_tstamp: '1727474524024', - label: nil, - property: nil, - value: nil, + se_label: nil, + se_property: nil, + se_value: nil, contexts: Gitlab::Json.parse(context) }, rawEvent: { parameters: Gitlab::Json.parse(body)['data'].first } @@ -116,9 +116,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'admin:dashboard:index', se_action: 'view_admin_dashboard_pageload', collector_tstamp: '1727473513835', - label: nil, - property: nil, - value: nil, + se_label: nil, + se_property: nil, + se_value: nil, contexts: Gitlab::Json.parse(context_1) }, rawEvent: { parameters: Gitlab::Json.parse(body)['data'].first } @@ -128,9 +128,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'admin:dashboard:index', se_action: 'render', collector_tstamp: '1727473513837', - label: 'version_badge', - property: 'Up to date', - value: nil, + se_label: 'version_badge', + se_property: 'Up to date', + se_value: nil, contexts: Gitlab::Json.parse(context_2) }, rawEvent: { parameters: Gitlab::Json.parse(body)['data'].last } @@ -153,9 +153,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'admin:dashboard:index', se_action: 'render', collector_tstamp: '1727473512782', - label: 'version_badge', - property: 'Up to date', - value: nil, + se_label: 'version_badge', + se_property: 'Up to date', + se_value: nil, contexts: Gitlab::Json.parse(context) }, rawEvent: { parameters: Gitlab::Json.parse(body)['data'].first } @@ -213,9 +213,9 @@ RSpec.describe Server, feature_category: :service_ping do se_category: 'category', se_action: 'super_action_thing', collector_tstamp: '1727476712646', - label: nil, - property: nil, - value: nil, + se_label: nil, + se_property: nil, + se_value: nil, contexts: nil }, rawEvent: { parameters: Rack::Utils.parse_query(query_params) } diff --git a/spec/tasks/gitlab/keep_around_rake_spec.rb b/spec/tasks/gitlab/keep_around_rake_spec.rb index 2e6e1ff7a85..ade11e59c37 100644 --- a/spec/tasks/gitlab/keep_around_rake_spec.rb +++ b/spec/tasks/gitlab/keep_around_rake_spec.rb @@ -13,7 +13,8 @@ RSpec.describe 'keep-around tasks', :silence_stdout, feature_category: :source_c describe 'orphaned' do subject { run_rake_task('gitlab:keep_around:orphaned') } - let_it_be(:project) { create(:project, :repository) } + let_it_be(:original_project) { create(:project, :repository) } + let_it_be(:project) { fork_project(original_project, nil, repository: true) } let_it_be(:keep_around_shas) do # Keep-around references only on branch tips is not necessarily accurate, # but this constant gives convenient access to commit IDs that actually @@ -123,8 +124,41 @@ RSpec.describe 'keep-around tasks', :silence_stdout, feature_category: :source_c end context "for merge request keep-arounds" do - let_it_be(:fork) { fork_project(project, nil, repository: true) } - let_it_be(:merge_request) { create(:merge_request, target_project: project, source_project: fork) } + let_it_be(:merge_request) do + create(:merge_request, :skip_diff_creation, + target_project: project, + source_project: project, + merge_commit_sha: TestEnv::BRANCH_SHA['changes-with-only-whitespace']) + end + + it_behaves_like 'orphans found', + keep_around_count: 3, + orphan_count: 2 + end + + context "for fork merge request keep-arounds" do + let_it_be(:merge_request) do + create(:merge_request, :skip_diff_creation, + target_project: original_project, + source_project: project, + merge_commit_sha: TestEnv::BRANCH_SHA['changes-with-only-whitespace']) + end + + it_behaves_like 'orphans found', + keep_around_count: 3, + orphan_count: 3 + end + + context "for merge request diff keep-arounds" do + let_it_be(:merge_request) { create(:merge_request, target_project: project, source_project: project) } + + it_behaves_like 'orphans found', + keep_around_count: 3, + orphan_count: 2 + end + + context "for fork merge request diff keep-arounds" do + let_it_be(:merge_request) { create(:merge_request, target_project: original_project, source_project: project) } it_behaves_like 'orphans found', keep_around_count: 3,