Merge branch 'fix-infinite-spinner-design-management' into 'master'

Resolve "Infinite spinner when trying to open a non-existent design"

See merge request gitlab-org/gitlab!30263

ee/app/assets/javascripts/design_management/pages/design/index.vue

@@ -31,7 +31,7 @@ import {
   ADD_IMAGE_DIFF_NOTE_ERROR,
   UPDATE_IMAGE_DIFF_NOTE_ERROR,
   DESIGN_NOT_FOUND_ERROR,
-  DESIGN_NOT_EXIST_ERROR,
+  DESIGN_VERSION_NOT_EXIST_ERROR,
   designDeletionError,
 } from '../../utils/error_messages';
 import { DESIGNS_ROUTE_NAME } from '../../router/constants';
@@ -84,18 +84,8 @@ export default {
         return this.designVariables;
       },
       update: data => extractDesign(data),
-      result({ data, loading }) {
-        // On the initial load with cache-and-network policy data is undefined while loading is true
-        // To prevent throwing an error, we don't perform any logic until loading is false
-        if (loading) {
-          return;
-        }
-        if (!data) {
-          this.onQueryError(DESIGN_NOT_FOUND_ERROR);
-        }
-        if (this.$route.query.version && !this.hasValidVersion) {
-          this.onQueryError(DESIGN_NOT_EXIST_ERROR);
-        }
+      result(res) {
+        this.onDesignQueryResult(res);
       },
       error() {
         this.onQueryError(DESIGN_NOT_FOUND_ERROR);
@@ -215,6 +205,19 @@ export default {
 
       return this.$apollo.mutate(mutationPayload).catch(e => this.onUpdateImageDiffNoteError(e));
     },
+    onDesignQueryResult({ data, loading }) {
+      // On the initial load with cache-and-network policy data is undefined while loading is true
+      // To prevent throwing an error, we don't perform any logic until loading is false
+      if (loading) {
+        return;
+      }
+
+      if (!data || !extractDesign(data)) {
+        this.onQueryError(DESIGN_NOT_FOUND_ERROR);
+      } else if (this.$route.query.version && !this.hasValidVersion) {
+        this.onQueryError(DESIGN_VERSION_NOT_EXIST_ERROR);
+      }
+    },
     onQueryError(message) {
       // because we redirect user to /designs (the issue page),
       // we want to create these flashes on the issue page

ee/app/assets/javascripts/design_management/utils/design_management_utils.js

@@ -39,7 +39,9 @@ export const findVersionId = id => (id.match('::Version/(.+$)') || [])[1];
 
 export const findNoteId = id => (id.match('DiffNote/(.+$)') || [])[1];
 
-export const extractDesign = data => data.project.issue.designCollection.designs.edges[0].node;
+export const extractDesigns = data => data.project.issue.designCollection.designs.edges;
+
+export const extractDesign = data => (extractDesigns(data) || [])[0]?.node;
 
 /**
  * Generates optimistic response for a design upload mutation

ee/app/assets/javascripts/design_management/utils/error_messages.js

@@ -20,9 +20,9 @@ export const UPLOAD_DESIGN_INVALID_FILETYPE_ERROR = __(
   'Could not upload your designs as one or more files uploaded are not supported.',
 );
 
-export const DESIGN_NOT_FOUND_ERROR = __('Could not find design');
+export const DESIGN_NOT_FOUND_ERROR = __('Could not find design.');
 
-export const DESIGN_NOT_EXIST_ERROR = __('Requested design version does not exist');
+export const DESIGN_VERSION_NOT_EXIST_ERROR = __('Requested design version does not exist.');
 
 const DESIGN_UPLOAD_SKIPPED_MESSAGE = s__('DesignManagement|Upload skipped.');
 

ee/changelogs/unreleased/fix-infinite-spinner-design-management.yml

+---
+title: Fix infinte loading spinner when visiting non-existent design
+merge_request: 30263
+author:
+type: fixed

ee/spec/frontend/design_management/mock_data/all_versions.js

+export default [
+  {
+    node: {
+      id: 'gid://gitlab/DesignManagement::Version/1',
+      sha: 'b389071a06c153509e11da1f582005b316667001',
+    },
+  },
+];

ee/spec/frontend/design_management/mock_data/designs.js

+import design from './design';
+
+export default {
+  project: {
+    issue: {
+      designCollection: {
+        designs: {
+          edges: [
+            {
+              node: design,
+            },
+          ],
+        },
+      },
+    },
+  },
+};

ee/spec/frontend/design_management/mock_data/no_designs.js

+export default {
+  project: {
+    issue: {
+      designCollection: {
+        designs: {
+          edges: [],
+        },
+      },
+    },
+  },
+};

ee/spec/frontend/design_management/pages/design/index_spec.js

 import { shallowMount } from '@vue/test-utils';
 import { GlAlert } from '@gitlab/ui';
 import { ApolloMutation } from 'vue-apollo';
+import createFlash from '~/flash';
 import DesignIndex from 'ee/design_management/pages/design/index.vue';
 import DesignDiscussion from 'ee/design_management/components/design_notes/design_discussion.vue';
 import DesignReplyForm from 'ee/design_management/components/design_notes/design_reply_form.vue';
 import Participants from '~/sidebar/components/participants/participants.vue';
 import createImageDiffNoteMutation from 'ee/design_management/graphql/mutations/createImageDiffNote.mutation.graphql';
 import design from '../../mock_data/design';
-
+import mockResponseWithDesigns from '../../mock_data/designs';
+import mockResponseNoDesigns from '../../mock_data/no_designs';
+import mockAllVersions from '../../mock_data/all_versions';
+import {
+  DESIGN_NOT_FOUND_ERROR,
+  DESIGN_VERSION_NOT_EXIST_ERROR,
+} from 'ee/design_management/utils/error_messages';
+import { DESIGNS_ROUTE_NAME } from 'ee/design_management/router/constants';
+
+jest.mock('~/flash');
 jest.mock('mousetrap', () => ({
   bind: jest.fn(),
   unbind: jest.fn(),
@@ -41,13 +51,14 @@ describe('Design management design index page', () => {
       },
     },
   };
-  const mutate = jest.fn(() => Promise.resolve());
+  const mutate = jest.fn().mockResolvedValue();
+  const routerPush = jest.fn();
 
   const findDiscussions = () => wrapper.findAll(DesignDiscussion);
   const findDiscussionForm = () => wrapper.find(DesignReplyForm);
   const findParticipants = () => wrapper.find(Participants);
 
-  function createComponent(loading = false) {
+  function createComponent(loading = false, { routeQuery = {} } = {}) {
     const $apollo = {
       queries: {
         design: {
@@ -57,9 +68,17 @@ describe('Design management design index page', () => {
       mutate,
     };
 
+    const $router = {
+      push: routerPush,
+    };
+
+    const $route = {
+      query: routeQuery,
+    };
+
     wrapper = shallowMount(DesignIndex, {
       propsData: { id: '1' },
-      mocks: { $apollo },
+      mocks: { $apollo, $router, $route },
       stubs: {
         ApolloMutation,
       },
@@ -75,6 +94,12 @@ describe('Design management design index page', () => {
     wrapper.vm.$apollo.queries.design.loading = false;
   }
 
+  function setDesignData() {
+    wrapper.setData({
+      design,
+    });
+  }
+
   afterEach(() => {
     wrapper.destroy();
   });
@@ -89,10 +114,7 @@ describe('Design management design index page', () => {
 
   it('renders design index', () => {
     setDesign();
-
-    wrapper.setData({
-      design,
-    });
+    setDesignData();
 
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.element).toMatchSnapshot();
@@ -102,10 +124,7 @@ describe('Design management design index page', () => {
 
   it('renders participants', () => {
     setDesign();
-
-    wrapper.setData({
-      design,
-    });
+    setDesignData();
 
     return wrapper.vm.$nextTick().then(() => {
       expect(findParticipants().exists()).toBe(true);
@@ -142,10 +161,7 @@ describe('Design management design index page', () => {
   describe('when has discussions', () => {
     beforeEach(() => {
       setDesign();
-
-      wrapper.setData({
-        design,
-      });
+      setDesignData();
     });
 
     it('renders correct amount of discussions', () => {
@@ -245,4 +261,38 @@ describe('Design management design index page', () => {
       expect(wrapper.element).toMatchSnapshot();
     });
   });
+
+  describe('onDesignQueryResult', () => {
+    describe('with no designs', () => {
+      it('redirects to /designs', () => {
+        createComponent(true);
+
+        wrapper.vm.onDesignQueryResult({ data: mockResponseNoDesigns, loading: false });
+        return wrapper.vm.$nextTick().then(() => {
+          expect(createFlash).toHaveBeenCalledTimes(1);
+          expect(createFlash).toHaveBeenCalledWith(DESIGN_NOT_FOUND_ERROR);
+          expect(routerPush).toHaveBeenCalledTimes(1);
+          expect(routerPush).toHaveBeenCalledWith({ name: DESIGNS_ROUTE_NAME });
+        });
+      });
+    });
+
+    describe('when no design exists for given version', () => {
+      it('redirects to /designs', () => {
+        // attempt to query for a version of the design that doesn't exist
+        createComponent(true, { routeQuery: { version: '999' } });
+        wrapper.setData({
+          allVersions: mockAllVersions,
+        });
+
+        wrapper.vm.onDesignQueryResult({ data: mockResponseWithDesigns, loading: false });
+        return wrapper.vm.$nextTick().then(() => {
+          expect(createFlash).toHaveBeenCalledTimes(1);
+          expect(createFlash).toHaveBeenCalledWith(DESIGN_VERSION_NOT_EXIST_ERROR);
+          expect(routerPush).toHaveBeenCalledTimes(1);
+          expect(routerPush).toHaveBeenCalledWith({ name: DESIGNS_ROUTE_NAME });
+        });
+      });
+    });
+  });
 });

ee/spec/frontend/design_management/utils/design_management_utils_spec.js

@@ -5,7 +5,11 @@ import {
   designUploadOptimisticResponse,
   updateImageDiffNoteOptimisticResponse,
   isValidDesignFile,
+  extractDesign,
 } from 'ee/design_management/utils/design_management_utils';
+import mockResponseNoDesigns from '../mock_data/no_designs';
+import mockResponseWithDesigns from '../mock_data/designs';
+import mockDesign from '../mock_data/design';
 
 jest.mock('lodash/uniqueId', () => () => 1);
 
@@ -152,7 +156,21 @@ describe('isValidDesignFile', () => {
     ${'video/mpeg'}               | ${false}
     ${'audio/midi'}               | ${false}
     ${'application/octet-stream'} | ${false}
-  `('returns $valid for file type $mimetype', ({ mimetype, isValid }) => {
+  `('returns $isValid for file type $mimetype', ({ mimetype, isValid }) => {
     expect(isValidDesignFile({ type: mimetype })).toBe(isValid);
   });
 });
+
+describe('extractDesign', () => {
+  describe('with no designs', () => {
+    it('returns undefined', () => {
+      expect(extractDesign(mockResponseNoDesigns)).toBeUndefined();
+    });
+  });
+
+  describe('with designs', () => {
+    it('returns the first design available', () => {
+      expect(extractDesign(mockResponseWithDesigns)).toEqual(mockDesign);
+    });
+  });
+});

locale/gitlab.pot

@@ -5992,7 +5992,7 @@ msgstr ""
 msgid "Could not delete chat nickname %{chat_name}."
 msgstr ""
 
-msgid "Could not find design"
+msgid "Could not find design."
 msgstr ""
 
 msgid "Could not remove the trigger."
@@ -17458,7 +17458,7 @@ msgstr ""
 msgid "Requested %{time_ago}"
 msgstr ""
 
-msgid "Requested design version does not exist"
+msgid "Requested design version does not exist."
 msgstr ""
 
 msgid "Requested states are invalid"