From b689ddd9b6d7ebed2f4d014a77ee223df2d3491b Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 7 Aug 2019 15:45:23 +0000 Subject: Do not persist notes filter when auto-switching Send a `persist_filter: false` param to backend when opening links to notes and auto-switching to show all notes --- .../notes/components/discussion_filter.vue | 10 +++++--- .../javascripts/notes/services/notes_service.js | 7 ++++-- app/assets/javascripts/notes/stores/actions.js | 8 +++---- app/controllers/concerns/issuable_actions.rb | 3 ++- ...nt-persisting-auto-switch-discussion-filter.yml | 6 +++++ spec/features/user_opens_link_to_comment_spec.rb | 5 ++++ spec/javascripts/notes/stores/actions_spec.js | 27 ++++++++++++++++++++++ .../issuable_notes_filter_shared_examples.rb | 10 +++++++- 8 files changed, 65 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/61445-prevent-persisting-auto-switch-discussion-filter.yml diff --git a/app/assets/javascripts/notes/components/discussion_filter.vue b/app/assets/javascripts/notes/components/discussion_filter.vue index eb3fbbe1385..743684e7046 100644 --- a/app/assets/javascripts/notes/components/discussion_filter.vue +++ b/app/assets/javascripts/notes/components/discussion_filter.vue @@ -61,7 +61,7 @@ export default { }, methods: { ...mapActions(['filterDiscussion', 'setCommentsDisabled', 'setTargetNoteHash']), - selectFilter(value) { + selectFilter(value, persistFilter = true) { const filter = parseInt(value, 10); // close dropdown @@ -69,7 +69,11 @@ export default { if (filter === this.currentValue) return; this.currentValue = filter; - this.filterDiscussion({ path: this.getNotesDataByProp('discussionsPath'), filter }); + this.filterDiscussion({ + path: this.getNotesDataByProp('discussionsPath'), + filter, + persistFilter, + }); this.toggleCommentsForm(); }, toggleDropdown() { @@ -85,7 +89,7 @@ export default { const hash = getLocationHash(); if (/^note_/.test(hash) && this.currentValue !== DISCUSSION_FILTERS_DEFAULT_VALUE) { - this.selectFilter(this.defaultValue); + this.selectFilter(this.defaultValue, false); this.toggleDropdown(); // close dropdown this.setTargetNoteHash(hash); } diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index 9e0392110b6..3d239d8cb6b 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/app/assets/javascripts/notes/services/notes_service.js @@ -5,8 +5,11 @@ import * as constants from '../constants'; Vue.use(VueResource); export default { - fetchDiscussions(endpoint, filter) { - const config = filter !== undefined ? { params: { notes_filter: filter } } : null; + fetchDiscussions(endpoint, filter, persistFilter = true) { + const config = + filter !== undefined + ? { params: { notes_filter: filter, persist_filter: persistFilter } } + : null; return Vue.http.get(endpoint, config); }, replyToDiscussion(endpoint, data) { diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 762a87ce0ff..b7857997d42 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -46,9 +46,9 @@ export const setNotesFetchedState = ({ commit }, state) => export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data); -export const fetchDiscussions = ({ commit, dispatch }, { path, filter }) => +export const fetchDiscussions = ({ commit, dispatch }, { path, filter, persistFilter }) => service - .fetchDiscussions(path, filter) + .fetchDiscussions(path, filter, persistFilter) .then(res => res.json()) .then(discussions => { commit(types.SET_INITIAL_DISCUSSIONS, discussions); @@ -411,9 +411,9 @@ export const setLoadingState = ({ commit }, data) => { commit(types.SET_NOTES_LOADING_STATE, data); }; -export const filterDiscussion = ({ dispatch }, { path, filter }) => { +export const filterDiscussion = ({ dispatch }, { path, filter, persistFilter }) => { dispatch('setLoadingState', true); - dispatch('fetchDiscussions', { path, filter }) + dispatch('fetchDiscussions', { path, filter, persistFilter }) .then(() => { dispatch('setLoadingState', false); dispatch('setNotesFetchedState', true); diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 398cb728e05..b86e4451a7e 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -127,7 +127,8 @@ module IssuableActions # GitLab Geo does not expect database UPDATE or INSERT statements to happen # on GET requests. # This is just a fail-safe in case notes_filter is sent via GET request in GitLab Geo. - if Gitlab::Database.read_only? + # In some cases, we also force the filter to not be persisted with the `persist_filter` param + if Gitlab::Database.read_only? || params[:persist_filter] == 'false' notes_filter_param || current_user&.notes_filter_for(issuable) else notes_filter = current_user&.set_notes_filter(notes_filter_param, issuable) || notes_filter_param diff --git a/changelogs/unreleased/61445-prevent-persisting-auto-switch-discussion-filter.yml b/changelogs/unreleased/61445-prevent-persisting-auto-switch-discussion-filter.yml new file mode 100644 index 00000000000..58e29212462 --- /dev/null +++ b/changelogs/unreleased/61445-prevent-persisting-auto-switch-discussion-filter.yml @@ -0,0 +1,6 @@ +--- +title: Prevent discussion filter from persisting to `Show all activity` when opening + links to notes +merge_request: 31229 +author: +type: fixed diff --git a/spec/features/user_opens_link_to_comment_spec.rb b/spec/features/user_opens_link_to_comment_spec.rb index f1e07e55799..9533a4fe40d 100644 --- a/spec/features/user_opens_link_to_comment_spec.rb +++ b/spec/features/user_opens_link_to_comment_spec.rb @@ -18,8 +18,13 @@ describe 'User opens link to comment', :js do visit Gitlab::UrlBuilder.build(note) + wait_for_requests + expect(page.find('#discussion-filter-dropdown')).to have_content('Show all activity') expect(page).not_to have_content('Something went wrong while fetching comments') + + # Auto-switching to show all notes shouldn't be persisted + expect(user.reload.notes_filter_for(note.noteable)).to eq(UserPreference::NOTES_FILTERS[:only_activity]) end end diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index c461c28a37b..e55aa0e965a 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -892,4 +892,31 @@ describe('Actions Notes Store', () => { }); }); }); + + describe('filterDiscussion', () => { + const path = 'some-discussion-path'; + const filter = 0; + + beforeEach(() => { + dispatch.and.returnValue(new Promise(() => {})); + }); + + it('fetches discussions with filter and persistFilter false', () => { + actions.filterDiscussion({ dispatch }, { path, filter, persistFilter: false }); + + expect(dispatch.calls.allArgs()).toEqual([ + ['setLoadingState', true], + ['fetchDiscussions', { path, filter, persistFilter: false }], + ]); + }); + + it('fetches discussions with filter and persistFilter true', () => { + actions.filterDiscussion({ dispatch }, { path, filter, persistFilter: true }); + + expect(dispatch.calls.allArgs()).toEqual([ + ['setLoadingState', true], + ['fetchDiscussions', { path, filter, persistFilter: true }], + ]); + }); + }); }); diff --git a/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb b/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb index fb22498f84f..26ed86bfe26 100644 --- a/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb +++ b/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb @@ -41,7 +41,15 @@ shared_examples 'issuable notes filter' do get :discussions, params: params.merge(notes_filter: notes_filter) - expect(user.reload.notes_filter_for(issuable)).to eq(0) + expect(user.reload.notes_filter_for(issuable)).to eq(UserPreference::NOTES_FILTERS[:all_notes]) + end + + it 'does not set notes filter when persist_filter param is false' do + notes_filter = UserPreference::NOTES_FILTERS[:only_comments] + + get :discussions, params: params.merge(notes_filter: notes_filter, persist_filter: false) + + expect(user.reload.notes_filter_for(issuable)).to eq(UserPreference::NOTES_FILTERS[:all_notes]) end it 'returns only user comments' do -- cgit v1.2.3