Extend Value Stream API to accept stages

This commit extends the Value Stream create API to accept stage
parameters. This makes the creation of value streams with stages easier
from frontend point of view.

The commit also adds PUT#update action for the value stream.

ee/app/controllers/groups/analytics/cycle_analytics/value_streams_controller.rb

@@ -13,12 +13,22 @@ class Groups::Analytics::CycleAnalytics::ValueStreamsController < Groups::Analyt
   end
 
   def create
-    value_stream = @group.value_streams.build(value_stream_params)
+    result = Analytics::CycleAnalytics::ValueStreams::CreateService.new(group: @group, params: create_params, current_user: current_user).execute
 
-    if value_stream.save
-      render json: Analytics::CycleAnalytics::GroupValueStreamSerializer.new.represent(value_stream)
+    if result.success?
+      render json: serialize_value_stream(result), status: result.http_status
     else
-      render json: { message: 'Invalid parameters', payload: { errors: value_stream.errors } }, status: :unprocessable_entity
+      render json: { message: result.message, payload: { errors: serialize_value_stream_error(result) } }, status: result.http_status
+    end
+  end
+
+  def update
+    result = Analytics::CycleAnalytics::ValueStreams::UpdateService.new(group: @group, params: update_params, current_user: current_user).execute
+
+    if result.success?
+      render json: serialize_value_stream(result), status: result.http_status
+    else
+      render json: { message: result.message, payload: { errors: serialize_value_stream_error(result) } }, status: result.http_status
     end
   end
 
@@ -39,6 +49,22 @@ class Groups::Analytics::CycleAnalytics::ValueStreamsController < Groups::Analyt
     params.require(:value_stream).permit(:name)
   end
 
+  def create_params
+    params.require(:value_stream).permit(:name, stages: stage_create_params)
+  end
+
+  def update_params
+    params.require(:value_stream).permit(:name, stages: stage_update_params)
+  end
+
+  def stage_create_params
+    [:name, :start_event_identifier, :end_event_identifier, :start_event_label_id, :end_event_label_id, :custom]
+  end
+
+  def stage_update_params
+    stage_create_params + [:id]
+  end
+
   def value_streams
     @group.value_streams.presence || [in_memory_default_value_stream]
   end
@@ -46,4 +72,12 @@ class Groups::Analytics::CycleAnalytics::ValueStreamsController < Groups::Analyt
   def in_memory_default_value_stream
     @group.value_streams.new(name: Analytics::CycleAnalytics::Stages::BaseService::DEFAULT_VALUE_STREAM_NAME)
   end
+
+  def serialize_value_stream(result)
+    Analytics::CycleAnalytics::GroupValueStreamSerializer.new.represent(result.payload[:value_stream])
+  end
+
+  def serialize_value_stream_error(result)
+    Analytics::CycleAnalytics::ValueStreamErrorsSerializer.new(result.payload[:value_stream])
+  end
 end

ee/app/models/analytics/cycle_analytics/group_value_stream.rb

@@ -8,6 +8,8 @@ class Analytics::CycleAnalytics::GroupValueStream < ApplicationRecord
   validates :group, :name, presence: true
   validates :name, length: { minimum: 3, maximum: 100, allow_nil: false }, uniqueness: { scope: :group_id }
 
+  accepts_nested_attributes_for :stages, allow_destroy: true
+
   def custom?
     name != Analytics::CycleAnalytics::Stages::BaseService::DEFAULT_VALUE_STREAM_NAME
   end

ee/app/serializers/analytics/cycle_analytics/group_value_stream_entity.rb

@@ -8,12 +8,17 @@ module Analytics
       expose :is_custom do |object|
         object.custom?
       end
+      expose :stages, using: Analytics::CycleAnalytics::StageEntity
 
       private
 
       def id
         object.id || object.name # use the name `default` if the record is not persisted
       end
+
+      def stages
+        object.stages.map { |s| ::Analytics::CycleAnalytics::StagePresenter.new(s) } # rubocop: disable CodeReuse/Presenter
+      end
     end
   end
 end

ee/app/serializers/analytics/cycle_analytics/value_stream_errors_serializer.rb

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    # This class serializes errors from the GroupValueStream models and also includes errors from the stages relation.
+    #
+    # Reason: The GroupValueStream model uses accepts_nested_attributes_for receiving stages (has many) and the error object
+    # generated by active record is incorrect (using the index_errors option)
+    #
+    # This custom serializer was introduced to give enough information to the frontend to map the errors to the respective
+    # form fields.
+    #
+    # Issue: https://github.com/rails/rails/issues/24390
+    #
+    # Example error object:
+    #
+    # {
+    #   name: ["can't be blank"],
+    #   stages: [
+    #     {
+    #       id: nil,
+    #       name: "",
+    #       errors: {
+    #         name: ["can't be blank"]
+    #       }
+    #     }
+    #   ]
+    # }
+    class ValueStreamErrorsSerializer
+      def initialize(value_stream)
+        @value_stream = value_stream
+      end
+
+      def as_json(options = {})
+        # skip all errors related to the associated stages
+        errors = value_stream.errors.messages.reject do |attribute, messages|
+          attribute.to_s.start_with?("stages.")
+        end
+
+        # add all stages errors
+        stages_errors = collect_stages_with_errors
+
+        errors[:stages] = stages_errors if stages_errors.any?
+
+        errors
+      end
+
+      private
+
+      attr_reader :value_stream
+
+      def collect_stages_with_errors
+        value_stream.stages.select(&:invalid?).map do |stage|
+          # id and name is enough to identify record on the UI
+          {
+            id: stage.id,
+            name: stage.name,
+            errors: stage.errors.messages
+          }
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    module ValueStreams
+      class CreateService
+        include Gitlab::Allowable
+
+        def initialize(group:, params:, current_user:, value_stream: ::Analytics::CycleAnalytics::GroupValueStream.new(group: group))
+          @value_stream = value_stream
+          @group = group
+          @params = process_params(params)
+          @current_user = current_user
+        end
+
+        def execute
+          error = authorize!
+          return error if error
+
+          value_stream.assign_attributes(params)
+
+          if value_stream.save
+            ServiceResponse.success(message: nil, payload: { value_stream: value_stream }, http_status: success_http_status)
+          else
+            ServiceResponse.error(message: 'Invalid parameters', payload: { errors: value_stream.errors, value_stream: value_stream }, http_status: :unprocessable_entity)
+          end
+        end
+
+        private
+
+        attr_reader :value_stream, :group, :params, :current_user
+
+        def process_params(raw_params)
+          if raw_params[:stages]
+            raw_params[:stages_attributes] = raw_params.delete(:stages)
+            raw_params[:stages_attributes].map! { |attrs| build_stage_attributes(attrs) }
+          end
+
+          raw_params
+        end
+
+        def build_stage_attributes(stage_attributes)
+          stage_attributes[:group] = group
+          return stage_attributes if stage_attributes[:custom]
+
+          # if we're persisting a default stage, ignore the user provided attributes and use our attributes
+          use_default_stage_params(stage_attributes)
+        end
+
+        def use_default_stage_params(stage_attributes)
+          default_stage_attributes = Gitlab::Analytics::CycleAnalytics::DefaultStages.find_by_name!(stage_attributes[:name].to_s.downcase)
+          stage_attributes.merge(default_stage_attributes)
+        end
+
+        def success_http_status
+          :created
+        end
+
+        def authorize!
+          unless can?(current_user, :read_group_cycle_analytics, group)
+            ServiceResponse.error(message: 'Forbidden', http_status: :forbidden, payload: { errors: nil })
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Analytics
+  module CycleAnalytics
+    module ValueStreams
+      class UpdateService < CreateService
+        include Gitlab::Allowable
+
+        private
+
+        def process_params(raw_params)
+          processed_params = super
+
+          persisted_stage_ids.each do |stage_id|
+            if to_be_deleted?(processed_params, stage_id)
+              processed_params[:stages_attributes] << { id: stage_id, _destroy: '1' }
+            end
+          end
+
+          processed_params
+        end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def persisted_stage_ids
+          @persisted_stage_ids ||= value_stream.stages.pluck(:id)
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        def to_be_deleted?(processed_params, stage_id)
+          processed_params[:stages_attributes].none? { |attrs| attrs[:id] == stage_id }
+        end
+
+        def success_http_status
+          :ok
+        end
+      end
+    end
+  end
+end

ee/config/routes/group.rb

@@ -29,7 +29,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
             get :records
           end
         end
-        resources :value_streams, only: [:index, :create, :destroy] do
+        resources :value_streams, only: [:index, :create, :update, :destroy] do
           resources :stages, only: [:index, :create, :update, :destroy] do
             member do
               get :duration_chart

ee/spec/controllers/groups/analytics/cycle_analytics/value_streams_controller_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe Groups::Analytics::CycleAnalytics::ValueStreamsController do
           post :create, params: { group_id: group, value_stream: { name: "busy value stream" } }
         end.to change { Analytics::CycleAnalytics::GroupValueStream.count }.by(1)
 
-        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to have_gitlab_http_status(:created)
       end
     end
 
@@ -60,6 +60,81 @@ RSpec.describe Groups::Analytics::CycleAnalytics::ValueStreamsController do
         expect(json_response["message"]).to eq('Invalid parameters')
       end
     end
+
+    context 'with stages' do
+      let(:value_stream_params) do
+        {
+          name: 'test',
+          stages: [
+            {
+              name: 'My Stage',
+              start_event_identifier: 'issue_created',
+              end_event_identifier: 'issue_closed',
+              custom: true
+            }
+          ]
+        }
+      end
+
+      it 'persists the value stream with stages' do
+        post :create, params: { group_id: group, value_stream: value_stream_params }
+
+        expect(response).to have_gitlab_http_status(:created)
+        stage_response = json_response['stages'].first
+        expect(stage_response['title']).to eq('My Stage')
+      end
+
+      context 'when invalid stage is given' do
+        before do
+          value_stream_params[:stages].first[:name] = ''
+        end
+
+        it 'renders errors with unprocessable entity, 422 response' do
+          post :create, params: { group_id: group, value_stream: value_stream_params }
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          stage_error = json_response['payload']['errors']['stages'].first
+          expect(stage_error['errors']['name']).to be_present
+        end
+      end
+    end
+  end
+
+  describe 'PUT #update' do
+    context 'with valid params' do
+      let!(:value_stream) { create(:cycle_analytics_group_value_stream, group: group, name: 'value stream') }
+
+      it 'returns a successful 200 response' do
+        put :update, params: { id: value_stream.id, group_id: group, value_stream: { name: 'new name' } }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['name']).to eq('new name')
+      end
+
+      context 'with stages' do
+        let!(:stage) { create(:cycle_analytics_group_stage, group: group, value_stream: value_stream, name: 'stage 1', custom: true) }
+
+        let(:value_stream_params) do
+          {
+            name: 'test',
+            stages: [
+              {
+                id: stage.id,
+                name: 'new stage name',
+                custom: true
+              }
+            ]
+          }
+        end
+
+        it 'returns a successful 200 response' do
+          put :update, params: { id: value_stream.id, group_id: group, value_stream: { name: 'new name' } }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['name']).to eq('new name')
+        end
+      end
+    end
   end
 
   describe 'DELETE #destroy' do

ee/spec/fixtures/api/schemas/analytics/cycle_analytics/value_stream.json

@@ -10,6 +10,12 @@
     },
     "is_custom": {
       "type": "boolean"
+    },
+    "stages": {
+      "type": "array",
+      "items": {
+        "$ref": "stage.json"
+      }
     }
   },
   "additionalProperties": false

ee/spec/serializers/analytics/cycle_analytics/value_stream_errors_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Analytics::CycleAnalytics::ValueStreamErrorsSerializer do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:value_stream) { create(:cycle_analytics_group_value_stream, name: 'name', group: group) }
+
+  subject { described_class.new(value_stream).as_json }
+
+  it 'serializes error on value stream object' do
+    value_stream.name = ''
+
+    value_stream.validate
+
+    expect(subject[:name]).not_to be_empty
+  end
+
+  context 'when nested value stream stages are given' do
+    let(:invalid_stage) { build(:cycle_analytics_group_stage, name: '', group: group) }
+    let(:valid_stage) { build(:cycle_analytics_group_stage, group: group) }
+
+    before do
+      value_stream.stages << invalid_stage
+      value_stream.stages << valid_stage
+    end
+
+    it 'serializes error on value stream object' do
+      value_stream.validate
+
+      stage = subject[:stages].first
+      expect(stage[:errors]).not_to be_empty
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Analytics::CycleAnalytics::ValueStreams::CreateService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group, refind: true) { create(:group) }
+  let(:params) { {} }
+
+  subject { described_class.new(group: group, params: params, current_user: user).execute }
+
+  it_behaves_like 'common value stream service examples'
+
+  context 'when the feature is available' do
+    before do
+      group.add_developer(user)
+      stub_licensed_features(cycle_analytics_for_groups: true)
+    end
+
+    context 'when stage params are passed' do
+      let(:params) do
+        {
+          name: 'my value stream',
+          stages: [
+            {
+              name: 'Custom stage 1',
+              start_event_identifier: 'merge_request_created',
+              end_event_identifier: 'merge_request_closed',
+              custom: true
+            },
+            {
+              name: 'Custom stage 2',
+              start_event_identifier: 'issue_created',
+              end_event_identifier: 'issue_closed',
+              custom: true
+            }
+          ]
+        }
+      end
+
+      it 'persists the value stream record' do
+        expect(subject).to be_success
+        expect(subject.payload[:value_stream]).to be_persisted
+      end
+
+      it 'persists the stages' do
+        value_stream = subject.payload[:value_stream]
+
+        expect(value_stream.stages.size).to eq(2)
+      end
+
+      context 'when the stage is invalid' do
+        it 'propagates validation errors' do
+          params[:stages].first[:name] = ''
+
+          errors = subject.payload[:errors].details
+          expect(errors[:'stages.name']).to eq([{ error: :blank }])
+        end
+      end
+
+      context 'when creating a default stage' do
+        before do
+          params[:stages] = [{ name: 'plan', custom: false }]
+        end
+
+        let(:custom_stage) { subject.payload[:value_stream].stages.first }
+
+        it 'persists the stage as custom stage' do
+          expect(subject).to be_success
+          expect(custom_stage).to be_persisted
+        end
+      end
+
+      context 'when no stage params are passed' do
+        let(:params) { { name: 'test' } }
+
+        it 'persists the value stream record' do
+          expect(subject).to be_success
+          expect(subject.payload[:value_stream]).to be_persisted
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Analytics::CycleAnalytics::ValueStreams::UpdateService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group, refind: true) { create(:group) }
+
+  let(:params) { {} }
+
+  let(:value_stream) do
+    create(:cycle_analytics_group_value_stream, name: 'VS 1', group: group, stages: [
+      build(:cycle_analytics_group_stage, group: group, name: 'stage 1', custom: true),
+      build(:cycle_analytics_group_stage, group: group, name: 'stage 2', custom: true)
+    ])
+  end
+
+  let(:first_stage) { value_stream.stages.first }
+  let(:last_stage) { value_stream.stages.last }
+
+  subject { described_class.new(value_stream: value_stream, group: group, params: params, current_user: user).execute }
+
+  context 'when the feature is available' do
+    before do
+      group.add_developer(user)
+      stub_licensed_features(cycle_analytics_for_groups: true)
+    end
+
+    context 'when empty stages are given' do
+      let(:params) { { name: 'VS 1', stages: [] } }
+
+      it 'removes the stages' do
+        expect(subject).to be_success
+        expect(subject.payload[:value_stream].reload.stages).to be_empty
+      end
+    end
+
+    context 'updating one stage within a value stream' do
+      let(:params) do
+        {
+          name: 'VS 1',
+          stages: [
+            { id: first_stage.id, name: first_stage.name, custom: true },
+            { id: last_stage.id, name: 'updated', custom: true }
+          ]
+        }
+      end
+
+      it 'updates the stage' do
+        expect(subject).to be_success
+        expect(last_stage.reload.name).to eq('updated')
+      end
+
+      context 'when the params are invalid' do
+        before do
+          params[:stages].last[:name] = ''
+        end
+
+        it 'returns error' do
+          expect(subject).to be_error
+          errors = subject.payload[:errors].details
+          expect(errors[:'stages.name']).to eq([{ error: :blank }])
+        end
+      end
+    end
+
+    context 'adding a new stage within a value stream' do
+      let(:params) do
+        {
+          name: 'VS 1',
+          stages: [
+            { id: first_stage.id, name: first_stage.name, custom: true },
+            { id: last_stage.id, name: last_stage.name, custom: true },
+            { name: 'new stage', custom: true, start_event_identifier: 'merge_request_created', end_event_identifier: 'merge_request_closed' }
+          ]
+        }
+      end
+
+      it 'creates the stage' do
+        expect(subject).to be_success
+        expect(subject.payload[:value_stream].stages.last.name).to eq('new stage')
+      end
+    end
+
+    context 'when adding a default stage' do
+      let(:params) do
+        {
+          name: 'VS 1',
+          stages: [
+            { id: first_stage.id, name: first_stage.name, custom: true },
+            { id: last_stage.id, name: last_stage.name, custom: true },
+            { name: 'plan', custom: false }
+          ]
+        }
+      end
+
+      it 'creates the stage' do
+        expect(subject).to be_success
+        expect(subject.payload[:value_stream].stages.last.name).to eq('plan')
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+RSpec.shared_examples 'common value stream service examples' do
+  context 'when the user has no permission' do
+    it 'returns error' do
+      expect(subject).to be_error
+      expect(subject.message).to eq('Forbidden')
+    end
+  end
+
+  context 'when the license is missing' do
+    before do
+      group.add_developer(user)
+      stub_licensed_features(cycle_analytics_for_groups: false)
+    end
+
+    it 'returns error' do
+      expect(subject).to be_error
+      expect(subject.message).to eq('Forbidden')
+    end
+  end
+end