Commit 3bcb04f1 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Add mutation toggling WIP state of merge requests

This is mainly the setup of mutations for GraphQL. Including
authorization and basic return type-structure.
parent 9fe58f5a
......@@ -7,5 +7,5 @@ class GitlabSchema < GraphQL::Schema
query(Types::QueryType)
default_max_page_size 100
# mutation(Types::MutationType)
mutation(Types::MutationType)
end
# frozen_string_literal: true
module Mutations
class BaseMutation < GraphQL::Schema::RelayClassicMutation
field :errors, [GraphQL::STRING_TYPE],
null: false,
description: "Reasons why the mutation failed."
def current_user
context[:current_user]
end
end
end
module Mutations
module ResolvesProject
extend ActiveSupport::Concern
def resolve_project(full_path:)
resolver.resolve(full_path: full_path)
end
def resolver
Resolvers::ProjectResolver.new(object: nil, context: context)
end
end
end
module Mutations
module MergeRequests
class Base < BaseMutation
include Gitlab::Graphql::Authorize::AuthorizeResource
include Mutations::ResolvesProject
argument :project_path, GraphQL::ID_TYPE,
required: true,
description: "The project the merge request to mutate is in"
argument :iid, GraphQL::ID_TYPE,
required: true,
description: "The iid of the merge request to mutate"
field :merge_request,
Types::MergeRequestType,
null: true,
description: "The merge request after mutation"
authorize :update_merge_request
private
def find_object(project_path:, iid:)
project = resolve_project(full_path: project_path)
resolver = Resolvers::MergeRequestResolver.new(object: project, context: context)
resolver.resolve(iid: iid)
end
end
end
end
# frozen_string_literal: true
module Mutations
module MergeRequests
class SetWip < Base
graphql_name 'MergeRequestSetWip'
argument :wip,
GraphQL::BOOLEAN_TYPE,
required: true,
description: <<~DESC
Whether or not to set the merge request as a WIP.
DESC
def resolve(project_path:, iid:, wip: nil)
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.project
::MergeRequests::UpdateService.new(project, current_user, wip_event: wip_event(merge_request, wip))
.execute(merge_request)
{
merge_request: merge_request,
errors: merge_request.errors.full_messages
}
end
private
def wip_event(merge_request, wip)
wip ? 'wip' : 'unwip'
end
end
end
end
# frozen_string_literal: true
module Types
class MutationType < BaseObject
include Gitlab::Graphql::MountMutation
graphql_name "Mutation"
# TODO: Add Mutations as fields
mount_mutation Mutations::MergeRequests::SetWip
end
end
---
title: Add the first mutations for merge requests to GraphQL
merge_request: 20443
author:
type: added
......@@ -46,7 +46,8 @@ module Gitlab
#{config.root}/app/services/concerns
#{config.root}/app/serializers/concerns
#{config.root}/app/finders/concerns
#{config.root}/app/graphql/resolvers/concerns])
#{config.root}/app/graphql/resolvers/concerns
#{config.root}/app/graphql/mutations/concerns])
config.generators.templates.push("#{config.root}/generator_templates")
......
......@@ -201,6 +201,148 @@ lot of dependant objects.
To limit the amount of queries performed, we can use `BatchLoader`.
## Mutations
Mutations are used to change any stored values, or to trigger
actions. In the same way a GET-request should not modify data, we
cannot modify data in a regular GraphQL-query. We can however in a
mutation.
### Fields
In the most common situations, a mutation would return 2 fields:
- The resource being modified
- A list of errors explaining why the action could not be
performed. If the mutation succeeded, this list would be empty.
By inheriting any new mutations from `Mutations::BaseMutation` the
`errors` field is automatically added. A `clientMutationId` field is
also added, this can be used by the client to identify the result of a
single mutation when multiple are performed within a single request.
### Building Mutations
Mutations live in `app/graphql/mutations` ideally grouped per
resources they are mutating, similar to our services. They should
inherit `Mutations::BaseMutation`. The fields defined on the mutation
will be returned as the result of the mutation.
Always provide a consistent GraphQL-name to the mutation, this name is
used to generate the input types and the field the mutation is mounted
on. The name should look like `<Resource being modified><Mutation
class name>`, for example the `Mutations::MergeRequests::SetWip`
mutation has GraphQL name `MergeRequestSetWip`.
Arguments required by the mutation can be defined as arguments
required for a field. These will be wrapped up in an input type for
the mutation. For example, the `Mutations::MergeRequests::SetWip`
with GraphQL-name `MergeRequestSetWip` defines these arguments:
```ruby
argument :project_path, GraphQL::ID_TYPE,
required: true,
description: "The project the merge request to mutate is in"
argument :iid, GraphQL::ID_TYPE,
required: true,
description: "The iid of the merge request to mutate"
argument :wip,
GraphQL::BOOLEAN_TYPE,
required: false,
description: <<~DESC
Whether or not to set the merge request as a WIP.
If not passed, the value will be toggled.
DESC
```
This would automatically generate an input type called
`MergeRequestSetWipInput` with the 3 arguments we specified and the
`clientMutationId`.
These arguments are then passed to the `resolve` method of a mutation
as keyword arguments. From here, we can call the service that will
modify the resource.
The `resolve` method should then return a hash with the same field
names as defined on the mutation and an `errors` array. For example,
the `Mutations::MergeRequests::SetWip` defines a `merge_request`
field:
```ruby
field :merge_request,
Types::MergeRequestType,
null: true,
description: "The merge request after mutation"
```
This means that the hash returned from `resolve` in this mutation
should look like this:
```ruby
{
# The merge request modified, this will be wrapped in the type
# defined on the field
merge_request: merge_request,
# An array if strings if the mutation failed after authorization
errors: merge_request.errors.full_messages
}
```
To make the mutation available it should be defined on the mutation
type that lives in `graphql/types/mutation_types`. The
`mount_mutation` helper method will define a field based on the
GraphQL-name of the mutation:
```ruby
module Types
class MutationType < BaseObject
include Gitlab::Graphql::MountMutation
graphql_name "Mutation"
mount_mutation Mutations::MergeRequests::SetWip
end
end
```
Will generate a field called `mergeRequestSetWip` that
`Mutations::MergeRequests::SetWip` to be resolved.
### Authorizing resources
To authorize resources inside a mutation, we can include the
`Gitlab::Graphql::Authorize::AuthorizeResource` concern in the
mutation.
This allows us to provide the required abilities on the mutation like
this:
```ruby
module Mutations
module MergeRequests
class SetWip < Base
graphql_name 'MergeRequestSetWip'
authorize :update_merge_request
end
end
end
```
We can then call `authorize!` in the `resolve` method, passing in the resource we
want to validate the abilities for.
Alternatively, we can add a `find_object` method that will load the
object on the mutation. This would allow you to use the
`authorized_find!` and `authorized_find!` helper methods.
When a user is not allowed to perform the action, or an object is not
found, we should raise a
`Gitlab::Graphql::Errors::ResourceNotAvailable` error. Which will be
correctly rendered to the clients.
## Testing
_full stack_ tests for a graphql query or mutation live in
......@@ -212,3 +354,35 @@ be used to test if the query renders valid results.
Using the `GraphqlHelpers#all_graphql_fields_for`-helper, a query
including all available fields can be constructed. This makes it easy
to add a test rendering all possible fields for a query.
To test GraphQL mutation requests, `GraphqlHelpers` provides 2
helpers: `graphql_mutation` which takes the name of the mutation, and
a hash with the input for the mutation. This will return a struct with
a mutation query, and prepared variables.
This struct can then be passed to the `post_graphql_mutation` helper,
that will post the request with the correct params, like a GraphQL
client would do.
To access the response of a mutation, the `graphql_mutation_response`
helper is available.
Using these helpers, we can build specs like this:
```ruby
let(:mutation) do
graphql_mutation(
:merge_request_set_wip,
project_path: 'gitlab-org/gitlab-ce',
iid: '1',
wip: true
)
end
it 'returns a successfull response' do
post_graphql_mutation(mutation, current_user: user)
expect(response).to have_gitlab_http_status(:success)
expect(graphql_mutation_response(:merge_request_set_wip)['errors']).to be_empty
end
```
......@@ -10,7 +10,14 @@ module Gitlab
end
def required_permissions
@required_permissions ||= []
# If the `#authorize` call is used on multiple classes, we add the
# permissions specified on a subclass, to the ones that were specified
# on it's superclass.
@required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions)
superclass.required_permissions.dup
else
[]
end
end
def authorize(*permissions)
......
module Gitlab
module Graphql
module Authorize
module AuthorizeResource
extend ActiveSupport::Concern
included do
extend Gitlab::Graphql::Authorize
end
def find_object(*args)
raise NotImplementedError, "Implement #find_object in #{self.class.name}"
end
def authorized_find(*args)
object = find_object(*args)
object if authorized?(object)
end
def authorized_find!(*args)
object = find_object(*args)
authorize!(object)
object
end
def authorize!(object)
unless authorized?(object)
raise Gitlab::Graphql::Errors::ResourceNotAvailable,
"The resource that you are attempting to access does not exist or you don't have permission to perform this action"
end
end
def authorized?(object)
self.class.required_permissions.all? do |ability|
# The actions could be performed across multiple objects. In which
# case the current user is common, and we could benefit from the
# caching in `DeclarativePolicy`.
Ability.allowed?(current_user, ability, object, scope: :user)
end
end
end
end
end
end
......@@ -3,6 +3,7 @@ module Gitlab
module Errors
BaseError = Class.new(GraphQL::ExecutionError)
ArgumentError = Class.new(BaseError)
ResourceNotAvailable = Class.new(BaseError)
end
end
end
# frozen_string_literal: true
module Gitlab
module Graphql
module MountMutation
extend ActiveSupport::Concern
module ClassMethods
def mount_mutation(mutation_class)
# Using an underscored field name symbol will make `graphql-ruby`
# standardize the field name
field mutation_class.graphql_name.underscore.to_sym,
mutation: mutation_class
end
end
end
end
end
......@@ -18,8 +18,6 @@ describe GitlabSchema do
end
it 'has the base mutation' do
pending('Adding an empty mutation breaks the documentation explorer')
expect(described_class.mutation).to eq(::Types::MutationType.to_graphql)
end
......
require 'spec_helper'
describe Mutations::ResolvesProject do
let(:mutation_class) do
Class.new(Mutations::BaseMutation) do
include Mutations::ResolvesProject
end
end
let(:context) { double }
subject(:mutation) { mutation_class.new(object: nil, context: context) }
it 'uses the ProjectsResolver to resolve projects by path' do
project = create(:project)
expect(Resolvers::ProjectResolver).to receive(:new).with(object: nil, context: context).and_call_original
expect(mutation.resolve_project(full_path: project.full_path)).to eq(project)
end
end
require 'spec_helper'
describe Mutations::MergeRequests::SetWip do
let(:merge_request) { create(:merge_request) }
let(:user) { create(:user) }
subject(:mutation) { described_class.new(object: nil, context: { current_user: user }) }
describe '#resolve' do
let(:wip) { true }
let(:mutated_merge_request) { subject[:merge_request] }
subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, wip: wip) }
it 'raises an error if the resource is not accessible to the user' do
expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
end
context 'when the user can update the merge request' do
before do
merge_request.project.add_developer(user)
end
it 'returns the merge request as a wip' do
expect(mutated_merge_request).to eq(merge_request)
expect(mutated_merge_request).to be_work_in_progress
expect(subject[:errors]).to be_empty
end
it 'returns errors merge request could not be updated' do
# Make the merge request invalid
merge_request.allow_broken = true
merge_request.update!(source_project: nil)
expect(subject[:errors]).not_to be_empty
end
context 'when passing wip as false' do
let(:wip) { false }
it 'removes `wip` from the title' do
merge_request.update(title: "WIP: working on it")
expect(mutated_merge_request).not_to be_work_in_progress
end
it 'does not do anything if the title did not start with wip' do
expect(mutated_merge_request).not_to be_work_in_progress
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Types::MutationType do
it 'is expected to have the MergeRequestSetWip' do
expect(described_class).to have_graphql_mutation(Mutations::MergeRequests::SetWip)
end
end
require 'spec_helper'
describe Gitlab::Graphql::Authorize::AuthorizeResource do
let(:fake_class) do
Class.new do
include Gitlab::Graphql::Authorize::AuthorizeResource
attr_reader :user, :found_object
authorize :read_the_thing
def initialize(user, found_object)
@user, @found_object = user, found_object
end
def find_object
found_object
end
def current_user
user
end
end
end
let(:user) { build(:user) }
let(:project) { build(:project) }
subject(:loading_resource) { fake_class.new(user, project) }
context 'when the user is allowed to perform the action' do
before do
allow(Ability).to receive(:allowed?).with(user, :read_the_thing, project, scope: :user) do
true
end
end
describe '#authorized_find' do
it 'returns the object' do
expect(loading_resource.authorized_find).to eq(project)
end
end
describe '#authorized_find!' do
it 'returns the object' do
expect(loading_resource.authorized_find!).to eq(project)
end
end
describe '#authorize!' do
it 'does not raise an error' do
expect { loading_resource.authorize!(project) }.not_to raise_error
end
end
describe '#authorized?' do
it 'is true' do
expect(loading_resource.authorized?(project)).to be(true)
end
end
end
context 'when the user is not allowed to perform the action' do
before do
allow(Ability).to receive(:allowed?).with(user, :read_the_thing, project, scope: :user) do
false
end
end
describe '#authorized_find' do
it 'returns `nil`' do
expect(loading_resource.authorized_find).to be_nil
end
end
describe '#authorized_find!' do
it 'raises an error' do
expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
end
end
describe '#authorize!' do
it 'does not raise an error' do
expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
end
end
describe '#authorized?' do
it 'is false' do
expect(loading_resource.authorized?(project)).to be(false)
end
end
end
context 'when the class does not define #find_object' do
let(:fake_class) do
Class.new { include Gitlab::Graphql::Authorize::AuthorizeResource }
end
it 'raises a comprehensive error message' do
expect { fake_class.new.find_object }.to raise_error(/Implement #find_object in #{fake_class.name}/)
end
end
end
require 'spec_helper'
describe Gitlab::Graphql::Authorize do
describe '#authorize' do
it 'adds permissions from subclasses to those of superclasses when used on classes' do
base_class = Class.new do
extend Gitlab::Graphql::Authorize
authorize :base_authorization
end
sub_class = Class.new(base_class) do
authorize :sub_authorization
end
expect(base_class.required_permissions).to contain_exactly(:base_authorization)
expect(sub_class.required_permissions)
.to contain_exactly(:base_authorization, :sub_authorization)
end
end
end
require 'spec_helper'
describe 'Setting WIP status of a merge request' do
include GraphqlHelpers
let(:current_user) { create(:user) }
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:input) { { wip: true } }
let(:mutation) do
variables = {
project_path: project.full_path,
iid: merge_request.iid
}
graphql_mutation(:merge_request_set_wip, variables.merge(input))
end
def mutation_response
graphql_mutation_response(:merge_request_set_wip)
end
before do
project.add_developer(current_user)
end
it 'returns an error if the user is not allowed to update the merge request' do
post_graphql_mutation(mutation, current_user: create(:user))
expect(graphql_errors).not_to be_empty
end
it 'marks the merge request as WIP' do
post_graphql_mutation(mutation, current_user: current_user)
expect(response).to have_gitlab_http_status(:success)
expect(mutation_response['mergeRequest']['title']).to start_with('WIP:')
end
it 'does not do anything if the merge request was already marked `WIP`' do
merge_request.update!(title: 'wip: hello world')
post_graphql_mutation(mutation, current_user: current_user)
expect(response).to have_gitlab_http_status(:success)
expect(mutation_response['mergeRequest']['title']).to start_with('wip:')
end
context 'when passing WIP false as input' do
let(:input) { { wip: false } }
it 'does not do anything if the merge reqeust was not marked wip' do
post_graphql_mutation(mutation, current_user: current_user)
expect(response).to have_gitlab_http_status(:success)
expect(mutation_response['mergeRequest']['title']).not_to start_with(/wip\:/)
end
it 'unmarks the merge request as `WIP`' do
merge_request.update!(title: 'wip: hello world')
post_graphql_mutation(mutation, current_user: current_user)
expect(response).to have_gitlab_http_status(:success)
expect(mutation_response['mergeRequest']['title']).not_to start_with('/wip\:/')
end
end
end
module GraphqlHelpers
MutationDefinition = Struct.new(:query, :variables)