diff --git a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue index 32fe841f16e..316408adfb2 100644 --- a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue +++ b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue @@ -65,12 +65,6 @@ export default { modalActionText() { return this.variableBeingEdited ? __('Update variable') : __('Add variable'); }, - primaryAction() { - return { - text: this.modalActionText, - attributes: { variant: 'success', disabled: !this.canSubmit }, - }; - }, maskedFeedback() { return __('This variable can not be masked'); }, @@ -120,6 +114,8 @@ export default { ref="modal" :modal-id="$options.modalId" :title="modalActionText" + static + lazy @hidden="resetModalHandler" >
@@ -127,7 +123,7 @@ export default { @@ -142,7 +138,7 @@ export default { v-model="variableData.secret_value" rows="3" max-rows="6" - data-qa-selector="variable_value" + data-qa-selector="ci_variable_value_field" /> @@ -189,7 +185,7 @@ export default { {{ __('Mask variable') }} @@ -218,6 +214,7 @@ export default { ref="deleteCiVariable" category="secondary" variant="danger" + data-qa-selector="ci_variable_delete_button" @click="deleteVarAndClose" >{{ __('Delete variable') }} @@ -225,6 +222,7 @@ export default { ref="updateOrAddVariable" :disabled="!canSubmit" variant="success" + data-qa-selector="ci_variable_save_button" @click="updateOrAddVariable" >{{ modalActionText }} diff --git a/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue b/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue index b374d950c1f..7eb791f97e4 100644 --- a/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue +++ b/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue @@ -26,7 +26,6 @@ export default { { key: 'value', label: s__('CiVariables|Value'), - tdClass: 'qa-ci-variable-input-value', customStyle: { width: '40%' }, }, { @@ -89,6 +88,7 @@ export default { :fields="fields" :items="variables" tbody-tr-class="js-ci-variable-row" + data-qa-selector="ci_variable_table_content" sort-by="key" sort-direction="asc" stacked="lg" @@ -150,6 +150,7 @@ export default { @@ -168,7 +169,7 @@ export default { {{ valuesButtonText }}{{ __('Add Variable') }} diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index f857e618d89..86714471823 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -232,3 +232,11 @@ export const truncateNamespace = (string = '') => { return namespace; }; + +/** + * Tests that the input is a String and has at least + * one non-whitespace character + * @param {String} obj The object to test + * @returns {Boolean} + */ +export const hasContent = obj => isString(obj) && obj.trim() !== ''; diff --git a/app/assets/javascripts/releases/components/app_edit.vue b/app/assets/javascripts/releases/components/app_edit.vue index 06e388002e4..df356c18417 100644 --- a/app/assets/javascripts/releases/components/app_edit.vue +++ b/app/assets/javascripts/releases/components/app_edit.vue @@ -1,6 +1,6 @@ @@ -69,60 +102,93 @@ export default {

{{ __( - 'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance.', + 'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed.', ) }}

+ + - - - {{ __('Remove asset link') }} - +
+ + + {{ __('Remove asset link') }} + +
- {{ __('Add another link') }} - + diff --git a/app/assets/javascripts/releases/stores/modules/detail/getters.js b/app/assets/javascripts/releases/stores/modules/detail/getters.js index 562284dc48d..84dc2fca4be 100644 --- a/app/assets/javascripts/releases/stores/modules/detail/getters.js +++ b/app/assets/javascripts/releases/stores/modules/detail/getters.js @@ -1,9 +1,13 @@ +import { isEmpty } from 'lodash'; +import { hasContent } from '~/lib/utils/text_utility'; + /** + * @param {Object} link The link to test * @returns {Boolean} `true` if the release link is empty, i.e. it has * empty (or whitespace-only) values for both `url` and `name`. * Otherwise, `false`. */ -const isEmptyReleaseLink = l => !/\S/.test(l.url) && !/\S/.test(l.name); +const isEmptyReleaseLink = link => !hasContent(link.url) && !hasContent(link.name); /** Returns all release links that aren't empty */ export const releaseLinksToCreate = state => { @@ -22,3 +26,67 @@ export const releaseLinksToDelete = state => { return state.originalRelease.assets.links; }; + +/** Returns all validation errors on the release object */ +export const validationErrors = state => { + const errors = { + assets: { + links: {}, + }, + }; + + if (!state.release) { + return errors; + } + + // Each key of this object is a URL, and the value is an + // array of Release link objects that share this URL. + // This is used for detecting duplicate URLs. + const urlToLinksMap = new Map(); + + state.release.assets.links.forEach(link => { + errors.assets.links[link.id] = {}; + + // Only validate non-empty URLs + if (isEmptyReleaseLink(link)) { + return; + } + + if (!hasContent(link.url)) { + errors.assets.links[link.id].isUrlEmpty = true; + } + + if (!hasContent(link.name)) { + errors.assets.links[link.id].isNameEmpty = true; + } + + const normalizedUrl = link.url.trim().toLowerCase(); + + // Compare each URL to every other URL and flag any duplicates + if (urlToLinksMap.has(normalizedUrl)) { + // a duplicate URL was found! + + // add a validation error for each link that shares this URL + const duplicates = urlToLinksMap.get(normalizedUrl); + duplicates.push(link); + duplicates.forEach(duplicateLink => { + errors.assets.links[duplicateLink.id].isDuplicate = true; + }); + } else { + // no duplicate URL was found + + urlToLinksMap.set(normalizedUrl, [link]); + } + + if (!/^(http|https|ftp):\/\//.test(normalizedUrl)) { + errors.assets.links[link.id].isBadFormat = true; + } + }); + + return errors; +}; + +/** Returns whether or not the release object is valid */ +export const isValid = (_state, getters) => { + return Object.values(getters.validationErrors.assets.links).every(isEmpty); +}; diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index c418b11ab13..34af1ecd6a5 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -9,6 +9,7 @@ class Import::GithubController < Import::BaseController before_action :expire_etag_cache, only: [:status, :create] rescue_from Octokit::Unauthorized, with: :provider_unauthorized + rescue_from Octokit::TooManyRequests, with: :provider_rate_limit def new if !ci_cd_only? && github_import_configured? && logged_in_with_provider? @@ -142,6 +143,13 @@ class Import::GithubController < Import::BaseController alert: "Access denied to your #{Gitlab::ImportSources.title(provider.to_s)} account." end + def provider_rate_limit(exception) + reset_time = Time.at(exception.response_headers['x-ratelimit-reset'].to_i) + session[access_token_key] = nil + redirect_to new_import_url, + alert: _("GitHub API rate limit exceeded. Try again after %{reset_time}") % { reset_time: reset_time } + end + def access_token_key :"#{provider}_access_token" end @@ -180,7 +188,7 @@ class Import::GithubController < Import::BaseController end def client_options - {} + { wait_for_rate_limit_reset: false } end def extra_import_params diff --git a/app/models/jira_import_state.rb b/app/models/jira_import_state.rb index ec1b8f03d36..543ee77917c 100644 --- a/app/models/jira_import_state.rb +++ b/app/models/jira_import_state.rb @@ -12,6 +12,8 @@ class JiraImportState < ApplicationRecord belongs_to :user belongs_to :label + scope :by_jira_project_key, -> (jira_project_key) { where(jira_project_key: jira_project_key) } + validates :project, presence: true validates :jira_project_key, presence: true validates :jira_project_name, presence: true diff --git a/app/models/project.rb b/app/models/project.rb index ee4cc6157eb..443b44dd023 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1190,14 +1190,14 @@ class Project < ApplicationRecord end def external_issue_tracker - if has_external_issue_tracker.nil? # To populate existing projects + if has_external_issue_tracker.nil? cache_has_external_issue_tracker end if has_external_issue_tracker? - return @external_issue_tracker if defined?(@external_issue_tracker) - - @external_issue_tracker = services.external_issue_trackers.first + strong_memoize(:external_issue_tracker) do + services.external_issue_trackers.first + end else nil end @@ -1217,7 +1217,7 @@ class Project < ApplicationRecord def external_wiki if has_external_wiki.nil? - cache_has_external_wiki # Populate + cache_has_external_wiki end if has_external_wiki diff --git a/app/services/jira_import/start_import_service.rb b/app/services/jira_import/start_import_service.rb index 6accc3cfffc..e8d9e6734bd 100644 --- a/app/services/jira_import/start_import_service.rb +++ b/app/services/jira_import/start_import_service.rb @@ -33,8 +33,10 @@ module JiraImport end def build_jira_import + label = create_import_label(project) project.jira_imports.build( user: user, + label: label, jira_project_key: jira_project_key, # we do not have the jira_project_name or jira_project_xid yet so just set a mock value, # we will once https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28190 @@ -43,6 +45,22 @@ module JiraImport ) end + def create_import_label(project) + label = ::Labels::CreateService.new(build_label_attrs(project)).execute(project: project) + raise Projects::ImportService::Error, _('Failed to create import label for jira import.') if label.blank? + + label + end + + def build_label_attrs(project) + import_start_time = Time.zone.now + jira_imports_for_project = project.jira_imports.by_jira_project_key(jira_project_key).size + 1 + title = "jira-import::#{jira_project_key}-#{jira_imports_for_project}" + description = "Label for issues that were imported from jira on #{import_start_time.strftime('%Y-%m-%d %H:%M:%S')}" + color = "#{Label.color_for(title)}" + { title: title, description: description, color: color } + end + def validate return build_error_response(_('Jira import feature is disabled.')) unless project.jira_issues_import_feature_flag_enabled? return build_error_response(_('You do not have permissions to run the import.')) unless user.can?(:admin_project, project) diff --git a/changelogs/unreleased/211460-annotations-post-endpoint-revised.yml b/changelogs/unreleased/211460-annotations-post-endpoint-revised.yml new file mode 100644 index 00000000000..26abcf76ea6 --- /dev/null +++ b/changelogs/unreleased/211460-annotations-post-endpoint-revised.yml @@ -0,0 +1,5 @@ +--- +title: API endpoint to create annotations for environments dashboard +merge_request: 29089 +author: +type: added diff --git a/changelogs/unreleased/213225-adjust-issues-label-on-jira-import.yml b/changelogs/unreleased/213225-adjust-issues-label-on-jira-import.yml new file mode 100644 index 00000000000..045756cd025 --- /dev/null +++ b/changelogs/unreleased/213225-adjust-issues-label-on-jira-import.yml @@ -0,0 +1,5 @@ +--- +title: Adjust label title applied to issues on import from Jira +merge_request: 29246 +author: +type: changed diff --git a/changelogs/unreleased/github-rate-limit-on-project-import.yml b/changelogs/unreleased/github-rate-limit-on-project-import.yml new file mode 100644 index 00000000000..24df1e68a89 --- /dev/null +++ b/changelogs/unreleased/github-rate-limit-on-project-import.yml @@ -0,0 +1,5 @@ +--- +title: Better error message when importing a Github project and Github API rate limit is exceeded +merge_request: 28785 +author: +type: fixed diff --git a/changelogs/unreleased/update-ci-variable-qa-test.yml b/changelogs/unreleased/update-ci-variable-qa-test.yml new file mode 100644 index 00000000000..ca34985917f --- /dev/null +++ b/changelogs/unreleased/update-ci-variable-qa-test.yml @@ -0,0 +1,5 @@ +--- +title: Fix failing ci variable e2e test +merge_request: 25924 +author: +type: fixed diff --git a/db/migrate/20200406102111_add_index_to_deployments_where_cluster_id_is_not_null.rb b/db/migrate/20200406102111_add_index_to_deployments_where_cluster_id_is_not_null.rb new file mode 100644 index 00000000000..67ffba6af5e --- /dev/null +++ b/db/migrate/20200406102111_add_index_to_deployments_where_cluster_id_is_not_null.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToDeploymentsWhereClusterIdIsNotNull < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :deployments, :id, where: 'cluster_id IS NOT NULL', name: 'index_deployments_on_id_where_cluster_id_present' + end + + def down + remove_concurrent_index :deployments, :id, where: 'cluster_id IS NOT NULL', name: 'index_deployments_on_id_where_cluster_id_present' + end +end diff --git a/db/post_migrate/20200406102120_backfill_deployment_clusters_from_deployments.rb b/db/post_migrate/20200406102120_backfill_deployment_clusters_from_deployments.rb new file mode 100644 index 00000000000..2db270d303c --- /dev/null +++ b/db/post_migrate/20200406102120_backfill_deployment_clusters_from_deployments.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class BackfillDeploymentClustersFromDeployments < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + MIGRATION = 'BackfillDeploymentClustersFromDeployments' + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 10_000 + + disable_ddl_transaction! + + class Deployment < ActiveRecord::Base + include EachBatch + + default_scope { where('cluster_id IS NOT NULL') } + + self.table_name = 'deployments' + end + + def up + say "Scheduling `#{MIGRATION}` jobs" + + queue_background_migration_jobs_by_range_at_intervals(Deployment, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end + + def down + # NOOP + end +end diff --git a/db/structure.sql b/db/structure.sql index cfa0f3c405a..f924d69fd75 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9087,6 +9087,8 @@ CREATE INDEX index_deployments_on_environment_id_and_status ON public.deployment CREATE INDEX index_deployments_on_id_and_status ON public.deployments USING btree (id, status); +CREATE INDEX index_deployments_on_id_where_cluster_id_present ON public.deployments USING btree (id) WHERE (cluster_id IS NOT NULL); + CREATE INDEX index_deployments_on_project_id_and_id ON public.deployments USING btree (project_id, id DESC); CREATE UNIQUE INDEX index_deployments_on_project_id_and_iid ON public.deployments USING btree (project_id, iid); @@ -13137,6 +13139,8 @@ COPY "schema_migrations" (version) FROM STDIN; 20200403184110 20200403185127 20200403185422 +20200406102111 +20200406102120 20200406135648 20200406165950 20200406171857 diff --git a/lib/api/api.rb b/lib/api/api.rb index eb7f47de9e2..de9a3120d90 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -152,6 +152,7 @@ module API mount ::API::Members mount ::API::MergeRequestDiffs mount ::API::MergeRequests + mount ::API::Metrics::Dashboard::Annotations mount ::API::Namespaces mount ::API::Notes mount ::API::Discussions diff --git a/lib/api/entities/metrics/dashboard/annotation.rb b/lib/api/entities/metrics/dashboard/annotation.rb new file mode 100644 index 00000000000..66bd09d84f9 --- /dev/null +++ b/lib/api/entities/metrics/dashboard/annotation.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module API + module Entities + module Metrics + module Dashboard + class Annotation < Grape::Entity + expose :id + expose :starting_at + expose :ending_at + expose :dashboard_path + expose :description + expose :environment_id + expose :cluster_id + end + end + end + end +end diff --git a/lib/api/metrics/dashboard/annotations.rb b/lib/api/metrics/dashboard/annotations.rb new file mode 100644 index 00000000000..691abac863a --- /dev/null +++ b/lib/api/metrics/dashboard/annotations.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module API + module Metrics + module Dashboard + class Annotations < Grape::API + desc 'Create a new monitoring dashboard annotation' do + success Entities::Metrics::Dashboard::Annotation + end + + params do + requires :starting_at, type: DateTime, + desc: 'Date time indicating starting moment to which the annotation relates.' + optional :ending_at, type: DateTime, + desc: 'Date time indicating ending moment to which the annotation relates.' + requires :dashboard_path, type: String, + desc: 'The path to a file defining the dashboard on which the annotation should be added' + requires :description, type: String, desc: 'The description of the annotation' + end + + resource :environments do + post ':id/metrics_dashboard/annotations' do + environment = ::Environment.find(params[:id]) + + not_found! unless Feature.enabled?(:metrics_dashboard_annotations, environment.project) + + forbidden! unless can?(current_user, :create_metrics_dashboard_annotation, environment) + + result = ::Metrics::Dashboard::Annotations::CreateService.new(current_user, declared(params).merge(environment: environment)).execute + + if result[:status] == :success + present result[:annotation], with: Entities::Metrics::Dashboard::Annotation + else + error!(result, 400) + end + end + end + end + end + end +end diff --git a/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments.rb b/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments.rb new file mode 100644 index 00000000000..9778f360e87 --- /dev/null +++ b/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # Backfill deployment_clusters for a range of deployments + class BackfillDeploymentClustersFromDeployments + def perform(start_id, end_id) + ActiveRecord::Base.connection.execute <<~SQL + INSERT INTO deployment_clusters (deployment_id, cluster_id) + SELECT deployments.id, deployments.cluster_id + FROM deployments + WHERE deployments.cluster_id IS NOT NULL + AND deployments.id BETWEEN #{start_id} AND #{end_id} + ON CONFLICT DO NOTHING + SQL + end + end + end +end diff --git a/lib/gitlab/jira_import/labels_importer.rb b/lib/gitlab/jira_import/labels_importer.rb index 142a2da5be9..35c434e48a4 100644 --- a/lib/gitlab/jira_import/labels_importer.rb +++ b/lib/gitlab/jira_import/labels_importer.rb @@ -11,28 +11,19 @@ module Gitlab end def execute - create_import_label(project) + cache_import_label(project) import_jira_labels end private - def create_import_label(project) - label = Labels::CreateService.new(build_label_attrs(project)).execute(project: project) - raise Projects::ImportService::Error, _('Failed to create import label for jira import.') unless label + def cache_import_label(project) + label = project.jira_imports.by_jira_project_key(jira_project_key).last.label + raise Projects::ImportService::Error, _('Failed to find import label for jira import.') unless label JiraImport.cache_import_label_id(project.id, label.id) end - def build_label_attrs(project) - import_start_time = project&.import_state&.last_update_started_at || Time.now - title = "jira-import-#{import_start_time.strftime('%Y-%m-%d-%H-%M-%S')}" - description = "Label for issues that were imported from jira on #{import_start_time.strftime('%Y-%m-%d %H:%M:%S')}" - color = "#{Label.color_for(title)}" - - { title: title, description: description, color: color } - end - def import_jira_labels # todo: import jira labels, see https://gitlab.com/gitlab-org/gitlab/-/issues/212651 job_waiter diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb index 34634d20a16..f7eaafeb446 100644 --- a/lib/gitlab/legacy_github_import/client.rb +++ b/lib/gitlab/legacy_github_import/client.rb @@ -6,13 +6,14 @@ module Gitlab GITHUB_SAFE_REMAINING_REQUESTS = 100 GITHUB_SAFE_SLEEP_TIME = 500 - attr_reader :access_token, :host, :api_version + attr_reader :access_token, :host, :api_version, :wait_for_rate_limit_reset - def initialize(access_token, host: nil, api_version: 'v3') + def initialize(access_token, host: nil, api_version: 'v3', wait_for_rate_limit_reset: true) @access_token = access_token @host = host.to_s.sub(%r{/+\z}, '') @api_version = api_version @users = {} + @wait_for_rate_limit_reset = wait_for_rate_limit_reset if access_token ::Octokit.auto_paginate = false @@ -120,7 +121,7 @@ module Gitlab end def request(method, *args, &block) - sleep rate_limit_sleep_time if rate_limit_exceed? + sleep rate_limit_sleep_time if wait_for_rate_limit_reset && rate_limit_exceed? data = api.__send__(method, *args) # rubocop:disable GitlabSecurity/PublicSend return data unless data.is_a?(Array) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 220c41fa318..b9863823be0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8670,6 +8670,9 @@ msgstr "" msgid "Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later." msgstr "" +msgid "Failed to find import label for jira import." +msgstr "" + msgid "Failed to get ref." msgstr "" @@ -9723,6 +9726,9 @@ msgstr "" msgid "Git version" msgstr "" +msgid "GitHub API rate limit exceeded. Try again after %{reset_time}" +msgstr "" + msgid "GitHub import" msgstr "" @@ -12153,6 +12159,9 @@ msgstr "" msgid "Link title" msgstr "" +msgid "Link title is required" +msgstr "" + msgid "Linked emails (%{email_count})" msgstr "" @@ -14996,7 +15005,7 @@ msgstr "" msgid "Pods in use" msgstr "" -msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance." +msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed." msgstr "" msgid "Preferences" @@ -17199,6 +17208,18 @@ msgstr "" msgid "Requirement" msgstr "" +msgid "Requirement %{reference} has been added" +msgstr "" + +msgid "Requirement %{reference} has been archived" +msgstr "" + +msgid "Requirement %{reference} has been reopened" +msgstr "" + +msgid "Requirement %{reference} has been updated" +msgstr "" + msgid "Requirement title cannot have more than %{limit} characters." msgstr "" @@ -20697,6 +20718,9 @@ msgstr "" msgid "This Project is currently archived and read-only. Please unarchive the project first if you want to resume Pull mirroring" msgstr "" +msgid "This URL is already used for another link; duplicate URLs are not allowed" +msgstr "" + msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention." msgstr "" @@ -21793,9 +21817,15 @@ msgstr "" msgid "URL" msgstr "" +msgid "URL is required" +msgstr "" + msgid "URL must be a valid url (ex: https://gitlab.com)" msgstr "" +msgid "URL must start with %{codeStart}http://%{codeEnd}, %{codeStart}https://%{codeEnd}, or %{codeStart}ftp://%{codeEnd}" +msgstr "" + msgid "URL of the external storage that will serve the repository static objects (e.g. archives, blobs, ...)." msgstr "" diff --git a/qa/qa/page/project/settings/ci_variables.rb b/qa/qa/page/project/settings/ci_variables.rb index 2bb285d6086..6cdf40cd1da 100644 --- a/qa/qa/page/project/settings/ci_variables.rb +++ b/qa/qa/page/project/settings/ci_variables.rb @@ -7,75 +7,47 @@ module QA class CiVariables < Page::Base include Common - view 'app/views/ci/variables/_variable_row.html.haml' do - element :variable_row, '.ci-variable-row-body' # rubocop:disable QA/ElementWithPattern - element :variable_key, '.qa-ci-variable-input-key' # rubocop:disable QA/ElementWithPattern - element :variable_value, '.qa-ci-variable-input-value' # rubocop:disable QA/ElementWithPattern - element :variable_masked + view 'app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue' do + element :ci_variable_key_field + element :ci_variable_value_field + element :ci_variable_masked_checkbox + element :ci_variable_save_button + element :ci_variable_delete_button end - view 'app/views/ci/variables/_index.html.haml' do - element :save_variables, '.js-ci-variables-save-button' # rubocop:disable QA/ElementWithPattern - element :reveal_values, '.js-secret-value-reveal-button' # rubocop:disable QA/ElementWithPattern + view 'app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue' do + element :ci_variable_table_content + element :add_ci_variable_button + element :edit_ci_variable_button + element :reveal_ci_variable_value_button end def fill_variable(key, value, masked) - keys = all_elements(:ci_variable_input_key, minimum: 1) - index = keys.size - 1 - - # After we fill the key, JS would generate another field so - # we need to use the same index to find the corresponding one. - keys[index].set(key) - node = all_elements(:ci_variable_input_value, count: keys.size + 1)[index] - - # Simply run `node.set(value)` is too slow for long text here, - # so we need to run JavaScript directly to set the value. - # The code was inspired from: - # https://github.com/teamcapybara/capybara/blob/679548cea10773d45e32808f4d964377cfe5e892/lib/capybara/selenium/node.rb#L217 - execute_script("arguments[0].value = #{value.to_json}", node) - - masked_node = all_elements(:variable_masked, count: keys.size + 1)[index] - toggle_masked(masked_node, masked) + fill_element :ci_variable_key_field, key + fill_element :ci_variable_value_field, value + click_ci_variable_save_button end - def save_variables - find('.js-ci-variables-save-button').click + def click_add_variable + click_element :add_ci_variable_button end - def reveal_variables - find('.js-secret-value-reveal-button').click - end - - def variable_value(key) - within('.ci-variable-row-body', text: key) do - find('.qa-ci-variable-input-value').value + def click_edit_ci_variable + within_element(:ci_variable_table_content) do + click_element :edit_ci_variable_button end end - def remove_variable(location: :first) - within('.ci-variable-row-body', match: location) do - find('button.ci-variable-row-remove-button').click - end - - save_variables + def click_ci_variable_save_button + click_element :ci_variable_save_button end - private - - def toggle_masked(masked_node, masked) - wait_until(reload: false) do - masked_node.click - - masked ? masked_enabled?(masked_node) : masked_disabled?(masked_node) - end + def click_reveal_ci_variable_value_button + click_element :reveal_ci_variable_value_button end - def masked_enabled?(masked_node) - masked_node[:class].include?('is-checked') - end - - def masked_disabled?(masked_node) - !masked_enabled?(masked_node) + def click_ci_variable_delete_button + click_element :ci_variable_delete_button end end end diff --git a/qa/qa/resource/ci_variable.rb b/qa/qa/resource/ci_variable.rb index b178a64b72d..f14fcdaac9f 100644 --- a/qa/qa/resource/ci_variable.rb +++ b/qa/qa/resource/ci_variable.rb @@ -19,9 +19,8 @@ module QA Page::Project::Settings::CICD.perform do |setting| setting.expand_ci_variables do |page| + page.click_add_variable page.fill_variable(key, value, masked) - - page.save_variables end end end diff --git a/qa/qa/specs/features/browser_ui/4_verify/ci_variable/add_remove_ci_variable_spec.rb b/qa/qa/specs/features/browser_ui/4_verify/ci_variable/add_remove_ci_variable_spec.rb index cff415dcf97..f7a6c8411db 100644 --- a/qa/qa/specs/features/browser_ui/4_verify/ci_variable/add_remove_ci_variable_spec.rb +++ b/qa/qa/specs/features/browser_ui/4_verify/ci_variable/add_remove_ci_variable_spec.rb @@ -2,7 +2,7 @@ module QA context 'Verify' do - describe 'Add or Remove CI variable via UI', :smoke, quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/issues/207915', type: :stale } do + describe 'Add or Remove CI variable via UI', :smoke do let!(:project) do Resource::Project.fabricate_via_api! do |project| project.name = 'project-with-ci-variables' @@ -10,6 +10,14 @@ module QA end end + before(:all) do + Runtime::Feature.enable_and_verify('new_variables_ui') + end + + after(:all) do + Runtime::Feature.remove('new_variables_ui') + end + before do Flow::Login.sign_in add_ci_variable @@ -19,12 +27,12 @@ module QA it 'user adds a CI variable' do Page::Project::Settings::CICD.perform do |settings| settings.expand_ci_variables do |page| - expect(page).to have_field(with: 'VARIABLE_KEY') - expect(page).not_to have_field(with: 'some_CI_variable') + expect(page).to have_text('VARIABLE_KEY') + expect(page).not_to have_text('some_CI_variable') - page.reveal_variables + page.click_reveal_ci_variable_value_button - expect(page).to have_field(with: 'some_CI_variable') + expect(page).to have_text('some_CI_variable') end end end @@ -32,9 +40,10 @@ module QA it 'user removes a CI variable' do Page::Project::Settings::CICD.perform do |settings| settings.expand_ci_variables do |page| - page.remove_variable + page.click_edit_ci_variable + page.click_ci_variable_delete_button - expect(page).not_to have_field(with: 'VARIABLE_KEY') + expect(page).not_to have_text('VARIABLE_KEY') end end end diff --git a/qa/qa/vendor/jenkins/page/last_job_console.rb b/qa/qa/vendor/jenkins/page/last_job_console.rb index 4c511a8c1f8..f41b91c2cdb 100644 --- a/qa/qa/vendor/jenkins/page/last_job_console.rb +++ b/qa/qa/vendor/jenkins/page/last_job_console.rb @@ -14,7 +14,12 @@ module QA end def has_successful_build? - page.has_text?('Finished: SUCCESS') + # Retry on errors such as: + # Selenium::WebDriver::Error::JavascriptError: + # javascript error: this.each is not a function + Support::Retrier.retry_on_exception(reload_page: page) do + page.has_text?('Finished: SUCCESS') + end end def no_failed_status_update? diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 4a3d591e94d..2a913069acc 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -748,7 +748,7 @@ describe ApplicationController do end end - describe '#current_user_mode', :do_not_mock_admin_mode do + describe '#current_user_mode' do include_context 'custom session' controller(described_class) do diff --git a/spec/controllers/concerns/enforces_admin_authentication_spec.rb b/spec/controllers/concerns/enforces_admin_authentication_spec.rb index a8494543558..1809bb2d636 100644 --- a/spec/controllers/concerns/enforces_admin_authentication_spec.rb +++ b/spec/controllers/concerns/enforces_admin_authentication_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe EnforcesAdminAuthentication, :do_not_mock_admin_mode do +describe EnforcesAdminAuthentication do include AdminModeHelper let(:user) { create(:user) } diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb index b5154f4f877..b2ae16e0ee6 100644 --- a/spec/controllers/groups/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/groups/settings/ci_cd_controller_spec.rb @@ -180,32 +180,38 @@ describe Groups::Settings::CiCdController do group.add_owner(user) end - it { is_expected.to redirect_to(group_settings_ci_cd_path) } - - context 'when service execution went wrong' do - let(:update_service) { double } - - before do - allow(Groups::UpdateService).to receive(:new).and_return(update_service) - allow(update_service).to receive(:execute).and_return(false) - allow_any_instance_of(Group).to receive_message_chain(:errors, :full_messages) - .and_return(['Error 1']) - - subject - end - - it 'returns a flash alert' do - expect(response).to set_flash[:alert] - .to eq("There was a problem updating the pipeline settings: [\"Error 1\"].") - end + context 'when admin mode is disabled' do + it { is_expected.to have_gitlab_http_status(:not_found) } end - context 'when service execution was successful' do - it 'returns a flash notice' do - subject + context 'when admin mode is enabled', :enable_admin_mode do + it { is_expected.to redirect_to(group_settings_ci_cd_path) } - expect(response).to set_flash[:notice] - .to eq('Pipeline settings was updated for the group') + context 'when service execution went wrong' do + let(:update_service) { double } + + before do + allow(Groups::UpdateService).to receive(:new).and_return(update_service) + allow(update_service).to receive(:execute).and_return(false) + allow_any_instance_of(Group).to receive_message_chain(:errors, :full_messages) + .and_return(['Error 1']) + + subject + end + + it 'returns a flash alert' do + expect(response).to set_flash[:alert] + .to eq("There was a problem updating the pipeline settings: [\"Error 1\"].") + end + end + + context 'when service execution was successful' do + it 'returns a flash notice' do + subject + + expect(response).to set_flash[:notice] + .to eq('Pipeline settings was updated for the group') + end end end end diff --git a/spec/controllers/projects/clusters/applications_controller_spec.rb b/spec/controllers/projects/clusters/applications_controller_spec.rb index 8dcbf575627..6de3593be28 100644 --- a/spec/controllers/projects/clusters/applications_controller_spec.rb +++ b/spec/controllers/projects/clusters/applications_controller_spec.rb @@ -10,7 +10,12 @@ describe Projects::Clusters::ApplicationsController do end shared_examples 'a secure endpoint' do - it { expect { subject }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { subject }.to be_allowed_for(:admin) + end + it 'is denied for admin when admin mode disabled' do + expect { subject }.to be_denied_for(:admin) + end it { expect { subject }.to be_allowed_for(:owner).of(project) } it { expect { subject }.to be_allowed_for(:maintainer).of(project) } it { expect { subject }.to be_denied_for(:developer).of(project) } diff --git a/spec/controllers/projects/clusters_controller_spec.rb b/spec/controllers/projects/clusters_controller_spec.rb index a5683a27837..07733ec30d9 100644 --- a/spec/controllers/projects/clusters_controller_spec.rb +++ b/spec/controllers/projects/clusters_controller_spec.rb @@ -65,7 +65,12 @@ describe Projects::ClustersController do describe 'security' do let(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -151,7 +156,12 @@ describe Projects::ClustersController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -240,7 +250,12 @@ describe Projects::ClustersController do allow(WaitForClusterCreationWorker).to receive(:perform_in).and_return(nil) end - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -346,7 +361,12 @@ describe Projects::ClustersController do stub_kubeclient_get_namespace('https://kubernetes.example.com', namespace: 'my-namespace') end - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -414,7 +434,12 @@ describe Projects::ClustersController do allow(WaitForClusterCreationWorker).to receive(:perform_in) end - it { expect { post_create_aws }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { post_create_aws }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { post_create_aws }.to be_denied_for(:admin) + end it { expect { post_create_aws }.to be_allowed_for(:owner).of(project) } it { expect { post_create_aws }.to be_allowed_for(:maintainer).of(project) } it { expect { post_create_aws }.to be_denied_for(:developer).of(project) } @@ -469,7 +494,12 @@ describe Projects::ClustersController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -501,7 +531,12 @@ describe Projects::ClustersController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -541,7 +576,12 @@ describe Projects::ClustersController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -574,7 +614,12 @@ describe Projects::ClustersController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -677,7 +722,12 @@ describe Projects::ClustersController do describe 'security' do let_it_be(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } @@ -746,7 +796,12 @@ describe Projects::ClustersController do describe 'security' do let_it_be(:cluster) { create(:cluster, :provided_by_gcp, :production_environment, projects: [project]) } - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is disabled for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_denied_for(:developer).of(project) } diff --git a/spec/controllers/projects/deploy_keys_controller_spec.rb b/spec/controllers/projects/deploy_keys_controller_spec.rb index a97f9ebf36b..a6bbe6bd012 100644 --- a/spec/controllers/projects/deploy_keys_controller_spec.rb +++ b/spec/controllers/projects/deploy_keys_controller_spec.rb @@ -163,7 +163,7 @@ describe Projects::DeployKeysController do end end - context 'with admin' do + context 'with admin', :enable_admin_mode do before do sign_in(admin) end @@ -228,7 +228,7 @@ describe Projects::DeployKeysController do end end - context 'with admin' do + context 'with admin', :enable_admin_mode do before do sign_in(admin) end @@ -284,7 +284,7 @@ describe Projects::DeployKeysController do end end - context 'with admin' do + context 'with admin', :enable_admin_mode do before do sign_in(admin) end @@ -311,8 +311,16 @@ describe Projects::DeployKeysController do context 'public deploy key attached to project' do let(:extra_params) { deploy_key_params('updated title', '1') } - it 'updates the title of the deploy key' do - expect { subject }.to change { deploy_key.reload.title }.to('updated title') + context 'admin mode disabled' do + it 'does not update the title of the deploy key' do + expect { subject }.not_to change { deploy_key.reload.title } + end + end + + context 'admin mode enabled', :enable_admin_mode do + it 'updates the title of the deploy key' do + expect { subject }.to change { deploy_key.reload.title }.to('updated title') + end end it 'updates can_push of deploy_keys_project' do diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index fdc8fe5f082..9526e14a748 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -586,12 +586,23 @@ describe Projects::IssuesController do expect(assigns(:issues)).to include request_forgery_timing_attack end - it 'lists confidential issues for admin' do - sign_in(admin) - get_issues + context 'when admin mode is enabled', :enable_admin_mode do + it 'lists confidential issues for admin' do + sign_in(admin) + get_issues - expect(assigns(:issues)).to include unescaped_parameter_value - expect(assigns(:issues)).to include request_forgery_timing_attack + expect(assigns(:issues)).to include unescaped_parameter_value + expect(assigns(:issues)).to include request_forgery_timing_attack + end + end + + context 'when admin mode is disabled' do + it 'does not list confidential issues for admin' do + sign_in(admin) + get_issues + + expect(assigns(:issues)).to eq [issue] + end end def get_issues @@ -648,11 +659,22 @@ describe Projects::IssuesController do expect(response).to have_gitlab_http_status http_status[:success] end - it "returns #{http_status[:success]} for admin" do - sign_in(admin) - go(id: unescaped_parameter_value.to_param) + context 'when admin mode is enabled', :enable_admin_mode do + it "returns #{http_status[:success]} for admin" do + sign_in(admin) + go(id: unescaped_parameter_value.to_param) - expect(response).to have_gitlab_http_status http_status[:success] + expect(response).to have_gitlab_http_status http_status[:success] + end + end + + context 'when admin mode is disabled' do + xit 'returns 404 for admin' do + sign_in(admin) + go(id: unescaped_parameter_value.to_param) + + expect(response).to have_gitlab_http_status :not_found + end end end diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 0071e6c8a19..ef1253edda5 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -391,10 +391,20 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do sign_in(user) end - it 'settings_path is available' do - expect(response).to have_gitlab_http_status(:ok) - expect(response).to match_response_schema('job/job_details') - expect(json_response['runners']['settings_path']).to match(/runners/) + context 'when admin mode is disabled' do + it 'settings_path is not available' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('job/job_details') + expect(json_response['runners']).not_to have_key('settings_path') + end + end + + context 'when admin mode is enabled', :enable_admin_mode do + it 'settings_path is available' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('job/job_details') + expect(json_response['runners']['settings_path']).to match(/runners/) + end end end end diff --git a/spec/controllers/projects/mirrors_controller_spec.rb b/spec/controllers/projects/mirrors_controller_spec.rb index 3579e4aa2cf..faeade0d737 100644 --- a/spec/controllers/projects/mirrors_controller_spec.rb +++ b/spec/controllers/projects/mirrors_controller_spec.rb @@ -39,12 +39,24 @@ describe Projects::MirrorsController do expect(response).to have_gitlab_http_status(:not_found) end - it 'allows requests from an admin user' do - user.update!(admin: true) - sign_in(user) + context 'when admin mode is enabled', :enable_admin_mode do + it 'allows requests from an admin user' do + user.update!(admin: true) + sign_in(user) - subject_action - expect(response).to redirect_to(project_settings_path) + subject_action + expect(response).to redirect_to(project_settings_path) + end + end + + context 'when admin mode is disabled' do + it 'disallows requests from an admin user' do + user.update!(admin: true) + sign_in(user) + + subject_action + expect(response).to have_gitlab_http_status(:not_found) + end end end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 72b282429e9..635980ba93b 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -127,7 +127,12 @@ describe Projects::PipelineSchedulesController do describe 'security' do let(:schedule) { attributes_for(:ci_pipeline_schedule) } - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is denied for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_allowed_for(:developer).of(project) } @@ -279,7 +284,12 @@ describe Projects::PipelineSchedulesController do describe 'security' do let(:schedule) { { description: 'updated_desc' } } - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is denied for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) } @@ -343,7 +353,12 @@ describe Projects::PipelineSchedulesController do end describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is denied for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) } @@ -361,7 +376,12 @@ describe Projects::PipelineSchedulesController do describe 'GET #take_ownership' do describe 'security' do - it { expect { go }.to be_allowed_for(:admin) } + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do + expect { go }.to be_allowed_for(:admin) + end + it 'is denied for admin when admin mode disabled' do + expect { go }.to be_denied_for(:admin) + end it { expect { go }.to be_allowed_for(:owner).of(project) } it { expect { go }.to be_allowed_for(:maintainer).of(project) } it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) } diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index 3684a1bb8d8..0facef85985 100644 --- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb @@ -245,11 +245,22 @@ describe Projects::Settings::CiCdController do context 'and user is an admin' do let(:user) { create(:admin) } - it 'sets max_artifacts_size' do - subject + context 'with admin mode disabled' do + it 'does not set max_artifacts_size' do + subject - project.reload - expect(project.max_artifacts_size).to eq(10) + project.reload + expect(project.max_artifacts_size).to be_nil + end + end + + context 'with admin mode enabled', :enable_admin_mode do + it 'sets max_artifacts_size' do + subject + + project.reload + expect(project.max_artifacts_size).to eq(10) + end end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index d0e0dabc9f2..fc3efc8e805 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -362,7 +362,7 @@ describe ProjectsController do end describe 'GET edit' do - it 'allows an admin user to access the page' do + it 'allows an admin user to access the page', :enable_admin_mode do sign_in(create(:user, :admin)) get :edit, @@ -531,7 +531,7 @@ describe ProjectsController do end end - describe "#update" do + describe "#update", :enable_admin_mode do render_views let(:admin) { create(:admin) } @@ -672,7 +672,7 @@ describe ProjectsController do end end - describe '#transfer' do + describe '#transfer', :enable_admin_mode do render_views let(:project) { create(:project, :repository) } @@ -720,7 +720,7 @@ describe ProjectsController do end end - describe "#destroy" do + describe "#destroy", :enable_admin_mode do let(:admin) { create(:admin) } it "redirects to the dashboard", :sidekiq_might_not_need_inline do @@ -1094,7 +1094,7 @@ describe ProjectsController do end end - context 'for a DELETE request' do + context 'for a DELETE request', :enable_admin_mode do before do sign_in(create(:admin)) end diff --git a/spec/frontend/lib/utils/text_utility_spec.js b/spec/frontend/lib/utils/text_utility_spec.js index dc8f6c64136..4969c591dcd 100644 --- a/spec/frontend/lib/utils/text_utility_spec.js +++ b/spec/frontend/lib/utils/text_utility_spec.js @@ -224,4 +224,18 @@ describe('text_utility', () => { }); }); }); + + describe('hasContent', () => { + it.each` + txt | result + ${null} | ${false} + ${undefined} | ${false} + ${{ an: 'object' }} | ${false} + ${''} | ${false} + ${' \t\r\n'} | ${false} + ${'hello'} | ${true} + `('returns $result for input $txt', ({ result, txt }) => { + expect(textUtils.hasContent(txt)).toEqual(result); + }); + }); }); diff --git a/spec/frontend/releases/components/app_edit_spec.js b/spec/frontend/releases/components/app_edit_spec.js index bf66f5a5183..09bafe4aa9b 100644 --- a/spec/frontend/releases/components/app_edit_spec.js +++ b/spec/frontend/releases/components/app_edit_spec.js @@ -5,14 +5,16 @@ import { release as originalRelease } from '../mock_data'; import * as commonUtils from '~/lib/utils/common_utils'; import { BACK_URL_PARAM } from '~/releases/constants'; import AssetLinksForm from '~/releases/components/asset_links_form.vue'; +import { merge } from 'lodash'; describe('Release edit component', () => { let wrapper; let release; let actions; + let getters; let state; - const factory = (featureFlags = {}) => { + const factory = ({ featureFlags = {}, store: storeUpdates = {} } = {}) => { state = { release, markdownDocsPath: 'path/to/markdown/docs', @@ -26,15 +28,30 @@ describe('Release edit component', () => { addEmptyAssetLink: jest.fn(), }; - const store = new Vuex.Store({ - modules: { - detail: { - namespaced: true, - actions, - state, + getters = { + isValid: () => true, + validationErrors: () => ({ + assets: { + links: [], }, - }, - }); + }), + }; + + const store = new Vuex.Store( + merge( + { + modules: { + detail: { + namespaced: true, + actions, + state, + getters, + }, + }, + }, + storeUpdates, + ), + ); wrapper = mount(ReleaseEditApp, { store, @@ -55,6 +72,8 @@ describe('Release edit component', () => { wrapper = null; }); + const findSubmitButton = () => wrapper.find('button[type=submit]'); + describe(`basic functionality tests: all tests unrelated to the "${BACK_URL_PARAM}" parameter`, () => { beforeEach(() => { factory(); @@ -101,7 +120,7 @@ describe('Release edit component', () => { }); it('renders the "Save changes" button as type="submit"', () => { - expect(wrapper.find('.js-submit-button').attributes('type')).toBe('submit'); + expect(findSubmitButton().attributes('type')).toBe('submit'); }); it('calls updateRelease when the form is submitted', () => { @@ -143,7 +162,7 @@ describe('Release edit component', () => { describe('when the release_asset_link_editing feature flag is disabled', () => { beforeEach(() => { - factory({ releaseAssetLinkEditing: false }); + factory({ featureFlags: { releaseAssetLinkEditing: false } }); }); it('does not render the asset links portion of the form', () => { @@ -153,7 +172,7 @@ describe('Release edit component', () => { describe('when the release_asset_link_editing feature flag is enabled', () => { beforeEach(() => { - factory({ releaseAssetLinkEditing: true }); + factory({ featureFlags: { releaseAssetLinkEditing: true } }); }); it('renders the asset links portion of the form', () => { @@ -161,4 +180,46 @@ describe('Release edit component', () => { }); }); }); + + describe('validation', () => { + describe('when the form is valid', () => { + beforeEach(() => { + factory({ + store: { + modules: { + detail: { + getters: { + isValid: () => true, + }, + }, + }, + }, + }); + }); + + it('renders the submit button as enabled', () => { + expect(findSubmitButton().attributes('disabled')).toBeUndefined(); + }); + }); + + describe('when the form is invalid', () => { + beforeEach(() => { + factory({ + store: { + modules: { + detail: { + getters: { + isValid: () => false, + }, + }, + }, + }, + }); + }); + + it('renders the submit button as disabled', () => { + expect(findSubmitButton().attributes('disabled')).toBe('disabled'); + }); + }); + }); }); diff --git a/spec/frontend/releases/components/asset_links_form_spec.js b/spec/frontend/releases/components/asset_links_form_spec.js new file mode 100644 index 00000000000..44542868cfe --- /dev/null +++ b/spec/frontend/releases/components/asset_links_form_spec.js @@ -0,0 +1,229 @@ +import Vuex from 'vuex'; +import { mount, createLocalVue } from '@vue/test-utils'; +import AssetLinksForm from '~/releases/components/asset_links_form.vue'; +import { release as originalRelease } from '../mock_data'; +import * as commonUtils from '~/lib/utils/common_utils'; + +const localVue = createLocalVue(); +localVue.use(Vuex); + +describe('Release edit component', () => { + let wrapper; + let release; + let actions; + let getters; + let state; + + const factory = ({ release: overriddenRelease, linkErrors } = {}) => { + state = { + release: overriddenRelease || release, + releaseAssetsDocsPath: 'path/to/release/assets/docs', + }; + + actions = { + addEmptyAssetLink: jest.fn(), + updateAssetLinkUrl: jest.fn(), + updateAssetLinkName: jest.fn(), + removeAssetLink: jest.fn().mockImplementation((_context, linkId) => { + state.release.assets.links = state.release.assets.links.filter(l => l.id !== linkId); + }), + }; + + getters = { + validationErrors: () => ({ + assets: { + links: linkErrors || {}, + }, + }), + }; + + const store = new Vuex.Store({ + modules: { + detail: { + namespaced: true, + actions, + state, + getters, + }, + }, + }); + + wrapper = mount(AssetLinksForm, { + localVue, + store, + }); + }; + + beforeEach(() => { + release = commonUtils.convertObjectPropsToCamelCase(originalRelease, { deep: true }); + }); + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + describe('with a basic store state', () => { + beforeEach(() => { + factory(); + }); + + it('calls the "addEmptyAssetLink" store method when the "Add another link" button is clicked', () => { + expect(actions.addEmptyAssetLink).not.toHaveBeenCalled(); + + wrapper.find({ ref: 'addAnotherLinkButton' }).vm.$emit('click'); + + expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1); + }); + + it('calls the "removeAssetLinks" store method when the remove button is clicked', () => { + expect(actions.removeAssetLink).not.toHaveBeenCalled(); + + wrapper.find('.remove-button').vm.$emit('click'); + + expect(actions.removeAssetLink).toHaveBeenCalledTimes(1); + }); + + it('calls the "updateAssetLinkUrl" store method when text is entered into the "URL" input field', () => { + const linkIdToUpdate = release.assets.links[0].id; + const newUrl = 'updated url'; + + expect(actions.updateAssetLinkUrl).not.toHaveBeenCalled(); + + wrapper.find({ ref: 'urlInput' }).vm.$emit('change', newUrl); + + expect(actions.updateAssetLinkUrl).toHaveBeenCalledTimes(1); + expect(actions.updateAssetLinkUrl).toHaveBeenCalledWith( + expect.anything(), + { + linkIdToUpdate, + newUrl, + }, + undefined, + ); + }); + + it('calls the "updateAssetLinName" store method when text is entered into the "Link title" input field', () => { + const linkIdToUpdate = release.assets.links[0].id; + const newName = 'updated name'; + + expect(actions.updateAssetLinkName).not.toHaveBeenCalled(); + + wrapper.find({ ref: 'nameInput' }).vm.$emit('change', newName); + + expect(actions.updateAssetLinkName).toHaveBeenCalledTimes(1); + expect(actions.updateAssetLinkName).toHaveBeenCalledWith( + expect.anything(), + { + linkIdToUpdate, + newName, + }, + undefined, + ); + }); + }); + + describe('validation', () => { + let linkId; + + beforeEach(() => { + linkId = release.assets.links[0].id; + }); + + const findUrlValidationMessage = () => wrapper.find('.url-field .invalid-feedback'); + const findNameValidationMessage = () => wrapper.find('.link-title-field .invalid-feedback'); + + it('does not show any validation messages if there are no validation errors', () => { + factory(); + + expect(findUrlValidationMessage().exists()).toBe(false); + expect(findNameValidationMessage().exists()).toBe(false); + }); + + it('shows a validation error message when two links have the same URLs', () => { + factory({ + linkErrors: { + [linkId]: { isDuplicate: true }, + }, + }); + + expect(findUrlValidationMessage().text()).toBe( + 'This URL is already used for another link; duplicate URLs are not allowed', + ); + }); + + it('shows a validation error message when a URL has a bad format', () => { + factory({ + linkErrors: { + [linkId]: { isBadFormat: true }, + }, + }); + + expect(findUrlValidationMessage().text()).toBe( + 'URL must start with http://, https://, or ftp://', + ); + }); + + it('shows a validation error message when the URL is empty (and the title is not empty)', () => { + factory({ + linkErrors: { + [linkId]: { isUrlEmpty: true }, + }, + }); + + expect(findUrlValidationMessage().text()).toBe('URL is required'); + }); + + it('shows a validation error message when the title is empty (and the URL is not empty)', () => { + factory({ + linkErrors: { + [linkId]: { isNameEmpty: true }, + }, + }); + + expect(findNameValidationMessage().text()).toBe('Link title is required'); + }); + }); + + describe('empty state', () => { + describe('when the release fetched from the API has no links', () => { + beforeEach(() => { + factory({ + release: { + ...release, + assets: { + links: [], + }, + }, + }); + }); + + it('calls the addEmptyAssetLink store method when the component is created', () => { + expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1); + }); + }); + + describe('when the release fetched from the API has one link', () => { + beforeEach(() => { + factory({ + release: { + ...release, + assets: { + links: release.assets.links.slice(0, 1), + }, + }, + }); + }); + + it('does not call the addEmptyAssetLink store method when the component is created', () => { + expect(actions.addEmptyAssetLink).not.toHaveBeenCalled(); + }); + + it('calls addEmptyAssetLink when the final link is deleted by the user', () => { + wrapper.find('.remove-button').vm.$emit('click'); + + expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1); + }); + }); + }); +}); diff --git a/spec/frontend/releases/stores/modules/detail/getters_spec.js b/spec/frontend/releases/stores/modules/detail/getters_spec.js index 7dc95c24055..8945ad97c93 100644 --- a/spec/frontend/releases/stores/modules/detail/getters_spec.js +++ b/spec/frontend/releases/stores/modules/detail/getters_spec.js @@ -56,4 +56,158 @@ describe('Release detail getters', () => { expect(getters.releaseLinksToDelete(state)).toEqual(originalLinks); }); }); + + describe('validationErrors', () => { + describe('when the form is valid', () => { + it('returns no validation errors', () => { + const state = { + release: { + assets: { + links: [ + { id: 1, url: 'https://example.com/valid', name: 'Link 1' }, + { id: 2, url: '', name: '' }, + { id: 3, url: '', name: ' ' }, + { id: 4, url: ' ', name: '' }, + { id: 5, url: ' ', name: ' ' }, + ], + }, + }, + }; + + const expectedErrors = { + assets: { + links: { + 1: {}, + 2: {}, + 3: {}, + 4: {}, + 5: {}, + }, + }, + }; + + expect(getters.validationErrors(state)).toEqual(expectedErrors); + }); + }); + + describe('when the form is invalid', () => { + let actualErrors; + + beforeEach(() => { + const state = { + release: { + assets: { + links: [ + // Duplicate URLs + { id: 1, url: 'https://example.com/duplicate', name: 'Link 1' }, + { id: 2, url: 'https://example.com/duplicate', name: 'Link 2' }, + + // the validation check ignores leading/trailing + // whitespace and is case-insensitive + { id: 3, url: ' \tHTTPS://EXAMPLE.COM/DUPLICATE\n\r\n ', name: 'Link 3' }, + + // Invalid URL format + { id: 4, url: 'invalid', name: 'Link 4' }, + + // Missing URL + { id: 5, url: '', name: 'Link 5' }, + { id: 6, url: ' ', name: 'Link 6' }, + + // Missing title + { id: 7, url: 'https://example.com/valid/1', name: '' }, + { id: 8, url: 'https://example.com/valid/2', name: ' ' }, + ], + }, + }, + }; + + actualErrors = getters.validationErrors(state); + }); + + it('returns a validation errors if links share a URL', () => { + const expectedErrors = { + assets: { + links: { + 1: { isDuplicate: true }, + 2: { isDuplicate: true }, + 3: { isDuplicate: true }, + }, + }, + }; + + expect(actualErrors).toMatchObject(expectedErrors); + }); + + it('returns a validation error if the URL is in the wrong format', () => { + const expectedErrors = { + assets: { + links: { + 4: { isBadFormat: true }, + }, + }, + }; + + expect(actualErrors).toMatchObject(expectedErrors); + }); + + it('returns a validation error if the URL missing (and the title is populated)', () => { + const expectedErrors = { + assets: { + links: { + 6: { isUrlEmpty: true }, + 5: { isUrlEmpty: true }, + }, + }, + }; + + expect(actualErrors).toMatchObject(expectedErrors); + }); + + it('returns a validation error if the title missing (and the URL is populated)', () => { + const expectedErrors = { + assets: { + links: { + 7: { isNameEmpty: true }, + 8: { isNameEmpty: true }, + }, + }, + }; + + expect(actualErrors).toMatchObject(expectedErrors); + }); + }); + }); + + describe('isValid', () => { + // the value of state is not actually used by this getter + const state = {}; + + it('returns true when the form is valid', () => { + const mockGetters = { + validationErrors: { + assets: { + links: { + 1: {}, + }, + }, + }, + }; + + expect(getters.isValid(state, mockGetters)).toBe(true); + }); + + it('returns false when the form is invalid', () => { + const mockGetters = { + validationErrors: { + assets: { + links: { + 1: { isNameEmpty: true }, + }, + }, + }, + }; + + expect(getters.isValid(state, mockGetters)).toBe(false); + }); + }); }); diff --git a/spec/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments_spec.rb b/spec/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments_spec.rb new file mode 100644 index 00000000000..fdabc8e8f7c --- /dev/null +++ b/spec/lib/gitlab/background_migration/backfill_deployment_clusters_from_deployments_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::BackfillDeploymentClustersFromDeployments, :migration, schema: 20200227140242 do + subject { described_class.new } + + describe '#perform' do + it 'backfills deployment_cluster for all deployments in the given range with a non-null cluster_id' do + deployment_clusters = table(:deployment_clusters) + + namespace = table(:namespaces).create(name: 'the-namespace', path: 'the-path') + project = table(:projects).create(name: 'the-project', namespace_id: namespace.id) + environment = table(:environments).create(name: 'the-environment', project_id: project.id, slug: 'slug') + cluster = table(:clusters).create(name: 'the-cluster') + + deployment_data = { cluster_id: cluster.id, project_id: project.id, environment_id: environment.id, ref: 'abc', tag: false, sha: 'sha', status: 1 } + expected_deployment_1 = create_deployment(**deployment_data) + create_deployment(**deployment_data, cluster_id: nil) # no cluster_id + expected_deployment_2 = create_deployment(**deployment_data) + out_of_range_deployment = create_deployment(**deployment_data, cluster_id: cluster.id) # expected to be out of range + + # to test "ON CONFLICT DO NOTHING" + existing_record_for_deployment_2 = deployment_clusters.create( + deployment_id: expected_deployment_2.id, + cluster_id: expected_deployment_2.cluster_id, + kubernetes_namespace: 'production' + ) + + subject.perform(expected_deployment_1.id, out_of_range_deployment.id - 1) + + expect(deployment_clusters.all.pluck(:deployment_id, :cluster_id, :kubernetes_namespace)).to contain_exactly( + [expected_deployment_1.id, cluster.id, nil], + [expected_deployment_2.id, cluster.id, existing_record_for_deployment_2.kubernetes_namespace] + ) + end + + def create_deployment(**data) + @iid ||= 0 + @iid += 1 + table(:deployments).create(iid: @iid, **data) + end + end +end diff --git a/spec/lib/gitlab/jira_import/labels_importer_spec.rb b/spec/lib/gitlab/jira_import/labels_importer_spec.rb index 2d0e2bc6b53..3eb4666a74f 100644 --- a/spec/lib/gitlab/jira_import/labels_importer_spec.rb +++ b/spec/lib/gitlab/jira_import/labels_importer_spec.rb @@ -5,7 +5,6 @@ require 'spec_helper' describe Gitlab::JiraImport::LabelsImporter do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } - let_it_be(:jira_import) { create(:jira_import_state, project: project) } let_it_be(:jira_service) { create(:jira_service, project: project) } subject { described_class.new(project).execute } @@ -15,29 +14,24 @@ describe Gitlab::JiraImport::LabelsImporter do end describe '#execute', :clean_gitlab_redis_cache do - context 'when label creation failes' do - before do - allow_next_instance_of(Labels::CreateService) do |instance| - allow(instance).to receive(:execute).and_return(nil) - end - end + context 'when label is missing from jira import' do + let_it_be(:no_label_jira_import) { create(:jira_import_state, label: nil, project: project) } it 'raises error' do - expect { subject }.to raise_error(Projects::ImportService::Error, 'Failed to create import label for jira import.') + expect { subject }.to raise_error(Projects::ImportService::Error, 'Failed to find import label for jira import.') end end - context 'when label is created successfully' do - it 'creates import label' do - expect { subject }.to change { Label.count }.by(1) - end + context 'when label exists' do + let_it_be(:label) { create(:label) } + let_it_be(:jira_import_with_label) { create(:jira_import_state, label: label, project: project) } it 'caches import label' do expect(Gitlab::Cache::Import::Caching.read(Gitlab::JiraImport.import_label_cache_key(project.id))).to be nil subject - expect(Gitlab::JiraImport.get_import_label_id(project.id).to_i).to be > 0 + expect(Gitlab::JiraImport.get_import_label_id(project.id).to_i).to eq(label.id) end end end diff --git a/spec/lib/gitlab/legacy_github_import/client_spec.rb b/spec/lib/gitlab/legacy_github_import/client_spec.rb index 8d1786ae49a..d266b39bd81 100644 --- a/spec/lib/gitlab/legacy_github_import/client_spec.rb +++ b/spec/lib/gitlab/legacy_github_import/client_spec.rb @@ -5,8 +5,9 @@ require 'spec_helper' describe Gitlab::LegacyGithubImport::Client do let(:token) { '123456' } let(:github_provider) { Settingslogic.new('app_id' => 'asd123', 'app_secret' => 'asd123', 'name' => 'github', 'args' => { 'client_options' => {} }) } + let(:wait_for_rate_limit_reset) { true } - subject(:client) { described_class.new(token) } + subject(:client) { described_class.new(token, wait_for_rate_limit_reset: wait_for_rate_limit_reset) } before do allow(Gitlab.config.omniauth).to receive(:providers).and_return([github_provider]) @@ -88,10 +89,23 @@ describe Gitlab::LegacyGithubImport::Client do end end - it 'does not raise error when rate limit is disabled' do - stub_request(:get, /api.github.com/) - allow(client.api).to receive(:rate_limit!).and_raise(Octokit::NotFound) + context 'github rate limit' do + it 'does not raise error when rate limit is disabled' do + stub_request(:get, /api.github.com/) + allow(client.api).to receive(:rate_limit!).and_raise(Octokit::NotFound) - expect { client.issues {} }.not_to raise_error + expect { client.repos }.not_to raise_error + end + + context 'when wait for rate limit is disabled' do + let(:wait_for_rate_limit_reset) { false } + + it 'raises the error limit error when requested' do + stub_request(:get, /api.github.com/) + allow(client.api).to receive(:repos).and_raise(Octokit::TooManyRequests) + + expect { client.repos }.to raise_error(Octokit::TooManyRequests) + end + end end end diff --git a/spec/migrations/20200406102120_backfill_deployment_clusters_from_deployments_spec.rb b/spec/migrations/20200406102120_backfill_deployment_clusters_from_deployments_spec.rb new file mode 100644 index 00000000000..fcb253677e1 --- /dev/null +++ b/spec/migrations/20200406102120_backfill_deployment_clusters_from_deployments_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200406102120_backfill_deployment_clusters_from_deployments.rb') + +describe BackfillDeploymentClustersFromDeployments, :migration, :sidekiq, schema: 20200227140242 do + describe '#up' do + it 'schedules BackfillDeploymentClustersFromDeployments background jobs' do + stub_const("#{described_class}::BATCH_SIZE", 2) + + namespace = table(:namespaces).create(name: 'the-namespace', path: 'the-path') + project = table(:projects).create(name: 'the-project', namespace_id: namespace.id) + environment = table(:environments).create(name: 'the-environment', project_id: project.id, slug: 'slug') + cluster = table(:clusters).create(name: 'the-cluster') + + deployment_data = { cluster_id: cluster.id, project_id: project.id, environment_id: environment.id, ref: 'abc', tag: false, sha: 'sha', status: 1 } + + # batch 1 + batch_1_begin = create_deployment(**deployment_data) + batch_1_end = create_deployment(**deployment_data) + + # value that should not be included due to default scope + create_deployment(**deployment_data, cluster_id: nil) + + # batch 2 + batch_2_begin = create_deployment(**deployment_data) + batch_2_end = create_deployment(**deployment_data) + + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + # batch 1 + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, batch_1_begin.id, batch_1_end.id) + + # batch 2 + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, batch_2_begin.id, batch_2_end.id) + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + end + end + end + + def create_deployment(**data) + @iid ||= 0 + @iid += 1 + table(:deployments).create(iid: @iid, **data) + end + end +end diff --git a/spec/requests/api/metrics/dashboard/annotations_spec.rb b/spec/requests/api/metrics/dashboard/annotations_spec.rb new file mode 100644 index 00000000000..0b51c46e474 --- /dev/null +++ b/spec/requests/api/metrics/dashboard/annotations_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Metrics::Dashboard::Annotations do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :private, :repository, namespace: user.namespace) } + let_it_be(:environment) { create(:environment, project: project) } + let(:dashboard) { 'config/prometheus/common_metrics.yml' } + let(:starting_at) { Time.now.iso8601 } + let(:ending_at) { 1.hour.from_now.iso8601 } + let(:params) { attributes_for(:metrics_dashboard_annotation, environment: environment, starting_at: starting_at, ending_at: ending_at, dashboard_path: dashboard)} + + describe 'POST /environments/:environment_id/metrics_dashboard/annotations' do + before :all do + project.add_developer(user) + end + + context 'feature flag metrics_dashboard_annotations' do + context 'is on' do + before do + stub_feature_flags(metrics_dashboard_annotations: { enabled: true, thing: project }) + end + context 'with correct permissions' do + context 'with valid parameters' do + it 'creates a new annotation', :aggregate_failures do + post api("/environments/#{environment.id}/metrics_dashboard/annotations", user), params: params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['environment_id']).to eq(environment.id) + expect(json_response['starting_at'].to_time).to eq(starting_at.to_time) + expect(json_response['ending_at'].to_time).to eq(ending_at.to_time) + expect(json_response['description']).to eq(params[:description]) + expect(json_response['dashboard_path']).to eq(dashboard) + end + end + + context 'with invalid parameters' do + it 'returns error messsage' do + post api("/environments/#{environment.id}/metrics_dashboard/annotations", user), + params: { dashboard_path: nil, starting_at: nil, description: nil } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to include({ "starting_at" => ["can't be blank"], "description" => ["can't be blank"], "dashboard_path" => ["can't be blank"] }) + end + end + + context 'with undeclared params' do + before do + params[:undeclared_param] = 'xyz' + end + it 'filters out undeclared params' do + expect(::Metrics::Dashboard::Annotations::CreateService).to receive(:new).with(user, hash_excluding(:undeclared_param)) + + post api("/environments/#{environment.id}/metrics_dashboard/annotations", user), params: params + end + end + end + + context 'without correct permissions' do + let_it_be(:guest) { create(:user) } + + before do + project.add_guest(guest) + end + + it 'returns error messsage' do + post api("/environments/#{environment.id}/metrics_dashboard/annotations", guest), params: params + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + context 'is off' do + before do + stub_feature_flags(metrics_dashboard_annotations: { enabled: false, thing: project }) + end + + it 'returns error messsage' do + post api("/environments/#{environment.id}/metrics_dashboard/annotations", user), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end +end diff --git a/spec/services/jira_import/start_import_service_spec.rb b/spec/services/jira_import/start_import_service_spec.rb index 785f8b6d792..ae0c4f63fee 100644 --- a/spec/services/jira_import/start_import_service_spec.rb +++ b/spec/services/jira_import/start_import_service_spec.rb @@ -81,6 +81,28 @@ describe JiraImport::StartImportService do expect(jira_import.jira_project_key).to eq(fake_key) expect(jira_import.user).to eq(user) end + + it 'creates jira import label' do + expect { subject }.to change { Label.count }.by(1) + end + + it 'creates jira label title with correct number' do + jira_import = subject.payload[:import_data] + + label_title = "jira-import::#{jira_import.jira_project_key}-1" + expect(jira_import.label.title).to eq(label_title) + end + + context 'when multiple jira imports for same jira project' do + let!(:jira_imports) { create_list(:jira_import_state, 3, :finished, project: project, jira_project_key: fake_key)} + + it 'creates jira label title with correct number' do + jira_import = subject.payload[:import_data] + + label_title = "jira-import::#{jira_import.jira_project_key}-4" + expect(jira_import.label.title).to eq(label_title) + end + end end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0e6078cc444..19d12a0f5cb 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -91,6 +91,10 @@ RSpec.configure do |config| match = location.match(%r{/spec/([^/]+)/}) metadata[:type] = match[1].singularize.to_sym if match end + + # Admin controller specs get auto admin mode enabled since they are + # protected by the 'EnforcesAdminAuthentication' concern + metadata[:enable_admin_mode] = true if location =~ %r{(ee)?/spec/controllers/admin/} end config.include LicenseHelpers @@ -226,7 +230,6 @@ RSpec.configure do |config| # # context 'some test in mocked dir', :do_not_mock_admin_mode do ... end admin_mode_mock_dirs = %w( - ./ee/spec/controllers ./ee/spec/elastic_integration ./ee/spec/features ./ee/spec/finders @@ -238,7 +241,6 @@ RSpec.configure do |config| ./ee/spec/services ./ee/spec/support/protected_tags ./ee/spec/support/shared_examples - ./spec/controllers ./spec/features ./spec/finders ./spec/frontend @@ -270,7 +272,7 @@ RSpec.configure do |config| # context 'some test that requires admin mode', :enable_admin_mode do ... end # # See also spec/support/helpers/admin_mode_helpers.rb - if example.metadata[:enable_admin_mode] + if example.metadata[:enable_admin_mode] && !example.metadata[:do_not_mock_admin_mode] allow_any_instance_of(Gitlab::Auth::CurrentUserMode).to receive(:admin_mode?) do |current_user_mode| current_user_mode.send(:user)&.admin? end diff --git a/spec/support/shared_contexts/navbar_structure_context.rb b/spec/support/shared_contexts/navbar_structure_context.rb index 9ffe13545f7..e4bc44c9d32 100644 --- a/spec/support/shared_contexts/navbar_structure_context.rb +++ b/spec/support/shared_contexts/navbar_structure_context.rb @@ -1,13 +1,6 @@ # frozen_string_literal: true RSpec.shared_context 'project navbar structure' do - let(:requirements_nav_item) do - { - nav_item: _('Requirements'), - nav_sub_items: [_('List')] - } - end - let(:analytics_nav_item) do { nav_item: _('Analytics'), @@ -56,7 +49,6 @@ RSpec.shared_context 'project navbar structure' do nav_item: _('Merge Requests'), nav_sub_items: [] }, - (requirements_nav_item if Gitlab.ee?), { nav_item: _('CI / CD'), nav_sub_items: [