diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-21 12:12:21 +0300 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-21 12:12:21 +0300 |
| commit | 63a014fe28a4d5cbcc4ac2e89abcc6ab40c0df12 (patch) | |
| tree | f3088c179faaca35048ecdb10c0c73bd43a9c3ff | |
| parent | c84d80849d0ce96d4464a8d60e519e4bfe044185 (diff) | |
Add latest changes from gitlab-org/gitlab@master
97 files changed, 889 insertions, 102 deletions
diff --git a/.gitlab/ci/docs.gitlab-ci.yml b/.gitlab/ci/docs.gitlab-ci.yml index c585047f916..f4d8698f22d 100644 --- a/.gitlab/ci/docs.gitlab-ci.yml +++ b/.gitlab/ci/docs.gitlab-ci.yml @@ -75,17 +75,3 @@ ui-docs-links lint: needs: [] script: - bundle exec haml-lint -i DocumentationLinks - -deprecations-doc check: - variables: - SETUP_DB: "false" - extends: - - .default-retry - - .rails-cache - - .default-before_script - - .docs:rules:deprecations - stage: test - needs: [] - script: - - bundle exec rake gitlab:docs:check_deprecations - allow_failure: true diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index a9fd63c75cb..36a11951d3f 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -150,13 +150,6 @@ - ".markdownlint.yml" - "scripts/lint-doc.sh" -.docs-deprecations-patterns: &docs-deprecations-patterns - - "doc/deprecations/index.md" - - "data/deprecations/*.yml" - - "data/deprecations/templates/_deprecation_template.md.erb" - - "lib/tasks/gitlab/docs/compile_deprecations.rake" - - "tooling/deprecations/docs.rb" - .bundler-patterns: &bundler-patterns - '{Gemfile.lock,*/Gemfile.lock,*/*/Gemfile.lock}' @@ -470,12 +463,6 @@ changes: *docs-patterns when: on_success -.docs:rules:deprecations: - rules: - - <<: *if-default-refs - changes: *docs-deprecations-patterns - when: on_success - ################## # GraphQL rules # ################## diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 2ec6b8c1965..bf2b03d0ec7 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -ccd8ea3e1436d6464ac5f67e6bebd1ae317f4d50 +e6374eaf67addb939516b5ad5d242418a6f2b688 diff --git a/app/assets/javascripts/packages_and_registries/package_registry/constants.js b/app/assets/javascripts/packages_and_registries/package_registry/constants.js index f023b4481a0..5f0ff89000d 100644 --- a/app/assets/javascripts/packages_and_registries/package_registry/constants.js +++ b/app/assets/javascripts/packages_and_registries/package_registry/constants.js @@ -1,5 +1,4 @@ -/* eslint-disable @gitlab/require-string-literal-i18n-helpers */ -import { __, s__ } from '~/locale'; +import { s__ } from '~/locale'; export const PACKAGE_TYPE_CONAN = 'CONAN'; export const PACKAGE_TYPE_MAVEN = 'MAVEN'; @@ -71,7 +70,7 @@ export const DELETE_PACKAGE_ERROR_MESSAGE = s__( 'PackageRegistry|Something went wrong while deleting the package.', ); export const DELETE_PACKAGE_FILE_ERROR_MESSAGE = s__( - __('PackageRegistry|Something went wrong while deleting the package file.'), + 'PackageRegistry|Something went wrong while deleting the package file.', ); export const DELETE_PACKAGE_FILE_SUCCESS_MESSAGE = s__( 'PackageRegistry|Package file deleted successfully', diff --git a/app/presenters/README.md b/app/presenters/README.md index 62aec4fc8a2..dfd1818f97d 100644 --- a/app/presenters/README.md +++ b/app/presenters/README.md @@ -66,14 +66,15 @@ we gain the following benefits: ### Presenter definition -Every presenter should inherit from `Gitlab::View::Presenter::Simple`, which -provides a `.presents` the method which allows you to define an accessor for the +If you need a presenter class that has only necessary interfaces for the view-related context, +inherit from `Gitlab::View::Presenter::Simple`. +It provides a `.presents` the method which allows you to define an accessor for the presented object. It also includes common helpers like `Gitlab::Routing` and `Gitlab::Allowable`. ```ruby class LabelPresenter < Gitlab::View::Presenter::Simple - presents :label + presents ::Label, as: :label def text_color label.color.to_s @@ -85,13 +86,14 @@ class LabelPresenter < Gitlab::View::Presenter::Simple end ``` -In some cases, it can be more practical to transparently delegate all missing -method calls to the presented object, in these cases, you can make your -presenter inherit from `Gitlab::View::Presenter::Delegated`: +If you need a presenter class that delegates missing method calls to the presented object, +inherit from `Gitlab::View::Presenter::Delegated`. +This is more like an "extension" in the sense that the produced object is going to have +all of interfaces of the presented object **AND** all of the interfaces in the presenter class: ```ruby class LabelPresenter < Gitlab::View::Presenter::Delegated - presents :label + presents ::Label, as: :label def text_color # color is delegated to label @@ -152,3 +154,82 @@ You can also present the model in the view: %div{ class: label.text_color } = render partial: label, label: label ``` + +### Validate accidental overrides + +We use presenters in many places, such as Controller, Haml, GraphQL/Rest API, +it's very handy to extend the core/backend logic of Active Record models, +however, there is a risk that it accidentally overrides important logic. + +For example, [this production incident](https://gitlab.com/gitlab-com/gl-infra/production/-/issues/5498) +was caused by [including `ActionView::Helpers::UrlHelper` in a presenter](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69537/diffs#4b581cff00ef3cc9780efd23682af383de302e7d_3_3). +The `tag` accesor in `Ci::Build` was accidentally overridden by `ActionView::Helpers::TagHelper#tag`, +and as a conseuqence, a wrong `tag` value was persited into database. + +Starting from GitLab 14.4, we validate the presenters (specifically all of the subclasses of `Gitlab::View::Presenter::Delegated`) +that they do not accidentally override core/backend logic. In such case, a pipeline in merge requests fails with an error message, +here is an example: + +```plaintext +We've detected that a presetner is overriding a specific method(s) on a subject model. +There is a risk that it accidentally modifies the backend/core logic that leads to production incident. +Please follow https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides +to resolve this error with caution. + +Here are the conflict details. + +- Ci::PipelinePresenter#tag is overriding Ci::Pipeline#tag. delegator_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/actionview-6.1.3.2/lib/action_view/helpers/tag_helper.rb:271 original_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/activemodel-6.1.3.2/lib/active_model/attribute_methods.rb:254 +``` + +Here are the potential solutions: + +- If the conflict happens on an instance method in the presenter: + - If you intend to override the core/backend logic, define `delegator_override <method-name>` on top of the conflicted method. + This explicitly adds the method to an allowlist. + - If you do NOT intend to override the core/backend logic, rename the method name in the presenter. +- If the conflict happens on an included module in the presenter, remove the module from the presenter and find a workaround. + +### How to use the `Gitlab::Utils::DelegatorOverride` validator + +If a presenter class inhertis from `Gitlab::View::Presenter::Delegated`, +you should define what object class is presented: + +```ruby +class WebHookLogPresenter < Gitlab::View::Presenter::Delegated + presents ::WebHookLog, as: :web_hook_log # This defines that the presenter presents `WebHookLog` Active Record model. +``` + +These presenters are validated not to accidentaly override the methods in the presented object. +You can run the validation locally with: + +```shell +bundle exec rake lint:static_verification +``` + +To add a method to an allowlist, use `delegator_override`. For example: + +```ruby +class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated + presents ::Vulnerability, as: :vulnerability + + delegator_override :description # This adds the `description` method to an allowlist that the override is intentional. + def description + vulnerability.description || finding.description + end +``` + +To add methods of a module to an allowlist, use `delegator_override_with`. For example: + +```ruby +module Ci + class PipelinePresenter < Gitlab::View::Presenter::Delegated + include Gitlab::Utils::StrongMemoize + include ActionView::Helpers::UrlHelper + + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag` +``` + +Keep in mind that if you use `delegator_override_with`, +there is a high chance that you're doing **something wrong**. +Read the [Validate Accidental Overrides](#validate-accidental-overrides) for more information. diff --git a/app/presenters/alert_management/alert_presenter.rb b/app/presenters/alert_management/alert_presenter.rb index 93983c989c8..86fe9859271 100644 --- a/app/presenters/alert_management/alert_presenter.rb +++ b/app/presenters/alert_management/alert_presenter.rb @@ -5,6 +5,9 @@ module AlertManagement include IncidentManagement::Settings include ActionView::Helpers::UrlHelper + presents ::AlertManagement::Alert + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + MARKDOWN_LINE_BREAK = " \n" HORIZONTAL_LINE = "\n\n---\n\n" @@ -29,6 +32,7 @@ module AlertManagement started_at&.strftime('%d %B %Y, %-l:%M%p (%Z)') end + delegator_override :details_url def details_url details_project_alert_management_url(project, alert.iid) end diff --git a/app/presenters/award_emoji_presenter.rb b/app/presenters/award_emoji_presenter.rb index 98713855d35..8a7b58e0aba 100644 --- a/app/presenters/award_emoji_presenter.rb +++ b/app/presenters/award_emoji_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AwardEmojiPresenter < Gitlab::View::Presenter::Delegated - presents :award_emoji + presents ::AwardEmoji, as: :award_emoji def description as_emoji['description'] diff --git a/app/presenters/blob_presenter.rb b/app/presenters/blob_presenter.rb index ecc16e2840c..c198859aa4c 100644 --- a/app/presenters/blob_presenter.rb +++ b/app/presenters/blob_presenter.rb @@ -7,7 +7,7 @@ class BlobPresenter < Gitlab::View::Presenter::Delegated include TreeHelper include ChecksCollaboration - presents :blob + presents ::Blob, as: :blob def highlight(to: nil, plain: nil) load_all_blob_data diff --git a/app/presenters/blobs/unfold_presenter.rb b/app/presenters/blobs/unfold_presenter.rb index 487c6fe0757..b921b5bf670 100644 --- a/app/presenters/blobs/unfold_presenter.rb +++ b/app/presenters/blobs/unfold_presenter.rb @@ -6,6 +6,8 @@ module Blobs include ActiveModel::AttributeAssignment include Gitlab::Utils::StrongMemoize + presents ::Blob + attribute :full, :boolean, default: false attribute :since, :integer, default: 1 attribute :to, :integer, default: 1 diff --git a/app/presenters/board_presenter.rb b/app/presenters/board_presenter.rb index d7cecd44dd7..bb3e96b8faf 100644 --- a/app/presenters/board_presenter.rb +++ b/app/presenters/board_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class BoardPresenter < Gitlab::View::Presenter::Delegated - presents :board + presents ::Board, as: :board end diff --git a/app/presenters/ci/bridge_presenter.rb b/app/presenters/ci/bridge_presenter.rb index 724e10c26c3..a62d7cdbbd4 100644 --- a/app/presenters/ci/bridge_presenter.rb +++ b/app/presenters/ci/bridge_presenter.rb @@ -2,6 +2,9 @@ module Ci class BridgePresenter < ProcessablePresenter + presents ::Ci::Bridge + + delegator_override :detailed_status def detailed_status @detailed_status ||= subject.detailed_status(user) end diff --git a/app/presenters/ci/build_metadata_presenter.rb b/app/presenters/ci/build_metadata_presenter.rb index 4871bb3a919..2f559adf77d 100644 --- a/app/presenters/ci/build_metadata_presenter.rb +++ b/app/presenters/ci/build_metadata_presenter.rb @@ -9,8 +9,9 @@ module Ci job_timeout_source: 'job' }.freeze - presents :metadata + presents ::Ci::BuildMetadata, as: :metadata + delegator_override :timeout_source def timeout_source return unless metadata.timeout_source? diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 06ed6791bb7..65e1c80085f 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -2,6 +2,8 @@ module Ci class BuildPresenter < ProcessablePresenter + presents ::Ci::Build + def erased_by_user? # Build can be erased through API, therefore it does not have # `erased_by` user assigned in that case. diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb index 99011150c84..dea9a42b622 100644 --- a/app/presenters/ci/group_variable_presenter.rb +++ b/app/presenters/ci/group_variable_presenter.rb @@ -2,7 +2,7 @@ module Ci class GroupVariablePresenter < Gitlab::View::Presenter::Delegated - presents :variable + presents ::Ci::GroupVariable, as: :variable def placeholder 'GROUP_VARIABLE' diff --git a/app/presenters/ci/legacy_stage_presenter.rb b/app/presenters/ci/legacy_stage_presenter.rb index d5c21baba28..c803abfab6a 100644 --- a/app/presenters/ci/legacy_stage_presenter.rb +++ b/app/presenters/ci/legacy_stage_presenter.rb @@ -2,7 +2,7 @@ module Ci class LegacyStagePresenter < Gitlab::View::Presenter::Delegated - presents :legacy_stage + presents ::Ci::LegacyStage, as: :legacy_stage def latest_ordered_statuses preload_statuses(legacy_stage.statuses.latest_ordered) diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 82f00f74692..e0cb899c9d3 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -5,6 +5,9 @@ module Ci include Gitlab::Utils::StrongMemoize include ActionView::Helpers::UrlHelper + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag` + # We use a class method here instead of a constant, allowing EE to redefine # the returned `Hash` more easily. def self.failure_reasons @@ -20,8 +23,9 @@ module Ci user_blocked: 'The user who created this pipeline is blocked.' } end - presents :pipeline + presents ::Ci::Pipeline, as: :pipeline + delegator_override :failed_builds def failed_builds return [] unless can?(current_user, :read_build, pipeline) @@ -30,6 +34,7 @@ module Ci end end + delegator_override :failure_reason def failure_reason return unless pipeline.failure_reason? diff --git a/app/presenters/ci/processable_presenter.rb b/app/presenters/ci/processable_presenter.rb index 5a8a6649071..9f3a83a00f3 100644 --- a/app/presenters/ci/processable_presenter.rb +++ b/app/presenters/ci/processable_presenter.rb @@ -2,5 +2,6 @@ module Ci class ProcessablePresenter < CommitStatusPresenter + presents ::Ci::Processable end end diff --git a/app/presenters/ci/runner_presenter.rb b/app/presenters/ci/runner_presenter.rb index 273328afc53..ad889d444f8 100644 --- a/app/presenters/ci/runner_presenter.rb +++ b/app/presenters/ci/runner_presenter.rb @@ -2,11 +2,14 @@ module Ci class RunnerPresenter < Gitlab::View::Presenter::Delegated - presents :runner + presents ::Ci::Runner, as: :runner + delegator_override :locked? def locked? read_attribute(:locked) && project_type? end + + delegator_override :locked alias_method :locked, :locked? end end diff --git a/app/presenters/ci/stage_presenter.rb b/app/presenters/ci/stage_presenter.rb index 21bda86cded..bd5bf08abbc 100644 --- a/app/presenters/ci/stage_presenter.rb +++ b/app/presenters/ci/stage_presenter.rb @@ -2,7 +2,7 @@ module Ci class StagePresenter < Gitlab::View::Presenter::Delegated - presents :stage + presents ::Ci::Stage, as: :stage PRELOADED_RELATIONS = [:pipeline, :metadata, :tags, :job_artifacts_archive, :downstream_pipeline].freeze diff --git a/app/presenters/ci/trigger_presenter.rb b/app/presenters/ci/trigger_presenter.rb index 605c8f328a4..5f0bd4b3a8b 100644 --- a/app/presenters/ci/trigger_presenter.rb +++ b/app/presenters/ci/trigger_presenter.rb @@ -2,12 +2,13 @@ module Ci class TriggerPresenter < Gitlab::View::Presenter::Delegated - presents :trigger + presents ::Ci::Trigger, as: :trigger def has_token_exposed? can?(current_user, :admin_trigger, trigger) end + delegator_override :token def token if has_token_exposed? trigger.token diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb index f027f3aa560..ec04dd5e9ff 100644 --- a/app/presenters/ci/variable_presenter.rb +++ b/app/presenters/ci/variable_presenter.rb @@ -2,7 +2,7 @@ module Ci class VariablePresenter < Gitlab::View::Presenter::Delegated - presents :variable + presents ::Ci::Variable, as: :variable def placeholder 'PROJECT_VARIABLE' diff --git a/app/presenters/clusterable_presenter.rb b/app/presenters/clusterable_presenter.rb index 03e26b92922..4b645510b51 100644 --- a/app/presenters/clusterable_presenter.rb +++ b/app/presenters/clusterable_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ClusterablePresenter < Gitlab::View::Presenter::Delegated - presents :clusterable + presents ::Project, ::Group, ::Clusters::Instance, as: :clusterable def self.fabricate(clusterable, **attributes) presenter_class = "#{clusterable.class.name}ClusterablePresenter".constantize diff --git a/app/presenters/clusters/cluster_presenter.rb b/app/presenters/clusters/cluster_presenter.rb index eb4bd8532af..892ea14267b 100644 --- a/app/presenters/clusters/cluster_presenter.rb +++ b/app/presenters/clusters/cluster_presenter.rb @@ -7,7 +7,9 @@ module Clusters include ActionView::Helpers::UrlHelper include IconsHelper - presents :cluster + delegator_override_with ::Gitlab::Utils::StrongMemoize # TODO: Remove `::Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + + presents ::Clusters::Cluster, as: :cluster # We do not want to show the group path for clusters belonging to the # clusterable, only for the ancestor clusters. diff --git a/app/presenters/clusters/integration_presenter.rb b/app/presenters/clusters/integration_presenter.rb index 57608be29b1..f7be59f00f3 100644 --- a/app/presenters/clusters/integration_presenter.rb +++ b/app/presenters/clusters/integration_presenter.rb @@ -2,7 +2,7 @@ module Clusters class IntegrationPresenter < Gitlab::View::Presenter::Delegated - presents :integration + presents ::Clusters::Integrations::Prometheus, ::Clusters::Integrations::ElasticStack, as: :integration def application_type integration.class.name.demodulize.underscore diff --git a/app/presenters/commit_presenter.rb b/app/presenters/commit_presenter.rb index c14dcab6000..7df45ca03bb 100644 --- a/app/presenters/commit_presenter.rb +++ b/app/presenters/commit_presenter.rb @@ -3,7 +3,7 @@ class CommitPresenter < Gitlab::View::Presenter::Delegated include GlobalID::Identification - presents :commit + presents ::Commit, as: :commit def status_for(ref) return unless can?(current_user, :read_commit_status, commit.project) diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb index 3c39470b730..2d524fd611e 100644 --- a/app/presenters/commit_status_presenter.rb +++ b/app/presenters/commit_status_presenter.rb @@ -33,7 +33,7 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated private_constant :CALLOUT_FAILURE_MESSAGES - presents :build + presents ::CommitStatus, as: :build def self.callout_failure_messages CALLOUT_FAILURE_MESSAGES diff --git a/app/presenters/dev_ops_report/metric_presenter.rb b/app/presenters/dev_ops_report/metric_presenter.rb index 4d7ac1cd3ec..55326f8f678 100644 --- a/app/presenters/dev_ops_report/metric_presenter.rb +++ b/app/presenters/dev_ops_report/metric_presenter.rb @@ -2,6 +2,8 @@ module DevOpsReport class MetricPresenter < Gitlab::View::Presenter::Simple + presents ::DevOpsReport::Metric + delegate :created_at, to: :subject def cards diff --git a/app/presenters/environment_presenter.rb b/app/presenters/environment_presenter.rb index 6f67bbe2a5a..6c8da86187c 100644 --- a/app/presenters/environment_presenter.rb +++ b/app/presenters/environment_presenter.rb @@ -3,7 +3,7 @@ class EnvironmentPresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :environment + presents ::Environment, as: :environment def path project_environment_path(project, self) diff --git a/app/presenters/event_presenter.rb b/app/presenters/event_presenter.rb index c37721f7213..4c787b88e20 100644 --- a/app/presenters/event_presenter.rb +++ b/app/presenters/event_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class EventPresenter < Gitlab::View::Presenter::Delegated - presents :event + presents ::Event, as: :event def initialize(subject, **attributes) super @@ -10,6 +10,7 @@ class EventPresenter < Gitlab::View::Presenter::Delegated end # Caching `visible_to_user?` method in the presenter beause it might be called multiple times. + delegator_override :visible_to_user? def visible_to_user?(user = nil) @visible_to_user_cache.fetch(user&.id) { super(user) } end diff --git a/app/presenters/gitlab/blame_presenter.rb b/app/presenters/gitlab/blame_presenter.rb index 1f2445b04a1..e9340a42e51 100644 --- a/app/presenters/gitlab/blame_presenter.rb +++ b/app/presenters/gitlab/blame_presenter.rb @@ -12,7 +12,7 @@ module Gitlab include TreeHelper include IconsHelper - presents :blame + presents nil, as: :blame CommitData = Struct.new( :author_avatar, diff --git a/app/presenters/group_clusterable_presenter.rb b/app/presenters/group_clusterable_presenter.rb index 34e7084ab02..207767af245 100644 --- a/app/presenters/group_clusterable_presenter.rb +++ b/app/presenters/group_clusterable_presenter.rb @@ -4,6 +4,8 @@ class GroupClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Group + override :cluster_status_cluster_path def cluster_status_cluster_path(cluster, params = {}) cluster_status_group_cluster_path(clusterable, cluster, params) diff --git a/app/presenters/group_member_presenter.rb b/app/presenters/group_member_presenter.rb index 5ab4b51f472..88facc3608d 100644 --- a/app/presenters/group_member_presenter.rb +++ b/app/presenters/group_member_presenter.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class GroupMemberPresenter < MemberPresenter + presents ::GroupMember + private def admin_member_permission diff --git a/app/presenters/instance_clusterable_presenter.rb b/app/presenters/instance_clusterable_presenter.rb index 56d91f90b2e..77ca171156d 100644 --- a/app/presenters/instance_clusterable_presenter.rb +++ b/app/presenters/instance_clusterable_presenter.rb @@ -4,6 +4,8 @@ class InstanceClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Clusters::Instance + def self.fabricate(clusterable, **attributes) attributes_with_presenter_class = attributes.merge(presenter_class: InstanceClusterablePresenter) diff --git a/app/presenters/invitation_presenter.rb b/app/presenters/invitation_presenter.rb index d8c07f327dd..ada8227a477 100644 --- a/app/presenters/invitation_presenter.rb +++ b/app/presenters/invitation_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class InvitationPresenter < Gitlab::View::Presenter::Delegated - presents :invitation + presents nil, as: :invitation end diff --git a/app/presenters/issue_presenter.rb b/app/presenters/issue_presenter.rb index b7f4ac0555d..72fe14d224d 100644 --- a/app/presenters/issue_presenter.rb +++ b/app/presenters/issue_presenter.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true class IssuePresenter < Gitlab::View::Presenter::Delegated - presents :issue + presents ::Issue, as: :issue def issue_path url_builder.build(issue, only_path: true) end + delegator_override :subscribed? def subscribed? issue.subscribed?(current_user, issue.project) end diff --git a/app/presenters/label_presenter.rb b/app/presenters/label_presenter.rb index 9e51e6fa4ba..fafade2828f 100644 --- a/app/presenters/label_presenter.rb +++ b/app/presenters/label_presenter.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true class LabelPresenter < Gitlab::View::Presenter::Delegated - presents :label + presents ::Label, as: :label delegate :name, :full_name, to: :label_subject, prefix: :subject + delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `Label#subject`. + def edit_path case label when GroupLabel then edit_group_label_path(label.group, label) diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index b37a43bf251..67d044dd01c 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MemberPresenter < Gitlab::View::Presenter::Delegated - presents :member + presents ::Member, as: :member def access_level_roles member.class.access_level_roles diff --git a/app/presenters/members_presenter.rb b/app/presenters/members_presenter.rb index 03ebea36d49..b572cf96235 100644 --- a/app/presenters/members_presenter.rb +++ b/app/presenters/members_presenter.rb @@ -3,7 +3,7 @@ class MembersPresenter < Gitlab::View::Presenter::Delegated include Enumerable - presents :members + presents nil, as: :members def to_ary to_a diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index fc8a290f5f7..d19d4964524 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -8,9 +8,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated include ChecksCollaboration include Gitlab::Utils::StrongMemoize + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + APPROVALS_WIDGET_BASE_TYPE = 'base' - presents :merge_request + presents ::MergeRequest, as: :merge_request def ci_status if pipeline @@ -183,6 +185,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated .can_push_to_branch?(source_branch) end + delegator_override :can_remove_source_branch? def can_remove_source_branch? source_branch_exists? && merge_request.can_remove_source_branch?(current_user) end @@ -202,6 +205,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end end + delegator_override :subscribed? def subscribed? merge_request.subscribed?(current_user, merge_request.target_project) end diff --git a/app/presenters/milestone_presenter.rb b/app/presenters/milestone_presenter.rb index 6bf8203702f..4084c8740f0 100644 --- a/app/presenters/milestone_presenter.rb +++ b/app/presenters/milestone_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MilestonePresenter < Gitlab::View::Presenter::Delegated - presents :milestone + presents ::Milestone, as: :milestone def milestone_path url_builder.build(milestone, only_path: true) diff --git a/app/presenters/pages_domain_presenter.rb b/app/presenters/pages_domain_presenter.rb index 6ef89760bec..0523f702416 100644 --- a/app/presenters/pages_domain_presenter.rb +++ b/app/presenters/pages_domain_presenter.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true class PagesDomainPresenter < Gitlab::View::Presenter::Delegated - presents :pages_domain + presents ::PagesDomain, as: :pages_domain + + delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `PagesDomain#subject`. def needs_verification? Gitlab::CurrentSettings.pages_domain_verification_enabled? && unverified? diff --git a/app/presenters/project_clusterable_presenter.rb b/app/presenters/project_clusterable_presenter.rb index 920304e743e..05344f2da96 100644 --- a/app/presenters/project_clusterable_presenter.rb +++ b/app/presenters/project_clusterable_presenter.rb @@ -4,6 +4,8 @@ class ProjectClusterablePresenter < ClusterablePresenter extend ::Gitlab::Utils::Override include ActionView::Helpers::UrlHelper + presents ::Project + override :cluster_status_cluster_path def cluster_status_cluster_path(cluster, params = {}) cluster_status_project_cluster_path(clusterable, cluster, params) diff --git a/app/presenters/project_hook_presenter.rb b/app/presenters/project_hook_presenter.rb index a65c7221b5a..a696e9fd0ec 100644 --- a/app/presenters/project_hook_presenter.rb +++ b/app/presenters/project_hook_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ProjectHookPresenter < Gitlab::View::Presenter::Delegated - presents :project_hook + presents ::ProjectHook, as: :project_hook def logs_details_path(log) project_hook_hook_log_path(project, self, log) diff --git a/app/presenters/project_member_presenter.rb b/app/presenters/project_member_presenter.rb index 17947266ed7..91d3ae96877 100644 --- a/app/presenters/project_member_presenter.rb +++ b/app/presenters/project_member_presenter.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ProjectMemberPresenter < MemberPresenter + presents ::ProjectMember + private def admin_member_permission diff --git a/app/presenters/project_presenter.rb b/app/presenters/project_presenter.rb index 066f4786cff..bbd8c715f5c 100644 --- a/app/presenters/project_presenter.rb +++ b/app/presenters/project_presenter.rb @@ -12,7 +12,10 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated include Gitlab::Utils::StrongMemoize include Gitlab::Experiment::Dsl - presents :project + delegator_override_with GitlabRoutingHelper # TODO: Remove `GitlabRoutingHelper` inclusion as it's duplicate + delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate + + presents ::Project, as: :project AnchorData = Struct.new(:is_link, :label, :link, :class_modifier, :icon, :itemprop, :data) MAX_TOPICS_TO_SHOW = 3 diff --git a/app/presenters/projects/import_export/project_export_presenter.rb b/app/presenters/projects/import_export/project_export_presenter.rb index f56760b55df..7b2ffb6d755 100644 --- a/app/presenters/projects/import_export/project_export_presenter.rb +++ b/app/presenters/projects/import_export/project_export_presenter.rb @@ -5,16 +5,24 @@ module Projects class ProjectExportPresenter < Gitlab::View::Presenter::Delegated include ActiveModel::Serializers::JSON - presents :project + presents ::Project, as: :project + # TODO: Remove `ActiveModel::Serializers::JSON` inclusion as it's duplicate + delegator_override_with ActiveModel::Serializers::JSON + delegator_override_with ActiveModel::Naming + delegator_override :include_root_in_json, :include_root_in_json? + + delegator_override :project_members def project_members super + converted_group_members end + delegator_override :description def description self.respond_to?(:override_description) ? override_description : super end + delegator_override :protected_branches def protected_branches project.exported_protected_branches end diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 13290a8e632..e3323b75188 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/app/presenters/projects/settings/deploy_keys_presenter.rb @@ -5,7 +5,7 @@ module Projects class DeployKeysPresenter < Gitlab::View::Presenter::Simple include Gitlab::Utils::StrongMemoize - presents :project + presents ::Project, as: :project delegate :size, to: :enabled_keys, prefix: true delegate :size, to: :available_project_keys, prefix: true delegate :size, to: :available_public_keys, prefix: true diff --git a/app/presenters/prometheus_alert_presenter.rb b/app/presenters/prometheus_alert_presenter.rb index 99e24bdcdb9..714329ede71 100644 --- a/app/presenters/prometheus_alert_presenter.rb +++ b/app/presenters/prometheus_alert_presenter.rb @@ -3,7 +3,7 @@ class PrometheusAlertPresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :prometheus_alert + presents ::PrometheusAlert, as: :prometheus_alert def humanized_text operator_text = diff --git a/app/presenters/release_presenter.rb b/app/presenters/release_presenter.rb index ac27e997b41..c919c7f4c60 100644 --- a/app/presenters/release_presenter.rb +++ b/app/presenters/release_presenter.rb @@ -3,8 +3,10 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :release + presents ::Release, as: :release + # TODO: Remove `delegate` as it's redundant due to SimpleDelegator. + delegator_override :tag, :project delegate :project, :tag, to: :release def commit_path @@ -51,6 +53,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated edit_project_release_url(project, release) end + delegator_override :assets_count def assets_count if can_download_code? release.assets_count @@ -59,6 +62,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated end end + delegator_override :name def name can_download_code? ? release.name : "Release-#{release.id}" end diff --git a/app/presenters/releases/evidence_presenter.rb b/app/presenters/releases/evidence_presenter.rb index a00cbacb7d8..bdc053a303b 100644 --- a/app/presenters/releases/evidence_presenter.rb +++ b/app/presenters/releases/evidence_presenter.rb @@ -4,7 +4,7 @@ module Releases class EvidencePresenter < Gitlab::View::Presenter::Delegated include ActionView::Helpers::UrlHelper - presents :evidence + presents ::Releases::Evidence, as: :evidence def filepath release = evidence.release diff --git a/app/presenters/search_service_presenter.rb b/app/presenters/search_service_presenter.rb index ab43800b9f2..72f967b8beb 100644 --- a/app/presenters/search_service_presenter.rb +++ b/app/presenters/search_service_presenter.rb @@ -3,7 +3,7 @@ class SearchServicePresenter < Gitlab::View::Presenter::Delegated include RendersCommits - presents :search_service + presents ::SearchService, as: :search_service SCOPE_PRELOAD_METHOD = { projects: :with_web_entity_associations, @@ -18,6 +18,7 @@ class SearchServicePresenter < Gitlab::View::Presenter::Delegated SORT_ENABLED_SCOPES = %w(issues merge_requests epics).freeze + delegator_override :search_objects def search_objects @search_objects ||= begin objects = search_service.search_objects(SCOPE_PRELOAD_METHOD[scope.to_sym]) diff --git a/app/presenters/sentry_error_presenter.rb b/app/presenters/sentry_error_presenter.rb index 669bcb68b7c..5862e54dfc7 100644 --- a/app/presenters/sentry_error_presenter.rb +++ b/app/presenters/sentry_error_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class SentryErrorPresenter < Gitlab::View::Presenter::Delegated - presents :error + presents nil, as: :error FrequencyStruct = Struct.new(:time, :count, keyword_init: true) diff --git a/app/presenters/service_hook_presenter.rb b/app/presenters/service_hook_presenter.rb index 8f2ba1a905f..91911eb3dff 100644 --- a/app/presenters/service_hook_presenter.rb +++ b/app/presenters/service_hook_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ServiceHookPresenter < Gitlab::View::Presenter::Delegated - presents :service_hook + presents ::ServiceHook, as: :service_hook def logs_details_path(log) project_service_hook_log_path(integration.project, integration, log) diff --git a/app/presenters/snippet_blob_presenter.rb b/app/presenters/snippet_blob_presenter.rb index ab8fc0f905b..4072696eb89 100644 --- a/app/presenters/snippet_blob_presenter.rb +++ b/app/presenters/snippet_blob_presenter.rb @@ -3,6 +3,8 @@ class SnippetBlobPresenter < BlobPresenter include GitlabRoutingHelper + presents ::SnippetBlob + def rich_data return unless blob.rich_viewer diff --git a/app/presenters/snippet_presenter.rb b/app/presenters/snippet_presenter.rb index 695aa266e2c..8badbe7f54a 100644 --- a/app/presenters/snippet_presenter.rb +++ b/app/presenters/snippet_presenter.rb @@ -1,16 +1,18 @@ # frozen_string_literal: true class SnippetPresenter < Gitlab::View::Presenter::Delegated - presents :snippet + presents ::Snippet, as: :snippet def raw_url url_builder.build(snippet, raw: true) end + delegator_override :ssh_url_to_repo def ssh_url_to_repo snippet.ssh_url_to_repo if snippet.repository_exists? end + delegator_override :http_url_to_repo def http_url_to_repo snippet.http_url_to_repo if snippet.repository_exists? end @@ -31,6 +33,7 @@ class SnippetPresenter < Gitlab::View::Presenter::Delegated snippet.submittable_as_spam_by?(current_user) end + delegator_override :blob def blob return snippet.blob if snippet.empty_repo? diff --git a/app/presenters/terraform/modules_presenter.rb b/app/presenters/terraform/modules_presenter.rb index 608f69e2019..9e9c6a5cd2b 100644 --- a/app/presenters/terraform/modules_presenter.rb +++ b/app/presenters/terraform/modules_presenter.rb @@ -4,7 +4,7 @@ module Terraform class ModulesPresenter < Gitlab::View::Presenter::Simple attr_accessor :packages, :system - presents :modules + presents nil, as: :modules def initialize(packages, system) @packages = packages diff --git a/app/presenters/todo_presenter.rb b/app/presenters/todo_presenter.rb index 291be7848e2..cb8d37be22d 100644 --- a/app/presenters/todo_presenter.rb +++ b/app/presenters/todo_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class TodoPresenter < Gitlab::View::Presenter::Delegated - presents :todo + presents ::Todo, as: :todo end diff --git a/app/presenters/tree_entry_presenter.rb b/app/presenters/tree_entry_presenter.rb index 216b3b0d4c9..0b313d81360 100644 --- a/app/presenters/tree_entry_presenter.rb +++ b/app/presenters/tree_entry_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class TreeEntryPresenter < Gitlab::View::Presenter::Delegated - presents :tree + presents nil, as: :tree def web_url Gitlab::Routing.url_helpers.project_tree_url(tree.repository.project, File.join(tree.commit_id, tree.path)) diff --git a/app/presenters/user_presenter.rb b/app/presenters/user_presenter.rb index 7cd94082bac..5a99f10b6e7 100644 --- a/app/presenters/user_presenter.rb +++ b/app/presenters/user_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class UserPresenter < Gitlab::View::Presenter::Delegated - presents :user + presents ::User, as: :user def group_memberships should_be_private? ? GroupMember.none : user.group_members diff --git a/app/presenters/web_hook_log_presenter.rb b/app/presenters/web_hook_log_presenter.rb index fca03ddb5d7..a5166589073 100644 --- a/app/presenters/web_hook_log_presenter.rb +++ b/app/presenters/web_hook_log_presenter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class WebHookLogPresenter < Gitlab::View::Presenter::Delegated - presents :web_hook_log + presents ::WebHookLog, as: :web_hook_log def details_path web_hook.present.logs_details_path(self) diff --git a/app/views/projects/usage_quotas/index.html.haml b/app/views/projects/usage_quotas/index.html.haml index dfd46af0499..6c7cccfb9b1 100644 --- a/app/views/projects/usage_quotas/index.html.haml +++ b/app/views/projects/usage_quotas/index.html.haml @@ -6,7 +6,7 @@ .row .col-sm-12 = s_('UsageQuota|Usage of project resources across the %{strong_start}%{project_name}%{strong_end} project').html_safe % { strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe, project_name: @project.name } + '.' - %a{ href: help_page_path('user/usage_quotas.md') } + %a{ href: help_page_path('user/usage_quotas.md'), target: '_blank', rel: 'noopener noreferrer' } = s_('UsageQuota|Learn more about usage quotas') + '.' .top-area.scrolling-tabs-container.inner-page-scroll-tabs diff --git a/data/deprecations/14-0-nfs-fot-git-repository-storage.yml b/data/deprecations/14-0-nfs-fot-git-repository-storage.yml index 416e4e3f447..f38742439aa 100644 --- a/data/deprecations/14-0-nfs-fot-git-repository-storage.yml +++ b/data/deprecations/14-0-nfs-fot-git-repository-storage.yml @@ -7,11 +7,11 @@ Gitaly Cluster offers tremendous benefits for our customers such as: - - [Variable replication factors](https://docs.gitlab.com/ee/administration/gitaly/praefect.html#replication-factor). - - [Strong consistency](https://docs.gitlab.com/ee/administration/gitaly/praefect.html#strong-consistency). - - [Distributed read capabilities](https://docs.gitlab.com/ee/administration/gitaly/praefect.html#distributed-reads). + - [Variable replication factors](https://docs.gitlab.com/ee/administration/gitaly/index.html#replication-factor). + - [Strong consistency](https://docs.gitlab.com/ee/administration/gitaly/index.html#strong-consistency). + - [Distributed read capabilities](https://docs.gitlab.com/ee/administration/gitaly/index.html#distributed-reads). - We encourage customers currently using NFS for Git repositories to plan their migration by reviewing our documentation on [migrating to Gitaly Cluster](https://docs.gitlab.com/ee/administration/gitaly/praefect.html#migrate-to-gitaly-cluster). + We encourage customers currently using NFS for Git repositories to plan their migration by reviewing our documentation on [migrating to Gitaly Cluster](https://docs.gitlab.com/ee/administration/gitaly/index.html#migrate-to-gitaly-cluster). stage: # (optional - may be required in the future) String value of the stage that the feature was created in. e.g., Growth tiers: # (optional - may be required in the future) An array of tiers that the feature is available in currently. e.g., [Free, Silver, Gold, Core, Premium, Ultimate] diff --git a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md index 491db37d9da..54cd11db546 100644 --- a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md +++ b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md @@ -726,6 +726,18 @@ group.require_two_factor_authentication=false group.save ``` +## Authentication + +### Re-enable standard web sign-in form + +Re-enable the standard username and password-based sign-in form if it was disabled as a [Sign-in restriction](../../user/admin_area/settings/sign_in_restrictions.md#password-authentication-enabled). + +You can use this method when a configured external authentication provider (through SSO or an LDAP configuration) is facing an outage and direct sign-in access to GitLab is required. + +```ruby +Gitlab::CurrentSettings.update!(password_authentication_enabled_for_web: true) +``` + ## SCIM ### Fixing bad SCIM identities diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ba09660186b..2c9e13e4d57 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -12624,7 +12624,7 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="projectdastsitevalidationsnormalizedtargeturls"></a>`normalizedTargetUrls` | [`[String!]`](#string) | Normalized URL of the target to be scanned. | -| <a id="projectdastsitevalidationsstatus"></a>`status` | [`DastSiteValidationStatusEnum`](#dastsitevalidationstatusenum) | Status of the site validation. Ignored if `dast_failed_site_validations` feature flag is disabled. | +| <a id="projectdastsitevalidationsstatus"></a>`status` | [`DastSiteValidationStatusEnum`](#dastsitevalidationstatusenum) | Status of the site validation. | ##### `Project.environment` diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index a6b49394733..9ef4375d795 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -133,9 +133,13 @@ Best practices for monitoring and maximizing frontend performance. Frontend security practices. -## [Accessibility](accessibility.md) +## Accessibility -Our accessibility standards and resources. +Our [accessibility standards and resources](accessibility.md). + +## Logging + +Best practices for [client-side logging](logging.md) for GitLab frontend development. ## [Internationalization (i18n) and Translations](../i18n/externalization.md) diff --git a/doc/development/fe_guide/logging.md b/doc/development/fe_guide/logging.md new file mode 100644 index 00000000000..26633eade43 --- /dev/null +++ b/doc/development/fe_guide/logging.md @@ -0,0 +1,86 @@ +--- +stage: none +group: unassigned +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Client-side logging for frontend development + +This guide contains the best practices for client-side logging for GitLab +frontend development. + +## When to log to the browser console + +We do not want to log unnecessarily to the browser console, as excessively +noisy console logs are not easy to read, parse, or process. We **do** want to +give visibility to unintended events in the system. If a possible but unexpected +exception occurs during runtime, we want to log the details of this exception. +These logs can give significantly helpful context to end users creating issues, or +contributors diagnosing problems. + +Whenever a `catch(e)` exists, and `e` is something unexpected, log the details. + +### What makes an error unexpected? + +Sometimes a caught exception can be part of normal operations. For instance, third-party +libraries might throw an exception based on certain inputs. If we can gracefully +handle these exceptions, then they are expected. Don't log them noisily. +For example: + +```javascript +try { + // Here, we call a method based on some user input. + // `doAThing` will throw an exception if the input is invalid. + const userInput = getUserInput(); + doAThing(userInput); +} catch (e) { + if (e instanceof FooSyntaxError) { + // To handle a `FooSyntaxError`, we just need to instruct the user to change their input. + // This isn't unexpected, and is part of normal operations. + setUserMessage(`Try writing better code. ${e.message}`); + } else { + // We're not sure what `e` is, so something unexpected and bad happened... + logError(e); + setUserMessage('Something unexpected happened...'); + } +} +``` + +## How to log an error + +We have a helpful `~/lib/logger` module which encapsulates how we can +consistently log runtime errors in GitLab. Import `logError` from this +module, and use it as you normally would `console.error`. Pass the actual `Error` +object, so the stack trace and other details can be captured in the log: + +```javascript +// 1. Import the logger module. +import { logError } from '~/lib/logger'; + +export const doThing = () => { + return foo() + .then(() => { + // ... + }) + .catch(e => { + // 2. Use `logError` like you would `console.error`. + logError('An unexpected error occurred while doing the thing', e); + + // We may or may not want to present that something bad happened to the end user. + showThingFailed(); + }); +}; +``` + +## Relation to frontend observability + +Client-side logging is strongly related to +[Frontend observability](https://about.gitlab.com/company/team/structure/working-groups/frontend-observability/). +We want unexpected errors to be observed by our monitoring systems, so +we can quickly react to user-facing issues. For a number of reasons, it is +unfeasible to send every log to the monitoring system. Don't shy away from using +`~/lib/logger`, but consider controlling which messages passed to `~/lib/logger` +are actually sent to the monitoring systems. + +A cohesive logging module helps us control these side effects consistently +across the various entry points. diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index ded6b074324..568e8a9d123 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -183,6 +183,8 @@ queries they produce. Everything in `app/presenters`, used for exposing complex data to a Rails view, without having to create many instance variables. +See [the documentation](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md) for more information. + ### Serializers Everything in `app/serializers`, used for presenting the response to a request, diff --git a/doc/update/deprecations.md b/doc/update/deprecations.md index 043a5865bd9..6d110c46b1b 100644 --- a/doc/update/deprecations.md +++ b/doc/update/deprecations.md @@ -16,6 +16,22 @@ To add a deprecation, use the example.yml file in `/data/deprecations/templates` then run `bin/rake gitlab:docs:compile_deprecations`. --> +## 15.2 + +### NFS for Git repository storage deprecated + +With the general availability of Gitaly Cluster ([introduced in GitLab 13.0](https://about.gitlab.com/releases/2020/05/22/gitlab-13-0-released/)), we have deprecated development (bugfixes, performance improvements, etc) for NFS for Git repository storage in GitLab 14.0. We will continue to provide technical support for NFS for Git repositories throughout 14.x, but we will remove all support for NFS in GitLab 15.0. Please see our official [Statement of Support](https://about.gitlab.com/support/statement-of-support.html#gitaly-and-nfs) for further information. + +Gitaly Cluster offers tremendous benefits for our customers such as: + +- [Variable replication factors](../administration/gitaly/index.md#replication-factor). +- [Strong consistency](../administration/gitaly/index.md#strong-consistency). +- [Distributed read capabilities](../administration/gitaly/index.md#distributed-reads). + +We encourage customers currently using NFS for Git repositories to plan their migration by reviewing our documentation on [migrating to Gitaly Cluster](../administration/gitaly/index.md#migrate-to-gitaly-cluster). + +Announced: 2021-06-22 + ## 15.0 ### Legacy database configuration @@ -28,6 +44,14 @@ This deprecation mainly impacts users compiling GitLab from source because Omnib Announced: 2021-09-22 +### GitLab Serverless + +[GitLab Serverless](../user/project/clusters/serverless/index.md) is a feature set to support Knative-based serverless development with automatic deployments and monitoring. + +We decided to remove the GitLab Serverless features as they never really resonated with our users. Besides, given the continuous development of Kubernetes and Knative, our current implementations do not even work with recent versions. + +Announced: 2021-09-22 + ### Audit events for repository push events Audit events for [repository events](../administration/audit_events.md#repository-push) are now deprecated and will be removed in GitLab 15.0. @@ -48,13 +72,13 @@ Note that we are not deprecating the Kerberos SPNEGO integration, only the old p Announced: 2021-09-22 -### GitLab Serverless +## 14.6 -[GitLab Serverless](../user/project/clusters/serverless/index.md) is a feature set to support Knative-based serverless development with automatic deployments and monitoring. +### Release CLI be distributed as a generic package -We decided to remove the GitLab Serverless features as they never really resonated with our users. Besides, given the continuous development of Kubernetes and Knative, our current implementations do not even work with recent versions. +The [release-cli](https://gitlab.com/gitlab-org/release-cli) will be released as a [generic package](https://gitlab.com/gitlab-org/release-cli/-/packages) starting in GitLab 14.2. We will continue to deploy it as a binary to S3 until GitLab 14.5 and stop distributing it in S3 in GitLab 14.6. -Announced: 2021-09-22 +Announced: 2021-08-22 ## 14.4 diff --git a/doc/user/admin_area/settings/sign_in_restrictions.md b/doc/user/admin_area/settings/sign_in_restrictions.md index 223ffeebd44..29cf0bb9d23 100644 --- a/doc/user/admin_area/settings/sign_in_restrictions.md +++ b/doc/user/admin_area/settings/sign_in_restrictions.md @@ -24,6 +24,8 @@ You can restrict the password authentication for web interface and Git over HTTP - **Web interface**: When this feature is disabled, the **Standard** sign-in tab is removed and an [external authentication provider](../../../administration/auth/index.md) must be used. - **Git over HTTP(S)**: When this feature is disabled, a [Personal Access Token](../../profile/personal_access_tokens.md) must be used to authenticate. +In the event of an external authentication provider outage, use the [GitLab Rails console](../../../administration/operations/rails_console.md) to [re-enable the standard web sign-in form](../../../administration/troubleshooting/gitlab_rails_cheat_sheet.md#re-enable-standard-web-sign-in-form). This configuration can also be changed over the [Application settings REST API](../../../api/settings.md#change-application-settings) while authenticating with an administrator account's personal access token. + ## Admin Mode > [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/2158) in GitLab 13.10. diff --git a/doc/user/application_security/dast/index.md b/doc/user/application_security/dast/index.md index 0d60701b030..cbd61747c62 100644 --- a/doc/user/application_security/dast/index.md +++ b/doc/user/application_security/dast/index.md @@ -1185,11 +1185,9 @@ If a validated site profile's target URL is edited, the site's validation status #### Retry a failed validation -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322609) in GitLab 14.3. - -FLAG: -On self-managed GitLab, by default this feature is available. To hide the feature, ask an -administrator to [disable the `dast_failed_site_validations` flag](../../../administration/feature_flags.md). +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322609) in GitLab 14.3. +> - [Deployed behind the `dast_failed_site_validations` flag](../../../administration/feature_flags.md), enabled by default. +> - [Feature flag `dast_failed_site_validations` removed](https://gitlab.com/gitlab-org/gitlab/-/issues/323961) in GitLab 14.4. If a site profile's validation fails, you can retry it by selecting the **Retry validation** button in the profiles list. diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start.rb index 8e87245e62b..fda4ab0207d 100644 --- a/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start.rb +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start.rb @@ -53,6 +53,10 @@ module Gitlab .on(mr_metrics_table[:merge_request_id].eq(mr_table[:id])) .join_sources end + + def include_in(query) + query.left_joins(merge_requests_closing_issues: { issue: [:metrics] }, metrics: []) + end end end end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production.rb index 4ca3c19051e..0cb081c64c4 100644 --- a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production.rb +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production.rb @@ -26,6 +26,10 @@ module Gitlab query.joins(merge_requests_closing_issues: { merge_request: [:metrics] }).where(mr_metrics_table[:first_deployed_to_production_at].gteq(mr_table[:created_at])) end # rubocop: enable CodeReuse/ActiveRecord + + def include_in(query) + query.left_joins(merge_requests_closing_issues: { merge_request: [:metrics] }) + end end end end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/metrics_based_stage_event.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/metrics_based_stage_event.rb index fd30ab5277d..e191b0fe897 100644 --- a/lib/gitlab/analytics/cycle_analytics/stage_events/metrics_based_stage_event.rb +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/metrics_based_stage_event.rb @@ -20,6 +20,10 @@ module Gitlab def column_list [timestamp_projection] end + + def include_in(query) + super.left_joins(:metrics) + end end end end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb index 8eb067ed0ec..945cecfcf8c 100644 --- a/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb @@ -61,6 +61,10 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord + def include_in(query) + query + end + def self.label_based? false end diff --git a/lib/gitlab/utils/delegator_override.rb b/lib/gitlab/utils/delegator_override.rb new file mode 100644 index 00000000000..15ba29d3916 --- /dev/null +++ b/lib/gitlab/utils/delegator_override.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + # This module is to validate that delegator classes (`SimpleDelegator`) do not + # accidentally override important logic on the fabricated object. + module DelegatorOverride + def delegator_target(target_class) + return unless ENV['STATIC_VERIFICATION'] + + unless self < ::SimpleDelegator + raise ArgumentError, "'#{self}' is not a subclass of 'SimpleDelegator' class." + end + + DelegatorOverride.validator(self).add_target(target_class) + end + + def delegator_override(*names) + return unless ENV['STATIC_VERIFICATION'] + raise TypeError unless names.all? { |n| n.is_a?(Symbol) } + + DelegatorOverride.validator(self).add_allowlist(names) + end + + def delegator_override_with(mod) + return unless ENV['STATIC_VERIFICATION'] + raise TypeError unless mod.is_a?(Module) + + DelegatorOverride.validator(self).add_allowlist(mod.instance_methods) + end + + def self.validator(delegator_class) + validators[delegator_class] ||= Validator.new(delegator_class) + end + + def self.validators + @validators ||= {} + end + + def self.verify! + validators.each_value do |validator| + validator.expand_on_ancestors(validators) + validator.validate_overrides! + end + end + end + end +end diff --git a/lib/gitlab/utils/delegator_override/error.rb b/lib/gitlab/utils/delegator_override/error.rb new file mode 100644 index 00000000000..dfe8d5468b4 --- /dev/null +++ b/lib/gitlab/utils/delegator_override/error.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module DelegatorOverride + class Error + attr_accessor :method_name, :target_class, :target_location, :delegator_class, :delegator_location + + def initialize(method_name, target_class, target_location, delegator_class, delegator_location) + @method_name = method_name + @target_class = target_class + @target_location = target_location + @delegator_class = delegator_class + @delegator_location = delegator_location + end + + def to_s + "#{delegator_class}##{method_name} is overriding #{target_class}##{method_name}. delegator_location: #{delegator_location} target_location: #{target_location}" + end + end + end + end +end diff --git a/lib/gitlab/utils/delegator_override/validator.rb b/lib/gitlab/utils/delegator_override/validator.rb new file mode 100644 index 00000000000..825b3efa203 --- /dev/null +++ b/lib/gitlab/utils/delegator_override/validator.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module DelegatorOverride + class Validator + UnexpectedDelegatorOverrideError = Class.new(StandardError) + + attr_reader :delegator_class, :target_classes + + OVERRIDE_ERROR_MESSAGE = <<~EOS + We've detected that the delegator is overriding a specific method(s) on the target class. + Please make sure if it's intentional and handle this error accordingly. + See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides for more information. + EOS + + def initialize(delegator_class) + @delegator_class = delegator_class + @target_classes = [] + end + + def add_allowlist(names) + allowed_method_names.concat(names) + end + + def allowed_method_names + @allowed_method_names ||= [] + end + + def add_target(target_class) + @target_classes << target_class if target_class + end + + # This will make sure allowlist we put into ancestors are all included + def expand_on_ancestors(validators) + delegator_class.ancestors.each do |ancestor| + next if delegator_class == ancestor # ancestor includes itself + + validator_ancestor = validators[ancestor] + + next unless validator_ancestor + + add_allowlist(validator_ancestor.allowed_method_names) + end + end + + def validate_overrides! + return if target_classes.empty? + + errors = [] + + # Workaround to fully load the instance methods in the target class. + # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69823#note_678887402 + target_classes.map(&:new) rescue nil + + (delegator_class.instance_methods - allowlist).each do |method_name| + target_classes.each do |target_class| + next unless target_class.instance_methods.include?(method_name) + + errors << generate_error(method_name, target_class, delegator_class) + end + end + + return if errors.empty? + + details = errors.map { |error| "- #{error}" }.join("\n") + + raise UnexpectedDelegatorOverrideError, + <<~TEXT + #{OVERRIDE_ERROR_MESSAGE} + Here are the conflict details. + + #{details} + TEXT + end + + private + + def generate_error(method_name, target_class, delegator_class) + target_location = extract_location(target_class, method_name) + delegator_location = extract_location(delegator_class, method_name) + Error.new(method_name, target_class, target_location, delegator_class, delegator_location) + end + + def extract_location(klass, method_name) + klass.instance_method(method_name).source_location&.join(':') || 'unknown' + end + + def allowlist + [].tap do |allowed| + allowed.concat(allowed_method_names) + allowed.concat(Object.instance_methods) + allowed.concat(::Delegator.instance_methods) + end + end + end + end + end +end diff --git a/lib/gitlab/view/presenter/base.rb b/lib/gitlab/view/presenter/base.rb index 9dc687f7740..3bacad72050 100644 --- a/lib/gitlab/view/presenter/base.rb +++ b/lib/gitlab/view/presenter/base.rb @@ -47,8 +47,18 @@ module Gitlab true end - def presents(name) - define_method(name) { subject } + def presents(*target_classes, as: nil) + if target_classes.any? { |k| k.is_a?(Symbol) } + raise ArgumentError, "Unsupported target class type: #{target_classes}." + end + + if self < ::Gitlab::View::Presenter::Delegated + target_classes.each { |k| delegator_target(k) } + elsif self < ::Gitlab::View::Presenter::Simple + # no-op + end + + define_method(as) { subject } if as end end end diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb index d14f8cc4e5e..259cf0cf457 100644 --- a/lib/gitlab/view/presenter/delegated.rb +++ b/lib/gitlab/view/presenter/delegated.rb @@ -4,7 +4,18 @@ module Gitlab module View module Presenter class Delegated < SimpleDelegator + extend ::Gitlab::Utils::DelegatorOverride + + # TODO: Stop including auxiliary methods/modules in `Presenter::Base` as + # it overrides many methods in the Active Record models. + # See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides + # for more information. include Gitlab::View::Presenter::Base + delegator_override_with Gitlab::Routing.url_helpers + delegator_override :can? + delegator_override :declarative_policy_delegate + delegator_override :present + delegator_override :web_url def initialize(subject, **attributes) @subject = subject diff --git a/lib/tasks/lint.rake b/lib/tasks/lint.rake index 976ec089011..7d1721f1df8 100644 --- a/lib/tasks/lint.rake +++ b/lib/tasks/lint.rake @@ -12,6 +12,7 @@ unless Rails.env.production? dev:load ] do Gitlab::Utils::Override.verify! + Gitlab::Utils::DelegatorOverride.verify! end desc "GitLab | Lint | Lint JavaScript files using ESLint" diff --git a/scripts/frontend/startup_css/startup_css_changed.sh b/scripts/frontend/startup_css/startup_css_changed.sh index 2713d752974..db6fb575d1d 100755 --- a/scripts/frontend/startup_css/startup_css_changed.sh +++ b/scripts/frontend/startup_css/startup_css_changed.sh @@ -7,7 +7,13 @@ echo "" echo "https://gitlab.com/gitlab-org/gitlab/-/issues/331812" echo "-----------------------------------------------------------" -startup_glob="*stylesheets/startup*" +startup_glob="app/assets/stylesheets/startup*" + +if ! [ "$FOSS_ONLY" ] +then + startup_glob="*${startup_glob}" +fi + echo "Staging changes to '${startup_glob}' so we can check for untracked files..." git add "${startup_glob}" diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb index b6f9c8106c9..2e96fd09602 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start_spec.rb @@ -19,4 +19,16 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::CodeStageStart do expect(records).to eq([merge_request]) expect(records).not_to include(other_merge_request) end + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) do + mr_closing_issue = FactoryBot.create(:merge_requests_closing_issues) + issue = mr_closing_issue.issue + issue.metrics.update!(first_mentioned_in_commit_at: Time.current) + + mr_closing_issue.merge_request + end + + let_it_be(:record_without_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created_spec.rb index 224a18653ed..3f50dd38322 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created_spec.rb @@ -4,4 +4,8 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::IssueCreated do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:issue) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production_spec.rb index 93e588675d3..e807565ecb5 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_deployed_to_production_spec.rb @@ -4,4 +4,16 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::IssueDeployedToProduction do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) do + mr_closing_issue = FactoryBot.create(:merge_requests_closing_issues) + mr = mr_closing_issue.merge_request + mr.metrics.update!(first_deployed_to_production_at: Time.current) + + mr_closing_issue.issue + end + + let_it_be(:record_without_data) { create(:issue) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit_spec.rb index bc0e388cf53..9bb023f9fdc 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::IssueFirstMentionedInCommit do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:issue).tap { |i| i.metrics.update!(first_mentioned_in_commit_at: Time.current) } } + let_it_be(:record_without_data) { create(:issue) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end_spec.rb index ddc5f015a8c..7b46a86cbe2 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::IssueStageEnd do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:issue).tap { |i| i.metrics.update!(first_added_to_board_at: Time.current) } } + let_it_be(:record_without_data) { create(:issue) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created_spec.rb index 281cc31c9e0..1139f9099cb 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created_spec.rb @@ -4,4 +4,8 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestCreated do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production_spec.rb index e1dd2e56e2b..a62facb6974 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestFirstDeployedToProduction do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:merge_request).tap { |mr| mr.metrics.update!(first_deployed_to_production_at: Time.current) } } + let_it_be(:record_without_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished_spec.rb index 51324966f26..c5cfe43895e 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestLastBuildFinished do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:merge_request).tap { |mr| mr.metrics.update!(latest_build_finished_at: Time.current) } } + let_it_be(:record_without_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started_spec.rb index 10dcaf23b81..6f8a82a9ae5 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestLastBuildStarted do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:merge_request).tap { |mr| mr.metrics.update!(latest_build_started_at: Time.current) } } + let_it_be(:record_without_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged_spec.rb index 6e20eb73ed9..0060ed0fd48 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged_spec.rb @@ -4,4 +4,9 @@ require 'spec_helper' RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestMerged do it_behaves_like 'value stream analytics event' + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:merge_request).tap { |mr| mr.metrics.update!(merged_at: Time.current) } } + let_it_be(:record_without_data) { create(:merge_request) } + end end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb index b8c68003127..379d59e4c5e 100644 --- a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start_spec.rb @@ -21,4 +21,9 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::StageEvents::PlanStageStart do expect(records).to match_array([issue1, issue2]) expect(records).not_to include(issue_without_metrics) end + + it_behaves_like 'LEFT JOIN-able value stream analytics event' do + let_it_be(:record_with_data) { create(:issue).tap { |i| i.metrics.update!(first_added_to_board_at: Time.current) } } + let_it_be(:record_without_data) { create(:issue) } + end end diff --git a/spec/lib/gitlab/utils/delegator_override/error_spec.rb b/spec/lib/gitlab/utils/delegator_override/error_spec.rb new file mode 100644 index 00000000000..59b67676eff --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override/error_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride::Error do + let(:error) { described_class.new('foo', 'Target', '/path/to/target', 'Delegator', '/path/to/delegator') } + + describe '#to_s' do + subject { error.to_s } + + it { is_expected.to eq("Delegator#foo is overriding Target#foo. delegator_location: /path/to/delegator target_location: /path/to/target") } + end +end diff --git a/spec/lib/gitlab/utils/delegator_override/validator_spec.rb b/spec/lib/gitlab/utils/delegator_override/validator_spec.rb new file mode 100644 index 00000000000..4cd1b18de82 --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override/validator_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride::Validator do + let(:delegator_class) do + Class.new(::SimpleDelegator) do + extend(::Gitlab::Utils::DelegatorOverride) + + def foo + end + end.prepend(ee_delegator_extension) + end + + let(:ee_delegator_extension) do + Module.new do + extend(::Gitlab::Utils::DelegatorOverride) + + def bar + end + end + end + + let(:target_class) do + Class.new do + def foo + end + + def bar + end + end + end + + let(:validator) { described_class.new(delegator_class) } + + describe '#add_allowlist' do + it 'adds a method name to the allowlist' do + validator.add_allowlist([:foo]) + + expect(validator.allowed_method_names).to contain_exactly(:foo) + end + end + + describe '#add_target' do + it 'adds the target class' do + validator.add_target(target_class) + + expect(validator.target_classes).to contain_exactly(target_class) + end + end + + describe '#expand_on_ancestors' do + it 'adds the allowlist in the ancestors' do + ancestor_validator = described_class.new(ee_delegator_extension) + ancestor_validator.add_allowlist([:bar]) + validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator }) + + expect(validator.allowed_method_names).to contain_exactly(:bar) + end + end + + describe '#validate_overrides!' do + before do + validator.add_target(target_class) + end + + it 'does not raise an error when the overrides are allowed' do + validator.add_allowlist([:foo]) + ancestor_validator = described_class.new(ee_delegator_extension) + ancestor_validator.add_allowlist([:bar]) + validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator }) + + expect { validator.validate_overrides! }.not_to raise_error + end + + it 'raises an error when there is an override' do + expect { validator.validate_overrides! } + .to raise_error(described_class::UnexpectedDelegatorOverrideError) + end + end +end diff --git a/spec/lib/gitlab/utils/delegator_override_spec.rb b/spec/lib/gitlab/utils/delegator_override_spec.rb new file mode 100644 index 00000000000..af4c7fa5d8e --- /dev/null +++ b/spec/lib/gitlab/utils/delegator_override_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Utils::DelegatorOverride do + let(:delegator_class) do + Class.new(::SimpleDelegator) do + extend(::Gitlab::Utils::DelegatorOverride) + + def foo + end + end + end + + let(:target_class) do + Class.new do + def foo + end + + def bar + end + end + end + + let(:dummy_module) do + Module.new do + def foobar + end + end + end + + before do + stub_env('STATIC_VERIFICATION', 'true') + end + + describe '.delegator_target' do + subject { delegator_class.delegator_target(target_class) } + + it 'sets the delegator target to the validator' do + expect(described_class.validator(delegator_class)) + .to receive(:add_target).with(target_class) + + subject + end + + context 'when the class does not inherit SimpleDelegator' do + let(:delegator_class) do + Class.new do + extend(::Gitlab::Utils::DelegatorOverride) + end + end + + it 'raises an error' do + expect { subject }.to raise_error(ArgumentError, /not a subclass of 'SimpleDelegator' class/) + end + end + end + + describe '.delegator_override' do + subject { delegator_class.delegator_override(:foo) } + + it 'adds the method name to the allowlist' do + expect(described_class.validator(delegator_class)) + .to receive(:add_allowlist).with([:foo]) + + subject + end + end + + describe '.delegator_override_with' do + subject { delegator_class.delegator_override_with(dummy_module) } + + it 'adds the method names of the module to the allowlist' do + expect(described_class.validator(delegator_class)) + .to receive(:add_allowlist).with([:foobar]) + + subject + end + end + + describe '.verify!' do + subject { described_class.verify! } + + it 'does not raise an error when an override is in allowlist' do + delegator_class.delegator_target(target_class) + delegator_class.delegator_override(:foo) + + expect { subject }.not_to raise_error + end + + it 'raises an error when there is an override' do + delegator_class.delegator_target(target_class) + + expect { subject }.to raise_error(Gitlab::Utils::DelegatorOverride::Validator::UnexpectedDelegatorOverrideError) + end + end +end diff --git a/spec/lib/gitlab/view/presenter/base_spec.rb b/spec/lib/gitlab/view/presenter/base_spec.rb index 97d5e2b280d..a7083bd2722 100644 --- a/spec/lib/gitlab/view/presenter/base_spec.rb +++ b/spec/lib/gitlab/view/presenter/base_spec.rb @@ -18,11 +18,43 @@ RSpec.describe Gitlab::View::Presenter::Base do describe '.presents' do it 'exposes #subject with the given keyword' do - presenter_class.presents(:foo) + presenter_class.presents(Object, as: :foo) presenter = presenter_class.new(project) expect(presenter.foo).to eq(project) end + + it 'raises an error when symbol is passed' do + expect { presenter_class.presents(:foo) }.to raise_error(ArgumentError) + end + + context 'when the presenter class inherits Presenter::Delegated' do + let(:presenter_class) do + Class.new(::Gitlab::View::Presenter::Delegated) do + include(::Gitlab::View::Presenter::Base) + end + end + + it 'sets the delegator target' do + expect(presenter_class).to receive(:delegator_target).with(Object) + + presenter_class.presents(Object, as: :foo) + end + end + + context 'when the presenter class inherits Presenter::Simple' do + let(:presenter_class) do + Class.new(::Gitlab::View::Presenter::Simple) do + include(::Gitlab::View::Presenter::Base) + end + end + + it 'does not set the delegator target' do + expect(presenter_class).not_to receive(:delegator_target).with(Object) + + presenter_class.presents(Object, as: :foo) + end + end end describe '#can?' do diff --git a/spec/support/shared_examples/lib/gitlab/cycle_analytics/event_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/cycle_analytics/event_shared_examples.rb index 6e12b5a0e85..bd8bdd70ce5 100644 --- a/spec/support/shared_examples/lib/gitlab/cycle_analytics/event_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/cycle_analytics/event_shared_examples.rb @@ -33,3 +33,38 @@ RSpec.shared_examples_for 'value stream analytics event' do end end end + +RSpec.shared_examples_for 'LEFT JOIN-able value stream analytics event' do + let(:params) { {} } + let(:instance) { described_class.new(params) } + let(:record_with_data) { nil } + let(:record_without_data) { nil } + let(:scope) { instance.object_type.all } + + let(:records) do + scope_with_left_join = instance.include_in(scope) + scope_with_left_join.select(scope.model.arel_table[:id], instance.timestamp_projection.as('timestamp_column_data')).to_a + end + + it 'can use the event as LEFT JOIN' do + expected_record_count = record_without_data.nil? ? 1 : 2 + + expect(records.count).to eq(expected_record_count) + end + + context 'when looking at the record with data' do + subject(:record) { records.to_a.find { |r| r.id == record_with_data.id } } + + it 'contains the timestamp expression' do + expect(record.timestamp_column_data).not_to eq(nil) + end + end + + context 'when looking at the record without data' do + subject(:record) { records.to_a.find { |r| r.id == record_without_data.id } } + + it 'returns nil for the timestamp expression' do + expect(record.timestamp_column_data).to eq(nil) if record_without_data + end + end +end |