Merge branch 'persisting-ca-group-stages' into 'master'

Services for persisting Cycle Analytics group stages

Closes #32570

See merge request gitlab-org/gitlab!17588

app/models/concerns/analytics/cycle_analytics/stage.rb

@@ -7,6 +7,7 @@ module Analytics
 
       included do
         validates :name, presence: true
+        validates :name, exclusion: { in: Gitlab::Analytics::CycleAnalytics::DefaultStages.names }, if: :custom?
         validates :start_event_identifier, presence: true
         validates :end_event_identifier, presence: true
         validate :validate_stage_event_pairs
@@ -15,6 +16,7 @@ module Analytics
         enum end_event_identifier: Gitlab::Analytics::CycleAnalytics::StageEvents.to_enum, _prefix: :end_event_identifier
 
         alias_attribute :custom_stage?, :custom
+        scope :default_stages, -> { where(custom: false) }
       end
 
       def parent=(_)

ee/app/controllers/analytics/cycle_analytics/stages_controller.rb

@@ -31,7 +31,7 @@ module Analytics
       end
 
       def stage_list_service
-        Analytics::CycleAnalytics::StageListService.new(
+        Analytics::CycleAnalytics::Stages::ListService.new(
           parent: @group,
           current_user: current_user
         )

ee/app/finders/analytics/cycle_analytics/stage_finder.rb

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    class StageFinder
+      NUMBERS_ONLY = /\A\d+\z/.freeze
+
+      def initialize(parent:, stage_id:)
+        @parent = parent
+        @stage_id = stage_id
+      end
+
+      def execute
+        if in_memory_default_stage?
+          build_in_memory_stage_by_name
+        else
+          parent.cycle_analytics_stages.find(stage_id)
+        end
+      end
+
+      private
+
+      attr_reader :parent, :stage_id
+
+      def in_memory_default_stage?
+        !NUMBERS_ONLY.match?(stage_id.to_s)
+      end
+
+      def build_in_memory_stage_by_name
+        parent.cycle_analytics_stages.build(find_in_memory_stage)
+      end
+
+      def find_in_memory_stage
+        # raise ActiveRecord::RecordNotFound, so it will behave similarly to AR models and produce 404 response in the controller
+        raw_stage = Gitlab::Analytics::CycleAnalytics::DefaultStages.all.find do |hash|
+          hash[:name].eql?(stage_id)
+        end
+
+        raise(ActiveRecord::RecordNotFound, "Stage with id '#{stage_id}' could not be found") unless raw_stage
+
+        raw_stage
+      end
+    end
+  end
+end

ee/app/policies/ee/group_policy.rb

@@ -69,8 +69,9 @@ module EE
       rule { can?(:read_group) & contribution_analytics_available }
         .enable :read_group_contribution_analytics
 
-      rule { reporter & cycle_analytics_available }
-        .enable :read_group_cycle_analytics
+      rule { reporter & cycle_analytics_available }.policy do
+        enable :read_group_cycle_analytics, :create_group_stage, :read_group_stage, :update_group_stage, :delete_group_stage
+      end
 
       rule { can?(:read_group) & dependency_proxy_available }
         .enable :read_dependency_proxy

ee/app/services/analytics/cycle_analytics/stage_list_service.rb → ee/app/services/analytics/cycle_analytics/stages/base_service.rb

@@ -2,40 +2,53 @@
 
 module Analytics
   module CycleAnalytics
-    class StageListService
+    class BaseService
       include Gitlab::Allowable
 
-      def initialize(parent:, current_user:)
+      def initialize(parent:, current_user:, params: {})
         @parent = parent
         @current_user = current_user
       end
 
       def execute
-        return forbidden unless allowed?
-
-        success(build_default_stages)
+        raise NotImplementedError
       end
 
       private
 
-      attr_reader :parent, :current_user
+      attr_reader :parent, :current_user, :params
 
-      def build_default_stages
-        Gitlab::Analytics::CycleAnalytics::DefaultStages.all.map do |params|
-          parent.cycle_analytics_stages.build(params)
-        end
+      def success(stage, http_status = :created)
+        ServiceResponse.success(payload: { stage: stage }, http_status: http_status)
+      end
+
+      def error(stage)
+        ServiceResponse.error(message: 'Invalid parameters', payload: { errors: stage.errors }, http_status: :unprocessable_entity)
       end
 
-      def success(stages)
-        ServiceResponse.success(payload: { stages: stages })
+      def not_found
+        ServiceResponse.error(message: 'Stage not found', payload: {}, http_status: :not_found)
       end
 
       def forbidden
-        ServiceResponse.error(message: 'Forbidden', http_status: :forbidden)
+        ServiceResponse.error(message: 'Forbidden', payload: {}, http_status: :forbidden)
+      end
+
+      def persist_default_stages!
+        persisted_default_stages = parent.cycle_analytics_stages.default_stages
+
+        # make sure that we persist default stages only once
+        stages_to_persist = build_default_stages.select do |new_default_stage|
+          !persisted_default_stages.find { |s| s.name.eql?(new_default_stage.name) }
+        end
+
+        stages_to_persist.each(&:save!)
       end
 
-      def allowed?
-        can?(current_user, :read_group_cycle_analytics, parent)
+      def build_default_stages
+        Gitlab::Analytics::CycleAnalytics::DefaultStages.all.map do |params|
+          parent.cycle_analytics_stages.build(params)
+        end
       end
     end
   end

ee/app/services/analytics/cycle_analytics/stages/create_service.rb

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    module Stages
+      class CreateService < BaseService
+        def initialize(parent:, current_user:, params:)
+          super
+
+          @stage = parent.cycle_analytics_stages.build(params)
+        end
+
+        def execute
+          return forbidden unless can?(current_user, :create_group_stage, parent)
+          return error(stage) unless stage.valid?
+
+          parent.class.transaction do
+            persist_default_stages!
+            stage.save!
+          end
+
+          success(stage)
+        end
+
+        private
+
+        attr_reader :stage
+      end
+    end
+  end
+end

ee/app/services/analytics/cycle_analytics/stages/list_service.rb

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    module Stages
+      class ListService < BaseService
+        def execute
+          return forbidden unless can?(current_user, :read_group_cycle_analytics, parent)
+
+          success(persisted_stages.presence || build_default_stages)
+        end
+
+        private
+
+        def success(stages)
+          ServiceResponse.success(payload: { stages: stages })
+        end
+
+        def persisted_stages
+          parent.cycle_analytics_stages
+        end
+      end
+    end
+  end
+end

ee/app/services/analytics/cycle_analytics/stages/update_service.rb

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    module Stages
+      class UpdateService < BaseService
+        def initialize(parent:, current_user:, params:)
+          super
+
+          @params = params
+        end
+
+        def execute
+          return forbidden unless can?(current_user, :update_group_stage, parent)
+
+          parent.cycle_analytics_stages.model.transaction do
+            persist_default_stages!
+
+            @stage = find_stage
+            @stage.assign_attributes(filtered_params)
+
+            raise ActiveRecord::Rollback unless @stage.valid?
+
+            @stage.save!
+          end
+
+          @stage.valid? ? success(@stage, :ok) : error(@stage)
+        end
+
+        private
+
+        def filtered_params
+          {}.tap do |new_params|
+            if default_stage?
+              new_params[:hidden] = params[:hidden] # for default stage only hidden parameter is allowed
+            else
+              new_params.merge!(params)
+            end
+          end.compact
+        end
+
+        def default_stage?
+          Gitlab::Analytics::CycleAnalytics::DefaultStages.names.include?(params[:id])
+        end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def find_stage
+          if default_stage?
+            # default stages are already persisted
+            parent.cycle_analytics_stages.find_by!(name: params[:id])
+          else
+            parent.cycle_analytics_stages.find(params[:id])
+          end
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

ee/spec/controllers/analytics/cycle_analytics/stages_controller_spec.rb

@@ -93,7 +93,7 @@ describe Analytics::CycleAnalytics::StagesController do
   end
 
   it 'renders 403 based on the response of the service object' do
-    expect_any_instance_of(Analytics::CycleAnalytics::StageListService).to receive(:allowed?).and_return(false)
+    expect_any_instance_of(Analytics::CycleAnalytics::Stages::ListService).to receive(:can?).and_return(false)
 
     subject
 

ee/spec/factories/analytics/cycle_analytics/group_stages.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :cycle_analytics_group_stage, class: Analytics::CycleAnalytics::GroupStage do
+    sequence(:name) { |n| "Stage ##{n}" }
+    start_event_identifier { Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestCreated.identifier }
+    end_event_identifier { Gitlab::Analytics::CycleAnalytics::StageEvents::MergeRequestMerged.identifier }
+    group
+  end
+end

ee/spec/finders/analytics/cycle_analytics/stage_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::CycleAnalytics::StageFinder do
+  let_it_be(:group) { create(:group) }
+  let(:stage_id) { { id: Gitlab::Analytics::CycleAnalytics::DefaultStages.names.first } }
+
+  subject { described_class.new(parent: group, stage_id: stage_id[:id]).execute }
+
+  context 'when looking up in-memory default stage by name exists' do
+    it { expect(subject).not_to be_persisted }
+    it { expect(subject.name).to eq(stage_id[:id]) }
+  end
+
+  context 'when in-memory default stage cannot be found' do
+    before do
+      stage_id[:id] = 'unknown_default_stage'
+    end
+
+    it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+  end
+
+  context 'when persisted stage exists' do
+    let(:stage) { create(:cycle_analytics_group_stage, group: group) }
+
+    before do
+      stage_id[:id] = stage.id
+    end
+
+    it { expect(subject).to be_persisted }
+    it { expect(subject.name).to eq(stage.name) }
+  end
+
+  context 'when persisted stage cannot be found' do
+    before do
+      stage_id[:id] = -1
+    end
+
+    it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+  end
+end

ee/spec/services/analytics/cycle_analytics/stage_list_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Analytics::CycleAnalytics::StageListService do
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
-  subject { described_class.new(parent: group, current_user: user) }
-
-  context 'succeeds' do
-    let(:stages) { subject.execute.payload[:stages] }
-
-    before do
-      stub_licensed_features(cycle_analytics_for_groups: true)
-
-      group.add_reporter(user)
-    end
-
-    it 'returns only the default stages' do
-      expect(stages.size).to eq(Gitlab::Analytics::CycleAnalytics::DefaultStages.all.size)
-    end
-
-    it 'provides the default stages as non-persisted objects' do
-      stage_ids = stages.map(&:id)
-      expect(stage_ids.all?(&:nil?)).to eq(true)
-    end
-  end
-
-  it 'returns forbidden response' do
-    result = subject.execute
-
-    expect(result).to be_error
-    expect(result.http_status).to eq(:forbidden)
-  end
-end

ee/spec/services/analytics/cycle_analytics/stages/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::CycleAnalytics::Stages::CreateService do
+  let_it_be(:group, refind: true) { create(:group) }
+  let_it_be(:user, refind: true) { create(:user) }
+  let(:params) { { name: 'my stage', start_event_identifier: :merge_request_created, end_event_identifier: :merge_request_merged } }
+
+  before_all do
+    group.add_user(user, :reporter)
+  end
+
+  before do
+    stub_licensed_features(cycle_analytics_for_groups: true)
+  end
+
+  subject { described_class.new(parent: group, params: params, current_user: user).execute }
+
+  it_behaves_like 'permission check for cycle analytics stage services', :cycle_analytics_for_groups
+
+  describe 'custom stage creation' do
+    context 'when service response is successful' do
+      let(:stage) { subject.payload[:stage] }
+
+      it { expect(subject).to be_success }
+      it { expect(subject.http_status).to eq(:created) }
+      it { expect(stage).to be_present }
+      it { expect(stage).to be_persisted }
+      it { expect(stage.start_event_identifier).to eq(params[:start_event_identifier].to_s) }
+      it { expect(stage.end_event_identifier).to eq(params[:end_event_identifier].to_s) }
+    end
+  end
+
+  context 'when params are invalid' do
+    before do
+      params.delete(:name)
+    end
+
+    it { expect(subject).to be_error }
+    it { expect(subject.http_status).to eq(:unprocessable_entity) }
+    it { expect(subject.payload[:errors].keys).to eq([:name]) }
+  end
+
+  describe 'persistence of default stages' do
+    let(:persisted_stages) { group.cycle_analytics_stages }
+    let(:customized_stages) { group.cycle_analytics_stages.where(custom: true) }
+    let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages.all }
+    let(:expected_stage_count) { default_stages.count + customized_stages.count }
+
+    context 'when creating custom stages' do
+      it { expect(subject).to be_success }
+
+      it 'persists all default stages' do
+        subject
+
+        expect(persisted_stages.count).to eq(expected_stage_count)
+      end
+
+      context 'when creating two custom stages' do
+        before do
+          described_class.new(parent: group, params: params.merge(name: 'other stage'), current_user: user).execute
+        end
+
+        it 'creates two customized stages' do
+          subject
+
+          expect(customized_stages.count).to eq(2)
+        end
+
+        it 'creates records for the default stages only once plus two customized stage records' do
+          expect(group.cycle_analytics_stages.count).to eq(expected_stage_count)
+        end
+      end
+    end
+
+    context 'when params are invalid' do
+      before do
+        params.delete(:name)
+      end
+
+      it { expect(subject).to be_error }
+
+      it 'skips persisting default stages on validation error' do
+        expect(group.cycle_analytics_stages.count).to eq(0)
+      end
+    end
+  end
+end

ee/spec/services/analytics/cycle_analytics/stages/list_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::CycleAnalytics::Stages::ListService do
+  let_it_be(:group, refind: true) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let(:stages) { subject.payload[:stages] }
+
+  subject { described_class.new(parent: group, current_user: user).execute }
+
+  before_all do
+    group.add_reporter(user)
+  end
+
+  before do
+    stub_licensed_features(cycle_analytics_for_groups: true)
+  end
+
+  it_behaves_like 'permission check for cycle analytics stage services', :cycle_analytics_for_groups
+
+  it 'returns only the default stages' do
+    expect(stages.size).to eq(Gitlab::Analytics::CycleAnalytics::DefaultStages.all.size)
+  end
+
+  it 'provides the default stages as non-persisted objects' do
+    expect(stages.map(&:id)).to all(be_nil)
+  end
+
+  context 'when there are persisted stages' do
+    let!(:stage1) { create(:cycle_analytics_group_stage, parent: group) }
+    let!(:stage2) { create(:cycle_analytics_group_stage, parent: group) }
+
+    it 'returns the persisted stages' do
+      expect(stages).to contain_exactly(stage1, stage2)
+    end
+  end
+end

ee/spec/services/analytics/cycle_analytics/stages/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::CycleAnalytics::Stages::UpdateService do
+  let_it_be(:group, refind: true) { create(:group) }
+  let_it_be(:user, refind: true) { create(:user) }
+  let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages.all }
+  let(:params) { {} }
+
+  subject { described_class.new(parent: group, params: params, current_user: user).execute }
+
+  before_all do
+    group.add_user(user, :reporter)
+  end
+
+  before do
+    stub_licensed_features(cycle_analytics_for_groups: true)
+  end
+
+  it_behaves_like 'permission check for cycle analytics stage services', :cycle_analytics_for_groups
+
+  context 'when updating a default stage' do
+    let(:stage) { Analytics::CycleAnalytics::GroupStage.new(default_stages.first.merge(group: group)) }
+    let(:params) { { id: stage.name, hidden: true } }
+    let(:updated_stage) { subject.payload[:stage] }
+
+    context 'when hiding a default stage' do
+      it { expect(subject).to be_success }
+      it { expect(updated_stage).to be_persisted }
+      it { expect(updated_stage).to be_hidden }
+    end
+
+    context 'when other parameters than "hidden" are given' do
+      before do
+        params[:name] = 'should not be updated'
+      end
+
+      it { expect(subject).to be_success }
+      it { expect(updated_stage.name).not_to eq(params[:name]) }
+    end
+
+    context 'when the first update happens on a default stage' do
+      let(:persisted_stages) { group.reload.cycle_analytics_stages }
+
+      it { expect(subject).to be_success }
+
+      it 'persists all default stages' do
+        subject
+
+        expect(persisted_stages.count).to eq(default_stages.count)
+        expect(persisted_stages).to all(be_persisted)
+      end
+
+      it 'matches with the configured default stage name' do
+        subject
+
+        default_stage_names = default_stages.map { |s| s[:name] }
+        expect(default_stage_names).to include(updated_stage.name)
+      end
+
+      context 'when the update fails' do
+        before do
+          invalid_stage = Analytics::CycleAnalytics::GroupStage.new(name: '')
+          expect_any_instance_of(described_class).to receive(:find_stage).and_return(invalid_stage)
+        end
+
+        it 'returns unsuccessful service response' do
+          subject
+
+          expect(subject).not_to be_success
+        end
+
+        it 'does not persist the default stages if the stage is invalid' do
+          subject
+
+          expect(persisted_stages).not_to include(be_persisted)
+        end
+      end
+    end
+
+    context 'when updating an already persisted default stage' do
+      let(:persisted_stage) { subject.payload[:stage] }
+
+      let(:updated_stage) do
+        described_class
+          .new(parent: group, params: { id: persisted_stage.id, hidden: false }, current_user: user)
+          .execute
+          .payload[:stage]
+      end
+
+      it { expect(updated_stage).to be_persisted }
+      it { expect(updated_stage).not_to be_hidden }
+    end
+  end
+
+  context 'when updating a custom stage' do
+    let_it_be(:stage) { create(:cycle_analytics_group_stage, group: group) }
+    let(:params) { { id: stage.id, name: 'my new stage name' } }
+
+    it { expect(subject).to be_success }
+    it { expect(subject.http_status).to eq(:ok) }
+    it { expect(subject.payload[:stage].name).to eq(params[:name]) }
+
+    context 'when params are invalid' do
+      before do
+        params[:name] = ''
+      end
+
+      it { expect(subject).to be_error }
+      it { expect(subject.http_status).to eq(:unprocessable_entity) }
+      it { expect(subject.payload[:errors].keys).to eq([:name]) }
+    end
+  end
+end

ee/spec/support/shared_examples/services/analytics/cycle_analytics/stages_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'permission check for cycle analytics stage services' do |required_license|
+  context 'when user has no access' do
+    before do
+      group.add_user(user, :guest)
+    end
+
+    it { expect(subject).to be_error }
+    it { expect(subject.http_status).to eq(:forbidden) }
+  end
+
+  context 'when license is missing' do
+    before do
+      stub_licensed_features(required_license => false)
+    end
+
+    it { expect(subject).to be_error }
+    it { expect(subject.http_status).to eq(:forbidden) }
+  end
+end

lib/gitlab/analytics/cycle_analytics/default_stages.rb

@@ -23,6 +23,10 @@ module Gitlab
           ]
         end
 
+        def self.names
+          all.map { |stage| stage[:name] }
+        end
+
         def self.params_for_issue_stage
           {
             name: 'issue',

spec/support/shared_examples/cycle_analytics_stage_examples.rb

@@ -46,6 +46,13 @@ shared_examples_for 'cycle analytics stage' do
       expect(stage).not_to be_valid
       expect(stage.errors.details[:end_event]).to eq([{ error: :not_allowed_for_the_given_start_event }])
     end
+
+    context 'disallows default stage names when creating custom stage' do
+      let(:invalid_params) { valid_params.merge(name: Gitlab::Analytics::CycleAnalytics::DefaultStages.names.first, custom: true) }
+      let(:stage) { described_class.new(invalid_params) }
+
+      it { expect(stage).not_to be_valid }
+    end
   end
 
   describe '#subject_model' do