From 568401ea45f66f347d4a5cd53edc13a86ad228d7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Jun 2021 15:59:52 -0600 Subject: [PATCH 1/2] Include a feature flag when checking if something is the diff head Changelog: fixed --- app/assets/javascripts/diffs/components/app.vue | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 61946d345e3ac3..3c8e5c3aff748d 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -229,7 +229,11 @@ export default { return !this.renderFileTree && !this.isParallelView && !this.isFluidLayout; }, isDiffHead() { - return parseBoolean(getParameterByName('diff_head')); + const defaultDiffHead = + window.gon?.features?.defaultMergeRefForDiffs && !getParameterByName('diff_id'); + const explicitDiffHead = parseBoolean(getParameterByName('diff_head')); + + return defaultDiffHead || explicitDiffHead; }, showFileByFileNavigation() { return this.diffFiles.length > 1 && this.viewDiffsFileByFile; -- GitLab From c4833844e4d750fec55149e3214f36b0ef11adf5 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Jun 2021 14:43:46 -0600 Subject: [PATCH 2/2] Update to use a more explicit merge conflict test `isFullChangeset` is the implied behavior we want: Only show the merge conflict warning if the user is viewing the diff at the latest version (HEAD) and seeing a diff from the BASE version (e.g.: not a subset of the diff). If they're viewing the full changeset and there are conflicts then we show the warning. Changelog: fixed --- .../javascripts/diffs/components/app.vue | 13 ++---- spec/frontend/diffs/components/app_spec.js | 46 ++++++++++++++++++- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 3c8e5c3aff748d..33cf2888c0b1ad 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -14,7 +14,7 @@ import { } from '~/behaviors/shortcuts/keybindings'; import createFlash from '~/flash'; import { isSingleViewStyle } from '~/helpers/diffs_helper'; -import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils'; +import { parseBoolean } from '~/lib/utils/common_utils'; import { updateHistory } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; @@ -186,6 +186,7 @@ export default { 'showTreeList', 'isLoading', 'startVersion', + 'latestDiff', 'currentDiffFileId', 'isTreeLoaded', 'conflictResolutionPath', @@ -228,12 +229,8 @@ export default { isLimitedContainer() { return !this.renderFileTree && !this.isParallelView && !this.isFluidLayout; }, - isDiffHead() { - const defaultDiffHead = - window.gon?.features?.defaultMergeRefForDiffs && !getParameterByName('diff_id'); - const explicitDiffHead = parseBoolean(getParameterByName('diff_head')); - - return defaultDiffHead || explicitDiffHead; + isFullChangeset() { + return this.startVersion === null && this.latestDiff; }, showFileByFileNavigation() { return this.diffFiles.length > 1 && this.viewDiffsFileByFile; @@ -256,7 +253,7 @@ export default { if (this.renderOverflowWarning) { visible = this.$options.alerts.ALERT_OVERFLOW_HIDDEN; - } else if (this.isDiffHead && this.hasConflicts) { + } else if (this.isFullChangeset && this.hasConflicts) { visible = this.$options.alerts.ALERT_MERGE_CONFLICT; } else if (this.whichCollapsedTypes.automatic && !this.viewDiffsFileByFile) { visible = this.$options.alerts.ALERT_COLLAPSED_FILES; diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 8a1c5547581ecb..b5eb3e1713c1fa 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -6,14 +6,19 @@ import Vue, { nextTick } from 'vue'; import Vuex from 'vuex'; import { TEST_HOST } from 'spec/test_constants'; import App from '~/diffs/components/app.vue'; -import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; import DiffFile from '~/diffs/components/diff_file.vue'; -import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; import NoChanges from '~/diffs/components/no_changes.vue'; import TreeList from '~/diffs/components/tree_list.vue'; +/* eslint-disable import/order */ +/* You know what: sometimes alphabetical isn't the best order */ +import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; +import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; +import MergeConflictWarning from '~/diffs/components/merge_conflict_warning.vue'; +/* eslint-enable import/order */ + import axios from '~/lib/utils/axios_utils'; import * as urlUtils from '~/lib/utils/url_utility'; import createDiffsStore from '../create_diffs_store'; @@ -541,6 +546,43 @@ describe('diffs/components/app', () => { expect(getCollapsedFilesWarning(wrapper).exists()).toBe(false); }); }); + + describe('merge conflicts', () => { + it('should render the merge conflicts banner if viewing the whole changeset and there are conflicts', () => { + createComponent({}, ({ state }) => { + Object.assign(state.diffs, { + latestDiff: true, + startVersion: null, + hasConflicts: true, + canMerge: false, + conflictResolutionPath: 'path', + }); + }); + + expect(wrapper.find(MergeConflictWarning).exists()).toBe(true); + }); + + it.each` + prop | value + ${'latestDiff'} | ${false} + ${'startVersion'} | ${'notnull'} + ${'hasConflicts'} | ${false} + `( + "should not render if any of the MR properties aren't correct - like $prop: $value", + ({ prop, value }) => { + createComponent({}, ({ state }) => { + Object.assign(state.diffs, { + latestDiff: true, + startVersion: null, + hasConflicts: true, + [prop]: value, + }); + }); + + expect(wrapper.find(MergeConflictWarning).exists()).toBe(false); + }, + ); + }); }); it('should display commit widget if store has a commit', () => { -- GitLab