Merge branch '61445-prevent-persisting-auto-switch-discussion-filter' into 'master'

Do not persist notes filter when auto-switching

See merge request gitlab-org/gitlab-ce!31229

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);
       }

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) {

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);

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

changelogs/unreleased/61445-prevent-persisting-auto-switch-discussion-filter.yml

+---
+title: Prevent discussion filter from persisting to `Show all activity` when opening
+  links to notes
+merge_request: 31229
+author:
+type: fixed

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
 

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 }],
+      ]);
+    });
+  });
 });

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