From 7621abc072eb2b137b3d975c7ed241a6673efa7b Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 24 Dec 2021 18:13:33 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .rubocop_todo/gitlab/namespaced_class.yml | 1 + app/controllers/projects/issues_controller.rb | 11 ++- app/graphql/mutations/work_items/create.rb | 57 +++++++++++++++ app/graphql/types/mutation_type.rb | 1 + app/graphql/types/work_item_type.rb | 23 ++++++ app/models/issue.rb | 5 ++ app/models/work_item.rb | 6 ++ app/policies/project_policy.rb | 5 ++ app/services/issues/build_service.rb | 24 +++++-- app/services/issues/create_service.rb | 5 +- app/services/work_items/build_service.rb | 11 +++ app/services/work_items/create_service.rb | 19 +++++ doc/api/graphql/reference/index.md | 44 ++++++++++++ spec/models/issue_spec.rb | 11 +++ spec/policies/project_policy_spec.rb | 4 +- .../mutations/work_items/create_spec.rb | 63 ++++++++++++++++ spec/services/issues/create_service_spec.rb | 13 ++++ .../services/work_items/build_service_spec.rb | 20 ++++++ .../work_items/create_service_spec.rb | 72 +++++++++++++++++++ .../graphql/mutation_shared_examples.rb | 2 +- 20 files changed, 385 insertions(+), 12 deletions(-) create mode 100644 app/graphql/mutations/work_items/create.rb create mode 100644 app/graphql/types/work_item_type.rb create mode 100644 app/models/work_item.rb create mode 100644 app/services/work_items/build_service.rb create mode 100644 app/services/work_items/create_service.rb create mode 100644 spec/requests/api/graphql/mutations/work_items/create_spec.rb create mode 100644 spec/services/work_items/build_service_spec.rb create mode 100644 spec/services/work_items/create_service_spec.rb diff --git a/.rubocop_todo/gitlab/namespaced_class.yml b/.rubocop_todo/gitlab/namespaced_class.yml index 898768c2425..f13325378d3 100644 --- a/.rubocop_todo/gitlab/namespaced_class.yml +++ b/.rubocop_todo/gitlab/namespaced_class.yml @@ -369,6 +369,7 @@ Gitlab/NamespacedClass: - app/models/wiki_page.rb - app/models/wiki_page/meta.rb - app/models/wiki_page/slug.rb + - app/models/work_item.rb - app/models/x509_certificate.rb - app/models/x509_commit_signature.rb - app/models/x509_issuer.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index fc67cd98d15..9d043ba0645 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -291,10 +291,12 @@ class Projects::IssuesController < Projects::ApplicationController end def issue_params - params.require(:issue).permit( + all_params = params.require(:issue).permit( *issue_params_attributes, sentry_issue_attributes: [:sentry_issue_identifier] ) + + clean_params(all_params) end def issue_params_attributes @@ -348,6 +350,13 @@ class Projects::IssuesController < Projects::ApplicationController private + def clean_params(all_params) + issue_type = all_params[:issue_type].to_s + all_params.delete(:issue_type) unless WorkItems::Type.allowed_types_for_issues.include?(issue_type) + + all_params + end + def finder_options options = super diff --git a/app/graphql/mutations/work_items/create.rb b/app/graphql/mutations/work_items/create.rb new file mode 100644 index 00000000000..88b8cefd8d2 --- /dev/null +++ b/app/graphql/mutations/work_items/create.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module Mutations + module WorkItems + class Create < BaseMutation + include Mutations::SpamProtection + include FindsProject + + graphql_name 'WorkItemCreate' + + authorize :create_work_item + + argument :description, GraphQL::Types::String, + required: false, + description: copy_field_description(Types::WorkItemType, :description) + argument :project_path, GraphQL::Types::ID, + required: true, + description: 'Full path of the project the work item is associated with.' + argument :title, GraphQL::Types::String, + required: true, + description: copy_field_description(Types::WorkItemType, :title) + argument :work_item_type_id, ::Types::GlobalIDType[::WorkItems::Type], + required: true, + description: 'Global ID of a work item type.' + + field :work_item, Types::WorkItemType, + null: true, + description: 'Created work item.' + + def resolve(project_path:, **attributes) + project = authorized_find!(project_path) + params = global_id_compatibility_params(attributes).merge(author_id: current_user.id) + + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) + work_item = ::WorkItems::CreateService.new(project: project, current_user: current_user, params: params, spam_params: spam_params).execute + + check_spam_action_response!(work_item) + + { + work_item: work_item.valid? ? work_item : nil, + errors: errors_on_object(work_item) + } + end + + private + + def global_id_compatibility_params(params) + # TODO: remove this line when the compatibility layer is removed + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883 + params[:work_item_type_id] = ::Types::GlobalIDType[::WorkItems::Type].coerce_isolated_input(params[:work_item_type_id]) if params[:work_item_type_id] + params[:work_item_type_id] = params[:work_item_type_id]&.model_id + + params + end + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index e8a952e9c61..9bba0e1bb37 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -123,6 +123,7 @@ module Types mount_mutation Mutations::Packages::Destroy mount_mutation Mutations::Packages::DestroyFile mount_mutation Mutations::Echo + mount_mutation Mutations::WorkItems::Create, feature_flag: :work_items end end diff --git a/app/graphql/types/work_item_type.rb b/app/graphql/types/work_item_type.rb new file mode 100644 index 00000000000..486c1e52987 --- /dev/null +++ b/app/graphql/types/work_item_type.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Types + class WorkItemType < BaseObject + graphql_name 'WorkItem' + + authorize :read_issue + + field :description, GraphQL::Types::String, null: true, + description: 'Description of the work item.' + field :id, Types::GlobalIDType[::WorkItem], null: false, + description: 'Global ID of the work item.' + field :iid, GraphQL::Types::ID, null: false, + description: 'Internal ID of the work item.' + field :title, GraphQL::Types::String, null: false, + description: 'Title of the work item.' + field :work_item_type, Types::WorkItems::TypeType, null: false, + description: 'Type assigned to the work item.' + + markdown_field :title_html, null: true + markdown_field :description_html, null: true + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index 01ed55cc668..6caf6f4b47e 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -603,6 +603,11 @@ class Issue < ApplicationRecord author&.banned? end + # Necessary until all issues are backfilled and we add a NOT NULL constraint on the DB + def work_item_type + super || WorkItems::Type.default_by_type(issue_type) + end + private def spammable_attribute_changed? diff --git a/app/models/work_item.rb b/app/models/work_item.rb new file mode 100644 index 00000000000..02f52f04c85 --- /dev/null +++ b/app/models/work_item.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class WorkItem < Issue + self.table_name = 'issues' + self.inheritance_column = :_type_disabled +end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index ce00acf6962..55f43cd9f7b 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -258,6 +258,11 @@ class ProjectPolicy < BasePolicy rule { can?(:reporter_access) & can?(:create_issue) }.enable :create_incident + rule { can?(:guest_access) & can?(:create_issue) }.policy do + enable :create_task + enable :create_work_item + end + # These abilities are not allowed to admins that are not members of the project, # that's why they are defined separately. rule { guest & can?(:download_code) }.enable :build_download_code diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb index 8fd844c4886..1ebf9bb6ba2 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -7,7 +7,7 @@ module Issues def execute filter_resolve_discussion_params - @issue = project.issues.new(issue_params).tap do |issue| + @issue = model_klass.new(issue_params.merge(project: project)).tap do |issue| ensure_milestone_available(issue) end end @@ -62,16 +62,25 @@ module Issues def issue_params @issue_params ||= build_issue_params - # If :issue_type is nil then params[:issue_type] was either nil - # or not permitted. Either way, the :issue_type will default - # to the column default of `issue`. And that means we need to - # ensure the work_item_type_id is set - @issue_params[:work_item_type_id] = get_work_item_type_id(@issue_params[:issue_type]) + if @issue_params[:work_item_type].present? + @issue_params[:issue_type] = @issue_params[:work_item_type].base_type + else + # If :issue_type is nil then params[:issue_type] was either nil + # or not permitted. Either way, the :issue_type will default + # to the column default of `issue`. And that means we need to + # ensure the work_item_type_id is set + @issue_params[:work_item_type_id] = get_work_item_type_id(@issue_params[:issue_type]) + end + @issue_params end private + def model_klass + ::Issue + end + def allowed_issue_params allowed_params = [ :title, @@ -79,8 +88,11 @@ module Issues :confidential ] + params[:work_item_type] = WorkItems::Type.find_by(id: params[:work_item_type_id]) if params[:work_item_type_id].present? # rubocop: disable CodeReuse/ActiveRecord + allowed_params << :milestone_id if can?(current_user, :admin_issue, project) allowed_params << :issue_type if create_issue_type_allowed?(project, params[:issue_type]) + allowed_params << :work_item_type if create_issue_type_allowed?(project, params[:work_item_type]&.base_type) params.slice(*allowed_params) end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 79b59eee5e1..e29bcf4a453 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -12,13 +12,14 @@ module Issues # spam_checking is likely to be necessary. However, if there is not a request available in scope # in the caller (for example, an issue created via email) and the required arguments to the # SpamParams constructor are not otherwise available, spam_params: must be explicitly passed as nil. - def initialize(project:, current_user: nil, params: {}, spam_params:) + def initialize(project:, current_user: nil, params: {}, spam_params:, build_service: nil) super(project: project, current_user: current_user, params: params) @spam_params = spam_params + @build_service = build_service || BuildService.new(project: project, current_user: current_user, params: params) end def execute(skip_system_notes: false) - @issue = BuildService.new(project: project, current_user: current_user, params: params).execute + @issue = @build_service.execute filter_resolve_discussion_params diff --git a/app/services/work_items/build_service.rb b/app/services/work_items/build_service.rb new file mode 100644 index 00000000000..15a26f81d7c --- /dev/null +++ b/app/services/work_items/build_service.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module WorkItems + class BuildService < ::Issues::BuildService + private + + def model_klass + ::WorkItem + end + end +end diff --git a/app/services/work_items/create_service.rb b/app/services/work_items/create_service.rb new file mode 100644 index 00000000000..49f7b89158b --- /dev/null +++ b/app/services/work_items/create_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module WorkItems + class CreateService + def initialize(project:, current_user: nil, params: {}, spam_params:) + @create_service = ::Issues::CreateService.new( + project: project, + current_user: current_user, + params: params, + spam_params: spam_params, + build_service: ::WorkItems::BuildService.new(project: project, current_user: current_user, params: params) + ) + end + + def execute + @create_service.execute + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 579b37c7f2d..c09afd852bc 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -4951,6 +4951,30 @@ Input type: `VulnerabilityRevertToDetectedInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `vulnerability` | [`Vulnerability`](#vulnerability) | Vulnerability after revert. | +### `Mutation.workItemCreate` + +Available only when feature flag `work_items` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice. + +Input type: `WorkItemCreateInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `description` | [`String`](#string) | Description of the work item. | +| `projectPath` | [`ID!`](#id) | Full path of the project the work item is associated with. | +| `title` | [`String!`](#string) | Title of the work item. | +| `workItemTypeId` | [`WorkItemsTypeID!`](#workitemstypeid) | Global ID of a work item type. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `workItem` | [`WorkItem`](#workitem) | Created work item. | + ## Connections Some types in our schema are `Connection` types - they represent a paginated @@ -16019,6 +16043,20 @@ Represents vulnerability letter grades with associated projects. | `grade` | [`VulnerabilityGrade!`](#vulnerabilitygrade) | Grade based on the highest severity vulnerability present. | | `projects` | [`ProjectConnection!`](#projectconnection) | Projects within this grade. (see [Connections](#connections)) | +### `WorkItem` + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `description` | [`String`](#string) | Description of the work item. | +| `descriptionHtml` | [`String`](#string) | The GitLab Flavored Markdown rendering of `description`. | +| `id` | [`WorkItemID!`](#workitemid) | Global ID of the work item. | +| `iid` | [`ID!`](#id) | Internal ID of the work item. | +| `title` | [`String!`](#string) | Title of the work item. | +| `titleHtml` | [`String`](#string) | The GitLab Flavored Markdown rendering of `title`. | +| `workItemType` | [`WorkItemType!`](#workitemtype) | Type assigned to the work item. | + ### `WorkItemType` #### Fields @@ -18121,6 +18159,12 @@ A `VulnerabilityID` is a global ID. It is encoded as a string. An example `VulnerabilityID` is: `"gid://gitlab/Vulnerability/1"`. +### `WorkItemID` + +A `WorkItemID` is a global ID. It is encoded as a string. + +An example `WorkItemID` is: `"gid://gitlab/WorkItem/1"`. + ### `WorkItemsTypeID` A `WorkItemsTypeID` is a global ID. It is encoded as a string. diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 8f290ed7215..b49b86dc12a 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -238,6 +238,17 @@ RSpec.describe Issue do end end + # TODO: Remove when NOT NULL constraint is added to the relationship + describe '#work_item_type' do + let(:issue) { create(:issue, :incident, project: reusable_project, work_item_type: nil) } + + it 'returns a default type if the legacy issue does not have a work item type associated yet' do + expect(issue.work_item_type_id).to be_nil + expect(issue.issue_type).to eq('incident') + expect(issue.work_item_type).to eq(WorkItems::Type.default_by_type(:incident)) + end + end + describe '#sort' do let(:project) { reusable_project } diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 2953c198af6..38e4e18c894 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -61,7 +61,7 @@ RSpec.describe ProjectPolicy do end it 'does not include the issues permissions' do - expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident + expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident, :create_work_item, :create_task end it 'disables boards and lists permissions' do @@ -73,7 +73,7 @@ RSpec.describe ProjectPolicy do it 'does not include the issues permissions' do create(:jira_integration, project: project) - expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident + expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident, :create_work_item, :create_task end end end diff --git a/spec/requests/api/graphql/mutations/work_items/create_spec.rb b/spec/requests/api/graphql/mutations/work_items/create_spec.rb new file mode 100644 index 00000000000..e7a0c7753fb --- /dev/null +++ b/spec/requests/api/graphql/mutations/work_items/create_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Create a work item' do + include GraphqlHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user).tap { |user| project.add_developer(user) } } + + let(:input) do + { + 'title' => 'new title', + 'description' => 'new description', + 'workItemTypeId' => WorkItems::Type.default_by_type(:task).to_global_id.to_s + } + end + + let(:mutation) { graphql_mutation(:workItemCreate, input.merge('projectPath' => project.full_path)) } + + let(:mutation_response) { graphql_mutation_response(:work_item_create) } + + context 'the user is not allowed to create a work item' do + let(:current_user) { create(:user) } + + it_behaves_like 'a mutation that returns a top-level access error' + end + + context 'when user has permissions to create a work item' do + let(:current_user) { developer } + + it 'creates the work item' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.to change(WorkItem, :count).by(1) + + created_work_item = WorkItem.last + + expect(response).to have_gitlab_http_status(:success) + expect(created_work_item.issue_type).to eq('task') + expect(created_work_item.work_item_type.base_type).to eq('task') + expect(mutation_response['workItem']).to include( + input.except('workItemTypeId').merge( + 'id' => created_work_item.to_global_id.to_s, + 'workItemType' => hash_including('name' => 'Task') + ) + ) + end + + it_behaves_like 'has spam protection' do + let(:mutation_class) { ::Mutations::WorkItems::Create } + end + + context 'when the work_items feature flag is disabled' do + before do + stub_feature_flags(work_items: false) + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: ["Field 'workItemCreate' doesn't exist on type 'Mutation'", "Variable $workItemCreateInput is declared by anonymous mutation but not used"] + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 8496bd31e00..732900a53d3 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -61,6 +61,7 @@ RSpec.describe Issues::CreateService do expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) expect(issue).to be_persisted + expect(issue).to be_a(::Issue) expect(issue.title).to eq('Awesome issue') expect(issue.assignees).to eq([assignee]) expect(issue.labels).to match_array(labels) @@ -69,6 +70,18 @@ RSpec.describe Issues::CreateService do expect(issue.work_item_type.base_type).to eq('issue') end + context 'when a build_service is provided' do + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params, build_service: build_service).execute } + + let(:issue_from_builder) { WorkItem.new(project: project, title: 'Issue from builder') } + let(:build_service) { double(:build_service, execute: issue_from_builder) } + + it 'uses the provided service to build the issue' do + expect(issue).to be_persisted + expect(issue).to be_a(WorkItem) + end + end + context 'when skip_system_notes is true' do let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute(skip_system_notes: true) } diff --git a/spec/services/work_items/build_service_spec.rb b/spec/services/work_items/build_service_spec.rb new file mode 100644 index 00000000000..6b2e2d8819e --- /dev/null +++ b/spec/services/work_items/build_service_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::BuildService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:guest) { create(:user) } + + let(:user) { guest } + + before_all do + project.add_guest(guest) + end + + describe '#execute' do + subject { described_class.new(project: project, current_user: user, params: {}).execute } + + it { is_expected.to be_a(::WorkItem) } + end +end diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb new file mode 100644 index 00000000000..2c054ae59a0 --- /dev/null +++ b/spec/services/work_items/create_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::CreateService do + include AfterNextHelpers + + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:project) { create(:project, group: group) } + let_it_be(:user) { create(:user) } + + let(:spam_params) { double } + + describe '#execute' do + let(:work_item) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + + before do + stub_spam_services + end + + context 'when params are valid' do + before_all do + project.add_guest(user) + end + + let(:opts) do + { + title: 'Awesome work_item', + description: 'please fix' + } + end + + it 'created instance is a WorkItem' do + expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) + + expect(work_item).to be_persisted + expect(work_item).to be_a(::WorkItem) + expect(work_item.title).to eq('Awesome work_item') + expect(work_item.description).to eq('please fix') + expect(work_item.work_item_type.base_type).to eq('issue') + end + end + + context 'checking spam' do + let(:params) do + { + title: 'Spam work_item' + } + end + + subject do + described_class.new(project: project, current_user: user, params: params, spam_params: spam_params) + end + + it 'executes SpamActionService' do + expect_next_instance_of( + Spam::SpamActionService, + { + spammable: kind_of(WorkItem), + spam_params: spam_params, + user: an_instance_of(User), + action: :create + } + ) do |instance| + expect(instance).to receive(:execute) + end + + subject.execute + end + end + end +end diff --git a/spec/support/shared_examples/graphql/mutation_shared_examples.rb b/spec/support/shared_examples/graphql/mutation_shared_examples.rb index 51d52cbb901..dc590e23ace 100644 --- a/spec/support/shared_examples/graphql/mutation_shared_examples.rb +++ b/spec/support/shared_examples/graphql/mutation_shared_examples.rb @@ -8,7 +8,7 @@ # There must be a method or let called `mutation` defined that executes # the mutation. RSpec.shared_examples 'a mutation that returns top-level errors' do |errors: []| - let(:match_errors) { eq(errors) } + let(:match_errors) { match_array(errors) } it do post_graphql_mutation(mutation, current_user: current_user)