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 3b36bab206b..ad838a32518 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/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}` : '#'; }, diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index a6f011ff31e..ca265dd892c 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/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]; - }, }, }; @@ -56,7 +53,7 @@ export default { :discussions="discussions" /> state.diffs.diffLineCommentForms, }), diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index 05e5cafc717..cc5248c25d9 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/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; }, diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 8f8d6bbc818..32528c9e7ab 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/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, }), diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index d3881fa1a0a..855de79adf8 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,7 +1,5 @@ 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); diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 82082ac508a..d9589baa76e 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/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; - }, {}); -} diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index e9e95dd4219..5c65e1c3bb5 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/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; diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index 6f95e6f9ca1..b8321037fa5 100644 --- a/app/serializers/discussion_entity.rb +++ b/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? } diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js index cb85d12daf2..2d136a63c52 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/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', () => { diff --git a/spec/javascripts/diffs/components/inline_diff_view_spec.js b/spec/javascripts/diffs/components/inline_diff_view_spec.js index 90dfa5c5a58..b02328dd359 100644 --- a/spec/javascripts/diffs/components/inline_diff_view_spec.js +++ b/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); diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 8cd57d2248b..41d0dfd8939 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/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: [ diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index f5628a01a55..6210d4a7124 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/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); - }); - }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 8e7bd8afca4..32136d9ebff 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/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); - }); - }); });