diff options
15 files changed, 146 insertions, 20 deletions
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 ad838a32518..3b36bab206b 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -77,7 +77,8 @@ export default { diffViewType: state => state.diffs.diffViewType, diffFiles: state => state.diffs.diffFiles, }), - ...mapGetters(['isLoggedIn', 'discussionsByLineCode']), + ...mapGetters(['isLoggedIn']), + ...mapGetters('diffs', ['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 ca265dd892c..a6f011ff31e 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -26,13 +26,16 @@ export default { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), - ...mapGetters(['discussionsByLineCode']), + ...mapGetters('diffs', ['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> @@ -53,7 +56,7 @@ export default { :discussions="discussions" /> <diff-line-note-form - v-if="diffLineCommentForms[line.lineCode]" + v-if="hasCommentForm" :diff-file-hash="diffFileHash" :line="line" :note-target-line="line" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 9fd19b74cd7..8e491d293e5 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,8 +20,7 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId']), - ...mapGetters(['discussionsByLineCode']), + ...mapGetters('diffs', ['commitId', 'discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => 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 cc5248c25d9..05e5cafc717 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(['discussionsByLineCode']), + ...mapGetters('diffs', ['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 32528c9e7ab..8f8d6bbc818 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,8 +21,7 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId']), - ...mapGetters(['discussionsByLineCode']), + ...mapGetters('diffs', ['commitId', '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 855de79adf8..d3881fa1a0a 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,5 +1,7 @@ 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; @@ -56,6 +58,44 @@ 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 d9589baa76e..82082ac508a 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -173,3 +173,24 @@ 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 5c65e1c3bb5..e9e95dd4219 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,18 +28,6 @@ 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 8a39a4950f5..7505bbdeb3d 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -4,6 +4,7 @@ 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/changelogs/unreleased/_acet-fix-outdated-discussions.yml b/changelogs/unreleased/_acet-fix-outdated-discussions.yml new file mode 100644 index 00000000000..d31483b4765 --- /dev/null +++ b/changelogs/unreleased/_acet-fix-outdated-discussions.yml @@ -0,0 +1,5 @@ +--- +title: Fix showing outdated discussions on Changes tab +merge_request: 20445 +author: +type: fixed 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 2d136a63c52..cb85d12daf2 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js @@ -18,10 +18,12 @@ 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 b02328dd359..90dfa5c5a58 100644 --- a/spec/javascripts/diffs/components/inline_diff_view_spec.js +++ b/spec/javascripts/diffs/components/inline_diff_view_spec.js @@ -33,6 +33,7 @@ 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 41d0dfd8939..8cd57d2248b 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -12,6 +12,17 @@ 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 6210d4a7124..f5628a01a55 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -2,6 +2,7 @@ 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; @@ -203,4 +204,38 @@ 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 32136d9ebff..8e7bd8afca4 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -207,4 +207,24 @@ 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); + }); + }); }); |