From a29707687893beecb0e333a12e6f9e093a77eeb9 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 5 May 2022 21:09:10 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/assets/images/file_icons.svg | 2 +- .../projects/merge_requests_controller.rb | 1 + .../remove_attention_request.rb | 8 ++ .../merge_requests/request_attention.rb | 8 ++ .../toggle_attention_requested.rb | 2 + app/graphql/types/mutation_type.rb | 6 +- app/helpers/merge_requests_helper.rb | 2 +- app/models/alert_management/alert.rb | 4 + .../concerns/merge_request_reviewer_state.rb | 2 - app/models/merge_request.rb | 4 - app/models/merge_request_assignee.rb | 6 -- app/models/merge_request_reviewer.rb | 6 -- app/models/project_setting.rb | 2 +- app/models/user.rb | 4 + app/serializers/merge_request_user_entity.rb | 2 +- .../alert_management/alert_processing.rb | 2 +- app/services/merge_requests/base_service.rb | 41 ++++++++- .../handle_assignees_change_service.rb | 2 - .../update_assignees_service.rb | 2 + ...ndroid_target_platform_detector_service.rb | 35 ++++++++ .../record_target_platforms_service.rb | 10 ++- app/views/layouts/header/_default.html.haml | 4 +- .../projects/merge_requests/show.html.haml | 2 +- .../shared/issuable/_assignees.html.haml | 2 +- .../shared/issuable/_reviewers.html.haml | 2 +- .../shared/issuable/_search_bar.html.haml | 2 +- .../record_target_platforms_worker.rb | 28 +++++- .../development/detect_android_projects.yml | 8 ++ .../15_0/15-0-removal-artifacts-keyword.yml | 15 ++++ doc/.vale/gitlab/SubstitutionWarning.yml | 1 + doc/api/graphql/reference/index.md | 6 -- doc/development/backend/ruby_style_guide.md | 2 +- .../documentation/restful_api_styleguide.md | 2 +- doc/development/documentation/structure.md | 2 +- .../documentation/styleguide/index.md | 2 +- .../documentation/styleguide/word_list.md | 8 ++ doc/development/documentation/versions.md | 12 +-- doc/update/removals.md | 6 ++ lib/api/user_counts.rb | 2 +- lib/gitlab/gon_helper.rb | 2 +- .../quick_actions/merge_request_actions.rb | 4 +- .../dashboard/issuables_counter_spec.rb | 5 ++ .../features/dashboard/merge_requests_spec.rb | 4 +- spec/finders/merge_requests_finder_spec.rb | 3 + ...ser_merge_request_interaction_type_spec.rb | 1 + spec/helpers/merge_requests_helper_spec.rb | 6 +- spec/models/alert_management/alert_spec.rb | 11 +++ spec/models/merge_request_assignee_spec.rb | 13 +-- spec/models/merge_request_reviewer_spec.rb | 13 +-- spec/models/merge_request_spec.rb | 2 + spec/models/user_spec.rb | 29 +++++- .../users/merge_request_interaction_spec.rb | 1 + .../merge_requests/request_attention_spec.rb | 13 +++ .../remove_attention_request_spec.rb | 13 +++ .../api/graphql/project/merge_request_spec.rb | 4 +- spec/requests/api/user_counts_spec.rb | 2 +- .../merge_request_user_entity_spec.rb | 4 + .../handle_assignees_change_service_spec.rb | 6 -- .../update_assignees_service_spec.rb | 43 +++++++++ .../merge_requests/update_service_spec.rb | 90 +++++++++++++++++++ ...d_target_platform_detector_service_spec.rb | 30 +++++++ .../record_target_platforms_service_spec.rb | 74 +++++++++++---- .../models/reviewer_state_shared_examples.rb | 15 ---- .../record_target_platforms_worker_spec.rb | 81 ++++++++++++++--- 64 files changed, 578 insertions(+), 148 deletions(-) create mode 100644 app/services/projects/android_target_platform_detector_service.rb create mode 100644 config/feature_flags/development/detect_android_projects.yml create mode 100644 data/removals/15_0/15-0-removal-artifacts-keyword.yml create mode 100644 spec/services/projects/android_target_platform_detector_service_spec.rb delete mode 100644 spec/support/shared_examples/models/reviewer_state_shared_examples.rb diff --git a/app/assets/images/file_icons.svg b/app/assets/images/file_icons.svg index def87dd9163..8b19f411c7b 100644 --- a/app/assets/images/file_icons.svg +++ b/app/assets/images/file_icons.svg @@ -1 +1 @@ -api-blueprintLayer 1Browserslist logoBrowserslist logoCfcucumber-mark-transparent-pipsNVIDIA-LogoDartGroup 3Group 3Asset 3logoklLayer 1MMocha Logonodemonnpostcss-logo-symbolprettier-icon-darkGroupGroup 2stylelint-icon-whitestylelint-icon-blackTEXTShoudinibadgeBrandVisualStudioCodewolframLanguage +api-blueprintLayer 1Browserslist logoBrowserslist logoCfcucumber-mark-transparent-pipsNVIDIA-LogoDartGroup 3Group 3Asset 3klLayer 1MMocha Logonodemonnpostcss-logo-symbolprettier-icon-darkGroupGroup 2stylelint-icon-whitestylelint-icon-blackTEXTShoudinibadgeBrandVisualStudioCodewolframLanguage diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f93b39dbfd3..6e46135dd8f 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -44,6 +44,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:issue_assignees_widget, @project, default_enabled: :yaml) push_frontend_feature_flag(:realtime_labels, project, default_enabled: :yaml) push_frontend_feature_flag(:updated_diff_expansion_buttons, project, default_enabled: :yaml) + push_frontend_feature_flag(:mr_attention_requests, current_user, default_enabled: :yaml) end before_action do diff --git a/app/graphql/mutations/merge_requests/remove_attention_request.rb b/app/graphql/mutations/merge_requests/remove_attention_request.rb index ab8617edb36..3b12b09528b 100644 --- a/app/graphql/mutations/merge_requests/remove_attention_request.rb +++ b/app/graphql/mutations/merge_requests/remove_attention_request.rb @@ -13,6 +13,8 @@ module Mutations 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( @@ -27,6 +29,12 @@ module Mutations 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 index 9a9f2f4cd43..5f5565285f6 100644 --- a/app/graphql/mutations/merge_requests/request_attention.rb +++ b/app/graphql/mutations/merge_requests/request_attention.rb @@ -13,6 +13,8 @@ module Mutations 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( @@ -27,6 +29,12 @@ module Mutations 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 index f316f23fb85..8913ca48531 100644 --- a/app/graphql/mutations/merge_requests/toggle_attention_requested.rb +++ b/app/graphql/mutations/merge_requests/toggle_attention_requested.rb @@ -13,6 +13,8 @@ module Mutations 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 diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 5c46145c846..c9f6a4648dd 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -69,9 +69,9 @@ 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, feature_flag: :mr_attention_requests - mount_mutation Mutations::MergeRequests::RemoveAttentionRequest, feature_flag: :mr_attention_requests - mount_mutation Mutations::MergeRequests::ToggleAttentionRequested, feature_flag: :mr_attention_requests + 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/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index ba09735ef7c..576b85fdeb1 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -156,7 +156,7 @@ module MergeRequestsHelper total: total_count } - if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml) + if current_user&.mr_attention_requests_enabled? attention_requested_count = attention_requested_merge_requests_count counts[:attention_requested_count] = attention_requested_count diff --git a/app/models/alert_management/alert.rb b/app/models/alert_management/alert.rb index ba23d52ca81..e62bf2ba4ea 100644 --- a/app/models/alert_management/alert.rb +++ b/app/models/alert_management/alert.rb @@ -119,6 +119,10 @@ module AlertManagement end end + def self.find_ongoing_alert(project, fingerprint) + for_fingerprint(project, fingerprint).not_resolved.take + end + def self.last_prometheus_alert_by_project_id ids = select(arel_table[:id].maximum).group(:project_id) with_prometheus_alert.where(id: ids) diff --git a/app/models/concerns/merge_request_reviewer_state.rb b/app/models/concerns/merge_request_reviewer_state.rb index 893d06b4da8..18ec1c253e1 100644 --- a/app/models/concerns/merge_request_reviewer_state.rb +++ b/app/models/concerns/merge_request_reviewer_state.rb @@ -16,8 +16,6 @@ module MergeRequestReviewerState belongs_to :updated_state_by, class_name: 'User', foreign_key: :updated_state_by_user_id - after_initialize :set_state, unless: :persisted? - def attention_requested_by return unless attention_requested? diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4c6ed399bf9..74095fad5e4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1977,10 +1977,6 @@ class MergeRequest < ApplicationRecord end end - def attention_requested_enabled? - Feature.enabled?(:mr_attention_requests, project, default_enabled: :yaml) - end - private attr_accessor :skip_fetch_ref diff --git a/app/models/merge_request_assignee.rb b/app/models/merge_request_assignee.rb index 77b46fa50f4..fd8e5860040 100644 --- a/app/models/merge_request_assignee.rb +++ b/app/models/merge_request_assignee.rb @@ -10,12 +10,6 @@ class MergeRequestAssignee < ApplicationRecord scope :in_projects, ->(project_ids) { joins(:merge_request).where(merge_requests: { target_project_id: project_ids }) } - def set_state - if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml) - self.state = MergeRequestReviewer.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested - end - end - def cache_key [model_name.cache_key, id, state, assignee.cache_key] end diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb index 8c75fb2e4e6..4abf0fa09f0 100644 --- a/app/models/merge_request_reviewer.rb +++ b/app/models/merge_request_reviewer.rb @@ -6,12 +6,6 @@ class MergeRequestReviewer < ApplicationRecord belongs_to :merge_request belongs_to :reviewer, class_name: 'User', foreign_key: :user_id, inverse_of: :merge_request_reviewers - def set_state - if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml) - self.state = MergeRequestAssignee.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested - end - end - def cache_key [model_name.cache_key, id, state, reviewer.cache_key] end diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index e5179b62fbb..c6f9147cad7 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ProjectSetting < ApplicationRecord - ALLOWED_TARGET_PLATFORMS = %w(ios osx tvos watchos).freeze + ALLOWED_TARGET_PLATFORMS = %w(ios osx tvos watchos android).freeze belongs_to :project, inverse_of: :project_setting diff --git a/app/models/user.rb b/app/models/user.rb index 26d47de4f00..0408cc2cb5d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2070,6 +2070,10 @@ class User < ApplicationRecord end end + def mr_attention_requests_enabled? + Feature.enabled?(:mr_attention_requests, self, default_enabled: :yaml) + end + protected # override, from Devise::Validatable diff --git a/app/serializers/merge_request_user_entity.rb b/app/serializers/merge_request_user_entity.rb index 97912656bbb..12c573d1a13 100644 --- a/app/serializers/merge_request_user_entity.rb +++ b/app/serializers/merge_request_user_entity.rb @@ -20,7 +20,7 @@ class MergeRequestUserEntity < ::API::Entities::UserBasic find_reviewer_or_assignee(user, options)&.reviewed? end - expose :attention_requested, if: satisfies(:present?, :allows_reviewers?, :attention_requested_enabled?) do |user, options| + 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 diff --git a/app/services/concerns/alert_management/alert_processing.rb b/app/services/concerns/alert_management/alert_processing.rb index 2b556a4339d..ce6985527ba 100644 --- a/app/services/concerns/alert_management/alert_processing.rb +++ b/app/services/concerns/alert_management/alert_processing.rb @@ -104,7 +104,7 @@ module AlertManagement def find_existing_alert return unless incoming_payload.gitlab_fingerprint - AlertManagement::Alert.not_resolved.for_fingerprint(project, incoming_payload.gitlab_fingerprint).first + AlertManagement::Alert.find_ongoing_alert(project, incoming_payload.gitlab_fingerprint) end def build_new_alert diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index e4784d1de79..30ad549db91 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -43,6 +43,8 @@ module MergeRequests end def handle_assignees_change(merge_request, old_assignees) + bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees) + MergeRequests::HandleAssigneesChangeService .new(project: project, current_user: current_user) .async_execute(merge_request, old_assignees) @@ -58,11 +60,10 @@ module MergeRequests new_reviewers = merge_request.reviewers - old_reviewers merge_request_activity_counter.track_users_review_requested(users: new_reviewers) merge_request_activity_counter.track_reviewers_changed_action(user: current_user) + bulk_update_reviewers_state(merge_request, new_reviewers) unless new_reviewers.include?(current_user) remove_attention_requested(merge_request) - - merge_request.merge_request_reviewers_with(new_reviewers).update_all(updated_state_by_user_id: current_user.id) end end @@ -246,7 +247,7 @@ module MergeRequests end def remove_all_attention_requests(merge_request) - return unless merge_request.attention_requested_enabled? + return unless current_user.mr_attention_requests_enabled? users = merge_request.reviewers + merge_request.assignees @@ -254,10 +255,42 @@ module MergeRequests end def remove_attention_requested(merge_request) - return unless merge_request.attention_requested_enabled? + return unless current_user.mr_attention_requests_enabled? ::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: current_user).execute end + + def bulk_update_assignees_state(merge_request, new_assignees) + return unless current_user.mr_attention_requests_enabled? + return if new_assignees.empty? + + assignees_map = merge_request.merge_request_assignees_with(new_assignees).to_h do |assignee| + state = merge_request.find_reviewer(assignee.assignee)&.state || :attention_requested + + [ + assignee, + { state: MergeRequestAssignee.states[state], updated_state_by_user_id: current_user.id } + ] + end + + ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], assignees_map) + end + + def bulk_update_reviewers_state(merge_request, new_reviewers) + return unless current_user.mr_attention_requests_enabled? + return if new_reviewers.empty? + + reviewers_map = merge_request.merge_request_reviewers_with(new_reviewers).to_h do |reviewer| + state = merge_request.find_assignee(reviewer.reviewer)&.state || :attention_requested + + [ + reviewer, + { state: MergeRequestReviewer.states[state], updated_state_by_user_id: current_user.id } + ] + end + + ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], reviewers_map) + end end end diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb index a169a6dc0b6..78c93d10f2a 100644 --- a/app/services/merge_requests/handle_assignees_change_service.rb +++ b/app/services/merge_requests/handle_assignees_change_service.rb @@ -21,8 +21,6 @@ module MergeRequests merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees) merge_request_activity_counter.track_assignees_changed_action(user: current_user) - merge_request.merge_request_assignees_with(new_assignees).update_all(updated_state_by_user_id: current_user.id) - execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks] unless new_assignees.include?(current_user) diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb index d52c1bbbcda..5b23f69ac4a 100644 --- a/app/services/merge_requests/update_assignees_service.rb +++ b/app/services/merge_requests/update_assignees_service.rb @@ -20,6 +20,8 @@ module MergeRequests attrs = update_attrs.merge(assignee_ids: new_ids) merge_request.update!(**attrs) + bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees) + # Defer the more expensive operations (handle_assignee_changes) to the background MergeRequests::HandleAssigneesChangeService .new(project: project, current_user: current_user) diff --git a/app/services/projects/android_target_platform_detector_service.rb b/app/services/projects/android_target_platform_detector_service.rb new file mode 100644 index 00000000000..11635ad18d5 --- /dev/null +++ b/app/services/projects/android_target_platform_detector_service.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Projects + # Service class to detect if a project is made to run on the Android platform. + # + # This service searches for an AndroidManifest.xml file which all Android app + # project must have. It returns the symbol :android if the given project is an + # Android app project. + # + # Ref: https://developer.android.com/guide/topics/manifest/manifest-intro + # + # Example usage: + # > AndroidTargetPlatformDetectorService.new(a_project).execute + # => nil + # > AndroidTargetPlatformDetectorService.new(an_android_project).execute + # => :android + class AndroidTargetPlatformDetectorService < BaseService + # element is required and must occur once inside AndroidManifest.xml + MANIFEST_FILE_SEARCH_QUERY = '