From c2fffdef684049a73ff8c8d9c40ca46d37c404a9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 8 Mar 2023 01:35:15 -0700 Subject: [PATCH 1/2] Add an action to parse the note hash post-load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that the UI has totally asynchronous load of ALL of the UI data, we need to trigger parsing that used to only happen once. That is: - Discussions are loaded async - Files are loaded async - Any number of files may be present, from 0—total. Reading the note hash could extract a note ID that doesn't yet exist in the `notes` store. Or, the existing note may point to a file that hasn't yet loaded. For the former, we will simply trigger this new action (which extracts the URL hash) when the notes indicate they have updated. For the latter, we will trigger `fetchFileByFile` to ensure that the file that this note points to is actually loaded. `fetchFileByFile` will quickly short-circuit if it has already been loaded, so there's no harm in triggering it. --- app/assets/javascripts/diffs/store/actions.js | 20 ++++++++ locale/gitlab.pot | 3 ++ spec/frontend/diffs/store/actions_spec.js | 48 +++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 6c461021f86587..a6a275030e6dcf 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -896,6 +896,26 @@ export function moveToNeighboringCommit({ dispatch, state }, { direction }) { } } +export const rereadNoteHash = ({ state, dispatch }) => { + const urlHash = window?.location?.hash; + + if (isUrlHashNoteLink(urlHash)) { + dispatch('setCurrentDiffFileIdFromNote', urlHash.split('_').pop()) + .then(() => { + if (state.viewDiffsFileByFile) { + dispatch('fetchFileByFile'); + } + }) + .catch(() => { + createAlert({ + message: s__( + 'MergeRequest|Encountered an issue while trying to fetch the single file diff for the discussion.', + ), + }); + }); + } +}; + export const setCurrentDiffFileIdFromNote = ({ commit, getters, rootGetters }, noteId) => { const note = rootGetters.notesById[noteId]; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d9d15871da24bf..1a6d8989d05ad7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27328,6 +27328,9 @@ msgstr "" msgid "MergeRequest|Compare %{target} and %{source}" msgstr "" +msgid "MergeRequest|Encountered an issue while trying to fetch the single file diff for the discussion." +msgstr "" + msgid "MergeRequest|Encountered an issue while trying to fetch the single file diff." msgstr "" diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 351e62fb6e009a..b988472f947a7a 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -1598,6 +1598,54 @@ describe('DiffsStoreActions', () => { ); }); + describe('rereadNoteHash', () => { + beforeEach(() => { + window.location.hash = 'note_123'; + }); + + it('dispatches setCurrentDiffFileIdFromNote if the hash is a note URL', () => { + window.location.hash = 'note_123'; + + return testAction( + diffActions.rereadNoteHash, + {}, + {}, + [], + [{ type: 'setCurrentDiffFileIdFromNote', payload: '123' }], + ); + }); + + it('dispatches fetchFileByFile if the app is in fileByFile mode', () => { + window.location.hash = 'note_123'; + + return testAction( + diffActions.rereadNoteHash, + {}, + { viewDiffsFileByFile: true }, + [], + [{ type: 'setCurrentDiffFileIdFromNote', payload: '123' }, { type: 'fetchFileByFile' }], + ); + }); + + it('does not try to fetch the diff file if the app is not in fileByFile mode', () => { + window.location.hash = 'note_123'; + + return testAction( + diffActions.rereadNoteHash, + {}, + { viewDiffsFileByFile: false }, + [], + [{ type: 'setCurrentDiffFileIdFromNote', payload: '123' }], + ); + }); + + it('does nothing if the hash is not a note URL', () => { + window.location.hash = 'abcdef1234567890'; + + return testAction(diffActions.rereadNoteHash, {}, {}, [], []); + }); + }); + describe('setCurrentDiffFileIdFromNote', () => { it('commits SET_CURRENT_DIFF_FILE', () => { const commit = jest.fn(); -- GitLab From ecc581d54c38c0ffac9e28181bf4ae070ffc4b28 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 30 Mar 2023 14:42:14 -0600 Subject: [PATCH 2/2] Update localization text from TW review --- app/assets/javascripts/diffs/i18n.js | 3 +++ app/assets/javascripts/diffs/store/actions.js | 6 ++---- locale/gitlab.pot | 6 +++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/diffs/i18n.js b/app/assets/javascripts/diffs/i18n.js index d8812de12d48a3..95155181c78ccc 100644 --- a/app/assets/javascripts/diffs/i18n.js +++ b/app/assets/javascripts/diffs/i18n.js @@ -4,6 +4,9 @@ export const GENERIC_ERROR = __('Something went wrong on our end. Please try aga export const LOAD_SINGLE_DIFF_FAILED = s__( 'MergeRequest|Encountered an issue while trying to fetch the single file diff.', ); +export const DISCUSSION_SINGLE_DIFF_FAILED = s__( + "MergeRequest|Can't fetch the single file diff for the discussion. Please reload this page.", +); export const DIFF_FILE_HEADER = { optionsDropdownTitle: __('Options'), diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index a6a275030e6dcf..f6552d391931bc 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -50,7 +50,7 @@ import { TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, } from '../constants'; -import { LOAD_SINGLE_DIFF_FAILED } from '../i18n'; +import { DISCUSSION_SINGLE_DIFF_FAILED, LOAD_SINGLE_DIFF_FAILED } from '../i18n'; import eventHub from '../event_hub'; import { isCollapsed } from '../utils/diff_file'; import { markFileReview, setReviewsForMergeRequest } from '../utils/file_reviews'; @@ -908,9 +908,7 @@ export const rereadNoteHash = ({ state, dispatch }) => { }) .catch(() => { createAlert({ - message: s__( - 'MergeRequest|Encountered an issue while trying to fetch the single file diff for the discussion.', - ), + message: DISCUSSION_SINGLE_DIFF_FAILED, }); }); } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1a6d8989d05ad7..f854259d9c9c86 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27313,6 +27313,9 @@ msgstr "" msgid "MergeRequest|Approved by @%{username}" msgstr "" +msgid "MergeRequest|Can't fetch the single file diff for the discussion. Please reload this page." +msgstr "" + msgid "MergeRequest|Can't show this merge request because of an internal error. Contact your administrator." msgstr "" @@ -27328,9 +27331,6 @@ msgstr "" msgid "MergeRequest|Compare %{target} and %{source}" msgstr "" -msgid "MergeRequest|Encountered an issue while trying to fetch the single file diff for the discussion." -msgstr "" - msgid "MergeRequest|Encountered an issue while trying to fetch the single file diff." msgstr "" -- GitLab