diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index b62d12a3971..031f5dc45ca 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -427,10 +427,10 @@ padding-inline-start: 28px; margin-inline-start: 0 !important; - input.task-list-item-checkbox { + > input.task-list-item-checkbox { position: absolute; - inset-inline-start: $gl-padding-8; - inset-block-start: 5px; + inset-inline-start: 8px; + top: 5px; } } } diff --git a/app/assets/stylesheets/page_bundles/issues_show.scss b/app/assets/stylesheets/page_bundles/issues_show.scss index 9876003e652..c664e0a734e 100644 --- a/app/assets/stylesheets/page_bundles/issues_show.scss +++ b/app/assets/stylesheets/page_bundles/issues_show.scss @@ -1,24 +1,77 @@ @import 'mixins_and_variables_and_functions'; .description { - ul.task-list > li.task-list-item { - margin-inline-start: 0.5rem !important; /* Override typography.scss */ + ul, + ol { + /* We're changing list-style-position to inside because the default of + * outside doesn't move negative margin to the left of the bullet. */ + list-style-position: inside; } li { position: relative; - margin-inline-start: 2.25rem; - - &.task-list-item > .drag-icon { - inset-inline-start: -0.6rem; - } + /* In the browser, the li element comes after (to the right of) the bullet point, so hovering + * over the left of the bullet point doesn't trigger a row hover. To trigger hovering on the + * left, we're applying negative margin here to shift the li element left. */ + margin-inline-start: -1rem; + padding-inline-start: 2.5rem; .drag-icon { position: absolute; inset-block-start: 0.3rem; - inset-inline-start: -2.3rem; - padding-inline-end: 1rem; - width: 2rem; + inset-inline-start: 1rem; + } + + /* The inside bullet aligns itself to the bottom, which we see when text to the right of + * a multi-line list item wraps. We fix this by aligning it to the top, and excluding + * other elements. Targeting ::marker doesn't seem to work, instead we exclude custom elements + * or anything with a class */ + > *:not(gl-emoji, code, [class]) { + vertical-align: top; + } + + /* The inside bullet is treated like an element inside the li element, so when we have a + * multi-paragraph list item, the text doesn't start on the right of the bullet because + * it is a block level p element. We make it inline to fix this. */ + > p:first-of-type { + display: inline-block; + max-width: calc(100% - 1.5rem); + } + + /* We fix the other paragraphs not indenting to the + * right of the bullet due to the inside bullet. */ + p ~ a, + p ~ blockquote, + p ~ code, + p ~ details, + p ~ dl, + p ~ h1, + p ~ h2, + p ~ h3, + p ~ h4, + p ~ h5, + p ~ h6, + p ~ hr, + p ~ ol, + p ~ p, + p ~ table:not(.code), /* We need :not(.code) to override typography.scss */ + p ~ ul, + p ~ .markdown-code-block { + margin-inline-start: 1rem; + } + } + + ul.task-list { + > li.task-list-item { + /* We're using !important to override the same selector in typography.scss */ + margin-inline-start: -1rem !important; + padding-inline-start: 2.5rem; + + > input.task-list-item-checkbox { + position: static; + vertical-align: middle; + margin-block-start: -2px; + } } } } diff --git a/app/graphql/mutations/merge_requests/remove_attention_request.rb b/app/graphql/mutations/merge_requests/remove_attention_request.rb deleted file mode 100644 index 3b12b09528b..00000000000 --- a/app/graphql/mutations/merge_requests/remove_attention_request.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Mutations - module MergeRequests - class RemoveAttentionRequest < Base - graphql_name 'MergeRequestRemoveAttentionRequest' - - argument :user_id, ::Types::GlobalIDType[::User], - loads: Types::UserType, - required: true, - description: <<~DESC - User ID of the user for attention request removal. - DESC - - def resolve(project_path:, iid:, user:) - raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless feature_enabled? - - merge_request = authorized_find!(project_path: project_path, iid: iid) - - result = ::MergeRequests::RemoveAttentionRequestedService.new( - project: merge_request.project, - current_user: current_user, - merge_request: merge_request, - user: user - ).execute - - { - merge_request: merge_request, - errors: Array(result[:message]) - } - end - - private - - def feature_enabled? - current_user&.mr_attention_requests_enabled? - end - end - end -end diff --git a/app/graphql/mutations/merge_requests/request_attention.rb b/app/graphql/mutations/merge_requests/request_attention.rb deleted file mode 100644 index 5f5565285f6..00000000000 --- a/app/graphql/mutations/merge_requests/request_attention.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Mutations - module MergeRequests - class RequestAttention < Base - graphql_name 'MergeRequestRequestAttention' - - argument :user_id, ::Types::GlobalIDType[::User], - loads: Types::UserType, - required: true, - description: <<~DESC - User ID of the user to request attention. - DESC - - def resolve(project_path:, iid:, user:) - raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless feature_enabled? - - merge_request = authorized_find!(project_path: project_path, iid: iid) - - result = ::MergeRequests::RequestAttentionService.new( - project: merge_request.project, - current_user: current_user, - merge_request: merge_request, - user: user - ).execute - - { - merge_request: merge_request, - errors: Array(result[:message]) - } - end - - private - - def feature_enabled? - current_user&.mr_attention_requests_enabled? - end - end - end -end diff --git a/app/graphql/mutations/merge_requests/toggle_attention_requested.rb b/app/graphql/mutations/merge_requests/toggle_attention_requested.rb deleted file mode 100644 index 8913ca48531..00000000000 --- a/app/graphql/mutations/merge_requests/toggle_attention_requested.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Mutations - module MergeRequests - class ToggleAttentionRequested < Base - graphql_name 'MergeRequestToggleAttentionRequested' - - argument :user_id, ::Types::GlobalIDType[::User], - loads: Types::UserType, - required: true, - description: <<~DESC - User ID for the user to toggle attention requested. - DESC - - def resolve(project_path:, iid:, user:) - raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless current_user&.mr_attention_requests_enabled? - - merge_request = authorized_find!(project_path: project_path, iid: iid) - - result = ::MergeRequests::ToggleAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: user).execute - - { - merge_request: merge_request, - errors: Array(result[:message]) - } - end - end - end -end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index db4fb8fb14a..47462b5e9f7 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -73,9 +73,6 @@ module Types mount_mutation Mutations::MergeRequests::SetDraft, calls_gitaly: true mount_mutation Mutations::MergeRequests::SetAssignees mount_mutation Mutations::MergeRequests::ReviewerRereview - mount_mutation Mutations::MergeRequests::RequestAttention - mount_mutation Mutations::MergeRequests::RemoveAttentionRequest - mount_mutation Mutations::MergeRequests::ToggleAttentionRequested mount_mutation Mutations::Metrics::Dashboard::Annotations::Create mount_mutation Mutations::Metrics::Dashboard::Annotations::Delete mount_mutation Mutations::Notes::Create::Note, calls_gitaly: true diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb index 5ab70115f34..a957c9ce9e0 100644 --- a/app/helpers/system_note_helper.rb +++ b/app/helpers/system_note_helper.rb @@ -30,6 +30,7 @@ module SystemNoteHelper 'locked' => 'lock', 'unlocked' => 'lock-open', 'due_date' => 'calendar', + 'start_date_or_due_date' => 'calendar', 'health_status' => 'status-health', 'designs_added' => 'doc-image', 'designs_modified' => 'doc-image', diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index f2038c77455..b81a9b51e1c 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -515,11 +515,23 @@ module Issuable changes end + def hook_reviewer_changes(old_associations) + changes = {} + old_reviewers = old_associations.fetch(:reviewers, reviewers) + + if old_reviewers != reviewers + changes[:reviewers] = [old_reviewers.map(&:hook_attrs), reviewers.map(&:hook_attrs)] + end + + changes + end + def to_hook_data(user, old_associations: {}) changes = previous_changes if old_associations.present? changes.merge!(hook_association_changes(old_associations)) + changes.merge!(hook_reviewer_changes(old_associations)) if allows_reviewers? end Gitlab::DataBuilder::Issuable.new(self).build(user: user, changes: changes) diff --git a/app/models/members/last_group_owner_assigner.rb b/app/models/members/last_group_owner_assigner.rb index c85116858c7..e411a0ef5eb 100644 --- a/app/models/members/last_group_owner_assigner.rb +++ b/app/models/members/last_group_owner_assigner.rb @@ -8,7 +8,7 @@ class LastGroupOwnerAssigner end def execute - @last_blocked_owner = no_owners_in_heirarchy? && group.single_blocked_owner? + @last_blocked_owner = no_owners_in_hierarchy? && group.single_blocked_owner? @group_single_owner = owners.size == 1 members.each { |member| set_last_owner(member) } @@ -18,7 +18,7 @@ class LastGroupOwnerAssigner attr_reader :group, :members, :last_blocked_owner, :group_single_owner - def no_owners_in_heirarchy? + def no_owners_in_hierarchy? owners.empty? end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 7cf15439b47..76c277e4b86 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -4,8 +4,6 @@ class ProtectedBranch < ApplicationRecord include ProtectedRef include Gitlab::SQL::Pattern - CACHE_EXPIRE_IN = 1.hour - scope :requiring_code_owner_approval, -> { where(code_owner_approval_required: true) } @@ -27,10 +25,30 @@ class ProtectedBranch < ApplicationRecord end # Check if branch name is marked as protected in the system - def self.protected?(project, ref_name) + def self.protected?(project, ref_name, dry_run: true) return true if project.empty_repo? && project.default_branch_protected? return false if ref_name.blank? + new_cache_result = new_cache(project, ref_name, dry_run: dry_run) + + return new_cache_result unless new_cache_result.nil? + + deprecated_cache(project, ref_name) + end + + def self.new_cache(project, ref_name, dry_run: true) + if Feature.enabled?(:hash_based_cache_for_protected_branches, project) + ProtectedBranches::CacheService.new(project).fetch(ref_name, dry_run: dry_run) do # rubocop: disable CodeReuse/ServiceClass + self.matching(ref_name, protected_refs: protected_refs(project)).present? + end + end + end + + # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/368279 + # ---------------------------------------------------------------- + CACHE_EXPIRE_IN = 1.hour + + def self.deprecated_cache(project, ref_name) Rails.cache.fetch(protected_ref_cache_key(project, ref_name), expires_in: CACHE_EXPIRE_IN) do self.matching(ref_name, protected_refs: protected_refs(project)).present? end @@ -39,6 +57,7 @@ class ProtectedBranch < ApplicationRecord def self.protected_ref_cache_key(project, ref_name) "protected_ref-#{project.cache_key}-#{Digest::SHA1.hexdigest(ref_name)}" end + # End of deprecation -------------------------------------------- def self.allow_force_push?(project, ref_name) project.protected_branches.allowing_force_push.matching(ref_name).any? diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 2643ef272d8..cc389dbe3f4 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -22,7 +22,7 @@ class SystemNoteMetadata < ApplicationRecord designs_added designs_modified designs_removed designs_discussion_added title time_tracking branch milestone discussion task moved cloned opened closed merged duplicate locked unlocked outdated reviewer - tag due_date pinned_embed cherry_pick health_status approved unapproved + tag due_date start_date_or_due_date pinned_embed cherry_pick health_status approved unapproved status alert_issue_added relate unrelate new_alert_added severity attention_requested attention_request_removed contact timeline_event ].freeze diff --git a/app/serializers/merge_request_user_entity.rb b/app/serializers/merge_request_user_entity.rb index 12c573d1a13..2e875af6531 100644 --- a/app/serializers/merge_request_user_entity.rb +++ b/app/serializers/merge_request_user_entity.rb @@ -20,10 +20,6 @@ class MergeRequestUserEntity < ::API::Entities::UserBasic find_reviewer_or_assignee(user, options)&.reviewed? end - expose :attention_requested, if: ->(_, options) { options[:merge_request].present? && options[:merge_request].allows_reviewers? && request.current_user&.mr_attention_requests_enabled? } do |user, options| - find_reviewer_or_assignee(user, options)&.attention_requested? - end - expose :approved, if: satisfies(:present?) do |user, options| # This approach is preferred over MergeRequest#approved_by? since this # makes one query per merge request, whereas #approved_by? makes one per user diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb index 5cf32ee3e40..db28be864a7 100644 --- a/app/services/issuable/common_system_notes_service.rb +++ b/app/services/issuable/common_system_notes_service.rb @@ -21,7 +21,7 @@ module Issuable create_discussion_lock_note if issuable.previous_changes.include?('discussion_locked') end - create_due_date_note if issuable.previous_changes.include?('due_date') + handle_start_date_or_due_date_change_note create_milestone_change_event(old_milestone) if issuable.previous_changes.include?('milestone_id') create_labels_note(old_labels) if old_labels && issuable.labels != old_labels end @@ -29,6 +29,13 @@ module Issuable private + def handle_start_date_or_due_date_change_note + # Type check needed as some issuables do their own date change handling for date fields other than due_date + change_date_fields = issuable.is_a?(Issue) ? %w[due_date start_date] : %w[due_date] + changed_dates = issuable.previous_changes.slice(*change_date_fields) + create_start_date_or_due_date_note(changed_dates) + end + def handle_time_tracking_note if issuable.previous_changes.include?('time_estimate') create_time_estimate_note @@ -99,8 +106,10 @@ module Issuable .execute end - def create_due_date_note - SystemNoteService.change_due_date(issuable, issuable.project, current_user, issuable.due_date) + def create_start_date_or_due_date_note(changed_dates) + return if changed_dates.blank? + + SystemNoteService.change_start_date_or_due_date(issuable, issuable.project, current_user, changed_dates) end def create_discussion_lock_note diff --git a/app/services/protected_branches/cache_service.rb b/app/services/protected_branches/cache_service.rb index fed5dca2581..8c521f4ebcb 100644 --- a/app/services/protected_branches/cache_service.rb +++ b/app/services/protected_branches/cache_service.rb @@ -7,17 +7,24 @@ module ProtectedBranches CACHE_EXPIRE_IN = 1.day CACHE_LIMIT = 1000 - def fetch(ref_name) + def fetch(ref_name, dry_run: false) record = OpenSSL::Digest::SHA256.hexdigest(ref_name) Gitlab::Redis::Cache.with do |redis| cached_result = redis.hget(redis_key, record) - break Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil? + decoded_result = Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil? - value = yield + # If we're dry-running, don't break because we need to check against + # the real value to ensure the cache is working properly. + # If the result is nil we'll need to run the block, so don't break yet. + break decoded_result unless dry_run || decoded_result.nil? - redis.hset(redis_key, record, Gitlab::Redis::Boolean.encode(value)) + calculated_value = yield + + check_and_log_discrepancy(decoded_result, calculated_value, ref_name) if dry_run + + redis.hset(redis_key, record, Gitlab::Redis::Boolean.encode(calculated_value)) # We don't want to extend cache expiration time if redis.ttl(redis_key) == TTL_UNSET @@ -30,7 +37,7 @@ module ProtectedBranches redis.unlink(redis_key) end - value + calculated_value end end @@ -40,6 +47,20 @@ module ProtectedBranches private + def check_and_log_discrepancy(cached_value, real_value, ref_name) + return if cached_value.nil? + return if cached_value == real_value + + encoded_ref_name = Gitlab::EncodingHelper.encode_utf8_with_replacement_character(ref_name) + + log_error( + 'class' => self.class.name, + 'message' => "Cache mismatch '#{encoded_ref_name}': cached value: #{cached_value}, real value: #{real_value}", + 'project_id' => @project.id, + 'project_path' => @project.full_path + ) + end + def redis_key @redis_key ||= [CACHE_ROOT_KEY, @project.id].join(':') end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 332333d2fc3..9baf749c508 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -57,7 +57,7 @@ module SystemNoteService ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).unrelate_issuable(noteable_ref) end - # Called when the due_date of a Noteable is changed + # Called when the due_date or start_date of a Noteable is changed # # noteable - Noteable object # project - Project owning noteable @@ -68,11 +68,15 @@ module SystemNoteService # # "removed due date" # - # "changed due date to September 20, 2018" + # "changed due date to September 20, 2018 and changed start date to September 25, 2018" # # Returns the created Note object - def change_due_date(noteable, project, author, due_date) - ::SystemNotes::TimeTrackingService.new(noteable: noteable, project: project, author: author).change_due_date(due_date) + def change_start_date_or_due_date(noteable, project, author, changed_dates) + ::SystemNotes::TimeTrackingService.new( + noteable: noteable, + project: project, + author: author + ).change_start_date_or_due_date(changed_dates) end # Called when the estimated time of a Noteable is changed diff --git a/app/services/system_notes/time_tracking_service.rb b/app/services/system_notes/time_tracking_service.rb index f99c7c9c0bb..c867cce8032 100644 --- a/app/services/system_notes/time_tracking_service.rb +++ b/app/services/system_notes/time_tracking_service.rb @@ -2,8 +2,9 @@ module SystemNotes class TimeTrackingService < ::SystemNotes::BaseService - # Called when the due_date of a Noteable is changed + # Called when the start_date or due_date of an Issue/WorkItem is changed # + # start_date - Start date being assigned, or nil # due_date - Due date being assigned, or nil # # Example Note text: @@ -11,14 +12,20 @@ module SystemNotes # "removed due date" # # "changed due date to September 20, 2018" + + # "changed start date to September 20, 2018 and changed due date to September 25, 2018" # # Returns the created Note object - def change_due_date(due_date) - body = due_date ? "changed due date to #{due_date.to_s(:long)}" : 'removed due date' + def change_start_date_or_due_date(changed_dates = {}) + return if changed_dates.empty? - issue_activity_counter.track_issue_due_date_changed_action(author: author) if noteable.is_a?(Issue) + if noteable.is_a?(Issue) && changed_dates.key?('due_date') + issue_activity_counter.track_issue_due_date_changed_action(author: author) + end - create_note(NoteSummary.new(noteable, project, author, body, action: 'due_date')) + create_note( + NoteSummary.new(noteable, project, author, changed_date_body(changed_dates), action: 'start_date_or_due_date') + ) end # Called when the estimated time of a Noteable is changed @@ -116,6 +123,27 @@ module SystemNotes private + def changed_date_body(changed_dates) + %w[start_date due_date].each_with_object([]) do |date_field, word_array| + next unless changed_dates.key?(date_field) + + word_array << 'and' if word_array.any? + + word_array << message_for_changed_date(changed_dates, date_field) + end.join(' ') + end + + def message_for_changed_date(changed_dates, date_key) + changed_date = changed_dates[date_key].last + readable_date = date_key.humanize.downcase + + if changed_date.nil? + "removed #{readable_date}" + else + "changed #{readable_date} to #{changed_date.to_s(:long)}" + end + end + def issue_activity_counter Gitlab::UsageDataCounters::IssueActivityUniqueCounter end diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 91100194f35..c220aa66c81 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -29,7 +29,7 @@ - if current_user - if current_user.admin? = link_to [:admin, @project], class: 'btn gl-button btn-icon gl-align-self-start gl-py-2! gl-mr-3', title: _('View project in admin area'), - data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do + data: {toggle: 'tooltip', placement: 'top', container: 'body'} do = sprite_icon('admin') .gl-display-flex.gl-align-items-start.gl-mr-3 - if @notification_setting diff --git a/config/feature_flags/development/hash_based_cache_for_protected_branches.yml b/config/feature_flags/development/hash_based_cache_for_protected_branches.yml new file mode 100644 index 00000000000..4e071707182 --- /dev/null +++ b/config/feature_flags/development/hash_based_cache_for_protected_branches.yml @@ -0,0 +1,8 @@ +--- +name: hash_based_cache_for_protected_branches +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92934 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/368279 +milestone: '15.3' +type: development +group: group::source code +default_enabled: false diff --git a/doc/administration/gitaly/troubleshooting.md b/doc/administration/gitaly/troubleshooting.md index 9286967c227..7dda0f0f1b5 100644 --- a/doc/administration/gitaly/troubleshooting.md +++ b/doc/administration/gitaly/troubleshooting.md @@ -360,6 +360,17 @@ push, which causes a significant delay. If Git pushes are too slow when Dynatrace is enabled, disable Dynatrace. +### Find storage resource details + +You can run the following commands in a [Rails conosole](../operations/rails_console.md#starting-a-rails-console-session) to determine the available and used space on a +Gitaly storage: + +```ruby +Gitlab::GitalyClient::ServerService.new("default").storage_disk_statistics +# For Gitaly Cluster +Gitlab::GitalyClient::ServerService.new("").disk_statistics +``` + ## Troubleshoot Praefect (Gitaly Cluster) The following sections provide possible solutions to Gitaly Cluster errors. diff --git a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md index 0618d363499..1c09e36fd57 100644 --- a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md +++ b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md @@ -1392,16 +1392,6 @@ registry = Geo::SnippetRepositoryRegistry.find(registry_id) registry.replicator.send(:sync_repository) ``` -## Gitaly - -### Find available and used space - -A Gitaly storage resource can be polled through Rails to determine the available and used space. - -```ruby -Gitlab::GitalyClient::ServerService.new("default").storage_disk_statistics -``` - ## Generate Service Ping The [Service Ping Guide](../../development/service_ping/index.md) in our developer documentation diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 60581481a71..4e021da238d 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3634,48 +3634,6 @@ Input type: `MergeRequestCreateInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | -### `Mutation.mergeRequestRemoveAttentionRequest` - -Input type: `MergeRequestRemoveAttentionRequestInput` - -#### Arguments - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `iid` | [`String!`](#string) | IID of the merge request to mutate. | -| `projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. | -| `userId` | [`UserID!`](#userid) | User ID of the user for attention request removal. | - -#### Fields - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | -| `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | - -### `Mutation.mergeRequestRequestAttention` - -Input type: `MergeRequestRequestAttentionInput` - -#### Arguments - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `iid` | [`String!`](#string) | IID of the merge request to mutate. | -| `projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. | -| `userId` | [`UserID!`](#userid) | User ID of the user to request attention. | - -#### Fields - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | -| `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | - ### `Mutation.mergeRequestReviewerRereview` Input type: `MergeRequestReviewerRereviewInput` @@ -3825,27 +3783,6 @@ Input type: `MergeRequestSetSubscriptionInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | -### `Mutation.mergeRequestToggleAttentionRequested` - -Input type: `MergeRequestToggleAttentionRequestedInput` - -#### Arguments - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `iid` | [`String!`](#string) | IID of the merge request to mutate. | -| `projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. | -| `userId` | [`UserID!`](#userid) | User ID for the user to toggle attention requested. | - -#### Fields - -| Name | Type | Description | -| ---- | ---- | ----------- | -| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | -| `mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | - ### `Mutation.mergeRequestUpdate` Update attributes of a merge request. diff --git a/doc/security/webhooks.md b/doc/security/webhooks.md index c3d445103c4..f2066ee4a42 100644 --- a/doc/security/webhooks.md +++ b/doc/security/webhooks.md @@ -74,7 +74,7 @@ allowlist: The allowed entries can be separated by semicolons, commas or whitespaces (including newlines) and be in different formats like hostnames, IP addresses and/or IP ranges. IPv6 is supported. Hostnames that contain Unicode characters should -use [Internationalized Domain Names in Applications](https://www.icann.org/resources/pages/glossary-2014-02-04-en#i) +use [Internationalized Domain Names in Applications](https://www.icann.org/en/icann-acronyms-and-terms/internationalized-domain-names-in-applications-en) (IDNA) encoding. The allowlist can hold a maximum of 1000 entries. Each entry can be a maximum of diff --git a/doc/subscriptions/gitlab_com/index.md b/doc/subscriptions/gitlab_com/index.md index b81424cddcd..6b83a00cac1 100644 --- a/doc/subscriptions/gitlab_com/index.md +++ b/doc/subscriptions/gitlab_com/index.md @@ -297,7 +297,7 @@ for your personal or group namespace. CI/CD minutes are a **one-time purchase**, ## Add-on subscription for additional Storage and Transfer NOTE: -Free namespaces are subject to a 5GB storage and 10GB transfer [soft limit](https://about.gitlab.com/pricing). Once all storage is available to view in the usage quota workflow, GitLab will automatically enforce the namespace storage limit and the project limit will be removed. This change will be announced separately. The storage and transfer add-on can be purchased to increase the limits. +Free namespaces are subject to a 5GB storage and 10GB transfer [soft limit](https://about.gitlab.com/pricing/). Once all storage is available to view in the usage quota workflow, GitLab will automatically enforce the namespace storage limit and the project limit will be removed. This change will be announced separately. The storage and transfer add-on can be purchased to increase the limits. Projects have a free storage quota of 10 GB. To exceed this quota you must first [purchase one or more storage subscription units](#purchase-more-storage-and-transfer). Each unit provides 10 GB of additional diff --git a/doc/topics/autodevops/cloud_deployments/auto_devops_with_gke.md b/doc/topics/autodevops/cloud_deployments/auto_devops_with_gke.md index b3ce2aa1683..1fea573faa1 100644 --- a/doc/topics/autodevops/cloud_deployments/auto_devops_with_gke.md +++ b/doc/topics/autodevops/cloud_deployments/auto_devops_with_gke.md @@ -35,7 +35,7 @@ you need a [Google Cloud Platform account](https://console.cloud.google.com). Sign in with an existing Google account, such as the one you use to access Gmail or Google Drive, or create a new one. -1. Follow the steps described in the ["Before you begin" section](https://cloud.google.com/kubernetes-engine/docs/quickstart#before-you-begin) +1. Follow the steps described in the ["Before you begin" section](https://cloud.google.com/kubernetes-engine/docs/deploy-app-cluster#before-you-begin) of the Kubernetes Engine documentation to enable the required APIs and related services. 1. Ensure you've created a [billing account](https://cloud.google.com/billing/docs/how-to/manage-billing-account) with Google Cloud Platform. diff --git a/doc/update/plan_your_upgrade.md b/doc/update/plan_your_upgrade.md index d1038fdb912..b86d3789fc2 100644 --- a/doc/update/plan_your_upgrade.md +++ b/doc/update/plan_your_upgrade.md @@ -14,7 +14,7 @@ General notes: - If possible, we recommend you test out the upgrade in a test environment before updating your production instance. Ideally, your test environment should mimic your production environment as closely as possible. -- If [working with Support](https://about.gitlab.com/support/scheduling-upgrade-assistance.html) +- If [working with Support](https://about.gitlab.com/support/scheduling-upgrade-assistance/) to create your plan, share details of your architecture, including: - How is GitLab installed? - What is the operating system of the node? diff --git a/doc/update/zero_downtime.md b/doc/update/zero_downtime.md index ae308925298..0abdd769a6b 100644 --- a/doc/update/zero_downtime.md +++ b/doc/update/zero_downtime.md @@ -395,7 +395,7 @@ HA. #### In the application node -According to [official Redis documentation](https://redis.io/topics/admin#upgrading-or-restarting-a-redis-instance-without-downtime), +According to [official Redis documentation](https://redis.io/docs/manual/admin/#upgrading-or-restarting-a-redis-instance-without-downtime), the easiest way to update an HA instance using Sentinel is to upgrade the secondaries one after the other, perform a manual failover from current primary (running old version) to a recently upgraded secondary (running a new diff --git a/doc/user/application_security/api_fuzzing/index.md b/doc/user/application_security/api_fuzzing/index.md index 96236f60417..c04d10a6e93 100644 --- a/doc/user/application_security/api_fuzzing/index.md +++ b/doc/user/application_security/api_fuzzing/index.md @@ -1802,7 +1802,7 @@ To detect and correct elements that don't comply with the OpenAPI specifications | Editor | OpenAPI 2.0 | OpenAPI 3.0.x | OpenAPI 3.1.x | | -- | -- | -- | -- | | [Swagger Editor](https://editor.swagger.io/) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{dotted-circle}** YAML, JSON | -| [Stoplight Studio](https://stoplight.io/studio/) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | +| [Stoplight Studio](https://stoplight.io/studio) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | If your OpenAPI document is generated manually, load your document in the editor and fix anything that is non-compliant. If your document is generated automatically, load it in your editor to identify the issues in the schema, then go to the application and perform the corrections based on the framework you are using. diff --git a/doc/user/application_security/container_scanning/index.md b/doc/user/application_security/container_scanning/index.md index 1249f27849b..a5b2cdc08ba 100644 --- a/doc/user/application_security/container_scanning/index.md +++ b/doc/user/application_security/container_scanning/index.md @@ -680,7 +680,7 @@ It's possible to run the [GitLab container scanning tool](https://gitlab.com/git against a Docker container without needing to run it within the context of a CI job. To scan an image directly, follow these steps: -1. Run [Docker Desktop](https://www.docker.com/products/docker-desktop) +1. Run [Docker Desktop](https://www.docker.com/products/docker-desktop/) or [Docker Machine](https://github.com/docker/machine). 1. Run the analyzer's Docker image, passing the image and tag you want to analyze in the diff --git a/doc/user/application_security/dast_api/index.md b/doc/user/application_security/dast_api/index.md index fdca02267e4..6bcc7b44b4e 100644 --- a/doc/user/application_security/dast_api/index.md +++ b/doc/user/application_security/dast_api/index.md @@ -1612,7 +1612,7 @@ To detect and correct elements that don't comply with the OpenAPI specifications | Editor | OpenAPI 2.0 | OpenAPI 3.0.x | OpenAPI 3.1.x | | -- | -- | -- | -- | | [Swagger Editor](https://editor.swagger.io/) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{dotted-circle}** YAML, JSON | -| [Stoplight Studio](https://stoplight.io/studio/) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | +| [Stoplight Studio](https://stoplight.io/studio) | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | **{check-circle}** YAML, JSON | If your OpenAPI document is generated manually, load your document in the editor and fix anything that is non-compliant. If your document is generated automatically, load it in your editor to identify the issues in the schema, then go to the application and perform the corrections based on the framework you are using. diff --git a/doc/user/clusters/agent/install/index.md b/doc/user/clusters/agent/install/index.md index 9282ac7fb40..4b0d8b77493 100644 --- a/doc/user/clusters/agent/install/index.md +++ b/doc/user/clusters/agent/install/index.md @@ -17,7 +17,7 @@ To connect a Kubernetes cluster to GitLab, you must install an agent in your clu Before you can install the agent in your cluster, you need: - An existing Kubernetes cluster. If you don't have a cluster, you can create one on a cloud provider, like: - - [Google Kubernetes Engine (GKE)](https://cloud.google.com/kubernetes-engine/docs/quickstart) + - [Google Kubernetes Engine (GKE)](https://cloud.google.com/kubernetes-engine/docs/deploy-app-cluster) - [Amazon Elastic Kubernetes Service (EKS)](https://docs.aws.amazon.com/eks/latest/userguide/getting-started.html) - [Digital Ocean](https://docs.digitalocean.com/products/kubernetes/quickstart/) - On self-managed GitLab instances, a GitLab administrator must set up the diff --git a/doc/user/group/saml_sso/scim_setup.md b/doc/user/group/saml_sso/scim_setup.md index e7c98e0ae08..4e9305b88cf 100644 --- a/doc/user/group/saml_sso/scim_setup.md +++ b/doc/user/group/saml_sso/scim_setup.md @@ -106,7 +106,7 @@ Before you start this section: - Check that you are using Okta [Lifecycle Management](https://www.okta.com/products/lifecycle-management/) product. This product tier is required to use SCIM on Okta. To check which Okta product you are using, check your signed Okta contract, contact your Okta AE, CSM, or Okta support. - Complete the [GitLab configuration](#gitlab-configuration) process. -- Complete the setup for SAML application for [Okta](https://developer.okta.com/docs/guides/build-sso-integration/saml2/overview/), as described in the [Okta setup notes](index.md#okta-setup-notes). +- Complete the setup for SAML application for [Okta](https://developer.okta.com/docs/guides/build-sso-integration/saml2/main/), as described in the [Okta setup notes](index.md#okta-setup-notes). - Check that your Okta SAML setup matches our documentation exactly, especially the NameID configuration. Otherwise, the Okta SCIM app may not work properly. After the above steps are complete: diff --git a/doc/user/project/clusters/add_gke_clusters.md b/doc/user/project/clusters/add_gke_clusters.md index 71b65867c4a..bfaf9aab7b7 100644 --- a/doc/user/project/clusters/add_gke_clusters.md +++ b/doc/user/project/clusters/add_gke_clusters.md @@ -37,7 +37,7 @@ Prerequisites: set up with access. - Kubernetes Engine API and related services enabled. It should work immediately but may take up to 10 minutes after you create a project. For more information see the - ["Before you begin" section of the Kubernetes Engine docs](https://cloud.google.com/kubernetes-engine/docs/quickstart#before-you-begin). + ["Before you begin" section of the Kubernetes Engine docs](https://cloud.google.com/kubernetes-engine/docs/deploy-app-cluster#before-you-begin). Note the following: diff --git a/doc/user/project/import/github.md b/doc/user/project/import/github.md index a190edb179d..a3dfa3edff0 100644 --- a/doc/user/project/import/github.md +++ b/doc/user/project/import/github.md @@ -241,3 +241,35 @@ Feature.disable(:github_importer_lower_per_page_limit, group) For information on automating user, group, and project import API calls, see [Automate group and project import](index.md#automate-group-and-project-import). + +## Troubleshooting + +### Manually continue a previously failed import process + +In some cases, the GitHub import process can fail to import the repository. This causes GitLab to abort the project import process and requires the +repository to be imported manually. Administrators can manually import the repository for a failed import process: + +1. Open a Rails console. +1. Run the following series of commands in the console: + + ```ruby + project_id = + github_access_token = + github_repository_path = '/' + + github_repository_url = "https://#{github_access_token}@github.com/#{github_repository_path}.git" + + # Find project by ID + project = Project.find(project_id) + # Set import URL and credentials + project.import_url = github_repository_url + project.import_type = 'github' + project.import_source = github_repository_path + project.save! + # Create an import state if the project was created manually and not from a failed import + project.create_import_state if project.import_state.blank? + # Set state to start + project.import_state.force_start + # Trigger import from second step + Gitlab::GithubImport::Stage::ImportRepositoryWorker.perform_async(project.id) + ``` diff --git a/doc/user/project/integrations/webhook_events.md b/doc/user/project/integrations/webhook_events.md index 871ada102e0..51049f156b8 100644 --- a/doc/user/project/integrations/webhook_events.md +++ b/doc/user/project/integrations/webhook_events.md @@ -878,7 +878,9 @@ Payload example: "source_branch": "ms-viewport", "source_project_id": 14, "author_id": 51, + "assignee_ids": [6], "assignee_id": 6, + "reviewer_ids": [6], "title": "MS-Viewport", "created_at": "2013-12-03T17:23:34Z", "updated_at": "2013-12-03T17:23:34Z", @@ -945,12 +947,7 @@ Payload example: "type": "ProjectLabel", "group_id": 41 }], - "action": "open", - "assignee": { - "name": "User1", - "username": "user1", - "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" - } + "action": "open" }, "labels": [{ "id": 206, @@ -999,7 +996,23 @@ Payload example: "group_id": 41 }] } - } + }, + "assignees": [ + { + "id": 6, + "name": "User1", + "username": "user1", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" + } + ], + "reviewers": [ + { + "id": 6, + "name": "User1", + "username": "user1", + "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" + } + ] } ``` diff --git a/lib/api/user_counts.rb b/lib/api/user_counts.rb index d0b1e458a27..388aa5e375c 100644 --- a/lib/api/user_counts.rb +++ b/lib/api/user_counts.rb @@ -12,19 +12,13 @@ module API get do unauthorized! unless current_user - counts = { + { merge_requests: current_user.assigned_open_merge_requests_count, # @deprecated assigned_issues: current_user.assigned_open_issues_count, assigned_merge_requests: current_user.assigned_open_merge_requests_count, review_requested_merge_requests: current_user.review_requested_open_merge_requests_count, todos: current_user.todos_pending_count } - - if current_user&.mr_attention_requests_enabled? - counts[:attention_requests] = current_user.attention_requested_open_merge_requests_count - end - - counts end end end diff --git a/lib/gitlab/data_builder/issuable.rb b/lib/gitlab/data_builder/issuable.rb index d12537c4874..9a7b4d0e2aa 100644 --- a/lib/gitlab/data_builder/issuable.rb +++ b/lib/gitlab/data_builder/issuable.rb @@ -26,6 +26,10 @@ module Gitlab hook_data[:assignees] = issuable.assignees.map(&:hook_attrs) if issuable.assignees.any? + if issuable.allows_reviewers? && issuable.reviewers.any? + hook_data[:reviewers] = issuable.reviewers.map(&:hook_attrs) + end + hook_data end diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb index b4f90715293..65c623c5d7d 100644 --- a/lib/gitlab/hook_data/merge_request_builder.rb +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -22,6 +22,7 @@ module Gitlab merge_user_id merge_when_pipeline_succeeds milestone_id + reviewer_ids source_branch source_project_id state_id @@ -38,6 +39,7 @@ module Gitlab %i[ assignees labels + reviewers total_time_spent time_change ].freeze @@ -60,6 +62,7 @@ module Gitlab human_time_estimate: merge_request.human_time_estimate, assignee_ids: merge_request.assignee_ids, assignee_id: merge_request.assignee_ids.first, # This key is deprecated + reviewer_ids: merge_request.reviewer_ids, labels: merge_request.labels_hook_attrs, state: merge_request.state, # This key is deprecated blocking_discussions_resolved: merge_request.mergeable_discussions_state?, diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb index 167e7ad67a9..3cb01db1491 100644 --- a/lib/gitlab/quick_actions/merge_request_actions.rb +++ b/lib/gitlab/quick_actions/merge_request_actions.rb @@ -292,76 +292,6 @@ module Gitlab @updates[:reviewer_ids] = [] end end - - desc do - if quick_action_target.allows_multiple_reviewers? - _('Request attention from assignee(s) or reviewer(s)') - else - _('Request attention from assignee or reviewer') - end - end - explanation do |users| - _('Request attention from %{users_sentence}.') % { users_sentence: reviewer_users_sentence(users) } - end - execution_message do |users = nil| - if users.blank? - _("Failed to request attention because no user was found.") - else - _('Requested attention from %{users_sentence}.') % { users_sentence: reviewer_users_sentence(users) } - end - end - params do - quick_action_target.allows_multiple_reviewers? ? '@user1 @user2' : '@user' - end - types MergeRequest - condition do - current_user.mr_attention_requests_enabled? && - current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project) - end - parse_params do |attention_param| - extract_users(attention_param) - end - command :attention, :attn do |users| - next if users.empty? - - users.each do |user| - ::MergeRequests::ToggleAttentionRequestedService.new(project: quick_action_target.project, merge_request: quick_action_target, current_user: current_user, user: user).execute - end - end - - desc do - if quick_action_target.allows_multiple_reviewers? - _('Remove attention request(s)') - else - _('Remove attention request') - end - end - explanation do |users| - _('Removes attention from %{users_sentence}.') % { users_sentence: reviewer_users_sentence(users) } - end - execution_message do |users = nil| - if users.blank? - _("Failed to remove attention because no user was found.") - else - _('Removed attention from %{users_sentence}.') % { users_sentence: reviewer_users_sentence(users) } - end - end - params do - quick_action_target.allows_multiple_reviewers? ? '@user1 @user2' : '@user' - end - types MergeRequest - condition do - current_user.mr_attention_requests_enabled? && - current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project) - end - parse_params do |attention_param| - extract_users(attention_param) - end - command :remove_attention do |users| - next if users.empty? - - ::MergeRequests::BulkRemoveAttentionRequestedService.new(project: quick_action_target.project, merge_request: quick_action_target, current_user: current_user, users: users).execute - end end def reviewer_users_sentence(users) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6986bea34e7..53c591a5147 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16030,9 +16030,6 @@ msgstr "" msgid "Failed to remove a to-do item for the design." msgstr "" -msgid "Failed to remove attention because no user was found." -msgstr "" - msgid "Failed to remove mirror." msgstr "" @@ -16048,9 +16045,6 @@ msgstr "" msgid "Failed to remove user key." msgstr "" -msgid "Failed to request attention because no user was found." -msgstr "" - msgid "Failed to retrieve page" msgstr "" @@ -32514,12 +32508,6 @@ msgstr "" msgid "Remove assignee" msgstr "" -msgid "Remove attention request" -msgstr "" - -msgid "Remove attention request(s)" -msgstr "" - msgid "Remove avatar" msgstr "" @@ -32661,9 +32649,6 @@ msgstr "" msgid "Removed an issue from an epic." msgstr "" -msgid "Removed attention from %{users_sentence}." -msgstr "" - msgid "Removed group can not be restored!" msgstr "" @@ -32712,9 +32697,6 @@ msgstr "" msgid "Removes an issue from an epic." msgstr "" -msgid "Removes attention from %{users_sentence}." -msgstr "" - msgid "Removes parent epic %{epic_ref}." msgstr "" @@ -33191,15 +33173,6 @@ msgstr "" msgid "Request a new one" msgstr "" -msgid "Request attention from %{users_sentence}." -msgstr "" - -msgid "Request attention from assignee or reviewer" -msgstr "" - -msgid "Request attention from assignee(s) or reviewer(s)" -msgstr "" - msgid "Request data is too large" msgstr "" @@ -33224,9 +33197,6 @@ msgstr "" msgid "Requested %{time_ago}" msgstr "" -msgid "Requested attention from %{users_sentence}." -msgstr "" - msgid "Requested design version does not exist." msgstr "" diff --git a/spec/frontend/sidebar/reviewer_title_spec.js b/spec/frontend/sidebar/reviewer_title_spec.js index 3c250be5d5e..6b4eed5ad0f 100644 --- a/spec/frontend/sidebar/reviewer_title_spec.js +++ b/spec/frontend/sidebar/reviewer_title_spec.js @@ -47,7 +47,7 @@ describe('ReviewerTitle component', () => { editable: false, }); - expect(wrapper.find(GlLoadingIcon).exists()).toBeFalsy(); + expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); }); it('renders spinner when loading', () => { @@ -57,7 +57,7 @@ describe('ReviewerTitle component', () => { editable: false, }); - expect(wrapper.find(GlLoadingIcon).exists()).toBeTruthy(); + expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); }); it('does not render edit link when not editable', () => { diff --git a/spec/frontend/vue_shared/components/sidebar/todo_button_spec.js b/spec/frontend/vue_shared/components/sidebar/todo_button_spec.js index 853cc4a6331..01958a144ed 100644 --- a/spec/frontend/vue_shared/components/sidebar/todo_button_spec.js +++ b/spec/frontend/vue_shared/components/sidebar/todo_button_spec.js @@ -37,7 +37,7 @@ describe('Todo Button', () => { createComponent({}, mount); wrapper.findComponent(GlButton).trigger('click'); - expect(wrapper.emitted().click).toBeTruthy(); + expect(wrapper.emitted().click).toHaveLength(1); }); it('calls dispatchDocumentEvent to update global To-Do counter correctly', () => { diff --git a/spec/lib/gitlab/data_builder/issuable_spec.rb b/spec/lib/gitlab/data_builder/issuable_spec.rb index c1ae65c160f..f0802f335f4 100644 --- a/spec/lib/gitlab/data_builder/issuable_spec.rb +++ b/spec/lib/gitlab/data_builder/issuable_spec.rb @@ -113,6 +113,7 @@ RSpec.describe Gitlab::DataBuilder::Issuable do expect(data[:object_attributes]['assignee_id']).to eq(user.id) expect(data[:assignees].first).to eq(user.hook_attrs) expect(data).not_to have_key(:assignee) + expect(data).not_to have_key(:reviewers) end end @@ -126,5 +127,25 @@ RSpec.describe Gitlab::DataBuilder::Issuable do expect(data).not_to have_key(:assignee) end end + + context 'merge_request is assigned reviewers' do + let(:merge_request) { create(:merge_request, reviewers: [user]) } + let(:data) { described_class.new(merge_request).build(user: user) } + + it 'returns correct hook data' do + expect(data[:object_attributes]['reviewer_ids']).to match_array([user.id]) + expect(data[:reviewers].first).to eq(user.hook_attrs) + end + end + + context 'when merge_request does not have reviewers and assignees' do + let(:merge_request) { create(:merge_request) } + let(:data) { described_class.new(merge_request).build(user: user) } + + it 'returns correct hook data' do + expect(data).not_to have_key(:assignees) + expect(data).not_to have_key(:reviewers) + end + end end end diff --git a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb index 25b84a67ab2..cb8fef60ab2 100644 --- a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb @@ -29,6 +29,7 @@ RSpec.describe Gitlab::HookData::MergeRequestBuilder do merge_user_id merge_when_pipeline_succeeds milestone_id + reviewer_ids source_branch source_project_id state_id @@ -72,6 +73,7 @@ RSpec.describe Gitlab::HookData::MergeRequestBuilder do human_time_estimate assignee_ids assignee_id + reviewer_ids labels state blocking_discussions_resolved diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 87821de3cf5..6763cc904b4 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -569,6 +569,27 @@ RSpec.describe Issuable do end end + context 'merge_request update reviewers' do + let(:merge_request) { create(:merge_request) } + let(:user2) { create(:user) } + + before do + merge_request.update!(reviewers: [user]) + merge_request.update!(reviewers: [user, user2]) + expect(Gitlab::DataBuilder::Issuable) + .to receive(:new).with(merge_request).and_return(builder) + end + + it 'delegates to Gitlab::DataBuilder::Issuable#build' do + expect(builder).to receive(:build).with( + user: user, + changes: hash_including( + 'reviewers' => [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]] + )) + merge_request.to_hook_data(user, old_associations: { reviewers: [user] }) + end + end + context 'incident severity is updated' do let(:issue) { create(:incident) } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index a3fc09b31fb..3936e7127b8 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -167,36 +167,130 @@ RSpec.describe ProtectedBranch do expect(described_class.protected?(project, nil)).to eq(false) end - context 'with caching', :use_clean_rails_memory_store_caching do + context 'with caching', :use_clean_rails_redis_caching do let_it_be(:project) { create(:project, :repository) } let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "“jawn”") } + let(:feature_flag) { true } + let(:dry_run) { true } + + shared_examples_for 'hash based cache implementation' do + it 'calls only hash based cache implementation' do + expect_next_instance_of(ProtectedBranches::CacheService) do |instance| + expect(instance).to receive(:fetch).with('missing-branch', anything).and_call_original + end + + expect(Rails.cache).not_to receive(:fetch) + + described_class.protected?(project, 'missing-branch', dry_run: dry_run) + end + end + before do - allow(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original + stub_feature_flags(hash_based_cache_for_protected_branches: feature_flag) + allow(described_class).to receive(:matching).and_call_original # the original call works and warms the cache - described_class.protected?(project, protected_branch.name) + described_class.protected?(project, protected_branch.name, dry_run: dry_run) end - it 'correctly invalidates a cache' do - expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original + context 'Dry-run: true' do + it 'recalculates a fresh value every time in order to check the cache is not returning stale data' do + expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).twice - create(:protected_branch, project: project, name: "bar") - # the cache is invalidated because the project has been "updated" - expect(described_class.protected?(project, protected_branch.name)).to eq(true) + 2.times { described_class.protected?(project, protected_branch.name) } + end + + it_behaves_like 'hash based cache implementation' end - it 'correctly uses the cached version' do - expect(described_class).not_to receive(:matching) - expect(described_class.protected?(project, protected_branch.name)).to eq(true) + context 'Dry-run: false' do + let(:dry_run) { false } + + it 'correctly invalidates a cache' do + expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).exactly(3).times.and_call_original + + create_params = { name: 'bar', merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }] } + branch = ProtectedBranches::CreateService.new(project, project.owner, create_params).execute + expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true) + + ProtectedBranches::UpdateService.new(project, project.owner, name: 'ber').execute(branch) + expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true) + + ProtectedBranches::DestroyService.new(project, project.owner).execute(branch) + expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true) + end + + it_behaves_like 'hash based cache implementation' + + context 'when project is updated' do + it 'does not invalidate a cache' do + expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything) + + project.touch + + described_class.protected?(project, protected_branch.name, dry_run: dry_run) + end + end + + context 'when other project protected branch is updated' do + it 'does not invalidate the current project cache' do + expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything) + + another_project = create(:project) + ProtectedBranches::CreateService.new(another_project, another_project.owner, name: 'bar').execute + + described_class.protected?(project, protected_branch.name, dry_run: dry_run) + end + end + + it 'correctly uses the cached version' do + expect(described_class).not_to receive(:matching) + + expect(described_class.protected?(project, protected_branch.name, dry_run: dry_run)).to eq(true) + end end - it 'sets expires_in for a cache key' do - cache_key = described_class.protected_ref_cache_key(project, protected_branch.name) + context 'when feature flag hash_based_cache_for_protected_branches is off' do + let(:feature_flag) { false } - expect(Rails.cache).to receive(:fetch).with(cache_key, expires_in: 1.hour) + it 'does not call hash based cache implementation' do + expect(ProtectedBranches::CacheService).not_to receive(:new) + expect(Rails.cache).to receive(:fetch).and_call_original - described_class.protected?(project, protected_branch.name) + described_class.protected?(project, 'missing-branch') + end + + it 'correctly invalidates a cache' do + expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original + + create(:protected_branch, project: project, name: "bar") + # the cache is invalidated because the project has been "updated" + expect(described_class.protected?(project, protected_branch.name)).to eq(true) + end + + it 'sets expires_in of 1 hour for the Rails cache key' do + cache_key = described_class.protected_ref_cache_key(project, protected_branch.name) + + expect(Rails.cache).to receive(:fetch).with(cache_key, expires_in: 1.hour) + + described_class.protected?(project, protected_branch.name) + end + + context 'when project is updated' do + it 'invalidates Rails cache' do + expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original + + project.touch + + described_class.protected?(project, protected_branch.name) + end + end + + it 'correctly uses the cached version' do + expect(described_class).not_to receive(:matching) + expect(described_class.protected?(project, protected_branch.name)).to eq(true) + end end end end diff --git a/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb deleted file mode 100644 index 9c751913827..00000000000 --- a/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb +++ /dev/null @@ -1,79 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Request attention' do - include GraphqlHelpers - - let_it_be(:current_user) { create(:user) } - let_it_be(:user) { create(:user) } - let_it_be(:merge_request) { create(:merge_request, reviewers: [user]) } - let_it_be(:project) { merge_request.project } - - let(:input) { { user_id: global_id_of(user) } } - - let(:mutation) do - variables = { - project_path: project.full_path, - iid: merge_request.iid.to_s - } - graphql_mutation(:merge_request_request_attention, variables.merge(input), - <<-QL.strip_heredoc - clientMutationId - errors - QL - ) - end - - def mutation_response - graphql_mutation_response(:merge_request_request_attention) - end - - def mutation_errors - mutation_response['errors'] - end - - before_all do - project.add_developer(current_user) - project.add_developer(user) - end - - it 'is successful' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).to be_empty - end - - context 'when current user is not allowed to update the merge request' do - it 'returns an error' do - post_graphql_mutation(mutation, current_user: create(:user)) - - expect(graphql_errors).not_to be_empty - end - end - - context 'when user is not a reviewer' do - let(:input) { { user_id: global_id_of(create(:user)) } } - - it 'returns an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).not_to be_empty - end - end - - context 'feature flag is disabled' do - before do - stub_feature_flags(mr_attention_requests: false) - end - - it 'returns an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(graphql_errors[0]["message"]).to eq "Feature disabled" - end - end -end diff --git a/spec/requests/api/graphql/mutations/merge_requests/update_reviewer_state_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/update_reviewer_state_spec.rb deleted file mode 100644 index cf497cb2579..00000000000 --- a/spec/requests/api/graphql/mutations/merge_requests/update_reviewer_state_spec.rb +++ /dev/null @@ -1,65 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Toggle attention requested for reviewer' do - include GraphqlHelpers - - let(:current_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [user]) } - let(:project) { merge_request.project } - let(:user) { create(:user) } - let(:input) { { user_id: global_id_of(user) } } - - let(:mutation) do - variables = { - project_path: project.full_path, - iid: merge_request.iid.to_s - } - graphql_mutation(:merge_request_toggle_attention_requested, variables.merge(input), - <<-QL.strip_heredoc - clientMutationId - errors - QL - ) - end - - def mutation_response - graphql_mutation_response(:merge_request_toggle_attention_requested) - end - - def mutation_errors - mutation_response['errors'] - end - - before do - project.add_developer(current_user) - project.add_developer(user) - end - - it 'returns an error if the user is not allowed to update the merge request' do - post_graphql_mutation(mutation, current_user: create(:user)) - - expect(graphql_errors).not_to be_empty - end - - describe 'reviewer does not exist' do - let(:input) { { user_id: global_id_of(create(:user)) } } - - it 'returns an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).not_to be_empty - end - end - - describe 'reviewer exists' do - it 'does not return an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).to be_empty - end - end -end diff --git a/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb b/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb deleted file mode 100644 index 053559b039d..00000000000 --- a/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb +++ /dev/null @@ -1,79 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe 'Remove attention request' do - include GraphqlHelpers - - let_it_be(:current_user) { create(:user) } - let_it_be(:user) { create(:user) } - let_it_be(:merge_request) { create(:merge_request, reviewers: [user]) } - let_it_be(:project) { merge_request.project } - - let(:input) { { user_id: global_id_of(user) } } - - let(:mutation) do - variables = { - project_path: project.full_path, - iid: merge_request.iid.to_s - } - graphql_mutation(:merge_request_remove_attention_request, variables.merge(input), - <<-QL.strip_heredoc - clientMutationId - errors - QL - ) - end - - def mutation_response - graphql_mutation_response(:merge_request_remove_attention_request) - end - - def mutation_errors - mutation_response['errors'] - end - - before_all do - project.add_developer(current_user) - project.add_developer(user) - end - - it 'is successful' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).to be_empty - end - - context 'when current user is not allowed to update the merge request' do - it 'returns an error' do - post_graphql_mutation(mutation, current_user: create(:user)) - - expect(graphql_errors).not_to be_empty - end - end - - context 'when user is not a reviewer' do - let(:input) { { user_id: global_id_of(create(:user)) } } - - it 'returns an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_errors).not_to be_empty - end - end - - context 'feature flag is disabled' do - before do - stub_feature_flags(mr_attention_requests: false) - end - - it 'returns an error' do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(graphql_errors[0]["message"]).to eq "Feature disabled" - end - end -end diff --git a/spec/requests/api/user_counts_spec.rb b/spec/requests/api/user_counts_spec.rb index 2d4705920cf..ab2aa87d1b7 100644 --- a/spec/requests/api/user_counts_spec.rb +++ b/spec/requests/api/user_counts_spec.rb @@ -43,21 +43,6 @@ RSpec.describe API::UserCounts do expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_a Hash expect(json_response['merge_requests']).to eq(2) - expect(json_response['attention_requests']).to eq(0) - end - - describe 'mr_attention_requests is disabled' do - before do - stub_feature_flags(mr_attention_requests: false) - end - - it 'does not include attention_requests count' do - create(:merge_request, source_project: project, author: user, assignees: [user]) - - get api('/user_counts', user) - - expect(json_response.key?('attention_requests')).to be(false) - end end end diff --git a/spec/serializers/merge_request_user_entity_spec.rb b/spec/serializers/merge_request_user_entity_spec.rb index 7877356ff0f..5c7120ab6a4 100644 --- a/spec/serializers/merge_request_user_entity_spec.rb +++ b/spec/serializers/merge_request_user_entity_spec.rb @@ -18,8 +18,7 @@ RSpec.describe MergeRequestUserEntity do it 'exposes needed attributes' do is_expected.to include( :id, :name, :username, :state, :avatar_url, :web_url, - :can_merge, :can_update_merge_request, :reviewed, :approved, - :attention_requested + :can_merge, :can_update_merge_request, :reviewed, :approved ) end @@ -57,14 +56,6 @@ RSpec.describe MergeRequestUserEntity do end end - context 'attention_requested' do - before do - merge_request.find_assignee(user).update!(state: :attention_requested) - end - - it { is_expected.to include(attention_requested: true ) } - end - describe 'performance' do let_it_be(:user_a) { create(:user) } let_it_be(:user_b) { create(:user) } diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index 1426ef2a1f6..0d2b8a4ac3c 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -8,6 +8,37 @@ RSpec.describe Issuable::CommonSystemNotesService do let(:issuable) { create(:issue, project: project) } + shared_examples 'system note for issuable date changes' do + it 'creates a system note for due_date set' do + issuable.update!(due_date: Date.today) + + expect { subject }.to change(issuable.notes, :count).from(0).to(1) + expect(issuable.notes.last.note).to match('changed due date to') + end + + it 'creates a system note for start_date set' do + issuable.update!(start_date: Date.today) + + expect { subject }.to change(issuable.notes, :count).from(0).to(1) + expect(issuable.notes.last.note).to match('changed start date to') + end + + it 'creates a note when both start and due date are changed' do + issuable.update!(start_date: Date.today, due_date: 1.week.from_now) + + expect { subject }.to change { issuable.notes.count }.from(0).to(1) + expect(issuable.notes.last.note).to match(/changed start date to.+and changed due date to/) + end + + it 'does not call SystemNoteService if no dates are changed' do + issuable.update!(title: 'new title') + + expect(SystemNoteService).not_to receive(:change_start_date_or_due_date) + + subject + end + end + context 'on issuable update' do it_behaves_like 'system note creation', { title: 'New title' }, 'changed title' it_behaves_like 'system note creation', { description: 'New description' }, 'changed the description' @@ -61,6 +92,12 @@ RSpec.describe Issuable::CommonSystemNotesService do end end end + + context 'when changing dates' do + it_behaves_like 'system note for issuable date changes' do + subject { described_class.new(project: project, current_user: user).execute(issuable) } + end + end end context 'on issuable create' do @@ -102,12 +139,8 @@ RSpec.describe Issuable::CommonSystemNotesService do end end - it 'creates a system note for due_date set' do - issuable.due_date = Date.today - issuable.save! - - expect { subject }.to change { issuable.notes.count }.from(0).to(1) - expect(issuable.notes.last.note).to match('changed due date') + context 'when changing dates' do + it_behaves_like 'system note for issuable date changes' end end end diff --git a/spec/services/issues/clone_service_spec.rb b/spec/services/issues/clone_service_spec.rb index cabd0d6732c..435488b7f66 100644 --- a/spec/services/issues/clone_service_spec.rb +++ b/spec/services/issues/clone_service_spec.rb @@ -182,19 +182,21 @@ RSpec.describe Issues::CloneService do context 'issue with due date' do let(:date) { Date.parse('2020-01-10') } + let(:new_date) { date + 1.week } let(:old_issue) do create(:issue, title: title, description: description, project: old_project, author: author, due_date: date) end before do - SystemNoteService.change_due_date(old_issue, old_project, author, old_issue.due_date) + old_issue.update!(due_date: new_date) + SystemNoteService.change_start_date_or_due_date(old_issue, old_project, author, old_issue.previous_changes.slice('due_date')) end it 'keeps the same due date' do new_issue = clone_service.execute(old_issue, new_project) - expect(new_issue.due_date).to eq(date) + expect(new_issue.due_date).to eq(old_issue.due_date) end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index c130863f928..863df810d01 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -140,7 +140,8 @@ RSpec.describe Issues::MoveService do end before do - SystemNoteService.change_due_date(old_issue, old_project, author, old_issue.due_date) + old_issue.update!(due_date: Date.today) + SystemNoteService.change_start_date_or_due_date(old_issue, old_project, author, old_issue.previous_changes.slice('due_date')) end it 'does not create extra system notes' do diff --git a/spec/services/protected_branches/cache_service_spec.rb b/spec/services/protected_branches/cache_service_spec.rb index 4c05d6dcbd9..4fa7553c23d 100644 --- a/spec/services/protected_branches/cache_service_spec.rb +++ b/spec/services/protected_branches/cache_service_spec.rb @@ -63,6 +63,38 @@ RSpec.describe ProtectedBranches::CacheService, :clean_gitlab_redis_cache do expect(service.fetch('not-found') { true }).to eq(true) end end + + context 'when dry_run is on' do + it 'does not use cached value' do + expect(service.fetch('main', dry_run: true) { true }).to eq(true) + expect(service.fetch('main', dry_run: true) { false }).to eq(false) + end + + context 'when cache mismatch' do + it 'logs an error' do + expect(service.fetch('main', dry_run: true) { true }).to eq(true) + + expect(Gitlab::AppLogger).to receive(:error).with( + 'class' => described_class.name, + 'message' => /Cache mismatch/, + 'project_id' => project.id, + 'project_path' => project.full_path + ) + + expect(service.fetch('main', dry_run: true) { false }).to eq(false) + end + end + + context 'when cache matches' do + it 'does not log an error' do + expect(service.fetch('main', dry_run: true) { true }).to eq(true) + + expect(Gitlab::AppLogger).not_to receive(:error) + + expect(service.fetch('main', dry_run: true) { true }).to eq(true) + end + end + end end describe '#refresh' do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index ae36871fcde..2d38d968ce4 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -810,38 +810,6 @@ RSpec.describe QuickActions::InterpretService do end end - shared_examples 'attention command' do - it 'updates reviewers attention status' do - _, _, message = service.execute(content, issuable) - - expect(message).to eq("Requested attention from #{developer.to_reference}.") - - reviewer.reload - - expect(reviewer).to be_attention_requested - end - - it 'supports attn alias' do - attn_cmd = content.gsub(/attention/, 'attn') - _, _, message = service.execute(attn_cmd, issuable) - - expect(message).to eq("Requested attention from #{developer.to_reference}.") - - reviewer.reload - - expect(reviewer).to be_attention_requested - end - end - - shared_examples 'remove attention command' do - it 'updates reviewers attention status' do - _, _, message = service.execute(content, issuable) - - expect(message).to eq("Removed attention from #{developer.to_reference}.") - expect(reviewer).not_to be_attention_requested - end - end - it_behaves_like 'reopen command' do let(:content) { '/reopen' } let(:issuable) { issue } @@ -2481,82 +2449,6 @@ RSpec.describe QuickActions::InterpretService do expect(message).to eq('One or more contacts were successfully removed.') end end - - describe 'attention command' do - let(:issuable) { create(:merge_request, reviewers: [developer], source_project: project) } - let(:reviewer) { issuable.merge_request_reviewers.find_by(user_id: developer.id) } - let(:content) { "/attention @#{developer.username}" } - - context 'with one user' do - before do - reviewer.update!(state: :reviewed) - end - - it_behaves_like 'attention command' - end - - context 'with no user' do - let(:content) { "/attention" } - - it_behaves_like 'failed command', 'Failed to request attention because no user was found.' - end - - context 'with incorrect permissions' do - let(:service) { described_class.new(project, create(:user)) } - - it_behaves_like 'failed command', 'Could not apply attention command.' - end - - context 'with feature flag disabled' do - before do - stub_feature_flags(mr_attention_requests: false) - end - - it_behaves_like 'failed command', 'Could not apply attention command.' - end - - context 'with an issue instead of a merge request' do - let(:issuable) { issue } - - it_behaves_like 'failed command', 'Could not apply attention command.' - end - end - - describe 'remove attention command' do - let(:issuable) { create(:merge_request, reviewers: [developer], source_project: project) } - let(:reviewer) { issuable.merge_request_reviewers.find_by(user_id: developer.id) } - let(:content) { "/remove_attention @#{developer.username}" } - - context 'with one user' do - it_behaves_like 'remove attention command' - end - - context 'with no user' do - let(:content) { "/remove_attention" } - - it_behaves_like 'failed command', 'Failed to remove attention because no user was found.' - end - - context 'with incorrect permissions' do - let(:service) { described_class.new(project, create(:user)) } - - it_behaves_like 'failed command', 'Could not apply remove_attention command.' - end - - context 'with feature flag disabled' do - before do - stub_feature_flags(mr_attention_requests: false) - end - - it_behaves_like 'failed command', 'Could not apply remove_attention command.' - end - - context 'with an issue instead of a merge request' do - let(:issuable) { issue } - - it_behaves_like 'failed command', 'Could not apply remove_attention command.' - end - end end describe '#explain' do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 81c48fb6e15..c7c5aeae670 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -134,15 +134,15 @@ RSpec.describe SystemNoteService do end end - describe '.change_due_date' do - let(:due_date) { double } + describe '.change_start_date_or_due_date' do + let(:changed_dates) { double } it 'calls TimeTrackingService' do expect_next_instance_of(::SystemNotes::TimeTrackingService) do |service| - expect(service).to receive(:change_due_date).with(due_date) + expect(service).to receive(:change_start_date_or_due_date).with(changed_dates) end - described_class.change_due_date(noteable, project, author, due_date) + described_class.change_start_date_or_due_date(noteable, project, author, changed_dates) end end diff --git a/spec/services/system_notes/time_tracking_service_spec.rb b/spec/services/system_notes/time_tracking_service_spec.rb index be86e8cf388..ef07518c707 100644 --- a/spec/services/system_notes/time_tracking_service_spec.rb +++ b/spec/services/system_notes/time_tracking_service_spec.rb @@ -6,38 +6,94 @@ RSpec.describe ::SystemNotes::TimeTrackingService do let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } - describe '#change_due_date' do - subject { described_class.new(noteable: noteable, project: project, author: author).change_due_date(due_date) } + describe '#change_start_date_or_due_date' do + subject(:note) { described_class.new(noteable: noteable, project: project, author: author).change_start_date_or_due_date(changed_dates) } - let(:due_date) { Date.today } + let(:start_date) { Date.today } + let(:due_date) { 1.week.from_now.to_date } + let(:changed_dates) { { 'due_date' => [nil, due_date], 'start_date' => [nil, start_date] } } + + let_it_be(:noteable) { create(:issue, project: project) } context 'when noteable is an issue' do - let_it_be(:noteable) { create(:issue, project: project) } - it_behaves_like 'a note with overridable created_at' it_behaves_like 'a system note' do - let(:action) { 'due_date' } + let(:action) { 'start_date_or_due_date' } end - context 'when due date added' do - it 'sets the note text' do - expect(subject.note).to eq "changed due date to #{due_date.to_s(:long)}" + context 'when both dates are added' do + it 'sets the correct note message' do + expect(note.note).to eq("changed start date to #{start_date.to_s(:long)} and changed due date to #{due_date.to_s(:long)}") end end - context 'when due date removed' do - let(:due_date) { nil } + context 'when both dates are removed' do + let(:changed_dates) { { 'due_date' => [due_date, nil], 'start_date' => [start_date, nil] } } - it 'sets the note text' do - expect(subject.note).to eq 'removed due date' + before do + noteable.update!(start_date: start_date, due_date: due_date) + end + + it 'sets the correct note message' do + expect(note.note).to eq('removed start date and removed due date') end end - it 'tracks the issue event in usage ping' do - expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_due_date_changed_action).with(author: author) + context 'when due date is added' do + let(:changed_dates) { { 'due_date' => [nil, due_date] } } - subject + it 'tracks the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_due_date_changed_action).with(author: author) + + subject + end + + it 'sets the correct note message' do + expect(note.note).to eq("changed due date to #{due_date.to_s(:long)}") + end + + context 'and start date removed' do + let(:changed_dates) { { 'due_date' => [nil, due_date], 'start_date' => [start_date, nil] } } + + it 'sets the correct note message' do + expect(note.note).to eq("removed start date and changed due date to #{due_date.to_s(:long)}") + end + end + end + + context 'when start_date is added' do + let(:changed_dates) { { 'start_date' => [nil, start_date] } } + + it 'does not track the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_due_date_changed_action) + + subject + end + + it 'sets the correct note message' do + expect(note.note).to eq("changed start date to #{start_date.to_s(:long)}") + end + + context 'and due date removed' do + let(:changed_dates) { { 'due_date' => [due_date, nil], 'start_date' => [nil, start_date] } } + + it 'sets the correct note message' do + expect(note.note).to eq("changed start date to #{start_date.to_s(:long)} and removed due date") + end + end + end + + context 'when no dates are changed' do + let(:changed_dates) { {} } + + it 'does not create a note and returns nil' do + expect do + note + end.to not_change(Note, :count) + + expect(note).to be_nil + end end end @@ -45,7 +101,7 @@ RSpec.describe ::SystemNotes::TimeTrackingService do let_it_be(:noteable) { create(:merge_request, source_project: project) } it 'does not track the issue event in usage ping' do - expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_due_date_changed_action).with(author: author) + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_due_date_changed_action) subject end