diff options
137 files changed, 2102 insertions, 883 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f89a52e7a3e..98fdda3593e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -852,8 +852,6 @@ qa:selectors: .qa-frontend-node: &qa-frontend-node <<: *dedicated-no-docs-no-db-pull-cache-job stage: test - variables: - NODE_OPTIONS: --max_old_space_size=3584 cache: key: "$CI_JOB_NAME" paths: @@ -1143,3 +1141,4 @@ schedule:review-performance: <<: *review-schedules-only script: - wait_for_job_to_be_done "schedule:review-deploy" + diff --git a/.gitlab/issue_templates/Feature proposal.md b/.gitlab/issue_templates/Feature proposal.md index b4007c1ba7b..8a49715e0e8 100644 --- a/.gitlab/issue_templates/Feature proposal.md +++ b/.gitlab/issue_templates/Feature proposal.md @@ -24,6 +24,10 @@ Personas can be found at https://about.gitlab.com/handbook/marketing/product-mar <!-- See the Feature Change Documentation Workflow https://docs.gitlab.com/ee/development/documentation/feature-change-workflow.html Add all known Documentation Requirements here, per https://docs.gitlab.com/ee/development/documentation/feature-change-workflow.html#documentation-requirements --> +### Testing + +<!-- What risks does this change pose? How might it affect the quality of the product? What additional test coverage or changes to tests will be needed? Will it require cross-browser testing? See the test engineering process for further guidelines: https://about.gitlab.com/handbook/engineering/quality/guidelines/test-engineering/ --> + ### What does success look like, and how can we measure that? <!-- Define both the success metrics and acceptance criteria. Note that success metrics indicate the desired business outcomes, while acceptance criteria indicate when the solution is working correctly. If there is no way to measure success, link to an issue that will implement a way to measure this. --> diff --git a/Dangerfile b/Dangerfile index 95dd48aae9e..3e8cb456003 100644 --- a/Dangerfile +++ b/Dangerfile @@ -1,16 +1,19 @@ danger.import_plugin('danger/plugins/helper.rb') -danger.import_dangerfile(path: 'danger/metadata') -danger.import_dangerfile(path: 'danger/changes_size') -danger.import_dangerfile(path: 'danger/changelog') -danger.import_dangerfile(path: 'danger/specs') -danger.import_dangerfile(path: 'danger/gemfile') -danger.import_dangerfile(path: 'danger/database') -danger.import_dangerfile(path: 'danger/documentation') -danger.import_dangerfile(path: 'danger/frozen_string') -danger.import_dangerfile(path: 'danger/commit_messages') -danger.import_dangerfile(path: 'danger/duplicate_yarn_dependencies') -danger.import_dangerfile(path: 'danger/prettier') -danger.import_dangerfile(path: 'danger/eslint') -danger.import_dangerfile(path: 'danger/roulette') -danger.import_dangerfile(path: 'danger/single_codebase') -danger.import_dangerfile(path: 'danger/gitlab_ui_wg') + +unless helper.release_automation? + danger.import_dangerfile(path: 'danger/metadata') + danger.import_dangerfile(path: 'danger/changes_size') + danger.import_dangerfile(path: 'danger/changelog') + danger.import_dangerfile(path: 'danger/specs') + danger.import_dangerfile(path: 'danger/gemfile') + danger.import_dangerfile(path: 'danger/database') + danger.import_dangerfile(path: 'danger/documentation') + danger.import_dangerfile(path: 'danger/frozen_string') + danger.import_dangerfile(path: 'danger/commit_messages') + danger.import_dangerfile(path: 'danger/duplicate_yarn_dependencies') + danger.import_dangerfile(path: 'danger/prettier') + danger.import_dangerfile(path: 'danger/eslint') + danger.import_dangerfile(path: 'danger/roulette') + danger.import_dangerfile(path: 'danger/single_codebase') + danger.import_dangerfile(path: 'danger/gitlab_ui_wg') +end diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 034552a83ee..34aae156b19 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.30.0 +1.31.0 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index d127a0ff9f1..a2f28f43be3 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.3.3 +8.4.0 @@ -419,7 +419,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 1.13.0', require: 'gitaly' +gem 'gitaly-proto', '~> 1.19.0', require: 'gitaly' gem 'grpc', '~> 1.15.0' diff --git a/Gemfile.lock b/Gemfile.lock index 4ebcc6c81b2..e8053ada8b2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -281,7 +281,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (1.13.0) + gitaly-proto (1.19.0) grpc (~> 1.0) github-markup (1.7.0) gitlab-default_value_for (3.1.1) @@ -1017,7 +1017,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 1.13.0) + gitaly-proto (~> 1.19.0) github-markup (~> 1.7.0) gitlab-default_value_for (~> 3.1.1) gitlab-markup (~> 1.7.0) diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index bb66ab36283..41670b45798 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -48,10 +48,13 @@ export default { noteableType: this.noteableType, noteTargetLine: this.noteTargetLine, diffViewType: this.diffViewType, - diffFile: this.getDiffFileByHash(this.diffFileHash), + diffFile: this.diffFile, linePosition: this.linePosition, }; }, + diffFile() { + return this.getDiffFileByHash(this.diffFileHash); + }, }, mounted() { if (this.isLoggedIn) { @@ -102,6 +105,7 @@ export default { :line-code="line.line_code" :line="line" :help-page-path="helpPagePath" + :diff-file="diffFile" save-button-title="Comment" class="diff-comment-form" @handleFormUpdateAddToReview="addToReview" diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index 57d6b181bd7..471323bfc83 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -61,6 +61,11 @@ export default { required: false, default: null, }, + diffFile: { + type: Object, + required: false, + default: null, + }, helpPagePath: { type: String, required: false, @@ -102,9 +107,42 @@ export default { } return '#'; }, + diffParams() { + if (this.diffFile) { + return { + filePath: this.diffFile.file_path, + refs: this.diffFile.diff_refs, + }; + } else if (this.note && this.note.position) { + return { + filePath: this.note.position.new_path, + refs: this.note.position, + }; + } else if (this.discussion && this.discussion.diff_file) { + return { + filePath: this.discussion.diff_file.file_path, + refs: this.discussion.diff_file.diff_refs, + }; + } + + return null; + }, markdownPreviewPath() { const notable = this.getNoteableDataByProp('preview_note_path'); - return mergeUrlParams({ preview_suggestions: true }, notable); + + const previewSuggestions = this.line && this.diffParams; + const params = previewSuggestions + ? { + preview_suggestions: previewSuggestions, + line: this.line.new_line, + file_path: this.diffParams.filePath, + base_sha: this.diffParams.refs.base_sha, + start_sha: this.diffParams.refs.start_sha, + head_sha: this.diffParams.refs.head_sha, + } + : {}; + + return mergeUrlParams(params, notable); }, markdownDocsPath() { return this.getNotesDataByProp('markdownDocsPath'); @@ -234,8 +272,8 @@ export default { placeholder="Write a comment or drag your files here…" @keydown.meta.enter="handleKeySubmit()" @keydown.ctrl.enter="handleKeySubmit()" - @keydown.up="editMyLastNote()" - @keydown.esc="cancelHandler(true)" + @keydown.exact.up="editMyLastNote()" + @keydown.exact.esc="cancelHandler(true)" @input="onInput" ></textarea> </markdown-field> diff --git a/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue b/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue index 706e6ca19c3..57125c78cf6 100644 --- a/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue +++ b/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue @@ -50,6 +50,9 @@ export default { buttonLabel() { return this.isTodo ? MARK_TEXT : TODO_TEXT; }, + buttonTooltip() { + return !this.collapsed ? undefined : this.buttonLabel; + }, collapsedButtonIconClasses() { return this.isTodo ? 'todo-undone' : ''; }, @@ -69,7 +72,7 @@ export default { <button v-tooltip :class="buttonClasses" - :title="buttonLabel" + :title="buttonTooltip" :aria-label="buttonLabel" :data-issuable-id="issuableId" :data-issuable-type="issuableType" diff --git a/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js b/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js new file mode 100644 index 00000000000..d1aba99ac22 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/lib/utils/diff_utils.js @@ -0,0 +1,20 @@ +/* eslint-disable import/prefer-default-export */ + +function trimFirstCharOfLineContent(text) { + if (!text) { + return text; + } + + return text.replace(/^( |\+|-)/, ''); +} + +function cleanSuggestionLine(line = {}) { + return { + ...line, + text: trimFirstCharOfLineContent(line.text), + }; +} + +export function selectDiffLines(lines) { + return lines.filter(line => line.type !== 'match').map(line => cleanSuggestionLine(line)); +} diff --git a/app/assets/javascripts/vue_shared/components/markdown/field.vue b/app/assets/javascripts/vue_shared/components/markdown/field.vue index eccf73e227c..0f3b3568414 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/field.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/field.vue @@ -76,6 +76,7 @@ export default { hasSuggestion: false, markdownPreviewLoading: false, previewMarkdown: false, + suggestions: this.note.suggestions || [], }; }, computed: { @@ -109,9 +110,6 @@ export default { } return lineNumber; }, - suggestions() { - return this.note.suggestions || []; - }, lineType() { return this.line ? this.line.type : ''; }, @@ -175,6 +173,7 @@ export default { this.referencedCommands = data.references.commands; this.referencedUsers = data.references.users; this.hasSuggestion = data.references.suggestions && data.references.suggestions.length; + this.suggestions = data.references.suggestions; } this.$nextTick() diff --git a/app/assets/javascripts/vue_shared/components/markdown/header.vue b/app/assets/javascripts/vue_shared/components/markdown/header.vue index cc6ecdb0395..a5a5b2ef415 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/header.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/header.vue @@ -38,7 +38,7 @@ export default { ].join('\n'); }, mdSuggestion() { - return ['```suggestion', `{text}`, '```'].join('\n'); + return ['```suggestion:-0+0', `{text}`, '```'].join('\n'); }, }, mounted() { diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue index a351ca62c94..2eb4ec12a4a 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue @@ -1,24 +1,14 @@ <script> import SuggestionDiffHeader from './suggestion_diff_header.vue'; +import SuggestionDiffRow from './suggestion_diff_row.vue'; +import { selectDiffLines } from '../lib/utils/diff_utils'; export default { components: { SuggestionDiffHeader, + SuggestionDiffRow, }, props: { - newLines: { - type: Array, - required: true, - }, - fromContent: { - type: String, - required: false, - default: '', - }, - fromLine: { - type: Number, - required: true, - }, suggestion: { type: Object, required: true, @@ -33,6 +23,11 @@ export default { required: true, }, }, + computed: { + lines() { + return selectDiffLines(this.suggestion.diff_lines); + }, + }, methods: { applySuggestion(callback) { this.$emit('apply', { suggestionId: this.suggestion.id, callback }); @@ -52,22 +47,11 @@ export default { /> <table class="mb-3 md-suggestion-diff js-syntax-highlight code"> <tbody> - <!-- Old Line --> - <tr class="line_holder old"> - <td class="diff-line-num old_line qa-old-diff-line-number old">{{ fromLine }}</td> - <td class="diff-line-num new_line old"></td> - <td class="line_content old"> - <span>{{ fromContent }}</span> - </td> - </tr> - <!-- New Line(s) --> - <tr v-for="(line, key) of newLines" :key="key" class="line_holder new"> - <td class="diff-line-num old_line new"></td> - <td class="diff-line-num new_line qa-new-diff-line-number new">{{ line.lineNumber }}</td> - <td class="line_content new"> - <span>{{ line.content }}</span> - </td> - </tr> + <suggestion-diff-row + v-for="(line, index) of lines" + :key="`${index}-${line.text}`" + :line="line" + /> </tbody> </table> </div> diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_row.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_row.vue new file mode 100644 index 00000000000..cafd3a515ea --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_row.vue @@ -0,0 +1,32 @@ +<script> +export default { + name: 'SuggestionDiffRow', + props: { + line: { + type: Object, + required: true, + }, + }, + computed: { + lineType() { + return this.line.type; + }, + }, +}; +</script> + +<template> + <tr class="line_holder" :class="lineType"> + <td class="diff-line-num old_line" :class="lineType"> + {{ line.old_line }} + </td> + <td class="diff-line-num new_line" :class="lineType"> + {{ line.new_line }} + </td> + <td class="line_content" :class="lineType"> + <span v-if="line.text">{{ line.text }}</span> + <!-- TODO: replace this hack with zero-width whitespace when we have rich_text from BE --> + <span v-else>​</span> + </td> + </tr> +</template> diff --git a/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue index 177d78cb904..8d3705e1e4a 100644 --- a/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue +++ b/app/assets/javascripts/vue_shared/components/markdown/suggestions.vue @@ -6,16 +6,6 @@ import Flash from '~/flash'; export default { components: { SuggestionDiff }, props: { - fromLine: { - type: Number, - required: false, - default: 0, - }, - fromContent: { - type: String, - required: false, - default: '', - }, lineType: { type: String, required: false, @@ -71,41 +61,19 @@ export default { suggestionElements.forEach((suggestionEl, i) => { const suggestionParentEl = suggestionEl.parentElement; - const newLines = this.extractNewLines(suggestionParentEl); - const diffComponent = this.generateDiff(newLines, i); + const diffComponent = this.generateDiff(i); diffComponent.$mount(suggestionParentEl); }); this.isRendered = true; }, - extractNewLines(suggestionEl) { - // extracts the suggested lines from the markdown - // calculates a line number for each line - - const newLines = suggestionEl.querySelectorAll('.line'); - const fromLine = this.suggestions.length ? this.suggestions[0].from_line : this.fromLine; - const lines = []; - - newLines.forEach((line, i) => { - const content = `${line.innerText}\n`; - const lineNumber = fromLine + i; - lines.push({ content, lineNumber }); - }); - - return lines; - }, - generateDiff(newLines, suggestionIndex) { - // generates the diff <suggestion-diff /> component - // all `suggestion` markdown will be swapped out by this component - + generateDiff(suggestionIndex) { const { suggestions, disabled, helpPagePath } = this; const suggestion = suggestions && suggestions[suggestionIndex] ? suggestions[suggestionIndex] : {}; - const fromContent = suggestion.from_content || this.fromContent; - const fromLine = suggestion.from_line || this.fromLine; const SuggestionDiffComponent = Vue.extend(SuggestionDiff); const suggestionDiff = new SuggestionDiffComponent({ - propsData: { newLines, fromLine, fromContent, disabled, suggestion, helpPagePath }, + propsData: { disabled, suggestion, helpPagePath }, }); suggestionDiff.$on('apply', ({ suggestionId, callback }) => { diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index b90db135b4a..efcd35a2e0e 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -287,7 +287,7 @@ list-style: none; padding: 0 1px; - a, + a:not(.btn), button, .menu-item { @include dropdown-link; @@ -351,6 +351,10 @@ // Expects up to 3 digits on the badge margin-right: 40px; } + + .dropdown-menu-content { + padding: $dropdown-item-padding-y $dropdown-item-padding-x; + } } .droplab-dropdown { diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 02364180c35..54d985df9b5 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -154,11 +154,17 @@ .swipe-wrap { overflow: hidden; - border-left: 1px solid $gl-gray-400; + border-right: 1px solid $gl-gray-400; position: absolute; display: block; top: 13px; right: 7px; + + &.left-oriented { + /* only for commit view (different swipe viewer) */ + border-right: 0; + border-left: 1px solid $gl-gray-400; + } } .swipe-bar { diff --git a/app/controllers/concerns/preview_markdown.rb b/app/controllers/concerns/preview_markdown.rb index f72d25fc54c..2a9729b6ffd 100644 --- a/app/controllers/concerns/preview_markdown.rb +++ b/app/controllers/concerns/preview_markdown.rb @@ -20,7 +20,7 @@ module PreviewMarkdown body: view_context.markdown(result[:text], markdown_params), references: { users: result[:users], - suggestions: result[:suggestions], + suggestions: SuggestionSerializer.new.represent_diff(result[:suggestions]), commands: view_context.markdown(result[:commands]) } } diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb index 4eeaeb860ee..3b4215b766e 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -23,7 +23,7 @@ class Projects::RepositoriesController < Projects::ApplicationController append_sha = false if @filename == shortname end - send_git_archive @repository, ref: @ref, format: params[:format], append_sha: append_sha + send_git_archive @repository, ref: @ref, path: params[:path], format: params[:format], append_sha: append_sha rescue => ex logger.error("#{self.class.name}: #{ex}") git_not_found! diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 06d26309b5b..ecc34eacc7d 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -5,6 +5,7 @@ class GitlabSchema < GraphQL::Schema use Gitlab::Graphql::Authorize use Gitlab::Graphql::Present use Gitlab::Graphql::Connections + use Gitlab::Graphql::Tracing query(Types::QueryType) diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb index 18696293b97..de7d6570a3e 100644 --- a/app/graphql/types/ci/pipeline_type.rb +++ b/app/graphql/types/ci/pipeline_type.rb @@ -3,10 +3,12 @@ module Types module Ci class PipelineType < BaseObject - expose_permissions Types::PermissionTypes::Ci::Pipeline - graphql_name 'Pipeline' + authorize :read_pipeline + + expose_permissions Types::PermissionTypes::Ci::Pipeline + field :id, GraphQL::ID_TYPE, null: false field :iid, GraphQL::ID_TYPE, null: false diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb index 5ad3ea52930..adb137dfee3 100644 --- a/app/graphql/types/issue_type.rb +++ b/app/graphql/types/issue_type.rb @@ -2,10 +2,12 @@ module Types class IssueType < BaseObject - expose_permissions Types::PermissionTypes::Issue - graphql_name 'Issue' + authorize :read_issue + + expose_permissions Types::PermissionTypes::Issue + present_using IssuePresenter field :iid, GraphQL::ID_TYPE, null: false @@ -15,16 +17,14 @@ module Types field :author, Types::UserType, null: false, - resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find }, - authorize: :read_user + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find } field :assignees, Types::UserType.connection_type, null: true field :labels, Types::LabelType.connection_type, null: true field :milestone, Types::MilestoneType, null: true, - resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find }, - authorize: :read_milestone + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find } field :due_date, Types::TimeType, null: true field :confidential, GraphQL::BOOLEAN_TYPE, null: false diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 1ed27a14e33..120ffe0dfde 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -2,12 +2,14 @@ module Types class MergeRequestType < BaseObject + graphql_name 'MergeRequest' + + authorize :read_merge_request + expose_permissions Types::PermissionTypes::MergeRequest present_using MergeRequestPresenter - graphql_name 'MergeRequest' - field :id, GraphQL::ID_TYPE, null: false field :iid, GraphQL::ID_TYPE, null: false field :title, GraphQL::STRING_TYPE, null: false @@ -48,7 +50,7 @@ module Types field :downvotes, GraphQL::INT_TYPE, null: false field :subscribed, GraphQL::BOOLEAN_TYPE, method: :subscribed?, null: false - field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline, authorize: :read_pipeline + field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline field :pipelines, Types::Ci::PipelineType.connection_type, resolver: Resolvers::MergeRequestPipelinesResolver end diff --git a/app/graphql/types/milestone_type.rb b/app/graphql/types/milestone_type.rb index af31b572c9a..2772fbec86f 100644 --- a/app/graphql/types/milestone_type.rb +++ b/app/graphql/types/milestone_type.rb @@ -4,6 +4,8 @@ module Types class MilestoneType < BaseObject graphql_name 'Milestone' + authorize :read_milestone + field :description, GraphQL::STRING_TYPE, null: true field :title, GraphQL::STRING_TYPE, null: false field :state, GraphQL::STRING_TYPE, null: false diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index b96c2f3afb2..fbb4eddd13c 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -2,10 +2,12 @@ module Types class ProjectType < BaseObject - expose_permissions Types::PermissionTypes::Project - graphql_name 'Project' + authorize :read_project + + expose_permissions Types::PermissionTypes::Project + field :id, GraphQL::ID_TYPE, null: false field :full_path, GraphQL::ID_TYPE, null: false @@ -67,14 +69,12 @@ module Types field :merge_requests, Types::MergeRequestType.connection_type, null: true, - resolver: Resolvers::MergeRequestsResolver, - authorize: :read_merge_request + resolver: Resolvers::MergeRequestsResolver field :merge_request, Types::MergeRequestType, null: true, - resolver: Resolvers::MergeRequestsResolver.single, - authorize: :read_merge_request + resolver: Resolvers::MergeRequestsResolver.single field :issues, Types::IssueType.connection_type, @@ -88,7 +88,7 @@ module Types field :pipelines, Types::Ci::PipelineType.connection_type, - null: false, + null: true, resolver: Resolvers::ProjectPipelinesResolver end end diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 472fe5d6ec2..0f655ab9d03 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -7,8 +7,7 @@ module Types field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, - description: "Find a project", - authorize: :read_project + description: "Find a project" field :metadata, Types::MetadataType, null: true, diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb index a13e65207df..6b53554314b 100644 --- a/app/graphql/types/user_type.rb +++ b/app/graphql/types/user_type.rb @@ -4,6 +4,8 @@ module Types class UserType < BaseObject graphql_name 'User' + authorize :read_user + present_using UserPresenter field :name, GraphQL::STRING_TYPE, null: false diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index f2abb241753..009dd70c2c9 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -299,6 +299,10 @@ module ProjectsHelper }.to_json end + def directory? + @path.present? + end + private def get_project_nav_tabs(project, current_user) diff --git a/app/models/repository.rb b/app/models/repository.rb index 574ce12b309..51ab2247a03 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -299,13 +299,14 @@ class Repository end end - def archive_metadata(ref, storage_path, format = "tar.gz", append_sha:) + def archive_metadata(ref, storage_path, format = "tar.gz", append_sha:, path: nil) raw_repository.archive_metadata( ref, storage_path, project.path, format, - append_sha: append_sha + append_sha: append_sha, + path: path ) end diff --git a/app/presenters/ci/bridge_presenter.rb b/app/presenters/ci/bridge_presenter.rb new file mode 100644 index 00000000000..ee11cffe355 --- /dev/null +++ b/app/presenters/ci/bridge_presenter.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Ci + class BridgePresenter < CommitStatusPresenter + def detailed_status + @detailed_status ||= subject.detailed_status(user) + end + end +end diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb index c3f7d4651fb..914ad628a99 100644 --- a/app/serializers/issue_entity.rb +++ b/app/serializers/issue_entity.rb @@ -42,6 +42,6 @@ class IssueEntity < IssuableEntity end expose :preview_note_path do |issue| - preview_markdown_path(issue.project, quick_actions_target_type: 'Issue', quick_actions_target_id: issue.iid) + preview_markdown_path(issue.project, target_type: 'Issue', target_id: issue.iid) end end diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index d673f8ae896..4831eb32c96 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -235,7 +235,7 @@ class MergeRequestWidgetEntity < IssuableEntity end expose :preview_note_path do |merge_request| - preview_markdown_path(merge_request.project, quick_actions_target_type: 'MergeRequest', quick_actions_target_id: merge_request.iid) + preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid) end expose :merge_commit_path do |merge_request| diff --git a/app/serializers/suggestion_entity.rb b/app/serializers/suggestion_entity.rb index 4d0d4da10be..2dd62e19e29 100644 --- a/app/serializers/suggestion_entity.rb +++ b/app/serializers/suggestion_entity.rb @@ -3,6 +3,8 @@ class SuggestionEntity < API::Entities::Suggestion include RequestAwareEntity + unexpose :from_line, :to_line, :from_content, :to_content + expose :diff_lines, using: DiffLineEntity expose :current_user do expose :can_apply do |suggestion| Ability.allowed?(current_user, :apply_suggestion, suggestion) diff --git a/app/serializers/suggestion_serializer.rb b/app/serializers/suggestion_serializer.rb new file mode 100644 index 00000000000..010344f9fcd --- /dev/null +++ b/app/serializers/suggestion_serializer.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class SuggestionSerializer < BaseSerializer + entity SuggestionEntity + + def represent_diff(resource) + represent(resource, { only: [:diff_lines] }) + end +end diff --git a/app/services/clusters/applications/base_helm_service.rb b/app/services/clusters/applications/base_helm_service.rb index c38b2656260..adaa68b1efb 100644 --- a/app/services/clusters/applications/base_helm_service.rb +++ b/app/services/clusters/applications/base_helm_service.rb @@ -13,16 +13,20 @@ module Clusters def log_error(error) meta = { - exception: error.class.name, error_code: error.respond_to?(:error_code) ? error.error_code : nil, service: self.class.name, app_id: app.id, project_ids: app.cluster.project_ids, - group_ids: app.cluster.group_ids, - message: error.message + group_ids: app.cluster.group_ids } - logger.error(meta) + logger_meta = meta.merge( + exception: error.class.name, + message: error.message, + backtrace: Gitlab::Profiler.clean_backtrace(error.backtrace) + ) + + logger.error(logger_meta) Gitlab::Sentry.track_acceptable_exception(error, extra: meta) end diff --git a/app/services/concerns/suggestible.rb b/app/services/concerns/suggestible.rb index 0b9822b1909..0cba9bf1b8a 100644 --- a/app/services/concerns/suggestible.rb +++ b/app/services/concerns/suggestible.rb @@ -2,10 +2,17 @@ module Suggestible extend ActiveSupport::Concern + include Gitlab::Utils::StrongMemoize # This translates into limiting suggestion changes to `suggestion:-100+100`. MAX_LINES_CONTEXT = 100.freeze + def diff_lines + strong_memoize(:diff_lines) do + Gitlab::Diff::SuggestionDiff.new(self).diff_lines + end + end + def fetch_from_content diff_file.new_blob_lines_between(from_line, to_line).join end diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb index 8c8acce5ca5..019cd047ae9 100644 --- a/app/services/groups/base_service.rb +++ b/app/services/groups/base_service.rb @@ -7,5 +7,11 @@ module Groups def initialize(group, user, params = {}) @group, @current_user, @params = group, user, params.dup end + + private + + def remove_unallowed_params + # overridden in EE + end end end diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 99ead467f74..74aad3b1c94 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -8,6 +8,8 @@ module Groups end def execute + remove_unallowed_params + @group = Group.new(params) after_build_hook(@group, params) diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 787445180f0..73e1e00dc33 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -6,6 +6,7 @@ module Groups def execute reject_parent_id! + remove_unallowed_params return false unless valid_visibility_level_change?(group, params[:visibility_level]) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 3e208241da5..f968e3693da 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -55,15 +55,7 @@ module MergeRequests end def create_pipeline_for(merge_request, user) - return unless Feature.enabled?(:ci_merge_request_pipeline, - merge_request.source_project, - default_enabled: true) - - ## - # UpdateMergeRequestsWorker could be retried by an exception. - # MR pipelines should not be recreated in such case. - return if merge_request.merge_request_pipeline_exists? - return if merge_request.has_no_commits? + return unless can_create_pipeline_for?(merge_request) create_detached_merge_request_pipeline(merge_request, user) end @@ -80,6 +72,16 @@ module MergeRequests end end + def can_create_pipeline_for?(merge_request) + ## + # UpdateMergeRequestsWorker could be retried by an exception. + # pipelines for merge request should not be recreated in such case. + return false if merge_request.merge_request_pipeline_exists? + return false if merge_request.has_no_commits? + + true + end + def can_use_merge_request_ref?(merge_request) Feature.enabled?(:ci_use_merge_request_ref, project, default_enabled: true) && !merge_request.for_fork? diff --git a/app/services/preview_markdown_service.rb b/app/services/preview_markdown_service.rb index c1655c38095..7386530f45f 100644 --- a/app/services/preview_markdown_service.rb +++ b/app/services/preview_markdown_service.rb @@ -17,7 +17,7 @@ class PreviewMarkdownService < BaseService private def explain_quick_actions(text) - return text, [] unless %w(Issue MergeRequest Commit).include?(commands_target_type) + return text, [] unless %w(Issue MergeRequest Commit).include?(target_type) quick_actions_service = QuickActions::InterpretService.new(project, current_user) quick_actions_service.explain(text, find_commands_target) @@ -30,22 +30,34 @@ class PreviewMarkdownService < BaseService end def find_suggestions(text) - return [] unless params[:preview_suggestions] + return [] unless preview_sugestions? - Banzai::SuggestionsParser.parse(text) + position = Gitlab::Diff::Position.new(new_path: params[:file_path], + new_line: params[:line].to_i, + base_sha: params[:base_sha], + head_sha: params[:head_sha], + start_sha: params[:start_sha]) + + Gitlab::Diff::SuggestionsParser.parse(text, position: position, project: project) + end + + def preview_sugestions? + params[:preview_suggestions] && + target_type == 'MergeRequest' && + Ability.allowed?(current_user, :download_code, project) end def find_commands_target QuickActions::TargetService .new(project, current_user) - .execute(commands_target_type, commands_target_id) + .execute(target_type, target_id) end - def commands_target_type - params[:quick_actions_target_type] + def target_type + params[:target_type] end - def commands_target_id - params[:quick_actions_target_id] + def target_id + params[:target_id] end end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 9a243e07936..00b51f92b12 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -46,6 +46,10 @@ module RecordsUploads File.join(store_dir, filename.to_s) end + def filename + upload&.path ? File.basename(upload.path) : super + end + private # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/views/projects/buttons/_download.html.haml b/app/views/projects/buttons/_download.html.haml index 4eb53faa6ff..acd63de2277 100644 --- a/app/views/projects/buttons/_download.html.haml +++ b/app/views/projects/buttons/_download.html.haml @@ -8,30 +8,20 @@ %span.sr-only= _('Select Archive Format') = sprite_icon("arrow-down") %ul.dropdown-menu.dropdown-menu-right{ role: 'menu' } - %li.dropdown-header - #{ _('Source code') } - %li - = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'zip'), rel: 'nofollow', download: '' do - %span= _('Download zip') - %li - = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar.gz'), rel: 'nofollow', download: '' do - %span= _('Download tar.gz') - %li - = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar.bz2'), rel: 'nofollow', download: '' do - %span= _('Download tar.bz2') - %li - = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar'), rel: 'nofollow', download: '' do - %span= _('Download tar') - + %li.dropdown-bold-header= _('Download source code') + %li.dropdown-menu-content + = render 'projects/buttons/download_links', project: project, ref: ref, archive_prefix: archive_prefix, path: nil + - if directory? + %li.separator + %li.dropdown-bold-header= _('Download this directory') + %li.dropdown-menu-content + = render 'projects/buttons/download_links', project: project, ref: ref, archive_prefix: archive_prefix, path: @path - if pipeline && pipeline.latest_builds_with_artifacts.any? - %li.dropdown-header Artifacts + %li.separator + %li.dropdown-bold-header= _('Download artifacts') - unless pipeline.latest? - - latest_pipeline = project.pipeline_for(ref) - %li - .unclickable= ci_status_for_statuseable(latest_pipeline) - %li.dropdown-header Previous Artifacts + %span.unclickable= ci_status_for_statuseable(project.pipeline_for(ref)) + %li.dropdown-header= _('Previous Artifacts') - pipeline.latest_builds_with_artifacts.each do |job| %li - = link_to latest_succeeded_project_artifacts_path(project, "#{ref}/download", job: job.name), rel: 'nofollow', download: '' do - %span - #{s_('DownloadArtifacts|Download')} '#{job.name}' + = link_to job.name, latest_succeeded_project_artifacts_path(project, "#{ref}/download", job: job.name), rel: 'nofollow', download: '' diff --git a/app/views/projects/buttons/_download_links.html.haml b/app/views/projects/buttons/_download_links.html.haml new file mode 100644 index 00000000000..47a1704f946 --- /dev/null +++ b/app/views/projects/buttons/_download_links.html.haml @@ -0,0 +1,9 @@ +%ul + %li.d-inline-block.m-0.p-0 + = link_to 'zip', project_archive_path(project, id: tree_join(ref, archive_prefix), path: path, format: 'zip'), rel: 'nofollow', download: '', class: 'btn btn-primary btn-xs' + %li.d-inline-block.m-0.p-0 + = link_to 'tar.gz', project_archive_path(project, id: tree_join(ref, archive_prefix), path: path, format: 'tar.gz'), rel: 'nofollow', download: '', class: 'btn btn-xs' + %li.d-inline-block.m-0.p-0 + = link_to 'tar.bz2', project_archive_path(project, id: tree_join(ref, archive_prefix), path: path, format: 'tar.bz2'), rel: 'nofollow', download: '', class: 'btn btn-xs' + %li.d-inline-block.m-0.p-0 + = link_to 'tar', project_archive_path(project, id: tree_join(ref, archive_prefix), path: path, format: 'tar'), rel: 'nofollow', download: '', class: 'btn btn-xs' diff --git a/app/views/projects/diffs/_replaced_image_diff.html.haml b/app/views/projects/diffs/_replaced_image_diff.html.haml index 6dffc7c4390..70521ed892e 100644 --- a/app/views/projects/diffs/_replaced_image_diff.html.haml +++ b/app/views/projects/diffs/_replaced_image_diff.html.haml @@ -37,7 +37,7 @@ .swipe-frame .frame.deleted = image_tag(old_blob_raw_url, alt: diff_file.old_path, lazy: false) - .swipe-wrap + .swipe-wrap.left-oriented = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_url, alt: diff_file.new_path } %span.swipe-bar %span.top-handle diff --git a/app/views/shared/form_elements/_description.html.haml b/app/views/shared/form_elements/_description.html.haml index 25df2fe5cd6..b11cb8a3076 100644 --- a/app/views/shared/form_elements/_description.html.haml +++ b/app/views/shared/form_elements/_description.html.haml @@ -5,7 +5,7 @@ - supports_quick_actions = model.new_record? - if supports_quick_actions - - preview_url = preview_markdown_path(project, quick_actions_target_type: model.class.name) + - preview_url = preview_markdown_path(project, target_type: model.class.name) - else - preview_url = preview_markdown_path(project) diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml index 6a1eea85fde..d91bc6e57c9 100644 --- a/app/views/shared/notes/_form.html.haml +++ b/app/views/shared/notes/_form.html.haml @@ -1,7 +1,7 @@ - supports_autocomplete = local_assigns.fetch(:supports_autocomplete, true) - supports_quick_actions = note_supports_quick_actions?(@note) - if supports_quick_actions - - preview_url = preview_markdown_path(@project, quick_actions_target_type: @note.noteable_type, quick_actions_target_id: @note.noteable_id) + - preview_url = preview_markdown_path(@project, target_type: @note.noteable_type, target_id: @note.noteable_id) - else - preview_url = preview_markdown_path(@project) diff --git a/changelogs/unreleased/24704-download-repository-path.yml b/changelogs/unreleased/24704-download-repository-path.yml new file mode 100644 index 00000000000..ff3082bec45 --- /dev/null +++ b/changelogs/unreleased/24704-download-repository-path.yml @@ -0,0 +1,5 @@ +--- +title: Download a folder from repository +merge_request: 26532 +author: kiameisomabes +type: added diff --git a/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml b/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml new file mode 100644 index 00000000000..e588fa79619 --- /dev/null +++ b/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml @@ -0,0 +1,5 @@ +--- +title: Remove `path` and `branch` labels from metrics +merge_request: 26744 +author: +type: fixed diff --git a/changelogs/unreleased/54417-graphql-type-authorization.yml b/changelogs/unreleased/54417-graphql-type-authorization.yml new file mode 100644 index 00000000000..528b58a858a --- /dev/null +++ b/changelogs/unreleased/54417-graphql-type-authorization.yml @@ -0,0 +1,5 @@ +--- +title: GraphQL Types can be made to always authorize access to resources of that Type +merge_request: 25724 +author: +type: added diff --git a/changelogs/unreleased/55964-fix-email-encoding.yml b/changelogs/unreleased/55964-fix-email-encoding.yml new file mode 100644 index 00000000000..2195a853702 --- /dev/null +++ b/changelogs/unreleased/55964-fix-email-encoding.yml @@ -0,0 +1,5 @@ +--- +title: Fix notfication emails having wrong encoding +merge_request: 26931 +author: +type: fixed diff --git a/changelogs/unreleased/add_backtrace_to_kubernetes_log.yml b/changelogs/unreleased/add_backtrace_to_kubernetes_log.yml new file mode 100644 index 00000000000..26b8ac4b1ef --- /dev/null +++ b/changelogs/unreleased/add_backtrace_to_kubernetes_log.yml @@ -0,0 +1,5 @@ +--- +title: Show error backtrace when logging errors to kubernetes.log +merge_request: 25726 +author: +type: other diff --git a/changelogs/unreleased/graphql-prometheus.yml b/changelogs/unreleased/graphql-prometheus.yml new file mode 100644 index 00000000000..180577f3aec --- /dev/null +++ b/changelogs/unreleased/graphql-prometheus.yml @@ -0,0 +1,5 @@ +--- +title: Added prometheus monitoring to GraphQL +merge_request: +author: +type: added diff --git a/changelogs/unreleased/osw-support-multi-line-suggestions.yml b/changelogs/unreleased/osw-support-multi-line-suggestions.yml new file mode 100644 index 00000000000..8c8206c3822 --- /dev/null +++ b/changelogs/unreleased/osw-support-multi-line-suggestions.yml @@ -0,0 +1,5 @@ +--- +title: Support multi-line suggestions +merge_request: 25211 +author: +type: added diff --git a/changelogs/unreleased/stop-signing-avatar-paths.yml b/changelogs/unreleased/stop-signing-avatar-paths.yml new file mode 100644 index 00000000000..2c2493f0f21 --- /dev/null +++ b/changelogs/unreleased/stop-signing-avatar-paths.yml @@ -0,0 +1,5 @@ +--- +title: Speed up generation of avatar URLs when using object storage +merge_request: +author: +type: performance diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb index e653556231d..f1bc289f1f0 100644 --- a/config/initializers/graphql.rb +++ b/config/initializers/graphql.rb @@ -1,4 +1,7 @@ # frozen_string_literal: true +GraphQL::ObjectType.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) + +GraphQL::Schema::Object.accepts_definition(:authorize) GraphQL::Schema::Field.accepts_definition(:authorize) diff --git a/config/initializers/premailer.rb b/config/initializers/premailer.rb index cb00d3cfe95..87f8e67ef1c 100644 --- a/config/initializers/premailer.rb +++ b/config/initializers/premailer.rb @@ -4,5 +4,6 @@ Premailer::Rails.config.merge!( preserve_styles: true, remove_comments: true, remove_ids: false, - remove_scripts: false + remove_scripts: false, + output_encoding: 'US-ASCII' ) diff --git a/db/post_migrate/20190325111602_rename_v2_root_namespaces.rb b/db/post_migrate/20190325111602_rename_v2_root_namespaces.rb new file mode 100644 index 00000000000..8571bb82fa0 --- /dev/null +++ b/db/post_migrate/20190325111602_rename_v2_root_namespaces.rb @@ -0,0 +1,27 @@ +# 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 RenameV2RootNamespaces < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + include Gitlab::Database::RenameReservedPathsMigration::V1 + + DOWNTIME = false + + disable_ddl_transaction! + + # We're taking over the /v2 namespace as it necessary for Docker client to + # work with GitLab as Dependency proxy for containers. + def up + disable_statement_timeout do + rename_root_paths 'v2' + end + end + + def down + disable_statement_timeout do + revert_renames + end + end +end diff --git a/doc/ci/variables/where_variables_can_be_used.md b/doc/ci/variables/where_variables_can_be_used.md index ceca4af1bee..1218ac0b071 100644 --- a/doc/ci/variables/where_variables_can_be_used.md +++ b/doc/ci/variables/where_variables_can_be_used.md @@ -15,26 +15,26 @@ There are two places defined variables can be used. On the: ### `.gitlab-ci.yml` file -| Definition | Can be expanded? | Expansion place | Description | -|--------------------------------------|-------------------|-----------------|--------------| -| `environment:url` | yes | GitLab | The variable expansion is made by GitLab's [internal variable expansion mechanism](#gitlab-internal-variable-expansion-mechanism).<ul><li>Supported: all variables defined for a job (project/group variables, variables from `.gitlab-ci.yml`, variables from triggers, variables from pipeline schedules)</li><li>Not supported: variables defined in Runner's `config.toml` and variables created in job's `script`</li></ul> | -| `environment:name` | yes | GitLab | Similar to `environment:url`, but the variables expansion doesn't support: <ul><li>variables that are based on the environment's name (`CI_ENVIRONMENT_NAME`, `CI_ENVIRONMENT_SLUG`)</li><li>any other variables related to environment (currently only `CI_ENVIRONMENT_URL`)</li><li>[persisted variables](#persisted-variables)</li></ul> | -| `variables` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | -| `image` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | -| `services:[]` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | -| `services:[]:name` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | -| `cache:key` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | -| `artifacts:name` | yes | Runner | The variable expansion is made by GitLab Runner's shell environment | -| `script`, `before_script`, `after_script` | yes | Script execution shell | The variable expansion is made by the [execution shell environment](#execution-shell-environment) | -| `only:variables:[]`, `except:variables:[]` | no | n/a | The variable must be in the form of `$variable`.<br/>Not supported:<ul><li>variables that are based on the environment's name (`CI_ENVIRONMENT_NAME`, `CI_ENVIRONMENT_SLUG`)</li><li>any other variables related to environment (currently only `CI_ENVIRONMENT_URL`)</li><li>[persisted variables](#persisted-variables)</li></ul> | +| Definition | Can be expanded? | Expansion place | Description | +|:-------------------------------------------|:-----------------|:-----------------------|:----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `environment:url` | yes | GitLab | The variable expansion is made by GitLab's [internal variable expansion mechanism](#gitlab-internal-variable-expansion-mechanism).<br/><br/>Supported are all variables defined for a job (project/group variables, variables from `.gitlab-ci.yml`, variables from triggers, variables from pipeline schedules).<br/><br/>Not supported are variables defined in Runner's `config.toml` and variables created in job's `script`. | +| `environment:name` | yes | GitLab | Similar to `environment:url`, but the variables expansion doesn't support the following:<br/><br/>- Variables that are based on the environment's name (`CI_ENVIRONMENT_NAME`, `CI_ENVIRONMENT_SLUG`).<br/>- Any other variables related to environment (currently only `CI_ENVIRONMENT_URL`).<br/>- [Persisted variables](#persisted-variables). | +| `variables` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | +| `image` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | +| `services:[]` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | +| `services:[]:name` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | +| `cache:key` | yes | Runner | The variable expansion is made by GitLab Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | +| `artifacts:name` | yes | Runner | The variable expansion is made by GitLab Runner's shell environment | +| `script`, `before_script`, `after_script` | yes | Script execution shell | The variable expansion is made by the [execution shell environment](#execution-shell-environment) | +| `only:variables:[]`, `except:variables:[]` | no | n/a | The variable must be in the form of `$variable`. Not supported are the following:<br/><br/>- Variables that are based on the environment's name (`CI_ENVIRONMENT_NAME`, `CI_ENVIRONMENT_SLUG`).<br/>- Any other variables related to environment (currently only `CI_ENVIRONMENT_URL`).<br/>- [Persisted variables](#persisted-variables). | ### `config.toml` file NOTE: **Note:** You can read more about `config.toml` in the [Runner's docs](https://docs.gitlab.com/runner/configuration/advanced-configuration.html). -| Definition | Can be expanded? | Description | -|--------------------------------------|------------------|-------------| +| Definition | Can be expanded? | Description | +|:-------------------------------------|:-----------------|:---------------------------------------------------------------------------------------------------------------------------------------------| | `runners.environment` | yes | The variable expansion is made by the Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | | `runners.kubernetes.pod_labels` | yes | The Variable expansion is made by the Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | | `runners.kubernetes.pod_annotations` | yes | The Variable expansion is made by the Runner's [internal variable expansion mechanism](#gitlab-runner-internal-variable-expansion-mechanism) | diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 501092ff2aa..8d2bfff3a5d 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -9,38 +9,6 @@ can be shared. It is also possible to add a `private_token` to the querystring, or add a `HTTP_PRIVATE_TOKEN` header. -### Authorization - -Fields can be authorized using the same abilities used in the Rails -app. This can be done by supplying the `authorize` option: - -```ruby -module Types - class QueryType < BaseObject - graphql_name 'Query' - - field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, authorize: :read_project - end -end -``` - -Fields can be authorized against multiple abilities, in which case all -ability checks must pass. This requires explicitly passing a block to `field`: - -```ruby -field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do - authorize [:read_project, :another_ability] -end -``` - -The object found by the resolve call is used for authorization. - -TIP: **Tip:** -When authorizing collections, try to load only what the currently -authenticated user is allowed to view with our existing finders first. -This minimizes database queries and unnecessary authorization checks of -the loaded records. - ## Types When exposing a model through the GraphQL API, we do so by creating a @@ -197,6 +165,114 @@ end policies at once. The fields for these will all have be non-nullable booleans with a default description. +## Authorization + +Authorizations can be applied to both types and fields using the same +abilities as in the Rails app. + +If the: + +- Currently authenticated user fails the authorization, the authorized +resource will be returned as `null`. +- Resource is part of a collection, the collection will be filtered to +exclude the objects that the user's authorization checks failed against. + +TIP: **Tip:** +Try to load only what the currently authenticated user is allowed to +view with our existing finders first, without relying on authorization +to filter the records. This minimizes database queries and unnecessary +authorization checks of the loaded records. + +### Type authorization + +Authorize a type by passing an ability to the `authorize` method. All +fields with the same type will be authorized by checking that the +currently authenticated user has the required ability. + +For example, the following authorization ensures that the currently +authenticated user can only see projects that they have the +`read_project` ability for (so long as the project is returned in a +field that uses `Types::ProjectType`): + +```ruby +module Types + class ProjectType < BaseObject + authorize :read_project + end +end +``` + +You can also authorize against multiple abilities, in which case all of +the ability checks must pass. + +For example, the following authorization ensures that the currently +authenticated user must have `read_project` and `another_ability` +abilities to see a project: + +```ruby +module Types + class ProjectType < BaseObject + authorize [:read_project, :another_ability] + end +end +``` + +### Field authorization + +Fields can be authorized with the `authorize` option. + +For example, the following authorization ensures that the currently +authenticated user must have the `owner_access` ability to see the +project: + +```ruby +module Types + class MyType < BaseObject + field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, authorize: :owner_access + end +end +``` + +Fields can also be authorized against multiple abilities, in which case +all of ability checks must pass. **Note:** This requires explicitly +passing a block to `field`: + +```ruby +module Types + class MyType < BaseObject + field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do + authorize [:owner_access, :another_ability] + end + end +end +``` + +NOTE: **Note:** If the field's type already [has a particular +authorization](#type-authorization) then there is no need to add that +same authorization to the field. + +### Type and Field authorizations together + +Authorizations are cumulative, so where authorizations are defined on +a field, and also on the field's type, then the currently authenticated +user would need to pass all ability checks. + +In the following simplified example the currently authenticated user +would need both `first_permission` and `second_permission` abilities in +order to see the author of the issue. + +```ruby +class UserType + authorize :first_permission +end +``` + +```ruby +class IssueType + field :author, UserType, authorize: :second_permission +end +``` + ## Resolvers To find objects to display in a field, we can add resolvers to diff --git a/doc/user/discussions/img/multi-line-suggestion-preview.png b/doc/user/discussions/img/multi-line-suggestion-preview.png Binary files differnew file mode 100644 index 00000000000..4288d0ba034 --- /dev/null +++ b/doc/user/discussions/img/multi-line-suggestion-preview.png diff --git a/doc/user/discussions/img/multi-line-suggestion-syntax.png b/doc/user/discussions/img/multi-line-suggestion-syntax.png Binary files differnew file mode 100644 index 00000000000..df0c99b84ef --- /dev/null +++ b/doc/user/discussions/img/multi-line-suggestion-syntax.png diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md index 23b9604a456..bf41fdecd10 100644 --- a/doc/user/discussions/index.md +++ b/doc/user/discussions/index.md @@ -344,6 +344,24 @@ and push the suggested change directly into the codebase in the merge request's Custom commit messages will be introduced by [#54404](https://gitlab.com/gitlab-org/gitlab-ce/issues/54404). +### Multi-line suggestions + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/53310) in GitLab 11.10. + +Reviewers can also suggest changes to +multiple lines with a single suggestion within Merge Request diff discussions. + + + +In the example above, the suggestion covers three lines above and four lines below the commented diff line. +It'd change from 3 lines _above_ to 4 lines _below_ the commented Diff line. + + + +NOTE: **Note:** +Suggestions covering multiple lines are limited to 100 lines _above_ and 100 lines _below_ +the commented diff line, allowing up to 200 changed lines per suggestion. + ## Start a discussion by replying to a standard comment > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/30299) in GitLab 11.9 diff --git a/doc/user/project/merge_requests/img/merge_when_pipeline_succeeds_only_if_succeeds_settings.png b/doc/user/project/merge_requests/img/merge_when_pipeline_succeeds_only_if_succeeds_settings.png Binary files differindex 2a2101719ba..ea3aff59aa1 100644 --- a/doc/user/project/merge_requests/img/merge_when_pipeline_succeeds_only_if_succeeds_settings.png +++ b/doc/user/project/merge_requests/img/merge_when_pipeline_succeeds_only_if_succeeds_settings.png diff --git a/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md b/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md index f8af71ab46b..1477e35dca8 100644 --- a/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md +++ b/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md @@ -35,11 +35,11 @@ You need to have jobs configured to enable this feature. You can prevent merge requests from being merged if their pipeline did not succeed or if there are discussions to be resolved. -Navigate to your project's settings page, select the -**Only allow merge requests to be merged if the pipeline succeeds** check box and -hit **Save** for the changes to take effect. +Navigate to your project's settings page and expand the **Merge requests** section. +In the **Merge checks** subsection, select the **Pipelines must succeed** check +box and hit **Save** for the changes to take effect. - +  From now on, every time the pipeline fails you will not be able to merge the merge request from the UI, until you make all relevant jobs pass. diff --git a/doc/user/project/repository/img/download_source_code.png b/doc/user/project/repository/img/download_source_code.png Binary files differnew file mode 100644 index 00000000000..17f2cb4b3e8 --- /dev/null +++ b/doc/user/project/repository/img/download_source_code.png diff --git a/doc/user/project/repository/index.md b/doc/user/project/repository/index.md index 22d912cd9d1..718566a539f 100644 --- a/doc/user/project/repository/index.md +++ b/doc/user/project/repository/index.md @@ -241,4 +241,24 @@ Projects that contain a `.xcodeproj` or `.xcworkspace` directory can now be clon in Xcode using the new **Open in Xcode** button, located next to the Git URL used for cloning your project. The button is only shown on macOS. +## Download Source Code + +Source code stored in the repository can be downloaded. + +By clicking the download icon, a dropdown will open with links to download the following: + + + +- **Source Code:** + This allows users to download the source code on branch they're currently + viewing. Available zip, tar, tar.gz and tar.bz2. +- **Directory:** + > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/24704) in GitLab 11.10 + + Only shows up when viewing a sub-directory. This allows users to download + the specific directory they're currently viewing. Also available in zip, tar, + tar.gz and tar.bz2. +- **Artifacts:** + This allows users to download the artifacts of the latest CI build. + [jupyter]: https://jupyter.org diff --git a/doc/user/reserved_names.md b/doc/user/reserved_names.md index 9aa81e33fc0..9e8475d8294 100644 --- a/doc/user/reserved_names.md +++ b/doc/user/reserved_names.md @@ -83,6 +83,7 @@ Currently the following names are reserved as top level groups: - unsubscribes - uploads - users +- v2 These group names are unavailable as subgroup names: diff --git a/lib/api/entities.rb b/lib/api/entities.rb index cc62b5a3661..2dd3120d3fc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -690,6 +690,10 @@ module API # Deprecated expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? } + expose :reference do |merge_request, options| + merge_request.to_reference(options[:project]) + end + expose :web_url do |merge_request| Gitlab::UrlBuilder.build(merge_request) end @@ -726,6 +730,8 @@ module API merge_request.metrics&.pipeline end + expose :head_pipeline, using: 'API::Entities::Pipeline' + expose :diff_refs, using: Entities::DiffRefs # Allow the status of a rebase to be determined @@ -1267,6 +1273,9 @@ module API expose :created_at, :updated_at, :started_at, :finished_at, :committed_at expose :duration expose :coverage + expose :detailed_status, using: DetailedStatusEntity do |pipeline, options| + pipeline.detailed_status(options[:current_user]) + end end class PipelineSchedule < Grape::Entity diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 9fcf476f537..ad16f26f5cc 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -26,6 +26,7 @@ module API optional :ldap_cn, type: String, desc: 'LDAP Common Name' optional :ldap_access, type: Integer, desc: 'A valid access level' optional :shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Pipeline minutes quota for this group' + optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this group' all_or_none_of :ldap_cn, :ldap_access end end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index fae20e45bf9..3dd90502050 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -310,7 +310,7 @@ module API .flatten present paginate(::Kaminari.paginate_array(merge_requests)), - with: Entities::MergeRequestBasic, + with: Entities::MergeRequest, current_user: current_user, project: user_project end diff --git a/lib/api/users.rb b/lib/api/users.rb index 776329622e2..2f23e33bd4a 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -54,6 +54,7 @@ module API if Gitlab.ee? optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user' + optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this user' end end diff --git a/lib/banzai/suggestions_parser.rb b/lib/banzai/suggestions_parser.rb deleted file mode 100644 index 0d7f751bfc1..00000000000 --- a/lib/banzai/suggestions_parser.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -# TODO: Delete when https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26107 -# exchange this parser by `Gitlab::Diff::SuggestionsParser`. -module Banzai - module SuggestionsParser - # Returns the content of each suggestion code block. - # - def self.parse(text) - html = Banzai.render(text, project: nil, no_original_data: true) - doc = Nokogiri::HTML(html) - - doc.search('pre.suggestion').map { |node| node.text } - end - end -end diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb index ac65cf74808..d347f3c13a4 100644 --- a/lib/gitlab/danger/helper.rb +++ b/lib/gitlab/danger/helper.rb @@ -7,6 +7,7 @@ require_relative 'teammate' module Gitlab module Danger module Helper + RELEASE_TOOLS_BOT = 'gitlab-release-tools-bot' ROULETTE_DATA_URL = URI.parse('https://about.gitlab.com/roulette.json').freeze # Returns a list of all files that have been added, modified or renamed. @@ -40,6 +41,10 @@ module Gitlab ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md') end + def release_automation? + gitlab.mr_author == RELEASE_TOOLS_BOT + end + def project_name ee? ? 'gitlab-ee' : 'gitlab-ce' end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 7d6851a4b8d..c33d243330d 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -231,12 +231,12 @@ module Gitlab end end - def archive_metadata(ref, storage_path, project_path, format = "tar.gz", append_sha:) + def archive_metadata(ref, storage_path, project_path, format = "tar.gz", append_sha:, path: nil) ref ||= root_ref commit = Gitlab::Git::Commit.find(self, ref) return {} if commit.nil? - prefix = archive_prefix(ref, commit.id, project_path, append_sha: append_sha) + prefix = archive_prefix(ref, commit.id, project_path, append_sha: append_sha, path: path) { 'ArchivePrefix' => prefix, @@ -248,13 +248,14 @@ module Gitlab # This is both the filename of the archive (missing the extension) and the # name of the top-level member of the archive under which all files go - def archive_prefix(ref, sha, project_path, append_sha:) + def archive_prefix(ref, sha, project_path, append_sha:, path:) append_sha = (ref != sha) if append_sha.nil? formatted_ref = ref.tr('/', '-') prefix_segments = [project_path, formatted_ref] prefix_segments << sha if append_sha + prefix_segments << path.tr('/', '-').gsub(%r{^/|/$}, '') if path prefix_segments.join('-') end diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb new file mode 100644 index 00000000000..f3ca82ec697 --- /dev/null +++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Authorize + class AuthorizeFieldService + def initialize(field) + @field = field + @old_resolve_proc = @field.resolve_proc + end + + def authorizations? + authorizations.present? + end + + def authorized_resolve + proc do |obj, args, ctx| + resolved_obj = @old_resolve_proc.call(obj, args, ctx) + checker = build_checker(ctx[:current_user]) + + if resolved_obj.respond_to?(:then) + resolved_obj.then(&checker) + else + checker.call(resolved_obj) + end + end + end + + private + + def authorizations + @authorizations ||= (type_authorizations + field_authorizations).uniq + end + + # Returns any authorize metadata from the return type of @field + def type_authorizations + type = @field.type + + # When the return type of @field is a collection, find the singular type + if type.get_field('edges') + type = node_type_for_relay_connection(type) + elsif type.list? + type = node_type_for_basic_connection(type) + end + + Array.wrap(type.metadata[:authorize]) + end + + # Returns any authorize metadata from @field + def field_authorizations + Array.wrap(@field.metadata[:authorize]) + end + + def build_checker(current_user) + lambda do |value| + # Load the elements if they were not loaded by BatchLoader yet + value = value.sync if value.respond_to?(:sync) + + check = lambda do |object| + authorizations.all? do |ability| + Ability.allowed?(current_user, ability, object) + end + end + + case value + when Array, ActiveRecord::Relation + value.select(&check) + else + value if check.call(value) + end + end + end + + # Returns the singular type for relay connections. + # This will be the type class of edges.node + def node_type_for_relay_connection(type) + type = type.get_field('edges').type.unwrap.get_field('node')&.type + + if type.nil? + raise Gitlab::Graphql::Errors::ConnectionDefinitionError, + 'Connection Type must conform to the Relay Cursor Connections Specification' + end + + type + end + + # Returns the singular type for basic connections, for example `[Types::ProjectType]` + def node_type_for_basic_connection(type) + type.unwrap + end + end + end + end +end diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb index 593da8471dd..15ecc3b04f0 100644 --- a/lib/gitlab/graphql/authorize/instrumentation.rb +++ b/lib/gitlab/graphql/authorize/instrumentation.rb @@ -7,46 +7,12 @@ module Gitlab # Replace the resolver for the field with one that will only return the # resolved object if the permissions check is successful. def instrument(_type, field) - required_permissions = Array.wrap(field.metadata[:authorize]) - return field if required_permissions.empty? + service = AuthorizeFieldService.new(field) - old_resolver = field.resolve_proc - - new_resolver = -> (obj, args, ctx) do - resolved_obj = old_resolver.call(obj, args, ctx) - checker = build_checker(ctx[:current_user], required_permissions) - - if resolved_obj.respond_to?(:then) - resolved_obj.then(&checker) - else - checker.call(resolved_obj) - end - end - - field.redefine do - resolve(new_resolver) - end - end - - private - - def build_checker(current_user, abilities) - lambda do |value| - # Load the elements if they weren't loaded by BatchLoader yet - value = value.sync if value.respond_to?(:sync) - - check = lambda do |object| - abilities.all? do |ability| - Ability.allowed?(current_user, ability, object) - end - end - - case value - when Array - value.select(&check) - else - value if check.call(value) - end + if service.authorizations? + field.redefine { resolve(service.authorized_resolve) } + else + field end end end diff --git a/lib/gitlab/graphql/errors.rb b/lib/gitlab/graphql/errors.rb index fe74549e322..bcbba72e017 100644 --- a/lib/gitlab/graphql/errors.rb +++ b/lib/gitlab/graphql/errors.rb @@ -6,6 +6,7 @@ module Gitlab BaseError = Class.new(GraphQL::ExecutionError) ArgumentError = Class.new(BaseError) ResourceNotAvailable = Class.new(BaseError) + ConnectionDefinitionError = Class.new(BaseError) end end end diff --git a/lib/gitlab/graphql/tracing.rb b/lib/gitlab/graphql/tracing.rb new file mode 100644 index 00000000000..6b505e4262b --- /dev/null +++ b/lib/gitlab/graphql/tracing.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + class Tracing < GraphQL::Tracing::PlatformTracing + self.platform_keys = { + 'lex' => 'graphql.lex', + 'parse' => 'graphql.parse', + 'validate' => 'graphql.validate', + 'analyze_query' => 'graphql.analyze', + 'analyze_multiplex' => 'graphql.analyze', + 'execute_multiplex' => 'graphql.execute', + 'execute_query' => 'graphql.execute', + 'execute_query_lazy' => 'graphql.execute', + 'execute_field' => 'graphql.execute', + 'execute_field_lazy' => 'graphql.execute' + } + + def platform_field_key(type, field) + "#{type.name}.#{field.name}" + end + + def platform_trace(platform_key, key, data, &block) + start = Gitlab::Metrics::System.monotonic_time + + yield + ensure + duration = Gitlab::Metrics::System.monotonic_time - start + + graphql_duration_seconds.observe({ platform_key: platform_key, key: key }, duration) + end + + private + + def graphql_duration_seconds + @graphql_duration_seconds ||= Gitlab::Metrics.histogram( + :graphql_duration_seconds, + 'GraphQL execution time' + ) + end + end + end +end diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index e91803ecd62..d7986685c65 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -8,6 +8,8 @@ module Gitlab # base labels shared among all transactions BASE_LABELS = { controller: nil, action: nil }.freeze + # labels that potentially contain sensitive information and will be filtered + FILTERED_LABELS = [:branch, :path].freeze THREAD_KEY = :_gitlab_metrics_transaction @@ -64,7 +66,7 @@ module Gitlab end def add_metric(series, values, tags = {}) - @metrics << Metric.new("#{::Gitlab::Metrics.series_prefix}#{series}", values, tags) + @metrics << Metric.new("#{::Gitlab::Metrics.series_prefix}#{series}", values, filter_tags(tags)) end # Tracks a business level event @@ -75,8 +77,9 @@ module Gitlab # event_name - The name of the event (e.g. "git_push"). # tags - A set of tags to attach to the event. def add_event(event_name, tags = {}) - self.class.transaction_metric(event_name, :counter, prefix: 'event_', use_feature_flag: true, tags: tags).increment(tags.merge(labels)) - @metrics << Metric.new(EVENT_SERIES, { count: 1 }, tags.merge(event: event_name), :event) + filtered_tags = filter_tags(tags) + self.class.transaction_metric(event_name, :counter, prefix: 'event_', use_feature_flag: true, tags: filtered_tags).increment(filtered_tags.merge(labels)) + @metrics << Metric.new(EVENT_SERIES, { count: 1 }, filtered_tags.merge(event: event_name), :event) end # Returns a MethodCall object for the given name. @@ -164,6 +167,12 @@ module Gitlab end end end + + private + + def filter_tags(tags) + tags.without(*FILTERED_LABELS) + end end end end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 3c888be0710..a07b1246bee 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -57,6 +57,7 @@ module Gitlab unsubscribes uploads users + v2 ].freeze # This list should contain all words following `/*namespace_id/:project_id` in diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 5d5a867c9ab..83eabb8d674 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -63,13 +63,26 @@ module Gitlab ] end - def send_git_archive(repository, ref:, format:, append_sha:) + def send_git_archive(repository, ref:, format:, append_sha:, path: nil) format ||= 'tar.gz' format = format.downcase - params = repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format, append_sha: append_sha) - raise "Repository or ref not found" if params.empty? + metadata = repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format, append_sha: append_sha, path: path) - params['GitalyServer'] = gitaly_server_hash(repository) + raise "Repository or ref not found" if metadata.empty? + + params = { + 'GitalyServer' => gitaly_server_hash(repository), + 'ArchivePath' => metadata['ArchivePath'], + 'GetArchiveRequest' => encode_binary( + Gitaly::GetArchiveRequest.new( + repository: repository.gitaly_repository, + commit_id: metadata['CommitId'], + prefix: metadata['ArchivePrefix'], + format: archive_format(format), + path: path.presence || "" + ).to_proto + ) + } # If present DisableCache must be a Boolean. Otherwise workhorse ignores it. params['DisableCache'] = true if git_archive_cache_disabled? @@ -220,6 +233,10 @@ module Gitlab Base64.urlsafe_encode64(JSON.dump(hash)) end + def encode_binary(binary) + Base64.urlsafe_encode64(binary) + end + def gitaly_server_hash(repository) { address: Gitlab::GitalyClient.address(repository.project.repository_storage), @@ -238,6 +255,19 @@ module Gitlab def git_archive_cache_disabled? ENV['WORKHORSE_ARCHIVE_CACHE_DISABLED'].present? || Feature.enabled?(:workhorse_archive_cache_disabled) end + + def archive_format(format) + case format + when "tar.bz2", "tbz", "tbz2", "tb2", "bz2" + Gitaly::GetArchiveRequest::Format::TAR_BZ2 + when "tar" + Gitaly::GetArchiveRequest::Format::TAR + when "zip" + Gitaly::GetArchiveRequest::Format::ZIP + else + Gitaly::GetArchiveRequest::Format::TAR_GZ + end + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f9c411642c7..9de34dd92ea 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3013,19 +3013,10 @@ msgstr "" msgid "Download asset" msgstr "" -msgid "Download tar" +msgid "Download source code" msgstr "" -msgid "Download tar.bz2" -msgstr "" - -msgid "Download tar.gz" -msgstr "" - -msgid "Download zip" -msgstr "" - -msgid "DownloadArtifacts|Download" +msgid "Download this directory" msgstr "" msgid "DownloadCommit|Email Patches" @@ -6028,6 +6019,9 @@ msgstr "" msgid "Preview payload" msgstr "" +msgid "Previous Artifacts" +msgstr "" + msgid "Prioritize" msgstr "" diff --git a/package.json b/package.json index ceb36a92cc8..711baf5d1a3 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "private": true, "scripts": { "clean": "rm -rf public/assets tmp/cache/*-loader", - "dev-server": "nodemon -w 'config/webpack.config.js' --exec 'webpack-dev-server --config config/webpack.config.js'", + "dev-server": "NODE_OPTIONS=\"--max-old-space-size=3584\" nodemon -w 'config/webpack.config.js' --exec 'webpack-dev-server --config config/webpack.config.js'", "eslint": "eslint --max-warnings 0 --report-unused-disable-directives --ext .js,.vue .", "eslint-fix": "eslint --max-warnings 0 --report-unused-disable-directives --ext .js,.vue --fix .", "eslint-report": "eslint --max-warnings 0 --ext .js,.vue --format html --output-file ./eslint-report.html --no-inline-config .", @@ -21,8 +21,8 @@ "stylelint-file": "node node_modules/stylelint/bin/stylelint.js", "stylelint-create-utility-map": "node scripts/frontend/stylelint/stylelint-utility-map.js", "test": "yarn jest && yarn karma", - "webpack": "webpack --config config/webpack.config.js", - "webpack-prod": "NODE_ENV=production webpack --config config/webpack.config.js" + "webpack": "NODE_OPTIONS=\"--max-old-space-size=3584\" webpack --config config/webpack.config.js", + "webpack-prod": "NODE_OPTIONS=\"--max-old-space-size=3584\" NODE_ENV=production webpack --config config/webpack.config.js" }, "dependencies": { "@babel/core": "^7.2.2", diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 356d606d5c5..56d38b9475e 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -703,6 +703,16 @@ describe ProjectsController do expect(JSON.parse(response.body).keys).to match_array(%w(body references)) end + context 'when not authorized' do + let(:private_project) { create(:project, :private) } + + it 'returns 404' do + post :preview_markdown, params: { namespace_id: private_project.namespace, id: private_project, text: '*Markdown* text' } + + expect(response).to have_gitlab_http_status(404) + end + end + context 'state filter on references' do let(:issue) { create(:issue, :closed, project: public_project) } let(:merge_request) { create(:merge_request, :closed, target_project: public_project) } diff --git a/spec/features/commits/user_uses_quick_actions_spec.rb b/spec/features/commits/user_uses_quick_actions_spec.rb index 9a4b7bd2444..4b7e7465df1 100644 --- a/spec/features/commits/user_uses_quick_actions_spec.rb +++ b/spec/features/commits/user_uses_quick_actions_spec.rb @@ -22,27 +22,6 @@ describe 'Commit > User uses quick actions', :js do let(:tag_message) { 'Stable release' } let(:truncated_commit_sha) { Commit.truncate_sha(commit.sha) } - it 'tags this commit' do - add_note("/tag #{tag_name} #{tag_message}") - - expect(page).to have_content 'Commands applied' - expect(page).to have_content "tagged commit #{truncated_commit_sha}" - expect(page).to have_content tag_name - - visit project_tag_path(project, tag_name) - expect(page).to have_content tag_name - expect(page).to have_content tag_message - expect(page).to have_content truncated_commit_sha - end - - describe 'preview', :js do - it 'removes quick action from note and explains it' do - preview_note("/tag #{tag_name} #{tag_message}") - - expect(page).not_to have_content '/tag' - expect(page).to have_content %{Tags this commit to #{tag_name} with "#{tag_message}"} - expect(page).to have_content tag_name - end - end + it_behaves_like 'tag quick action' end end diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index c19e299097e..1b5dd6945e0 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -6,6 +6,14 @@ describe 'User comments on a diff', :js do include MergeRequestDiffHelpers include RepoHelpers + def expect_suggestion_has_content(element, expected_changing_content, expected_suggested_content) + changing_content = element.all(:css, '.line_holder.old').map(&:text) + suggested_content = element.all(:css, '.line_holder.new').map(&:text) + + expect(changing_content).to eq(expected_changing_content) + expect(suggested_content).to eq(expected_suggested_content) + end + let(:project) { create(:project, :repository) } let(:merge_request) do create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test') @@ -33,8 +41,18 @@ describe 'User comments on a diff', :js do page.within('.diff-discussions') do expect(page).to have_button('Apply suggestion') expect(page).to have_content('Suggested change') - expect(page).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') - expect(page).to have_content('# change to a comment') + end + + page.within('.md-suggestion-diff') do + expected_changing_content = [ + "6 url = https://github.com/gitlabhq/gitlab-shell.git" + ] + + expected_suggested_content = [ + "6 # change to a comment" + ] + + expect_suggestion_has_content(page, expected_changing_content, expected_suggested_content) end end @@ -64,7 +82,7 @@ describe 'User comments on a diff', :js do click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do - fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion\n# or that\n```") + fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion:-2\n# or that\n# heh\n```") click_button('Comment') end @@ -74,11 +92,90 @@ describe 'User comments on a diff', :js do suggestion_1 = page.all(:css, '.md-suggestion-diff')[0] suggestion_2 = page.all(:css, '.md-suggestion-diff')[1] - expect(suggestion_1).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') - expect(suggestion_1).to have_content('# change to a comment') + suggestion_1_expected_changing_content = [ + "6 url = https://github.com/gitlabhq/gitlab-shell.git" + ] + suggestion_1_expected_suggested_content = [ + "6 # change to a comment" + ] + + suggestion_2_expected_changing_content = [ + "4 [submodule \"gitlab-shell\"]", + "5 path = gitlab-shell", + "6 url = https://github.com/gitlabhq/gitlab-shell.git" + ] + suggestion_2_expected_suggested_content = [ + "4 # or that", + "5 # heh" + ] + + expect_suggestion_has_content(suggestion_1, + suggestion_1_expected_changing_content, + suggestion_1_expected_suggested_content) + + expect_suggestion_has_content(suggestion_2, + suggestion_2_expected_changing_content, + suggestion_2_expected_suggested_content) + end + end + end + + context 'multi-line suggestions' do + it 'suggestion is presented' do + click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + + page.within('.js-discussion-note-form') do + fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```") + click_button('Comment') + end - expect(suggestion_2).to have_content(' url = https://github.com/gitlabhq/gitlab-shell.git') - expect(suggestion_2).to have_content('# or that') + wait_for_requests + + page.within('.diff-discussions') do + expect(page).to have_button('Apply suggestion') + expect(page).to have_content('Suggested change') + end + + page.within('.md-suggestion-diff') do + expected_changing_content = [ + "3 url = git://github.com/randx/six.git", + "4 [submodule \"gitlab-shell\"]", + "5 path = gitlab-shell", + "6 url = https://github.com/gitlabhq/gitlab-shell.git", + "7 [submodule \"gitlab-grack\"]", + "8 path = gitlab-grack", + "9 url = https://gitlab.com/gitlab-org/gitlab-grack.git" + ] + + expected_suggested_content = [ + "3 # change to a", + "4 # comment", + "5 # with", + "6 # broken", + "7 # lines" + ] + + expect_suggestion_has_content(page, expected_changing_content, expected_suggested_content) + end + end + + it 'suggestion is appliable' do + click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + + page.within('.js-discussion-note-form') do + fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```") + click_button('Comment') + end + + wait_for_requests + + page.within('.diff-discussions') do + expect(page).not_to have_content('Applied') + + click_button('Apply suggestion') + wait_for_requests + + expect(page).to have_content('Applied') end end end diff --git a/spec/features/merge_request/user_uses_quick_actions_spec.rb b/spec/features/merge_request/user_uses_quick_actions_spec.rb index a2b5859bd1e..56774896795 100644 --- a/spec/features/merge_request/user_uses_quick_actions_spec.rb +++ b/spec/features/merge_request/user_uses_quick_actions_spec.rb @@ -56,6 +56,8 @@ describe 'Merge request > User uses quick actions', :js do project.add_maintainer(user) end + it_behaves_like 'merge quick action' + describe 'toggling the WIP prefix in the title from note' do context 'when the current user can toggle the WIP prefix' do before do @@ -103,74 +105,6 @@ describe 'Merge request > User uses quick actions', :js do end end - describe 'merging the MR from the note' do - context 'when the current user can merge the MR' do - before do - sign_in(user) - visit project_merge_request_path(project, merge_request) - end - - it 'merges the MR' do - add_note("/merge") - - expect(page).to have_content 'Commands applied' - - expect(merge_request.reload).to be_merged - end - end - - context 'when the head diff changes in the meanwhile' do - before do - merge_request.source_branch = 'another_branch' - merge_request.save - sign_in(user) - visit project_merge_request_path(project, merge_request) - end - - it 'does not merge the MR' do - add_note("/merge") - - expect(page).not_to have_content 'Your commands have been executed!' - - expect(merge_request.reload).not_to be_merged - end - end - - context 'when the current user cannot merge the MR' do - before do - project.add_guest(guest) - sign_in(guest) - visit project_merge_request_path(project, merge_request) - end - - it 'does not merge the MR' do - add_note("/merge") - - expect(page).not_to have_content 'Your commands have been executed!' - - expect(merge_request.reload).not_to be_merged - end - end - end - - describe 'adding a due date from note' do - before do - sign_in(user) - visit project_merge_request_path(project, merge_request) - end - - it_behaves_like 'due quick action not available' - end - - describe 'removing a due date from note' do - before do - sign_in(user) - visit project_merge_request_path(project, merge_request) - end - - it_behaves_like 'remove_due_date action not available' - end - describe '/target_branch command in merge request' do let(:another_project) { create(:project, :public, :repository) } let(:new_url_opts) { { merge_request: { source_branch: 'feature' } } } diff --git a/spec/features/projects/branches/download_buttons_spec.rb b/spec/features/projects/branches/download_buttons_spec.rb index c8dc72a34ec..3e75890725e 100644 --- a/spec/features/projects/branches/download_buttons_spec.rb +++ b/spec/features/projects/branches/download_buttons_spec.rb @@ -35,7 +35,7 @@ describe 'Download buttons in branches page' do it 'shows download artifacts button' do href = latest_succeeded_project_artifacts_path(project, 'binary-encoding/download', job: 'build') - expect(page).to have_link "Download '#{build.name}'", href: href + expect(page).to have_link build.name, href: href end end end diff --git a/spec/features/projects/files/download_buttons_spec.rb b/spec/features/projects/files/download_buttons_spec.rb index 03cb3530e2b..111972a6b00 100644 --- a/spec/features/projects/files/download_buttons_spec.rb +++ b/spec/features/projects/files/download_buttons_spec.rb @@ -30,7 +30,7 @@ describe 'Projects > Files > Download buttons in files tree' do it 'shows download artifacts button' do href = latest_succeeded_project_artifacts_path(project, "#{project.default_branch}/download", job: 'build') - expect(page).to have_link "Download '#{build.name}'", href: href + expect(page).to have_link build.name, href: href end end end diff --git a/spec/features/projects/show/download_buttons_spec.rb b/spec/features/projects/show/download_buttons_spec.rb index 3a2dcc5aa55..fee5f8001b0 100644 --- a/spec/features/projects/show/download_buttons_spec.rb +++ b/spec/features/projects/show/download_buttons_spec.rb @@ -35,11 +35,10 @@ describe 'Projects > Show > Download buttons' do it 'shows download artifacts button' do href = latest_succeeded_project_artifacts_path(project, "#{project.default_branch}/download", job: 'build') - expect(page).to have_link "Download '#{build.name}'", href: href + expect(page).to have_link build.name, href: href end it 'download links have download attribute' do - expect(page).to have_selector('a', text: 'Download') page.all('a', text: 'Download').each do |link| expect(link[:download]).to eq '' end diff --git a/spec/features/projects/tags/download_buttons_spec.rb b/spec/features/projects/tags/download_buttons_spec.rb index fbfd8cee7aa..4c8ec53836a 100644 --- a/spec/features/projects/tags/download_buttons_spec.rb +++ b/spec/features/projects/tags/download_buttons_spec.rb @@ -36,7 +36,7 @@ describe 'Download buttons in tags page' do it 'shows download artifacts button' do href = latest_succeeded_project_artifacts_path(project, "#{tag}/download", job: 'build') - expect(page).to have_link "Download '#{build.name}'", href: href + expect(page).to have_link build.name, href: href end end end diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_request.json b/spec/fixtures/api/schemas/public_api/v4/merge_request.json index cd50be00418..918f2c4b47d 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_request.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_request.json @@ -119,6 +119,12 @@ "merge_status", "sha", "merge_commit_sha", "user_notes_count", "should_remove_source_branch", "force_remove_source_branch", "web_url", "squash" - ] + ], + "head_pipeline": { + "oneOf": [ + { "type": "null" }, + { "$ref": "pipeline/detail.json" } + ] + } } } diff --git a/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json b/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json index 56f86856dd4..a7207d2d991 100644 --- a/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json +++ b/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json @@ -13,6 +13,5 @@ "ref": { "type": "string" }, "status": { "type": "string" }, "web_url": { "type": "string" } - }, - "additionalProperties": false + } } diff --git a/spec/fixtures/api/schemas/public_api/v4/pipeline/detail.json b/spec/fixtures/api/schemas/public_api/v4/pipeline/detail.json new file mode 100644 index 00000000000..63e130d4055 --- /dev/null +++ b/spec/fixtures/api/schemas/public_api/v4/pipeline/detail.json @@ -0,0 +1,32 @@ +{ + "type": "object", + "allOf": [ + { "$ref": "basic.json" }, + { + "properties": { + "before_sha": { "type": ["string", "null"] }, + "tag": { "type": ["boolean"] }, + "yaml_errors": { "type": ["string", "null"] }, + "user": { + "anyOf": [ + { "type": ["object", "null"] }, + { "$ref": "../user/basic.json" } + ] + }, + "created_at": { "type": ["date", "null"] }, + "updated_at": { "type": ["date", "null"] }, + "started_at": { "type": ["date", "null"] }, + "finished_at": { "type": ["date", "null"] }, + "committed_at": { "type": ["date", "null"] }, + "duration": { "type": ["number", "null"] }, + "coverage": { "type": ["string", "null"] }, + "detailed_status": { + "oneOf": [ + { "type": "null" }, + { "$ref": "../../../status/ci_detailed_status.json" } + ] + } + } + } + ] +} diff --git a/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js b/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js new file mode 100644 index 00000000000..866d6eb05c6 --- /dev/null +++ b/spec/frontend/vue_shared/components/markdown/suggestion_diff_row_spec.js @@ -0,0 +1,98 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import SuggestionDiffRow from '~/vue_shared/components/markdown/suggestion_diff_row.vue'; + +const oldLine = { + can_receive_suggestion: false, + line_code: null, + meta_data: null, + new_line: null, + old_line: 5, + rich_text: '-oldtext', + text: '-oldtext', + type: 'old', +}; + +const newLine = { + can_receive_suggestion: false, + line_code: null, + meta_data: null, + new_line: 6, + old_line: null, + rich_text: '-newtext', + text: '-newtext', + type: 'new', +}; + +describe(SuggestionDiffRow.name, () => { + let wrapper; + + const factory = (options = {}) => { + const localVue = createLocalVue(); + + wrapper = shallowMount(SuggestionDiffRow, { + localVue, + ...options, + }); + }; + + const findOldLineWrapper = () => wrapper.find('.old_line'); + const findNewLineWrapper = () => wrapper.find('.new_line'); + + afterEach(() => { + wrapper.destroy(); + }); + + it('renders correctly', () => { + factory({ + propsData: { + line: oldLine, + }, + }); + + expect(wrapper.is('.line_holder')).toBe(true); + }); + + describe('when passed line has type old', () => { + beforeEach(() => { + factory({ + propsData: { + line: oldLine, + }, + }); + }); + + it('has old class when line has type old', () => { + expect(wrapper.find('td').classes()).toContain('old'); + }); + + it('has old line number rendered', () => { + expect(findOldLineWrapper().text()).toBe('5'); + }); + + it('has no new line number rendered', () => { + expect(findNewLineWrapper().text()).toBe(''); + }); + }); + + describe('when passed line has type new', () => { + beforeEach(() => { + factory({ + propsData: { + line: newLine, + }, + }); + }); + + it('has new class when line has type new', () => { + expect(wrapper.find('td').classes()).toContain('new'); + }); + + it('has no old line number rendered', () => { + expect(findOldLineWrapper().text()).toBe(''); + }); + + it('has no new line number rendered', () => { + expect(findNewLineWrapper().text()).toBe('6'); + }); + }); +}); diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index a229d29afa6..f863c4444b8 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -5,61 +5,192 @@ require 'spec_helper' describe 'Gitlab::Graphql::Authorization' do set(:user) { create(:user) } + let(:permission_single) { :foo } + let(:permission_collection) { [:foo, :bar] } let(:test_object) { double(name: 'My name') } - let(:object_type) { object_type_class } - let(:query_type) { query_type_class(object_type, test_object) } - let(:schema) { schema_class(query_type) } + let(:query_string) { '{ object() { name } }' } + let(:result) { execute_query(query_type)['data'] } - let(:execute) do - schema.execute( - query_string, - context: { current_user: user }, - variables: {} - ) + subject { result['object'] } + + shared_examples 'authorization with a single permission' do + it 'returns the protected field when user has permission' do + permit(permission_single) + + expect(subject).to eq('name' => test_object.name) + end + + it 'returns nil when user is not authorized' do + expect(subject).to be_nil + end end - let(:result) { execute['data'] } + shared_examples 'authorization with a collection of permissions' do + it 'returns the protected field when user has all permissions' do + permit(*permission_collection) + + expect(subject).to eq('name' => test_object.name) + end + + it 'returns nil when user only has one of the permissions' do + permit(permission_collection.first) + + expect(subject).to be_nil + end + + it 'returns nil when user only has none of the permissions' do + expect(subject).to be_nil + end + end before do # By default, disallow all permissions. allow(Ability).to receive(:allowed?).and_return(false) end - describe 'authorizing with a single permission' do - let(:query_string) { '{ singlePermission() { name } }' } + describe 'Field authorizations' do + let(:type) { type_factory } - subject { result['singlePermission'] } + describe 'with a single permission' do + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object }, authorize: permission_single + end + end + + include_examples 'authorization with a single permission' + end + + describe 'with a collection of permissions' do + let(:query_type) do + permissions = permission_collection + query_factory do |qt| + qt.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object } do + authorize permissions + end + end + end - it 'should return the protected field when user has permission' do - permit(:foo) + include_examples 'authorization with a collection of permissions' + end + end - expect(subject['name']).to eq(test_object.name) + describe 'Type authorizations' do + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object } + end end - it 'should return nil when user is not authorized' do - expect(subject).to be_nil + describe 'with a single permission' do + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end + + include_examples 'authorization with a single permission' + end + + describe 'with a collection of permissions' do + let(:type) do + type_factory do |type| + type.authorize permission_collection + end + end + + include_examples 'authorization with a collection of permissions' end end - describe 'authorizing with an Array of permissions' do - let(:query_string) { '{ permissionCollection() { name } }' } + describe 'type and field authorizations together' do + let(:permission_1) { permission_collection.first } + let(:permission_2) { permission_collection.last } - subject { result['permissionCollection'] } + let(:type) do + type_factory do |type| + type.authorize permission_1 + end + end - it 'should return the protected field when user has all permissions' do - permit(:foo, :bar) + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object }, authorize: permission_2 + end + end - expect(subject['name']).to eq(test_object.name) + include_examples 'authorization with a collection of permissions' + end + + describe 'type authorizations when applied to a relay connection' do + let(:query_string) { '{ object() { edges { node { name } } } }' } + + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, type.connection_type, null: true, resolve: ->(obj, args, ctx) { [test_object] } + end end - it 'should return nil when user only has one of the permissions' do - permit(:foo) + subject { result.dig('object', 'edges') } - expect(subject).to be_nil + it 'returns the protected field when user has permission' do + permit(permission_single) + + expect(subject).not_to be_empty + expect(subject.first['node']).to eq('name' => test_object.name) end - it 'should return nil when user only has none of the permissions' do - expect(subject).to be_nil + it 'returns nil when user is not authorized' do + expect(subject).to be_empty + end + end + + describe 'type authorizations when applied to a basic connection' do + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, [type], null: true, resolve: ->(obj, args, ctx) { [test_object] } + end + end + + subject { result['object'].first } + + include_examples 'authorization with a single permission' + end + + describe 'when connections do not follow the correct specification' do + let(:query_string) { '{ object() { edges { node { name }} } }' } + + let(:type) do + bad_node = type_factory do |type| + type.graphql_name 'BadNode' + type.field :bad_node, GraphQL::STRING_TYPE, null: true + end + + type_factory do |type| + type.field :edges, [bad_node], null: true + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true + end + end + + it 'throws an error' do + expect { result }.to raise_error(Gitlab::Graphql::Errors::ConnectionDefinitionError) end end @@ -71,36 +202,34 @@ describe 'Gitlab::Graphql::Authorization' do end end - def object_type_class + def type_factory Class.new(Types::BaseObject) do - graphql_name 'TestObject' + graphql_name 'TestType' field :name, GraphQL::STRING_TYPE, null: true + + yield(self) if block_given? end end - def query_type_class(type, object) + def query_factory Class.new(Types::BaseObject) do graphql_name 'TestQuery' - field :single_permission, type, - null: true, - authorize: :foo, - resolve: ->(obj, args, ctx) { object } - - field :permission_collection, type, - null: true, - resolve: ->(obj, args, ctx) { object } do - authorize [:foo, :bar] - end + yield(self) if block_given? end end - def schema_class(query) - Class.new(GraphQL::Schema) do + def execute_query(query_type) + schema = Class.new(GraphQL::Schema) do use Gitlab::Graphql::Authorize - - query(query) + query(query_type) end + + schema.execute( + query_string, + context: { current_user: user }, + variables: {} + ) end end diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb index 63a07647a60..dc37b15001f 100644 --- a/spec/graphql/types/issue_type_spec.rb +++ b/spec/graphql/types/issue_type_spec.rb @@ -4,4 +4,6 @@ describe GitlabSchema.types['Issue'] do it { expect(described_class).to expose_permissions_using(Types::PermissionTypes::Issue) } it { expect(described_class.graphql_name).to eq('Issue') } + + it { expect(described_class).to require_graphql_authorizations(:read_issue) } end diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb index c369953e3ea..89c12879074 100644 --- a/spec/graphql/types/merge_request_type_spec.rb +++ b/spec/graphql/types/merge_request_type_spec.rb @@ -3,14 +3,9 @@ require 'spec_helper' describe GitlabSchema.types['MergeRequest'] do it { expect(described_class).to expose_permissions_using(Types::PermissionTypes::MergeRequest) } - describe 'head pipeline' do - it 'has a head pipeline field' do - expect(described_class).to have_graphql_field(:head_pipeline) - end + it { expect(described_class).to require_graphql_authorizations(:read_merge_request) } - it 'authorizes the field' do - expect(described_class.fields['headPipeline']) - .to require_graphql_authorizations(:read_pipeline) - end + describe 'nested head pipeline' do + it { expect(described_class).to have_graphql_field(:head_pipeline) } end end diff --git a/spec/graphql/types/milestone_type_spec.rb b/spec/graphql/types/milestone_type_spec.rb new file mode 100644 index 00000000000..f7ee79eae9f --- /dev/null +++ b/spec/graphql/types/milestone_type_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GitlabSchema.types['Milestone'] do + it { expect(described_class.graphql_name).to eq('Milestone') } + + it { expect(described_class).to require_graphql_authorizations(:read_milestone) } +end diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index e8f1c84f8d6..e0ad09bdf22 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -5,19 +5,11 @@ describe GitlabSchema.types['Project'] do it { expect(described_class.graphql_name).to eq('Project') } + it { expect(described_class).to require_graphql_authorizations(:read_project) } + describe 'nested merge request' do it { expect(described_class).to have_graphql_field(:merge_requests) } it { expect(described_class).to have_graphql_field(:merge_request) } - - it 'authorizes the merge request' do - expect(described_class.fields['mergeRequest']) - .to require_graphql_authorizations(:read_merge_request) - end - - it 'authorizes the merge requests' do - expect(described_class.fields['mergeRequests']) - .to require_graphql_authorizations(:read_merge_request) - end end describe 'nested issues' do diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index 07c61ea7647..69e3ea8a4a9 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -15,10 +15,6 @@ describe GitlabSchema.types['Query'] do is_expected.to have_graphql_type(Types::ProjectType) is_expected.to have_graphql_resolver(Resolvers::ProjectResolver) end - - it 'authorizes with read_project' do - is_expected.to require_graphql_authorizations(:read_project) - end end describe 'metadata field' do diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb new file mode 100644 index 00000000000..8134cc13eb4 --- /dev/null +++ b/spec/graphql/types/user_type_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GitlabSchema.types['User'] do + it { expect(described_class.graphql_name).to eq('User') } + + it { expect(described_class).to require_graphql_authorizations(:read_user) } +end diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 348743081eb..1df5cf9ef68 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -44,8 +44,7 @@ export const noteableDataMock = { milestone: null, milestone_id: null, moved_to_id: null, - preview_note_path: - '/gitlab-org/gitlab-ce/preview_markdown?quick_actions_target_id=98&quick_actions_target_type=Issue', + preview_note_path: '/gitlab-org/gitlab-ce/preview_markdown?target_id=98&target_type=Issue', project_id: 2, state: 'opened', time_estimate: 0, @@ -347,8 +346,7 @@ export const loggedOutnoteableData = { }, noteable_note_url: '/group/project/merge_requests/1#note_1', create_note_path: '/gitlab-org/gitlab-ce/notes?target_id=98&target_type=issue', - preview_note_path: - '/gitlab-org/gitlab-ce/preview_markdown?quick_actions_target_id=98&quick_actions_target_type=Issue', + preview_note_path: '/gitlab-org/gitlab-ce/preview_markdown?target_id=98&target_type=Issue', }; export const collapseNotesMock = [ diff --git a/spec/javascripts/sidebar/todo_spec.js b/spec/javascripts/sidebar/todo_spec.js index 657e88ecb96..f46ea5a0499 100644 --- a/spec/javascripts/sidebar/todo_spec.js +++ b/spec/javascripts/sidebar/todo_spec.js @@ -116,7 +116,7 @@ describe('SidebarTodo', () => { const dataAttributes = { issuableId: '1', issuableType: 'epic', - originalTitle: 'Mark todo as done', + originalTitle: '', placement: 'left', container: 'body', boundary: 'viewport', @@ -130,6 +130,10 @@ describe('SidebarTodo', () => { }); }); + it('check button label computed property', () => { + expect(vm.buttonLabel).toEqual('Mark todo as done'); + }); + it('renders button label element when `collapsed` prop is `false`', () => { const buttonLabelEl = vm.$el.querySelector('span.issuable-todo-inner'); diff --git a/spec/javascripts/vue_shared/components/markdown/header_spec.js b/spec/javascripts/vue_shared/components/markdown/header_spec.js index e733a95288e..d4be2451f0b 100644 --- a/spec/javascripts/vue_shared/components/markdown/header_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/header_spec.js @@ -98,7 +98,7 @@ describe('Markdown field header component', () => { it('renders suggestion template', () => { vm.lineContent = 'Some content'; - expect(vm.mdSuggestion).toEqual('```suggestion\n{text}\n```'); + expect(vm.mdSuggestion).toEqual('```suggestion:-0+0\n{text}\n```'); }); it('does not render suggestion button if `canSuggest` is set to false', () => { diff --git a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js index f87c2a92f47..ea74cb9eb21 100644 --- a/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js @@ -1,21 +1,50 @@ import Vue from 'vue'; import SuggestionDiffComponent from '~/vue_shared/components/markdown/suggestion_diff.vue'; +import { selectDiffLines } from '~/vue_shared/components/lib/utils/diff_utils'; const MOCK_DATA = { canApply: true, - newLines: [ - { content: 'Line 1\n', lineNumber: 1 }, - { content: 'Line 2\n', lineNumber: 2 }, - { content: 'Line 3\n', lineNumber: 3 }, - ], - fromLine: 1, - fromContent: 'Old content', suggestion: { id: 1, + diff_lines: [ + { + can_receive_suggestion: false, + line_code: null, + meta_data: null, + new_line: null, + old_line: 5, + rich_text: '-test', + text: '-test', + type: 'old', + }, + { + can_receive_suggestion: true, + line_code: null, + meta_data: null, + new_line: 5, + old_line: null, + rich_text: '+new test', + text: '+new test', + type: 'new', + }, + { + can_receive_suggestion: true, + line_code: null, + meta_data: null, + new_line: 5, + old_line: null, + rich_text: '+new test2', + text: '+new test2', + type: 'new', + }, + ], }, helpPagePath: 'path_to_docs', }; +const lines = selectDiffLines(MOCK_DATA.suggestion.diff_lines); +const newLines = lines.filter(line => line.type === 'new'); + describe('Suggestion Diff component', () => { let vm; @@ -39,30 +68,23 @@ describe('Suggestion Diff component', () => { }); it('renders the oldLineNumber', () => { - const fromLine = vm.$el.querySelector('.qa-old-diff-line-number').innerHTML; + const fromLine = vm.$el.querySelector('.old_line').innerHTML; - expect(parseInt(fromLine, 10)).toBe(vm.fromLine); + expect(parseInt(fromLine, 10)).toBe(lines[0].old_line); }); it('renders the oldLineContent', () => { const fromContent = vm.$el.querySelector('.line_content.old').innerHTML; - expect(fromContent.includes(vm.fromContent)).toBe(true); - }); - - it('renders the contents of newLines', () => { - const newLines = vm.$el.querySelectorAll('.line_holder.new'); - - newLines.forEach((line, i) => { - expect(newLines[i].innerHTML.includes(vm.newLines[i].content)).toBe(true); - }); + expect(fromContent.includes(lines[0].text)).toBe(true); }); - it('renders a line number for each line', () => { - const newLineNumbers = vm.$el.querySelectorAll('.qa-new-diff-line-number'); + it('renders new lines', () => { + const newLinesElements = vm.$el.querySelectorAll('.line_holder.new'); - newLineNumbers.forEach((line, i) => { - expect(newLineNumbers[i].innerHTML.includes(vm.newLines[i].lineNumber)).toBe(true); + newLinesElements.forEach((line, i) => { + expect(newLinesElements[i].innerHTML.includes(newLines[i].new_line)).toBe(true); + expect(newLinesElements[i].innerHTML.includes(newLines[i].text)).toBe(true); }); }); }); diff --git a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js index 33be63a3a1e..b7de40b4831 100644 --- a/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/suggestions_spec.js @@ -2,46 +2,52 @@ import Vue from 'vue'; import SuggestionsComponent from '~/vue_shared/components/markdown/suggestions.vue'; const MOCK_DATA = { - fromLine: 1, - fromContent: 'Old content', - suggestions: [], + suggestions: [ + { + id: 1, + appliable: true, + applied: false, + current_user: { + can_apply: true, + }, + diff_lines: [ + { + can_receive_suggestion: false, + line_code: null, + meta_data: null, + new_line: null, + old_line: 5, + rich_text: '-test', + text: '-test', + type: 'old', + }, + { + can_receive_suggestion: true, + line_code: null, + meta_data: null, + new_line: 5, + old_line: null, + rich_text: '+new test', + text: '+new test', + type: 'new', + }, + ], + }, + ], noteHtml: ` + <div class="suggestion"> + <div class="line">-oldtest</div> + </div> <div class="suggestion"> - <div class="line">Suggestion 1</div> + <div class="line">+newtest</div> </div> - - <div class="suggestion"> - <div class="line">Suggestion 2</div> - </div> `, isApplied: false, helpPagePath: 'path_to_docs', }; -const generateLine = content => { - const line = document.createElement('div'); - line.className = 'line'; - line.innerHTML = content; - - return line; -}; - -const generateMockLines = () => { - const line1 = generateLine('Line 1'); - const line2 = generateLine('Line 2'); - const line3 = generateLine('- Line 3'); - const container = document.createElement('div'); - - container.appendChild(line1); - container.appendChild(line2); - container.appendChild(line3); - - return container; -}; - describe('Suggestion component', () => { let vm; - let extractedLines; let diffTable; beforeEach(done => { @@ -51,8 +57,7 @@ describe('Suggestion component', () => { propsData: MOCK_DATA, }).$mount(); - extractedLines = vm.extractNewLines(generateMockLines()); - diffTable = vm.generateDiff(extractedLines).$mount().$el; + diffTable = vm.generateDiff(0).$mount().$el; spyOn(vm, 'renderSuggestions'); vm.renderSuggestions(); @@ -70,32 +75,8 @@ describe('Suggestion component', () => { it('renders suggestions', () => { expect(vm.renderSuggestions).toHaveBeenCalled(); - expect(vm.$el.innerHTML.includes('Suggestion 1')).toBe(true); - expect(vm.$el.innerHTML.includes('Suggestion 2')).toBe(true); - }); - }); - - describe('extractNewLines', () => { - it('extracts suggested lines', () => { - const expectedReturn = [ - { content: 'Line 1\n', lineNumber: 1 }, - { content: 'Line 2\n', lineNumber: 2 }, - { content: '- Line 3\n', lineNumber: 3 }, - ]; - - expect(vm.extractNewLines(generateMockLines())).toEqual(expectedReturn); - }); - - it('increments line number for each extracted line', () => { - expect(extractedLines[0].lineNumber).toEqual(1); - expect(extractedLines[1].lineNumber).toEqual(2); - expect(extractedLines[2].lineNumber).toEqual(3); - }); - - it('returns empty array if no lines are found', () => { - const el = document.createElement('div'); - - expect(vm.extractNewLines(el)).toEqual([]); + expect(vm.$el.innerHTML.includes('oldtest')).toBe(true); + expect(vm.$el.innerHTML.includes('newtest')).toBe(true); }); }); @@ -109,17 +90,17 @@ describe('Suggestion component', () => { }); it('generates a diff table that contains contents the suggested lines', () => { - extractedLines.forEach((line, i) => { - expect(diffTable.innerHTML.includes(extractedLines[i].content)).toBe(true); + MOCK_DATA.suggestions[0].diff_lines.forEach(line => { + const text = line.text.substring(1); + + expect(diffTable.innerHTML.includes(text)).toBe(true); }); }); it('generates a diff table with the correct line number for each suggested line', () => { - const lines = diffTable.getElementsByClassName('qa-new-diff-line-number'); + const lines = diffTable.querySelectorAll('.old_line'); - expect([...lines][0].innerHTML).toBe('1'); - expect([...lines][1].innerHTML).toBe('2'); - expect([...lines][2].innerHTML).toBe('3'); + expect(parseInt([...lines][0].innerHTML, 10)).toBe(5); }); }); }); diff --git a/spec/lib/banzai/suggestions_parser_spec.rb b/spec/lib/banzai/suggestions_parser_spec.rb deleted file mode 100644 index 79658d710ce..00000000000 --- a/spec/lib/banzai/suggestions_parser_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Banzai::SuggestionsParser do - describe '.parse' do - it 'returns a list of suggestion contents' do - markdown = <<-MARKDOWN.strip_heredoc - ```suggestion - foo - bar - ``` - - ``` - nothing - ``` - - ```suggestion - xpto - baz - ``` - - ```thing - this is not a suggestion, it's a thing - ``` - MARKDOWN - - expect(described_class.parse(markdown)).to eq([" foo\n bar", - " xpto\n baz"]) - end - end -end diff --git a/spec/lib/gitlab/diff/suggestion_spec.rb b/spec/lib/gitlab/diff/suggestion_spec.rb index 71fd25df698..d7ca0e0a522 100644 --- a/spec/lib/gitlab/diff/suggestion_spec.rb +++ b/spec/lib/gitlab/diff/suggestion_spec.rb @@ -10,6 +10,16 @@ describe Gitlab::Diff::Suggestion do lines_above: above, lines_below: below) end + + it 'returns diff lines with correct line numbers' do + diff_lines = suggestion.diff_lines + + expect(diff_lines).to all(be_a(Gitlab::Diff::Line)) + + expected_diff_lines.each_with_index do |expected_line, index| + expect(diff_lines[index].to_hash).to include(expected_line) + end + end end let(:merge_request) { create(:merge_request) } @@ -48,6 +58,18 @@ describe Gitlab::Diff::Suggestion do let(:expected_above) { line - 1 } let(:expected_below) { below } let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) } + let(:expected_diff_lines) do + [ + { old_pos: 1, new_pos: 1, type: 'old', text: "-require 'fileutils'" }, + { old_pos: 2, new_pos: 1, type: 'old', text: "-require 'open3'" }, + { old_pos: 3, new_pos: 1, type: 'old', text: "-" }, + { old_pos: 4, new_pos: 1, type: 'old', text: "-module Popen" }, + { old_pos: 5, new_pos: 1, type: 'old', text: "- extend self" }, + { old_pos: 6, new_pos: 1, type: 'old', text: "-" }, + { old_pos: 7, new_pos: 1, type: 'new', text: "+# parsed suggestion content" }, + { old_pos: 7, new_pos: 2, type: 'new', text: "+# with comments" } + ] + end it_behaves_like 'correct suggestion raw content' end @@ -59,6 +81,47 @@ describe Gitlab::Diff::Suggestion do let(:expected_below) { below } let(:expected_above) { above } let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) } + let(:expected_diff_lines) do + [ + { old_pos: 4, new_pos: 4, type: "match", text: "@@ -4 +4" }, + { old_pos: 4, new_pos: 4, type: "old", text: "-module Popen" }, + { old_pos: 5, new_pos: 4, type: "old", text: "- extend self" }, + { old_pos: 6, new_pos: 4, type: "old", text: "-" }, + { old_pos: 7, new_pos: 4, type: "old", text: "- def popen(cmd, path=nil)" }, + { old_pos: 8, new_pos: 4, type: "old", text: "- unless cmd.is_a?(Array)" }, + { old_pos: 9, new_pos: 4, type: "old", text: "- raise RuntimeError, \"System commands must be given as an array of strings\"" }, + { old_pos: 10, new_pos: 4, type: "old", text: "- end" }, + { old_pos: 11, new_pos: 4, type: "old", text: "-" }, + { old_pos: 12, new_pos: 4, type: "old", text: "- path ||= Dir.pwd" }, + { old_pos: 13, new_pos: 4, type: "old", text: "-" }, + { old_pos: 14, new_pos: 4, type: "old", text: "- vars = {" }, + { old_pos: 15, new_pos: 4, type: "old", text: "- \"PWD\" => path" }, + { old_pos: 16, new_pos: 4, type: "old", text: "- }" }, + { old_pos: 17, new_pos: 4, type: "old", text: "-" }, + { old_pos: 18, new_pos: 4, type: "old", text: "- options = {" }, + { old_pos: 19, new_pos: 4, type: "old", text: "- chdir: path" }, + { old_pos: 20, new_pos: 4, type: "old", text: "- }" }, + { old_pos: 21, new_pos: 4, type: "old", text: "-" }, + { old_pos: 22, new_pos: 4, type: "old", text: "- unless File.directory?(path)" }, + { old_pos: 23, new_pos: 4, type: "old", text: "- FileUtils.mkdir_p(path)" }, + { old_pos: 24, new_pos: 4, type: "old", text: "- end" }, + { old_pos: 25, new_pos: 4, type: "old", text: "-" }, + { old_pos: 26, new_pos: 4, type: "old", text: "- @cmd_output = \"\"" }, + { old_pos: 27, new_pos: 4, type: "old", text: "- @cmd_status = 0" }, + { old_pos: 28, new_pos: 4, type: "old", text: "-" }, + { old_pos: 29, new_pos: 4, type: "old", text: "- Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|" }, + { old_pos: 30, new_pos: 4, type: "old", text: "- @cmd_output << stdout.read" }, + { old_pos: 31, new_pos: 4, type: "old", text: "- @cmd_output << stderr.read" }, + { old_pos: 32, new_pos: 4, type: "old", text: "- @cmd_status = wait_thr.value.exitstatus" }, + { old_pos: 33, new_pos: 4, type: "old", text: "- end" }, + { old_pos: 34, new_pos: 4, type: "old", text: "-" }, + { old_pos: 35, new_pos: 4, type: "old", text: "- return @cmd_output, @cmd_status" }, + { old_pos: 36, new_pos: 4, type: "old", text: "- end" }, + { old_pos: 37, new_pos: 4, type: "old", text: "-end" }, + { old_pos: 38, new_pos: 4, type: "new", text: "+# parsed suggestion content" }, + { old_pos: 38, new_pos: 5, type: "new", text: "+# with comments" } + ] + end it_behaves_like 'correct suggestion raw content' end @@ -70,17 +133,19 @@ describe Gitlab::Diff::Suggestion do let(:expected_below) { below } let(:expected_above) { above } let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) } - - it_behaves_like 'correct suggestion raw content' - end - - context 'when no extra lines (single-line suggestion)' do - let(:line) { 5 } - let(:above) { 0 } - let(:below) { 0 } - let(:expected_below) { below } - let(:expected_above) { above } - let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) } + let(:expected_diff_lines) do + [ + { old_pos: 3, new_pos: 3, type: "match", text: "@@ -3 +3" }, + { old_pos: 3, new_pos: 3, type: "old", text: "-" }, + { old_pos: 4, new_pos: 3, type: "old", text: "-module Popen" }, + { old_pos: 5, new_pos: 3, type: "old", text: "- extend self" }, + { old_pos: 6, new_pos: 3, type: "old", text: "-" }, + { old_pos: 7, new_pos: 3, type: "old", text: "- def popen(cmd, path=nil)" }, + { old_pos: 8, new_pos: 3, type: "old", text: "- unless cmd.is_a?(Array)" }, + { old_pos: 9, new_pos: 3, type: "new", text: "+# parsed suggestion content" }, + { old_pos: 9, new_pos: 4, type: "new", text: "+# with comments" } + ] + end it_behaves_like 'correct suggestion raw content' end diff --git a/spec/lib/gitlab/diff/suggestions_parser_spec.rb b/spec/lib/gitlab/diff/suggestions_parser_spec.rb index 1119ea04995..1f2af42f6e7 100644 --- a/spec/lib/gitlab/diff/suggestions_parser_spec.rb +++ b/spec/lib/gitlab/diff/suggestions_parser_spec.rb @@ -69,5 +69,66 @@ describe Gitlab::Diff::SuggestionsParser do lines_below: 0) end end + + context 'multi-line suggestions' do + let(:markdown) do + <<-MARKDOWN.strip_heredoc + ```suggestion:-2+1 + # above and below + ``` + + ``` + nothing + ``` + + ```suggestion:-3 + # only above + ``` + + ```suggestion:+3 + # only below + ``` + + ```thing + this is not a suggestion, it's a thing + ``` + MARKDOWN + end + + it 'returns a list of Gitlab::Diff::Suggestion' do + expect(subject).to all(be_a(Gitlab::Diff::Suggestion)) + expect(subject.size).to eq(3) + end + + it 'suggestion with above and below param has correct data' do + from_line = position.new_line - 2 + to_line = position.new_line + 1 + + expect(subject.first.to_hash).to include(from_content: blob_lines_data(from_line, to_line), + to_content: " # above and below\n", + lines_above: 2, + lines_below: 1) + end + + it 'suggestion with above param has correct data' do + from_line = position.new_line - 3 + to_line = position.new_line + + expect(subject.second.to_hash).to eq(from_content: blob_lines_data(from_line, to_line), + to_content: " # only above\n", + lines_above: 3, + lines_below: 0) + end + + it 'suggestion with below param has correct data' do + from_line = position.new_line + to_line = position.new_line + 3 + + expect(subject.third.to_hash).to eq(from_content: blob_lines_data(from_line, to_line), + to_content: " # only below\n", + lines_above: 0, + lines_below: 3) + end + end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 8ba6862392c..fc8f590068a 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -152,13 +152,14 @@ describe Gitlab::Git::Repository, :seed_helper do let(:append_sha) { true } let(:ref) { 'master' } let(:format) { nil } + let(:path) { nil } let(:expected_extension) { 'tar.gz' } let(:expected_filename) { "#{expected_prefix}.#{expected_extension}" } let(:expected_path) { File.join(storage_path, cache_key, expected_filename) } let(:expected_prefix) { "gitlab-git-test-#{ref}-#{SeedRepo::LastCommit::ID}" } - subject(:metadata) { repository.archive_metadata(ref, storage_path, 'gitlab-git-test', format, append_sha: append_sha) } + subject(:metadata) { repository.archive_metadata(ref, storage_path, 'gitlab-git-test', format, append_sha: append_sha, path: path) } it 'sets CommitId to the commit SHA' do expect(metadata['CommitId']).to eq(SeedRepo::LastCommit::ID) @@ -176,6 +177,14 @@ describe Gitlab::Git::Repository, :seed_helper do expect(metadata['ArchivePath']).to eq(expected_path) end + context 'path is set' do + let(:path) { 'foo/bar' } + + it 'appends the path to the prefix' do + expect(metadata['ArchivePrefix']).to eq("#{expected_prefix}-foo-bar") + end + end + context 'append_sha varies archive path and filename' do where(:append_sha, :ref, :expected_prefix) do sha = SeedRepo::LastCommit::ID diff --git a/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb index cf3a8bcc8b4..ce320a2bdb0 100644 --- a/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb @@ -2,13 +2,17 @@ require 'spec_helper' -describe Gitlab::Graphql::Authorize::Instrumentation do +# Also see spec/graphql/features/authorization_spec.rb for +# integration tests of AuthorizeFieldService +describe Gitlab::Graphql::Authorize::AuthorizeFieldService do describe '#build_checker' do let(:current_user) { double(:current_user) } let(:abilities) { [double(:first_ability), double(:last_ability)] } let(:checker) do - described_class.new.__send__(:build_checker, current_user, abilities) + service = described_class.new(double(resolve_proc: proc {})) + allow(service).to receive(:authorizations).and_return(abilities) + service.__send__(:build_checker, current_user) end it 'returns a checker which checks for a single object' do @@ -56,12 +60,14 @@ describe Gitlab::Graphql::Authorize::Instrumentation do .to contain_exactly(allowed) end end + end - def spy_ability_check_for(ability, object, passed: true) - expect(Ability) - .to receive(:allowed?) - .with(current_user, ability, object) - .and_return(passed) - end + private + + def spy_ability_check_for(ability, object, passed: true) + expect(Ability) + .to receive(:allowed?) + .with(current_user, ability, object) + .and_return(passed) end end diff --git a/spec/lib/gitlab/graphql/tracing_spec.rb b/spec/lib/gitlab/graphql/tracing_spec.rb new file mode 100644 index 00000000000..7300a9a572e --- /dev/null +++ b/spec/lib/gitlab/graphql/tracing_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Graphql::Tracing do + let(:graphql_duration_seconds_histogram) { double('Gitlab::Metrics::NullMetric') } + + it 'updates graphql histogram with expected labels' do + query = 'query { users { id } }' + tracer = described_class.new + + allow(tracer) + .to receive(:graphql_duration_seconds) + .and_return(graphql_duration_seconds_histogram) + + expect_metric('graphql.lex', 'lex') + expect_metric('graphql.parse', 'parse') + expect_metric('graphql.validate', 'validate') + expect_metric('graphql.analyze', 'analyze_multiplex') + expect_metric('graphql.execute', 'execute_query_lazy') + expect_metric('graphql.execute', 'execute_multiplex') + + GitlabSchema.execute(query, context: { tracers: [tracer] }) + end + + private + + def expect_metric(platform_key, key) + expect(graphql_duration_seconds_histogram) + .to receive(:observe) + .with({ platform_key: platform_key, key: key }, be > 0.0) + end +end diff --git a/spec/lib/gitlab/metrics/transaction_spec.rb b/spec/lib/gitlab/metrics/transaction_spec.rb new file mode 100644 index 00000000000..e70fde09edd --- /dev/null +++ b/spec/lib/gitlab/metrics/transaction_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Metrics::Transaction do + let(:transaction) { described_class.new } + let(:metric) { transaction.metrics[0] } + + let(:sensitive_tags) do + { + path: 'private', + branch: 'sensitive' + } + end + + shared_examples 'tag filter' do |sane_tags| + it 'filters potentially sensitive tags' do + expect(metric.tags).to eq(sane_tags) + end + end + + describe '#duration' do + it 'returns the duration of a transaction in seconds' do + transaction.run { } + + expect(transaction.duration).to be > 0 + end + end + + describe '#allocated_memory' do + it 'returns the allocated memory in bytes' do + transaction.run { 'a' * 32 } + + expect(transaction.allocated_memory).to be_a_kind_of(Numeric) + end + end + + describe '#run' do + it 'yields the supplied block' do + expect { |b| transaction.run(&b) }.to yield_control + end + + it 'stores the transaction in the current thread' do + transaction.run do + expect(described_class.current).to eq(transaction) + end + end + + it 'removes the transaction from the current thread upon completion' do + transaction.run { } + + expect(described_class.current).to be_nil + end + end + + describe '#add_metric' do + it 'adds a metric to the transaction' do + transaction.add_metric('foo', value: 1) + + expect(metric.series).to eq('rails_foo') + expect(metric.tags).to eq({}) + expect(metric.values).to eq(value: 1) + end + + context 'with sensitive tags' do + before do + transaction + .add_metric('foo', { value: 1 }, **sensitive_tags.merge(sane: 'yes')) + end + + it_behaves_like 'tag filter', sane: 'yes' + end + end + + describe '#method_call_for' do + it 'returns a MethodCall' do + method = transaction.method_call_for('Foo#bar', :Foo, '#bar') + + expect(method).to be_an_instance_of(Gitlab::Metrics::MethodCall) + end + end + + describe '#increment' do + it 'increments a counter' do + transaction.increment(:time, 1) + transaction.increment(:time, 2) + + values = metric_values(time: 3) + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#set' do + it 'sets a value' do + transaction.set(:number, 10) + + values = metric_values(number: 10) + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#finish' do + it 'tracks the transaction details and submits them to Sidekiq' do + expect(transaction).to receive(:track_self) + expect(transaction).to receive(:submit) + + transaction.finish + end + end + + describe '#track_self' do + it 'adds a metric for the transaction itself' do + values = metric_values + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#submit' do + it 'submits the metrics to Sidekiq' do + transaction.track_self + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([an_instance_of(Hash)]) + + transaction.submit + end + + it 'adds the action as a tag for every metric' do + allow(transaction) + .to receive(:labels) + .and_return(controller: 'Foo', action: 'bar') + + transaction.track_self + + hash = { + series: 'rails_transactions', + tags: { action: 'Foo#bar' }, + values: metric_values, + timestamp: a_kind_of(Integer) + } + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([hash]) + + transaction.submit + end + + it 'does not add an action tag for events' do + allow(transaction) + .to receive(:labels) + .and_return(controller: 'Foo', action: 'bar') + + transaction.add_event(:meow) + + hash = { + series: 'events', + tags: { event: :meow }, + values: { count: 1 }, + timestamp: a_kind_of(Integer) + } + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([hash]) + + transaction.submit + end + end + + describe '#add_event' do + it 'adds a metric' do + transaction.add_event(:meow) + + expect(metric).to be_an_instance_of(Gitlab::Metrics::Metric) + end + + it "does not prefix the metric's series name" do + transaction.add_event(:meow) + + expect(metric.series).to eq(described_class::EVENT_SERIES) + end + + it 'tracks a counter for every event' do + transaction.add_event(:meow) + + expect(metric.values).to eq(count: 1) + end + + it 'tracks the event name' do + transaction.add_event(:meow) + + expect(metric.tags).to eq(event: :meow) + end + + it 'allows tracking of custom tags' do + transaction.add_event(:meow, animal: 'cat') + + expect(metric.tags).to eq(event: :meow, animal: 'cat') + end + + context 'with sensitive tags' do + before do + transaction.add_event(:meow, **sensitive_tags.merge(sane: 'yes')) + end + + it_behaves_like 'tag filter', event: :meow, sane: 'yes' + end + end + + private + + def metric_values(opts = {}) + { + duration: 0.0, + allocated_memory: a_kind_of(Numeric) + }.merge(opts) + end +end diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index 312e5e55af8..71e69a0d418 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -100,7 +100,7 @@ describe Gitlab::PathRegex do end let(:ee_top_level_words) do - ['unsubscribes'] + %w(unsubscribes v2) end let(:files_in_public) do diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index d88086b01b1..fed7834e2a9 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -16,20 +16,12 @@ describe Gitlab::Workhorse do let(:ref) { 'master' } let(:format) { 'zip' } let(:storage_path) { Gitlab.config.gitlab.repository_downloads_path } - let(:base_params) { repository.archive_metadata(ref, storage_path, format, append_sha: nil) } - let(:gitaly_params) do - base_params.merge( - 'GitalyServer' => { - 'address' => Gitlab::GitalyClient.address(project.repository_storage), - 'token' => Gitlab::GitalyClient.token(project.repository_storage) - }, - 'GitalyRepository' => repository.gitaly_repository.to_h.deep_stringify_keys - ) - end + let(:path) { 'some/path' } + let(:metadata) { repository.archive_metadata(ref, storage_path, format, append_sha: nil, path: path) } let(:cache_disabled) { false } subject do - described_class.send_git_archive(repository, ref: ref, format: format, append_sha: nil) + described_class.send_git_archive(repository, ref: ref, format: format, append_sha: nil, path: path) end before do @@ -41,7 +33,22 @@ describe Gitlab::Workhorse do expect(key).to eq('Gitlab-Workhorse-Send-Data') expect(command).to eq('git-archive') - expect(params).to include(gitaly_params) + expect(params).to eq({ + 'GitalyServer' => { + address: Gitlab::GitalyClient.address(project.repository_storage), + token: Gitlab::GitalyClient.token(project.repository_storage) + }, + 'ArchivePath' => metadata['ArchivePath'], + 'GetArchiveRequest' => Base64.urlsafe_encode64( + Gitaly::GetArchiveRequest.new( + repository: repository.gitaly_repository, + commit_id: metadata['CommitId'], + prefix: metadata['ArchivePrefix'], + format: Gitaly::GetArchiveRequest::Format::ZIP, + path: path + ).to_proto + ) + }.deep_stringify_keys) end context 'when archive caching is disabled' do diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 3c8897ed37c..5fa1369c00a 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -30,6 +30,19 @@ describe Notify do description: 'My awesome description!') end + describe 'with HTML-encoded entities' do + before do + described_class.test_email('test@test.com', 'Subject', 'Some body with —').deliver + end + + subject { ActionMailer::Base.deliveries.last } + + it 'retains 7bit encoding' do + expect(subject.body.ascii_only?).to eq(true) + expect(subject.body.encoding).to eq('7bit') + end + end + context 'for a project' do shared_examples 'an assignee email' do it 'is sent to the assignee as the author' do diff --git a/spec/models/suggestion_spec.rb b/spec/models/suggestion_spec.rb index cafc725dddb..8d4e9070b19 100644 --- a/spec/models/suggestion_spec.rb +++ b/spec/models/suggestion_spec.rb @@ -21,6 +21,22 @@ describe Suggestion do end end + describe '#diff_lines' do + let(:suggestion) { create(:suggestion, :content_from_repo) } + + it 'returns parsed diff lines' do + expected_diff_lines = Gitlab::Diff::SuggestionDiff.new(suggestion).diff_lines + diff_lines = suggestion.diff_lines + + expect(diff_lines.size).to eq(expected_diff_lines.size) + expect(diff_lines).to all(be_a(Gitlab::Diff::Line)) + + expected_diff_lines.each_with_index do |expected_line, index| + expect(diff_lines[index].to_hash).to eq(expected_line.to_hash) + end + end + end + describe '#appliable?' do context 'when note does not support suggestions' do it 'returns false' do diff --git a/spec/presenters/ci/bridge_presenter_spec.rb b/spec/presenters/ci/bridge_presenter_spec.rb new file mode 100644 index 00000000000..986818a7b9e --- /dev/null +++ b/spec/presenters/ci/bridge_presenter_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe Ci::BridgePresenter do + set(:project) { create(:project) } + set(:pipeline) { create(:ci_pipeline, project: project) } + set(:bridge) { create(:ci_bridge, pipeline: pipeline, status: :failed) } + + subject(:presenter) do + described_class.new(bridge) + end + + it 'presents information about recoverable state' do + expect(presenter).to be_recoverable + end +end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4259fda7f04..7ffa365c651 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -729,6 +729,14 @@ describe API::MergeRequests do end describe "GET /projects/:id/merge_requests/:merge_request_iid" do + it 'matches json schema' do + merge_request = create(:merge_request, :with_test_reports, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to match_response_schema('public_api/v4/merge_request') + end + it 'exposes known attributes' do create(:award_emoji, :downvote, awardable: merge_request) create(:award_emoji, :upvote, awardable: merge_request) @@ -1353,7 +1361,12 @@ describe API::MergeRequests do end it 'returns 405 if the build failed for a merge request that requires success' do - allow_any_instance_of(MergeRequest).to receive(:mergeable_ci_state?).and_return(false) + project.update!(only_allow_merge_if_pipeline_succeeds: true) + + create(:ci_pipeline, + :failed, + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user) diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index 52599db9a9e..c26d31c5e0d 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -399,6 +399,13 @@ describe API::Pipelines do describe 'GET /projects/:id/pipelines/:pipeline_id' do context 'authorized user' do + it 'exposes known attributes' do + get api("/projects/#{project.id}/pipelines/#{pipeline.id}", user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to match_response_schema('public_api/v4/pipeline/detail') + end + it 'returns project pipelines' do get api("/projects/#{project.id}/pipelines/#{pipeline.id}", user) diff --git a/spec/serializers/suggestion_entity_spec.rb b/spec/serializers/suggestion_entity_spec.rb index d38fc2b132b..d282a7f9c7a 100644 --- a/spec/serializers/suggestion_entity_spec.rb +++ b/spec/serializers/suggestion_entity_spec.rb @@ -13,8 +13,7 @@ describe SuggestionEntity do subject { entity.as_json } it 'exposes correct attributes' do - expect(subject).to include(:id, :from_line, :to_line, :appliable, - :applied, :from_content, :to_content) + expect(subject.keys).to match_array([:id, :appliable, :applied, :diff_lines, :current_user]) end it 'exposes current user abilities' do diff --git a/spec/services/clusters/applications/check_installation_progress_service_spec.rb b/spec/services/clusters/applications/check_installation_progress_service_spec.rb index 19446ce1cf8..1cca89d31d7 100644 --- a/spec/services/clusters/applications/check_installation_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_installation_progress_service_spec.rb @@ -33,14 +33,22 @@ describe Clusters::Applications::CheckInstallationProgressService, '#execute' do end end - shared_examples 'error logging' do + shared_examples 'error handling' do context 'when installation raises a Kubeclient::HttpError' do let(:cluster) { create(:cluster, :provided_by_user, :project) } + let(:logger) { service.send(:logger) } + let(:error) { Kubeclient::HttpError.new(401, 'Unauthorized', nil) } before do application.update!(cluster: cluster) - expect(service).to receive(:installation_phase).and_raise(Kubeclient::HttpError.new(401, 'Unauthorized', nil)) + expect(service).to receive(:installation_phase).and_raise(error) + end + + include_examples 'logs kubernetes errors' do + let(:error_name) { 'Kubeclient::HttpError' } + let(:error_message) { 'Unauthorized' } + let(:error_code) { 401 } end it 'shows the response code from the error' do @@ -49,12 +57,6 @@ describe Clusters::Applications::CheckInstallationProgressService, '#execute' do expect(application).to be_errored.or(be_update_errored) expect(application.status_reason).to eq('Kubernetes error: 401') end - - it 'should log error' do - expect(service.send(:logger)).to receive(:error) - - service.execute - end end end @@ -66,7 +68,7 @@ describe Clusters::Applications::CheckInstallationProgressService, '#execute' do context 'when application is updating' do let(:application) { create(:clusters_applications_helm, :updating) } - include_examples 'error logging' + include_examples 'error handling' RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } @@ -127,7 +129,7 @@ describe Clusters::Applications::CheckInstallationProgressService, '#execute' do end context 'when application is installing' do - include_examples 'error logging' + include_examples 'error handling' RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } diff --git a/spec/services/clusters/applications/install_service_spec.rb b/spec/services/clusters/applications/install_service_spec.rb index 018d9822d3e..db0c33a95b2 100644 --- a/spec/services/clusters/applications/install_service_spec.rb +++ b/spec/services/clusters/applications/install_service_spec.rb @@ -39,51 +39,34 @@ describe Clusters::Applications::InstallService do expect(helm_client).to receive(:install).with(install_command).and_raise(error) end + include_examples 'logs kubernetes errors' do + let(:error_name) { 'Kubeclient::HttpError' } + let(:error_message) { 'system failure' } + let(:error_code) { 500 } + end + it 'make the application errored' do service.execute expect(application).to be_errored expect(application.status_reason).to match('Kubernetes error: 500') end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::InstallService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::InstallService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - service.execute - end end context 'a non kubernetes error happens' do let(:application) { create(:clusters_applications_helm, :scheduled) } - let(:error) { StandardError.new("something bad happened") } + let(:error) { StandardError.new('something bad happened') } before do expect(application).to receive(:make_installing!).once.and_raise(error) end + include_examples 'logs kubernetes errors' do + let(:error_name) { 'StandardError' } + let(:error_message) { 'something bad happened' } + let(:error_code) { nil } + end + it 'make the application errored' do expect(helm_client).not_to receive(:install) @@ -92,35 +75,6 @@ describe Clusters::Applications::InstallService do expect(application).to be_errored expect(application.status_reason).to eq("Can't start installation process.") end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::InstallService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::InstallService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - service.execute - end end end end diff --git a/spec/services/clusters/applications/patch_service_spec.rb b/spec/services/clusters/applications/patch_service_spec.rb index d4ee3243b84..10b1379a127 100644 --- a/spec/services/clusters/applications/patch_service_spec.rb +++ b/spec/services/clusters/applications/patch_service_spec.rb @@ -41,47 +41,30 @@ describe Clusters::Applications::PatchService do expect(helm_client).to receive(:update).with(update_command).and_raise(error) end + include_examples 'logs kubernetes errors' do + let(:error_name) { 'Kubeclient::HttpError' } + let(:error_message) { 'system failure' } + let(:error_code) { 500 } + end + it 'make the application errored' do service.execute expect(application).to be_update_errored expect(application.status_reason).to match('Kubernetes error: 500') end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::PatchService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::PatchService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - service.execute - end end context 'a non kubernetes error happens' do let(:application) { create(:clusters_applications_knative, :scheduled) } let(:error) { StandardError.new('something bad happened') } + include_examples 'logs kubernetes errors' do + let(:error_name) { 'StandardError' } + let(:error_message) { 'something bad happened' } + let(:error_code) { nil } + end + before do expect(application).to receive(:make_updating!).once.and_raise(error) end @@ -94,35 +77,6 @@ describe Clusters::Applications::PatchService do expect(application).to be_update_errored expect(application.status_reason).to eq("Can't start update process.") end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::PatchService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::PatchService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - service.execute - end end end end diff --git a/spec/services/clusters/applications/upgrade_service_spec.rb b/spec/services/clusters/applications/upgrade_service_spec.rb index 1822fc38dbd..dd2e6e94e4f 100644 --- a/spec/services/clusters/applications/upgrade_service_spec.rb +++ b/spec/services/clusters/applications/upgrade_service_spec.rb @@ -41,41 +41,18 @@ describe Clusters::Applications::UpgradeService do expect(helm_client).to receive(:update).with(install_command).and_raise(error) end + include_examples 'logs kubernetes errors' do + let(:error_name) { 'Kubeclient::HttpError' } + let(:error_message) { 'system failure' } + let(:error_code) { 500 } + end + it 'make the application errored' do service.execute expect(application).to be_update_errored expect(application.status_reason).to match('Kubernetes error: 500') end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::UpgradeService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'Kubeclient::HttpError', - message: 'system failure', - service: 'Clusters::Applications::UpgradeService', - app_id: application.id, - project_ids: application.cluster.project_ids, - group_ids: [], - error_code: 500 - } - ) - - service.execute - end end context 'a non kubernetes error happens' do @@ -86,6 +63,12 @@ describe Clusters::Applications::UpgradeService do expect(application).to receive(:make_updating!).once.and_raise(error) end + include_examples 'logs kubernetes errors' do + let(:error_name) { 'StandardError' } + let(:error_message) { 'something bad happened' } + let(:error_code) { nil } + end + it 'make the application errored' do expect(helm_client).not_to receive(:update) @@ -94,35 +77,6 @@ describe Clusters::Applications::UpgradeService do expect(application).to be_update_errored expect(application.status_reason).to eq("Can't start upgrade process.") end - - it 'logs errors' do - expect(service.send(:logger)).to receive(:error).with( - { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::UpgradeService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( - error, - extra: { - exception: 'StandardError', - error_code: nil, - message: 'something bad happened', - service: 'Clusters::Applications::UpgradeService', - app_id: application.id, - project_ids: application.cluster.projects.pluck(:id), - group_ids: [] - } - ) - - service.execute - end end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 55e7b46248b..dc5d1cf2f04 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -263,19 +263,6 @@ describe MergeRequests::CreateService do expect(merge_request.actual_head_pipeline).to be_merge_request_event end end - - context "when the 'ci_merge_request_pipeline' feature flag is disabled" do - before do - stub_feature_flags(ci_merge_request_pipeline: false) - end - - it 'does not create a detached merge request pipeline' do - expect(merge_request).to be_persisted - - merge_request.reload - expect(merge_request.merge_request_pipelines.count).to eq(0) - end - end end context "when .gitlab-ci.yml does not have merge_requests keywords" do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 25cbac6d7ee..5cf3577f01f 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -230,17 +230,6 @@ describe MergeRequests::RefreshService do end.not_to change { @merge_request.merge_request_pipelines.count } end end - - context "when the 'ci_merge_request_pipeline' feature flag is disabled" do - before do - stub_feature_flags(ci_merge_request_pipeline: false) - end - - it 'does not create a detached merge request pipeline' do - expect { subject } - .not_to change { @merge_request.merge_request_pipelines.count } - end - end end context "when .gitlab-ci.yml does not have merge_requests keywords" do diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb index 85515d548a7..a1d31464e07 100644 --- a/spec/services/preview_markdown_service_spec.rb +++ b/spec/services/preview_markdown_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe PreviewMarkdownService do let(:user) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } before do project.add_developer(user) @@ -20,23 +20,72 @@ describe PreviewMarkdownService do end describe 'suggestions' do - let(:params) { { text: "```suggestion\nfoo\n```", preview_suggestions: preview_suggestions } } + let(:merge_request) do + create(:merge_request, target_project: project, source_project: project) + end + let(:text) { "```suggestion\nfoo\n```" } + let(:params) do + suggestion_params.merge(text: text, + target_type: 'MergeRequest', + target_id: merge_request.iid) + end let(:service) { described_class.new(project, user, params) } context 'when preview markdown param is present' do - let(:preview_suggestions) { true } + let(:path) { "files/ruby/popen.rb" } + let(:line) { 10 } + let(:diff_refs) { merge_request.diff_refs } + + let(:suggestion_params) do + { + preview_suggestions: true, + file_path: path, + line: line, + base_sha: diff_refs.base_sha, + start_sha: diff_refs.start_sha, + head_sha: diff_refs.head_sha + } + end + + it 'returns suggestions referenced in text' do + position = Gitlab::Diff::Position.new(new_path: path, + new_line: line, + diff_refs: diff_refs) + + expect(Gitlab::Diff::SuggestionsParser) + .to receive(:parse) + .with(text, position: position, project: merge_request.project) + .and_call_original - it 'returns users referenced in text' do result = service.execute - expect(result[:suggestions]).to eq(['foo']) + expect(result[:suggestions]).to all(be_a(Gitlab::Diff::Suggestion)) + end + + context 'when user is not authorized' do + let(:another_user) { create(:user) } + let(:service) { described_class.new(project, another_user, params) } + + before do + project.add_guest(another_user) + end + + it 'returns no suggestions' do + result = service.execute + + expect(result[:suggestions]).to be_empty + end end end context 'when preview markdown param is not present' do - let(:preview_suggestions) { false } + let(:suggestion_params) do + { + preview_suggestions: false + } + end - it 'returns users referenced in text' do + it 'returns suggestions referenced in text' do result = service.execute expect(result[:suggestions]).to eq([]) @@ -49,8 +98,8 @@ describe PreviewMarkdownService do let(:params) do { text: "Please do it\n/assign #{user.to_reference}", - quick_actions_target_type: 'Issue', - quick_actions_target_id: issue.id + target_type: 'Issue', + target_id: issue.id } end let(:service) { described_class.new(project, user, params) } @@ -72,7 +121,7 @@ describe PreviewMarkdownService do let(:params) do { text: "My work\n/estimate 2y", - quick_actions_target_type: 'MergeRequest' + target_type: 'MergeRequest' } end let(:service) { described_class.new(project, user, params) } @@ -96,8 +145,8 @@ describe PreviewMarkdownService do let(:params) do { text: "My work\n/tag v1.2.3 Stable release", - quick_actions_target_type: 'Commit', - quick_actions_target_id: commit.id + target_type: 'Commit', + target_id: commit.id } end let(:service) { described_class.new(project, user, params) } diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index 80b5dcac6c7..7732767137c 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -51,6 +51,10 @@ describe Suggestions::ApplyService do diff_refs: merge_request.diff_refs) end + let(:diff_note) do + create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project) + end + let(:suggestion) do create(:suggestion, :content_from_repo, note: diff_note, to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n") @@ -108,12 +112,6 @@ describe Suggestions::ApplyService do target_project: project) end - let!(:diff_note) do - create(:diff_note_on_merge_request, noteable: merge_request, - position: position, - project: project) - end - before do project.add_maintainer(user) end @@ -192,11 +190,6 @@ describe Suggestions::ApplyService do CONTENT end - let(:merge_request) do - create(:merge_request, source_project: project, - target_project: project) - end - def create_suggestion(diff, old_line: nil, new_line: nil, from_content:, to_content:, path:) position = Gitlab::Diff::Position.new(old_path: path, new_path: path, @@ -291,6 +284,55 @@ describe Suggestions::ApplyService do expect(suggestion_2_diff.strip).to eq(expected_suggestion_2_diff.strip) end end + + context 'multi-line suggestion' do + let(:expected_content) do + <<~CONTENT + require 'fileutils' + require 'open3' + + module Popen + extend self + + # multi + # line + + vars = { + "PWD" => path + } + + options = { + chdir: path + } + + unless File.directory?(path) + FileUtils.mkdir_p(path) + end + + @cmd_output = "" + @cmd_status = 0 + + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| + @cmd_output << stdout.read + @cmd_output << stderr.read + @cmd_status = wait_thr.value.exitstatus + end + + return @cmd_output, @cmd_status + end + end + CONTENT + end + + let(:suggestion) do + create(:suggestion, :content_from_repo, note: diff_note, + lines_above: 2, + lines_below: 3, + to_content: "# multi\n# line\n") + end + + it_behaves_like 'successfully creates commit and updates suggestion' + end end context 'fork-project' do diff --git a/spec/support/shared_examples/quick_actions/commit/tag_quick_action_shared_examples.rb b/spec/support/shared_examples/quick_actions/commit/tag_quick_action_shared_examples.rb index 4604d867507..b337a1c18d8 100644 --- a/spec/support/shared_examples/quick_actions/commit/tag_quick_action_shared_examples.rb +++ b/spec/support/shared_examples/quick_actions/commit/tag_quick_action_shared_examples.rb @@ -1,4 +1,28 @@ # frozen_string_literal: true shared_examples 'tag quick action' do + context "post note to existing commit" do + it 'tags this commit' do + add_note("/tag #{tag_name} #{tag_message}") + + expect(page).to have_content 'Commands applied' + expect(page).to have_content "tagged commit #{truncated_commit_sha}" + expect(page).to have_content tag_name + + visit project_tag_path(project, tag_name) + expect(page).to have_content tag_name + expect(page).to have_content tag_message + expect(page).to have_content truncated_commit_sha + end + end + + context 'preview', :js do + it 'removes quick action from note and explains it' do + preview_note("/tag #{tag_name} #{tag_message}") + + expect(page).not_to have_content '/tag' + expect(page).to have_content %{Tags this commit to #{tag_name} with "#{tag_message}"} + expect(page).to have_content tag_name + end + end end diff --git a/spec/support/shared_examples/quick_actions/merge_request/merge_quick_action_shared_examples.rb b/spec/support/shared_examples/quick_actions/merge_request/merge_quick_action_shared_examples.rb index 31d88183f0d..c454ddc4bba 100644 --- a/spec/support/shared_examples/quick_actions/merge_request/merge_quick_action_shared_examples.rb +++ b/spec/support/shared_examples/quick_actions/merge_request/merge_quick_action_shared_examples.rb @@ -1,4 +1,51 @@ # frozen_string_literal: true shared_examples 'merge quick action' do + context 'when the current user can merge the MR' do + before do + sign_in(user) + visit project_merge_request_path(project, merge_request) + end + + it 'merges the MR' do + add_note("/merge") + + expect(page).to have_content 'Commands applied' + + expect(merge_request.reload).to be_merged + end + end + + context 'when the head diff changes in the meanwhile' do + before do + merge_request.source_branch = 'another_branch' + merge_request.save + sign_in(user) + visit project_merge_request_path(project, merge_request) + end + + it 'does not merge the MR' do + add_note("/merge") + + expect(page).not_to have_content 'Your commands have been executed!' + + expect(merge_request.reload).not_to be_merged + end + end + + context 'when the current user cannot merge the MR' do + before do + project.add_guest(guest) + sign_in(guest) + visit project_merge_request_path(project, merge_request) + end + + it 'does not merge the MR' do + add_note("/merge") + + expect(page).not_to have_content 'Your commands have been executed!' + + expect(merge_request.reload).not_to be_merged + end + end end diff --git a/spec/support/shared_examples/services/base_helm_service_shared_examples.rb b/spec/support/shared_examples/services/base_helm_service_shared_examples.rb new file mode 100644 index 00000000000..78a8e49fd76 --- /dev/null +++ b/spec/support/shared_examples/services/base_helm_service_shared_examples.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +shared_examples 'logs kubernetes errors' do + let(:error_hash) do + { + service: service.class.name, + app_id: application.id, + project_ids: application.cluster.project_ids, + group_ids: [], + error_code: error_code + } + end + + let(:logger_hash) do + error_hash.merge( + exception: error_name, + message: error_message, + backtrace: instance_of(Array) + ) + end + + it 'logs into kubernetes.log and Sentry' do + expect(service.send(:logger)).to receive(:error).with(logger_hash) + + expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( + error, + extra: hash_including(error_hash) + ) + + service.execute + end +end diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index 3592a11360d..ab98976ec27 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -94,4 +94,13 @@ describe RecordsUploads do expect { uploader.remove! }.to change { Upload.count }.from(1).to(0) end end + + describe '#filename' do + it 'gets the filename from the path recorded in the database, not CarrierWave' do + uploader.store!(upload_fixture('rails_sample.jpg')) + expect_any_instance_of(GitlabUploader).not_to receive(:filename) + + expect(uploader.filename).to eq('rails_sample.jpg') + end + end end diff --git a/yarn.lock b/yarn.lock index 15fa876300b..4de579fb290 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1164,15 +1164,7 @@ apollo-link-http-common@^0.2.8: ts-invariant "^0.3.2" tslib "^1.9.3" -apollo-link@^1.0.0, apollo-link@^1.2.3: - version "1.2.3" - resolved "https://registry.yarnpkg.com/apollo-link/-/apollo-link-1.2.3.tgz#9bd8d5fe1d88d31dc91dae9ecc22474d451fb70d" - integrity sha512-iL9yS2OfxYhigme5bpTbmRyC+Htt6tyo2fRMHT3K1XRL/C5IQDDz37OjpPy4ndx7WInSvfSZaaOTKFja9VWqSw== - dependencies: - apollo-utilities "^1.0.0" - zen-observable-ts "^0.8.10" - -apollo-link@^1.2.11, apollo-link@^1.2.6: +apollo-link@^1.0.0, apollo-link@^1.2.11, apollo-link@^1.2.3, apollo-link@^1.2.6: version "1.2.11" resolved "https://registry.yarnpkg.com/apollo-link/-/apollo-link-1.2.11.tgz#493293b747ad3237114ccd22e9f559e5e24a194d" integrity sha512-PQvRCg13VduLy3X/0L79M6uOpTh5iHdxnxYuo8yL7sJlWybKRJwsv4IcRBJpMFbChOOaHY7Og9wgPo6DLKDKDA== @@ -1191,7 +1183,7 @@ apollo-upload-client@^10.0.0: apollo-link-http-common "^0.2.8" extract-files "^5.0.0" -apollo-utilities@1.2.1, apollo-utilities@^1.0.0, apollo-utilities@^1.2.1: +apollo-utilities@1.2.1, apollo-utilities@^1.2.1: version "1.2.1" resolved "https://registry.yarnpkg.com/apollo-utilities/-/apollo-utilities-1.2.1.tgz#1c3a1ebf5607d7c8efe7636daaf58e7463b41b3c" integrity sha512-Zv8Udp9XTSFiN8oyXOjf6PMHepD4yxxReLsl6dPUy5Ths7jti3nmlBzZUOxuTWRwZn0MoclqL7RQ5UEJN8MAxg== @@ -11536,13 +11528,6 @@ yeast@0.1.2: resolved "https://registry.yarnpkg.com/yeast/-/yeast-0.1.2.tgz#008e06d8094320c372dbc2f8ed76a0ca6c8ac419" integrity sha1-AI4G2AlDIMNy28L47XagymyKxBk= -zen-observable-ts@^0.8.10: - version "0.8.10" - resolved "https://registry.yarnpkg.com/zen-observable-ts/-/zen-observable-ts-0.8.10.tgz#18e2ce1c89fe026e9621fd83cc05168228fce829" - integrity sha512-5vqMtRggU/2GhePC9OU4sYEWOdvmayp2k3gjPf4F0mXwB3CSbbNznfDUvDJx9O2ZTa1EIXdJhPchQveFKwNXPQ== - dependencies: - zen-observable "^0.8.0" - zen-observable-ts@^0.8.18: version "0.8.18" resolved "https://registry.yarnpkg.com/zen-observable-ts/-/zen-observable-ts-0.8.18.tgz#ade44b1060cc4a800627856ec10b9c67f5f639c8" |
