Merge branch 'acet-fix-fetching-diffs-data' into 'master'

Prevent fetching diffs and discussions data unnecessarily on MR page

Closes #48493

See merge request gitlab-org/gitlab-ce!20190

app/assets/javascripts/diffs/components/app.vue

@@ -3,6 +3,7 @@ import { mapState, mapGetters, mapActions } from 'vuex';
 import Icon from '~/vue_shared/components/icon.vue';
 import { __ } from '~/locale';
 import createFlash from '~/flash';
+import eventHub from '../../notes/event_hub';
 import LoadingIcon from '../../vue_shared/components/loading_icon.vue';
 import CompareVersions from './compare_versions.vue';
 import ChangedFiles from './changed_files.vue';
@@ -62,7 +63,7 @@ export default {
       plainDiffPath: state => state.diffs.plainDiffPath,
       emailPatchPath: state => state.diffs.emailPatchPath,
     }),
-    ...mapGetters(['isParallelView']),
+    ...mapGetters(['isParallelView', 'isNotesFetched']),
     targetBranch() {
       return {
         branchName: this.targetBranchName,
@@ -94,20 +95,36 @@ export default {
       this.adjustView();
     },
     shouldShow() {
+      // When the shouldShow property changed to true, the route is rendered for the first time
+      // and if we have the isLoading as true this means we didn't fetch the data
+      if (this.isLoading) {
+        this.fetchData();
+      }
+
       this.adjustView();
     },
   },
   mounted() {
     this.setBaseConfig({ endpoint: this.endpoint, projectPath: this.projectPath });
-    this.fetchDiffFiles().catch(() => {
-      createFlash(__('Fetching diff files failed. Please reload the page to try again!'));
-    });
+
+    if (this.shouldShow) {
+      this.fetchData();
+    }
   },
   created() {
     this.adjustView();
   },
   methods: {
     ...mapActions(['setBaseConfig', 'fetchDiffFiles']),
+    fetchData() {
+      this.fetchDiffFiles().catch(() => {
+        createFlash(__('Something went wrong on our end. Please try again!'));
+      });
+
+      if (!this.isNotesFetched) {
+        eventHub.$emit('fetchNotesData');
+      }
+    },
     setActive(filePath) {
       this.activeFile = filePath;
     },

app/assets/javascripts/diffs/store/actions.js

@@ -15,10 +15,6 @@ export const setBaseConfig = ({ commit }, options) => {
   commit(types.SET_BASE_CONFIG, { endpoint, projectPath });
 };
 
-export const setLoadingState = ({ commit }, state) => {
-  commit(types.SET_LOADING, state);
-};
-
 export const fetchDiffFiles = ({ state, commit }) => {
   commit(types.SET_LOADING, true);
 
@@ -88,7 +84,6 @@ export const expandAllFiles = ({ commit }) => {
 
 export default {
   setBaseConfig,
-  setLoadingState,
   fetchDiffFiles,
   setInlineDiffViewType,
   setParallelDiffViewType,

app/assets/javascripts/mr_notes/index.js

@@ -45,17 +45,17 @@ export default function initMrNotes() {
         this.updateDiscussionTabCounter();
       },
     },
+    created() {
+      this.setActiveTab(window.mrTabs.getCurrentAction());
+    },
     mounted() {
       this.notesCountBadge = $('.issuable-details').find('.notes-tab .badge');
-      this.setActiveTab(window.mrTabs.getCurrentAction());
-
-      window.mrTabs.eventHub.$on('MergeRequestTabChange', tab => {
-        this.setActiveTab(tab);
-      });
       $(document).on('visibilitychange', this.updateDiscussionTabCounter);
+      window.mrTabs.eventHub.$on('MergeRequestTabChange', this.setActiveTab);
     },
     beforeDestroy() {
       $(document).off('visibilitychange', this.updateDiscussionTabCounter);
+      window.mrTabs.eventHub.$off('MergeRequestTabChange', this.setActiveTab);
     },
     methods: {
       ...mapActions(['setActiveTab']),

app/assets/javascripts/notes/components/notes_app.vue

@@ -3,6 +3,7 @@ import { mapGetters, mapActions } from 'vuex';
 import { getLocationHash } from '../../lib/utils/url_utility';
 import Flash from '../../flash';
 import * as constants from '../constants';
+import eventHub from '../event_hub';
 import noteableNote from './noteable_note.vue';
 import noteableDiscussion from './noteable_discussion.vue';
 import systemNote from '../../vue_shared/components/notes/system_note.vue';
@@ -49,7 +50,7 @@ export default {
     };
   },
   computed: {
-    ...mapGetters(['discussions', 'getNotesDataByProp', 'discussionCount']),
+    ...mapGetters(['isNotesFetched', 'discussions', 'getNotesDataByProp', 'discussionCount']),
     noteableType() {
       return this.noteableData.noteableType;
     },
@@ -61,19 +62,30 @@ export default {
           isSkeletonNote: true,
         });
       }
+
       return this.discussions;
     },
   },
+  watch: {
+    shouldShow() {
+      if (!this.isNotesFetched) {
+        this.fetchNotes();
+      }
+    },
+  },
   created() {
     this.setNotesData(this.notesData);
     this.setNoteableData(this.noteableData);
     this.setUserData(this.userData);
     this.setTargetNoteHash(getLocationHash());
+    eventHub.$once('fetchNotesData', this.fetchNotes);
   },
   mounted() {
-    this.fetchNotes();
-    const { parentElement } = this.$el;
+    if (this.shouldShow) {
+      this.fetchNotes();
+    }
 
+    const { parentElement } = this.$el;
     if (parentElement && parentElement.classList.contains('js-vue-notes-event')) {
       parentElement.addEventListener('toggleAward', event => {
         const { awardName, noteId } = event.detail;
@@ -93,6 +105,7 @@ export default {
       setLastFetchedAt: 'setLastFetchedAt',
       setTargetNoteHash: 'setTargetNoteHash',
       toggleDiscussion: 'toggleDiscussion',
+      setNotesFetchedState: 'setNotesFetchedState',
     }),
     getComponentName(discussion) {
       if (discussion.isSkeletonNote) {
@@ -119,11 +132,13 @@ export default {
         })
         .then(() => {
           this.isLoading = false;
+          this.setNotesFetchedState(true);
         })
         .then(() => this.$nextTick())
         .then(() => this.checkLocationHash())
         .catch(() => {
           this.isLoading = false;
+          this.setNotesFetchedState(true);
           Flash('Something went wrong while fetching comments. Please try again.');
         });
     },
@@ -161,11 +176,12 @@ export default {
 <template>
   <div
     v-if="shouldShow"
-    id="notes">
+    id="notes"
+  >
     <ul
       id="notes-list"
-      class="notes main-notes-list timeline">
-
+      class="notes main-notes-list timeline"
+    >
       <component
         v-for="discussion in allDiscussions"
         :is="getComponentName(discussion)"

app/assets/javascripts/notes/stores/actions.js

@@ -28,6 +28,9 @@ export const setInitialNotes = ({ commit }, discussions) =>
 
 export const setTargetNoteHash = ({ commit }, data) => commit(types.SET_TARGET_NOTE_HASH, data);
 
+export const setNotesFetchedState = ({ commit }, state) =>
+  commit(types.SET_NOTES_FETCHED_STATE, state);
+
 export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data);
 
 export const fetchDiscussions = ({ commit }, path) =>

app/assets/javascripts/notes/stores/getters.js

@@ -8,6 +8,8 @@ export const targetNoteHash = state => state.targetNoteHash;
 
 export const getNotesData = state => state.notesData;
 
+export const isNotesFetched = state => state.isNotesFetched;
+
 export const getNotesDataByProp = state => prop => state.notesData[prop];
 
 export const getNoteableData = state => state.noteableData;

app/assets/javascripts/notes/stores/modules/index.js

@@ -10,6 +10,7 @@ export default {
 
     // View layer
     isToggleStateButtonLoading: false,
+    isNotesFetched: false,
 
     // holds endpoints and permissions provided through haml
     notesData: {

app/assets/javascripts/notes/stores/mutation_types.js

@@ -15,6 +15,7 @@ export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION';
 export const UPDATE_NOTE = 'UPDATE_NOTE';
 export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION';
 export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES';
+export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
 
 // Issue
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';

app/assets/javascripts/notes/stores/mutations.js

@@ -205,6 +205,10 @@ export default {
     Object.assign(state, { isToggleStateButtonLoading: value });
   },
 
+  [types.SET_NOTES_FETCHED_STATE](state, value) {
+    Object.assign(state, { isNotesFetched: value });
+  },
+
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);
     const index = state.discussions.indexOf(discussion);

spec/javascripts/diffs/store/actions_spec.js

@@ -5,7 +5,6 @@ import {
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
 } from '~/diffs/constants';
-import store from '~/diffs/store';
 import * as actions from '~/diffs/store/actions';
 import * as types from '~/diffs/store/mutation_types';
 import axios from '~/lib/utils/axios_utils';
@@ -28,22 +27,6 @@ describe('DiffsStoreActions', () => {
     });
   });
 
-  describe('setLoadingState', () => {
-    it('should set loading state', done => {
-      expect(store.state.diffs.isLoading).toEqual(true);
-      const loadingState = false;
-
-      testAction(
-        actions.setLoadingState,
-        loadingState,
-        {},
-        [{ type: types.SET_LOADING, payload: loadingState }],
-        [],
-        done,
-      );
-    });
-  });
-
   describe('fetchDiffFiles', () => {
     it('should fetch diff files', done => {
       const endpoint = '/fetch/diff/files';

spec/javascripts/notes/stores/actions_spec.js

@@ -291,4 +291,17 @@ describe('Actions Notes Store', () => {
         .catch(done.fail);
     });
   });
+
+  describe('setNotesFetchedState', () => {
+    it('should set notes fetched state', done => {
+      testAction(
+        actions.setNotesFetchedState,
+        true,
+        {},
+        [{ type: 'SET_NOTES_FETCHED_STATE', payload: true }],
+        [],
+        done,
+      );
+    });
+  });
 });

spec/javascripts/notes/stores/getters_spec.js

@@ -15,6 +15,7 @@ describe('Getters Notes Store', () => {
       discussions: [individualNote],
       targetNoteHash: 'hash',
       lastFetchedAt: 'timestamp',
+      isNotesFetched: false,
 
       notesData: notesDataMock,
       userData: userDataMock,
@@ -84,4 +85,10 @@ describe('Getters Notes Store', () => {
       expect(getters.openState(state)).toEqual(noteableDataMock.state);
     });
   });
+
+  describe('isNotesFetched', () => {
+    it('should return the state for the fetching notes', () => {
+      expect(getters.isNotesFetched(state)).toBeFalsy();
+    });
+  });
 });

spec/javascripts/notes/stores/mutation_spec.js

@@ -318,4 +318,15 @@ describe('Notes Store mutations', () => {
       expect(state.isToggleStateButtonLoading).toEqual(false);
     });
   });
+
+  describe('SET_NOTES_FETCHING_STATE', () => {
+    it('should set the given state', () => {
+      const state = {
+        isNotesFetched: false,
+      };
+
+      mutations.SET_NOTES_FETCHED_STATE(state, true);
+      expect(state.isNotesFetched).toEqual(true);
+    });
+  });
 });