Refine input validation

- Remove usage of JS core checkValidity() in favour of custom backend compliant validation - Rewrite and refactor with removal of form tag in favour of section - Scope styles - Remove many uses of $nextTick - Refine disabled state logic - Translate account property constants Signed-off-by: Christopher Ng <chrng8@gmail.com>
iametza · Aug 24, 2021 · d738ca4 · d738ca4
1 parent 5e67677
commit d738ca4
Show file tree

Hide file tree

Showing 8 changed files with 157 additions and 82 deletions.
diff --git a/apps/settings/src/components/PersonalInfo/DisplayNameSection/DisplayName.vue b/apps/settings/src/components/PersonalInfo/DisplayNameSection/DisplayName.vue
@@ -48,6 +48,7 @@ import { showError } from '@nextcloud/dialogs'
 import debounce from 'debounce'
 
 import { savePrimaryDisplayName } from '../../../service/PersonalInfo/DisplayNameService'
+import { validateDisplayName } from '../../../utils/validate'
 
 // TODO Global avatar updating on events (e.g. updating the displayname) is currently being handled by global js, investigate using https://github.com/nextcloud/nextcloud-event-bus for global avatar updating
 
@@ -81,7 +82,7 @@ export default {
 		},
 
 		debounceDisplayNameChange: debounce(async function(displayName) {
-			if (this.$refs.displayName?.checkValidity() && this.isValid(displayName)) {
+			if (validateDisplayName(displayName)) {
 				await this.updatePrimaryDisplayName(displayName)
 			}
 		}, 500),
@@ -115,10 +116,6 @@ export default {
 			}
 		},
 
-		isValid(displayName) {
-			return displayName !== ''
-		},
-
 		onScopeChange(scope) {
 			this.$emit('update:scope', scope)
 		},
@@ -131,8 +128,18 @@ export default {
 		display: grid;
 		align-items: center;
 
-		input[type=text] {
+		input {
 			grid-area: 1 / 1;
+			height: 34px;
+			width: 100%;
+			margin: 3px 3px 3px 0;
+			padding: 7px 6px;
+			cursor: text;
+			font-family: var(--font-face);
+			border: 1px solid var(--color-border-dark);
+			border-radius: var(--border-radius);
+			background-color: var(--color-main-background);
+			color: var(--color-main-text);
 		}
 
 		.displayname__actions-container {

diff --git a/apps/settings/src/components/PersonalInfo/DisplayNameSection/DisplayNameSection.vue b/apps/settings/src/components/PersonalInfo/DisplayNameSection/DisplayNameSection.vue
@@ -20,29 +20,25 @@
 -->
 
 <template>
-	<form
-		ref="form"
-		class="section"
-		@submit.stop.prevent="() => {}">
+	<section>
 		<HeaderBar
 			:account-property="accountProperty"
 			label-for="displayname"
 			:is-editable="displayNameChangeSupported"
-			:is-valid-form="isValidForm"
+			:is-valid-section="isValidSection"
 			:handle-scope-change="savePrimaryDisplayNameScope"
 			:scope.sync="primaryDisplayName.scope" />
 
 		<template v-if="displayNameChangeSupported">
 			<DisplayName
-				:scope.sync="primaryDisplayName.scope"
 				:display-name.sync="primaryDisplayName.value"
-				@update:display-name="onUpdateDisplayName" />
+				:scope.sync="primaryDisplayName.scope" />
 		</template>
 
 		<span v-else>
 			{{ primaryDisplayName.value || t('settings', 'No full name set') }}
 		</span>
-	</form>
+	</section>
 </template>
 
 <script>
@@ -53,6 +49,7 @@ import HeaderBar from '../shared/HeaderBar'
 
 import { ACCOUNT_PROPERTY_READABLE_ENUM } from '../../../constants/AccountPropertyConstants'
 import { savePrimaryDisplayNameScope } from '../../../service/PersonalInfo/DisplayNameService'
+import { validateDisplayName } from '../../../utils/validate'
 
 const { displayNames: { primaryDisplayName } } = loadState('settings', 'personalInfoParameters', {})
 const { displayNameChangeSupported } = loadState('settings', 'accountParameters', {})
@@ -69,31 +66,24 @@ export default {
 		return {
 			accountProperty: ACCOUNT_PROPERTY_READABLE_ENUM.DISPLAYNAME,
 			displayNameChangeSupported,
-			isValidForm: true,
 			primaryDisplayName,
 			savePrimaryDisplayNameScope,
 		}
 	},
 
-	mounted() {
-		this.$nextTick(() => this.updateFormValidity())
-	},
-
-	methods: {
-		onUpdateDisplayName() {
-			this.$nextTick(() => this.updateFormValidity())
-		},
-
-		updateFormValidity() {
-			this.isValidForm = this.$refs.form?.checkValidity()
+	computed: {
+		isValidSection() {
+			return validateDisplayName(this.primaryDisplayName.value)
 		},
 	},
 }
 </script>
 
 <style lang="scss" scoped>
-	form::v-deep button {
-		&:disabled {
+	section {
+		padding: 10px 10px;
+
+		&::v-deep button:disabled {
 			cursor: default;
 		}
 	}

diff --git a/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue b/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue
@@ -59,6 +59,7 @@
 					<ActionButton
 						:aria-label="deleteEmailLabel"
 						:close-after-click="true"
+						:disabled="deleteDisabled"
 						icon="icon-delete"
 						@click.stop.prevent="deleteEmail">
 						{{ deleteEmailLabel }}
@@ -83,6 +84,7 @@ import FederationControl from '../shared/FederationControl'
 
 import { ACCOUNT_PROPERTY_READABLE_ENUM } from '../../../constants/AccountPropertyConstants'
 import { savePrimaryEmail, saveAdditionalEmail, saveAdditionalEmailScope, updateAdditionalEmail, removeAdditionalEmail } from '../../../service/PersonalInfo/EmailService'
+import { validateEmail } from '../../../utils/validate'
 
 export default {
 	name: 'Email',
@@ -126,9 +128,13 @@ export default {
 	computed: {
 		deleteDisabled() {
 			if (this.primary) {
-				return this.email === ''
+				// Disable for empty primary email as there is nothing to delete
+				// OR when initialEmail (reflects server state) and email (current input) are not the same
+				return this.email === '' || this.initialEmail !== this.email
+			} else if (this.initialEmail !== '') {
+				return this.initialEmail !== this.email
 			}
-			return this.email !== '' && !this.isValid(this.email)
+			return false
 		},
 
 		deleteEmailLabel() {
@@ -159,6 +165,7 @@ export default {
 
 	mounted() {
 		if (!this.primary && this.initialEmail === '') {
+			// $nextTick is needed here, otherwise it may not always work https://stackoverflow.com/questions/51922767/autofocus-input-on-mount-vue-ios/63485725#63485725
 			this.$nextTick(() => this.$refs.email?.focus())
 		}
 	},
@@ -170,7 +177,7 @@ export default {
 		},
 
 		debounceEmailChange: debounce(async function(email) {
-			if (this.$refs.email?.checkValidity() || email === '') {
+			if (validateEmail(email) || email === '') {
 				if (this.primary) {
 					await this.updatePrimaryEmail(email)
 				} else {
@@ -282,10 +289,6 @@ export default {
 			}
 		},
 
-		isValid(email) {
-			return /^\S+$/.test(email)
-		},
-
 		onScopeChange(scope) {
 			this.$emit('update:scope', scope)
 		},
@@ -298,8 +301,18 @@ export default {
 		display: grid;
 		align-items: center;
 
-		input[type=email] {
+		input {
 			grid-area: 1 / 1;
+			height: 34px;
+			width: 100%;
+			margin: 3px 3px 3px 0;
+			padding: 7px 6px;
+			cursor: text;
+			font-family: var(--font-face);
+			border: 1px solid var(--color-border-dark);
+			border-radius: var(--border-radius);
+			background-color: var(--color-main-background);
+			color: var(--color-main-text);
 		}
 
 		.email__actions-container {

diff --git a/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue b/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue
@@ -20,17 +20,14 @@
 -->
 
 <template>
-	<form
-		ref="form"
-		class="section"
-		@submit.stop.prevent="() => {}">
+	<section>
 		<HeaderBar
 			:account-property="accountProperty"
 			label-for="email"
 			:handle-scope-change="savePrimaryEmailScope"
 			:is-editable="displayNameChangeSupported"
 			:is-multi-value-supported="true"
-			:is-valid-form="isValidForm"
+			:is-valid-section="isValidSection"
 			:scope.sync="primaryEmail.scope"
 			@add-additional="onAddAdditionalEmail" />
 
@@ -52,7 +49,7 @@
 		<span v-else>
 			{{ primaryEmail.value || t('settings', 'No email address set') }}
 		</span>
-	</form>
+	</section>
 </template>
 
 <script>
@@ -64,6 +61,7 @@ import HeaderBar from '../shared/HeaderBar'
 
 import { ACCOUNT_PROPERTY_READABLE_ENUM, DEFAULT_ADDITIONAL_EMAIL_SCOPE } from '../../../constants/AccountPropertyConstants'
 import { savePrimaryEmail, savePrimaryEmailScope, removeAdditionalEmail } from '../../../service/PersonalInfo/EmailService'
+import { validateEmail } from '../../../utils/validate'
 
 const { emails: { additionalEmails, primaryEmail } } = loadState('settings', 'personalInfoParameters', {})
 const { displayNameChangeSupported } = loadState('settings', 'accountParameters', {})
@@ -81,13 +79,24 @@ export default {
 			accountProperty: ACCOUNT_PROPERTY_READABLE_ENUM.EMAIL,
 			additionalEmails,
 			displayNameChangeSupported,
-			isValidForm: true,
 			primaryEmail,
 			savePrimaryEmailScope,
 		}
 	},
 
 	computed: {
+		firstAdditionalEmail() {
+			if (this.additionalEmails.length) {
+				return this.additionalEmails[0].value
+			}
+			return null
+		},
+
+		isValidSection() {
+			return validateEmail(this.primaryEmail.value)
+				&& this.additionalEmails.map(({ value }) => value).every(validateEmail)
+		},
+
 		primaryEmailValue: {
 			get() {
 				return this.primaryEmail.value
@@ -96,41 +105,25 @@ export default {
 				this.primaryEmail.value = value
 			},
 		},
-
-		firstAdditionalEmail() {
-			if (this.additionalEmails.length) {
-				return this.additionalEmails[0].value
-			}
-			return null
-		},
-	},
-
-	mounted() {
-		this.$nextTick(() => this.updateFormValidity())
 	},
 
 	methods: {
 		onAddAdditionalEmail() {
-			if (this.$refs.form?.checkValidity()) {
+			if (this.isValidSection) {
 				this.additionalEmails.push({ value: '', scope: DEFAULT_ADDITIONAL_EMAIL_SCOPE })
-				this.$nextTick(() => this.updateFormValidity())
 			}
 		},
 
 		onDeleteAdditionalEmail(index) {
 			this.$delete(this.additionalEmails, index)
-			this.$nextTick(() => this.updateFormValidity())
 		},
 
 		async onUpdateEmail() {
-			this.$nextTick(() => this.updateFormValidity())
-
 			if (this.primaryEmailValue === '' && this.firstAdditionalEmail) {
 				const deletedEmail = this.firstAdditionalEmail
 				await this.deleteFirstAdditionalEmail()
 				this.primaryEmailValue = deletedEmail
 				await this.updatePrimaryEmail()
-				this.$nextTick(() => this.updateFormValidity())
 			}
 		},
 
@@ -166,17 +159,15 @@ export default {
 				this.logger.error(errorMessage, error)
 			}
 		},
-
-		updateFormValidity() {
-			this.isValidForm = this.$refs.form?.checkValidity()
-		},
 	},
 }
 </script>
 
 <style lang="scss" scoped>
-	form::v-deep button {
-		&:disabled {
+	section {
+		padding: 10px 10px;
+
+		&::v-deep button:disabled {
 			cursor: default;
 		}
 	}

diff --git a/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue b/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue
@@ -87,7 +87,7 @@ export default {
 
 	data() {
 		return {
-			accountPropertyLowerCase: this.accountProperty.toLowerCase(),
+			accountPropertyLowerCase: this.accountProperty.toLocaleLowerCase(),
 			initialScope: this.scope,
 		}
 	},

diff --git a/apps/settings/src/components/PersonalInfo/shared/HeaderBar.vue b/apps/settings/src/components/PersonalInfo/shared/HeaderBar.vue
@@ -22,7 +22,8 @@
 <template>
 	<h3>
 		<label :for="labelFor">
-			{{ t('settings', accountProperty) }}
+			<!-- Already translated as required by prop validator -->
+			{{ accountProperty }}
 		</label>
 
 		<FederationControl
@@ -35,7 +36,7 @@
 		<template v-if="isEditable && isMultiValueSupported">
 			<AddButton
 				class="add-button"
-				:disabled="!isValidForm"
+				:disabled="!isValidSection"
 				@click.stop.prevent="onAddAdditional" />
 		</template>
 	</h3>
@@ -73,7 +74,7 @@ export default {
 			type: Boolean,
 			default: false,
 		},
-		isValidForm: {
+		isValidSection: {
 			type: Boolean,
 			default: true,
 		},
@@ -106,6 +107,18 @@ export default {
 </script>
 
 <style lang="scss" scoped>
+	h3 {
+    display: inline-flex;
+    width: 100%;
+    margin: 12px 0 0 0;
+		font-size: 16px;
+    color: var(--color-text-light);
+
+		label {
+			cursor: pointer;
+		}
+	}
+
 	.federation-control {
 		margin: -12px 0 0 8px;
 	}