diff options
author | Phil Hughes <me@iamphill.com> | 2018-12-13 13:57:45 +0300 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2018-12-13 13:57:45 +0300 |
commit | ee2f3cac35e630cb5d5aef93752e3eb28b6852c2 (patch) | |
tree | ecd852085055cc4b8e930644e618d239a9e6887d | |
parent | 6b68d82fbf2da3f73d411e7a6fbda16cd3b94604 (diff) |
Fix diff changes empty state
The empty state now only gets shown when no files exist in the branch.
If the user is reviewing 2 versions with no files, we don't show the state.
Refactors the diff app spec to use Vue test utils.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/48635
-rw-r--r-- | app/assets/javascripts/diffs/components/app.vue | 18 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/components/no_changes.vue | 53 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/index.js | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show.html.haml | 3 | ||||
-rw-r--r-- | changelogs/unreleased/diff-empty-state-fixes.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/javascripts/diffs/components/app_spec.js | 91 | ||||
-rw-r--r-- | spec/javascripts/diffs/components/no_changes_spec.js | 41 |
8 files changed, 169 insertions, 47 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index bf9244df7f7..f0e82b1ed27 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -42,6 +42,11 @@ export default { type: Object, required: true, }, + changesEmptyStateIllustration: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -63,7 +68,7 @@ export default { plainDiffPath: state => state.diffs.plainDiffPath, emailPatchPath: state => state.diffs.emailPatchPath, }), - ...mapState('diffs', ['showTreeList', 'isLoading']), + ...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']), ...mapGetters('diffs', ['isParallelView']), ...mapGetters(['isNotesFetched', 'getNoteableData']), targetBranch() { @@ -79,6 +84,13 @@ export default { showCompareVersions() { return this.mergeRequestDiffs && this.mergeRequestDiff; }, + renderDiffFiles() { + return ( + this.diffFiles.length > 0 || + (this.startVersion && + this.startVersion.version_index === this.mergeRequestDiff.version_index) + ); + }, }, watch: { diffViewType() { @@ -191,7 +203,7 @@ export default { <div v-show="showTreeList" class="diff-tree-list"><tree-list /></div> <div class="diff-files-holder"> <commit-widget v-if="commit" :commit="commit" /> - <template v-if="diffFiles.length > 0"> + <template v-if="renderDiffFiles"> <diff-file v-for="file in diffFiles" :key="file.newPath" @@ -199,7 +211,7 @@ export default { :can-current-user-fork="canCurrentUserFork" /> </template> - <no-changes v-else /> + <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" /> </div> </div> </div> diff --git a/app/assets/javascripts/diffs/components/no_changes.vue b/app/assets/javascripts/diffs/components/no_changes.vue index 25ec157ed25..47e9627a957 100644 --- a/app/assets/javascripts/diffs/components/no_changes.vue +++ b/app/assets/javascripts/diffs/components/no_changes.vue @@ -1,34 +1,51 @@ <script> -import { mapState } from 'vuex'; -import emptyImage from '~/../../views/shared/icons/_mr_widget_empty_state.svg'; +import { mapGetters } from 'vuex'; +import _ from 'underscore'; +import { GlButton } from '@gitlab/ui'; +import { __, sprintf } from '~/locale'; export default { - data() { - return { - emptyImage, - }; + components: { + GlButton, + }, + props: { + changesEmptyStateIllustration: { + type: String, + required: true, + }, }, computed: { - ...mapState({ - sourceBranch: state => state.notes.noteableData.source_branch, - targetBranch: state => state.notes.noteableData.target_branch, - newBlobPath: state => state.notes.noteableData.new_blob_path, - }), + ...mapGetters(['getNoteableData']), + emptyStateText() { + return sprintf( + __( + 'No changes between %{ref_start}%{source_branch}%{ref_end} and %{ref_start}%{target_branch}%{ref_end}', + ), + { + ref_start: '<span class="ref-name">', + ref_end: '</span>', + source_branch: _.escape(this.getNoteableData.source_branch), + target_branch: _.escape(this.getNoteableData.target_branch), + }, + false, + ); + }, }, }; </script> <template> - <div class="row empty-state nothing-here-block"> - <div class="col-xs-12"> - <div class="svg-content"><span v-html="emptyImage"></span></div> + <div class="row empty-state"> + <div class="col-12"> + <div class="svg-content svg-250"><img :src="changesEmptyStateIllustration" /></div> </div> - <div class="col-xs-12"> + <div class="col-12"> <div class="text-content text-center"> - No changes between <span class="ref-name">{{ sourceBranch }}</span> and - <span class="ref-name">{{ targetBranch }}</span> + <span v-html="emptyStateText"></span> <div class="text-center"> - <a :href="newBlobPath" class="btn btn-success"> {{ __('Create commit') }} </a> + <gl-button :href="getNoteableData.new_blob_path" variant="success">{{ + __('Create commit') + }}</gl-button> </div> </div> </div> diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 06ef4207d85..915cacb374f 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -17,6 +17,7 @@ export default function initDiffsApp(store) { endpoint: dataset.endpoint, projectPath: dataset.projectPath, currentUser: JSON.parse(dataset.currentUserData) || {}, + changesEmptyStateIllustration: dataset.changesEmptyStateIllustration, }; }, computed: { @@ -31,6 +32,7 @@ export default function initDiffsApp(store) { currentUser: this.currentUser, projectPath: this.projectPath, shouldShow: this.activeTab === 'diffs', + changesEmptyStateIllustration: this.changesEmptyStateIllustration, }, }); }, diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index 4ebb029e48b..c178206dda4 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -77,7 +77,8 @@ #js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?, endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters), current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json, - project_path: project_path(@merge_request.project)} } + project_path: project_path(@merge_request.project), + changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg') } } .mr-loading-status = spinner diff --git a/changelogs/unreleased/diff-empty-state-fixes.yml b/changelogs/unreleased/diff-empty-state-fixes.yml new file mode 100644 index 00000000000..0d347dd17e4 --- /dev/null +++ b/changelogs/unreleased/diff-empty-state-fixes.yml @@ -0,0 +1,5 @@ +--- +title: Fixed merge request diffs empty states +merge_request: +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 792b6c8cecc..a042caf0af1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4368,6 +4368,9 @@ msgstr "" msgid "No changes" msgstr "" +msgid "No changes between %{ref_start}%{source_branch}%{ref_end} and %{ref_start}%{target_branch}%{ref_end}" +msgstr "" + msgid "No connection could be made to a Gitaly Server, please check your logs!" msgstr "" diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js index 1e2f7ff4fd8..a2cbc0f3c72 100644 --- a/spec/javascripts/diffs/components/app_spec.js +++ b/spec/javascripts/diffs/components/app_spec.js @@ -1,33 +1,44 @@ -import Vue from 'vue'; -import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import Vuex from 'vuex'; +import { shallowMount, createLocalVue } from '@vue/test-utils'; import { TEST_HOST } from 'spec/test_constants'; import App from '~/diffs/components/app.vue'; +import NoChanges from '~/diffs/components/no_changes.vue'; +import DiffFile from '~/diffs/components/diff_file.vue'; import createDiffsStore from '../create_diffs_store'; describe('diffs/components/app', () => { const oldMrTabs = window.mrTabs; - const Component = Vue.extend(App); - + let store; let vm; - beforeEach(() => { - // setup globals (needed for component to mount :/) - window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']); - window.mrTabs.expandViewContainer = jasmine.createSpy(); - window.location.hash = 'ABC_123'; + function createComponent(props = {}, extendStore = () => {}) { + const localVue = createLocalVue(); - // setup component - const store = createDiffsStore(); + localVue.use(Vuex); + + store = createDiffsStore(); store.state.diffs.isLoading = false; - vm = mountComponentWithStore(Component, { - store, - props: { + extendStore(store); + + vm = shallowMount(localVue.extend(App), { + localVue, + propsData: { endpoint: `${TEST_HOST}/diff/endpoint`, projectPath: 'namespace/project', currentUser: {}, + changesEmptyStateIllustration: '', + ...props, }, + store, }); + } + + beforeEach(() => { + // setup globals (needed for component to mount :/) + window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']); + window.mrTabs.expandViewContainer = jasmine.createSpy(); + window.location.hash = 'ABC_123'; }); afterEach(() => { @@ -35,21 +46,53 @@ describe('diffs/components/app', () => { window.mrTabs = oldMrTabs; // reset component - vm.$destroy(); + vm.destroy(); }); it('does not show commit info', () => { - expect(vm.$el).not.toContainElement('.blob-commit-info'); + createComponent(); + + expect(vm.contains('.blob-commit-info')).toBe(false); }); it('sets highlighted row if hash exists in location object', done => { - vm.$props.shouldShow = true; - - vm.$nextTick() - .then(() => { - expect(vm.$store.state.diffs.highlightedRow).toBe('ABC_123'); - }) - .then(done) - .catch(done.fail); + createComponent({ + shouldShow: true, + }); + + // Component uses $nextTick so we wait until that has finished + setTimeout(() => { + expect(store.state.diffs.highlightedRow).toBe('ABC_123'); + + done(); + }); + }); + + describe('empty state', () => { + it('renders empty state when no diff files exist', () => { + createComponent(); + + expect(vm.contains(NoChanges)).toBe(true); + }); + + it('does not render empty state when diff files exist', () => { + createComponent({}, () => { + store.state.diffs.diffFiles.push({ + id: 1, + }); + }); + + expect(vm.contains(NoChanges)).toBe(false); + expect(vm.findAll(DiffFile).length).toBe(1); + }); + + it('does not render empty state when versions match', () => { + createComponent({}, () => { + store.state.diffs.startVersion = { version_index: 1 }; + store.state.diffs.mergeRequestDiff = { version_index: 1 }; + }); + + expect(vm.contains(NoChanges)).toBe(false); + }); }); }); diff --git a/spec/javascripts/diffs/components/no_changes_spec.js b/spec/javascripts/diffs/components/no_changes_spec.js index 7237274eb43..e45d34bf9d5 100644 --- a/spec/javascripts/diffs/components/no_changes_spec.js +++ b/spec/javascripts/diffs/components/no_changes_spec.js @@ -1 +1,40 @@ -// TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 +import { createLocalVue, shallowMount } from '@vue/test-utils'; +import Vuex from 'vuex'; +import { createStore } from '~/mr_notes/stores'; +import NoChanges from '~/diffs/components/no_changes.vue'; + +describe('Diff no changes empty state', () => { + let vm; + + function createComponent(extendStore = () => {}) { + const localVue = createLocalVue(); + localVue.use(Vuex); + + const store = createStore(); + extendStore(store); + + vm = shallowMount(localVue.extend(NoChanges), { + localVue, + store, + propsData: { + changesEmptyStateIllustration: '', + }, + }); + } + + afterEach(() => { + vm.destroy(); + }); + + it('prevents XSS', () => { + createComponent(store => { + // eslint-disable-next-line no-param-reassign + store.state.notes.noteableData = { + source_branch: '<script>alert("test");</script>', + target_branch: '<script>alert("test");</script>', + }; + }); + + expect(vm.contains('script')).toBe(false); + }); +}); |