Commit 5c215606 authored by Mark Florian's avatar Mark Florian

Merge branch 'nfriend-add-validation-to-release-assets-form' into 'master'

Add client-side validation to Release asset edit form

See merge request gitlab-org/gitlab!28493
parents 02ed457d b2173f6e
......@@ -232,3 +232,11 @@ export const truncateNamespace = (string = '') => {
return namespace;
};
/**
* Tests that the input is a String and has at least
* one non-whitespace character
* @param {String} obj The object to test
* @returns {Boolean}
*/
export const hasContent = obj => isString(obj) && obj.trim() !== '';
<script>
import { mapState, mapActions } from 'vuex';
import { GlDeprecatedButton, GlLink, GlFormInput, GlFormGroup } from '@gitlab/ui';
import { mapState, mapActions, mapGetters } from 'vuex';
import { GlNewButton, GlFormInput, GlFormGroup } from '@gitlab/ui';
import { escape as esc } from 'lodash';
import { __, sprintf } from '~/locale';
import MarkdownField from '~/vue_shared/components/markdown/field.vue';
......@@ -15,8 +15,7 @@ export default {
components: {
GlFormInput,
GlFormGroup,
GlDeprecatedButton,
GlLink,
GlNewButton,
MarkdownField,
AssetLinksForm,
},
......@@ -27,12 +26,14 @@ export default {
computed: {
...mapState('detail', [
'isFetchingRelease',
'isUpdatingRelease',
'fetchError',
'markdownDocsPath',
'markdownPreviewPath',
'releasesPagePath',
'updateReleaseApiDocsPath',
]),
...mapGetters('detail', ['isValid']),
showForm() {
return !this.isFetchingRelease && !this.fetchError;
},
......@@ -87,6 +88,9 @@ export default {
showAssetLinksForm() {
return this.glFeatures.releaseAssetLinkEditing;
},
isSaveChangesDisabled() {
return this.isUpdatingRelease || !this.isValid;
},
},
created() {
this.fetchRelease();
......@@ -163,17 +167,19 @@ export default {
<asset-links-form v-if="showAssetLinksForm" />
<div class="d-flex pt-3">
<gl-deprecated-button
class="mr-auto js-submit-button"
<gl-new-button
class="mr-auto js-no-auto-disable"
category="primary"
variant="success"
type="submit"
:aria-label="__('Save changes')"
:disabled="isSaveChangesDisabled"
>
{{ __('Save changes') }}
</gl-deprecated-button>
<gl-link :href="cancelPath" class="js-cancel-button btn btn-default">
</gl-new-button>
<gl-new-button :href="cancelPath" class="js-cancel-button">
{{ __('Cancel') }}
</gl-link>
</gl-new-button>
</div>
</form>
</div>
......
<script>
import { mapState, mapActions } from 'vuex';
import { mapState, mapActions, mapGetters } from 'vuex';
import {
GlSprintf,
GlLink,
GlFormGroup,
GlDeprecatedButton,
GlNewButton,
GlIcon,
GlTooltipDirective,
GlFormInput,
......@@ -12,13 +12,14 @@ import {
export default {
name: 'AssetLinksForm',
components: { GlSprintf, GlLink, GlFormGroup, GlDeprecatedButton, GlIcon, GlFormInput },
components: { GlSprintf, GlLink, GlFormGroup, GlNewButton, GlIcon, GlFormInput },
directives: { GlTooltip: GlTooltipDirective },
computed: {
...mapState('detail', ['release', 'releaseAssetsDocsPath']),
...mapGetters('detail', ['validationErrors']),
},
created() {
this.addEmptyAssetLink();
this.ensureAtLeastOneLink();
},
methods: {
...mapActions('detail', [
......@@ -32,6 +33,7 @@ export default {
},
onRemoveClicked(linkId) {
this.removeAssetLink(linkId);
this.ensureAtLeastOneLink();
},
onUrlInput(linkIdToUpdate, newUrl) {
this.updateAssetLinkUrl({ linkIdToUpdate, newUrl });
......@@ -39,6 +41,37 @@ export default {
onLinkTitleInput(linkIdToUpdate, newName) {
this.updateAssetLinkName({ linkIdToUpdate, newName });
},
hasDuplicateUrl(link) {
return Boolean(this.getLinkErrors(link).isDuplicate);
},
hasBadFormat(link) {
return Boolean(this.getLinkErrors(link).isBadFormat);
},
hasEmptyUrl(link) {
return Boolean(this.getLinkErrors(link).isUrlEmpty);
},
hasEmptyName(link) {
return Boolean(this.getLinkErrors(link).isNameEmpty);
},
getLinkErrors(link) {
return this.validationErrors.assets.links[link.id] || {};
},
isUrlValid(link) {
return !this.hasDuplicateUrl(link) && !this.hasBadFormat(link) && !this.hasEmptyUrl(link);
},
isNameValid(link) {
return !this.hasEmptyName(link);
},
/**
* Make sure the form is never completely empty by adding an
* empty row if the form contains 0 links
*/
ensureAtLeastOneLink() {
if (this.release.assets.links.length === 0) {
this.addEmptyAssetLink();
}
},
},
};
</script>
......@@ -69,60 +102,93 @@ export default {
<p>
{{
__(
'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance.',
'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed.',
)
}}
</p>
<div
v-for="(link, index) in release.assets.links"
:key="link.id"
class="d-flex flex-column flex-sm-row align-items-stretch align-items-sm-end"
class="row flex-column flex-sm-row align-items-stretch align-items-sm-start"
>
<gl-form-group
class="url-field form-group flex-grow-1 mr-sm-4"
class="url-field form-group col"
:label="__('URL')"
:label-for="`asset-url-${index}`"
>
<gl-form-input
:id="`asset-url-${index}`"
ref="urlInput"
:value="link.url"
type="text"
class="form-control"
:state="isUrlValid(link)"
@change="onUrlInput(link.id, $event)"
/>
<template #invalid-feedback>
<span v-if="hasEmptyUrl(link)" class="invalid-feedback d-inline">
{{ __('URL is required') }}
</span>
<span v-else-if="hasBadFormat(link)" class="invalid-feedback d-inline">
<gl-sprintf
:message="
__(
'URL must start with %{codeStart}http://%{codeEnd}, %{codeStart}https://%{codeEnd}, or %{codeStart}ftp://%{codeEnd}',
)
"
>
<template #code="{ content }">
<code>{{ content }}</code>
</template>
</gl-sprintf>
</span>
<span v-else-if="hasDuplicateUrl(link)" class="invalid-feedback d-inline">
{{ __('This URL is already used for another link; duplicate URLs are not allowed') }}
</span>
</template>
</gl-form-group>
<gl-form-group
class="link-title-field flex-grow-1 mr-sm-4"
class="link-title-field col"
:label="__('Link title')"
:label-for="`asset-link-name-${index}`"
>
<gl-form-input
:id="`asset-link-name-${index}`"
ref="nameInput"
:value="link.name"
type="text"
class="form-control"
:state="isNameValid(link)"
@change="onLinkTitleInput(link.id, $event)"
/>
<template v-slot:invalid-feedback>
<span v-if="hasEmptyName(link)" class="invalid-feedback d-inline">
{{ __('Link title is required') }}
</span>
</template>
</gl-form-group>
<gl-deprecated-button
v-gl-tooltip
class="mb-5 mb-sm-3 flex-grow-0 flex-shrink-0 remove-button"
:aria-label="__('Remove asset link')"
:title="__('Remove asset link')"
@click="onRemoveClicked(link.id)"
>
<gl-icon class="m-0" name="remove" />
<span class="d-inline d-sm-none">{{ __('Remove asset link') }}</span>
</gl-deprecated-button>
<div class="mb-5 mb-sm-3 mt-sm-4 col col-sm-auto">
<gl-new-button
v-gl-tooltip
class="remove-button w-100"
:aria-label="__('Remove asset link')"
:title="__('Remove asset link')"
@click="onRemoveClicked(link.id)"
>
<gl-icon class="mr-1 mr-sm-0 mb-1" :size="16" name="remove" />
<span class="d-inline d-sm-none">{{ __('Remove asset link') }}</span>
</gl-new-button>
</div>
</div>
<gl-deprecated-button
<gl-new-button
ref="addAnotherLinkButton"
variant="link"
class="align-self-end mb-5 mb-sm-0"
@click="onAddAnotherClicked"
>
{{ __('Add another link') }}
</gl-deprecated-button>
</gl-new-button>
</div>
</template>
import { isEmpty } from 'lodash';
import { hasContent } from '~/lib/utils/text_utility';
/**
* @param {Object} link The link to test
* @returns {Boolean} `true` if the release link is empty, i.e. it has
* empty (or whitespace-only) values for both `url` and `name`.
* Otherwise, `false`.
*/
const isEmptyReleaseLink = l => !/\S/.test(l.url) && !/\S/.test(l.name);
const isEmptyReleaseLink = link => !hasContent(link.url) && !hasContent(link.name);
/** Returns all release links that aren't empty */
export const releaseLinksToCreate = state => {
......@@ -22,3 +26,67 @@ export const releaseLinksToDelete = state => {
return state.originalRelease.assets.links;
};
/** Returns all validation errors on the release object */
export const validationErrors = state => {
const errors = {
assets: {
links: {},
},
};
if (!state.release) {
return errors;
}
// Each key of this object is a URL, and the value is an
// array of Release link objects that share this URL.
// This is used for detecting duplicate URLs.
const urlToLinksMap = new Map();
state.release.assets.links.forEach(link => {
errors.assets.links[link.id] = {};
// Only validate non-empty URLs
if (isEmptyReleaseLink(link)) {
return;
}
if (!hasContent(link.url)) {
errors.assets.links[link.id].isUrlEmpty = true;
}
if (!hasContent(link.name)) {
errors.assets.links[link.id].isNameEmpty = true;
}
const normalizedUrl = link.url.trim().toLowerCase();
// Compare each URL to every other URL and flag any duplicates
if (urlToLinksMap.has(normalizedUrl)) {
// a duplicate URL was found!
// add a validation error for each link that shares this URL
const duplicates = urlToLinksMap.get(normalizedUrl);
duplicates.push(link);
duplicates.forEach(duplicateLink => {
errors.assets.links[duplicateLink.id].isDuplicate = true;
});
} else {
// no duplicate URL was found
urlToLinksMap.set(normalizedUrl, [link]);
}
if (!/^(http|https|ftp):\/\//.test(normalizedUrl)) {
errors.assets.links[link.id].isBadFormat = true;
}
});
return errors;
};
/** Returns whether or not the release object is valid */
export const isValid = (_state, getters) => {
return Object.values(getters.validationErrors.assets.links).every(isEmpty);
};
......@@ -12159,6 +12159,9 @@ msgstr ""
msgid "Link title"
msgstr ""
msgid "Link title is required"
msgstr ""
msgid "Linked emails (%{email_count})"
msgstr ""
......@@ -15002,7 +15005,7 @@ msgstr ""
msgid "Pods in use"
msgstr ""
msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance."
msgid "Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed."
msgstr ""
msgid "Preferences"
......@@ -20715,6 +20718,9 @@ msgstr ""
msgid "This Project is currently archived and read-only. Please unarchive the project first if you want to resume Pull mirroring"
msgstr ""
msgid "This URL is already used for another link; duplicate URLs are not allowed"
msgstr ""
msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention."
msgstr ""
......@@ -21811,9 +21817,15 @@ msgstr ""
msgid "URL"
msgstr ""
msgid "URL is required"
msgstr ""
msgid "URL must be a valid url (ex: https://gitlab.com)"
msgstr ""
msgid "URL must start with %{codeStart}http://%{codeEnd}, %{codeStart}https://%{codeEnd}, or %{codeStart}ftp://%{codeEnd}"
msgstr ""
msgid "URL of the external storage that will serve the repository static objects (e.g. archives, blobs, ...)."
msgstr ""
......
......@@ -224,4 +224,18 @@ describe('text_utility', () => {
});
});
});
describe('hasContent', () => {
it.each`
txt | result
${null} | ${false}
${undefined} | ${false}
${{ an: 'object' }} | ${false}
${''} | ${false}
${' \t\r\n'} | ${false}
${'hello'} | ${true}
`('returns $result for input $txt', ({ result, txt }) => {
expect(textUtils.hasContent(txt)).toEqual(result);
});
});
});
......@@ -5,14 +5,16 @@ import { release as originalRelease } from '../mock_data';
import * as commonUtils from '~/lib/utils/common_utils';
import { BACK_URL_PARAM } from '~/releases/constants';
import AssetLinksForm from '~/releases/components/asset_links_form.vue';
import { merge } from 'lodash';
describe('Release edit component', () => {
let wrapper;
let release;
let actions;
let getters;
let state;
const factory = (featureFlags = {}) => {
const factory = ({ featureFlags = {}, store: storeUpdates = {} } = {}) => {
state = {
release,
markdownDocsPath: 'path/to/markdown/docs',
......@@ -26,15 +28,30 @@ describe('Release edit component', () => {
addEmptyAssetLink: jest.fn(),
};
const store = new Vuex.Store({
modules: {
detail: {
namespaced: true,
actions,
state,
getters = {
isValid: () => true,
validationErrors: () => ({
assets: {
links: [],
},
},
});
}),
};
const store = new Vuex.Store(
merge(
{
modules: {
detail: {
namespaced: true,
actions,
state,
getters,
},
},
},
storeUpdates,
),
);
wrapper = mount(ReleaseEditApp, {
store,
......@@ -55,6 +72,8 @@ describe('Release edit component', () => {
wrapper = null;
});
const findSubmitButton = () => wrapper.find('button[type=submit]');
describe(`basic functionality tests: all tests unrelated to the "${BACK_URL_PARAM}" parameter`, () => {
beforeEach(() => {
factory();
......@@ -101,7 +120,7 @@ describe('Release edit component', () => {
});
it('renders the "Save changes" button as type="submit"', () => {
expect(wrapper.find('.js-submit-button').attributes('type')).toBe('submit');
expect(findSubmitButton().attributes('type')).toBe('submit');
});
it('calls updateRelease when the form is submitted', () => {
......@@ -143,7 +162,7 @@ describe('Release edit component', () => {
describe('when the release_asset_link_editing feature flag is disabled', () => {
beforeEach(() => {
factory({ releaseAssetLinkEditing: false });
factory({ featureFlags: { releaseAssetLinkEditing: false } });
});
it('does not render the asset links portion of the form', () => {
......@@ -153,7 +172,7 @@ describe('Release edit component', () => {
describe('when the release_asset_link_editing feature flag is enabled', () => {
beforeEach(() => {
factory({ releaseAssetLinkEditing: true });
factory({ featureFlags: { releaseAssetLinkEditing: true } });
});
it('renders the asset links portion of the form', () => {
......@@ -161,4 +180,46 @@ describe('Release edit component', () => {
});
});
});
describe('validation', () => {
describe('when the form is valid', () => {
beforeEach(() => {
factory({
store: {
modules: {
detail: {
getters: {
isValid: () => true,
},
},
},
},
});
});
it('renders the submit button as enabled', () => {
expect(findSubmitButton().attributes('disabled')).toBeUndefined();
});
});
describe('when the form is invalid', () => {
beforeEach(() => {
factory({
store: {
modules: {
detail: {
getters: {
isValid: () => false,
},
},
},
},
});
});
it('renders the submit button as disabled', () => {
expect(findSubmitButton().attributes('disabled')).toBe('disabled');
});
});
});
});
import Vuex from 'vuex';
import { mount, createLocalVue } from '@vue/test-utils';
import AssetLinksForm from '~/releases/components/asset_links_form.vue';
import { release as originalRelease } from '../mock_data';
import * as commonUtils from '~/lib/utils/common_utils';
const localVue = createLocalVue();
localVue.use(Vuex);
describe('Release edit component', () => {
let wrapper;
let release;
let actions;
let getters;
let state;
const factory = ({ release: overriddenRelease, linkErrors } = {}) => {
state = {
release: overriddenRelease || release,
releaseAssetsDocsPath: 'path/to/release/assets/docs',
};
actions = {
addEmptyAssetLink: jest.fn(),
updateAssetLinkUrl: jest.fn(),
updateAssetLinkName: jest.fn(),
removeAssetLink: jest.fn().mockImplementation((_context, linkId) => {
state.release.assets.links = state.release.assets.links.filter(l => l.id !== linkId);
}),
};
getters = {
validationErrors: () => ({
assets: {
links: linkErrors || {},
},
}),
};
const store = new Vuex.Store({
modules: {
detail: {
namespaced: true,
actions,
state,
getters,
},
},
});
wrapper = mount(AssetLinksForm, {
localVue,
store,
});
};
beforeEach(() => {
release = commonUtils.convertObjectPropsToCamelCase(originalRelease, { deep: true });
});
afterEach(() => {
wrapper.destroy();
wrapper = null;
});
describe('with a basic store state', () => {
beforeEach(() => {
factory();
});
it('calls the "addEmptyAssetLink" store method when the "Add another link" button is clicked', () => {
expect(actions.addEmptyAssetLink).not.toHaveBeenCalled();
wrapper.find({ ref: 'addAnotherLinkButton' }).vm.$emit('click');
expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1);
});
it('calls the "removeAssetLinks" store method when the remove button is clicked', () => {
expect(actions.removeAssetLink).not.toHaveBeenCalled();
wrapper.find('.remove-button').vm.$emit('click');
expect(actions.removeAssetLink).toHaveBeenCalledTimes(1);
});
it('calls the "updateAssetLinkUrl" store method when text is entered into the "URL" input field', () => {
const linkIdToUpdate = release.assets.links[0].id;
const newUrl = 'updated url';
expect(actions.updateAssetLinkUrl).not.toHaveBeenCalled();
wrapper.find({ ref: 'urlInput' }).vm.$emit('change', newUrl);
expect(actions.updateAssetLinkUrl).toHaveBeenCalledTimes(1);
expect(actions.updateAssetLinkUrl).toHaveBeenCalledWith(
expect.anything(),
{
linkIdToUpdate,
newUrl,
},
undefined,
);
});
it('calls the "updateAssetLinName" store method when text is entered into the "Link title" input field', () => {
const linkIdToUpdate = release.assets.links[0].id;
const newName = 'updated name';
expect(actions.updateAssetLinkName).not.toHaveBeenCalled();
wrapper.find({ ref: 'nameInput' }).vm.$emit('change', newName);
expect(actions.updateAssetLinkName).toHaveBeenCalledTimes(1);
expect(actions.updateAssetLinkName).toHaveBeenCalledWith(
expect.anything(),
{
linkIdToUpdate,
newName,
},
undefined,
);
});
});
describe('validation', () => {
let linkId;
beforeEach(() => {
linkId = release.assets.links[0].id;
});
const findUrlValidationMessage = () => wrapper.find('.url-field .invalid-feedback');
const findNameValidationMessage = () => wrapper.find('.link-title-field .invalid-feedback');
it('does not show any validation messages if there are no validation errors', () => {
factory();
expect(findUrlValidationMessage().exists()).toBe(false);
expect(findNameValidationMessage().exists()).toBe(false);
});
it('shows a validation error message when two links have the same URLs', () => {
factory({
linkErrors: {
[linkId]: { isDuplicate: true },
},
});
expect(findUrlValidationMessage().text()).toBe(
'This URL is already used for another link; duplicate URLs are not allowed',
);
});
it('shows a validation error message when a URL has a bad format', () => {
factory({
linkErrors: {
[linkId]: { isBadFormat: true },
},
});
expect(findUrlValidationMessage().text()).toBe(
'URL must start with http://, https://, or ftp://',
);
});
it('shows a validation error message when the URL is empty (and the title is not empty)', () => {
factory({
linkErrors: {
[linkId]: { isUrlEmpty: true },
},
});
expect(findUrlValidationMessage().text()).toBe('URL is required');
});
it('shows a validation error message when the title is empty (and the URL is not empty)', () => {
factory({
linkErrors: {
[linkId]: { isNameEmpty: true },
},
});
expect(findNameValidationMessage().text()).toBe('Link title is required');
});
});
describe('empty state', () => {
describe('when the release fetched from the API has no links', () => {
beforeEach(() => {
factory({
release: {
...release,
assets: {
links: [],
},
},
});
});
it('calls the addEmptyAssetLink store method when the component is created', () => {
expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1);
});
});
describe('when the release fetched from the API has one link', () => {
beforeEach(() => {
factory({
release: {
...release,
assets: {
links: release.assets.links.slice(0, 1),
},
},
});
});
it('does not call the addEmptyAssetLink store method when the component is created', () => {
expect(actions.addEmptyAssetLink).not.toHaveBeenCalled();
});
it('calls addEmptyAssetLink when the final link is deleted by the user', () => {
wrapper.find('.remove-button').vm.$emit('click');
expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1);
});
});
});
});
......@@ -56,4 +56,158 @@ describe('Release detail getters', () => {
expect(getters.releaseLinksToDelete(state)).toEqual(originalLinks);
});
});
describe('validationErrors', () => {
describe('when the form is valid', () => {
it('returns no validation errors', () => {
const state = {
release: {
assets: {
links: [
{ id: 1, url: 'https://example.com/valid', name: 'Link 1' },
{ id: 2, url: '', name: '' },
{ id: 3, url: '', name: ' ' },
{ id: 4, url: ' ', name: '' },
{ id: 5, url: ' ', name: ' ' },
],
},
},
};
const expectedErrors = {
assets: {
links: {
1: {},
2: {},
3: {},
4: {},
5: {},
},
},
};
expect(getters.validationErrors(state)).toEqual(expectedErrors);
});
});
describe('when the form is invalid', () => {
let actualErrors;
beforeEach(() => {
const state = {
release: {
assets: {
links: [
// Duplicate URLs
{ id: 1, url: 'https://example.com/duplicate', name: 'Link 1' },
{ id: 2, url: 'https://example.com/duplicate', name: 'Link 2' },
// the validation check ignores leading/trailing
// whitespace and is case-insensitive
{ id: 3, url: ' \tHTTPS://EXAMPLE.COM/DUPLICATE\n\r\n ', name: 'Link 3' },
// Invalid URL format
{ id: 4, url: 'invalid', name: 'Link 4' },
// Missing URL
{ id: 5, url: '', name: 'Link 5' },
{ id: 6, url: ' ', name: 'Link 6' },
// Missing title
{ id: 7, url: 'https://example.com/valid/1', name: '' },
{ id: 8, url: 'https://example.com/valid/2', name: ' ' },
],
},
},
};
actualErrors = getters.validationErrors(state);
});
it('returns a validation errors if links share a URL', () => {
const expectedErrors = {
assets: {
links: {
1: { isDuplicate: true },
2: { isDuplicate: true },
3: { isDuplicate: true },
},
},
};
expect(actualErrors).toMatchObject(expectedErrors);
});
it('returns a validation error if the URL is in the wrong format', () => {
const expectedErrors = {
assets: {
links: {
4: { isBadFormat: true },
},
},
};
expect(actualErrors).toMatchObject(expectedErrors);
});
it('returns a validation error if the URL missing (and the title is populated)', () => {
const expectedErrors = {
assets: {
links: {
6: { isUrlEmpty: true },
5: { isUrlEmpty: true },
},
},
};
expect(actualErrors).toMatchObject(expectedErrors);
});
it('returns a validation error if the title missing (and the URL is populated)', () => {
const expectedErrors = {
assets: {
links: {
7: { isNameEmpty: true },
8: { isNameEmpty: true },
},
},
};
expect(actualErrors).toMatchObject(expectedErrors);
});
});
});
describe('isValid', () => {
// the value of state is not actually used by this getter
const state = {};
it('returns true when the form is valid', () => {
const mockGetters = {
validationErrors: {
assets: {
links: {
1: {},
},
},
},
};
expect(getters.isValid(state, mockGetters)).toBe(true);
});
it('returns false when the form is invalid', () => {
const mockGetters = {
validationErrors: {
assets: {
links: {
1: { isNameEmpty: true },
},
},
},
};
expect(getters.isValid(state, mockGetters)).toBe(false);
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment