Skip to content

G
gitlab-ce
Commit e94e2691 authored by GitLab Bot's avatar GitLab Bot
Browse files
Options

Automatic merge of gitlab-org/gitlab master

parents da7dc02b 8cbc43ab
Hide whitespace changes
Inline Side-by-side
Showing with 315 additions and 144 deletions
Gemfile
View file @ e94e2691
......@@ -93,7 +93,7 @@ gem 'graphql', '~> 1.10.5'
# TODO: remove app/views/graphiql/rails/editors/show.html.erb when https://github.com/rmosolgo/graphiql-rails/pull/71 is released:
# https://gitlab.com/gitlab-org/gitlab/issues/31747
gem 'graphiql-rails', '~> 1.4.10'
gem 'apollo_upload_server', '~> 2.0.0.beta3'
gem 'apollo_upload_server', '~> 2.0.2'
gem 'graphql-docs', '~> 1.6.0', group: [:development, :test]
# Disable strong_params so that Mash does not respond to :permitted?
......
Gemfile.lock
View file @ e94e2691
......@@ -73,7 +73,7 @@ GEM
public_suffix (>= 2.0.2, < 5.0)
aes_key_wrap (1.0.1)
akismet (3.0.0)
apollo_upload_server (2.0.0.beta.3)
apollo_upload_server (2.0.2)
graphql (>= 1.8)
rails (>= 4.2)
asana (0.10.0)
......@@ -1220,7 +1220,7 @@ DEPENDENCIES
acts-as-taggable-on (~> 6.0)
addressable (~> 2.7)
akismet (~> 3.0)
apollo_upload_server (~> 2.0.0.beta3)
apollo_upload_server (~> 2.0.2)
asana (= 0.10.0)
asciidoctor (~> 2.0.10)
asciidoctor-include-ext (~> 0.3.1)
......
app/assets/javascripts/diffs/components/settings_dropdown.vue
View file @ e94e2691
<script>
import { mapActions, mapGetters, mapState } from 'vuex';
import { GlDeprecatedButton, GlIcon } from '@gitlab/ui';
import { GlButtonGroup, GlButton, GlDropdown } from '@gitlab/ui';
import { __ } from '~/locale';
export default {
components: {
GlDeprecatedButton,
GlIcon,
GlButtonGroup,
GlButton,
GlDropdown,
},
computed: {
...mapGetters('diffs', ['isInlineView', 'isParallelView']),
...mapState('diffs', ['renderTreeList', 'showWhitespace']),
},
mounted() {
this.patchAriaLabel();
},
updated() {
this.patchAriaLabel();
},
methods: {
...mapActions('diffs', [
'setInlineDiffViewType',
......@@ -18,74 +26,69 @@ export default {
'setRenderTreeList',
'setShowWhitespace',
]),
patchAriaLabel() {
this.$el
.querySelector('.js-show-diff-settings')
.setAttribute('aria-label', __('Diff view settings'));
},
},
};
</script>
<template>
<div class="dropdown">
<button
type="button"
class="btn btn-default js-show-diff-settings"
data-toggle="dropdown"
data-display="static"
>
<gl-icon name="settings" /> <gl-icon name="chevron-down" />
</button>
<div class="dropdown-menu dropdown-menu-right p-2 pt-3 pb-3">
<div>
<span class="bold d-block mb-1">{{ __('File browser') }}</span>
<div class="btn-group d-flex">
<gl-deprecated-button
:class="{ active: !renderTreeList }"
class="w-100 js-list-view"
@click="setRenderTreeList(false)"
>
{{ __('List view') }}
</gl-deprecated-button>
<gl-deprecated-button
:class="{ active: renderTreeList }"
class="w-100 js-tree-view"
@click="setRenderTreeList(true)"
>
{{ __('Tree view') }}
</gl-deprecated-button>
</div>
</div>
<div class="mt-2">
<span class="bold d-block mb-1">{{ __('Compare changes') }}</span>
<div class="btn-group d-flex js-diff-view-buttons">
<gl-deprecated-button
id="inline-diff-btn"
:class="{ active: isInlineView }"
class="w-100 js-inline-diff-button"
data-view-type="inline"
@click="setInlineDiffViewType"
>
{{ __('Inline') }}
</gl-deprecated-button>
<gl-deprecated-button
id="parallel-diff-btn"
:class="{ active: isParallelView }"
class="w-100 js-parallel-diff-button"
data-view-type="parallel"
@click="setParallelDiffViewType"
>
{{ __('Side-by-side') }}
</gl-deprecated-button>
</div>
</div>
<div class="mt-2">
<label class="mb-0">
<input
id="show-whitespace"
type="checkbox"
:checked="showWhitespace"
@change="setShowWhitespace({ showWhitespace: $event.target.checked, pushState: true })"
/>
{{ __('Show whitespace changes') }}
</label>
</div>
<gl-dropdown icon="settings" toggle-class="js-show-diff-settings" right>
<div class="gl-px-3">
<span class="gl-font-weight-bold gl-display-block gl-mb-2">{{ __('File browser') }}</span>
<gl-button-group class="gl-display-flex">
<gl-button
:class="{ selected: !renderTreeList }"
class="gl-w-half js-list-view"
@click="setRenderTreeList(false)"
>
{{ __('List view') }}
</gl-button>
<gl-button
:class="{ selected: renderTreeList }"
class="gl-w-half js-tree-view"
@click="setRenderTreeList(true)"
>
{{ __('Tree view') }}
</gl-button>
</gl-button-group>
</div>
<div class="gl-mt-3 gl-px-3">
<span class="gl-font-weight-bold gl-display-block gl-mb-2">{{ __('Compare changes') }}</span>
<gl-button-group class="gl-display-flex js-diff-view-buttons">
<gl-button
id="inline-diff-btn"
:class="{ selected: isInlineView }"
class="gl-w-half js-inline-diff-button"
data-view-type="inline"
@click="setInlineDiffViewType"
>
{{ __('Inline') }}
</gl-button>
<gl-button
id="parallel-diff-btn"
:class="{ selected: isParallelView }"
class="gl-w-half js-parallel-diff-button"
data-view-type="parallel"
@click="setParallelDiffViewType"
>
{{ __('Side-by-side') }}
</gl-button>
</gl-button-group>
</div>
<div class="gl-mt-3 gl-px-3">
<label class="gl-mb-0">
<input
id="show-whitespace"
type="checkbox"
:checked="showWhitespace"
@change="setShowWhitespace({ showWhitespace: $event.target.checked, pushState: true })"
/>
{{ __('Show whitespace changes') }}
</label>
</div>
</div>
</gl-dropdown>
</template>
app/models/snippet.rb
View file @ e94e2691
......@@ -345,6 +345,10 @@ class Snippet < ApplicationRecord
repository.ls_files(ref)
end
def multiple_files?
list_files(repository.root_ref).size > 1
end
class << self
# Searches for snippets with a matching title, description or file name.
#
......
changelogs/unreleased/228657-version-bump-apollo_upload_server-gem.yml 0 → 100644
View file @ e94e2691
---
title: Bug fix GraphQL file uploads accepting non-file input
merge_request: 39763
author:
type: fixed
changelogs/unreleased/229320-migrate-bootstrap-button-to-gitlab-ui-glbutton-in-app-assets-javas.yml 0 → 100644
View file @ e94e2691
---
title: Migrating buttons and classes to match GitLab UI
merge_request: 40409
author:
type: other
lib/api/helpers.rb
View file @ e94e2691
......@@ -537,6 +537,10 @@ module API
)
end
def with_api_params(&block)
yield({ api: true, request: request })
end
protected
def project_finder_params_visibility_ce
......
lib/api/helpers/snippets_helpers.rb
View file @ e94e2691
......@@ -27,6 +27,20 @@ module API
exactly_one_of :files, :content
end
params :update_file_params do |options|
optional :files, type: Array, desc: 'An array of files to update' do
requires :action, type: String,
values: SnippetInputAction::ACTIONS.map(&:to_s),
desc: "The type of action to perform on the file, must be one of: #{SnippetInputAction::ACTIONS.join(", ")}"
optional :content, type: String, desc: 'The content of a snippet'
optional :file_path, file_path: true, type: String, desc: 'The file path of a snippet file'
optional :previous_path, file_path: true, type: String, desc: 'The previous path of a snippet file'
end
mutually_exclusive :files, :content
mutually_exclusive :files, :file_name
end
def content_for(snippet)
if snippet.empty_repo?
env['api.format'] = :txt
......@@ -53,10 +67,30 @@ module API
end
end
def process_file_args(args)
args[:snippet_actions] = args.delete(:files)&.map do |file|
file[:action] = :create
file.symbolize_keys
def process_create_params(args)
with_api_params do |api_params|
args[:snippet_actions] = args.delete(:files)&.map do |file|
file[:action] = :create
file.symbolize_keys
end
args.merge(api_params)
end
end
def process_update_params(args)
with_api_params do |api_params|
args[:snippet_actions] = args.delete(:files)&.map(&:symbolize_keys)
args.merge(api_params)
end
end
def validate_params_for_multiple_files(snippet)
return unless params[:content] || params[:file_name]
if Feature.enabled?(:snippet_multiple_files, current_user) && snippet.multiple_files?
render_api_error!({ error: _('To update Snippets with multiple files, you must use the `files` parameter') }, 400)
end
end
end
......
lib/api/project_snippets.rb
View file @ e94e2691
......@@ -64,12 +64,8 @@ module API
end
post ":id/snippets" do
authorize! :create_snippet, user_project
snippet_params = declared_params(include_missing: false).tap do |create_args|
create_args[:request] = request
create_args[:api] = true
process_file_args(create_args)
end
snippet_params = process_create_params(declared_params(include_missing: false))
service_response = ::Snippets::CreateService.new(user_project, current_user, snippet_params).execute
snippet = service_response.payload[:snippet]
......
lib/api/snippets.rb
View file @ e94e2691
......@@ -76,12 +76,7 @@ module API
post do
authorize! :create_snippet
attrs = declared_params(include_missing: false).tap do |create_args|
create_args[:request] = request
create_args[:api] = true
process_file_args(create_args)
end
attrs = process_create_params(declared_params(include_missing: false))
service_response = ::Snippets::CreateService.new(nil, current_user, attrs).execute
snippet = service_response.payload[:snippet]
......@@ -99,16 +94,20 @@ module API
detail 'This feature was introduced in GitLab 8.15.'
success Entities::PersonalSnippet
end
params do
requires :id, type: Integer, desc: 'The ID of a snippet'
optional :title, type: String, allow_blank: false, desc: 'The title of a snippet'
optional :file_name, type: String, desc: 'The name of a snippet file'
optional :content, type: String, allow_blank: false, desc: 'The content of a snippet'
optional :description, type: String, desc: 'The description of a snippet'
optional :file_name, type: String, desc: 'The name of a snippet file'
optional :title, type: String, allow_blank: false, desc: 'The title of a snippet'
optional :visibility, type: String,
values: Gitlab::VisibilityLevel.string_values,
desc: 'The visibility of the snippet'
at_least_one_of :title, :file_name, :content, :visibility
use :update_file_params
at_least_one_of :title, :file_name, :content, :files, :visibility
end
put ':id' do
snippet = snippets_for_current_user.find_by_id(params.delete(:id))
......@@ -116,8 +115,12 @@ module API
authorize! :update_snippet, snippet
attrs = declared_params(include_missing: false).merge(request: request, api: true)
validate_params_for_multiple_files(snippet)
attrs = process_update_params(declared_params(include_missing: false))
service_response = ::Snippets::UpdateService.new(nil, current_user, attrs).execute(snippet)
snippet = service_response.payload[:snippet]
if service_response.success?
......
locale/gitlab.pot
View file @ e94e2691
......@@ -8613,6 +8613,9 @@ msgstr ""
msgid "Diff limits"
msgstr ""
msgid "Diff view settings"
msgstr ""
msgid "Difference between start date and now"
msgstr ""
......@@ -25971,6 +25974,9 @@ msgstr ""
msgid "To unsubscribe from this issue, please paste the following link into your browser:"
msgstr ""
msgid "To update Snippets with multiple files, you must use the `files` parameter"
msgstr ""
msgid "To view all %{scannedResourcesCount} scanned URLs, please download the CSV file"
msgstr ""
......
spec/frontend/diffs/components/settings_dropdown_spec.js
View file @ e94e2691
......@@ -7,7 +7,7 @@ import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constant
const localVue = createLocalVue();
localVue.use(Vuex);
describe('Diff settiings dropdown component', () => {
describe('Diff settings dropdown component', () => {
let vm;
let actions;
......@@ -61,50 +61,50 @@ describe('Diff settiings dropdown component', () => {
expect(actions.setRenderTreeList).toHaveBeenCalledWith(expect.anything(), true, undefined);
});
it('sets list button as active when renderTreeList is false', () => {
it('sets list button as selected when renderTreeList is false', () => {
createComponent(store => {
Object.assign(store.state.diffs, {
renderTreeList: false,
});
});
expect(vm.find('.js-list-view').classes('active')).toBe(true);
expect(vm.find('.js-tree-view').classes('active')).toBe(false);
expect(vm.find('.js-list-view').classes('selected')).toBe(true);
expect(vm.find('.js-tree-view').classes('selected')).toBe(false);
});
it('sets tree button as active when renderTreeList is true', () => {
it('sets tree button as selected when renderTreeList is true', () => {
createComponent(store => {
Object.assign(store.state.diffs, {
renderTreeList: true,
});
});
expect(vm.find('.js-list-view').classes('active')).toBe(false);
expect(vm.find('.js-tree-view').classes('active')).toBe(true);
expect(vm.find('.js-list-view').classes('selected')).toBe(false);
expect(vm.find('.js-tree-view').classes('selected')).toBe(true);
});
});
describe('compare changes', () => {
it('sets inline button as active', () => {
it('sets inline button as selected', () => {
createComponent(store => {
Object.assign(store.state.diffs, {
diffViewType: INLINE_DIFF_VIEW_TYPE,
});
});
expect(vm.find('.js-inline-diff-button').classes('active')).toBe(true);
expect(vm.find('.js-parallel-diff-button').classes('active')).toBe(false);
expect(vm.find('.js-inline-diff-button').classes('selected')).toBe(true);
expect(vm.find('.js-parallel-diff-button').classes('selected')).toBe(false);
});
it('sets parallel button as active', () => {
it('sets parallel button as selected', () => {
createComponent(store => {
Object.assign(store.state.diffs, {
diffViewType: PARALLEL_DIFF_VIEW_TYPE,
});
});
expect(vm.find('.js-inline-diff-button').classes('active')).toBe(false);
expect(vm.find('.js-parallel-diff-button').classes('active')).toBe(true);
expect(vm.find('.js-inline-diff-button').classes('selected')).toBe(false);
expect(vm.find('.js-parallel-diff-button').classes('selected')).toBe(true);
});
it('calls setInlineDiffViewType when clicking inline button', () => {
......
spec/models/snippet_spec.rb
View file @ e94e2691
......@@ -787,4 +787,26 @@ RSpec.describe Snippet do
end
end
end
describe '#multiple_files?' do
subject { snippet.multiple_files? }
context 'when snippet has multiple files' do
let(:snippet) { create(:snippet, :repository) }
it { is_expected.to be_truthy }
end
context 'when snippet does not have multiple files' do
let(:snippet) { create(:snippet, :empty_repo) }
it { is_expected.to be_falsey }
end
context 'when the snippet does not have a repository' do
let(:snippet) { build(:snippet) }
it { is_expected.to be_falsey }
end
end
end
spec/requests/api/graphql/mutations/design_management/upload_spec.rb
View file @ e94e2691
......@@ -12,11 +12,11 @@ RSpec.describe "uploading designs" do
let(:files) { [fixture_file_upload("spec/fixtures/dk.png")] }
let(:variables) { {} }
let(:mutation) do
def mutation
input = {
project_path: project.full_path,
iid: issue.iid,
files: files
files: files.dup
}.merge(variables)
graphql_mutation(:design_management_upload, input)
end
......@@ -30,31 +30,15 @@ RSpec.describe "uploading designs" do
end
it "returns an error if the user is not allowed to upload designs" do
post_graphql_mutation(mutation, current_user: create(:user))
post_graphql_mutation_with_uploads(mutation, current_user: create(:user))
expect(graphql_errors).to be_present
end
it "succeeds (backward compatibility)" do
post_graphql_mutation(mutation, current_user: current_user)
it "succeeds, and responds with the created designs" do
post_graphql_mutation_with_uploads(mutation, current_user: current_user)
expect(graphql_errors).not_to be_present
end
it 'succeeds' do
file_path_in_params = ['designManagementUploadInput', 'files', 0]
params = mutation_to_apollo_uploads_param(mutation, files: [file_path_in_params])
workhorse_post_with_file(api('/', current_user, version: 'graphql'),
params: params,
file_key: '1'
)
expect(graphql_errors).not_to be_present
end
it "responds with the created designs" do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response).to include(
"designs" => a_collection_containing_exactly(
......@@ -65,7 +49,7 @@ RSpec.describe "uploading designs" do
it "can respond with skipped designs" do
2.times do
post_graphql_mutation(mutation, current_user: current_user)
post_graphql_mutation_with_uploads(mutation, current_user: current_user)
files.each(&:rewind)
end
......@@ -80,7 +64,7 @@ RSpec.describe "uploading designs" do
let(:variables) { { iid: "123" } }
it "returns an error" do
post_graphql_mutation(mutation, current_user: create(:user))
post_graphql_mutation_with_uploads(mutation, current_user: create(:user))
expect(graphql_errors).not_to be_empty
end
......@@ -92,7 +76,7 @@ RSpec.describe "uploading designs" do
expect(service).to receive(:execute).and_return({ status: :error, message: "Something went wrong" })
end
post_graphql_mutation(mutation, current_user: current_user)
post_graphql_mutation_with_uploads(mutation, current_user: current_user)
expect(mutation_response["errors"].first).to eq("Something went wrong")
end
end
......
spec/requests/api/snippets_spec.rb
View file @ e94e2691
......@@ -391,21 +391,98 @@ RSpec.describe API::Snippets do
create(:personal_snippet, :repository, author: user, visibility_level: visibility_level)
end
shared_examples 'snippet updates' do
it 'updates a snippet' do
new_content = 'New content'
let(:create_action) { { action: 'create', file_path: 'foo.txt', content: 'bar' } }
let(:update_action) { { action: 'update', file_path: 'CHANGELOG', content: 'bar' } }
let(:move_action) { { action: 'move', file_path: '.old-gitattributes', previous_path: '.gitattributes' } }
let(:delete_action) { { action: 'delete', file_path: 'CONTRIBUTING.md' } }
let(:bad_file_path) { { action: 'create', file_path: '../../etc/passwd', content: 'bar' } }
let(:bad_previous_path) { { action: 'create', previous_path: '../../etc/passwd', file_path: 'CHANGELOG', content: 'bar' } }
let(:invalid_move) { { action: 'move', file_path: 'missing_previous_path.txt' } }
context 'with snippet file changes' do
using RSpec::Parameterized::TableSyntax
where(:is_multi_file, :file_name, :content, :files, :status) do
true | nil | nil | [create_action] | :success
true | nil | nil | [update_action] | :success
true | nil | nil | [move_action] | :success
true | nil | nil | [delete_action] | :success
true | nil | nil | [create_action, update_action] | :success
true | 'foo.txt' | 'bar' | [create_action] | :bad_request
true | 'foo.txt' | 'bar' | nil | :bad_request
true | nil | nil | nil | :bad_request
true | 'foo.txt' | nil | [create_action] | :bad_request
true | nil | 'bar' | [create_action] | :bad_request
true | '' | nil | [create_action] | :bad_request
true | nil | '' | [create_action] | :bad_request
true | nil | nil | [bad_file_path] | :bad_request
true | nil | nil | [bad_previous_path] | :bad_request
true | nil | nil | [invalid_move] | :forbidden
false | 'foo.txt' | 'bar' | nil | :success
false | 'foo.txt' | nil | nil | :success
false | nil | 'bar' | nil | :success
false | 'foo.txt' | 'bar' | [create_action] | :bad_request
false | nil | nil | nil | :bad_request
false | nil | '' | nil | :bad_request
false | nil | nil | [bad_file_path] | :bad_request
false | nil | nil | [bad_previous_path] | :bad_request
end
with_them do
before do
allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(is_multi_file)
end
it 'has the correct response' do
update_params = {}.tap do |params|
params[:files] = files if files
params[:file_name] = file_name if file_name
params[:content] = content if content
end
update_snippet(params: update_params)
expect(response).to have_gitlab_http_status(status)
end
end
context 'when save fails due to a repository commit error' do
before do
allow_next_instance_of(Repository) do |instance|
allow(instance).to receive(:multi_action).and_raise(Gitlab::Git::CommitError)
end
update_snippet(params: { files: [create_action] })
end
it 'returns a bad request response' do
expect(response).to have_gitlab_http_status(:bad_request)
end
end
end
shared_examples 'snippet non-file updates' do
it 'updates a snippet non-file attributes' do
new_description = 'New description'
new_title = 'New title'
new_visibility = 'internal'
update_snippet(params: { content: new_content, description: new_description, visibility: 'internal' })
update_snippet(params: { title: new_title, description: new_description, visibility: new_visibility })
expect(response).to have_gitlab_http_status(:ok)
snippet.reload
expect(snippet.content).to eq(new_content)
expect(snippet.description).to eq(new_description)
expect(snippet.visibility).to eq('internal')
aggregate_failures do
expect(response).to have_gitlab_http_status(:ok)
expect(snippet.description).to eq(new_description)
expect(snippet.visibility).to eq(new_visibility)
expect(snippet.title).to eq(new_title)
end
end
end
it_behaves_like 'snippet non-file updates'
context 'with restricted visibility settings' do
before do
stub_application_setting(restricted_visibility_levels:
......@@ -413,11 +490,9 @@ RSpec.describe API::Snippets do
Gitlab::VisibilityLevel::PRIVATE])
end
it_behaves_like 'snippet updates'
it_behaves_like 'snippet non-file updates'
end
it_behaves_like 'snippet updates'
it 'returns 404 for invalid snippet id' do
update_snippet(snippet_id: non_existing_record_id, params: { title: 'Foo' })
......@@ -438,13 +513,6 @@ RSpec.describe API::Snippets do
expect(response).to have_gitlab_http_status(:bad_request)
end
it 'returns 400 if content is blank' do
update_snippet(params: { content: '' })
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq 'content is empty'
end
it 'returns 400 if title is blank' do
update_snippet(params: { title: '' })
......
spec/support/helpers/graphql_helpers.rb
View file @ e94e2691
......@@ -241,6 +241,39 @@ module GraphqlHelpers
post_graphql(mutation.query, current_user: current_user, variables: mutation.variables)
end
def post_graphql_mutation_with_uploads(mutation, current_user: nil)
file_paths = file_paths_in_mutation(mutation)
params = mutation_to_apollo_uploads_param(mutation, files: file_paths)
workhorse_post_with_file(api('/', current_user, version: 'graphql'),
params: params,
file_key: '1'
)
end
def file_paths_in_mutation(mutation)
paths = []
find_uploads(paths, [], mutation.variables)
paths
end
# Depth first search for UploadedFile values
def find_uploads(paths, path, value)
case value
when Rack::Test::UploadedFile
paths << path
when Hash
value.each do |k, v|
find_uploads(paths, path + [k], v)
end
when Array
value.each_with_index do |v, i|
find_uploads(paths, path + [i], v)
end
end
end
# this implements GraphQL multipart request v2
# https://github.com/jaydenseric/graphql-multipart-request-spec/tree/v2.0.0-alpha.2
# this is simplified and do not support file deduplication
......
spec/support/shared_examples/requests/snippet_shared_examples.rb
View file @ e94e2691
......@@ -2,6 +2,10 @@
RSpec.shared_examples 'update with repository actions' do
context 'when the repository exists' do
before do
allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(false)
end
it 'commits the changes to the repository' do
existing_blob = snippet.blobs.first
new_file_name = existing_blob.path + '_new'
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7