Merge branch '35546-child-epic-error-messages' into 'master'

Return meaningful error when adding one child epic at time and it fails

See merge request gitlab-org/gitlab!22688

app/assets/javascripts/lib/utils/http_status.js

@@ -19,6 +19,7 @@ const httpStatusCodes = {
   UNAUTHORIZED: 401,
   FORBIDDEN: 403,
   NOT_FOUND: 404,
+  CONFLICT: 409,
   GONE: 410,
   UNPROCESSABLE_ENTITY: 422,
   SERVICE_UNAVAILABLE: 503,

changelogs/unreleased/35546-child-epic-error-messages.yml

+---
+title: Improve error messages when adding a child epic
+merge_request: 22688
+author:
+type: fixed

ee/app/assets/javascripts/related_issues/components/add_issuable_form.vue

@@ -59,6 +59,11 @@ export default {
       required: false,
       default: itemAddFailureTypesMap.NOT_FOUND,
     },
+    itemAddFailureMessage: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   data() {
     return {
@@ -86,7 +91,9 @@ export default {
       );
     },
     addRelatedErrorMessage() {
-      if (this.itemAddFailureType === itemAddFailureTypesMap.NOT_FOUND) {
+      if (this.itemAddFailureMessage) {
+        return this.itemAddFailureMessage;
+      } else if (this.itemAddFailureType === itemAddFailureTypesMap.NOT_FOUND) {
         return addRelatedIssueErrorMap[this.issuableType];
       }
       // Only other failure is MAX_NUMBER_OF_CHILD_EPICS at the moment

ee/app/assets/javascripts/related_items_tree/components/related_items_tree_app.vue

@@ -50,6 +50,7 @@ export default {
       'itemAddInProgress',
       'itemAddFailure',
       'itemAddFailureType',
+      'itemAddFailureMessage',
       'itemCreateInProgress',
       'showAddItemForm',
       'showCreateEpicForm',
@@ -194,6 +195,7 @@ export default {
           :path-id-separator="itemPathIdSeparator"
           :has-error="itemAddFailure"
           :item-add-failure-type="itemAddFailureType"
+          :item-add-failure-message="itemAddFailureMessage"
           @pendingIssuableRemoveRequest="handlePendingItemRemove"
           @addIssuableFormInput="handleAddItemFormInput"
           @addIssuableFormBlur="handleAddItemFormBlur"

ee/app/assets/javascripts/related_items_tree/store/actions.js

@@ -307,8 +307,11 @@ export const receiveAddItemSuccess = ({ dispatch, commit, getters }, { rawItems
   dispatch('setItemInputValue', '');
   dispatch('toggleAddItemForm', { toggleState: false });
 };
-export const receiveAddItemFailure = ({ commit }, { itemAddFailureType } = {}) => {
-  commit(types.RECEIVE_ADD_ITEM_FAILURE, { itemAddFailureType });
+export const receiveAddItemFailure = (
+  { commit },
+  { itemAddFailureType, itemAddFailureMessage = '' } = {},
+) => {
+  commit(types.RECEIVE_ADD_ITEM_FAILURE, { itemAddFailureType, itemAddFailureMessage });
 };
 export const addItem = ({ state, dispatch, getters }) => {
   dispatch('requestAddItem');
@@ -325,20 +328,22 @@ export const addItem = ({ state, dispatch, getters }) => {
     })
     .catch(data => {
       const { response } = data;
-      if (response.status === 404) {
+      if (response.status === httpStatusCodes.NOT_FOUND) {
         dispatch('receiveAddItemFailure', { itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND });
       }
       // Ignore 409 conflict when the issue or epic is already attached to epic
       /* eslint-disable @gitlab/i18n/no-non-i18n-strings */
       else if (
-        response.status === 409 &&
+        response.status === httpStatusCodes.CONFLICT &&
         response.data.message === 'Epic hierarchy level too deep'
       ) {
         dispatch('receiveAddItemFailure', {
           itemAddFailureType: itemAddFailureTypesMap.MAX_NUMBER_OF_CHILD_EPICS,
         });
       } else {
-        dispatch('receiveAddItemFailure');
+        dispatch('receiveAddItemFailure', {
+          itemAddFailureMessage: response.data.message,
+        });
       }
     });
 };

ee/app/assets/javascripts/related_items_tree/store/mutations.js

@@ -172,9 +172,10 @@ export default {
     state.itemAddInProgress = false;
     state.itemsFetchResultEmpty = false;
   },
-  [types.RECEIVE_ADD_ITEM_FAILURE](state, { itemAddFailureType }) {
+  [types.RECEIVE_ADD_ITEM_FAILURE](state, { itemAddFailureType, itemAddFailureMessage }) {
     state.itemAddInProgress = false;
     state.itemAddFailure = true;
+    state.itemAddFailureMessage = itemAddFailureMessage;
     if (itemAddFailureType) {
       state.itemAddFailureType = itemAddFailureType;
     }

ee/app/assets/javascripts/related_items_tree/store/state.js

@@ -23,6 +23,7 @@ export default () => ({
   pendingReferences: [],
   itemAutoCompleteSources: {},
   itemAddFailureType: null,
+  itemAddFailureMessage: '',
 
   // UI Flags
   itemsFetchInProgress: false,

ee/app/models/ee/epic.rb

@@ -326,24 +326,30 @@ module EE
     def update_project_counter_caches
     end
 
-    # we call this when creating a new epic (Epics::CreateService) or linking an existing one (EpicLinks::CreateService)
-    # when called from EpicLinks::CreateService we pass
-    #   parent_epic - because we don't have parent attribute set on epic
-    #   parent_group_descendants - we have preloaded them in the service and we want to prevent performance problems
-    #     when linking a lot of issues
     def valid_parent?(parent_epic: nil, parent_group_descendants: nil)
-      parent_epic ||= parent
+      self.parent = parent_epic if parent_epic
 
-      return true unless parent_epic
+      validate_parent(parent_group_descendants)
 
-      parent_group_descendants ||= parent_epic.group.self_and_descendants
+      errors.empty?
+    end
+
+    def validate_parent(preloaded_parent_group_and_descendants = nil)
+      return unless parent
 
-      return false if self == parent_epic
-      return false if level_depth_exceeded?(parent_epic)
-      return false if parent_epic.has_ancestor?(self)
-      return false if parent_epic.children.to_a.include?(self)
+      preloaded_parent_group_and_descendants ||= parent.group.self_and_descendants
 
-      parent_group_descendants.include?(group)
+      if self == parent
+        errors.add :parent, 'Cannot add an epic as a child of itself'
+      elsif parent.children.to_a.include?(self)
+        errors.add :parent, "This epic can't be added as it is already assigned to the parent"
+      elsif parent.has_ancestor?(self)
+        errors.add :parent, "This epic can't be added as it is already assigned to this epic's ancestor"
+      elsif !preloaded_parent_group_and_descendants.include?(group)
+        errors.add :parent, "This epic can't be added because parent and child epics must belong to the same group"
+      elsif level_depth_exceeded?(parent)
+        errors.add :parent, "This epic can't be added as the maximum depth of nested epics would be exceeded"
+      end
     end
 
     def issues_readable_by(current_user, preload: nil)
@@ -364,13 +370,6 @@ module EE
       super.merge(label_url_method: :group_epics_url)
     end
 
-    def validate_parent
-      return true if valid_parent?
-
-      errors.add :parent, 'The parent is not valid'
-    end
-    private :validate_parent
-
     def level_depth_exceeded?(parent_epic)
       hierarchy.max_descendants_depth.to_i + parent_epic.base_and_ancestors.count >= MAX_HIERARCHY_DEPTH
     end

ee/app/services/epic_links/create_service.rb

@@ -4,14 +4,31 @@ module EpicLinks
   class CreateService < IssuableLinks::CreateService
     def execute
       if issuable.max_hierarchy_depth_achieved?
-        return error('Epic hierarchy level too deep', 409)
+        return error("This epic can't be added because the parent is already at the maximum depth from its most distant ancestor", 409)
       end
 
-      super
+      if referenced_issuables.count == 1
+        create_single_link
+      else
+        super
+      end
     end
 
     private
 
+    def create_single_link
+      child_epic = referenced_issuables.first
+
+      if linkable_epic?(child_epic)
+        set_child_epic!(child_epic)
+
+        create_notes(child_epic, nil)
+        success
+      else
+        error(child_epic.errors.messages[:parent].first, 409)
+      end
+    end
+
     def affected_epics(epics)
       [issuable, epics].flatten.uniq
     end

ee/spec/features/epics/epic_issues_spec.rb

@@ -199,7 +199,7 @@ describe 'Epic Issues', :js do
         add_epics(references)
 
         expect(page).to have_selector('.gl-field-error')
-        expect(find('.gl-field-error')).to have_text("This epic already has the maximum number of child epics.")
+        expect(find('.gl-field-error')).to have_text("This epic can't be added because the parent is already at the maximum depth from its most distant ancestor")
       end
     end
 

ee/spec/frontend/related_items_tree/store/mutations_spec.js

@@ -479,10 +479,14 @@ describe('RelatedItemsTree', () => {
 
       describe(types.RECEIVE_ADD_ITEM_FAILURE, () => {
         it('should set `itemAddInProgress` to false, `itemAddFailure` to true and `itemAddFailureType` value within state', () => {
-          mutations[types.RECEIVE_ADD_ITEM_FAILURE](state, { itemAddFailureType: 'bar' });
+          mutations[types.RECEIVE_ADD_ITEM_FAILURE](state, {
+            itemAddFailureMessage: 'foo',
+            itemAddFailureType: 'bar',
+          });
 
           expect(state.itemAddInProgress).toBe(false);
           expect(state.itemAddFailure).toBe(true);
+          expect(state.itemAddFailureMessage).toEqual('foo');
           expect(state.itemAddFailureType).toEqual('bar');
         });
       });

ee/spec/javascripts/issuable/related_issues/components/add_issuable_form_spec.js

@@ -209,6 +209,20 @@ describe('AddIssuableForm', () => {
           done();
         });
       });
+
+      it('shows error message when error is present', done => {
+        const itemAddFailureMessage = 'Something went wrong while submitting.';
+        wrapper.setProps({
+          hasError: true,
+          itemAddFailureMessage,
+        });
+
+        wrapper.vm.$nextTick(() => {
+          expect(wrapper.find('.gl-field-error').exists()).toBe(true);
+          expect(wrapper.find('.gl-field-error').text()).toContain(itemAddFailureMessage);
+          done();
+        });
+      });
     });
   });
 });

ee/spec/javascripts/related_items_tree/store/actions_spec.js

@@ -919,12 +919,18 @@ describe('RelatedItemTree', () => {
         it('should set `state.itemAddInProgress` to false', done => {
           testAction(
             actions.receiveAddItemFailure,
-            { itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND },
+            {
+              itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND,
+              itemAddFailureMessage: 'Foobar',
+            },
             {},
             [
               {
                 type: types.RECEIVE_ADD_ITEM_FAILURE,
-                payload: { itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND },
+                payload: {
+                  itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND,
+                  itemAddFailureMessage: 'Foobar',
+                },
               },
             ],
             [],
@@ -940,7 +946,7 @@ describe('RelatedItemTree', () => {
             [
               {
                 type: types.RECEIVE_ADD_ITEM_FAILURE,
-                payload: { itemAddFailureType: undefined },
+                payload: { itemAddFailureType: undefined, itemAddFailureMessage: '' },
               },
             ],
             [],
@@ -1004,7 +1010,9 @@ describe('RelatedItemTree', () => {
               },
               {
                 type: 'receiveAddItemFailure',
-                payload: { itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND },
+                payload: {
+                  itemAddFailureType: itemAddFailureTypesMap.NOT_FOUND,
+                },
               },
             ],
             done,

ee/spec/services/epic_links/create_service_spec.rb

@@ -8,6 +8,8 @@ describe EpicLinks::CreateService do
     let(:user) { create(:user) }
     let(:epic) { create(:epic, group: group) }
     let(:epic_to_add) { create(:epic, group: group) }
+    let(:expected_error) { 'No Epic found for given params' }
+    let(:expected_code) { 404 }
 
     let(:valid_reference) { epic_to_add.to_reference(full: true) }
 
@@ -37,9 +39,9 @@ describe EpicLinks::CreateService do
       end
     end
 
-    shared_examples 'returns not found error' do
+    shared_examples 'returns an error' do
       it 'returns an error' do
-        expect(subject).to eq(message: 'No Epic found for given params', status: :error, http_status: 404)
+        expect(subject).to eq(message: expected_error, status: :error, http_status: expected_code)
       end
 
       it 'no relationship is created' do
@@ -56,7 +58,7 @@ describe EpicLinks::CreateService do
     context 'when epics feature is disabled' do
       subject { add_epic([valid_reference]) }
 
-      include_examples 'returns not found error'
+      include_examples 'returns an error'
     end
 
     context 'when epics feature is enabled' do
@@ -64,78 +66,201 @@ describe EpicLinks::CreateService do
         stub_licensed_features(epics: true)
       end
 
-      context 'when user has permissions to link the issue' do
-        before do
-          group.add_developer(user)
-        end
+      context 'when an error occurs' do
+        context 'when a single epic is given' do
+          subject { add_epic([valid_reference]) }
 
-        context 'when the reference list is empty' do
-          subject { add_epic([]) }
+          context 'when an epic from a another group is given' do
+            let(:other_group) { create(:group) }
+            let(:expected_error) { "This epic can't be added because parent and child epics must belong to the same group" }
+            let(:expected_code) { 409 }
 
-          include_examples 'returns not found error'
-        end
+            before do
+              epic_to_add.update!(group: other_group)
+            end
 
-        context 'when a correct reference is given' do
-          subject { add_epic([valid_reference]) }
+            include_examples 'returns an error'
+          end
 
-          include_examples 'returns success'
-          include_examples 'system notes created'
-        end
+          context 'when hierarchy is cyclic' do
+            context 'when given child epic is the same as given parent' do
+              let(:expected_error) { 'Cannot add an epic as a child of itself' }
+              let(:expected_code) { 409 }
 
-        context 'when an epic from a subgroup is given' do
-          let(:subgroup) { create(:group, parent: group) }
+              subject { add_epic([epic.to_reference(full: true)]) }
 
-          before do
-            epic_to_add.update!(group: subgroup)
+              include_examples 'returns an error'
+            end
+
+            context 'when given child epic is parent of the given parent' do
+              let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
+              let(:expected_code) { 409 }
+
+              before do
+                epic.update(parent: epic_to_add)
+              end
+
+              include_examples 'returns an error'
+            end
+
+            context 'when new child epic is an ancestor of the given parent' do
+              let(:expected_error) { "This epic can't be added as it is already assigned to this epic's ancestor" }
+              let(:expected_code) { 409 }
+
+              before do
+                # epic_to_add -> epic1 -> epic2 -> epic
+                epic1 = create(:epic, group: group, parent: epic_to_add)
+                epic2 = create(:epic, group: group, parent: epic1)
+                epic.update(parent: epic2)
+              end
+
+              include_examples 'returns an error'
+            end
           end
 
-          subject { add_epic([valid_reference]) }
+          context 'when adding an epic that is already a child of the parent epic' do
+            before do
+              epic_to_add.update(parent: epic)
+            end
 
-          include_examples 'returns success'
-          include_examples 'system notes created'
-        end
+            let(:expected_error) { "This epic can't be added as it is already assigned to the parent" }
+            let(:expected_code) { 409 }
 
-        context 'when an epic from a another group is given' do
-          let(:other_group) { create(:group) }
+            include_examples 'returns an error'
+          end
 
-          before do
-            epic_to_add.update!(group: other_group)
+          context 'when adding to an Epic that is already at maximum depth' do
+            before do
+              epic1 = create(:epic, group: group)
+              epic2 = create(:epic, group: group, parent: epic1)
+              epic3 = create(:epic, group: group, parent: epic2)
+              epic4 = create(:epic, group: group, parent: epic3)
+
+              epic.update(parent: epic4)
+            end
+
+            let(:expected_error) { "This epic can't be added because the parent is already at the maximum depth from its most distant ancestor" }
+            let(:expected_code) { 409 }
+
+            include_examples 'returns an error'
           end
 
-          subject { add_epic([valid_reference]) }
+          context 'when total depth after adding would exceed limit' do
+            let(:expected_error) { "This epic can't be added as the maximum depth of nested epics would be exceeded" }
+            let(:expected_code) { 409 }
+
+            before do
+              epic1 = create(:epic, group: group)
+
+              epic.update(parent: epic1) # epic is on level 2
+
+              # epic_to_add has 3 children (level 4 including epic_to_add)
+              # that would mean level 6 after relating epic_to_add on epic
+              epic2 = create(:epic, group: group, parent: epic_to_add)
+              epic3 = create(:epic, group: group, parent: epic2)
+              create(:epic, group: group, parent: epic3)
+            end
 
-          include_examples 'returns not found error'
+            include_examples 'returns an error'
+          end
         end
 
-        context 'when hierarchy is cyclic' do
-          context 'when given child epic is the same as given parent' do
-            subject { add_epic([epic.to_reference(full: true)]) }
+        context 'when multiple epics are given' do
+          let(:another_epic) { create(:epic) }
+
+          subject do
+            add_epic(
+              [epic_to_add.to_reference(full: true), another_epic.to_reference(full: true)]
+            )
+          end
 
-            include_examples 'returns not found error'
+          context 'when adding epics that are already a child of the parent epic' do
+            let(:expected_error) { 'Epic(s) already assigned' }
+            let(:expected_code) { 409 }
+
+            before do
+              epic_to_add.update(parent: epic)
+              another_epic.update(parent: epic)
+            end
+
+            include_examples 'returns an error'
           end
 
-          context 'when given child epic is parent of the given parent' do
+          context 'when total depth after adding would exceed limit' do
             before do
-              epic.update(parent: epic_to_add)
+              epic1 = create(:epic, group: group)
+
+              epic.update(parent: epic1) # epic is on level 2
+
+              # epic_to_add has 3 children (level 4 including epic_to_add)
+              # that would mean level 6 after relating epic_to_add on epic
+              epic2 = create(:epic, group: group, parent: epic_to_add)
+              epic3 = create(:epic, group: group, parent: epic2)
+              create(:epic, group: group, parent: epic3)
             end
 
-            subject { add_epic([valid_reference]) }
+            let(:another_epic) { create(:epic) }
 
-            include_examples 'returns not found error'
+            include_examples 'returns an error'
           end
 
-          context 'when new child epic is an ancestor of the given parent' do
+          context 'when an epic from a another group is given' do
+            let(:other_group) { create(:group) }
+
             before do
-              # epic_to_add -> epic1 -> epic2 -> epic
-              epic1 = create(:epic, group: group, parent: epic_to_add)
-              epic2 = create(:epic, group: group, parent: epic1)
-              epic.update(parent: epic2)
+              epic_to_add.update!(group: other_group)
             end
 
-            subject { add_epic([valid_reference]) }
+            include_examples 'returns an error'
+          end
+
+          context 'when hierarchy is cyclic' do
+            context 'when given child epic is the same as given parent' do
+              subject { add_epic([epic.to_reference(full: true), another_epic.to_reference(full: true)]) }
+
+              include_examples 'returns an error'
+            end
+
+            context 'when given child epic is parent of the given parent' do
+              before do
+                epic.update(parent: epic_to_add)
+              end
+
+              include_examples 'returns an error'
+            end
+          end
+        end
+      end
+
+      context 'when user has permissions to link the epic' do
+        before do
+          group.add_developer(user)
+        end
+
+        context 'when the reference list is empty' do
+          subject { add_epic([]) }
 
-            include_examples 'returns not found error'
+          include_examples 'returns an error'
+        end
+
+        context 'when a correct reference is given' do
+          subject { add_epic([valid_reference]) }
+
+          include_examples 'returns success'
+          include_examples 'system notes created'
+        end
+
+        context 'when an epic from a subgroup is given' do
+          let(:subgroup) { create(:group, parent: group) }
+
+          before do
+            epic_to_add.update!(group: subgroup)
           end
+
+          subject { add_epic([valid_reference]) }
+
+          include_examples 'returns success'
+          include_examples 'system notes created'
         end
 
         context 'when multiple valid epics are given' do
@@ -206,63 +331,10 @@ describe EpicLinks::CreateService do
           end
         end
 
-        context 'when adding an epic that is already a child of the parent epic' do
-          before do
-            epic_to_add.update(parent: epic)
-          end
-
-          subject { add_epic([valid_reference]) }
-
-          it 'returns an error' do
-            expect(subject).to eq(message: 'Epic(s) already assigned', status: :error, http_status: 409)
-          end
-
-          it 'no relationship is created' do
-            expect { subject }.not_to change { epic.children.count }
-          end
-        end
-
-        context 'when adding to an Epic that is already at maximum depth' do
-          before do
-            epic1 = create(:epic, group: group)
-            epic2 = create(:epic, group: group, parent: epic1)
-            epic3 = create(:epic, group: group, parent: epic2)
-            epic4 = create(:epic, group: group, parent: epic3)
-
-            epic.update(parent: epic4)
-          end
-
-          subject { add_epic([valid_reference]) }
-
-          it 'returns an error' do
-            expect(subject).to eq(message: 'Epic hierarchy level too deep', status: :error, http_status: 409)
-          end
-
-          it 'no relationship is created' do
-            expect { subject }.not_to change { epic.children.count }
-          end
-        end
-
         context 'when adding an Epic that has existing children' do
-          subject { add_epic([valid_reference]) }
-
-          context 'when total depth after adding would exceed limit' do
-            before do
-              epic1 = create(:epic, group: group)
-
-              epic.update(parent: epic1) # epic is on level 2
-
-              # epic_to_add has 3 children (level 4 including epic_to_add)
-              # that would mean level 6 after relating epic_to_add on epic
-              epic2 = create(:epic, group: group, parent: epic_to_add)
-              epic3 = create(:epic, group: group, parent: epic2)
-              create(:epic, group: group, parent: epic3)
-            end
-
-            include_examples 'returns not found error'
-          end
-
           context 'when Epic to add has more than 5 children' do
+            subject { add_epic([valid_reference]) }
+
             before do
               create_list(:epic, 8, group: group, parent: epic_to_add)
             end