From d68be8df062a2d98c5a4e1681fb2a6b13f03ea62 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Tue, 13 Dec 2022 10:01:33 +0000
Subject: [PATCH] Fix setting of work item type
* Work item type is set in BuildService when creating an issue, we
should avoid setting the type from params in later phase.
* allows creation of key results
* add okrs licence check
---
app/services/issues/base_service.rb | 5 ++
ee/app/policies/ee/project_policy.rb | 12 ++-
ee/spec/policies/project_policy_spec.rb | 15 +++-
.../mutations/work_items/create_spec.rb | 73 ++++++++++++++++++-
.../api/graphql/project/work_items_spec.rb | 2 +-
spec/factories/work_items/work_item_types.rb | 2 +-
.../mutations/work_items/update_spec.rb | 4 +-
spec/services/issues/create_service_spec.rb | 24 +++++-
8 files changed, 123 insertions(+), 14 deletions(-)
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 28ea6b0ebf82f..10407e997154a 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -114,6 +114,11 @@ def delete_milestone_total_issue_counter_cache(milestone)
Milestones::IssuesCountService.new(milestone).delete_cache
end
+
+ override :allowed_create_params
+ def allowed_create_params(params)
+ super(params).except(:issue_type, :work_item_type_id, :work_item_type)
+ end
end
end
diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb
index 05b49f42eeb7b..0217061b0dded 100644
--- a/ee/app/policies/ee/project_policy.rb
+++ b/ee/app/policies/ee/project_policy.rb
@@ -162,6 +162,11 @@ module ProjectPolicy
@subject.group && (@subject.group.membership_lock? || ::Gitlab::CurrentSettings.lock_memberships_to_ldap?)
end
+ with_scope :subject
+ condition(:okrs_enabled) do
+ @subject.okrs_mvc_feature_flag_enabled? && @subject.feature_available?(:okrs)
+ end
+
rule { membership_locked_via_parent_group }.policy do
prevent :import_project_members_from_another_project
end
@@ -186,8 +191,6 @@ module ProjectPolicy
@user.read_code_for?(project)
end
- condition(:okrs_enabled, scope: :subject) { project&.okrs_mvc_feature_flag_enabled? }
-
# Owners can be banned from their own project except for top-level group
# owners. This exception is made at the service layer
# (Users::Abuse::GitAbuse::NamespaceThrottleService) where the ban record
@@ -534,7 +537,10 @@ module ProjectPolicy
rule { custom_roles_allowed & role_enables_read_code }.enable :read_code
- rule { can?(:create_issue) & okrs_enabled }.enable :create_objective
+ rule { can?(:create_issue) & okrs_enabled }.policy do
+ enable :create_objective
+ enable :create_key_result
+ end
end
override :lookup_access_level!
diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb
index d991bb724105d..ad0555fded125 100644
--- a/ee/spec/policies/project_policy_spec.rb
+++ b/ee/spec/policies/project_policy_spec.rb
@@ -2425,7 +2425,7 @@ def expect_private_project_permissions_as_if_non_member
describe 'create_objective' do
using RSpec::Parameterized::TableSyntax
- let(:policy) { :create_objective }
+ let(:okr_policies) { [:create_objective, :create_key_result] }
where(:role, :allowed) do
:guest | true
@@ -2442,10 +2442,11 @@ def expect_private_project_permissions_as_if_non_member
before do
enable_admin_mode!(current_user) if role == :admin
+ stub_licensed_features(okrs: true)
end
context 'when okrs_mvc feature flag is enabled' do
- it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
+ it { is_expected.to(allowed ? be_allowed(*okr_policies) : be_disallowed(*okr_policies)) }
end
context 'when okrs_mvc feature flag is disabled' do
@@ -2453,7 +2454,15 @@ def expect_private_project_permissions_as_if_non_member
stub_feature_flags(okrs_mvc: false)
end
- it { is_expected.to(be_disallowed(policy)) }
+ it { is_expected.to be_disallowed(*okr_policies) }
+ end
+
+ context 'when okrs license feature is not available' do
+ before do
+ stub_licensed_features(okrs: false)
+ end
+
+ it { is_expected.to be_disallowed(*okr_policies) }
end
end
end
diff --git a/ee/spec/requests/api/graphql/mutations/work_items/create_spec.rb b/ee/spec/requests/api/graphql/mutations/work_items/create_spec.rb
index c522178891619..f2527e9de136a 100644
--- a/ee/spec/requests/api/graphql/mutations/work_items/create_spec.rb
+++ b/ee/spec/requests/api/graphql/mutations/work_items/create_spec.rb
@@ -11,12 +11,12 @@
let(:mutation) { graphql_mutation(:workItemCreate, input.merge('projectPath' => project.full_path)) }
let(:mutation_response) { graphql_mutation_response(:work_item_create) }
+ let(:widgets_response) { mutation_response['workItem']['widgets'] }
context 'when user has permissions to create a work item' do
let(:current_user) { developer }
context 'with iteration widget input' do
- let(:widgets_response) { mutation_response['workItem']['widgets'] }
let(:fields) do
<<~FIELDS
workItem {
@@ -85,5 +85,76 @@
end
end
end
+
+ context 'when creating a key result' do
+ let_it_be(:parent) { create(:work_item, :objective, project: project) }
+
+ let(:fields) do
+ <<~FIELDS
+ workItem {
+ id
+ workItemType {
+ id
+ }
+ widgets {
+ type
+ ... on WorkItemWidgetHierarchy {
+ parent {
+ id
+ }
+ }
+ }
+ }
+ errors
+ FIELDS
+ end
+
+ let(:input) do
+ {
+ title: 'key result',
+ workItemTypeId: WorkItems::Type.default_by_type(:key_result).to_global_id.to_s,
+ hierarchyWidget: { 'parentId' => parent.to_global_id.to_s }
+ }
+ end
+
+ let(:mutation) { graphql_mutation(:workItemCreate, input.merge('projectPath' => project.full_path), fields) }
+ let(:widgets_response) { mutation_response['workItem']['widgets'] }
+
+ context 'when okrs are available' do
+ before do
+ stub_licensed_features(okrs: true)
+ end
+
+ it 'creates the work item' do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.to change { WorkItem.count }.by(1)
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(widgets_response).to include(
+ {
+ 'parent' => { 'id' => parent.to_global_id.to_s },
+ 'type' => 'HIERARCHY'
+ }
+ )
+ end
+ end
+
+ context 'when okrs are not available' do
+ before do
+ stub_licensed_features(okrs: false)
+ end
+
+ it 'returns error' do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.to not_change(WorkItem, :count)
+
+ expect(mutation_response['errors'])
+ .to contain_exactly(/cannot be added: is not allowed to add this type of parent/)
+ expect(mutation_response['workItem']).to be_nil
+ end
+ end
+ end
end
end
diff --git a/ee/spec/requests/api/graphql/project/work_items_spec.rb b/ee/spec/requests/api/graphql/project/work_items_spec.rb
index cfc9923ae123f..cd3354bb55293 100644
--- a/ee/spec/requests/api/graphql/project/work_items_spec.rb
+++ b/ee/spec/requests/api/graphql/project/work_items_spec.rb
@@ -56,7 +56,7 @@
end
before do
- stub_licensed_features(requirements: true)
+ stub_licensed_features(requirements: true, okrs: true)
end
it 'returns work items including status', :aggregate_failures do
diff --git a/spec/factories/work_items/work_item_types.rb b/spec/factories/work_items/work_item_types.rb
index 1b6137503d3a9..d36cb6260c623 100644
--- a/spec/factories/work_items/work_item_types.rb
+++ b/spec/factories/work_items/work_item_types.rb
@@ -13,7 +13,7 @@
# Expect base_types to exist on the DB
if type_base_attributes.slice(:namespace, :namespace_id).compact.empty?
- WorkItems::Type.find_or_initialize_by(type_base_attributes).tap { |type| type.assign_attributes(attributes) }
+ WorkItems::Type.find_or_initialize_by(type_base_attributes)
else
WorkItems::Type.new(attributes)
end
diff --git a/spec/requests/api/graphql/mutations/work_items/update_spec.rb b/spec/requests/api/graphql/mutations/work_items/update_spec.rb
index c71c2ae6c39b3..14cb18d04b85e 100644
--- a/spec/requests/api/graphql/mutations/work_items/update_spec.rb
+++ b/spec/requests/api/graphql/mutations/work_items/update_spec.rb
@@ -632,7 +632,7 @@
end
context 'when unsupported widget input is sent' do
- let_it_be(:test_case) { create(:work_item_type, :default, :test_case, name: 'some_test_case_name') }
+ let_it_be(:test_case) { create(:work_item_type, :default, :test_case) }
let_it_be(:work_item) { create(:work_item, work_item_type: test_case, project: project) }
let(:input) do
@@ -642,7 +642,7 @@
end
it_behaves_like 'a mutation that returns top-level errors',
- errors: ["Following widget keys are not supported by some_test_case_name type: [:hierarchy_widget]"]
+ errors: ["Following widget keys are not supported by Test Case type: [:hierarchy_widget]"]
end
end
end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 7b51f9db8cd6f..7ab2046b6be46 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -9,21 +9,22 @@
let_it_be_with_reload(:project) { create(:project, :public, group: group) }
let_it_be(:user) { create(:user) }
+ let(:opts) { { title: 'title' } }
let(:spam_params) { double }
+ let(:service) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params) }
it_behaves_like 'rate limited service' do
let(:key) { :issues_create }
let(:key_scope) { %i[project current_user external_author] }
let(:application_limit_key) { :issues_create_limit }
let(:created_model) { Issue }
- let(:service) { described_class.new(project: project, current_user: user, params: { title: 'title' }, spam_params: double) }
end
describe '#execute' do
let_it_be(:assignee) { create(:user) }
let_it_be(:milestone) { create(:milestone, project: project) }
- let(:result) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute }
+ let(:result) { service.execute }
let(:issue) { result[:issue] }
before do
@@ -54,6 +55,7 @@
let(:opts) do
{ title: 'Awesome issue',
+ issue_type: :task,
description: 'please fix',
assignee_ids: [assignee.id],
label_ids: labels.map(&:id),
@@ -118,10 +120,26 @@
expect(issue.labels).to match_array(labels)
expect(issue.milestone).to eq(milestone)
expect(issue.due_date).to eq(Date.tomorrow)
- expect(issue.work_item_type.base_type).to eq('issue')
+ expect(issue.work_item_type.base_type).to eq('task')
expect(issue.issue_customer_relations_contacts).to be_empty
end
+ context 'when the work item type is not allowed to create' do
+ before do
+ allow_next_instance_of(::Issues::BuildService) do |instance|
+ allow(instance).to receive(:create_issue_type_allowed?).twice.and_return(false)
+ end
+ end
+
+ it 'ignores the type and creates default issue' do
+ expect(result).to be_success
+ expect(issue).to be_persisted
+ expect(issue).to be_a(::Issue)
+ expect(issue.work_item_type.base_type).to eq('issue')
+ expect(issue.issue_type).to eq('issue')
+ end
+ end
+
it 'calls NewIssueWorker with correct arguments' do
expect(NewIssueWorker).to receive(:perform_async).with(Integer, user.id, 'Issue')
--
GitLab