diff options
67 files changed, 1270 insertions, 441 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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/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/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 1d139200535..7ffa365c651 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1361,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/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/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/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 |
