From 67a6d75d428999b1bf0ca6334dde4dcb8eabffbc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 15 Sep 2020 08:22:24 -0600 Subject: [PATCH 1/6] Add component to warn when some diff files are collapsed --- .../javascripts/diffs/components/app.vue | 34 ++++++++- .../components/collapsed_files_warning.vue | 71 +++++++++++++++++++ locale/gitlab.pot | 9 +++ 3 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/diffs/components/collapsed_files_warning.vue diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 207f39340f7b4c..f88a0e617f9640 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -18,6 +18,7 @@ import TreeList from './tree_list.vue'; import HiddenFilesWarning from './hidden_files_warning.vue'; import MergeConflictWarning from './merge_conflict_warning.vue'; +import CollapsedFilesWarning from './collapsed_files_warning.vue'; import { TREE_LIST_WIDTH_STORAGE_KEY, @@ -37,6 +38,7 @@ export default { NoChanges, HiddenFilesWarning, MergeConflictWarning, + CollapsedFilesWarning, CommitWidget, TreeList, GlLoadingIcon, @@ -114,6 +116,7 @@ export default { return { treeWidth, diffFilesLength: 0, + collapsedWarningDismissed: false, }; }, computed: { @@ -142,7 +145,7 @@ export default { 'canMerge', 'hasConflicts', ]), - ...mapGetters('diffs', ['isParallelView', 'currentDiffIndex']), + ...mapGetters('diffs', ['hasCollapsedFile', 'isParallelView', 'currentDiffIndex']), ...mapGetters(['isNotesFetched', 'getNoteableData']), diffs() { if (!this.viewDiffsFileByFile) { @@ -188,6 +191,23 @@ export default { return currentFileNumber < diffFiles.length ? currentFileNumber + 1 : null; }, + visibleWarning() { + let visible = false; + + if (this.renderOverflowWarning) { + visible = 'overflow'; + } else if (this.isDiffHead && this.hasConflicts) { + visible = 'merge-conflict'; + } else if ( + this.hasCollapsedFile && + !this.collapsedWarningDismissed && + !this.viewDiffsFileByFile + ) { + visible = 'collapsed'; + } + + return visible; + }, }, watch: { commit(newCommit, oldCommit) { @@ -401,6 +421,9 @@ export default { this.toggleShowTreeList(false); } }, + dismissCollapsedWarning() { + this.collapsedWarningDismissed = true; + }, }, minTreeWidth: MIN_TREE_WIDTH, maxTreeWidth: MAX_TREE_WIDTH, @@ -418,18 +441,23 @@ export default { /> +
+import { mapActions } from 'vuex'; + +import { GlAlert, GlButton } from '@gitlab/ui'; + +import { CENTERED_LIMITED_CONTAINER_CLASSES } from '../constants'; + +export default { + components: { + GlAlert, + GlButton, + }, + props: { + limited: { + type: Boolean, + required: false, + default: false, + }, + dismissed: { + type: Boolean, + required: false, + default: false, + }, + }, + data() { + return { + isDismissed: this.dismissed, + }; + }, + computed: { + containerClasses() { + return { + [CENTERED_LIMITED_CONTAINER_CLASSES]: this.limited, + }; + }, + }, + + methods: { + ...mapActions('diffs', ['expandAllFiles']), + dismiss() { + this.isDismissed = true; + this.$emit('dismiss'); + }, + expand() { + this.expandAllFiles(); + this.dismiss(); + }, + }, +}; + + + diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a59786d97ca3aa..906cf574b3facb 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10374,6 +10374,9 @@ msgstr "" msgid "Expand all" msgstr "" +msgid "Expand all files" +msgstr "" + msgid "Expand approvers" msgstr "" @@ -11216,6 +11219,9 @@ msgstr "" msgid "Footer message" msgstr "" +msgid "For a faster browsing experience, some files are collapsed by default." +msgstr "" + msgid "For help setting up the Service Desk for your instance, please contact an administrator." msgstr "" @@ -23556,6 +23562,9 @@ msgstr "" msgid "Solution" msgstr "" +msgid "Some changes are not shown" +msgstr "" + msgid "Some child epics may be hidden due to applied filters" msgstr "" -- GitLab From ea9426254d8a743bdf29f3e4b8d95642ca0bda7f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 3 Sep 2020 06:41:42 -0600 Subject: [PATCH 2/6] Add a changelog for the new collapsed files warning --- changelogs/unreleased/feature-collapsed-files-warning.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-collapsed-files-warning.yml diff --git a/changelogs/unreleased/feature-collapsed-files-warning.yml b/changelogs/unreleased/feature-collapsed-files-warning.yml new file mode 100644 index 00000000000000..8fc1cc6b701a15 --- /dev/null +++ b/changelogs/unreleased/feature-collapsed-files-warning.yml @@ -0,0 +1,5 @@ +--- +title: Add a warning when any diff files are collapsed +merge_request: 40752 +author: +type: added -- GitLab From dd066c1046e70f708ca517c7a4f5ac35f6a33419 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 3 Sep 2020 06:42:02 -0600 Subject: [PATCH 3/6] Test the diffs app showing collapsed files warning --- spec/frontend/diffs/components/app_spec.js | 64 +++++++++---- .../collapsed_files_warning_spec.js | 90 +++++++++++++++++++ 2 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 spec/frontend/diffs/components/collapsed_files_warning_spec.js diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 5c2b684e165e0c..cd3a6aa0e28431 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -9,6 +9,7 @@ import NoChanges from '~/diffs/components/no_changes.vue'; import DiffFile from '~/diffs/components/diff_file.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; +import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import TreeList from '~/diffs/components/tree_list.vue'; import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '~/diffs/constants'; @@ -22,6 +23,10 @@ const TEST_ENDPOINT = `${TEST_HOST}/diff/endpoint`; const COMMIT_URL = '[BASE URL]/OLD'; const UPDATED_COMMIT_URL = '[BASE URL]/NEW'; +function getCollapsedFilesWarning(wrapper) { + return wrapper.find(CollapsedFilesWarning); +} + describe('diffs/components/app', () => { const oldMrTabs = window.mrTabs; let store; @@ -668,24 +673,51 @@ describe('diffs/components/app', () => { ); }); - it('should render hidden files warning if render overflow warning is present', () => { - createComponent({}, ({ state }) => { - state.diffs.renderOverflowWarning = true; - state.diffs.realSize = '5'; - state.diffs.plainDiffPath = 'plain diff path'; - state.diffs.emailPatchPath = 'email patch path'; - state.diffs.size = 1; + describe('warnings', () => { + describe('hidden files', () => { + it('should render hidden files warning if render overflow warning is present', () => { + createComponent({}, ({ state }) => { + state.diffs.renderOverflowWarning = true; + state.diffs.realSize = '5'; + state.diffs.plainDiffPath = 'plain diff path'; + state.diffs.emailPatchPath = 'email patch path'; + state.diffs.size = 1; + }); + + expect(wrapper.find(HiddenFilesWarning).exists()).toBe(true); + expect(wrapper.find(HiddenFilesWarning).props()).toEqual( + expect.objectContaining({ + total: '5', + plainDiffPath: 'plain diff path', + emailPatchPath: 'email patch path', + visible: 1, + }), + ); + }); }); - expect(wrapper.find(HiddenFilesWarning).exists()).toBe(true); - expect(wrapper.find(HiddenFilesWarning).props()).toEqual( - expect.objectContaining({ - total: '5', - plainDiffPath: 'plain diff path', - emailPatchPath: 'email patch path', - visible: 1, - }), - ); + describe('collapsed files', () => { + it('should render the collapsed files warning if there are any collapsed files', () => { + createComponent({}, ({ state }) => { + state.diffs.diffFiles = [{ viewer: { collapsed: true } }]; + }); + + expect(getCollapsedFilesWarning(wrapper).exists()).toBe(true); + }); + + it('should not render the collapsed files warning if the user has dismissed the alert already', async () => { + createComponent({}, ({ state }) => { + state.diffs.diffFiles = [{ viewer: { collapsed: true } }]; + }); + + expect(getCollapsedFilesWarning(wrapper).exists()).toBe(true); + + wrapper.vm.collapsedWarningDismissed = true; + await wrapper.vm.$nextTick(); + + expect(getCollapsedFilesWarning(wrapper).exists()).toBe(false); + }); + }); }); it('should display commit widget if store has a commit', () => { diff --git a/spec/frontend/diffs/components/collapsed_files_warning_spec.js b/spec/frontend/diffs/components/collapsed_files_warning_spec.js new file mode 100644 index 00000000000000..90382d448cab59 --- /dev/null +++ b/spec/frontend/diffs/components/collapsed_files_warning_spec.js @@ -0,0 +1,90 @@ +import Vuex from 'vuex'; +import { shallowMount, mount, createLocalVue } from '@vue/test-utils'; +import createStore from '~/diffs/store/modules'; +import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; +import { CENTERED_LIMITED_CONTAINER_CLASSES } from '~/diffs/constants'; + +const propsData = { + limited: true, + mergeable: true, + resolutionPath: 'a-path', +}; +const limitedClasses = CENTERED_LIMITED_CONTAINER_CLASSES.split(' '); + +function getAlertActionButton(wrapper) { + return wrapper.find('.gl-alert-actions button.gl-alert-action:first-child').element; +} + +function getAlertCloseButton(wrapper) { + return wrapper.find('[data-testid="close-icon"]').element.parentNode; +} + +describe('CollapsedFilesWarning', () => { + const localVue = createLocalVue(); + let store; + let wrapper; + + localVue.use(Vuex); + + const createComponent = (props = {}, { full } = { full: false }) => { + const mounter = full ? mount : shallowMount; + store = new Vuex.Store({ + modules: { + diffs: createStore(), + }, + }); + + wrapper = mounter(CollapsedFilesWarning, { + propsData: { ...propsData, ...props }, + localVue, + store, + }); + }; + + afterEach(() => { + wrapper.destroy(); + }); + + it.each` + limited | containerClasses + ${true} | ${limitedClasses} + ${false} | ${[]} + `( + 'has the correct container classes when limited is $limited', + ({ limited, containerClasses }) => { + createComponent({ limited }); + + expect(wrapper.classes()).toEqual(containerClasses); + }, + ); + + it.each` + present | dismissed + ${false} | ${true} + ${true} | ${false} + `('toggles the alert when dismissed is $dismissed', ({ present, dismissed }) => { + createComponent({ dismissed }); + + expect(wrapper.find('[data-testid="root"]').exists()).toBe(present); + }); + + it('dismisses the component when the alert "x" is clicked', () => { + createComponent({}, { full: true }); + + expect(wrapper.vm.isDismissed).toBe(false); + + getAlertCloseButton(wrapper).click(); + + expect(wrapper.vm.isDismissed).toBe(true); + }); + + it('triggers the expandAllFiles action when the alert action button is clicked', () => { + createComponent({}, { full: true }); + + jest.spyOn(wrapper.vm.$store, 'dispatch').mockReturnValue(undefined); + + getAlertActionButton(wrapper).click(); + + expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/expandAllFiles', undefined); + }); +}); -- GitLab From 1838b74832e77f9f02afefee630ba4cfda11e299 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 15 Sep 2020 08:39:51 -0600 Subject: [PATCH 4/6] Move alert strings into constants (and $options) --- .../javascripts/diffs/components/app.vue | 20 +++++++++++++------ app/assets/javascripts/diffs/constants.js | 5 +++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index f88a0e617f9640..dd5addbf1e3361 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -28,6 +28,9 @@ import { TREE_HIDE_STATS_WIDTH, MR_TREE_SHOW_KEY, CENTERED_LIMITED_CONTAINER_CLASSES, + ALERT_OVERFLOW_HIDDEN, + ALERT_MERGE_CONFLICT, + ALERT_COLLAPSED_FILES, } from '../constants'; export default { @@ -47,6 +50,11 @@ export default { GlSprintf, }, mixins: [glFeatureFlagsMixin()], + alerts: { + ALERT_OVERFLOW_HIDDEN, + ALERT_MERGE_CONFLICT, + ALERT_COLLAPSED_FILES, + }, props: { endpoint: { type: String, @@ -195,15 +203,15 @@ export default { let visible = false; if (this.renderOverflowWarning) { - visible = 'overflow'; + visible = this.$options.alerts.ALERT_OVERFLOW_HIDDEN; } else if (this.isDiffHead && this.hasConflicts) { - visible = 'merge-conflict'; + visible = this.$options.alerts.ALERT_MERGE_CONFLICT; } else if ( this.hasCollapsedFile && !this.collapsedWarningDismissed && !this.viewDiffsFileByFile ) { - visible = 'collapsed'; + visible = this.$options.alerts.ALERT_COLLAPSED_FILES; } return visible; @@ -441,20 +449,20 @@ export default { /> diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index a79a385b9bc204..dc97d9993da21b 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -68,6 +68,11 @@ export const DIFFS_PER_PAGE = 20; export const DIFF_COMPARE_BASE_VERSION_INDEX = -1; export const DIFF_COMPARE_HEAD_VERSION_INDEX = -2; +// Diff View Alerts +export const ALERT_OVERFLOW_HIDDEN = 'overflow'; +export const ALERT_MERGE_CONFLICT = 'merge-conflict'; +export const ALERT_COLLAPSED_FILES = 'collapsed'; + // State machine states export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; -- GitLab From 229b5e44afa7103b1864febd1644f5cd462f9941 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 15 Sep 2020 10:28:39 -0600 Subject: [PATCH 5/6] Update to GitLab-recommended finders Of note here: `.vm` is only available on certain types of elements, and the close button of the alert is not one of those elements. That's why this uses the native browser API to trigger a click event, rather than the synthetic Vue `$emit` like in the second test. --- .../components/collapsed_files_warning_spec.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/spec/frontend/diffs/components/collapsed_files_warning_spec.js b/spec/frontend/diffs/components/collapsed_files_warning_spec.js index 90382d448cab59..31df3460c74950 100644 --- a/spec/frontend/diffs/components/collapsed_files_warning_spec.js +++ b/spec/frontend/diffs/components/collapsed_files_warning_spec.js @@ -11,14 +11,6 @@ const propsData = { }; const limitedClasses = CENTERED_LIMITED_CONTAINER_CLASSES.split(' '); -function getAlertActionButton(wrapper) { - return wrapper.find('.gl-alert-actions button.gl-alert-action:first-child').element; -} - -function getAlertCloseButton(wrapper) { - return wrapper.find('[data-testid="close-icon"]').element.parentNode; -} - describe('CollapsedFilesWarning', () => { const localVue = createLocalVue(); let store; @@ -26,6 +18,10 @@ describe('CollapsedFilesWarning', () => { localVue.use(Vuex); + const getAlertActionButton = () => + wrapper.find(CollapsedFilesWarning).find('button.gl-alert-action:first-child'); + const getAlertCloseButton = () => wrapper.find(CollapsedFilesWarning).find('button'); + const createComponent = (props = {}, { full } = { full: false }) => { const mounter = full ? mount : shallowMount; store = new Vuex.Store({ @@ -73,7 +69,7 @@ describe('CollapsedFilesWarning', () => { expect(wrapper.vm.isDismissed).toBe(false); - getAlertCloseButton(wrapper).click(); + getAlertCloseButton().element.click(); expect(wrapper.vm.isDismissed).toBe(true); }); @@ -83,7 +79,7 @@ describe('CollapsedFilesWarning', () => { jest.spyOn(wrapper.vm.$store, 'dispatch').mockReturnValue(undefined); - getAlertActionButton(wrapper).click(); + getAlertActionButton().vm.$emit('click'); expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/expandAllFiles', undefined); }); -- GitLab From 3c25004361fc81b557f335d0115072ad53c4fc1e Mon Sep 17 00:00:00 2001 From: Himanshu Kapoor Date: Thu, 17 Sep 2020 08:15:54 +0530 Subject: [PATCH 6/6] Avoid evaluating internal data or computed props Instead check for output --- .../diffs/components/collapsed_files_warning_spec.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/frontend/diffs/components/collapsed_files_warning_spec.js b/spec/frontend/diffs/components/collapsed_files_warning_spec.js index 31df3460c74950..670eab5472f95c 100644 --- a/spec/frontend/diffs/components/collapsed_files_warning_spec.js +++ b/spec/frontend/diffs/components/collapsed_files_warning_spec.js @@ -64,14 +64,16 @@ describe('CollapsedFilesWarning', () => { expect(wrapper.find('[data-testid="root"]').exists()).toBe(present); }); - it('dismisses the component when the alert "x" is clicked', () => { + it('dismisses the component when the alert "x" is clicked', async () => { createComponent({}, { full: true }); - expect(wrapper.vm.isDismissed).toBe(false); + expect(wrapper.find('[data-testid="root"]').exists()).toBe(true); getAlertCloseButton().element.click(); - expect(wrapper.vm.isDismissed).toBe(true); + await wrapper.vm.$nextTick(); + + expect(wrapper.find('[data-testid="root"]').exists()).toBe(false); }); it('triggers the expandAllFiles action when the alert action button is clicked', () => { -- GitLab