Merge branch...

Merge branch '34408-reducing-the-data-being-loaded-per-design-on-first-load-of-design-management' into 'master'

[FE] Reduce the data being loaded per Design on first load

See merge request gitlab-org/gitlab!19212

doc/api/graphql/reference/gitlab_schema.graphql

@@ -539,7 +539,7 @@ type DesignCollection {
 
     """
     Filters designs to only those that existed at the version. If argument is
-    omitted or nil then all designs will reflect the latest version.
+    omitted or nil then all designs will reflect the latest version
     """
     atVersion: ID
 
@@ -548,13 +548,18 @@ type DesignCollection {
     """
     before: String
 
+    """
+    Filters designs by their filename
+    """
+    filenames: [String!]
+
     """
     Returns the first _n_ elements from the list.
     """
     first: Int
 
     """
-    The list of IDs of designs.
+    Filters designs by their ID
     """
     ids: [ID!]
 

doc/api/graphql/reference/gitlab_schema.json

@@ -7979,7 +7979,7 @@
               "args": [
                 {
                   "name": "ids",
-                  "description": "The list of IDs of designs.",
+                  "description": "Filters designs by their ID",
                   "type": {
                     "kind": "LIST",
                     "name": null,
@@ -7995,9 +7995,27 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "filenames",
+                  "description": "Filters designs by their filename",
+                  "type": {
+                    "kind": "LIST",
+                    "name": null,
+                    "ofType": {
+                      "kind": "NON_NULL",
+                      "name": null,
+                      "ofType": {
+                        "kind": "SCALAR",
+                        "name": "String",
+                        "ofType": null
+                      }
+                    }
+                  },
+                  "defaultValue": null
+                },
                 {
                   "name": "atVersion",
-                  "description": "Filters designs to only those that existed at the version. If argument is omitted or nil then all designs will reflect the latest version.",
+                  "description": "Filters designs to only those that existed at the version. If argument is omitted or nil then all designs will reflect the latest version",
                   "type": {
                     "kind": "SCALAR",
                     "name": "ID",

ee/app/assets/javascripts/design_management/components/design_destroyer.vue

 <script>
+import { __, s__, sprintf } from '~/locale';
+import createFlash from '~/flash';
 import { ApolloMutation } from 'vue-apollo';
 import projectQuery from '../graphql/queries/project.query.graphql';
 import destroyDesignMutation from '../graphql/mutations/destroyDesign.mutation.graphql';
-import {
-  updateStoreAfterDesignsDelete,
-  onDesignDeletionError,
-} from '../utils/design_management_utils';
+import { updateStoreAfterDesignsDelete } from '../utils/design_management_utils';
 
 export default {
   components: {
@@ -34,8 +33,13 @@ export default {
     },
   },
   methods: {
-    onError(...args) {
-      onDesignDeletionError(...args);
+    onError() {
+      const design = this.filenames.length > 1 ? __('designs') : __('a design');
+      createFlash(
+        sprintf(s__('DesignManagement|We could not delete %{design}. Please try again.'), {
+          design,
+        }),
+      );
     },
     updateStoreAfterDelete(
       store,

ee/app/assets/javascripts/design_management/components/design_notes/design_discussion.vue

@@ -8,7 +8,7 @@ import createNoteMutation from '../../graphql/mutations/createNote.mutation.grap
 import getDesignQuery from '../../graphql/queries/getDesign.query.graphql';
 import DesignNote from './design_note.vue';
 import DesignReplyForm from './design_reply_form.vue';
-import { extractCurrentDiscussion } from '../../utils/design_management_utils';
+import { extractCurrentDiscussion, extractDesign } from '../../utils/design_management_utils';
 
 export default {
   components: {
@@ -55,6 +55,14 @@ export default {
         discussionId: this.discussion.id,
       };
     },
+    designVariables() {
+      return {
+        fullPath: this.projectPath,
+        iid: this.issueIid,
+        filenames: [this.$route.params.id],
+        atVersion: this.designsVersion,
+      };
+    },
   },
   methods: {
     addDiscussionComment(
@@ -65,54 +73,29 @@ export default {
     ) {
       const data = store.readQuery({
         query: getDesignQuery,
-        variables: {
-          id: this.designId,
-          version: this.designsVersion,
-        },
+        variables: this.designVariables,
       });
-      const currentDiscussion = extractCurrentDiscussion(
-        data.design.discussions,
-        this.discussion.id,
-      );
 
-      const updatedDiscussion = {
-        ...currentDiscussion,
-        node: {
-          ...currentDiscussion.node,
-          notes: {
-            ...currentDiscussion.node.notes,
-            edges: [
+      const design = extractDesign(data);
+      const currentDiscussion = extractCurrentDiscussion(design.discussions, this.discussion.id);
+      currentDiscussion.node.notes.edges = [
         ...currentDiscussion.node.notes.edges,
-              { __typename: 'NoteEdge', node: createNote.note },
-            ],
-          },
+        {
+          __typename: 'NoteEdge',
+          node: createNote.note,
         },
-      };
-
-      const currentDiscussionIndex = data.design.discussions.edges.indexOf(currentDiscussion);
+      ];
 
-      const payload = {
+      design.notesCount += 1;
+      store.writeQuery({
+        query: getDesignQuery,
+        variables: this.designVariables,
+        data: {
           ...data,
           design: {
-          ...data.design,
-          discussions: {
-            ...data.design.discussions,
-            edges: [
-              ...data.design.discussions.edges.slice(0, currentDiscussionIndex),
-              updatedDiscussion,
-              ...data.design.discussions.edges.slice(
-                currentDiscussionIndex + 1,
-                data.design.discussions.edges.length,
-              ),
-            ],
-          },
-          notesCount: data.design.notesCount + 1,
+            ...design,
+          },
         },
-      };
-
-      store.writeQuery({
-        query: getDesignQuery,
-        data: payload,
       });
     },
     onDone() {

ee/app/assets/javascripts/design_management/graphql/fragments/design.fragment.graphql

+#import "./designNote.fragment.graphql"
+#import "./designList.fragment.graphql"
+#import "./diffRefs.fragment.graphql"
+
+fragment DesignItem on Design {
+  ...DesignListItem
+  fullPath
+  diffRefs {
+    ...DesignDiffRefs
+  }
+  discussions {
+    edges {
+      node {
+        id
+        replyId
+        notes {
+          edges {
+            node {
+              ...DesignNote
+            }
+          }
+        }
+      }
+    }
+  }
+}

ee/app/assets/javascripts/design_management/graphql/fragments/designList.fragment.graphql

-#import "./designNote.fragment.graphql"
-#import "./diffRefs.fragment.graphql"
-
 fragment DesignListItem on Design {
   id
   image
   event
   filename
-  fullPath
-  diffRefs {
-    ...DesignDiffRefs
-  }
   notesCount
-  discussions {
-    edges {
-      node {
-        id
-        replyId
-        notes {
-          edges {
-            node {
-              ...DesignNote
-            }
-          }
-        }
-      }
-    }
-  }
 }

ee/app/assets/javascripts/design_management/graphql/mutations/uploadDesign.mutation.graphql

-#import "../fragments/designList.fragment.graphql"
+#import "../fragments/design.fragment.graphql"
 
 mutation uploadDesign($files: [Upload!]!, $projectPath: ID!, $iid: ID!) {
   designManagementUpload(input: { projectPath: $projectPath, iid: $iid, files: $files }) {
     designs {
-      ...DesignListItem
+      ...DesignItem
       versions {
         edges {
           node {

ee/app/assets/javascripts/design_management/graphql/queries/getDesign.query.graphql

-#import "../fragments/designList.fragment.graphql"
+#import "../fragments/design.fragment.graphql"
 
-query getDesign($id: String!, $version: String) {
-  design(id: $id, version: $version) @client {
-    ...DesignListItem
+query getDesign($fullPath: ID!, $iid: String!, $atVersion: ID, $filenames: [String!]) {
+  project(fullPath: $fullPath) {
+    id
+    issue(iid: $iid) {
+      designCollection {
+        designs(atVersion: $atVersion, filenames: $filenames) {
+          edges {
+            node {
+              ...DesignItem
+            }
+          }
+        }
+      }
+    }
   }
 }

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

@@ -11,8 +11,9 @@ import DesignDiscussion from '../../components/design_notes/design_discussion.vu
 import DesignReplyForm from '../../components/design_notes/design_reply_form.vue';
 import DesignDestroyer from '../../components/design_destroyer.vue';
 import getDesignQuery from '../../graphql/queries/getDesign.query.graphql';
+import appDataQuery from '../../graphql/queries/appData.query.graphql';
 import createImageDiffNoteMutation from '../../graphql/mutations/createImageDiffNote.mutation.graphql';
-import { extractDiscussions } from '../../utils/design_management_utils';
+import { extractDiscussions, extractDesign } from '../../utils/design_management_utils';
 
 export default {
   components: {
@@ -41,18 +42,25 @@ export default {
         height: 0,
       },
       projectPath: '',
+      issueId: '',
       isNoteSaving: false,
     };
   },
   apollo: {
+    appData: {
+      query: appDataQuery,
+      manual: true,
+      result({ data: { projectPath, issueIid } }) {
+        this.projectPath = projectPath;
+        this.issueIid = issueIid;
+      },
+    },
     design: {
       query: getDesignQuery,
       variables() {
-        return {
-          id: this.id,
-          version: this.designsVersion,
-        };
+        return this.designVariables;
       },
+      update: data => extractDesign(data),
       result({ data }) {
         if (!data) {
           createFlash(s__('DesignManagement|Could not find design, please try again.'));
@@ -84,6 +92,14 @@ export default {
     renderDiscussions() {
       return this.discussions.length || this.annotationCoordinates;
     },
+    designVariables() {
+      return {
+        fullPath: this.projectPath,
+        iid: this.issueIid,
+        filenames: [this.$route.params.id],
+        atVersion: this.designsVersion,
+      };
+    },
   },
   mounted() {
     Mousetrap.bind('esc', this.closeDesign);
@@ -119,10 +135,7 @@ export default {
           update: (store, { data: { createImageDiffNote } }) => {
             const data = store.readQuery({
               query: getDesignQuery,
-              variables: {
-                id: this.id,
-                version: this.designsVersion,
-              },
+              variables: this.designVariables,
             });
             const newDiscussion = {
               __typename: 'DiscussionEdge',
@@ -143,9 +156,20 @@ export default {
                 },
               },
             };
-            data.design.discussions.edges.push(newDiscussion);
-            data.design.notesCount += 1;
-            store.writeQuery({ query: getDesignQuery, data });
+            const design = extractDesign(data);
+            design.discussions.edges = [...design.discussions.edges, newDiscussion];
+            design.notesCount += 1;
+            store.writeQuery({
+              query: getDesignQuery,
+              variables: this.designVariables,
+              data: {
+                ...data,
+                design: {
+                  ...design,
+                  notesCount: design.notesCount + 1,
+                },
+              },
+            });
           },
         })
         .then(() => {

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

@@ -47,6 +47,13 @@ const deleteDesignsFromStore = (store, query, selectedDesigns) => {
   });
 };
 
+/**
+ * Adds a new version of designs to store
+ *
+ * @param {Object} store
+ * @param {Object} query
+ * @param {Object} version
+ */
 const addNewVersionToStore = (store, query, version) => {
   if (!version) return;
 
@@ -64,14 +71,19 @@ const addNewVersionToStore = (store, query, version) => {
   });
 };
 
-export const onDesignDeletionError = e => {
-  createFlash(s__('DesignManagement|We could not delete design(s). Please try again.'));
-  throw e;
-};
+/**
+ * Updates a store after design deletion
+ *
+ * @param {Object} store
+ * @param {Object} data
+ * @param {Object} query
+ * @param {Array} designs
+ */
 
 export const updateStoreAfterDesignsDelete = (store, data, query, designs) => {
   if (data.errors) {
-    onDesignDeletionError(new Error(data.errors));
+    createFlash(s__('DesignManagement|We could not delete design(s). Please try again.'));
+    throw new Error(data.errors);
   } else {
     deleteDesignsFromStore(store, query, designs);
     addNewVersionToStore(store, query, data.version);
@@ -79,3 +91,5 @@ export const updateStoreAfterDesignsDelete = (store, data, query, designs) => {
 };
 
 export const findVersionId = id => id.match('::Version/(.+$)')[1];
+
+export const extractDesign = data => data.project.issue.designCollection.designs.edges[0].node;

ee/app/finders/design_management/designs_finder.rb

@@ -17,7 +17,8 @@ module DesignManagement
       items = init_collection
 
       items = by_visible_at_version(items)
-      items = by_ids(items)
+      items = by_filename(items)
+      items = by_id(items)
 
       items
     end
@@ -37,7 +38,13 @@ module DesignManagement
       items.visible_at_version(params[:visible_at_version])
     end
 
-    def by_ids(items)
+    def by_filename(items)
+      return items unless params[:filenames].present?
+
+      items.with_filename(params[:filenames])
+    end
+
+    def by_id(items)
       return items unless params[:ids].present?
 
       items.id_in(params[:ids])

ee/app/graphql/resolvers/design_management/design_resolver.rb

@@ -3,12 +3,19 @@
 module Resolvers
   module DesignManagement
     class DesignResolver < BaseResolver
-      argument :ids, [GraphQL::ID_TYPE], required: false, description: 'The list of IDs of designs.'
+      argument :ids,
+               [GraphQL::ID_TYPE],
+               required: false,
+               description: 'Filters designs by their ID'
+      argument :filenames,
+               [GraphQL::STRING_TYPE],
+               required: false,
+               description: 'Filters designs by their filename'
       argument :at_version,
                GraphQL::ID_TYPE,
                required: false,
                description: 'Filters designs to only those that existed at the version. ' \
-                            'If argument is omitted or nil then all designs will reflect the latest version.'
+                            'If argument is omitted or nil then all designs will reflect the latest version'
 
       def resolve(**args)
         find_designs(args)
@@ -27,6 +34,7 @@ module Resolvers
           object.issue,
           context[:current_user],
           ids: design_ids(args),
+          filenames: args[:filenames],
           visible_at_version: version(args)
         ).execute
       end

ee/app/models/design_management/design.rb

@@ -49,6 +49,8 @@ module DesignManagement
       joins(join.join_sources).where(actions[:event].not_eq(deletion)).order(:id)
     end
 
+    scope :with_filename, -> (filenames) { where(filename: filenames) }
+
     # Scope called by our REST API to avoid N+1 problems
     scope :with_api_entity_associations, -> { preload(:issue) }
 

ee/spec/finders/design_management/designs_finder_spec.rb

@@ -5,11 +5,12 @@ require 'spec_helper'
 describe DesignManagement::DesignsFinder do
   include DesignManagementTestHelpers
 
-  set(:user) { create(:user) }
-  set(:project) { create(:project, :private) }
-  set(:issue) { create(:issue, project: project) }
-  set(:design1) { create(:design, :with_file, issue: issue, versions_count: 1) }
-  set(:design2) { create(:design, :with_file, issue: issue, versions_count: 1) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :private) }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:design1) { create(:design, :with_file, issue: issue, versions_count: 1) }
+  let_it_be(:design2) { create(:design, :with_file, issue: issue, versions_count: 1) }
+  let_it_be(:design3) { create(:design, :with_file, issue: issue, versions_count: 1) }
   let(:params) { {} }
 
   subject(:designs) { described_class.new(issue, user, params).execute }
@@ -38,13 +39,25 @@ describe DesignManagement::DesignsFinder do
         end
 
         it 'returns the designs' do
-          is_expected.to contain_exactly(design2, design1)
+          is_expected.to contain_exactly(design1, design2, design3)
+        end
+
+        context 'when argument is the ids of designs' do
+          let(:params) { { ids: [design1.id] } }
+
+          it { is_expected.to eq([design1]) }
+        end
+
+        context 'when argument is the filenames of designs' do
+          let(:params) { { filenames: [design2.filename] } }
+
+          it { is_expected.to eq([design2]) }
         end
 
         describe 'returning designs that existed at a particular given version' do
           let(:all_versions) { issue.design_collection.versions.ordered }
           let(:first_version) { all_versions.last }
-          let(:second_version) { all_versions.first }
+          let(:second_version) { all_versions.second }
 
           context 'when argument is the first version' do
             let(:params) { { visible_at_version: first_version } }
@@ -52,12 +65,6 @@ describe DesignManagement::DesignsFinder do
             it { is_expected.to eq([design1]) }
           end
 
-          context 'when argument is the ids of designs' do
-            let(:params) { { ids: [design1.id] } }
-
-            it { is_expected.to eq([design1]) }
-          end
-
           context 'when arguments are version and id' do
             context 'when id is absent at version' do
               let(:params) { { visible_at_version: first_version, ids: [design2.id] } }
@@ -75,7 +82,7 @@ describe DesignManagement::DesignsFinder do
           context 'when argument is the second version' do
             let(:params) { { visible_at_version: second_version } }
 
-            it { is_expected.to contain_exactly(design2, design1) }
+            it { is_expected.to contain_exactly(design1, design2) }
           end
         end
       end

ee/spec/models/design_management/design_spec.rb

@@ -144,6 +144,18 @@ describe DesignManagement::Design do
       end
     end
 
+    describe '.with_filename' do
+      it 'returns correct design when passed a single filename' do
+        expect(described_class.with_filename(design1.filename)).to eq([design1])
+      end
+
+      it 'returns correct designs when passed an Array of filenames' do
+        expect(
+          described_class.with_filename([design1, design2].map(&:filename))
+        ).to contain_exactly(design1, design2)
+      end
+    end
+
     describe '.current' do
       it 'returns just the undeleted designs' do
         delete_designs(design3)

locale/gitlab.pot

@@ -5800,6 +5800,9 @@ msgstr ""
 msgid "DesignManagement|Upload and view the latest designs for this issue. Consistent and easy to find, so everyone is up to date."
 msgstr ""
 
+msgid "DesignManagement|We could not delete %{design}. Please try again."
+msgstr ""
+
 msgid "DesignManagement|We could not delete design(s). Please try again."
 msgstr ""
 
@@ -20127,6 +20130,9 @@ msgstr ""
 msgid "a deleted user"
 msgstr ""
 
+msgid "a design"
+msgstr ""
+
 msgid "added %{created_at_timeago}"
 msgstr ""
 
@@ -20521,6 +20527,9 @@ msgstr ""
 msgid "design"
 msgstr ""
 
+msgid "designs"
+msgstr ""
+
 msgid "detached"
 msgstr ""