Use index_errors when serializing stage errors

Use index_errors when serializing stage errors

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

@@ -23,7 +23,8 @@ class Groups::Analytics::CycleAnalytics::ValueStreamsController < Groups::Analyt
   end
 
   def update
-    result = Analytics::CycleAnalytics::ValueStreams::UpdateService.new(group: @group, params: update_params, current_user: current_user).execute
+    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

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

@@ -3,7 +3,7 @@
 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 }

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

@@ -4,61 +4,47 @@ 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
+    # 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: [
-    #     {
-    #       id: nil,
-    #       name: "",
-    #       errors: {
+    #   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 = {})
-        # skip all errors related to the associated stages
-        errors = value_stream.errors.messages.reject do |attribute, messages|
-          attribute.to_s.start_with?("stages.")
+        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
-
-        # 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

@@ -9,7 +9,7 @@ module Analytics
         def initialize(group:, params:, current_user:, value_stream: ::Analytics::CycleAnalytics::GroupValueStream.new(group: group))
           @value_stream = value_stream
           @group = group
-          @params = process_params(params)
+          @params = process_params(params.dup)
           @current_user = current_user
         end
 
@@ -22,6 +22,9 @@ module Analytics
           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

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

@@ -20,14 +20,13 @@ module Analytics
           processed_params
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def persisted_stage_ids
-          @persisted_stage_ids ||= value_stream.stages.pluck(:id)
+          @persisted_stage_ids ||= value_stream.stages.pluck_primary_key
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         def to_be_deleted?(processed_params, stage_id)
-          processed_params[:stages_attributes].none? { |attrs| attrs[:id] == stage_id }
+          processed_params.has_key?(:stages_attributes) &&
+          processed_params[:stages_attributes].none? { |attrs| Integer(attrs[:id]) == stage_id }
         end
 
         def success_http_status

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

@@ -93,8 +93,8 @@ RSpec.describe Groups::Analytics::CycleAnalytics::ValueStreamsController 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
+          stage_errors = json_response['payload']['errors']['stages']['0']
+          expect(stage_errors).to be_present
         end
       end
     end
@@ -116,11 +116,11 @@ RSpec.describe Groups::Analytics::CycleAnalytics::ValueStreamsController do
 
         let(:value_stream_params) do
           {
-            name: 'test',
+            name: 'updated name',
             stages: [
               {
                 id: stage.id,
-                name: 'new stage name',
+                name: 'updated stage name',
                 custom: true
               }
             ]
@@ -128,10 +128,12 @@ RSpec.describe Groups::Analytics::CycleAnalytics::ValueStreamsController do
         end
 
         it 'returns a successful 200 response' do
-          put :update, params: { id: value_stream.id, group_id: group, value_stream: { name: 'new name' } }
+          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('new name')
+          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

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

@@ -13,7 +13,11 @@ RSpec.describe Analytics::CycleAnalytics::ValueStreamErrorsSerializer do
 
     value_stream.validate
 
-    expect(subject[:name]).not_to be_empty
+    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
@@ -28,8 +32,28 @@ RSpec.describe Analytics::CycleAnalytics::ValueStreamErrorsSerializer do
     it 'serializes error on value stream object' do
       value_stream.validate
 
-      stage = subject[:stages].first
-      expect(stage[:errors]).not_to be_empty
+      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

@@ -54,7 +54,7 @@ RSpec.describe Analytics::CycleAnalytics::ValueStreams::CreateService do
           params[:stages].first[:name] = ''
 
           errors = subject.payload[:errors].details
-          expect(errors[:'stages.name']).to eq([{ error: :blank }])
+          expect(errors[:'stages[0].name']).to eq([{ error: :blank }])
         end
       end
 

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

@@ -20,6 +20,8 @@ RSpec.describe Analytics::CycleAnalytics::ValueStreams::UpdateService do
 
   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)
@@ -59,7 +61,7 @@ RSpec.describe Analytics::CycleAnalytics::ValueStreams::UpdateService do
         it 'returns error' do
           expect(subject).to be_error
           errors = subject.payload[:errors].details
-          expect(errors[:'stages.name']).to eq([{ error: :blank }])
+          expect(errors[:'stages[1].name']).to eq([{ error: :blank }])
         end
       end
     end