Revert "Merge branch '_acet-fix-outdated-discussions' into 'master'"

This reverts commit 740ae2d1, reversing
changes made to 1ba47de5.

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

@@ -77,8 +77,7 @@ export default {
       diffViewType: state => state.diffs.diffViewType,
       diffFiles: state => state.diffs.diffFiles,
     }),
-    ...mapGetters(['isLoggedIn']),
-    ...mapGetters('diffs', ['discussionsByLineCode']),
+    ...mapGetters(['isLoggedIn', 'discussionsByLineCode']),
     lineHref() {
       return this.lineCode ? `#${this.lineCode}` : '#';
     },

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

@@ -26,16 +26,13 @@ export default {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters('diffs', ['discussionsByLineCode']),
+    ...mapGetters(['discussionsByLineCode']),
     discussions() {
       return this.discussionsByLineCode[this.line.lineCode] || [];
     },
     className() {
       return this.discussions.length ? '' : 'js-temp-notes-holder';
     },
-    hasCommentForm() {
-      return this.diffLineCommentForms[this.line.lineCode];
-    },
   },
 };
 </script>
@@ -56,7 +53,7 @@ export default {
           :discussions="discussions"
         />
         <diff-line-note-form
-          v-if="hasCommentForm"
+          v-if="diffLineCommentForms[line.lineCode]"
           :diff-file-hash="diffFileHash"
           :line="line"
           :note-target-line="line"

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

@@ -20,7 +20,8 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId', 'discussionsByLineCode']),
+    ...mapGetters('diffs', ['commitId']),
+    ...mapGetters(['discussionsByLineCode']),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),

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

@@ -26,7 +26,7 @@ export default {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters('diffs', ['discussionsByLineCode']),
+    ...mapGetters(['discussionsByLineCode']),
     leftLineCode() {
       return this.line.left.lineCode;
     },

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

@@ -21,7 +21,8 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId', 'discussionsByLineCode']),
+    ...mapGetters('diffs', ['commitId']),
+    ...mapGetters(['discussionsByLineCode']),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),

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

 import _ from 'underscore';
-import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants';
-import { getDiffRefsByLineCode } from './utils';
 
 export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE;
 
@@ -58,44 +56,6 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
       discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash),
   ) || [];
 
-/**
- * Returns an Object with discussions by their diff line code
- * To avoid rendering outdated discussions on the Changes tab we should do a bunch of SHA
- * comparisions. `note.position.formatter` have the current version diff refs but
- * `note.original_position.formatter` will have the first version's diff refs.
- * If line diff refs matches with one of them, we should render it as a discussion on Changes tab.
- *
- * @param {Object} diff
- * @returns {Array}
- */
-export const discussionsByLineCode = (state, getters, rootState, rootGetters) => {
-  const diffRefsByLineCode = getDiffRefsByLineCode(state.diffFiles);
-
-  return rootGetters.discussions.reduce((acc, note) => {
-    const isDiffDiscussion = note.diff_discussion;
-    const hasLineCode = note.line_code;
-    const isResolvable = note.resolvable;
-    const diffRefs = diffRefsByLineCode[note.line_code];
-
-    if (isDiffDiscussion && hasLineCode && isResolvable && diffRefs) {
-      const refs = convertObjectPropsToCamelCase(note.position.formatter);
-      const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter);
-
-      if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) {
-        const lineCode = note.line_code;
-
-        if (acc[lineCode]) {
-          acc[lineCode].push(note);
-        } else {
-          acc[lineCode] = [note];
-        }
-      }
-    }
-
-    return acc;
-  }, {});
-};
-
 // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
 export const getDiffFileByHash = state => fileHash =>
   state.diffFiles.find(file => file.fileHash === fileHash);

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

@@ -173,24 +173,3 @@ export function trimFirstCharOfLineContent(line = {}) {
 
   return parsedLine;
 }
-
-export function getDiffRefsByLineCode(diffFiles) {
-  return diffFiles.reduce((acc, diffFile) => {
-    const { baseSha, headSha, startSha } = diffFile.diffRefs;
-    const { newPath, oldPath } = diffFile;
-
-    // We can only use highlightedDiffLines to create the map of diff lines because
-    // highlightedDiffLines will also include every parallel diff line in it.
-    if (diffFile.highlightedDiffLines) {
-      diffFile.highlightedDiffLines.forEach(line => {
-        const { lineCode, oldLine, newLine } = line;
-
-        if (lineCode) {
-          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
-        }
-      });
-    }
-
-    return acc;
-  }, {});
-}

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

@@ -28,6 +28,18 @@ export const notesById = state =>
     return acc;
   }, {});
 
+export const discussionsByLineCode = state =>
+  state.discussions.reduce((acc, note) => {
+    if (note.diff_discussion && note.line_code && note.resolvable) {
+      // For context about line notes: there might be multiple notes with the same line code
+      const items = acc[note.line_code] || [];
+      items.push(note);
+
+      Object.assign(acc, { [note.line_code]: items });
+    }
+    return acc;
+  }, {});
+
 export const noteableType = state => {
   const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
 

app/serializers/discussion_entity.rb

@@ -6,7 +6,6 @@ class DiscussionEntity < Grape::Entity
 
   expose :id, :reply_id
   expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
-  expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
   expose :line_code, if: -> (d, _) { d.diff_discussion? }
   expose :expanded?, as: :expanded
   expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? }

spec/javascripts/diffs/components/diff_line_gutter_content_spec.js

@@ -18,12 +18,10 @@ describe('DiffLineGutterContent', () => {
   };
   const setDiscussions = component => {
     component.$store.dispatch('setInitialNotes', getDiscussionsMockData());
-    component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] });
   };
 
   const resetDiscussions = component => {
     component.$store.dispatch('setInitialNotes', []);
-    component.$store.commit('diffs/SET_DIFF_DATA', {});
   };
 
   describe('computed', () => {

spec/javascripts/diffs/components/inline_diff_view_spec.js

@@ -33,7 +33,6 @@ describe('InlineDiffView', () => {
     it('should render discussions', done => {
       const el = component.$el;
       component.$store.dispatch('setInitialNotes', getDiscussionsMockData());
-      component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] });
 
       Vue.nextTick(() => {
         expect(el.querySelectorAll('.notes_holder').length).toEqual(1);

spec/javascripts/diffs/mock_data/diff_discussions.js

@@ -12,17 +12,6 @@ export default {
       head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
     },
   },
-  original_position: {
-    formatter: {
-      old_line: null,
-      new_line: 2,
-      old_path: 'CHANGELOG',
-      new_path: 'CHANGELOG',
-      base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
-      start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
-      head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
-    },
-  },
   line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
   expanded: true,
   notes: [

spec/javascripts/diffs/store/getters_spec.js

@@ -2,7 +2,6 @@ import * as getters from '~/diffs/store/getters';
 import state from '~/diffs/store/modules/diff_state';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants';
 import discussion from '../mock_data/diff_discussions';
-import diffFile from '../mock_data/diff_file';
 
 describe('Diffs Module Getters', () => {
   let localState;
@@ -204,38 +203,4 @@ describe('Diffs Module Getters', () => {
       expect(getters.getDiffFileByHash(localState)('123')).toBeUndefined();
     });
   });
-
-  describe('discussionsByLineCode', () => {
-    let mockState;
-
-    beforeEach(() => {
-      mockState = { diffFiles: [diffFile] };
-    });
-
-    it('should return a map of diff lines with their line codes', () => {
-      const mockGetters = { discussions: [discussionMock] };
-
-      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
-      expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined();
-      expect(Object.keys(map).length).toEqual(1);
-    });
-
-    it('should have the diff discussion on the map if the original position matches', () => {
-      discussionMock.position.formatter.base_sha = 'ff9200';
-      const mockGetters = { discussions: [discussionMock] };
-
-      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
-      expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined();
-      expect(Object.keys(map).length).toEqual(1);
-    });
-
-    it('should not add an outdated diff discussion to the returned map', () => {
-      discussionMock.position.formatter.base_sha = 'ff9200';
-      discussionMock.original_position.formatter.base_sha = 'ff9200';
-      const mockGetters = { discussions: [discussionMock] };
-
-      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
-      expect(Object.keys(map).length).toEqual(0);
-    });
-  });
 });

spec/javascripts/diffs/store/utils_spec.js

@@ -207,24 +207,4 @@ describe('DiffsStoreUtils', () => {
       expect(utils.trimFirstCharOfLineContent()).toEqual({});
     });
   });
-
-  describe('getDiffRefsByLineCode', () => {
-    it('should return diffRefs for all highlightedDiffLines', () => {
-      const diffFile = getDiffFileMock();
-      const map = utils.getDiffRefsByLineCode([diffFile]);
-      const { highlightedDiffLines } = diffFile;
-      const lineCodeCount = highlightedDiffLines.reduce(
-        (acc, line) => (line.lineCode ? acc + 1 : acc),
-        0,
-      );
-
-      const { baseSha, headSha, startSha } = diffFile.diffRefs;
-      const targetLine = map[highlightedDiffLines[4].lineCode];
-
-      expect(Object.keys(map).length).toEqual(lineCodeCount);
-      expect(targetLine.baseSha).toEqual(baseSha);
-      expect(targetLine.headSha).toEqual(headSha);
-      expect(targetLine.startSha).toEqual(startSha);
-    });
-  });
 });