Merge branch...

Merge branch '30497-race-condition-in-discussions-json-request-could-lead-to-notes-showing-inconsistently-with-selected-notes-filter' into 'master'

Fix notes race condition when linking to specific note

See merge request gitlab-org/gitlab!17777

app/assets/javascripts/lib/utils/url_utility.js

@@ -87,6 +87,14 @@ export function getLocationHash(url = window.location.href) {
   return hashIndex === -1 ? null : url.substring(hashIndex + 1);
 }
 
+/**
+ * Returns a boolean indicating whether the URL hash contains the given string value
+ */
+export function doesHashExistInUrl(hashName) {
+  const hash = getLocationHash();
+  return hash && hash.includes(hashName);
+}
+
 /**
  * Apply the fragment to the given url by returning a new url string that includes
  * the fragment. If the given url already contains a fragment, the original fragment

app/assets/javascripts/notes/components/discussion_filter.vue

 <script>
 import $ from 'jquery';
 import { mapGetters, mapActions } from 'vuex';
-import { getLocationHash } from '../../lib/utils/url_utility';
+import { getLocationHash, doesHashExistInUrl } from '../../lib/utils/url_utility';
 import Icon from '~/vue_shared/components/icon.vue';
 import {
   DISCUSSION_FILTERS_DEFAULT_VALUE,
   HISTORY_ONLY_FILTER_VALUE,
   DISCUSSION_TAB_LABEL,
   DISCUSSION_FILTER_TYPES,
+  NOTE_UNDERSCORE,
 } from '../constants';
 import notesEventHub from '../event_hub';
 
@@ -28,7 +29,9 @@ export default {
   },
   data() {
     return {
-      currentValue: this.selectedValue,
+      currentValue: doesHashExistInUrl(NOTE_UNDERSCORE)
+        ? DISCUSSION_FILTERS_DEFAULT_VALUE
+        : this.selectedValue,
       defaultValue: DISCUSSION_FILTERS_DEFAULT_VALUE,
       displayFilters: true,
     };
@@ -50,7 +53,6 @@ export default {
 
     notesEventHub.$on('dropdownSelect', this.selectFilter);
     window.addEventListener('hashchange', this.handleLocationHash);
-    this.handleLocationHash();
   },
   mounted() {
     this.toggleCommentsForm();

app/assets/javascripts/notes/components/notes_app.vue

 <script>
 import { __ } from '~/locale';
 import { mapGetters, mapActions } from 'vuex';
-import { getLocationHash } from '../../lib/utils/url_utility';
+import { getLocationHash, doesHashExistInUrl } from '../../lib/utils/url_utility';
 import Flash from '../../flash';
 import * as constants from '../constants';
 import eventHub from '../event_hub';
@@ -156,19 +156,17 @@ export default {
 
       this.isFetching = true;
 
-      return this.fetchDiscussions({ path: this.getNotesDataByProp('discussionsPath') })
-        .then(() => {
-          this.initPolling();
-        })
+      return this.fetchDiscussions(this.getFetchDiscussionsConfig())
+        .then(this.initPolling)
         .then(() => {
           this.setLoadingState(false);
           this.setNotesFetchedState(true);
           eventHub.$emit('fetchedNotesData');
           this.isFetching = false;
         })
-        .then(() => this.$nextTick())
-        .then(() => this.startTaskList())
-        .then(() => this.checkLocationHash())
+        .then(this.$nextTick)
+        .then(this.startTaskList)
+        .then(this.checkLocationHash)
         .catch(() => {
           this.setLoadingState(false);
           this.setNotesFetchedState(true);
@@ -199,9 +197,20 @@ export default {
     },
     startReplying(discussionId) {
       return this.convertToDiscussion(discussionId)
-        .then(() => this.$nextTick())
+        .then(this.$nextTick)
         .then(() => eventHub.$emit('startReplying', discussionId));
     },
+    getFetchDiscussionsConfig() {
+      const defaultConfig = { path: this.getNotesDataByProp('discussionsPath') };
+
+      if (doesHashExistInUrl(constants.NOTE_UNDERSCORE)) {
+        return Object.assign({}, defaultConfig, {
+          filter: constants.DISCUSSION_FILTERS_DEFAULT_VALUE,
+          persistFilter: false,
+        });
+      }
+      return defaultConfig;
+    },
   },
   systemNote: constants.SYSTEM_NOTE,
 };

app/assets/javascripts/notes/constants.js

@@ -8,8 +8,6 @@ export const OPENED = 'opened';
 export const REOPENED = 'reopened';
 export const CLOSED = 'closed';
 export const MERGED = 'merged';
-export const EMOJI_THUMBSUP = 'thumbsup';
-export const EMOJI_THUMBSDOWN = 'thumbsdown';
 export const ISSUE_NOTEABLE_TYPE = 'issue';
 export const EPIC_NOTEABLE_TYPE = 'epic';
 export const MERGE_REQUEST_NOTEABLE_TYPE = 'MergeRequest';
@@ -19,6 +17,7 @@ export const DESCRIPTION_TYPE = 'changed the description';
 export const HISTORY_ONLY_FILTER_VALUE = 2;
 export const DISCUSSION_FILTERS_DEFAULT_VALUE = 0;
 export const DISCUSSION_TAB_LABEL = 'show';
+export const NOTE_UNDERSCORE = 'note_';
 
 export const NOTEABLE_TYPE_MAPPING = {
   Issue: ISSUE_NOTEABLE_TYPE,

changelogs/unreleased/30497-race-condition-in-discussions-json-request-could-lead-to-notes-show.yml

+---
+title: Fix notes race condition when linking to specific note
+merge_request: 17777
+author:
+type: fixed

spec/frontend/lib/utils/url_utility_spec.js

@@ -136,6 +136,24 @@ describe('URL utility', () => {
     });
   });
 
+  describe('doesHashExistInUrl', () => {
+    it('should return true when the given string exists in the URL hash', () => {
+      setWindowLocation({
+        href: 'https://gitlab.com/gitlab-org/gitlab-test/issues/1#note_1',
+      });
+
+      expect(urlUtils.doesHashExistInUrl('note_')).toBe(true);
+    });
+
+    it('should return false when the given string does not exist in the URL hash', () => {
+      setWindowLocation({
+        href: 'https://gitlab.com/gitlab-org/gitlab-test/issues/1#note_1',
+      });
+
+      expect(urlUtils.doesHashExistInUrl('doesnotexist')).toBe(false);
+    });
+  });
+
   describe('setUrlFragment', () => {
     it('should set fragment when url has no fragment', () => {
       const url = urlUtils.setUrlFragment('/home/feature', 'usage');

spec/javascripts/notes/components/discussion_filter_spec.js

@@ -160,5 +160,28 @@ describe('DiscussionFilter component', () => {
         done();
       });
     });
+
+    it('fetches discussions when there is a hash', done => {
+      window.location.hash = `note_${discussionMock.notes[0].id}`;
+      vm.currentValue = discussionFiltersMock[2].value;
+      spyOn(vm, 'selectFilter');
+      vm.handleLocationHash();
+
+      vm.$nextTick(() => {
+        expect(vm.selectFilter).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('does not fetch discussions when there is no hash', done => {
+      window.location.hash = '';
+      spyOn(vm, 'selectFilter');
+      vm.handleLocationHash();
+
+      vm.$nextTick(() => {
+        expect(vm.selectFilter).not.toHaveBeenCalled();
+        done();
+      });
+    });
   });
 });