From e506244af48e9a6c4c9c34b2779eec7782673c04 Mon Sep 17 00:00:00 2001 From: Abhilash Kotte Date: Tue, 8 Nov 2022 19:20:53 +0530 Subject: [PATCH] Support creation of Objective via work-items Add OKRs as ultimate feature, create objectives using createWorkItem API, and include Objectives in Issue list. The entire work is under an ee-only feature flag okrs_mvc Issue: https://gitlab.com/gitlab-org/incubation-engineering/okr/meta/-/issues/9 Changelog: added MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/10335 EE: true --- app/graphql/types/issue_type_enum.rb | 4 +++ app/helpers/routing/projects_helper.rb | 2 ++ app/models/issue.rb | 2 +- app/models/work_items/type.rb | 2 +- doc/api/graphql/reference/index.md | 1 + .../ee/projects/issues_controller.rb | 5 +++ ee/app/helpers/ee/issues_helper.rb | 3 +- ee/app/helpers/ee/routing/projects_helper.rb | 15 ++++++++ ee/app/models/ee/project.rb | 4 +++ .../models/gitlab_subscriptions/features.rb | 1 + ee/app/policies/ee/project_policy.rb | 4 +++ .../feature_flags/development/okrs_mvc.yml | 8 +++++ ee/spec/helpers/ee/issues_helper_spec.rb | 18 ++++++---- ee/spec/lib/ee/gitlab/url_builder_spec.rb | 26 +++++++++++++- ee/spec/models/project_spec.rb | 14 ++++++++ ee/spec/policies/project_policy_spec.rb | 36 +++++++++++++++++++ spec/graphql/types/issue_type_enum_spec.rb | 4 +-- 17 files changed, 137 insertions(+), 12 deletions(-) create mode 100644 ee/app/helpers/ee/routing/projects_helper.rb create mode 100644 ee/config/feature_flags/development/okrs_mvc.yml diff --git a/app/graphql/types/issue_type_enum.rb b/app/graphql/types/issue_type_enum.rb index 80f59e4708b653..78cd27f60c3a7d 100644 --- a/app/graphql/types/issue_type_enum.rb +++ b/app/graphql/types/issue_type_enum.rb @@ -12,5 +12,9 @@ class IssueTypeEnum < BaseEnum value 'TASK', value: 'task', description: 'Task issue type.', alpha: { milestone: '15.2' } + + value 'OBJECTIVE', value: 'objective', + description: 'Objective issue type. Available only when feature flag `okrs_mvc` is enabled.', + alpha: { milestone: '15.6' } end end diff --git a/app/helpers/routing/projects_helper.rb b/app/helpers/routing/projects_helper.rb index 60bf205f3f3d62..f4732e398f0041 100644 --- a/app/helpers/routing/projects_helper.rb +++ b/app/helpers/routing/projects_helper.rb @@ -100,3 +100,5 @@ def use_work_items_path?(issue) end end end + +Routing::ProjectsHelper.prepend_mod diff --git a/app/models/issue.rb b/app/models/issue.rb index 5a078e13fb2b32..fc083002c41961 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -46,7 +46,7 @@ class Issue < ApplicationRecord # # This should be kept consistent with the enums used for the GraphQL issue list query in # https://gitlab.com/gitlab-org/gitlab/-/blob/1379c2d7bffe2a8d809f23ac5ef9b4114f789c07/app/assets/javascripts/issues/list/constants.js#L154-158 - TYPES_FOR_LIST = %w(issue incident test_case task).freeze + TYPES_FOR_LIST = %w(issue incident test_case task objective).freeze # Types of issues that should be displayed on issue board lists TYPES_FOR_BOARD_LIST = %w(issue incident).freeze diff --git a/app/models/work_items/type.rb b/app/models/work_items/type.rb index f5fea356831c61..dc30899d24fb6c 100644 --- a/app/models/work_items/type.rb +++ b/app/models/work_items/type.rb @@ -31,7 +31,7 @@ class Type < ApplicationRecord requirement: [Widgets::Description], task: [Widgets::Assignees, Widgets::Labels, Widgets::Description, Widgets::Hierarchy, Widgets::StartAndDueDate, Widgets::Milestone], - objective: [Widgets::Assignees, Widgets::Labels, Widgets::Description, Widgets::Hierarchy], + objective: [Widgets::Assignees, Widgets::Labels, Widgets::Description, Widgets::Hierarchy, Widgets::Milestone], key_result: [Widgets::Assignees, Widgets::Labels, Widgets::Description, Widgets::StartAndDueDate] }.freeze diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index bd0704175b7866..f8018d05d3e8b6 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -21406,6 +21406,7 @@ Issue type. | ----- | ----------- | | `INCIDENT` | Incident issue type. | | `ISSUE` | Issue issue type. | +| `OBJECTIVE` **{warning-solid}** | **Introduced** in 15.6. This feature is in Alpha. It can be changed or removed at any time. Objective issue type. Available only when feature flag `okrs_mvc` is enabled. | | `REQUIREMENT` | Requirement issue type. | | `TASK` **{warning-solid}** | **Introduced** in 15.2. This feature is in Alpha. It can be changed or removed at any time. Task issue type. | | `TEST_CASE` | Test Case issue type. | diff --git a/ee/app/controllers/ee/projects/issues_controller.rb b/ee/app/controllers/ee/projects/issues_controller.rb index 847eb264a32296..270ad2a554894e 100644 --- a/ee/app/controllers/ee/projects/issues_controller.rb +++ b/ee/app/controllers/ee/projects/issues_controller.rb @@ -22,6 +22,11 @@ module IssuesController before_action :redirect_if_test_case, only: [:show] + before_action only: :index do + push_force_frontend_feature_flag(:okrs_mvc, project&.okrs_mvc_feature_flag_enabled?) + push_licensed_feature(:okrs, project) + end + before_action only: %i[show index] do @seat_count_data = generate_seat_count_alert_data(@project) end diff --git a/ee/app/helpers/ee/issues_helper.rb b/ee/app/helpers/ee/issues_helper.rb index 8bec52064d861c..617eb5d8237353 100644 --- a/ee/app/helpers/ee/issues_helper.rb +++ b/ee/app/helpers/ee/issues_helper.rb @@ -48,7 +48,8 @@ def common_issues_list_data(namespace, current_user) has_issuable_health_status_feature: namespace.feature_available?(:issuable_health_status).to_s, has_issue_weights_feature: namespace.feature_available?(:issue_weights).to_s, has_iterations_feature: namespace.feature_available?(:iterations).to_s, - has_scoped_labels_feature: namespace.feature_available?(:scoped_labels).to_s + has_scoped_labels_feature: namespace.feature_available?(:scoped_labels).to_s, + has_okrs_feature: namespace.feature_available?(:okrs).to_s ) end diff --git a/ee/app/helpers/ee/routing/projects_helper.rb b/ee/app/helpers/ee/routing/projects_helper.rb new file mode 100644 index 00000000000000..f673e367c71a94 --- /dev/null +++ b/ee/app/helpers/ee/routing/projects_helper.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true +module EE + module Routing + module ProjectsHelper + extend ::Gitlab::Utils::Override + + private + + override :use_work_items_path? + def use_work_items_path?(issue) + super || (issue.issue_type == 'objective' && issue.project.okrs_mvc_feature_flag_enabled?) + end + end + end +end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index b07beb9c0ffffe..41a158ea4da951 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -368,6 +368,10 @@ def can_store_security_reports? namespace.store_security_reports_available? || public? end + def okrs_mvc_feature_flag_enabled? + ::Feature.enabled?(:okrs_mvc, self) + end + def latest_ingested_security_pipeline vulnerability_statistic&.pipeline end diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb index 2d4353d4cb3e47..18324ee8272a64 100644 --- a/ee/app/models/gitlab_subscriptions/features.rb +++ b/ee/app/models/gitlab_subscriptions/features.rb @@ -194,6 +194,7 @@ class Features jira_issue_association_enforcement kubernetes_cluster_vulnerabilities license_scanning + okrs personal_access_token_expiration_policy product_analytics project_quality_summary diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index e856f7746e9757..cbbee3fab8e42e 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -184,6 +184,8 @@ module ProjectPolicy @user.download_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 @@ -532,6 +534,8 @@ module ProjectPolicy end rule { custom_roles_allowed & role_enables_download_code }.enable :download_code + + rule { can?(:create_issue) & okrs_enabled }.enable :create_objective end override :lookup_access_level! diff --git a/ee/config/feature_flags/development/okrs_mvc.yml b/ee/config/feature_flags/development/okrs_mvc.yml new file mode 100644 index 00000000000000..13300fc5a3e4c0 --- /dev/null +++ b/ee/config/feature_flags/development/okrs_mvc.yml @@ -0,0 +1,8 @@ +--- +name: okrs_mvc +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102721 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/382070 +milestone: '15.6' +type: development +group: group::incubation +default_enabled: false diff --git a/ee/spec/helpers/ee/issues_helper_spec.rb b/ee/spec/helpers/ee/issues_helper_spec.rb index 37f098b3c84de0..851afa6ecfe0d2 100644 --- a/ee/spec/helpers/ee/issues_helper_spec.rb +++ b/ee/spec/helpers/ee/issues_helper_spec.rb @@ -143,7 +143,8 @@ issue_weights: true, iterations: true, multiple_issue_assignees: true, - scoped_labels: true + scoped_labels: true, + okrs: true ) end @@ -154,6 +155,7 @@ has_issue_weights_feature: 'true', has_iterations_feature: 'true', has_scoped_labels_feature: 'true', + has_okrs_feature: 'true', group_path: project.group.full_path } @@ -171,7 +173,7 @@ context 'when features are disabled' do before do - stub_licensed_features(epics: false, iterations: false, issue_weights: false, issuable_health_status: false, blocked_issues: false, multiple_issue_assignees: false) + stub_licensed_features(epics: false, iterations: false, issue_weights: false, issuable_health_status: false, blocked_issues: false, multiple_issue_assignees: false, okrs: false) end it 'returns data with licensed features disabled' do @@ -180,7 +182,8 @@ has_issuable_health_status_feature: 'false', has_issue_weights_feature: 'false', has_iterations_feature: 'false', - has_scoped_labels_feature: 'false' + has_scoped_labels_feature: 'false', + has_okrs_feature: 'false' } result = helper.project_issues_list_data(project, current_user) @@ -210,7 +213,8 @@ issue_weights: true, iterations: true, multiple_issue_assignees: true, - scoped_labels: true + scoped_labels: true, + okrs: true ) end @@ -222,6 +226,7 @@ has_issue_weights_feature: 'true', has_iterations_feature: 'true', has_scoped_labels_feature: 'true', + has_okrs_feature: 'true', group_path: project.group.full_path } @@ -231,7 +236,7 @@ context 'when features are disabled' do before do - stub_licensed_features(blocked_issues: false, epics: false, group_bulk_edit: false, issuable_health_status: false, issue_weights: false, iterations: false, multiple_issue_assignees: false) + stub_licensed_features(blocked_issues: false, epics: false, group_bulk_edit: false, issuable_health_status: false, issue_weights: false, iterations: false, multiple_issue_assignees: false, okrs: false) end it 'returns data with licensed features disabled' do @@ -241,7 +246,8 @@ has_issuable_health_status_feature: 'false', has_issue_weights_feature: 'false', has_iterations_feature: 'false', - has_scoped_labels_feature: 'false' + has_scoped_labels_feature: 'false', + has_okrs_feature: 'false' } result = helper.group_issues_list_data(group, current_user) diff --git a/ee/spec/lib/ee/gitlab/url_builder_spec.rb b/ee/spec/lib/ee/gitlab/url_builder_spec.rb index 263e6fcb22ca79..41e42eb7e55de3 100644 --- a/ee/spec/lib/ee/gitlab/url_builder_spec.rb +++ b/ee/spec/lib/ee/gitlab/url_builder_spec.rb @@ -17,10 +17,12 @@ :note_on_vulnerability | ->(note) { "/#{note.project.full_path}/-/security/vulnerabilities/#{note.noteable.id}#note_#{note.id}" } :group_wiki | ->(wiki) { "/groups/#{wiki.container.full_path}/-/wikis/home" } + + [:issue, :objective] | ->(issue) { "/#{issue.project.full_path}/-/work_items/#{issue.iid}?iid_path=true" } end with_them do - let(:object) { build_stubbed(factory) } + let(:object) { build_stubbed(*Array(factory)) } let(:path) { path_generator.call(object) } it 'returns the full URL' do @@ -31,5 +33,27 @@ expect(subject.build(object, only_path: true)).to eq(path) end end + + context 'when use_iid_in_work_items_path feature flag is disabled' do + before do + stub_feature_flags(use_iid_in_work_items_path: false) + end + + context 'when a objective issue is passed' do + it 'returns a path using the work item\'s ID and no query params' do + objective = create(:issue, :objective) + + expect(subject.build(objective, only_path: true)).to eq("/#{objective.project.full_path}/-/work_items/#{objective.id}") + end + end + + context 'when a work item is passed' do + it 'returns a path using the work item\'s ID and no query params' do + work_item = create(:work_item) + + expect(subject.build(work_item, only_path: true)).to eq("/#{work_item.project.full_path}/-/work_items/#{work_item.id}") + end + end + end end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 21d84bd1e42b20..ed7c2e6d04531a 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -3632,4 +3632,18 @@ def stub_default_url_options(host) it { is_expected.to be_falsey } end end + + describe '#okrs_mvc_feature_flag_enabled?' do + let_it_be(:project) { create(:project) } + + it 'returns true if feature_flag is enabled' do + stub_feature_flags(okrs_mvc: true) + expect(project.okrs_mvc_feature_flag_enabled?).to be_truthy + end + + it 'returns false if feature_flag is disabled' do + stub_feature_flags(okrs_mvc: false) + expect(project.okrs_mvc_feature_flag_enabled?).to be_falsey + end + end end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 0a94c6ce08b32e..5b3b815c5ace21 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2422,6 +2422,42 @@ def expect_private_project_permissions_as_if_non_member end end + describe 'create_objective' do + using RSpec::Parameterized::TableSyntax + + let(:policy) { :create_objective } + + where(:role, :allowed) do + :guest | true + :reporter | true + :developer | true + :maintainer | true + :auditor | false + :owner | true + :admin | true + end + + with_them do + let(:current_user) { public_send(role) } + + before do + enable_admin_mode!(current_user) if role == :admin + end + + context 'when okrs_mvc feature flag is enabled' do + it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) } + end + + context 'when okrs_mvc feature flag is disabled' do + before do + stub_feature_flags(okrs_mvc: false) + end + + it { is_expected.to(be_disallowed(policy)) } + end + end + end + context 'hidden projects' do let(:project) { create(:project, :repository, hidden: true) } let(:current_user) { create(:user) } diff --git a/spec/graphql/types/issue_type_enum_spec.rb b/spec/graphql/types/issue_type_enum_spec.rb index d462c26c6ac20d..cd1737c3ebb958 100644 --- a/spec/graphql/types/issue_type_enum_spec.rb +++ b/spec/graphql/types/issue_type_enum_spec.rb @@ -5,9 +5,9 @@ RSpec.describe Types::IssueTypeEnum do specify { expect(described_class.graphql_name).to eq('IssueType') } - it 'exposes all the existing issue type values except objective & key_result' do + it 'exposes all the existing issue type values except key_result' do expect(described_class.values.keys).to match_array( - %w[ISSUE INCIDENT TEST_CASE REQUIREMENT TASK] + %w[ISSUE INCIDENT TEST_CASE REQUIREMENT TASK OBJECTIVE] ) end end -- GitLab