Merge branch '296832-update-value-stream-create-api' into 'master'

Extend value stream create and update actions

See merge request gitlab-org/gitlab!51623

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

@@ -13,12 +13,23 @@ 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
+    value_stream = Analytics::CycleAnalytics::GroupValueStream.find(params[:id])
+    result = Analytics::CycleAnalytics::ValueStreams::UpdateService.new(group: @group, params: update_params, current_user: current_user, value_stream: value_stream).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 +50,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 +73,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

@@ -3,11 +3,13 @@
 class Analytics::CycleAnalytics::GroupValueStream < ApplicationRecord
   belongs_to :group
 
-  has_many :stages, class_name: 'Analytics::CycleAnalytics::GroupStage'
+  has_many :stages, class_name: 'Analytics::CycleAnalytics::GroupStage', index_errors: true
 
   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.
+    #
+    # The GroupValueStream model uses accepts_nested_attributes_for when receiving stages (has many). The error
+    # messages will be mapped to the respective form fields on the frontend. To do so, the serializer adds the
+    # index of the stage (index from the incoming stages array) object in the response.
+    #
+    # Example error object:
+    #
+    # {
+    #   name: ["can't be blank"],
+    #   stages: {
+    #     "1": {
+    #       name: ["can't be blank"]
+    #     }
+    #   }
+    class ValueStreamErrorsSerializer
+      STAGE_ATTRIBUTE_REGEX = /stages\[(\d+)\]\.(.+)/.freeze
+
+      def initialize(value_stream)
+        @value_stream = value_stream
+      end
+
+      def as_json(options = {})
+        value_stream.errors.messages.each_with_object({}) do |(attribute, messages), errors|
+          # Parse out the indexed stage errors: "stages[1].name"
+          if attribute.to_s.start_with?('stages[')
+            attribute.match(STAGE_ATTRIBUTE_REGEX) do |matchdata|
+              index, stage_attribute_name = matchdata.captures
+              index = Integer(index)
+
+              errors['stages'] ||= {}
+              errors['stages'][index] ||= {}
+              errors['stages'][index][stage_attribute_name] = messages
+            end
+          else
+            errors[attribute.to_s] = messages
+          end
+        end
+      end
+
+      private
+
+      attr_reader :value_stream
+    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.dup)
+          @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
+            # workaround to properly index nested stage errors
+            # More info: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51623#note_490919557
+            value_stream.valid?(:context_to_validate_all_stages)
+            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
+
+        def persisted_stage_ids
+          @persisted_stage_ids ||= value_stream.stages.pluck_primary_key
+        end
+
+        def to_be_deleted?(processed_params, stage_id)
+          processed_params.has_key?(:stages_attributes) &&
+          processed_params[:stages_attributes].none? { |attrs| Integer(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,83 @@ 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_errors = json_response['payload']['errors']['stages']['0']
+          expect(stage_errors).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: 'updated name',
+            stages: [
+              {
+                id: stage.id,
+                name: 'updated stage name',
+                custom: true
+              }
+            ]
+          }
+        end
+
+        it 'returns a successful 200 response' do
+          put :update, params: { id: value_stream.id, group_id: group, value_stream: value_stream_params }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['name']).to eq('updated name')
+          expect(json_response['id']).to eq(value_stream.id)
+          expect(json_response['stages'].first['title']).to eq('updated stage 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
+
+  it 'does not contain stage errors' do
+    expect(subject).not_to have_key('stages')
+  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_errors = subject['stages'][0]
+      expect(stage_errors).not_to be_empty
+    end
+  end
+
+  describe '::STAGE_ATTRIBUTE_REGEX' do
+    let(:attribute) { '' }
+
+    subject do
+      attribute.match(described_class::STAGE_ATTRIBUTE_REGEX).captures
+    end
+
+    context 'extracts the index and the stage attribute name' do
+      let(:attribute) { 'stages[0].name' }
+
+      it { is_expected.to eq(%w[0 name]) }
+
+      context 'when large index is given' do
+        let(:attribute) { 'stages[11].name' }
+
+        it { is_expected.to eq(%w[11 name]) }
+      end
+    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[0].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 }
+
+  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 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[1].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