diff options
53 files changed, 1354 insertions, 215 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index a6c8eccf0b2..ffca09a92e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.2.1 + +### Fixed (3 changes) + +- Fix for embedded metrics undefined params. !31975 +- Fix "ERR value is not an integer or out of range" errors. !32126 +- Prevent duplicated trigger action button. + +### Performance (1 change) + +- Fix Gitaly N+1 calls with listing issues/MRs via API. !31938 + + ## 12.2.0 ### Security (4 changes, 1 of them is from the community) diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb index 5ecf4f114cf..da39d64c93d 100644 --- a/app/controllers/jwt_controller.rb +++ b/app/controllers/jwt_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class JwtController < ApplicationController + skip_around_action :set_session_storage skip_before_action :authenticate_user! skip_before_action :verify_authenticity_token before_action :authenticate_project_or_user diff --git a/app/models/analytics/cycle_analytics/project_stage.rb b/app/models/analytics/cycle_analytics/project_stage.rb index 88c8cb40ccb..a312bd24e78 100644 --- a/app/models/analytics/cycle_analytics/project_stage.rb +++ b/app/models/analytics/cycle_analytics/project_stage.rb @@ -3,7 +3,12 @@ module Analytics module CycleAnalytics class ProjectStage < ApplicationRecord + include Analytics::CycleAnalytics::Stage + + validates :project, presence: true belongs_to :project + + alias_attribute :parent, :project end end end diff --git a/app/models/concerns/analytics/cycle_analytics/stage.rb b/app/models/concerns/analytics/cycle_analytics/stage.rb new file mode 100644 index 00000000000..0c603c2d5e6 --- /dev/null +++ b/app/models/concerns/analytics/cycle_analytics/stage.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Analytics + module CycleAnalytics + module Stage + extend ActiveSupport::Concern + + included do + validates :name, presence: true + validates :start_event_identifier, presence: true + validates :end_event_identifier, presence: true + validate :validate_stage_event_pairs + + enum start_event_identifier: Gitlab::Analytics::CycleAnalytics::StageEvents.to_enum, _prefix: :start_event_identifier + enum end_event_identifier: Gitlab::Analytics::CycleAnalytics::StageEvents.to_enum, _prefix: :end_event_identifier + + alias_attribute :custom_stage?, :custom + end + + def parent=(_) + raise NotImplementedError + end + + def parent + raise NotImplementedError + end + + def start_event + Gitlab::Analytics::CycleAnalytics::StageEvents[start_event_identifier].new(params_for_start_event) + end + + def end_event + Gitlab::Analytics::CycleAnalytics::StageEvents[end_event_identifier].new(params_for_end_event) + end + + def params_for_start_event + {} + end + + def params_for_end_event + {} + end + + def default_stage? + !custom + end + + # The model that is going to be queried, Issue or MergeRequest + def subject_model + start_event.object_type + end + + private + + def validate_stage_event_pairs + return if start_event_identifier.nil? || end_event_identifier.nil? + + unless pairing_rules.fetch(start_event.class, []).include?(end_event.class) + errors.add(:end_event, :not_allowed_for_the_given_start_event) + end + end + + def pairing_rules + Gitlab::Analytics::CycleAnalytics::StageEvents.pairing_rules + end + end + end +end diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 68586e7a1fd..bff5d348ca0 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -162,6 +162,14 @@ class Deployment < ApplicationRecord deployed_at&.to_time&.in_time_zone&.to_s(:medium) end + def deployed_by + # We use deployable's user if available because Ci::PlayBuildService + # does not update the deployment's user, just the one for the deployable. + # TODO: use deployment's user once https://gitlab.com/gitlab-org/gitlab-ce/issues/66442 + # is completed. + deployable&.user || user + end + private def ref_path diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb index d6466ad1cdd..8b967459173 100644 --- a/app/serializers/deployment_entity.rb +++ b/app/serializers/deployment_entity.rb @@ -21,7 +21,7 @@ class DeploymentEntity < Grape::Entity expose :deployed_at expose :tag expose :last? - expose :user, using: UserEntity + expose :deployed_by, as: :user, using: UserEntity expose :deployable do |deployment, opts| deployment.deployable.yield_self do |deployable| diff --git a/app/views/projects/deployments/_deployment.html.haml b/app/views/projects/deployments/_deployment.html.haml index a11e23b6daa..752be02443c 100644 --- a/app/views/projects/deployments/_deployment.html.haml +++ b/app/views/projects/deployments/_deployment.html.haml @@ -15,10 +15,10 @@ .flex-truncate-child = link_to [@project.namespace.becomes(Namespace), @project, deployment.deployable], class: 'build-link' do #{deployment.deployable.name} (##{deployment.deployable.id}) - - if deployment.user + - if deployment.deployed_by %div by - = user_avatar(user: deployment.user, size: 20, css_class: "mr-0 float-none") + = user_avatar(user: deployment.deployed_by, size: 20, css_class: "mr-0 float-none") .table-section.section-15{ role: 'gridcell' } .table-mobile-header{ role: 'rowheader' }= _("Created") diff --git a/changelogs/unreleased/55360-redundant-index-in-the-releases-table_v2.yml b/changelogs/unreleased/55360-redundant-index-in-the-releases-table_v2.yml new file mode 100644 index 00000000000..91a1fb5e6c9 --- /dev/null +++ b/changelogs/unreleased/55360-redundant-index-in-the-releases-table_v2.yml @@ -0,0 +1,5 @@ +--- +title: Removed redundant index on releases table +merge_request: 31487 +author: +type: removed diff --git a/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml b/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml new file mode 100644 index 00000000000..7e1eeddef32 --- /dev/null +++ b/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml @@ -0,0 +1,5 @@ +--- +title: Add optional label_id parameter to label API for PUT and DELETE +merge_request: 31804 +author: +type: changed diff --git a/changelogs/unreleased/64269-pipeline-api-fails-with-401.yml b/changelogs/unreleased/64269-pipeline-api-fails-with-401.yml new file mode 100644 index 00000000000..582339901ae --- /dev/null +++ b/changelogs/unreleased/64269-pipeline-api-fails-with-401.yml @@ -0,0 +1,5 @@ +--- +title: Read pipelines from public projects through API without an access token +merge_request: 31816 +author: +type: fixed diff --git a/changelogs/unreleased/66037-deployment-user.yml b/changelogs/unreleased/66037-deployment-user.yml new file mode 100644 index 00000000000..8a61b8145af --- /dev/null +++ b/changelogs/unreleased/66037-deployment-user.yml @@ -0,0 +1,5 @@ +--- +title: Return correct user for manual deployments +merge_request: 32004 +author: +type: fixed diff --git a/changelogs/unreleased/sh-eliminate-gitaly-nplus-one-notes.yml b/changelogs/unreleased/sh-eliminate-gitaly-nplus-one-notes.yml new file mode 100644 index 00000000000..00523f53dd7 --- /dev/null +++ b/changelogs/unreleased/sh-eliminate-gitaly-nplus-one-notes.yml @@ -0,0 +1,5 @@ +--- +title: Eliminate Gitaly N+1 queries with notes API +merge_request: 32089 +author: +type: performance diff --git a/changelogs/unreleased/sh-fix-issues-api-gitaly-nplusone.yml b/changelogs/unreleased/sh-fix-issues-api-gitaly-nplusone.yml deleted file mode 100644 index 3177cb8d18c..00000000000 --- a/changelogs/unreleased/sh-fix-issues-api-gitaly-nplusone.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix Gitaly N+1 calls with listing issues/MRs via API -merge_request: 31938 -author: -type: performance diff --git a/changelogs/unreleased/sh-revert-redis-cache-store.yml b/changelogs/unreleased/sh-revert-redis-cache-store.yml deleted file mode 100644 index 710b6e7941d..00000000000 --- a/changelogs/unreleased/sh-revert-redis-cache-store.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix "ERR value is not an integer or out of range" errors -merge_request: 32126 -author: -type: fixed diff --git a/changelogs/unreleased/tr-param-undefined-fix.yml b/changelogs/unreleased/tr-param-undefined-fix.yml deleted file mode 100644 index 0a9051485bd..00000000000 --- a/changelogs/unreleased/tr-param-undefined-fix.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix for embedded metrics undefined params -merge_request: 31975 -author: -type: fixed diff --git a/db/migrate/20190805140353_remove_rendundant_index_from_releases.rb b/db/migrate/20190805140353_remove_rendundant_index_from_releases.rb new file mode 100644 index 00000000000..fc4bc1a423b --- /dev/null +++ b/db/migrate/20190805140353_remove_rendundant_index_from_releases.rb @@ -0,0 +1,21 @@ +# 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 RemoveRendundantIndexFromReleases < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + remove_concurrent_index :releases, :project_id + end + + def down + add_concurrent_index :releases, :project_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 3f7917654cf..944d23e5365 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3015,7 +3015,6 @@ ActiveRecord::Schema.define(version: 2019_08_15_093949) do t.datetime_with_timezone "released_at", null: false t.index ["author_id"], name: "index_releases_on_author_id" t.index ["project_id", "tag"], name: "index_releases_on_project_id_and_tag" - t.index ["project_id"], name: "index_releases_on_project_id" end create_table "remote_mirrors", id: :serial, force: :cascade do |t| diff --git a/doc/api/labels.md b/doc/api/labels.md index 5db0edcf14d..fde1d861cf6 100644 --- a/doc/api/labels.md +++ b/doc/api/labels.md @@ -137,8 +137,9 @@ DELETE /projects/:id/labels | Attribute | Type | Required | Description | | --------- | ------- | -------- | --------------------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `name` | string | yes | The name of the label | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `label_id` | integer | yes (or `name`) | The id of the existing label | +| `name` | string | yes (or `label_id`) | The name of the existing label | ```bash curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/labels?name=bug" @@ -156,7 +157,8 @@ PUT /projects/:id/labels | Attribute | Type | Required | Description | | --------------- | ------- | --------------------------------- | ------------------------------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `name` | string | yes | The name of the existing label | +| `label_id` | integer | yes (or `name`) | The id of the existing label | +| `name` | string | yes (or `label_id`) | The name of the existing label | | `new_name` | string | yes if `color` is not provided | The new name of the label | | `color` | string | yes if `new_name` is not provided | The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the [CSS color names](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Color_keywords) | | `description` | string | no | The new description of the label | diff --git a/lib/api/helpers/label_helpers.rb b/lib/api/helpers/label_helpers.rb index 896b0aba52b..ec5b688dd1c 100644 --- a/lib/api/helpers/label_helpers.rb +++ b/lib/api/helpers/label_helpers.rb @@ -11,9 +11,9 @@ module API optional :description, type: String, desc: 'The description of label to be created' end - def find_label(parent, id, include_ancestor_groups: true) + def find_label(parent, id_or_title, include_ancestor_groups: true) labels = available_labels_for(parent, include_ancestor_groups: include_ancestor_groups) - label = labels.find_by_id(id) || labels.find_by_title(id) + label = labels.find_by_id(id_or_title) || labels.find_by_title(id_or_title) label || not_found!('Label') end @@ -35,12 +35,7 @@ module API priority = params.delete(:priority) label_params = declared_params(include_missing: false) - label = - if parent.is_a?(Project) - ::Labels::CreateService.new(label_params).execute(project: parent) - else - ::Labels::CreateService.new(label_params).execute(group: parent) - end + label = ::Labels::CreateService.new(label_params).execute(create_service_params(parent)) if label.persisted? if parent.is_a?(Project) @@ -56,10 +51,13 @@ module API def update_label(parent, entity) authorize! :admin_label, parent - label = find_label(parent, params[:name], include_ancestor_groups: false) + label = find_label(parent, params_id_or_title, include_ancestor_groups: false) update_priority = params.key?(:priority) priority = params.delete(:priority) + # params is used to update the label so we need to remove this field here + params.delete(:label_id) + label = ::Labels::UpdateService.new(declared_params(include_missing: false)).execute(label) render_validation_error!(label) unless label.valid? @@ -77,10 +75,24 @@ module API def delete_label(parent) authorize! :admin_label, parent - label = find_label(parent, params[:name], include_ancestor_groups: false) + label = find_label(parent, params_id_or_title, include_ancestor_groups: false) destroy_conditionally!(label) end + + def params_id_or_title + @params_id_or_title ||= params[:label_id] || params[:name] + end + + def create_service_params(parent) + if parent.is_a?(Project) + { project: parent } + elsif parent.is_a?(Group) + { group: parent } + else + raise TypeError, 'Parent type is not supported' + end + end end end end diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index 6bf9057fad7..b2bf6bf7417 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -3,6 +3,8 @@ module API module Helpers module NotesHelpers + include ::RendersNotes + def self.noteable_types # This is a method instead of a constant, allowing EE to more easily # extend it. diff --git a/lib/api/labels.rb b/lib/api/labels.rb index c183198d3c6..83d645ca07a 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -38,11 +38,13 @@ module API success Entities::ProjectLabel end params do - requires :name, type: String, desc: 'The name of the label to be updated' + optional :label_id, type: Integer, desc: 'The id of the label to be updated' + optional :name, type: String, desc: 'The name of the label to be updated' optional :new_name, type: String, desc: 'The new name of the label' optional :color, type: String, desc: "The new color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the allowed CSS color names" optional :description, type: String, desc: 'The new description of label' optional :priority, type: Integer, desc: 'The priority of the label', allow_blank: true + exactly_one_of :label_id, :name at_least_one_of :new_name, :color, :description, :priority end put ':id/labels' do @@ -53,7 +55,9 @@ module API success Entities::ProjectLabel end params do - requires :name, type: String, desc: 'The name of the label to be deleted' + optional :label_id, type: Integer, desc: 'The id of the label to be deleted' + optional :name, type: String, desc: 'The name of the label to be deleted' + exactly_one_of :label_id, :name end delete ':id/labels' do delete_label(user_project) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 9381f045144..84563d66ee8 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -36,12 +36,13 @@ module API # page can have less elements than :per_page even if # there's more than one page. raw_notes = noteable.notes.with_metadata.reorder(order_options_with_tie_breaker) - notes = - # paginate() only works with a relation. This could lead to a - # mismatch between the pagination headers info and the actual notes - # array returned, but this is really a edge-case. - paginate(raw_notes) - .reject { |n| n.cross_reference_not_visible_for?(current_user) } + + # paginate() only works with a relation. This could lead to a + # mismatch between the pagination headers info and the actual notes + # array returned, but this is really a edge-case. + notes = paginate(raw_notes) + notes = prepare_notes_for_rendering(notes) + notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } present notes, with: Entities::Note end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 667bf1ec801..9e888368e7b 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -4,7 +4,7 @@ module API class Pipelines < Grape::API include PaginationParams - before { authenticate! } + before { authenticate_non_get! } params do requires :id, type: String, desc: 'The project ID' @@ -32,6 +32,7 @@ module API end get ':id/pipelines' do authorize! :read_pipeline, user_project + authorize! :read_build, user_project pipelines = PipelinesFinder.new(user_project, current_user, params).execute present paginate(pipelines), with: Entities::PipelineBasic diff --git a/lib/feature/gitaly.rb b/lib/feature/gitaly.rb index be397a478cd..656becbffd3 100644 --- a/lib/feature/gitaly.rb +++ b/lib/feature/gitaly.rb @@ -10,6 +10,7 @@ class Feature get_commit_signatures cache_invalidator inforef_uploadpack_cache + get_all_lfs_pointers_go ].freeze DEFAULT_ON_FLAGS = Set.new([]).freeze diff --git a/lib/gitlab/analytics/cycle_analytics/default_stages.rb b/lib/gitlab/analytics/cycle_analytics/default_stages.rb new file mode 100644 index 00000000000..286c393005f --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/default_stages.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +# This module represents the default Cycle Analytics stages that are currently provided by CE +# Each method returns a hash that can be used to build a new stage object. +# +# Example: +# +# params = Gitlab::Analytics::CycleAnalytics::DefaultStages.params_for_issue_stage +# Analytics::CycleAnalytics::ProjectStage.new(params) +module Gitlab + module Analytics + module CycleAnalytics + module DefaultStages + def self.all + [ + params_for_issue_stage, + params_for_plan_stage, + params_for_code_stage, + params_for_test_stage, + params_for_review_stage, + params_for_staging_stage, + params_for_production_stage + ] + end + + def self.params_for_issue_stage + { + name: 'issue', + custom: false, # this stage won't be customizable, we provide it as it is + relative_position: 1, # when opening the CycleAnalytics page in CE, this stage will be the first item + start_event_identifier: :issue_created, # IssueCreated class is used as start event + end_event_identifier: :issue_stage_end # IssueStageEnd class is used as end event + } + end + + def self.params_for_plan_stage + { + name: 'plan', + custom: false, + relative_position: 2, + start_event_identifier: :plan_stage_start, + end_event_identifier: :issue_first_mentioned_in_commit + } + end + + def self.params_for_code_stage + { + name: 'code', + custom: false, + relative_position: 3, + start_event_identifier: :code_stage_start, + end_event_identifier: :merge_request_created + } + end + + def self.params_for_test_stage + { + name: 'test', + custom: false, + relative_position: 4, + start_event_identifier: :merge_request_last_build_started, + end_event_identifier: :merge_request_last_build_finished + } + end + + def self.params_for_review_stage + { + name: 'review', + custom: false, + relative_position: 5, + start_event_identifier: :merge_request_created, + end_event_identifier: :merge_request_merged + } + end + + def self.params_for_staging_stage + { + name: 'staging', + custom: false, + relative_position: 6, + start_event_identifier: :merge_request_merged, + end_event_identifier: :merge_request_first_deployed_to_production + } + end + + def self.params_for_production_stage + { + name: 'production', + custom: false, + relative_position: 7, + start_event_identifier: :merge_request_merged, + end_event_identifier: :merge_request_first_deployed_to_production + } + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events.rb b/lib/gitlab/analytics/cycle_analytics/stage_events.rb new file mode 100644 index 00000000000..d21f344f483 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + # Convention: + # Issue: < 100 + # MergeRequest: >= 100 && < 1000 + # Custom events for default stages: >= 1000 (legacy) + ENUM_MAPPING = { + StageEvents::IssueCreated => 1, + StageEvents::IssueFirstMentionedInCommit => 2, + StageEvents::MergeRequestCreated => 100, + StageEvents::MergeRequestFirstDeployedToProduction => 101, + StageEvents::MergeRequestLastBuildFinished => 102, + StageEvents::MergeRequestLastBuildStarted => 103, + StageEvents::MergeRequestMerged => 104, + StageEvents::CodeStageStart => 1_000, + StageEvents::IssueStageEnd => 1_001, + StageEvents::PlanStageStart => 1_002 + }.freeze + + EVENTS = ENUM_MAPPING.keys.freeze + + # Defines which start_event and end_event pairs are allowed + PAIRING_RULES = { + StageEvents::PlanStageStart => [ + StageEvents::IssueFirstMentionedInCommit + ], + StageEvents::CodeStageStart => [ + StageEvents::MergeRequestCreated + ], + StageEvents::IssueCreated => [ + StageEvents::IssueStageEnd + ], + StageEvents::MergeRequestCreated => [ + StageEvents::MergeRequestMerged + ], + StageEvents::MergeRequestLastBuildStarted => [ + StageEvents::MergeRequestLastBuildFinished + ], + StageEvents::MergeRequestMerged => [ + StageEvents::MergeRequestFirstDeployedToProduction + ] + }.freeze + + def [](identifier) + events.find { |e| e.identifier.to_s.eql?(identifier.to_s) } || raise(KeyError) + end + + # hash for defining ActiveRecord enum: identifier => number + def to_enum + ENUM_MAPPING.each_with_object({}) { |(k, v), hash| hash[k.identifier] = v } + end + + # will be overridden in EE with custom events + def pairing_rules + PAIRING_RULES + end + + # will be overridden in EE with custom events + def events + EVENTS + end + + module_function :[], :to_enum, :pairing_rules, :events + end + end + end +end 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 new file mode 100644 index 00000000000..ff9c8a79225 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/code_stage_start.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class CodeStageStart < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Issue first mentioned in a commit") + end + + def self.identifier + :code_stage_start + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created.rb new file mode 100644 index 00000000000..a601c9797f8 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_created.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class IssueCreated < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Issue created") + end + + def self.identifier + :issue_created + end + + def object_type + Issue + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit.rb new file mode 100644 index 00000000000..7424043ef7b --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_first_mentioned_in_commit.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class IssueFirstMentionedInCommit < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Issue first mentioned in a commit") + end + + def self.identifier + :issue_first_mentioned_in_commit + end + + def object_type + Issue + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb new file mode 100644 index 00000000000..ceb229c552f --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class IssueStageEnd < SimpleStageEvent + def self.name + PlanStageStart.name + end + + def self.identifier + :issue_stage_end + end + + def object_type + Issue + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created.rb new file mode 100644 index 00000000000..8be00831b4f --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_created.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class MergeRequestCreated < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Merge request created") + end + + def self.identifier + :merge_request_created + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb new file mode 100644 index 00000000000..6d7a2c023ff --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class MergeRequestFirstDeployedToProduction < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Merge request first deployed to production") + end + + def self.identifier + :merge_request_first_deployed_to_production + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished.rb new file mode 100644 index 00000000000..12d82fe2c62 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_finished.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class MergeRequestLastBuildFinished < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Merge request last build finish time") + end + + def self.identifier + :merge_request_last_build_finished + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started.rb new file mode 100644 index 00000000000..9e749b0fdfa --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_last_build_started.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class MergeRequestLastBuildStarted < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Merge request last build start time") + end + + def self.identifier + :merge_request_last_build_started + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged.rb new file mode 100644 index 00000000000..bbfb5d12992 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_merged.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class MergeRequestMerged < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Merge request merged") + end + + def self.identifier + :merge_request_merged + end + + def object_type + MergeRequest + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start.rb new file mode 100644 index 00000000000..803317d8b55 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/plan_stage_start.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + class PlanStageStart < SimpleStageEvent + def self.name + s_("CycleAnalyticsEvent|Issue first associated with a milestone or issue first added to a board") + end + + def self.identifier + :plan_stage_start + end + + def object_type + Issue + end + end + end + end + end +end diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/simple_stage_event.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/simple_stage_event.rb new file mode 100644 index 00000000000..253c489d822 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/simple_stage_event.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + # Represents a simple event that usually refers to one database column and does not require additional user input + class SimpleStageEvent < StageEvent + end + 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 new file mode 100644 index 00000000000..a55eee048c2 --- /dev/null +++ b/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Gitlab + module Analytics + module CycleAnalytics + module StageEvents + # Base class for expressing an event that can be used for a stage. + class StageEvent + def initialize(params) + @params = params + end + + def self.name + raise NotImplementedError + end + + def self.identifier + raise NotImplementedError + end + + def object_type + raise NotImplementedError + end + end + end + end + end +end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index e6cbfb00f60..201db9fec26 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -406,7 +406,8 @@ module Gitlab def self.filesystem_id(storage) response = Gitlab::GitalyClient::ServerService.new(storage).info storage_status = response.storage_statuses.find { |status| status.storage_name == storage } - storage_status.filesystem_id + + storage_status&.filesystem_id end def self.filesystem_id_from_disk(storage) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4f3c8e8046d..d57ab4bf66f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3602,6 +3602,30 @@ msgstr "" msgid "Cycle Analytics gives an overview of how much time it takes to go from idea to production in your project." msgstr "" +msgid "CycleAnalyticsEvent|Issue created" +msgstr "" + +msgid "CycleAnalyticsEvent|Issue first associated with a milestone or issue first added to a board" +msgstr "" + +msgid "CycleAnalyticsEvent|Issue first mentioned in a commit" +msgstr "" + +msgid "CycleAnalyticsEvent|Merge request created" +msgstr "" + +msgid "CycleAnalyticsEvent|Merge request first deployed to production" +msgstr "" + +msgid "CycleAnalyticsEvent|Merge request last build finish time" +msgstr "" + +msgid "CycleAnalyticsEvent|Merge request last build start time" +msgstr "" + +msgid "CycleAnalyticsEvent|Merge request merged" +msgstr "" + msgid "CycleAnalyticsStage|Code" msgstr "" diff --git a/spec/lib/api/helpers/label_helpers_spec.rb b/spec/lib/api/helpers/label_helpers_spec.rb new file mode 100644 index 00000000000..138e9a22d70 --- /dev/null +++ b/spec/lib/api/helpers/label_helpers_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Helpers::LabelHelpers do + describe 'create_service_params' do + let(:label_helper) do + Class.new do + include API::Helpers::LabelHelpers + end.new + end + + context 'when a project is given' do + it 'returns the expected params' do + project = create(:project) + expect(label_helper.create_service_params(project)).to eq({ project: project }) + end + end + + context 'when a group is given' do + it 'returns the expected params' do + group = create(:group) + expect(label_helper.create_service_params(group)).to eq({ group: group }) + end + end + + context 'when something else is given' do + it 'raises a type error' do + expect { label_helper.create_service_params(Class.new) }.to raise_error(TypeError) + end + end + end +end diff --git a/spec/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event_spec.rb new file mode 100644 index 00000000000..29f4be76a65 --- /dev/null +++ b/spec/lib/gitlab/analytics/cycle_analytics/stage_events/stage_event_spec.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Analytics::CycleAnalytics::StageEvents::StageEvent do + it { expect(described_class).to respond_to(:name) } + it { expect(described_class).to respond_to(:identifier) } + + it { expect(described_class.new({})).to respond_to(:object_type) } +end diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index e9fb6c0125c..99d563e03ec 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -27,6 +27,16 @@ describe Gitlab::GitalyClient do end end + describe '.filesystem_id' do + it 'returns an empty string when the storage is not found in the response' do + response = double("response") + allow(response).to receive(:storage_statuses).and_return([]) + allow_any_instance_of(Gitlab::GitalyClient::ServerService).to receive(:info).and_return(response) + + expect(described_class.filesystem_id('default')).to eq(nil) + end + end + describe '.stub_class' do it 'returns the gRPC health check stub' do expect(described_class.stub_class(:health_check)).to eq(::Grpc::Health::V1::Health::Stub) diff --git a/spec/models/analytics/cycle_analytics/project_stage_spec.rb b/spec/models/analytics/cycle_analytics/project_stage_spec.rb index 4e3923e82b1..83d6ff754c5 100644 --- a/spec/models/analytics/cycle_analytics/project_stage_spec.rb +++ b/spec/models/analytics/cycle_analytics/project_stage_spec.rb @@ -6,4 +6,18 @@ describe Analytics::CycleAnalytics::ProjectStage do describe 'associations' do it { is_expected.to belong_to(:project) } end + + it 'default stages must be valid' do + project = create(:project) + + Gitlab::Analytics::CycleAnalytics::DefaultStages.all.each do |params| + stage = described_class.new(params.merge(project: project)) + expect(stage).to be_valid + end + end + + it_behaves_like "cycle analytics stage" do + let(:parent) { create(:project) } + let(:parent_name) { :project } + end end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index d4e631f109b..51ed8e9421b 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -322,4 +322,30 @@ describe Deployment do end end end + + describe '#deployed_by' do + it 'returns the deployment user if there is no deployable' do + deployment_user = create(:user) + deployment = create(:deployment, deployable: nil, user: deployment_user) + + expect(deployment.deployed_by).to eq(deployment_user) + end + + it 'returns the deployment user if the deployable have no user' do + deployment_user = create(:user) + build = create(:ci_build, user: nil) + deployment = create(:deployment, deployable: build, user: deployment_user) + + expect(deployment.deployed_by).to eq(deployment_user) + end + + it 'returns the deployable user if there is one' do + build_user = create(:user) + deployment_user = create(:user) + build = create(:ci_build, user: build_user) + deployment = create(:deployment, deployable: build, user: deployment_user) + + expect(deployment.deployed_by).to eq(build_user) + end + end end diff --git a/spec/requests/api/discussions_spec.rb b/spec/requests/api/discussions_spec.rb index ef09c6effbb..0420201efe3 100644 --- a/spec/requests/api/discussions_spec.rb +++ b/spec/requests/api/discussions_spec.rb @@ -9,59 +9,11 @@ describe API::Discussions do project.add_developer(user) end - context 'with cross-reference system notes', :request_store do - let(:merge_request) { create(:merge_request) } - let(:project) { merge_request.project } - let(:new_merge_request) { create(:merge_request) } - let(:commit) { new_merge_request.project.commit } - let!(:note) { create(:system_note, noteable: merge_request, project: project, note: cross_reference) } - let!(:note_metadata) { create(:system_note_metadata, note: note, action: 'cross_reference') } - let(:cross_reference) { "test commit #{commit.to_reference(project)}" } - let(:pat) { create(:personal_access_token, user: user) } - + context 'when discussions have cross-reference system notes' do let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/discussions" } + let(:notes_in_response) { json_response.first['notes'] } - before do - project.add_developer(user) - new_merge_request.project.add_developer(user) - end - - it 'returns only the note that the user should see' do - hidden_merge_request = create(:merge_request) - new_cross_reference = "test commit #{hidden_merge_request.project.commit}" - new_note = create(:system_note, noteable: merge_request, project: project, note: new_cross_reference) - create(:system_note_metadata, note: new_note, action: 'cross_reference') - - get api(url, user, personal_access_token: pat) - expect(response).to have_gitlab_http_status(200) - expect(json_response.count).to eq(1) - expect(json_response.first['notes'].count).to eq(1) - - parsed_note = json_response.first['notes'].first - expect(parsed_note['id']).to eq(note.id) - expect(parsed_note['body']).to eq(cross_reference) - expect(parsed_note['system']).to be true - end - - it 'avoids Git calls and N+1 SQL queries' do - expect_any_instance_of(Repository).not_to receive(:find_commit).with(commit.id) - - control = ActiveRecord::QueryRecorder.new do - get api(url, user, personal_access_token: pat) - end - - expect(response).to have_gitlab_http_status(200) - - RequestStore.clear! - - new_note = create(:system_note, noteable: merge_request, project: project, note: cross_reference) - create(:system_note_metadata, note: new_note, action: 'cross_reference') - - RequestStore.clear! - - expect { get api(url, user, personal_access_token: pat) }.not_to exceed_query_limit(control) - expect(response).to have_gitlab_http_status(200) - end + it_behaves_like 'with cross-reference system notes' end context 'when noteable is an Issue' do diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index ad0974f55a3..f6640fe41d0 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -6,6 +6,180 @@ describe API::Labels do let!(:label1) { create(:label, title: 'label1', project: project) } let!(:priority_label) { create(:label, title: 'bug', project: project, priority: 3) } + shared_examples 'label update API' do + it 'returns 200 if name is changed' do + request_params = { + new_name: 'New Label' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq('New Label') + expect(json_response['color']).to eq(label1.color) + end + + it 'returns 200 if colors is changed' do + request_params = { + color: '#FFFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq(label1.name) + expect(json_response['color']).to eq('#FFFFFF') + end + + it 'returns 200 if a priority is added' do + request_params = { + priority: 3 + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['name']).to eq(label1.name) + expect(json_response['priority']).to eq(3) + end + + it 'returns 400 if no new parameters given' do + put api("/projects/#{project.id}/labels", user), params: spec_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\ + 'at least one parameter must be provided') + end + + it 'returns 400 when color code is too short' do + request_params = { + color: '#FF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['color']).to eq(['must be a valid color code']) + end + + it 'returns 400 for too long color code' do + request_params = { + color: '#FFAAFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['color']).to eq(['must be a valid color code']) + end + + it 'returns 400 for invalid priority' do + request_params = { + priority: 'foo' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns 200 if name and colors and description are changed' do + request_params = { + new_name: 'New Label', + color: '#FFFFFF', + description: 'test' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq('New Label') + expect(json_response['color']).to eq('#FFFFFF') + expect(json_response['description']).to eq('test') + end + + it 'returns 400 for invalid name' do + request_params = { + new_name: ',', + color: '#FFFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['title']).to eq(['is invalid']) + end + + it 'returns 200 if description is changed' do + request_params = { + description: 'test' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['description']).to eq('test') + end + + it 'returns 200 if priority is changed' do + request_params = { + priority: 10 + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['priority']).to eq(10) + end + + it 'returns 200 if a priority is removed' do + label = find_by_spec_params(spec_params) + expect(label).not_to be_nil + + label.priorities.create(project: label.project, priority: 1) + label.save! + + request_params = { + priority: nil + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['priority']).to be_nil + end + + def find_by_spec_params(params) + if params.key?(:label_id) + Label.find(params[:label_id]) + else + Label.find_by(name: params[:name]) + end + end + end + + shared_examples 'label delete API' do + it 'returns 204 for existing label' do + delete api("/projects/#{project.id}/labels", user), params: spec_params + + expect(response).to have_gitlab_http_status(204) + end + end + before do project.add_maintainer(user) end @@ -208,174 +382,92 @@ describe API::Labels do end describe 'DELETE /projects/:id/labels' do - it 'returns 204 for existing label' do - delete api("/projects/#{project.id}/labels", user), params: { name: 'label1' } + it_behaves_like 'label delete API' do + let(:spec_params) { { name: 'label1' } } + end - expect(response).to have_gitlab_http_status(204) + it_behaves_like 'label delete API' do + let(:spec_params) { { label_id: label1.id } } end it 'returns 404 for non existing label' do delete api("/projects/#{project.id}/labels", user), params: { name: 'label2' } + expect(response).to have_gitlab_http_status(404) expect(json_response['message']).to eq('404 Label Not Found') end it 'returns 400 for wrong parameters' do delete api("/projects/#{project.id}/labels", user) - expect(response).to have_gitlab_http_status(400) - end - - it_behaves_like '412 response' do - let(:request) { api("/projects/#{project.id}/labels", user) } - let(:params) { { name: 'label1' } } - end - end - describe 'PUT /projects/:id/labels' do - it 'returns 200 if name and colors and description are changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - new_name: 'New Label', - color: '#FFFFFF', - description: 'test' - } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq('New Label') - expect(json_response['color']).to eq('#FFFFFF') - expect(json_response['description']).to eq('test') + expect(response).to have_gitlab_http_status(400) end - it 'returns 200 if name is changed' do - put api("/projects/#{project.id}/labels", user), + it 'fails if label_id and name are given in params' do + delete api("/projects/#{project.id}/labels", user), params: { - name: 'label1', - new_name: 'New Label' + label_id: label1.id, + name: priority_label.name } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq('New Label') - expect(json_response['color']).to eq(label1.color) - end - it 'returns 200 if colors is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - color: '#FFFFFF' - } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq(label1.name) - expect(json_response['color']).to eq('#FFFFFF') + expect(response).to have_gitlab_http_status(400) end - it 'returns 200 if description is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'bug', - description: 'test' - } - - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['description']).to eq('test') - expect(json_response['priority']).to eq(3) + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/labels", user) } + let(:params) { { name: 'label1' } } end + end - it 'returns 200 if priority is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'bug', - priority: 10 - } - - expect(response.status).to eq(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['priority']).to eq(10) + describe 'PUT /projects/:id/labels' do + context 'when using name' do + it_behaves_like 'label update API' do + let(:spec_params) { { name: 'label1' } } + let(:expected_response_label_id) { label1.id } + end end - it 'returns 200 if a priority is added' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - priority: 3 - } - - expect(response.status).to eq(200) - expect(json_response['name']).to eq(label1.name) - expect(json_response['priority']).to eq(3) + context 'when using label_id' do + it_behaves_like 'label update API' do + let(:spec_params) { { label_id: label1.id } } + let(:expected_response_label_id) { label1.id } + end end - it 'returns 200 if the priority is removed' do + it 'returns 404 if label does not exist' do put api("/projects/#{project.id}/labels", user), params: { - name: priority_label.name, - priority: nil + name: 'label2', + new_name: 'label3' } - expect(response.status).to eq(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['priority']).to be_nil + expect(response).to have_gitlab_http_status(404) end - it 'returns 404 if label does not exist' do + it 'returns 404 if label by id does not exist' do put api("/projects/#{project.id}/labels", user), params: { - name: 'label2', + label_id: 0, new_name: 'label3' } + expect(response).to have_gitlab_http_status(404) end - it 'returns 400 if no label name given' do + it 'returns 400 if no label name and id is given' do put api("/projects/#{project.id}/labels", user), params: { new_name: 'label2' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['error']).to eq('name is missing') - end - - it 'returns 400 if no new parameters given' do - put api("/projects/#{project.id}/labels", user), params: { name: 'label1' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\ - 'at least one parameter must be provided') - end - it 'returns 400 for invalid name' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - new_name: ',', - color: '#FFFFFF' - } expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['title']).to eq(['is invalid']) + expect(json_response['error']).to eq('label_id, name are missing, exactly one parameter must be provided') end - it 'returns 400 when color code is too short' do + it 'fails if label_id and name are given in params' do put api("/projects/#{project.id}/labels", user), params: { - name: 'label1', - color: '#FF' + label_id: label1.id, + name: priority_label.name, + new_name: 'New Label' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['color']).to eq(['must be a valid color code']) - end - - it 'returns 400 for too long color code' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - color: '#FFAAFFFF' - } - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['color']).to eq(['must be a valid color code']) - end - - it 'returns 400 for invalid priority' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - priority: 'foo' - } expect(response).to have_gitlab_http_status(400) end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 424f0a82e43..6c1e30791d2 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -9,6 +9,13 @@ describe API::Notes do project.add_reporter(user) end + context 'when there are cross-reference system notes' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes" } + let(:notes_in_response) { json_response } + + it_behaves_like 'with cross-reference system notes' + end + context "when noteable is an Issue" do let!(:issue) { create(:issue, project: project, author: user) } let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) } diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 35b3dd219f7..174b3214d13 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -17,6 +17,8 @@ describe API::Pipelines do end describe 'GET /projects/:id/pipelines ' do + it_behaves_like 'pipelines visibility table' + context 'authorized user' do it 'returns project pipelines' do get api("/projects/#{project.id}/pipelines", user) @@ -401,6 +403,15 @@ describe API::Pipelines do end describe 'GET /projects/:id/pipelines/:pipeline_id' do + it_behaves_like 'pipelines visibility table' do + let(:pipelines_api_path) do + "/projects/#{project.id}/pipelines/#{pipeline.id}" + end + + let(:api_response) { response_status == 200 ? response : json_response } + let(:response_200) { match_response_schema('public_api/v4/pipeline/detail') } + end + context 'authorized user' do it 'exposes known attributes' do get api("/projects/#{project.id}/pipelines/#{pipeline.id}", user) diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb index bba473f1c20..8b2c698fee1 100644 --- a/spec/requests/jwt_controller_spec.rb +++ b/spec/requests/jwt_controller_spec.rb @@ -108,6 +108,14 @@ describe JwtController do end end end + + it 'does not cause session based checks to be activated' do + expect(Gitlab::Session).not_to receive(:with_session) + + get '/jwt/auth', params: parameters, headers: headers + + expect(response).to have_gitlab_http_status(200) + end end context 'using invalid login' do diff --git a/spec/support/shared_examples/cycle_analytics_stage_examples.rb b/spec/support/shared_examples/cycle_analytics_stage_examples.rb new file mode 100644 index 00000000000..151f5325e84 --- /dev/null +++ b/spec/support/shared_examples/cycle_analytics_stage_examples.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +shared_examples_for 'cycle analytics stage' do + let(:valid_params) do + { + name: 'My Stage', + parent: parent, + start_event_identifier: :merge_request_created, + end_event_identifier: :merge_request_merged + } + end + + describe 'validation' do + it 'is valid' do + expect(described_class.new(valid_params)).to be_valid + end + + it 'validates presence of parent' do + stage = described_class.new(valid_params.except(:parent)) + + expect(stage).not_to be_valid + expect(stage.errors.details[parent_name]).to eq([{ error: :blank }]) + end + + it 'validates presence of start_event_identifier' do + stage = described_class.new(valid_params.except(:start_event_identifier)) + + expect(stage).not_to be_valid + expect(stage.errors.details[:start_event_identifier]).to eq([{ error: :blank }]) + end + + it 'validates presence of end_event_identifier' do + stage = described_class.new(valid_params.except(:end_event_identifier)) + + expect(stage).not_to be_valid + expect(stage.errors.details[:end_event_identifier]).to eq([{ error: :blank }]) + end + + it 'is invalid when end_event is not allowed for the given start_event' do + invalid_params = valid_params.merge( + start_event_identifier: :merge_request_merged, + end_event_identifier: :merge_request_created + ) + stage = described_class.new(invalid_params) + + expect(stage).not_to be_valid + expect(stage.errors.details[:end_event]).to eq([{ error: :not_allowed_for_the_given_start_event }]) + end + end + + describe '#subject_model' do + it 'infers the model from the start event' do + stage = described_class.new(valid_params) + + expect(stage.subject_model).to eq(MergeRequest) + end + end + + describe '#start_event' do + it 'builds start_event object based on start_event_identifier' do + stage = described_class.new(start_event_identifier: 'merge_request_created') + + expect(stage.start_event).to be_a_kind_of(Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestCreated) + end + end + + describe '#end_event' do + it 'builds end_event object based on end_event_identifier' do + stage = described_class.new(end_event_identifier: 'merge_request_merged') + + expect(stage.end_event).to be_a_kind_of(Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestMerged) + end + end +end diff --git a/spec/support/shared_examples/requests/api/discussions.rb b/spec/support/shared_examples/requests/api/discussions.rb index fc72287f265..a36bc2dc9b5 100644 --- a/spec/support/shared_examples/requests/api/discussions.rb +++ b/spec/support/shared_examples/requests/api/discussions.rb @@ -1,5 +1,59 @@ # frozen_string_literal: true +shared_examples 'with cross-reference system notes' do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + let(:new_merge_request) { create(:merge_request) } + let(:commit) { new_merge_request.project.commit } + let!(:note) { create(:system_note, noteable: merge_request, project: project, note: cross_reference) } + let!(:note_metadata) { create(:system_note_metadata, note: note, action: 'cross_reference') } + let(:cross_reference) { "test commit #{commit.to_reference(project)}" } + let(:pat) { create(:personal_access_token, user: user) } + + before do + project.add_developer(user) + new_merge_request.project.add_developer(user) + + hidden_merge_request = create(:merge_request) + new_cross_reference = "test commit #{hidden_merge_request.project.commit}" + new_note = create(:system_note, noteable: merge_request, project: project, note: new_cross_reference) + create(:system_note_metadata, note: new_note, action: 'cross_reference') + end + + it 'returns only the note that the user should see' do + get api(url, user, personal_access_token: pat) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.count).to eq(1) + expect(notes_in_response.count).to eq(1) + + parsed_note = notes_in_response.first + expect(parsed_note['id']).to eq(note.id) + expect(parsed_note['body']).to eq(cross_reference) + expect(parsed_note['system']).to be true + end + + it 'avoids Git calls and N+1 SQL queries', :request_store do + expect_any_instance_of(Repository).not_to receive(:find_commit).with(commit.id) + + control = ActiveRecord::QueryRecorder.new do + get api(url, user, personal_access_token: pat) + end + + expect(response).to have_gitlab_http_status(200) + + RequestStore.clear! + + new_note = create(:system_note, noteable: merge_request, project: project, note: cross_reference) + create(:system_note_metadata, note: new_note, action: 'cross_reference') + + RequestStore.clear! + + expect { get api(url, user, personal_access_token: pat) }.not_to exceed_query_limit(control) + expect(response).to have_gitlab_http_status(200) + end +end + shared_examples 'discussions API' do |parent_type, noteable_type, id_name, can_reply_to_individual_notes: false| describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do it "returns an array of discussions" do diff --git a/spec/support/shared_examples/requests/api/pipelines/visibility_table_examples.rb b/spec/support/shared_examples/requests/api/pipelines/visibility_table_examples.rb new file mode 100644 index 00000000000..dfd07176b1c --- /dev/null +++ b/spec/support/shared_examples/requests/api/pipelines/visibility_table_examples.rb @@ -0,0 +1,235 @@ +# frozen_string_literal: true + +shared_examples 'pipelines visibility table' do + using RSpec::Parameterized::TableSyntax + + let(:ci_user) { create(:user) } + let(:api_user) { user_role && ci_user } + + let(:pipelines_api_path) do + "/projects/#{project.id}/pipelines" + end + + let(:response_200) do + a_collection_containing_exactly( + a_hash_including('sha', 'ref', 'status', 'web_url', 'id' => pipeline.id) + ) + end + + let(:response_40x) do + a_hash_including('message') + end + + let(:expected_response) do + if response_status == 200 + response_200 + else + response_40x + end + end + + let(:api_response) { json_response } + + let(:visibility_levels) do + { + private: Gitlab::VisibilityLevel::PRIVATE, + internal: Gitlab::VisibilityLevel::INTERNAL, + public: Gitlab::VisibilityLevel::PUBLIC + } + end + + let(:builds_access_levels) do + { + enabled: ProjectFeature::ENABLED, + private: ProjectFeature::PRIVATE + } + end + + let(:project_attributes) do + { + visibility_level: visibility_levels[visibility_level], + public_builds: public_builds + } + end + + let(:project_feature_attributes) do + { + builds_access_level: builds_access_levels[builds_access_level] + } + end + + where(:visibility_level, :builds_access_level, :public_builds, :is_admin, :user_role, :response_status) do + :private | :enabled | true | true | :non_member | 200 + :private | :enabled | true | true | :guest | 200 + :private | :enabled | true | true | :reporter | 200 + :private | :enabled | true | true | :developer | 200 + :private | :enabled | true | true | :maintainer | 200 + + :private | :enabled | true | false | nil | 404 + :private | :enabled | true | false | :non_member | 404 + :private | :enabled | true | false | :guest | 200 + :private | :enabled | true | false | :reporter | 200 + :private | :enabled | true | false | :developer | 200 + :private | :enabled | true | false | :maintainer | 200 + + :private | :enabled | false | true | :non_member | 200 + :private | :enabled | false | true | :guest | 200 + :private | :enabled | false | true | :reporter | 200 + :private | :enabled | false | true | :developer | 200 + :private | :enabled | false | true | :maintainer | 200 + + :private | :enabled | false | false | nil | 404 + :private | :enabled | false | false | :non_member | 404 + :private | :enabled | false | false | :guest | 403 + :private | :enabled | false | false | :reporter | 200 + :private | :enabled | false | false | :developer | 200 + :private | :enabled | false | false | :maintainer | 200 + + :private | :private | true | true | :non_member | 200 + :private | :private | true | true | :guest | 200 + :private | :private | true | true | :reporter | 200 + :private | :private | true | true | :developer | 200 + :private | :private | true | true | :maintainer | 200 + + :private | :private | true | false | nil | 404 + :private | :private | true | false | :non_member | 404 + :private | :private | true | false | :guest | 200 + :private | :private | true | false | :reporter | 200 + :private | :private | true | false | :developer | 200 + :private | :private | true | false | :maintainer | 200 + + :private | :private | false | true | :non_member | 200 + :private | :private | false | true | :guest | 200 + :private | :private | false | true | :reporter | 200 + :private | :private | false | true | :developer | 200 + :private | :private | false | true | :maintainer | 200 + + :private | :private | false | false | nil | 404 + :private | :private | false | false | :non_member | 404 + :private | :private | false | false | :guest | 403 + :private | :private | false | false | :reporter | 200 + :private | :private | false | false | :developer | 200 + :private | :private | false | false | :maintainer | 200 + + :internal | :enabled | true | true | :non_member | 200 + :internal | :enabled | true | true | :guest | 200 + :internal | :enabled | true | true | :reporter | 200 + :internal | :enabled | true | true | :developer | 200 + :internal | :enabled | true | true | :maintainer | 200 + + :internal | :enabled | true | false | nil | 404 + :internal | :enabled | true | false | :non_member | 200 + :internal | :enabled | true | false | :guest | 200 + :internal | :enabled | true | false | :reporter | 200 + :internal | :enabled | true | false | :developer | 200 + :internal | :enabled | true | false | :maintainer | 200 + + :internal | :enabled | false | true | :non_member | 200 + :internal | :enabled | false | true | :guest | 200 + :internal | :enabled | false | true | :reporter | 200 + :internal | :enabled | false | true | :developer | 200 + :internal | :enabled | false | true | :maintainer | 200 + + :internal | :enabled | false | false | nil | 404 + :internal | :enabled | false | false | :non_member | 403 + :internal | :enabled | false | false | :guest | 403 + :internal | :enabled | false | false | :reporter | 200 + :internal | :enabled | false | false | :developer | 200 + :internal | :enabled | false | false | :maintainer | 200 + + :internal | :private | true | true | :non_member | 200 + :internal | :private | true | true | :guest | 200 + :internal | :private | true | true | :reporter | 200 + :internal | :private | true | true | :developer | 200 + :internal | :private | true | true | :maintainer | 200 + + :internal | :private | true | false | nil | 404 + :internal | :private | true | false | :non_member | 403 + :internal | :private | true | false | :guest | 200 + :internal | :private | true | false | :reporter | 200 + :internal | :private | true | false | :developer | 200 + :internal | :private | true | false | :maintainer | 200 + + :internal | :private | false | true | :non_member | 200 + :internal | :private | false | true | :guest | 200 + :internal | :private | false | true | :reporter | 200 + :internal | :private | false | true | :developer | 200 + :internal | :private | false | true | :maintainer | 200 + + :internal | :private | false | false | nil | 404 + :internal | :private | false | false | :non_member | 403 + :internal | :private | false | false | :guest | 403 + :internal | :private | false | false | :reporter | 200 + :internal | :private | false | false | :developer | 200 + :internal | :private | false | false | :maintainer | 200 + + :public | :enabled | true | true | :non_member | 200 + :public | :enabled | true | true | :guest | 200 + :public | :enabled | true | true | :reporter | 200 + :public | :enabled | true | true | :developer | 200 + :public | :enabled | true | true | :maintainer | 200 + + :public | :enabled | true | false | nil | 200 + :public | :enabled | true | false | :non_member | 200 + :public | :enabled | true | false | :guest | 200 + :public | :enabled | true | false | :reporter | 200 + :public | :enabled | true | false | :developer | 200 + :public | :enabled | true | false | :maintainer | 200 + + :public | :enabled | false | true | :non_member | 200 + :public | :enabled | false | true | :guest | 200 + :public | :enabled | false | true | :reporter | 200 + :public | :enabled | false | true | :developer | 200 + :public | :enabled | false | true | :maintainer | 200 + + :public | :enabled | false | false | nil | 403 + :public | :enabled | false | false | :non_member | 403 + :public | :enabled | false | false | :guest | 403 + :public | :enabled | false | false | :reporter | 200 + :public | :enabled | false | false | :developer | 200 + :public | :enabled | false | false | :maintainer | 200 + + :public | :private | true | true | :non_member | 200 + :public | :private | true | true | :guest | 200 + :public | :private | true | true | :reporter | 200 + :public | :private | true | true | :developer | 200 + :public | :private | true | true | :maintainer | 200 + + :public | :private | true | false | nil | 403 + :public | :private | true | false | :non_member | 403 + :public | :private | true | false | :guest | 200 + :public | :private | true | false | :reporter | 200 + :public | :private | true | false | :developer | 200 + :public | :private | true | false | :maintainer | 200 + + :public | :private | false | true | :non_member | 200 + :public | :private | false | true | :guest | 200 + :public | :private | false | true | :reporter | 200 + :public | :private | false | true | :developer | 200 + :public | :private | false | true | :maintainer | 200 + + :public | :private | false | false | nil | 403 + :public | :private | false | false | :non_member | 403 + :public | :private | false | false | :guest | 403 + :public | :private | false | false | :reporter | 200 + :public | :private | false | false | :developer | 200 + :public | :private | false | false | :maintainer | 200 + end + + with_them do + before do + ci_user.update!(admin: is_admin) if user_role + + project.update!(project_attributes) + project.project_feature.update!(project_feature_attributes) + project.add_role(ci_user, user_role) if user_role && user_role != :non_member + + get api(pipelines_api_path, api_user) + end + + it do + expect(response).to have_gitlab_http_status(response_status) + expect(api_response).to match(expected_response) + end + end +end |