Commit b2173f6e authored by Nathan Friend's avatar Nathan Friend

Add validation to the "Edit Release" form

This commit adds form validation to the "Edit Release" form.
These validations mirrors the validations performed by the backend when
the form is submitted.
parent 1a30d4aa
......@@ -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);
};
......@@ -12150,6 +12150,9 @@ msgstr ""
msgid "Link title"
msgstr ""
msgid "Link title is required"
msgstr ""
msgid "Linked emails (%{email_count})"
msgstr ""
......@@ -14987,7 +14990,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"
......@@ -20685,6 +20688,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 ""
......@@ -21781,9 +21787,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