diff options
author | André Luís <aluis@gitlab.com> | 2018-07-31 01:50:53 +0100 |
---|---|---|
committer | André Luís <aluis@gitlab.com> | 2018-08-01 13:45:15 +0100 |
commit | 8693aa139dcdc5d4ed8939d12b360731f5f9dab8 (patch) | |
tree | a95be3bd3baf011bb43f73879d3c2545619fedd2 | |
parent | 113f9f337c678129e811fcdfb418ba011ac1eb2b (diff) | |
download | gitlab-ce-8693aa139dcdc5d4ed8939d12b360731f5f9dab8.tar.gz |
Revert "Merge branch '48817-fix-mr-changes-discussion-navigation' into 'master'"
This reverts commit ced005f330419ec81657e852c5cb9124fdb29fbb, reversing
changes made to 9b01b293ce5ddbaeedaf014cdc804af2c5e86416.
10 files changed, 68 insertions, 407 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index e64d5511d78..20483161033 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -30,7 +30,6 @@ export default { :render-header="false" :render-diff-file="false" :always-expanded="true" - :discussions-by-diff-order="true" /> </ul> </div> diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ad6e7cf501d..6385b75e557 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -5,20 +5,19 @@ import resolvedSvg from 'icons/_icon_status_success_solid.svg'; import mrIssueSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionSvg from 'icons/_next_discussion.svg'; import { pluralize } from '../../lib/utils/text_utility'; -import discussionNavigation from '../mixins/discussion_navigation'; +import { scrollToElement } from '../../lib/utils/common_utils'; import tooltip from '../../vue_shared/directives/tooltip'; export default { directives: { tooltip, }, - mixins: [discussionNavigation], computed: { ...mapGetters([ 'getUserData', 'getNoteableData', 'discussionCount', - 'firstUnresolvedDiscussionId', + 'unresolvedDiscussions', 'resolvedDiscussionCount', ]), isLoggedIn() { @@ -36,6 +35,11 @@ export default { resolveAllDiscussionsIssuePath() { return this.getNoteableData.create_issue_to_resolve_discussions_path; }, + firstUnresolvedDiscussionId() { + const item = this.unresolvedDiscussions[0] || {}; + + return item.id; + }, }, created() { this.resolveSvg = resolveSvg; @@ -46,10 +50,22 @@ export default { methods: { ...mapActions(['expandDiscussion']), jumpToFirstUnresolvedDiscussion() { - const diffTab = window.mrTabs.currentAction === 'diffs'; - const discussionId = this.firstUnresolvedDiscussionId(diffTab); + const discussionId = this.firstUnresolvedDiscussionId; + if (!discussionId) { + return; + } + + const el = document.querySelector(`[data-discussion-id="${discussionId}"]`); + const activeTab = window.mrTabs.currentAction; + + if (activeTab === 'commits' || activeTab === 'pipelines') { + window.mrTabs.activateTab('show'); + } - this.jumpToDiscussion(discussionId); + if (el) { + this.expandDiscussion({ discussionId }); + scrollToElement(el); + } }, }, }; diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 0fe1c16854a..2f1a68731c7 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -1,8 +1,9 @@ <script> +import _ from 'underscore'; import { mapActions, mapGetters } from 'vuex'; import resolveDiscussionsSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionsSvg from 'icons/_next_discussion.svg'; -import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; +import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils'; import { truncateSha } from '~/lib/utils/text_utility'; import systemNote from '~/vue_shared/components/notes/system_note.vue'; import { s__ } from '~/locale'; @@ -20,7 +21,6 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder import autosave from '../mixins/autosave'; import noteable from '../mixins/noteable'; import resolvable from '../mixins/resolvable'; -import discussionNavigation from '../mixins/discussion_navigation'; import tooltip from '../../vue_shared/directives/tooltip'; export default { @@ -40,7 +40,7 @@ export default { directives: { tooltip, }, - mixins: [autosave, noteable, resolvable, discussionNavigation], + mixins: [autosave, noteable, resolvable], props: { discussion: { type: Object, @@ -61,11 +61,6 @@ export default { required: false, default: false, }, - discussionsByDiffOrder: { - type: Boolean, - required: false, - default: false, - }, }, data() { return { @@ -80,12 +75,7 @@ export default { 'discussionCount', 'resolvedDiscussionCount', 'allDiscussions', - 'unresolvedDiscussionsIdsByDiff', - 'unresolvedDiscussionsIdsByDate', 'unresolvedDiscussions', - 'unresolvedDiscussionsIdsOrdered', - 'nextUnresolvedDiscussionId', - 'isLastUnresolvedDiscussion', ]), transformedDiscussion() { return { @@ -136,10 +126,6 @@ export default { hasMultipleUnresolvedDiscussions() { return this.unresolvedDiscussions.length > 1; }, - showJumpToNextDiscussion() { - return this.hasMultipleUnresolvedDiscussions && - !this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder); - }, shouldRenderDiffs() { const { diffDiscussion, diffFile } = this.transformedDiscussion; @@ -256,10 +242,21 @@ Please check your network connection and try again.`; }); }, jumpToNextDiscussion() { - const nextId = - this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder); + const discussionIds = this.allDiscussions.map(d => d.id); + const unresolvedIds = this.unresolvedDiscussions.map(d => d.id); + const currentIndex = discussionIds.indexOf(this.discussion.id); + const remainingAfterCurrent = discussionIds.slice(currentIndex + 1); + const nextIndex = _.findIndex(remainingAfterCurrent, id => unresolvedIds.indexOf(id) > -1); + + if (nextIndex > -1) { + const nextId = remainingAfterCurrent[nextIndex]; + const el = document.querySelector(`[data-discussion-id="${nextId}"]`); - this.jumpToDiscussion(nextId); + if (el) { + this.expandDiscussion({ discussionId: nextId }); + scrollToElement(el); + } + } }, }, }; @@ -401,7 +398,7 @@ Please check your network connection and try again.`; </a> </div> <div - v-if="showJumpToNextDiscussion" + v-if="hasMultipleUnresolvedDiscussions" class="btn-group" role="group"> <button diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js deleted file mode 100644 index f7c4deee1f8..00000000000 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ /dev/null @@ -1,29 +0,0 @@ -import { scrollToElement } from '~/lib/utils/common_utils'; - -export default { - methods: { - jumpToDiscussion(id) { - if (id) { - const activeTab = window.mrTabs.currentAction; - const selector = - activeTab === 'diffs' - ? `ul.notes[data-discussion-id="${id}"]` - : `div.discussion[data-discussion-id="${id}"]`; - const el = document.querySelector(selector); - - if (activeTab === 'commits' || activeTab === 'pipelines') { - window.mrTabs.activateTab('show'); - } - - if (el) { - this.expandDiscussion({ discussionId: id }); - - scrollToElement(el); - return true; - } - } - - return false; - }, - }, -}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 0d8d197bf71..e9e95dd4219 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -70,9 +70,6 @@ export const allDiscussions = (state, getters) => { return Object.values(resolved).concat(unresolved); }; -export const allResolvableDiscussions = (state, getters) => - getters.allDiscussions.filter(d => !d.individual_note && d.resolvable); - export const resolvedDiscussionsById = state => { const map = {}; @@ -89,51 +86,6 @@ export const resolvedDiscussionsById = state => { return map; }; -// Gets Discussions IDs ordered by the date of their initial note -export const unresolvedDiscussionsIdsByDate = (state, getters) => - getters.allResolvableDiscussions - .filter(d => !d.resolved) - .sort((a, b) => { - const aDate = new Date(a.notes[0].created_at); - const bDate = new Date(b.notes[0].created_at); - - if (aDate < bDate) { - return -1; - } - - return aDate === bDate ? 0 : 1; - }) - .map(d => d.id); - -// Gets Discussions IDs ordered by their position in the diff -// -// Sorts the array of resolvable yet unresolved discussions by -// comparing file names first. If file names are the same, compares -// line numbers. -export const unresolvedDiscussionsIdsByDiff = (state, getters) => - getters.allResolvableDiscussions - .filter(d => !d.resolved) - .sort((a, b) => { - if (!a.diff_file || !b.diff_file) { - return 0; - } - - // Get file names comparison result - const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path); - - // Get the line numbers, to compare within the same file - const aLines = [a.position.formatter.new_line, a.position.formatter.old_line]; - const bLines = [b.position.formatter.new_line, b.position.formatter.old_line]; - - return filenameComparison < 0 || - (filenameComparison === 0 && - // .max() because one of them might be zero (if removed/added) - Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1])) - ? -1 - : 1; - }) - .map(d => d.id); - export const resolvedDiscussionCount = (state, getters) => { const resolvedMap = getters.resolvedDiscussionsById; @@ -150,42 +102,5 @@ export const discussionTabCounter = state => { return all.length; }; -// Returns the list of discussion IDs ordered according to given parameter -// @param {Boolean} diffOrder - is ordered by diff? -export const unresolvedDiscussionsIdsOrdered = (state, getters) => diffOrder => { - if (diffOrder) { - return getters.unresolvedDiscussionsIdsByDiff; - } - return getters.unresolvedDiscussionsIdsByDate; -}; - -// Checks if a given discussion is the last in the current order (diff or date) -// @param {Boolean} discussionId - id of the discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const lastDiscussionId = idsOrdered[idsOrdered.length - 1]; - - return lastDiscussionId === discussionId; -}; - -// Gets the ID of the discussion following the one provided, respecting order (diff or date) -// @param {Boolean} discussionId - id of the current discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - - return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0]; -}; - -// @param {Boolean} diffOrder - is ordered by diff? -export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => { - if (diffOrder) { - return getters.unresolvedDiscussionsIdsByDiff[0]; - } - return getters.unresolvedDiscussionsIdsByDate[0]; -}; - // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 2d268ecab58..bf4d5396df9 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -342,9 +342,8 @@ describe 'Merge request > User resolves diff notes and discussions', :js do end end - it 'shows jump to next discussion button, apart from the last one' do - expect(page).to have_selector('.discussion-reply-holder', count: 2) - expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1) + it 'shows jump to next discussion button' do + expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn')) end it 'displays next discussion even if hidden' do diff --git a/spec/javascripts/notes/components/discussion_counter_spec.js b/spec/javascripts/notes/components/discussion_counter_spec.js index d09bc5037ef..a3869cc6498 100644 --- a/spec/javascripts/notes/components/discussion_counter_spec.js +++ b/spec/javascripts/notes/components/discussion_counter_spec.js @@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => { discussions, }); setFixtures(` - <div class="discussion" data-discussion-id="${firstDiscussionId}"></div> + <div data-discussion-id="${firstDiscussionId}"></div> `); vm.jumpToFirstUnresolvedDiscussion(); diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js index 2a01bd85520..f3f50aed232 100644 --- a/spec/javascripts/notes/components/noteable_discussion_spec.js +++ b/spec/javascripts/notes/components/noteable_discussion_spec.js @@ -14,7 +14,6 @@ describe('noteable_discussion component', () => { preloadFixtures(discussionWithTwoUnresolvedNotes); beforeEach(() => { - window.mrTabs = {}; store = createStore(); store.dispatch('setNoteableData', noteableDataMock); store.dispatch('setNotesData', notesDataMock); @@ -107,29 +106,33 @@ describe('noteable_discussion component', () => { describe('methods', () => { describe('jumpToNextDiscussion', () => { - it('expands next unresolved discussion', done => { - const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0]; - discussion2.resolved = false; - discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to) - vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]); - window.mrTabs.currentAction = 'show'; - - Vue.nextTick() - .then(() => { - spyOn(vm, 'expandDiscussion').and.stub(); - - const nextDiscussionId = discussion2.id; - - setFixtures(` - <div class="discussion" data-discussion-id="${nextDiscussionId}"></div> - `); + it('expands next unresolved discussion', () => { + spyOn(vm, 'expandDiscussion').and.stub(); + const discussions = [ + discussionMock, + { + ...discussionMock, + id: discussionMock.id + 1, + notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }], + }, + { + ...discussionMock, + id: discussionMock.id + 2, + notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }], + }, + ]; + const nextDiscussionId = discussionMock.id + 2; + store.replaceState({ + ...store.state, + discussions, + }); + setFixtures(` + <div data-discussion-id="${nextDiscussionId}"></div> + `); - vm.jumpToNextDiscussion(); + vm.jumpToNextDiscussion(); - expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); - }) - .then(done) - .catch(done.fail); + expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); }); }); }); diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 67f6a9629d9..be2a8ba67fe 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -1168,87 +1168,3 @@ export const collapsedSystemNotes = [ diff_discussion: false, }, ]; - -export const discussion1 = { - id: 'abc1', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'about.md', - }, - position: { - formatter: { - new_line: 50, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-04T16:25:41.749Z', - }, - ], -}; - -export const resolvedDiscussion1 = { - id: 'abc1', - resolvable: true, - resolved: true, - diff_file: { - file_path: 'about.md', - }, - position: { - formatter: { - new_line: 50, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-04T16:25:41.749Z', - }, - ], -}; - -export const discussion2 = { - id: 'abc2', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'README.md', - }, - position: { - formatter: { - new_line: null, - old_line: 20, - }, - }, - notes: [ - { - created_at: '2018-07-04T12:05:41.749Z', - }, - ], -}; - -export const discussion3 = { - id: 'abc3', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'README.md', - }, - position: { - formatter: { - new_line: 21, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-05T17:25:41.749Z', - }, - ], -}; - -export const unresolvableDiscussion = { - resolvable: false, -}; diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index 7f8ede51508..41599e00122 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -5,11 +5,6 @@ import { noteableDataMock, individualNote, collapseNotesMock, - discussion1, - discussion2, - discussion3, - resolvedDiscussion1, - unresolvableDiscussion, } from '../mock_data'; const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json'; @@ -114,154 +109,4 @@ describe('Getters Notes Store', () => { expect(getters.isNotesFetched(state)).toBeFalsy(); }); }); - - describe('allResolvableDiscussions', () => { - it('should return only resolvable discussions in same order', () => { - const localGetters = { - allDiscussions: [ - discussion3, - unresolvableDiscussion, - discussion1, - unresolvableDiscussion, - discussion2, - ], - }; - - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([ - discussion3, - discussion1, - discussion2, - ]); - }); - - it('should return empty array if there are no resolvable discussions', () => { - const localGetters = { - allDiscussions: [unresolvableDiscussion, unresolvableDiscussion], - }; - - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsByDiff', () => { - it('should return all discussions IDs in diff order', () => { - const localGetters = { - allResolvableDiscussions: [discussion3, discussion1, discussion2], - }; - - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([ - 'abc1', - 'abc2', - 'abc3', - ]); - }); - - it('should return empty array if all discussions have been resolved', () => { - const localGetters = { - allResolvableDiscussions: [resolvedDiscussion1], - }; - - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsByDate', () => { - it('should return all discussions in date ascending order', () => { - const localGetters = { - allResolvableDiscussions: [discussion3, discussion1, discussion2], - }; - - expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([ - 'abc2', - 'abc1', - 'abc3', - ]); - }); - - it('should return empty array if all discussions have been resolved', () => { - const localGetters = { - allResolvableDiscussions: [resolvedDiscussion1], - }; - - expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsOrdered', () => { - const localGetters = { - unresolvedDiscussionsIdsByDate: ['123', '456'], - unresolvedDiscussionsIdsByDiff: ['abc', 'def'], - }; - - it('should return IDs ordered by diff when diffOrder param is true', () => { - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(true)).toEqual([ - 'abc', - 'def', - ]); - }); - - it('should return IDs ordered by date when diffOrder param is not true', () => { - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(false)).toEqual([ - '123', - '456', - ]); - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(undefined)).toEqual([ - '123', - '456', - ]); - }); - }); - - describe('isLastUnresolvedDiscussion', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; - - it('should return true if the discussion id provided is the last', () => { - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('789')).toBe(true); - }); - - it('should return false if the discussion id provided is not the last', () => { - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('123')).toBe(false); - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('456')).toBe(false); - }); - }); - - describe('nextUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; - - it('should return the ID of the discussion after the ID provided', () => { - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined); - }); - }); - - describe('firstUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsByDate: ['123', '456'], - unresolvedDiscussionsIdsByDiff: ['abc', 'def'], - }; - - it('should return the first discussion id by diff when diffOrder param is true', () => { - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(true)).toBe('abc'); - }); - - it('should return the first discussion id by date when diffOrder param is not true', () => { - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(false)).toBe('123'); - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(undefined)).toBe('123'); - }); - - it('should be falsy if all discussions are resolved', () => { - const localGettersFalsy = { - unresolvedDiscussionsIdsByDiff: [], - unresolvedDiscussionsIdsByDate: [], - }; - - expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(true)).toBeFalsy(); - expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(false)).toBeFalsy(); - }); - }); }); |