Merge branch 'bvl-graphql-wip-mutation' into 'master'

toggle wip status using GraphQL mutation

Closes #48719

See merge request gitlab-org/gitlab-ce!20443

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

app/graphql/mutations/base_mutation.rb

+# 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

app/graphql/mutations/concerns/mutations/resolves_project.rb

+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

app/graphql/mutations/merge_requests/base.rb

+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

app/graphql/mutations/merge_requests/set_wip.rb

+# 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

app/graphql/types/mutation_type.rb

+# 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

changelogs/unreleased/bvl-graphql-wip-mutation.yml

+---
+title: Add the first mutations for merge requests to GraphQL
+merge_request: 20443
+author:
+type: added

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")

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 `<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
+```

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)

lib/gitlab/graphql/authorize/authorize_resource.rb

+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

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

lib/gitlab/graphql/mount_mutation.rb

+# 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

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

spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb

+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

spec/graphql/mutations/merge_requests/set_wip_spec.rb

+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

spec/graphql/types/mutation_type_spec.rb

+# 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

spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb

+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

spec/lib/gitlab/graphql/authorize_spec.rb

+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

spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb

+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