Merge branch 'issue_36427' into 'master'

Allow to edit issues and epics health status

See merge request gitlab-org/gitlab!24202

app/services/issuable_base_service.rb

@@ -11,10 +11,14 @@ class IssuableBaseService < BaseService
     @skip_milestone_email = @params.delete(:skip_milestone_email)
   end
 
-  def filter_params(issuable)
+  def can_admin_issuable?(issuable)
     ability_name = :"admin_#{issuable.to_ability_name}"
 
-    unless can?(current_user, ability_name, issuable)
+    can?(current_user, ability_name, issuable)
+  end
+
+  def filter_params(issuable)
+    unless can_admin_issuable?(issuable)
       params.delete(:milestone_id)
       params.delete(:labels)
       params.delete(:add_label_ids)

db/migrate/20200131181354_add_health_status_to_epics.rb

+# frozen_string_literal: true
+
+class AddHealthStatusToEpics < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :epics, :health_status, :integer, limit: 2
+  end
+end

db/migrate/20200131191754_add_health_status_to_issues.rb

+# frozen_string_literal: true
+
+class AddHealthStatusToIssues < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :issues, :health_status, :integer, limit: 2
+  end
+end

db/schema.rb

@@ -1554,6 +1554,7 @@ ActiveRecord::Schema.define(version: 2020_02_07_151640) do
     t.integer "state_id", limit: 2, default: 1, null: false
     t.integer "start_date_sourcing_epic_id"
     t.integer "due_date_sourcing_epic_id"
+    t.integer "health_status", limit: 2
     t.index ["assignee_id"], name: "index_epics_on_assignee_id"
     t.index ["author_id"], name: "index_epics_on_author_id"
     t.index ["closed_by_id"], name: "index_epics_on_closed_by_id"
@@ -2181,6 +2182,7 @@ ActiveRecord::Schema.define(version: 2020_02_07_151640) do
     t.integer "state_id", limit: 2, default: 1, null: false
     t.integer "duplicated_to_id"
     t.integer "promoted_to_epic_id"
+    t.integer "health_status", limit: 2
     t.index ["author_id"], name: "index_issues_on_author_id"
     t.index ["closed_by_id"], name: "index_issues_on_closed_by_id"
     t.index ["confidential"], name: "index_issues_on_confidential"

ee/app/models/concerns/ee/issuable.rb

@@ -21,5 +21,9 @@ module EE
     def supports_epic?
       is_a?(Issue) && project.group
     end
+
+    def supports_health_status?
+      false
+    end
   end
 end

ee/app/models/concerns/health_status.rb

+# frozen_string_literal: true
+
+module HealthStatus
+  extend ActiveSupport::Concern
+  extend ::Gitlab::Utils::Override
+
+  included do
+    enum health_status: {
+      on_track: 1,
+      needs_attention: 2,
+      at_risk: 3
+    }
+  end
+
+  override :supports_health_status?
+  def supports_health_status?
+    resource_parent.feature_available?(:issuable_health_status) &&
+      ::Feature.enabled?(:save_issuable_health_status, resource_parent)
+  end
+end

ee/app/models/ee/epic.rb

@@ -15,6 +15,7 @@ module EE
       include UsageStatistics
       include FromUnion
       include EpicTreeSorting
+      include HealthStatus
 
       enum state_id: {
         opened: ::Epic.available_states[:opened],

ee/app/models/ee/issue.rb

@@ -14,6 +14,7 @@ module EE
       include Elastic::ApplicationVersionedSearch
       include UsageStatistics
       include WeightEventable
+      include HealthStatus
 
       scope :order_weight_desc, -> { reorder ::Gitlab::Database.nulls_last_order('weight', 'DESC') }
       scope :order_weight_asc, -> { reorder ::Gitlab::Database.nulls_last_order('weight') }

ee/app/models/license.rb

@@ -115,6 +115,7 @@ class License < ApplicationRecord
     group_level_compliance_dashboard
     incident_management
     insights
+    issuable_health_status
     license_management
     personal_access_token_expiration_policy
     pod_logs

ee/app/services/ee/issuable_base_service.rb

@@ -11,14 +11,16 @@ module EE
 
     override :filter_params
     def filter_params(issuable)
-      # This security check is repeated here to avoid multiple backports,
-      # this should be refactored to be reused from the base class.
-      ability_name = :"admin_#{issuable.to_ability_name}"
+      can_admin_issuable = can_admin_issuable?(issuable)
 
-      unless issuable.supports_weight? && can?(current_user, ability_name, issuable)
+      unless can_admin_issuable && issuable.supports_weight?
         params.delete(:weight)
       end
 
+      unless can_admin_issuable && issuable.supports_health_status?
+        params.delete(:health_status)
+      end
+
       super
     end
 

ee/changelogs/unreleased/issue_36427.yml

+---
+title: Add health_status column to issues and epics tables
+merge_request: 24202
+author:
+type: added

ee/spec/models/epic_spec.rb

@@ -565,4 +565,6 @@ describe Epic do
   end
 
   it_behaves_like 'versioned description'
+
+  it_behaves_like 'having health status'
 end

ee/spec/models/issue_spec.rb

@@ -572,4 +572,6 @@ describe Issue do
       it { is_expected.to eq(expected) }
     end
   end
+
+  it_behaves_like 'having health status'
 end

ee/spec/services/ee/issues/update_service_spec.rb

@@ -58,6 +58,11 @@ describe Issues::UpdateService do
         end
       end
 
+      it_behaves_like 'updating issuable health status' do
+        let(:issuable) { issue }
+        let(:parent) { project }
+      end
+
       context 'updating other fields' do
         it 'does not call UpdateDatesService' do
           expect(Epics::UpdateDatesService).not_to receive(:new)

ee/spec/services/epics/update_service_spec.rb

@@ -254,5 +254,10 @@ describe Epics::UpdateService do
       let(:issuable) { epic }
       let(:parent) { group }
     end
+
+    it_behaves_like 'updating issuable health status' do
+      let(:issuable) { epic }
+      let(:parent) { group }
+    end
   end
 end

ee/spec/support/shared_examples/models/concerns/health_status_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'having health status' do
+  context 'validations' do
+    it do
+      is_expected.to define_enum_for(:health_status)
+        .with_values(on_track: 1, needs_attention: 2, at_risk: 3)
+    end
+
+    it { is_expected.to allow_value(nil).for(:health_status) }
+  end
+end

ee/spec/support/shared_examples/services/update_issuable_health_status_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'updating issuable health status' do
+  context 'updating health_status' do
+    let(:current_user) { create(:user) }
+    let(:opts) { { health_status: 1 } }
+    let(:service) { described_class.new(parent, current_user, opts) }
+
+    context 'when feature is not available' do
+      it 'does not update issue health status' do
+        expect { service.execute(issuable) }.not_to change { issuable.health_status }
+      end
+    end
+
+    context 'when feature is available' do
+      before do
+        stub_licensed_features(issuable_health_status: true, epics: true)
+      end
+
+      context 'when feature flag is disabled' do
+        it 'does not update issuable' do
+          stub_feature_flags(save_issuable_health_status: false)
+
+          expect { service.execute(issuable) }.not_to change { issuable.health_status }
+        end
+      end
+
+      context 'when user has reporter permissions' do
+        before do
+          issuable.resource_parent.add_reporter(current_user)
+        end
+
+        it 'updates issuable with given health_status' do
+          expect { service.execute(issuable) }.to change { issuable.health_status }.to('on_track')
+        end
+      end
+
+      context 'when user does not have permissions' do
+        it 'does not update issuable status' do
+          expect { service.execute(issuable) }.not_to change { issuable.health_status }
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -30,6 +30,7 @@ Issue:
 - last_edited_at
 - last_edited_by_id
 - discussion_locked
+- health_status
 Event:
 - id
 - target_type
@@ -824,3 +825,4 @@ Epic:
   - state_id
   - start_date_sourcing_epic_id
   - due_date_sourcing_epic_id
+  - health_status