From 22d5c28435b18abcad7a9ee95a22993cf782c055 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 8 Dec 2020 11:28:58 +0000 Subject: [PATCH] Add support for MRs assigned for review This is in analogue to authoredMergeRequests and assignedMergeRequests. Support for this feature is complete at the model and finder level, and this change exposes that at the API level. This supports filtering by review with: - User.reviewRequestedMergeRequests - User.assignedMergeRequests(reviewer_username:) - User.authoredMergeRequests(reviewer_username:) - Project.mergeRequests(reviewer_username:) --- .../assigned_merge_requests_resolver.rb | 1 + .../authored_merge_requests_resolver.rb | 1 + .../resolvers/merge_requests_resolver.rb | 6 + .../project_merge_requests_resolver.rb | 1 + ...eview_requested_merge_requests_resolver.rb | 13 + app/graphql/types/user_type.rb | 5 +- .../ajk-gql-reviewer-merge-requests.yml | 5 + .../graphql/reference/gitlab_schema.graphql | 105 ++++++++ doc/api/graphql/reference/gitlab_schema.json | 245 ++++++++++++++++++ doc/api/graphql/reference/index.md | 1 + .../project_merge_requests_resolver_spec.rb | 24 +- spec/graphql/types/project_type_spec.rb | 1 + spec/graphql/types/user_type_spec.rb | 1 + spec/requests/api/graphql/user_query_spec.rb | 139 ++++++++++ 14 files changed, 543 insertions(+), 5 deletions(-) create mode 100644 app/graphql/resolvers/review_requested_merge_requests_resolver.rb create mode 100644 changelogs/unreleased/ajk-gql-reviewer-merge-requests.yml diff --git a/app/graphql/resolvers/assigned_merge_requests_resolver.rb b/app/graphql/resolvers/assigned_merge_requests_resolver.rb index 30415ef5d2d258..385f8db51b0075 100644 --- a/app/graphql/resolvers/assigned_merge_requests_resolver.rb +++ b/app/graphql/resolvers/assigned_merge_requests_resolver.rb @@ -4,6 +4,7 @@ module Resolvers class AssignedMergeRequestsResolver < UserMergeRequestsResolverBase type ::Types::MergeRequestType.connection_type, null: true accept_author + accept_reviewer def user_role :assignee diff --git a/app/graphql/resolvers/authored_merge_requests_resolver.rb b/app/graphql/resolvers/authored_merge_requests_resolver.rb index 1426ca83c064d4..4de1046ce0d2e2 100644 --- a/app/graphql/resolvers/authored_merge_requests_resolver.rb +++ b/app/graphql/resolvers/authored_merge_requests_resolver.rb @@ -4,6 +4,7 @@ module Resolvers class AuthoredMergeRequestsResolver < UserMergeRequestsResolverBase type ::Types::MergeRequestType.connection_type, null: true accept_assignee + accept_reviewer def user_role :author diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb index c6b9448c9b68b1..4629fed2a4ef6b 100644 --- a/app/graphql/resolvers/merge_requests_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -18,6 +18,12 @@ def self.accept_author description: 'Username of the author' end + def self.accept_reviewer + argument :reviewer_username, GraphQL::STRING_TYPE, + required: false, + description: 'Username of the reviewer' + end + argument :iids, [GraphQL::STRING_TYPE], required: false, description: 'Array of IIDs of merge requests, for example `[1, 2]`' diff --git a/app/graphql/resolvers/project_merge_requests_resolver.rb b/app/graphql/resolvers/project_merge_requests_resolver.rb index bf082c0b1824d1..830649d5e52b1a 100644 --- a/app/graphql/resolvers/project_merge_requests_resolver.rb +++ b/app/graphql/resolvers/project_merge_requests_resolver.rb @@ -5,5 +5,6 @@ class ProjectMergeRequestsResolver < MergeRequestsResolver type ::Types::MergeRequestType.connection_type, null: true accept_assignee accept_author + accept_reviewer end end diff --git a/app/graphql/resolvers/review_requested_merge_requests_resolver.rb b/app/graphql/resolvers/review_requested_merge_requests_resolver.rb new file mode 100644 index 00000000000000..e0ab7b5b600548 --- /dev/null +++ b/app/graphql/resolvers/review_requested_merge_requests_resolver.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Resolvers + class ReviewRequestedMergeRequestsResolver < UserMergeRequestsResolverBase + type ::Types::MergeRequestType.connection_type, null: true + accept_author + accept_assignee + + def user_role + :reviewer + end + end +end diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb index 86c10b908684bd..935032683195f7 100644 --- a/app/graphql/types/user_type.rb +++ b/app/graphql/types/user_type.rb @@ -48,13 +48,16 @@ class UserType < BaseObject description: 'Projects starred by the user', resolver: Resolvers::UserStarredProjectsResolver - # Merge request field: MRs can be either authored or assigned: + # Merge request field: MRs can be authored, assigned, or assigned-for-review: field :authored_merge_requests, resolver: Resolvers::AuthoredMergeRequestsResolver, description: 'Merge Requests authored by the user' field :assigned_merge_requests, resolver: Resolvers::AssignedMergeRequestsResolver, description: 'Merge Requests assigned to the user' + field :review_requested_merge_requests, + resolver: Resolvers::ReviewRequestedMergeRequestsResolver, + description: 'Merge Requests assigned to the user for review' field :snippets, Types::SnippetType.connection_type, diff --git a/changelogs/unreleased/ajk-gql-reviewer-merge-requests.yml b/changelogs/unreleased/ajk-gql-reviewer-merge-requests.yml new file mode 100644 index 00000000000000..8156e684bb4e58 --- /dev/null +++ b/changelogs/unreleased/ajk-gql-reviewer-merge-requests.yml @@ -0,0 +1,5 @@ +--- +title: Support merge requests filtered by reviewer in GraphQL API +merge_request: 49464 +author: +type: changed diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index e9924bd52d79d1..253d9232470841 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -16948,6 +16948,11 @@ type Project { """ milestoneTitle: String + """ + Username of the reviewer + """ + reviewerUsername: String + """ Sort merge requests by this criteria """ @@ -23909,6 +23914,11 @@ type User { """ projectPath: String + """ + Username of the reviewer + """ + reviewerUsername: String + """ Sort merge requests by this criteria """ @@ -23994,6 +24004,11 @@ type User { """ projectPath: String + """ + Username of the reviewer + """ + reviewerUsername: String + """ Sort merge requests by this criteria """ @@ -24100,6 +24115,96 @@ type User { """ publicEmail: String + """ + Merge Requests assigned to the user for review + """ + reviewRequestedMergeRequests( + """ + Returns the elements in the list that come after the specified cursor. + """ + after: String + + """ + Username of the assignee + """ + assigneeUsername: String + + """ + Username of the author + """ + authorUsername: String + + """ + Returns the elements in the list that come before the specified cursor. + """ + before: String + + """ + Returns the first _n_ elements from the list. + """ + first: Int + + """ + Array of IIDs of merge requests, for example `[1, 2]` + """ + iids: [String!] + + """ + Array of label names. All resolved merge requests will have all of these labels. + """ + labels: [String!] + + """ + Returns the last _n_ elements from the list. + """ + last: Int + + """ + Merge requests merged after this date + """ + mergedAfter: Time + + """ + Merge requests merged before this date + """ + mergedBefore: Time + + """ + Title of the milestone + """ + milestoneTitle: String + + """ + The global ID of the project the authored merge requests should be in. Incompatible with projectPath. + """ + projectId: ProjectID + + """ + The full-path of the project the authored merge requests should be in. Incompatible with projectId. + """ + projectPath: String + + """ + Sort merge requests by this criteria + """ + sort: MergeRequestSort = created_desc + + """ + Array of source branch names. All resolved merge requests will have one of these branches as their source. + """ + sourceBranches: [String!] + + """ + A merge request state. If provided, all resolved merge requests will have this state. + """ + state: MergeRequestState + + """ + Array of target branch names. All resolved merge requests will have one of these branches as their target. + """ + targetBranches: [String!] + ): MergeRequestConnection + """ Snippets authored by the user """ diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 3782960aece9fb..51d355baa4bfb5 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -49877,6 +49877,16 @@ }, "defaultValue": null }, + { + "name": "reviewerUsername", + "description": "Username of the reviewer", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, { "name": "after", "description": "Returns the elements in the list that come after the specified cursor.", @@ -69727,6 +69737,16 @@ }, "defaultValue": null }, + { + "name": "reviewerUsername", + "description": "Username of the reviewer", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, { "name": "after", "description": "Returns the elements in the list that come after the specified cursor.", @@ -69932,6 +69952,16 @@ }, "defaultValue": null }, + { + "name": "reviewerUsername", + "description": "Username of the reviewer", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, { "name": "after", "description": "Returns the elements in the list that come after the specified cursor.", @@ -70193,6 +70223,221 @@ "isDeprecated": false, "deprecationReason": null }, + { + "name": "reviewRequestedMergeRequests", + "description": "Merge Requests assigned to the user for review", + "args": [ + { + "name": "iids", + "description": "Array of IIDs of merge requests, for example `[1, 2]`", + "type": { + "kind": "LIST", + "name": null, + "ofType": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } + }, + "defaultValue": null + }, + { + "name": "sourceBranches", + "description": "Array of source branch names. All resolved merge requests will have one of these branches as their source.", + "type": { + "kind": "LIST", + "name": null, + "ofType": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } + }, + "defaultValue": null + }, + { + "name": "targetBranches", + "description": "Array of target branch names. All resolved merge requests will have one of these branches as their target.", + "type": { + "kind": "LIST", + "name": null, + "ofType": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } + }, + "defaultValue": null + }, + { + "name": "state", + "description": "A merge request state. If provided, all resolved merge requests will have this state.", + "type": { + "kind": "ENUM", + "name": "MergeRequestState", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "labels", + "description": "Array of label names. All resolved merge requests will have all of these labels.", + "type": { + "kind": "LIST", + "name": null, + "ofType": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } + }, + "defaultValue": null + }, + { + "name": "mergedAfter", + "description": "Merge requests merged after this date", + "type": { + "kind": "SCALAR", + "name": "Time", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "mergedBefore", + "description": "Merge requests merged before this date", + "type": { + "kind": "SCALAR", + "name": "Time", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "milestoneTitle", + "description": "Title of the milestone", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "sort", + "description": "Sort merge requests by this criteria", + "type": { + "kind": "ENUM", + "name": "MergeRequestSort", + "ofType": null + }, + "defaultValue": "created_desc" + }, + { + "name": "projectPath", + "description": "The full-path of the project the authored merge requests should be in. Incompatible with projectId.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "projectId", + "description": "The global ID of the project the authored merge requests should be in. Incompatible with projectPath.", + "type": { + "kind": "SCALAR", + "name": "ProjectID", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "authorUsername", + "description": "Username of the author", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "assigneeUsername", + "description": "Username of the assignee", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "after", + "description": "Returns the elements in the list that come after the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "before", + "description": "Returns the elements in the list that come before the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "first", + "description": "Returns the first _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "last", + "description": "Returns the last _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + } + ], + "type": { + "kind": "OBJECT", + "name": "MergeRequestConnection", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, { "name": "snippets", "description": "Snippets authored by the user", diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 4380152d79e503..e47bf137159cd3 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3643,6 +3643,7 @@ Autogenerated return type of UpdateSnippet. | `name` | String! | Human-readable name of the user | | `projectMemberships` | ProjectMemberConnection | Project memberships of the user | | `publicEmail` | String | User's public email | +| `reviewRequestedMergeRequests` | MergeRequestConnection | Merge Requests assigned to the user for review | | `snippets` | SnippetConnection | Snippets authored by the user | | `starredProjects` | ProjectConnection | Projects starred by the user | | `state` | UserState! | State of the user | diff --git a/spec/graphql/resolvers/project_merge_requests_resolver_spec.rb b/spec/graphql/resolvers/project_merge_requests_resolver_spec.rb index 96ca679620dd05..32a264469ba0db 100644 --- a/spec/graphql/resolvers/project_merge_requests_resolver_spec.rb +++ b/spec/graphql/resolvers/project_merge_requests_resolver_spec.rb @@ -8,14 +8,16 @@ let_it_be(:project) { create(:project, :repository) } let_it_be(:current_user) { create(:user) } let_it_be(:other_user) { create(:user) } + let_it_be(:reviewer) { create(:user) } - let_it_be(:merge_request_with_author_and_assignee) do + let_it_be(:merge_request) do create(:merge_request, :unique_branches, source_project: project, target_project: project, author: other_user, - assignee: other_user) + assignee: other_user, + reviewers: [reviewer]) end before do @@ -26,7 +28,7 @@ it 'filters merge requests by assignee username' do result = resolve_mr(project, assignee_username: other_user.username) - expect(result).to eq([merge_request_with_author_and_assignee]) + expect(result).to eq([merge_request]) end it 'does not find anything' do @@ -40,7 +42,7 @@ it 'filters merge requests by author username' do result = resolve_mr(project, author_username: other_user.username) - expect(result).to eq([merge_request_with_author_and_assignee]) + expect(result).to eq([merge_request]) end it 'does not find anything' do @@ -50,6 +52,20 @@ end end + context 'by reviewer' do + it 'filters merge requests by reviewer username' do + result = resolve_mr(project, reviewer_username: reviewer.username) + + expect(result).to eq([merge_request]) + end + + it 'does not find anything' do + result = resolve_mr(project, reviewer_username: 'unknown-user') + + expect(result).to be_empty + end + end + def resolve_mr(project, resolver: described_class, user: current_user, **args) resolve(resolver, obj: project, args: args, ctx: { current_user: user }) end diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index ff99dd772339ba..fe0af39f9f3168 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -79,6 +79,7 @@ :merged_before, :author_username, :assignee_username, + :reviewer_username, :milestone_title, :sort ) diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb index c7357782fbfa5f..0eff33bb25bdd1 100644 --- a/spec/graphql/types/user_type_spec.rb +++ b/spec/graphql/types/user_type_spec.rb @@ -25,6 +25,7 @@ location authoredMergeRequests assignedMergeRequests + reviewRequestedMergeRequests groupMemberships groupCount projectMemberships diff --git a/spec/requests/api/graphql/user_query_spec.rb b/spec/requests/api/graphql/user_query_spec.rb index 3e7ef8838c0d9a..60520906e875de 100644 --- a/spec/requests/api/graphql/user_query_spec.rb +++ b/spec/requests/api/graphql/user_query_spec.rb @@ -58,9 +58,25 @@ source_project: project_b, author: user) end + let_it_be(:reviewed_mr) do + create(:merge_request, :unique_branches, :unique_author, + source_project: project_a, reviewers: [user]) + end + + let_it_be(:reviewed_mr_b) do + create(:merge_request, :unique_branches, :unique_author, + source_project: project_b, reviewers: [user]) + end + + let_it_be(:reviewed_mr_c) do + create(:merge_request, :unique_branches, :unique_author, + source_project: project_b, reviewers: [user]) + end + let(:current_user) { authorised_user } let(:authored_mrs) { graphql_data_at(:user, :authored_merge_requests, :nodes) } let(:assigned_mrs) { graphql_data_at(:user, :assigned_merge_requests, :nodes) } + let(:reviewed_mrs) { graphql_data_at(:user, :review_requested_merge_requests, :nodes) } let(:user_params) { { username: user.username } } before do @@ -157,6 +173,23 @@ ) end end + + context 'filtering by reviewer' do + let(:reviewer) { create(:user) } + let(:mr_args) { { reviewer_username: reviewer.username } } + + it 'finds the assigned mrs' do + assigned_mr_b.reviewers << reviewer + assigned_mr_c.reviewers << reviewer + + post_graphql(query, current_user: current_user) + + expect(assigned_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(assigned_mr_b)), + a_hash_including('id' => global_id_of(assigned_mr_c)) + ) + end + end end context 'the current user does not have access' do @@ -168,6 +201,95 @@ end end + describe 'reviewRequestedMergeRequests' do + let(:user_fields) do + query_graphql_field(:review_requested_merge_requests, mr_args, 'nodes { id }') + end + + let(:mr_args) { nil } + + it_behaves_like 'a working graphql query' + + it 'can be found' do + expect(reviewed_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(reviewed_mr)), + a_hash_including('id' => global_id_of(reviewed_mr_b)), + a_hash_including('id' => global_id_of(reviewed_mr_c)) + ) + end + + context 'applying filters' do + context 'filtering by IID without specifying a project' do + let(:mr_args) do + { iids: [reviewed_mr_b.iid.to_s] } + end + + it 'return an argument error that mentions the missing fields' do + expect_graphql_errors_to_include(/projectPath/) + end + end + + context 'filtering by project path and IID' do + let(:mr_args) do + { project_path: project_b.full_path, iids: [reviewed_mr_b.iid.to_s] } + end + + it 'selects the correct MRs' do + expect(reviewed_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(reviewed_mr_b)) + ) + end + end + + context 'filtering by project path' do + let(:mr_args) do + { project_path: project_b.full_path } + end + + it 'selects the correct MRs' do + expect(reviewed_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(reviewed_mr_b)), + a_hash_including('id' => global_id_of(reviewed_mr_c)) + ) + end + end + + context 'filtering by author' do + let(:author) { reviewed_mr_b.author } + let(:mr_args) { { author_username: author.username } } + + it 'finds the authored mrs' do + expect(reviewed_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(reviewed_mr_b)) + ) + end + end + + context 'filtering by assignee' do + let(:assignee) { create(:user) } + let(:mr_args) { { assignee_username: assignee.username } } + + it 'finds the authored mrs' do + reviewed_mr_c.assignees << assignee + + post_graphql(query, current_user: current_user) + + expect(reviewed_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(reviewed_mr_c)) + ) + end + end + end + + context 'the current user does not have access' do + let(:current_user) { unauthorized_user } + + it 'cannot be found' do + expect(reviewed_mrs).to be_empty + end + end + end + describe 'authoredMergeRequests' do let(:user_fields) do query_graphql_field(:authored_merge_requests, mr_args, 'nodes { id }') @@ -213,6 +335,23 @@ end end + context 'filtering by reviewer' do + let(:reviewer) { create(:user) } + let(:mr_args) { { reviewer_username: reviewer.username } } + + it 'finds the assigned mrs' do + authored_mr_b.reviewers << reviewer + authored_mr_c.reviewers << reviewer + + post_graphql(query, current_user: current_user) + + expect(authored_mrs).to contain_exactly( + a_hash_including('id' => global_id_of(authored_mr_b)), + a_hash_including('id' => global_id_of(authored_mr_c)) + ) + end + end + context 'filtering by project path and IID' do let(:mr_args) do { project_path: project_b.full_path, iids: [authored_mr_b.iid.to_s] } -- GitLab