From 3bcb04f100f5e982379fbeae37a30a191581d1ef Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 10 Jul 2018 16:19:45 +0200 Subject: [PATCH] Add mutation toggling WIP state of merge requests This is mainly the setup of mutations for GraphQL. Including authorization and basic return type-structure. --- app/graphql/gitlab_schema.rb | 2 +- app/graphql/mutations/base_mutation.rb | 13 ++ .../concerns/mutations/resolves_project.rb | 13 ++ app/graphql/mutations/merge_requests/base.rb | 32 ++++ .../mutations/merge_requests/set_wip.rb | 35 ++++ app/graphql/types/mutation_type.rb | 6 +- .../unreleased/bvl-graphql-wip-mutation.yml | 5 + config/application.rb | 3 +- doc/development/api_graphql_styleguide.md | 174 ++++++++++++++++++ lib/gitlab/graphql/authorize.rb | 9 +- .../graphql/authorize/authorize_resource.rb | 46 +++++ lib/gitlab/graphql/errors.rb | 1 + lib/gitlab/graphql/mount_mutation.rb | 18 ++ spec/graphql/gitlab_schema_spec.rb | 2 - .../mutations/resolves_project_spec.rb | 19 ++ .../mutations/merge_requests/set_wip_spec.rb | 51 +++++ spec/graphql/types/mutation_type_spec.rb | 9 + .../authorize/authorize_resource_spec.rb | 103 +++++++++++ spec/lib/gitlab/graphql/authorize_spec.rb | 20 ++ .../mutations/merge_requests/set_wip_spec.rb | 68 +++++++ spec/support/helpers/graphql_helpers.rb | 61 +++++- spec/support/matchers/graphql_matchers.rb | 9 + .../requests/graphql_shared_examples.rb | 2 +- 23 files changed, 686 insertions(+), 15 deletions(-) create mode 100644 app/graphql/mutations/base_mutation.rb create mode 100644 app/graphql/mutations/concerns/mutations/resolves_project.rb create mode 100644 app/graphql/mutations/merge_requests/base.rb create mode 100644 app/graphql/mutations/merge_requests/set_wip.rb create mode 100644 changelogs/unreleased/bvl-graphql-wip-mutation.yml create mode 100644 lib/gitlab/graphql/authorize/authorize_resource.rb create mode 100644 lib/gitlab/graphql/mount_mutation.rb create mode 100644 spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb create mode 100644 spec/graphql/mutations/merge_requests/set_wip_spec.rb create mode 100644 spec/graphql/types/mutation_type_spec.rb create mode 100644 spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb create mode 100644 spec/lib/gitlab/graphql/authorize_spec.rb create mode 100644 spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index d9f9129d08a..8755a1a62e7 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -7,5 +7,5 @@ class GitlabSchema < GraphQL::Schema query(Types::QueryType) default_max_page_size 100 - # mutation(Types::MutationType) + mutation(Types::MutationType) end diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb new file mode 100644 index 00000000000..eb03dfe1624 --- /dev/null +++ b/app/graphql/mutations/base_mutation.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/app/graphql/mutations/concerns/mutations/resolves_project.rb b/app/graphql/mutations/concerns/mutations/resolves_project.rb new file mode 100644 index 00000000000..0dd1f264a52 --- /dev/null +++ b/app/graphql/mutations/concerns/mutations/resolves_project.rb @@ -0,0 +1,13 @@ +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 diff --git a/app/graphql/mutations/merge_requests/base.rb b/app/graphql/mutations/merge_requests/base.rb new file mode 100644 index 00000000000..2149e72e2df --- /dev/null +++ b/app/graphql/mutations/merge_requests/base.rb @@ -0,0 +1,32 @@ +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 diff --git a/app/graphql/mutations/merge_requests/set_wip.rb b/app/graphql/mutations/merge_requests/set_wip.rb new file mode 100644 index 00000000000..a2aa0c84ee4 --- /dev/null +++ b/app/graphql/mutations/merge_requests/set_wip.rb @@ -0,0 +1,35 @@ +# 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 diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 06ed91c1658..2b4ef299296 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -1,7 +1,11 @@ +# 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 diff --git a/changelogs/unreleased/bvl-graphql-wip-mutation.yml b/changelogs/unreleased/bvl-graphql-wip-mutation.yml new file mode 100644 index 00000000000..00aa1c48677 --- /dev/null +++ b/changelogs/unreleased/bvl-graphql-wip-mutation.yml @@ -0,0 +1,5 @@ +--- +title: Add the first mutations for merge requests to GraphQL +merge_request: 20443 +author: +type: added diff --git a/config/application.rb b/config/application.rb index 0304f466734..f743df7ca11 100644 --- a/config/application.rb +++ b/config/application.rb @@ -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") diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index b4a2349844b..6c6e198a7c3 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -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 ``, 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 +``` diff --git a/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb index 04f25c53e49..93a903915b0 100644 --- a/lib/gitlab/graphql/authorize.rb +++ b/lib/gitlab/graphql/authorize.rb @@ -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) diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb new file mode 100644 index 00000000000..40895686a8a --- /dev/null +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -0,0 +1,46 @@ +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 diff --git a/lib/gitlab/graphql/errors.rb b/lib/gitlab/graphql/errors.rb index 1d8e8307ab9..f8c7ec24be1 100644 --- a/lib/gitlab/graphql/errors.rb +++ b/lib/gitlab/graphql/errors.rb @@ -3,6 +3,7 @@ module Gitlab module Errors BaseError = Class.new(GraphQL::ExecutionError) ArgumentError = Class.new(BaseError) + ResourceNotAvailable = Class.new(BaseError) end end end diff --git a/lib/gitlab/graphql/mount_mutation.rb b/lib/gitlab/graphql/mount_mutation.rb new file mode 100644 index 00000000000..8cab84d7a5f --- /dev/null +++ b/lib/gitlab/graphql/mount_mutation.rb @@ -0,0 +1,18 @@ +# 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 diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index 515bbe78cb7..b9ddb427e85 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -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 diff --git a/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb b/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb new file mode 100644 index 00000000000..19f5a8907a2 --- /dev/null +++ b/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb @@ -0,0 +1,19 @@ +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 diff --git a/spec/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/graphql/mutations/merge_requests/set_wip_spec.rb new file mode 100644 index 00000000000..e600abf3941 --- /dev/null +++ b/spec/graphql/mutations/merge_requests/set_wip_spec.rb @@ -0,0 +1,51 @@ +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 diff --git a/spec/graphql/types/mutation_type_spec.rb b/spec/graphql/types/mutation_type_spec.rb new file mode 100644 index 00000000000..a67d83b1edf --- /dev/null +++ b/spec/graphql/types/mutation_type_spec.rb @@ -0,0 +1,9 @@ +# 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 diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb new file mode 100644 index 00000000000..95bf7685ade --- /dev/null +++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb @@ -0,0 +1,103 @@ +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 diff --git a/spec/lib/gitlab/graphql/authorize_spec.rb b/spec/lib/gitlab/graphql/authorize_spec.rb new file mode 100644 index 00000000000..9c17a3b0e4b --- /dev/null +++ b/spec/lib/gitlab/graphql/authorize_spec.rb @@ -0,0 +1,20 @@ +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 diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb new file mode 100644 index 00000000000..8f427d71a32 --- /dev/null +++ b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb @@ -0,0 +1,68 @@ +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 diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index b9322975b5a..75827df80dc 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -1,4 +1,6 @@ module GraphqlHelpers + MutationDefinition = Struct.new(:query, :variables) + # makes an underscored string look like a fieldname # "merge_request" => "mergeRequest" def self.fieldnamerize(underscored_field_name) @@ -41,6 +43,37 @@ module GraphqlHelpers QUERY end + def graphql_mutation(name, input, fields = nil) + mutation_name = GraphqlHelpers.fieldnamerize(name) + input_variable_name = "$#{input_variable_name_for_mutation(name)}" + mutation_field = GitlabSchema.mutation.fields[mutation_name] + fields ||= all_graphql_fields_for(mutation_field.type) + + query = <<~MUTATION + mutation(#{input_variable_name}: #{mutation_field.arguments['input'].type}) { + #{mutation_name}(input: #{input_variable_name}) { + #{fields} + } + } + MUTATION + variables = variables_for_mutation(name, input) + + MutationDefinition.new(query, variables) + end + + def variables_for_mutation(name, input) + graphql_input = input.map { |name, value| [GraphqlHelpers.fieldnamerize(name), value] }.to_h + { input_variable_name_for_mutation(name) => graphql_input }.to_json + end + + def input_variable_name_for_mutation(mutation_name) + mutation_name = GraphqlHelpers.fieldnamerize(mutation_name) + mutation_field = GitlabSchema.mutation.fields[mutation_name] + input_type = field_type(mutation_field.arguments['input']) + + GraphqlHelpers.fieldnamerize(input_type) + end + def query_graphql_field(name, attributes = {}, fields = nil) fields ||= all_graphql_fields_for(name.classify) attributes = attributes_to_graphql(attributes) @@ -73,8 +106,12 @@ module GraphqlHelpers end.join(", ") end - def post_graphql(query, current_user: nil) - post api('/', current_user, version: 'graphql'), query: query + def post_graphql(query, current_user: nil, variables: nil) + post api('/', current_user, version: 'graphql'), query: query, variables: variables + end + + def post_graphql_mutation(mutation, current_user: nil) + post_graphql(mutation.query, current_user: current_user, variables: mutation.variables) end def graphql_data @@ -82,7 +119,11 @@ module GraphqlHelpers end def graphql_errors - json_response['data'] + json_response['errors'] + end + + def graphql_mutation_response(mutation_name) + graphql_data[GraphqlHelpers.fieldnamerize(mutation_name)] end def nested_fields?(field) @@ -102,10 +143,14 @@ module GraphqlHelpers end def field_type(field) - if field.type.respond_to?(:of_type) - field.type.of_type - else - field.type - end + field_type = field.type + + # The type could be nested. For example `[GraphQL::STRING_TYPE]`: + # - List + # - String! + # - String + field_type = field_type.of_type while field_type.respond_to?(:of_type) + + field_type end end diff --git a/spec/support/matchers/graphql_matchers.rb b/spec/support/matchers/graphql_matchers.rb index be6fa4c71a0..7be84838e00 100644 --- a/spec/support/matchers/graphql_matchers.rb +++ b/spec/support/matchers/graphql_matchers.rb @@ -34,6 +34,15 @@ RSpec::Matchers.define :have_graphql_field do |field_name| end end +RSpec::Matchers.define :have_graphql_mutation do |mutation_class| + match do |mutation_type| + field = mutation_type.fields[GraphqlHelpers.fieldnamerize(mutation_class.graphql_name)] + + expect(field).to be_present + expect(field.resolver).to eq(mutation_class) + end +end + RSpec::Matchers.define :have_graphql_arguments do |*expected| include GraphqlHelpers diff --git a/spec/support/shared_examples/requests/graphql_shared_examples.rb b/spec/support/shared_examples/requests/graphql_shared_examples.rb index fe7b7bc306f..04140cad3f0 100644 --- a/spec/support/shared_examples/requests/graphql_shared_examples.rb +++ b/spec/support/shared_examples/requests/graphql_shared_examples.rb @@ -5,7 +5,7 @@ shared_examples 'a working graphql query' do it 'returns a successful response', :aggregate_failures do expect(response).to have_gitlab_http_status(:success) - expect(graphql_errors['errors']).to be_nil + expect(graphql_errors).to be_nil expect(json_response.keys).to include('data') end end