summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndré Luís <aluis@gitlab.com>2018-09-03 15:59:03 +0100
committerAndré Luís <aluis@gitlab.com>2018-09-03 17:54:01 +0100
commitb5a27d48179b4a2ae0b9ed9c3bb5b53141ba0795 (patch)
treee5000380b34697202f73fe74bef0bcaf24008954
parent48541cacc050b931d884ba828d0901232308b4c4 (diff)
downloadgitlab-ce-48167-fix-outdated-discussions.tar.gz
Revert "Revert "Merge branch '_acet-fix-outdated-discussions' into 'master'""48167-fix-outdated-discussions
This reverts commit 09c1b008eb4b90c0a8becdf7ebb5723a8bd05468.
-rw-r--r--app/assets/javascripts/diffs/components/diff_line_gutter_content.vue1
-rw-r--r--app/assets/javascripts/diffs/components/inline_diff_comment_row.vue11
-rw-r--r--app/assets/javascripts/diffs/components/inline_diff_view.vue1
-rw-r--r--app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue3
-rw-r--r--app/assets/javascripts/diffs/components/parallel_diff_view.vue1
-rw-r--r--app/assets/javascripts/diffs/store/getters.js43
-rw-r--r--app/assets/javascripts/notes/stores/getters.js12
-rw-r--r--app/serializers/discussion_entity.rb1
-rw-r--r--spec/javascripts/diffs/components/diff_line_gutter_content_spec.js2
-rw-r--r--spec/javascripts/diffs/components/inline_diff_view_spec.js1
-rw-r--r--spec/javascripts/diffs/mock_data/diff_discussions.js11
-rw-r--r--spec/javascripts/diffs/store/getters_spec.js35
-rw-r--r--spec/javascripts/diffs/store/utils_spec.js20
13 files changed, 126 insertions, 16 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 8ad1ea34245..16bfa6c5920 100644
--- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
@@ -88,6 +88,7 @@ export default {
diffFiles: state => state.diffs.diffFiles,
}),
...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 caf84dc9573..5073209f7de 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
@@ -1,5 +1,5 @@
<script>
-import { mapState } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
import diffDiscussions from './diff_discussions.vue';
import diffLineNoteForm from './diff_line_note_form.vue';
@@ -31,9 +31,16 @@ export default {
...mapState({
diffLineCommentForms: state => state.diffs.diffLineCommentForms,
}),
+ ...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>
@@ -54,7 +61,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 e7d789734c3..797b4733252 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue
@@ -24,6 +24,7 @@ export default {
'commitId',
'shouldRenderInlineCommentRow',
'singleDiscussionByLineCode',
+ '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 48b8feeb0b4..e0637055b39 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
@@ -1,5 +1,5 @@
<script>
-import { mapState } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
import diffDiscussions from './diff_discussions.vue';
import diffLineNoteForm from './diff_line_note_form.vue';
@@ -36,6 +36,7 @@ export default {
...mapState({
diffLineCommentForms: state => state.diffs.diffLineCommentForms,
}),
+ ...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 24ceb52a04a..cdd14fe9f9a 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
@@ -25,6 +25,7 @@ export default {
'commitId',
'singleDiscussionByLineCode',
'shouldRenderParallelCommentRow',
+ '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 4a47646d7fa..25b819547e0 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;
@@ -66,7 +68,8 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
export const singleDiscussionByLineCode = (state, getters, rootState, rootGetters) => lineCode => {
if (!lineCode || lineCode === undefined) return [];
- const discussions = rootGetters.discussionsByLineCode;
+ const discussions = getters.discussionsByLineCode;
+
return discussions[lineCode] || [];
};
@@ -105,6 +108,44 @@ export const shouldRenderInlineCommentRow = (state, getters) => line => {
return lineDiscussions.every(discussion => discussion.expanded);
};
+/**
+ * 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/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 5b3b9f8776f..0d8d197bf71 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 ed09db0f3f4..ebe76c9fcda 100644
--- a/app/serializers/discussion_entity.rb
+++ b/app/serializers/discussion_entity.rb
@@ -6,6 +6,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/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js
index a1a37b342b7..bdc94131fc2 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 a59b26b2634..bfe07fda8d3 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;
@@ -319,4 +320,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);
+ });
+ });
});