Merge branch '13049-graphql-resolve-discussion' into 'master'

Add GraphQL mutation for toggling the resolved status of a discussion

See merge request gitlab-org/gitlab!32934

app/graphql/mutations/discussions/toggle_resolve.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Discussions
+    class ToggleResolve < BaseMutation
+      graphql_name 'DiscussionToggleResolve'
+
+      description 'Toggles the resolved state of a discussion'
+
+      argument :id,
+                GraphQL::ID_TYPE,
+                required: true,
+                description: 'The global id of the discussion'
+
+      argument :resolve,
+               GraphQL::BOOLEAN_TYPE,
+               required: true,
+               description: 'Will resolve the discussion when true, and unresolve the discussion when false'
+
+      field :discussion,
+            Types::Notes::DiscussionType,
+            null: true,
+            description: 'The discussion after mutation'
+
+      def resolve(id:, resolve:)
+        discussion = authorized_find_discussion!(id: id)
+        errors = []
+
+        begin
+          if resolve
+            resolve!(discussion)
+          else
+            unresolve!(discussion)
+          end
+        rescue ActiveRecord::RecordNotSaved
+          errors << "Discussion failed to be #{'un' unless resolve}resolved"
+        end
+
+        {
+          discussion: discussion,
+          errors: errors
+        }
+      end
+
+      private
+
+      # `Discussion` permissions are checked through `Discussion#can_resolve?`,
+      # so we use this method of checking permissions rather than by defining
+      # an `authorize` permission and calling `authorized_find!`.
+      def authorized_find_discussion!(id:)
+        find_object(id: id).tap do |discussion|
+          raise_resource_not_available_error! unless discussion&.can_resolve?(current_user)
+        end
+      end
+
+      def find_object(id:)
+        GitlabSchema.object_from_id(id, expected_type: ::Discussion)
+      end
+
+      def resolve!(discussion)
+        ::Discussions::ResolveService.new(
+          discussion.project,
+          current_user,
+          one_or_more_discussions: discussion
+        ).execute
+      end
+
+      def unresolve!(discussion)
+        discussion.unresolve!
+      end
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -14,6 +14,7 @@ module Types
     mount_mutation Mutations::AwardEmojis::Toggle
     mount_mutation Mutations::Branches::Create, calls_gitaly: true
     mount_mutation Mutations::Commits::Create, calls_gitaly: true
+    mount_mutation Mutations::Discussions::ToggleResolve
     mount_mutation Mutations::Issues::SetConfidential
     mount_mutation Mutations::Issues::SetDueDate
     mount_mutation Mutations::Issues::Update

app/graphql/types/notes/discussion_type.rb

@@ -7,6 +7,8 @@ module Types
 
       authorize :read_note
 
+      implements(Types::ResolvableInterface)
+
       field :id, GraphQL::ID_TYPE, null: false,
             description: "ID of this discussion"
       field :reply_id, GraphQL::ID_TYPE, null: false,

app/graphql/types/notes/note_type.rb

@@ -9,6 +9,8 @@ module Types
 
       expose_permissions Types::PermissionTypes::Note
 
+      implements(Types::ResolvableInterface)
+
       field :id, GraphQL::ID_TYPE, null: false,
             description: 'ID of the note'
 
@@ -22,11 +24,6 @@ module Types
             description: 'User who wrote this note',
             resolve: -> (note, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.author_id).find }
 
-      field :resolved_by, Types::UserType,
-            null: true,
-            description: 'User that resolved the discussion',
-            resolve: -> (note, _args, _context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.resolved_by_id).find }
-
       field :system, GraphQL::BOOLEAN_TYPE,
             null: false,
             description: 'Indicates whether this note was created by the system or by a user'
@@ -44,11 +41,6 @@ module Types
             description: "Timestamp of the note's last activity"
       field :discussion, Types::Notes::DiscussionType, null: true,
             description: 'The discussion this note is a part of'
-      field :resolvable, GraphQL::BOOLEAN_TYPE, null: false,
-            description: 'Indicates if this note can be resolved. That is, if it is a resolvable discussion or simply a standalone note',
-            method: :resolvable?
-      field :resolved_at, Types::TimeType, null: true,
-            description: "Timestamp of the note's resolution"
       field :position, Types::Notes::DiffPositionType, null: true,
             description: 'The position of this note on a diff'
       field :confidential, GraphQL::BOOLEAN_TYPE, null: true,

app/graphql/types/resolvable_interface.rb

+# frozen_string_literal: true
+
+module Types
+  # This Interface contains fields that are shared between objects that include either
+  # the `ResolvableNote` or `ResolvableDiscussion` modules.
+  module ResolvableInterface
+    include Types::BaseInterface
+
+    field :resolved_by, Types::UserType,
+          null: true,
+          description: 'User who resolved the object'
+
+    def resolved_by
+      return unless object.resolved_by_id
+
+      Gitlab::Graphql::Loaders::BatchModelLoader.new(User, object.resolved_by_id).find
+    end
+
+    field :resolved, GraphQL::BOOLEAN_TYPE, null: false,
+          description: 'Indicates if the object is resolved',
+          method: :resolved?
+    field :resolvable, GraphQL::BOOLEAN_TYPE, null: false,
+          description: 'Indicates if the object can be resolved',
+          method: :resolvable?
+    field :resolved_at, Types::TimeType, null: true,
+          description: 'Timestamp of when the object was resolved'
+  end
+end

app/models/discussion.rb

@@ -20,6 +20,7 @@ class Discussion
             :noteable_ability_name,
             :to_ability_name,
             :editable?,
+            :resolved_by_id,
             :system_note_with_references_visible_for?,
             :resource_parent,
 

changelogs/unreleased/13049-graphql-resolve-discussion.yml

+---
+title: Add a GraphQL mutation for toggling the resolved state of a Discussion
+merge_request: 32934
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -2757,7 +2757,7 @@ type DiffRefs {
   startSha: String!
 }
 
-type Discussion {
+type Discussion implements ResolvableInterface {
   """
   Timestamp of the discussion's creation
   """
@@ -2797,6 +2797,26 @@ type Discussion {
   ID used to reply to this discussion
   """
   replyId: ID!
+
+  """
+  Indicates if the object can be resolved
+  """
+  resolvable: Boolean!
+
+  """
+  Indicates if the object is resolved
+  """
+  resolved: Boolean!
+
+  """
+  Timestamp of when the object was resolved
+  """
+  resolvedAt: Time
+
+  """
+  User who resolved the object
+  """
+  resolvedBy: User
 }
 
 """
@@ -2834,6 +2854,46 @@ type DiscussionEdge {
   node: Discussion
 }
 
+"""
+Autogenerated input type of DiscussionToggleResolve
+"""
+input DiscussionToggleResolveInput {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The global id of the discussion
+  """
+  id: ID!
+
+  """
+  Will resolve the discussion when true, and unresolve the discussion when false
+  """
+  resolve: Boolean!
+}
+
+"""
+Autogenerated return type of DiscussionToggleResolve
+"""
+type DiscussionToggleResolvePayload {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The discussion after mutation
+  """
+  discussion: Discussion
+
+  """
+  Errors encountered during execution of the mutation.
+  """
+  errors: [String!]!
+}
+
 """
 Autogenerated input type of DismissVulnerability
 """
@@ -7114,6 +7174,11 @@ type Mutation {
   designManagementUpload(input: DesignManagementUploadInput!): DesignManagementUploadPayload
   destroyNote(input: DestroyNoteInput!): DestroyNotePayload
   destroySnippet(input: DestroySnippetInput!): DestroySnippetPayload
+
+  """
+  Toggles the resolved state of a discussion
+  """
+  discussionToggleResolve(input: DiscussionToggleResolveInput!): DiscussionToggleResolvePayload
   dismissVulnerability(input: DismissVulnerabilityInput!): DismissVulnerabilityPayload
   epicAddIssue(input: EpicAddIssueInput!): EpicAddIssuePayload
   epicSetSubscription(input: EpicSetSubscriptionInput!): EpicSetSubscriptionPayload
@@ -7305,7 +7370,7 @@ type NamespaceEdge {
   node: Namespace
 }
 
-type Note {
+type Note implements ResolvableInterface {
   """
   User who wrote this note
   """
@@ -7352,17 +7417,22 @@ type Note {
   project: Project
 
   """
-  Indicates if this note can be resolved. That is, if it is a resolvable discussion or simply a standalone note
+  Indicates if the object can be resolved
   """
   resolvable: Boolean!
 
   """
-  Timestamp of the note's resolution
+  Indicates if the object is resolved
+  """
+  resolved: Boolean!
+
+  """
+  Timestamp of when the object was resolved
   """
   resolvedAt: Time
 
   """
-  User that resolved the discussion
+  User who resolved the object
   """
   resolvedBy: User
 
@@ -9946,6 +10016,28 @@ type RequirementStatesCount {
   opened: Int
 }
 
+interface ResolvableInterface {
+  """
+  Indicates if the object can be resolved
+  """
+  resolvable: Boolean!
+
+  """
+  Indicates if the object is resolved
+  """
+  resolved: Boolean!
+
+  """
+  Timestamp of when the object was resolved
+  """
+  resolvedAt: Time
+
+  """
+  User who resolved the object
+  """
+  resolvedBy: User
+}
+
 type RootStorageStatistics {
   """
   The CI artifacts size in bytes

doc/api/graphql/reference/index.md

@@ -465,6 +465,20 @@ Autogenerated return type of DestroySnippet
 | `createdAt` | Time! | Timestamp of the discussion's creation |
 | `id` | ID! | ID of this discussion |
 | `replyId` | ID! | ID used to reply to this discussion |
+| `resolvable` | Boolean! | Indicates if the object can be resolved |
+| `resolved` | Boolean! | Indicates if the object is resolved |
+| `resolvedAt` | Time | Timestamp of when the object was resolved |
+| `resolvedBy` | User | User who resolved the object |
+
+## DiscussionToggleResolvePayload
+
+Autogenerated return type of DiscussionToggleResolve
+
+| Name  | Type  | Description |
+| ---   |  ---- | ----------  |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `discussion` | Discussion | The discussion after mutation |
+| `errors` | String! => Array | Errors encountered during execution of the mutation. |
 
 ## DismissVulnerabilityPayload
 
@@ -1095,9 +1109,10 @@ Represents a milestone.
 | `id` | ID! | ID of the note |
 | `position` | DiffPosition | The position of this note on a diff |
 | `project` | Project | Project associated with the note |
-| `resolvable` | Boolean! | Indicates if this note can be resolved. That is, if it is a resolvable discussion or simply a standalone note |
-| `resolvedAt` | Time | Timestamp of the note's resolution |
-| `resolvedBy` | User | User that resolved the discussion |
+| `resolvable` | Boolean! | Indicates if the object can be resolved |
+| `resolved` | Boolean! | Indicates if the object is resolved |
+| `resolvedAt` | Time | Timestamp of when the object was resolved |
+| `resolvedBy` | User | User who resolved the object |
 | `system` | Boolean! | Indicates whether this note was created by the system or by a user |
 | `updatedAt` | Time! | Timestamp of the note's last activity |
 | `userPermissions` | NotePermissions! | Permissions for the current user on the resource |

spec/factories/notes.rb

@@ -23,11 +23,6 @@ FactoryBot.define do
 
     factory :discussion_note_on_merge_request, traits: [:on_merge_request], class: 'DiscussionNote' do
       association :project, :repository
-
-      trait :resolved do
-        resolved_at { Time.now }
-        resolved_by { create(:user) }
-      end
     end
 
     factory :track_mr_picking_note, traits: [:on_merge_request, :system] do
@@ -76,11 +71,6 @@ FactoryBot.define do
         end
       end
 
-      trait :resolved do
-        resolved_at { Time.now }
-        resolved_by { create(:user) }
-      end
-
       factory :image_diff_note_on_merge_request do
         position do
           build(:image_diff_position,
@@ -155,6 +145,11 @@ FactoryBot.define do
       end
     end
 
+    trait :resolved do
+      resolved_at { Time.now }
+      resolved_by { association(:user) }
+    end
+
     trait :system do
       system { true }
     end

spec/graphql/mutations/discussions/toggle_resolve_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Mutations::Discussions::ToggleResolve do
+  subject(:mutation) do
+    described_class.new(object: nil, context: { current_user: user }, field: nil)
+  end
+
+  let_it_be(:project) { create(:project, :repository) }
+
+  describe '#resolve' do
+    subject do
+      mutation.resolve({ id: id_arg, resolve: resolve_arg })
+    end
+
+    let(:id_arg) { discussion.to_global_id.to_s }
+    let(:resolve_arg) { true }
+    let(:mutated_discussion) { subject[:discussion] }
+    let(:errors) { subject[:errors] }
+
+    shared_examples 'a working resolve method' do
+      context 'when the user does not have permission' do
+        let_it_be(:user) { create(:user) }
+
+        it 'raises an error if the resource is not accessible to the user' do
+          expect { subject }.to raise_error(
+            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
+
+      context 'when the user has permission' do
+        let_it_be(:user) { create(:user, developer_projects: [project]) }
+
+        context 'when discussion cannot be found' do
+          let(:id_arg) { "#{discussion.to_global_id}foo" }
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(
+              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
+
+        context 'when discussion is not a Discussion' do
+          let(:discussion) { create(:note, noteable: noteable, project: project) }
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(
+              Gitlab::Graphql::Errors::ArgumentError,
+              "#{discussion.to_global_id} is not a valid id for Discussion."
+            )
+          end
+        end
+
+        shared_examples 'returns a resolved discussion without errors' do
+          it 'returns a resolved discussion' do
+            expect(mutated_discussion).to be_resolved
+          end
+
+          it 'returns empty errors' do
+            expect(errors).to be_empty
+          end
+        end
+
+        shared_examples 'returns an unresolved discussion without errors' do
+          it 'returns an unresolved discussion' do
+            expect(mutated_discussion).not_to be_resolved
+          end
+
+          it 'returns empty errors' do
+            expect(errors).to be_empty
+          end
+        end
+
+        context 'when the `resolve` argument is true' do
+          include_examples 'returns a resolved discussion without errors'
+
+          context 'when the discussion is already resolved' do
+            before do
+              discussion.resolve!(user)
+            end
+
+            include_examples 'returns a resolved discussion without errors'
+          end
+
+          context 'when the service raises an `ActiveRecord::RecordNotSaved` error' do
+            before do
+              allow_next_instance_of(::Discussions::ResolveService) do |service|
+                allow(service).to receive(:execute).and_raise(ActiveRecord::RecordNotSaved)
+              end
+            end
+
+            it 'does not resolve the discussion' do
+              expect(mutated_discussion).not_to be_resolved
+            end
+
+            it 'returns errors' do
+              expect(errors).to contain_exactly('Discussion failed to be resolved')
+            end
+          end
+        end
+
+        context 'when the `resolve` argument is false' do
+          let(:resolve_arg) { false }
+
+          context 'when the discussion is resolved' do
+            before do
+              discussion.resolve!(user)
+            end
+
+            include_examples 'returns an unresolved discussion without errors'
+
+            context 'when the service raises an `ActiveRecord::RecordNotSaved` error' do
+              before do
+                allow_next_instance_of(discussion.class) do |instance|
+                  allow(instance).to receive(:unresolve!).and_raise(ActiveRecord::RecordNotSaved)
+                end
+              end
+
+              it 'does not unresolve the discussion' do
+                expect(mutated_discussion).to be_resolved
+              end
+
+              it 'returns errors' do
+                expect(errors).to contain_exactly('Discussion failed to be unresolved')
+              end
+            end
+          end
+
+          context 'when the discussion is already unresolved' do
+            include_examples 'returns an unresolved discussion without errors'
+          end
+        end
+      end
+    end
+
+    context 'when discussion is on a merge request' do
+      let_it_be(:noteable) { create(:merge_request, source_project: project) }
+      let(:discussion) { create(:diff_note_on_merge_request, noteable: noteable, project: project).to_discussion }
+
+      it_behaves_like 'a working resolve method'
+    end
+
+    context 'when discussion is on a design' do
+      let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project)) }
+      let(:discussion) { create(:diff_note_on_design, noteable: noteable, project: project).to_discussion }
+
+      it_behaves_like 'a working resolve method'
+    end
+  end
+end

spec/graphql/types/notes/diff_position_type_spec.rb

 # frozen_string_literal: true
+
 require 'spec_helper'
 
 describe GitlabSchema.types['DiffPosition'] do
   it 'exposes the expected fields' do
-    expected_fields = [:diff_refs, :file_path, :old_path,
-                       :new_path, :position_type, :old_line, :new_line, :x, :y,
-                       :width, :height]
+    expected_fields = %i[
+      diff_refs
+      file_path
+      height
+      new_line
+      new_path
+      old_line
+      old_path
+      position_type
+      width
+      x
+      y
+    ]
 
     expect(described_class).to have_graphql_fields(*expected_fields)
   end

spec/graphql/types/notes/discussion_type_spec.rb

 # frozen_string_literal: true
+
 require 'spec_helper'
 
 describe GitlabSchema.types['Discussion'] do
-  specify { expect(described_class).to have_graphql_fields(:id, :created_at, :notes, :reply_id) }
+  it 'exposes the expected fields' do
+    expected_fields = %i[
+      created_at
+      id
+      notes
+      reply_id
+      resolvable
+      resolved
+      resolved_at
+      resolved_by
+    ]
+
+    expect(described_class).to have_graphql_fields(*expected_fields)
+  end
 
   specify { expect(described_class).to require_graphql_authorizations(:read_note) }
 end

spec/graphql/types/notes/note_type_spec.rb

 # frozen_string_literal: true
+
 require 'spec_helper'
 
 describe GitlabSchema.types['Note'] do
   it 'exposes the expected fields' do
-    expected_fields = [:id, :project, :author, :body, :created_at,
-                       :updated_at, :discussion, :resolvable, :position, :user_permissions,
-                       :resolved_by, :resolved_at, :system, :body_html, :confidential]
+    expected_fields = %i[
+      author
+      body
+      body_html
+      confidential
+      created_at
+      discussion
+      id
+      position
+      project
+      resolvable
+      resolved
+      resolved_at
+      resolved_by
+      system
+      updated_at
+      user_permissions
+    ]
 
     expect(described_class).to have_graphql_fields(*expected_fields)
   end

spec/graphql/types/notes/noteable_type_spec.rb

 # frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Types::Notes::NoteableType do
-  specify { expect(described_class).to have_graphql_fields(:notes, :discussions) }
+  it 'exposes the expected fields' do
+    expected_fields = %i[
+      discussions
+      notes
+    ]
+
+    expect(described_class).to have_graphql_fields(*expected_fields)
+  end
 
   describe ".resolve_type" do
     it 'knows the correct type for objects' do

spec/graphql/types/resolvable_interface_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Types::ResolvableInterface do
+  it 'exposes the expected fields' do
+    expected_fields = %i[
+      resolvable
+      resolved
+      resolved_at
+      resolved_by
+    ]
+
+    expect(described_class).to have_graphql_fields(*expected_fields)
+  end
+end

spec/requests/api/graphql/mutations/discussions/toggle_resolve_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Toggling the resolve status of a discussion' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:noteable) { create(:merge_request, source_project: project) }
+  let(:discussion) do
+    create(:diff_note_on_merge_request, noteable: noteable, project: project).to_discussion
+  end
+  let(:mutation) do
+    graphql_mutation(:discussion_toggle_resolve, { id: discussion.to_global_id.to_s, resolve: true })
+  end
+  let(:mutation_response) { graphql_mutation_response(:discussion_toggle_resolve) }
+
+  context 'when the user does not have permission' do
+    let_it_be(:current_user) { create(:user) }
+
+    it_behaves_like 'a mutation that returns top-level errors',
+      errors: ["The resource that you are attempting to access does not exist or you don't have permission to perform this action"]
+  end
+
+  context 'when user has permission' do
+    let_it_be(:current_user) { create(:user, developer_projects: [project]) }
+
+    it 'returns the discussion without errors', :aggregate_failures do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_response).to include(
+        'discussion' => be_present,
+        'errors' => be_empty
+      )
+    end
+
+    context 'when an error is encountered' do
+      before do
+        allow_next_instance_of(::Discussions::ResolveService) do |service|
+          allow(service).to receive(:execute).and_raise(ActiveRecord::RecordNotSaved)
+        end
+      end
+
+      it_behaves_like 'a mutation that returns errors in the response',
+        errors: ['Discussion failed to be resolved']
+    end
+  end
+end