Merge branch 'ph/39095/fixDiscussionsNotRendering' into 'master'

Fix issue where diff discussions would not get rendered

Closes #39095

See merge request gitlab-org/gitlab!21498

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

@@ -95,7 +95,6 @@ export default {
       parseInt(localStorage.getItem(TREE_LIST_WIDTH_STORAGE_KEY), 10) || INITIAL_TREE_WIDTH;
 
     return {
-      assignedDiscussions: false,
       treeWidth,
     };
   },
@@ -114,6 +113,7 @@ export default {
       numVisibleFiles: state => state.diffs.size,
       plainDiffPath: state => state.diffs.plainDiffPath,
       emailPatchPath: state => state.diffs.emailPatchPath,
+      retrievingBatches: state => state.diffs.retrievingBatches,
     }),
     ...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']),
     ...mapGetters('diffs', ['isParallelView', 'currentDiffIndex']),
@@ -144,9 +144,6 @@ export default {
     isLimitedContainer() {
       return !this.showTreeList && !this.isParallelView && !this.isFluidLayout;
     },
-    shouldSetDiscussions() {
-      return this.isNotesFetched && !this.assignedDiscussions && !this.isLoading;
-    },
   },
   watch: {
     diffViewType() {
@@ -163,10 +160,8 @@ export default {
     },
     isLoading: 'adjustView',
     showTreeList: 'adjustView',
-    shouldSetDiscussions(newVal) {
-      if (newVal) {
-        this.setDiscussions();
-      }
+    retrievingBatches(newVal) {
+      if (!newVal) this.unwatchDiscussions();
     },
   },
   mounted() {
@@ -192,10 +187,14 @@ export default {
   },
   created() {
     this.adjustView();
-    eventHub.$once('fetchedNotesData', this.setDiscussions);
     eventHub.$once('fetchDiffData', this.fetchData);
     eventHub.$on('refetchDiffData', this.refetchDiffData);
     this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
+
+    this.unwatchDiscussions = this.$watch(
+      () => `${this.diffFiles.length}:${this.$store.state.notes.discussions.length}`,
+      () => this.setDiscussions(),
+    );
   },
   beforeDestroy() {
     eventHub.$off('fetchDiffData', this.fetchData);
@@ -217,7 +216,6 @@ export default {
       'toggleShowTreeList',
     ]),
     refetchDiffData() {
-      this.assignedDiscussions = false;
       this.fetchData(false);
     },
     startDiffRendering() {
@@ -269,17 +267,13 @@ export default {
       }
     },
     setDiscussions() {
-      if (this.shouldSetDiscussions) {
-        this.assignedDiscussions = true;
-
-        requestIdleCallback(
-          () =>
-            this.assignDiscussionsToDiff()
-              .then(this.$nextTick)
-              .then(this.startTaskList),
-          { timeout: 1000 },
-        );
-      }
+      requestIdleCallback(
+        () =>
+          this.assignDiscussionsToDiff()
+            .then(this.$nextTick)
+            .then(this.startTaskList),
+        { timeout: 1000 },
+      );
     },
     adjustView() {
       if (this.shouldShow) {

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

@@ -91,6 +91,7 @@ export const fetchDiffFiles = ({ state, commit }) => {
 
 export const fetchDiffFilesBatch = ({ commit, state }) => {
   commit(types.SET_BATCH_LOADING, true);
+  commit(types.SET_RETRIEVING_BATCHES, true);
 
   const getBatch = page =>
     axios
@@ -100,9 +101,11 @@ export const fetchDiffFilesBatch = ({ commit, state }) => {
       .then(({ data: { pagination, diff_files } }) => {
         commit(types.SET_DIFF_DATA_BATCH, { diff_files });
         commit(types.SET_BATCH_LOADING, false);
+        if (!pagination.next_page) commit(types.SET_RETRIEVING_BATCHES, false);
         return pagination.next_page;
       })
-      .then(nextPage => nextPage && getBatch(nextPage));
+      .then(nextPage => nextPage && getBatch(nextPage))
+      .catch(() => commit(types.SET_RETRIEVING_BATCHES, false));
 
   return getBatch()
     .then(handleLocationHash)

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -9,6 +9,7 @@ const defaultViewType = INLINE_DIFF_VIEW_TYPE;
 export default () => ({
   isLoading: true,
   isBatchLoading: false,
+  retrievingBatches: false,
   addedLines: null,
   removedLines: null,
   endpoint: '',

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

 export const SET_BASE_CONFIG = 'SET_BASE_CONFIG';
 export const SET_LOADING = 'SET_LOADING';
 export const SET_BATCH_LOADING = 'SET_BATCH_LOADING';
+export const SET_RETRIEVING_BATCHES = 'SET_RETRIEVING_BATCHES';
 export const SET_DIFF_DATA = 'SET_DIFF_DATA';
 export const SET_DIFF_DATA_BATCH = 'SET_DIFF_DATA_BATCH';
 export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';

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

@@ -40,6 +40,10 @@ export default {
     Object.assign(state, { isBatchLoading });
   },
 
+  [types.SET_RETRIEVING_BATCHES](state, retrievingBatches) {
+    Object.assign(state, { retrievingBatches });
+  },
+
   [types.SET_DIFF_DATA](state, data) {
     if (
       !(

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -97,9 +97,7 @@ describe 'User comments on a diff', :js do
   end
 
   context 'multiple suggestions in expanded lines' do
-    # Report issue: https://gitlab.com/gitlab-org/gitlab/issues/38277
-    # Fix issue: https://gitlab.com/gitlab-org/gitlab/issues/39095
-    it 'suggestions are appliable', :quarantine do
+    it 'suggestions are appliable' do
       diff_file = merge_request.diffs(paths: ['files/ruby/popen.rb']).diff_files.first
       hash = Digest::SHA1.hexdigest(diff_file.file_path)
 

spec/javascripts/diffs/components/app_spec.js

@@ -69,13 +69,19 @@ describe('diffs/components/app', () => {
 
   describe('fetch diff methods', () => {
     beforeEach(() => {
+      const fetchResolver = () => {
+        store.state.diffs.retrievingBatches = false;
+        return Promise.resolve();
+      };
       spyOn(window, 'requestIdleCallback').and.callFake(fn => fn());
       createComponent();
-      spyOn(wrapper.vm, 'fetchDiffFiles').and.callFake(() => Promise.resolve());
-      spyOn(wrapper.vm, 'fetchDiffFilesMeta').and.callFake(() => Promise.resolve());
-      spyOn(wrapper.vm, 'fetchDiffFilesBatch').and.callFake(() => Promise.resolve());
+      spyOn(wrapper.vm, 'fetchDiffFiles').and.callFake(fetchResolver);
+      spyOn(wrapper.vm, 'fetchDiffFilesMeta').and.callFake(fetchResolver);
+      spyOn(wrapper.vm, 'fetchDiffFilesBatch').and.callFake(fetchResolver);
       spyOn(wrapper.vm, 'setDiscussions');
       spyOn(wrapper.vm, 'startRenderDiffsQueue');
+      spyOn(wrapper.vm, 'unwatchDiscussions');
+      store.state.diffs.retrievingBatches = true;
     });
 
     it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => {
@@ -87,6 +93,7 @@ describe('diffs/components/app', () => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
+        expect(wrapper.vm.unwatchDiscussions).toHaveBeenCalled();
 
         done();
       });
@@ -102,6 +109,7 @@ describe('diffs/components/app', () => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();
+        expect(wrapper.vm.unwatchDiscussions).toHaveBeenCalled();
       });
     });
 
@@ -114,6 +122,7 @@ describe('diffs/components/app', () => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();
+        expect(wrapper.vm.unwatchDiscussions).toHaveBeenCalled();
       });
     });
   });

spec/javascripts/diffs/store/actions_spec.js

@@ -163,10 +163,12 @@ describe('DiffsStoreActions', () => {
         { endpointBatch },
         [
           { type: types.SET_BATCH_LOADING, payload: true },
+          { type: types.SET_RETRIEVING_BATCHES, payload: true },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } },
           { type: types.SET_BATCH_LOADING, payload: false },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: [] } },
           { type: types.SET_BATCH_LOADING, payload: false },
+          { type: types.SET_RETRIEVING_BATCHES, payload: false },
         ],
         [],
         () => {
@@ -215,6 +217,8 @@ describe('DiffsStoreActions', () => {
 
   describe('assignDiscussionsToDiff', () => {
     it('should merge discussions into diffs', done => {
+      window.location.hash = 'ABC_123';
+
       const state = {
         diffFiles: [
           {

spec/javascripts/diffs/store/mutations_spec.js

@@ -40,6 +40,16 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
+  describe('SET_RETRIEVING_BATCHES', () => {
+    it('should set retrievingBatches state', () => {
+      const state = {};
+
+      mutations[types.SET_RETRIEVING_BATCHES](state, false);
+
+      expect(state.retrievingBatches).toEqual(false);
+    });
+  });
+
   describe('SET_DIFF_DATA', () => {
     it('should set diff data type properly', () => {
       const state = {};