From c9bacfd6823de77de6b60db39190190b502681e0 Mon Sep 17 00:00:00 2001
From: Tim Zallmann <tzallmann@gitlab.com>
Date: Wed, 5 Sep 2018 10:28:49 +0200
Subject: Fixed Resolving, Loading more and Line Bugs

---
 .../components/resolve_discussion_btn.js           | 20 ++++----
 .../javascripts/diff_notes/services/resolve.js     |  9 +---
 app/assets/javascripts/diffs/components/app.vue    | 18 ++++---
 .../javascripts/diffs/components/diff_file.vue     | 33 ++++++++-----
 .../diffs/components/diff_line_gutter_content.vue  |  3 +-
 app/assets/javascripts/diffs/store/actions.js      | 21 +++++---
 .../javascripts/diffs/store/mutation_types.js      |  2 +
 app/assets/javascripts/diffs/store/mutations.js    | 57 ++++++----------------
 app/assets/javascripts/diffs/store/utils.js        | 39 +++++++++++++++
 .../javascripts/notes/components/notes_app.vue     |  1 +
 app/assets/javascripts/notes/stores/mutations.js   | 11 +----
 11 files changed, 119 insertions(+), 95 deletions(-)

(limited to 'app/assets')

diff --git a/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js
index 5ed13488788..ae31005a2d2 100644
--- a/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js
+++ b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js
@@ -25,44 +25,44 @@ const ResolveDiscussionBtn = Vue.extend({
     };
   },
   computed: {
-    showButton: function () {
+    showButton: function() {
       if (this.discussion) {
         return this.discussion.isResolvable();
       } else {
         return false;
       }
     },
-    isDiscussionResolved: function () {
+    isDiscussionResolved: function() {
       if (this.discussion) {
         return this.discussion.isResolved();
       } else {
         return false;
       }
     },
-    buttonText: function () {
+    buttonText: function() {
       if (this.isDiscussionResolved) {
-        return "Unresolve discussion";
+        return 'Unresolve discussion';
       } else {
-        return "Resolve discussion";
+        return 'Resolve discussion';
       }
     },
-    loading: function () {
+    loading: function() {
       if (this.discussion) {
         return this.discussion.loading;
       } else {
         return false;
       }
-    }
+    },
   },
-  created: function () {
+  created: function() {
     CommentsStore.createDiscussion(this.discussionId, this.canResolve);
 
     this.discussion = CommentsStore.state[this.discussionId];
   },
   methods: {
-    resolve: function () {
+    resolve: function() {
       ResolveService.toggleResolveForDiscussion(this.mergeRequestId, this.discussionId);
-    }
+    },
   },
 });
 
diff --git a/app/assets/javascripts/diff_notes/services/resolve.js b/app/assets/javascripts/diff_notes/services/resolve.js
index 0b3568e432d..e69eaad4423 100644
--- a/app/assets/javascripts/diff_notes/services/resolve.js
+++ b/app/assets/javascripts/diff_notes/services/resolve.js
@@ -8,9 +8,7 @@ window.gl = window.gl || {};
 
 class ResolveServiceClass {
   constructor(root) {
-    this.noteResource = Vue.resource(
-      `${root}/notes{/noteId}/resolve?html=true`,
-    );
+    this.noteResource = Vue.resource(`${root}/notes{/noteId}/resolve?html=true`);
     this.discussionResource = Vue.resource(
       `${root}/merge_requests{/mergeRequestId}/discussions{/discussionId}/resolve?html=true`,
     );
@@ -51,10 +49,7 @@ class ResolveServiceClass {
         discussion.updateHeadline(data);
       })
       .catch(
-        () =>
-          new Flash(
-            'An error occurred when trying to resolve a discussion. Please try again.',
-          ),
+        () => new Flash('An error occurred when trying to resolve a discussion. Please try again.'),
       );
   }
 
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 7ec7ee893af..13ff79029d0 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -112,6 +112,7 @@ export default {
   },
   created() {
     this.adjustView();
+    eventHub.$once('fetchedNotesData', this.setDiscussions);
   },
   methods: {
     ...mapActions('diffs', [
@@ -128,12 +129,7 @@ export default {
             () => {
               this.startRenderDiffsQueue()
                 .then(() => {
-                  requestIdleCallback(
-                    () => {
-                      this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
-                    },
-                    { timeout: 1000 },
-                  );
+                  this.setDiscussions();
                 })
                 .catch(() => {
                   createFlash(__('Something went wrong on our end. Please try again!'));
@@ -150,6 +146,16 @@ export default {
         eventHub.$emit('fetchNotesData');
       }
     },
+    setDiscussions() {
+      if (this.isNotesFetched) {
+        requestIdleCallback(
+          () => {
+            this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
+          },
+          { timeout: 1000 },
+        );
+      }
+    },
     adjustView() {
       if (this.shouldShow && this.isParallelView) {
         window.mrTabs.expandViewContainer();
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index 59e9ba08b8b..a0197e95456 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -1,5 +1,5 @@
 <script>
-import { mapActions } from 'vuex';
+import { mapActions, mapGetters } from 'vuex';
 import _ from 'underscore';
 import { __, sprintf } from '~/locale';
 import createFlash from '~/flash';
@@ -30,6 +30,7 @@ export default {
     };
   },
   computed: {
+    ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']),
     isCollapsed() {
       return this.file.collapsed || false;
     },
@@ -44,23 +45,22 @@ export default {
       );
     },
     showExpandMessage() {
-      return this.isCollapsed && !this.isLoadingCollapsedDiff && !this.file.tooLarge;
+      return (
+        !this.isCollapsed &&
+        !this.file.highlightedDiffLines &&
+        !this.isLoadingCollapsedDiff &&
+        !this.file.tooLarge
+      );
     },
     showLoadingIcon() {
       return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed);
     },
   },
   methods: {
-    ...mapActions('diffs', ['loadCollapsedDiff']),
+    ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']),
     handleToggle() {
-      const { collapsed, highlightedDiffLines, parallelDiffLines } = this.file;
-
-      if (
-        collapsed &&
-        !highlightedDiffLines &&
-        parallelDiffLines !== undefined &&
-        !parallelDiffLines.length
-      ) {
+      const { highlightedDiffLines, parallelDiffLines } = this.file;
+      if (!highlightedDiffLines && parallelDiffLines !== undefined && !parallelDiffLines.length) {
         this.handleLoadCollapsedDiff();
       } else {
         this.file.collapsed = !this.file.collapsed;
@@ -76,6 +76,15 @@ export default {
           this.file.collapsed = false;
           this.file.renderIt = true;
         })
+        .then(() => this.$nextTick())
+        .then(() => {
+          requestIdleCallback(
+            () => {
+              this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
+            },
+            { timeout: 1000 },
+          );
+        })
         .catch(() => {
           this.isLoadingCollapsedDiff = false;
           createFlash(__('Something went wrong on our end. Please try again!'));
@@ -136,7 +145,7 @@ export default {
       :diff-file="file"
     />
     <loading-icon
-      v-else-if="showLoadingIcon"
+      v-if="showLoadingIcon"
       class="diff-content loading"
     />
     <div
diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
index f07a048d6a3..88bf026c0dd 100644
--- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
@@ -73,7 +73,7 @@ export default {
     }),
     ...mapGetters(['isLoggedIn']),
     lineHref() {
-      return this.line && this.line.code ? `#${this.line.code}` : '#';
+      return this.line && this.line.lineCode ? `#${this.line.lineCode}` : '#';
     },
     shouldShowCommentButton() {
       return (
@@ -93,7 +93,6 @@ export default {
       if (this.line && !this.line.type && this.linePosition === LINE_POSITION_RIGHT) {
         return false;
       }
-
       return this.showCommentButton && this.hasDiscussions;
     },
   },
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 1838394422a..d04fa5d2cae 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -29,7 +29,7 @@ export const fetchDiffFiles = ({ state, commit }) => {
     .then(handleLocationHash);
 };
 
-export const assignDiscussionsToDiff = ({ state }, allLineDiscussions) => {
+export const assignDiscussionsToDiff = ({ state, commit }, allLineDiscussions) => {
   Object.values(allLineDiscussions).forEach(discussions => {
     if (discussions.length > 0) {
       const { fileHash } = discussions[0];
@@ -40,12 +40,11 @@ export const assignDiscussionsToDiff = ({ state }, allLineDiscussions) => {
             (line.left && line.left.lineCode === discussions[0].line_code) ||
             (line.right && line.right.lineCode === discussions[0].line_code),
         );
-
         if (targetLine) {
           if (targetLine.left && targetLine.left.lineCode === discussions[0].line_code) {
-            Object.assign(targetLine.left, { discussions });
+            commit(types.SET_LINE_DISCUSSIONS, { line: targetLine.left, discussions });
           } else {
-            Object.assign(targetLine.right, { discussions });
+            commit(types.SET_LINE_DISCUSSIONS, { line: targetLine.right, discussions });
           }
         }
 
@@ -55,7 +54,7 @@ export const assignDiscussionsToDiff = ({ state }, allLineDiscussions) => {
           );
 
           if (targetInlineLine) {
-            Object.assign(targetInlineLine, { discussions });
+            commit(types.SET_LINE_DISCUSSIONS, { line: targetInlineLine, discussions });
           }
         }
       }
@@ -63,7 +62,7 @@ export const assignDiscussionsToDiff = ({ state }, allLineDiscussions) => {
   });
 };
 
-export const removeDiscussionsFromDiff = ({ state }, removeDiscussion) => {
+export const removeDiscussionsFromDiff = ({ state, commit }, removeDiscussion) => {
   const { fileHash } = removeDiscussion;
   const selectedFile = state.diffFiles.find(file => file.fileHash === fileHash);
   if (selectedFile) {
@@ -74,7 +73,13 @@ export const removeDiscussionsFromDiff = ({ state }, removeDiscussion) => {
     );
 
     if (targetLine) {
-      Object.assign(targetLine.right, { discussions: [] });
+      if (targetLine) {
+        if (targetLine.left && targetLine.left.lineCode === removeDiscussion.line_code) {
+          commit(types.REMOVE_LINE_DISCUSSIONS, targetLine.left);
+        } else {
+          commit(types.REMOVE_LINE_DISCUSSIONS, targetLine.right);
+        }
+      }
     }
 
     const targetInlineLine = selectedFile.highlightedDiffLines.find(
@@ -82,7 +87,7 @@ export const removeDiscussionsFromDiff = ({ state }, removeDiscussion) => {
     );
 
     if (targetInlineLine) {
-      Object.assign(targetInlineLine, { discussions: [] });
+      commit(types.REMOVE_LINE_DISCUSSIONS, targetInlineLine);
     }
   }
 };
diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js
index c999d637d50..80f2807682a 100644
--- a/app/assets/javascripts/diffs/store/mutation_types.js
+++ b/app/assets/javascripts/diffs/store/mutation_types.js
@@ -9,3 +9,5 @@ export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES';
 export const ADD_COLLAPSED_DIFFS = 'ADD_COLLAPSED_DIFFS';
 export const EXPAND_ALL_FILES = 'EXPAND_ALL_FILES';
 export const RENDER_FILE = 'RENDER_FILE';
+export const SET_LINE_DISCUSSIONS = 'SET_LINE_DISCUSSIONS';
+export const REMOVE_LINE_DISCUSSIONS = 'REMOVE_LINE_DISCUSSIONS';
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index af40a657211..f7f6a187e0d 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/app/assets/javascripts/diffs/store/mutations.js
@@ -6,9 +6,8 @@ import {
   addLineReferences,
   removeMatchLine,
   addContextLines,
-  trimFirstCharOfLineContent,
+  prepareDiffData,
 } from './utils';
-import { LINES_TO_BE_RENDERED_DIRECTLY, MAX_LINES_TO_BE_RENDERED } from '../constants';
 import * as types from './mutation_types';
 
 export default {
@@ -23,40 +22,7 @@ export default {
 
   [types.SET_DIFF_DATA](state, data) {
     const diffData = convertObjectPropsToCamelCase(data, { deep: true });
-    let showingLines = 0;
-    const filesLength = diffData.diffFiles.length;
-    let i;
-    for (i = 0; i < filesLength; i += 1) {
-      const file = diffData.diffFiles[i];
-
-      if (file.parallelDiffLines) {
-        const linesLength = file.parallelDiffLines.length;
-        let u = 0;
-        for (u = 0; u < linesLength; u += 1) {
-          const line = file.parallelDiffLines[u];
-          if (line.left) {
-            line.left = trimFirstCharOfLineContent(line.left);
-          }
-          if (line.right) {
-            line.right = trimFirstCharOfLineContent(line.right);
-          }
-        }
-      }
-
-      if (file.highlightedDiffLines) {
-        const linesLength = file.highlightedDiffLines.length;
-        let u;
-        for (u = 0; u < linesLength; u += 1) {
-          trimFirstCharOfLineContent(file.highlightedDiffLines[u]);
-        }
-        showingLines += file.parallelDiffLines.length;
-      }
-
-      Object.assign(file, {
-        renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY,
-        collapsed: file.text && showingLines > MAX_LINES_TO_BE_RENDERED,
-      });
-    }
+    prepareDiffData(diffData);
 
     Object.assign(state, {
       ...diffData,
@@ -106,12 +72,9 @@ export default {
 
   [types.ADD_COLLAPSED_DIFFS](state, { file, data }) {
     const normalizedData = convertObjectPropsToCamelCase(data, { deep: true });
+    prepareDiffData(normalizedData);
     const [newFileData] = normalizedData.diffFiles.filter(f => f.fileHash === file.fileHash);
-
-    if (newFileData) {
-      const index = _.findIndex(state.diffFiles, f => f.fileHash === file.fileHash);
-      state.diffFiles.splice(index, 1, newFileData);
-    }
+    Object.assign(file, { ...newFileData });
   },
 
   [types.EXPAND_ALL_FILES](state) {
@@ -120,4 +83,16 @@ export default {
       collapsed: false,
     }));
   },
+
+  [types.SET_LINE_DISCUSSIONS](state, { line, discussions }) {
+    Object.assign(line, {
+      discussions,
+    });
+  },
+
+  [types.REMOVE_LINE_DISCUSSIONS](state, line) {
+    Object.assign(line, {
+      discussions: [],
+    });
+  },
 };
diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index 5a29d3e1ac7..eb9bab09d38 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/app/assets/javascripts/diffs/store/utils.js
@@ -8,6 +8,8 @@ import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
   MATCH_LINE_TYPE,
+  LINES_TO_BE_RENDERED_DIRECTLY,
+  MAX_LINES_TO_BE_RENDERED,
 } from '../constants';
 
 export function findDiffFile(files, hash) {
@@ -179,6 +181,43 @@ export function trimFirstCharOfLineContent(line = {}) {
   return parsedLine;
 }
 
+export function prepareDiffData(diffData) {
+  let showingLines = 0;
+  const filesLength = diffData.diffFiles.length;
+  let i;
+  for (i = 0; i < filesLength; i += 1) {
+    const file = diffData.diffFiles[i];
+
+    if (file.parallelDiffLines) {
+      const linesLength = file.parallelDiffLines.length;
+      let u = 0;
+      for (u = 0; u < linesLength; u += 1) {
+        const line = file.parallelDiffLines[u];
+        if (line.left) {
+          line.left = trimFirstCharOfLineContent(line.left);
+        }
+        if (line.right) {
+          line.right = trimFirstCharOfLineContent(line.right);
+        }
+      }
+    }
+
+    if (file.highlightedDiffLines) {
+      const linesLength = file.highlightedDiffLines.length;
+      let u;
+      for (u = 0; u < linesLength; u += 1) {
+        trimFirstCharOfLineContent(file.highlightedDiffLines[u]);
+      }
+      showingLines += file.parallelDiffLines.length;
+    }
+
+    Object.assign(file, {
+      renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY,
+      collapsed: file.text && showingLines > MAX_LINES_TO_BE_RENDERED,
+    });
+  }
+}
+
 export function getDiffRefsByLineCode(diffFiles) {
   return diffFiles.reduce((acc, diffFile) => {
     const { baseSha, headSha, startSha } = diffFile.diffRefs;
diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue
index 9b8713b40fb..7f9d23b211b 100644
--- a/app/assets/javascripts/notes/components/notes_app.vue
+++ b/app/assets/javascripts/notes/components/notes_app.vue
@@ -138,6 +138,7 @@ export default {
         .then(() => {
           this.isLoading = false;
           this.setNotesFetchedState(true);
+          eventHub.$emit('fetchedNotesData');
         })
         .then(() => this.$nextTick())
         .then(() => this.checkLocationHash())
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index ab6a95e2601..b6fd19dedf9 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -185,16 +185,9 @@ export default {
 
   [types.UPDATE_DISCUSSION](state, noteData) {
     const note = noteData;
-    let index = 0;
-
-    state.discussions.forEach((n, i) => {
-      if (n.id === note.id) {
-        index = i;
-      }
-    });
-
+    const selectedDiscussion = state.discussions.find(disc => disc.id === note.id);
     note.expanded = true; // override expand flag to prevent collapse
-    state.discussions.splice(index, 1, note);
+    Object.assign(selectedDiscussion, { ...note });
   },
 
   [types.CLOSE_ISSUE](state) {
-- 
cgit v1.2.1