From c014b9385448a67143ef401d5461681caa49633c Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Thu, 14 May 2020 12:15:33 +0530 Subject: [PATCH] UX: don't disable "create account" button & display error message for required fields. (#9643) --- .../discourse/app/components/user-field.js | 10 +++ .../app/controllers/create-account.js | 79 ++++++++++++------- .../discourse/app/mixins/name-validation.js | 6 +- .../app/mixins/password-validation.js | 51 +++++++----- .../app/mixins/user-fields-validation.js | 11 ++- .../app/mixins/username-validation.js | 64 +++++++++------ app/assets/stylesheets/desktop/login.scss | 1 + config/locales/client.en.yml | 5 ++ .../create-account-user-fields-test.js | 33 +++----- test/javascripts/acceptance/sign-in-test.js | 14 +--- 10 files changed, 162 insertions(+), 112 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/user-field.js b/app/assets/javascripts/discourse/app/components/user-field.js index f8e2554ab02..386b869c00e 100644 --- a/app/assets/javascripts/discourse/app/components/user-field.js +++ b/app/assets/javascripts/discourse/app/components/user-field.js @@ -6,6 +6,16 @@ export default Component.extend({ classNameBindings: [":user-field", "field.field_type", "customFieldClass"], layoutName: fmt("field.field_type", "components/user-fields/%@"), + didInsertElement() { + this._super(...arguments); + + let element = this.element.querySelector( + ".user-field.dropdown .select-kit-header" + ); + element = element || this.element.querySelector("input"); + this.field.element = element; + }, + @discourseComputed noneLabel() { return "user_fields.none"; diff --git a/app/assets/javascripts/discourse/app/controllers/create-account.js b/app/assets/javascripts/discourse/app/controllers/create-account.js index 1593ff95ad3..75ec7b2a66f 100644 --- a/app/assets/javascripts/discourse/app/controllers/create-account.js +++ b/app/assets/javascripts/discourse/app/controllers/create-account.js @@ -61,27 +61,9 @@ export default Controller.extend( this._createUserFields(); }, - @discourseComputed( - "passwordRequired", - "nameValidation.failed", - "emailValidation.failed", - "usernameValidation.failed", - "passwordValidation.failed", - "userFieldsValidation.failed", - "formSubmitted", - "inviteCode" - ) + @discourseComputed("formSubmitted") submitDisabled() { if (this.formSubmitted) return true; - if (this.get("nameValidation.failed")) return true; - if (this.get("emailValidation.failed")) return true; - if (this.get("usernameValidation.failed") && this.usernameRequired) - return true; - if (this.get("passwordValidation.failed") && this.passwordRequired) - return true; - if (this.get("userFieldsValidation.failed")) return true; - - if (this.requireInviteCode && !this.inviteCode) return true; return false; }, @@ -114,18 +96,26 @@ export default Controller.extend( // Check the email address @discourseComputed("accountEmail", "rejectedEmails.[]") emailValidation(email, rejectedEmails) { + const failedAttrs = { + failed: true, + element: document.querySelector("#new-account-email") + }; + // If blank, fail without a reason if (isEmpty(email)) { - return EmberObject.create({ - failed: true - }); + return EmberObject.create( + Object.assign(failedAttrs, { + message: I18n.t("user.email.required") + }) + ); } if (rejectedEmails.includes(email)) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.email.invalid") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.email.invalid") + }) + ); } if ( @@ -149,10 +139,11 @@ export default Controller.extend( }); } - return EmberObject.create({ - failed: true, - reason: I18n.t("user.email.invalid") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.email.invalid") + }) + ); }, @discourseComputed( @@ -312,6 +303,34 @@ export default Controller.extend( }, createAccount() { + this.clearFlash(); + + const validation = [ + this.emailValidation, + this.usernameValidation, + this.nameValidation, + this.passwordValidation, + this.userFieldsValidation + ].find(v => v.failed); + + if (validation) { + if (validation.message) { + this.flash(validation.message, "error"); + } + + const element = validation.element; + if (element.tagName === "DIV") { + if (element.scrollIntoView) { + element.scrollIntoView(); + } + element.click(); + } else { + element.focus(); + } + + return; + } + if (new Date() - this._challengeDate > 1000 * this._challengeExpiry) { this.fetchConfirmationValue().then(() => this.performAccountCreation() diff --git a/app/assets/javascripts/discourse/app/mixins/name-validation.js b/app/assets/javascripts/discourse/app/mixins/name-validation.js index a36ea6a7c0e..a2e024b633c 100644 --- a/app/assets/javascripts/discourse/app/mixins/name-validation.js +++ b/app/assets/javascripts/discourse/app/mixins/name-validation.js @@ -18,7 +18,11 @@ export default Mixin.create({ @discourseComputed("accountName") nameValidation() { if (this.siteSettings.full_name_required && isEmpty(this.accountName)) { - return EmberObject.create({ failed: true }); + return EmberObject.create({ + failed: true, + message: I18n.t("user.name.required"), + element: document.querySelector("#new-account-name") + }); } return EmberObject.create({ ok: true }); diff --git a/app/assets/javascripts/discourse/app/mixins/password-validation.js b/app/assets/javascripts/discourse/app/mixins/password-validation.js index 355e9bf4b49..0348362d75f 100644 --- a/app/assets/javascripts/discourse/app/mixins/password-validation.js +++ b/app/assets/javascripts/discourse/app/mixins/password-validation.js @@ -43,44 +43,57 @@ export default Mixin.create({ accountEmail, passwordMinLength ) { + const failedAttrs = { + failed: true, + element: document.querySelector("#new-account-password") + }; + if (!passwordRequired) { return EmberObject.create({ ok: true }); } if (rejectedPasswords.includes(password)) { - return EmberObject.create({ - failed: true, - reason: - this.rejectedPasswordsMessages.get(password) || - I18n.t("user.password.common") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: + this.rejectedPasswordsMessages.get(password) || + I18n.t("user.password.common") + }) + ); } // If blank, fail without a reason if (isEmpty(password)) { - return EmberObject.create({ failed: true }); + return EmberObject.create( + Object.assign(failedAttrs, { + message: I18n.t("user.password.required") + }) + ); } // If too short if (password.length < passwordMinLength) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.password.too_short") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.password.too_short") + }) + ); } if (!isEmpty(accountUsername) && password === accountUsername) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.password.same_as_username") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.password.same_as_username") + }) + ); } if (!isEmpty(accountEmail) && password === accountEmail) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.password.same_as_email") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.password.same_as_email") + }) + ); } // Looks good! diff --git a/app/assets/javascripts/discourse/app/mixins/user-fields-validation.js b/app/assets/javascripts/discourse/app/mixins/user-fields-validation.js index 5a7ff188084..520fb8af2aa 100644 --- a/app/assets/javascripts/discourse/app/mixins/user-fields-validation.js +++ b/app/assets/javascripts/discourse/app/mixins/user-fields-validation.js @@ -27,12 +27,17 @@ export default Mixin.create({ userFields = userFields.filterBy("field.required"); } if (!isEmpty(userFields)) { - const anyEmpty = userFields.any(uf => { + const emptyUserField = userFields.find(uf => { const val = uf.get("value"); return !val || isEmpty(val); }); - if (anyEmpty) { - return EmberObject.create({ failed: true }); + if (emptyUserField) { + const userField = emptyUserField.field; + return EmberObject.create({ + failed: true, + message: I18n.t("user_fields.required", { name: userField.name }), + element: userField.element + }); } } return EmberObject.create({ ok: true }); diff --git a/app/assets/javascripts/discourse/app/mixins/username-validation.js b/app/assets/javascripts/discourse/app/mixins/username-validation.js index 081956f5c79..06776501f89 100644 --- a/app/assets/javascripts/discourse/app/mixins/username-validation.js +++ b/app/assets/javascripts/discourse/app/mixins/username-validation.js @@ -31,6 +31,10 @@ export default Mixin.create({ @discourseComputed("accountUsername") basicUsernameValidation(accountUsername) { + const failedAttrs = { + failed: true, + element: document.querySelector("#new-account-username") + }; this.set("uniqueUsernameValidation", null); if (accountUsername && accountUsername === this.prefilledUsername) { @@ -42,31 +46,38 @@ export default Mixin.create({ // If blank, fail without a reason if (isEmpty(accountUsername)) { - return EmberObject.create({ failed: true }); + return EmberObject.create( + Object.assign(failedAttrs, { + message: I18n.t("user.username.required") + }) + ); } // If too short if (accountUsername.length < this.siteSettings.min_username_length) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.username.too_short") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.username.too_short") + }) + ); } // If too long if (accountUsername.length > this.maxUsernameLength) { - return EmberObject.create({ - failed: true, - reason: I18n.t("user.username.too_long") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.username.too_long") + }) + ); } this.checkUsernameAvailability(); // Let's check it out asynchronously - return EmberObject.create({ - failed: true, - reason: I18n.t("user.username.checking") - }); + return EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.username.checking") + }) + ); }, shouldCheckUsernameAvailability() { @@ -93,23 +104,30 @@ export default Mixin.create({ }) ); } else { + const failedAttrs = { + failed: true, + element: document.querySelector("#new-account-username") + }; + if (result.suggestion) { return this.set( "uniqueUsernameValidation", - EmberObject.create({ - failed: true, - reason: I18n.t("user.username.not_available", result) - }) + EmberObject.create( + Object.assign(failedAttrs, { + reason: I18n.t("user.username.not_available", result) + }) + ) ); } else { return this.set( "uniqueUsernameValidation", - EmberObject.create({ - failed: true, - reason: result.errors - ? result.errors.join(" ") - : I18n.t("user.username.not_available_no_suggestion") - }) + EmberObject.create( + Object.assign(failedAttrs, { + reason: result.errors + ? result.errors.join(" ") + : I18n.t("user.username.not_available_no_suggestion") + }) + ) ); } } diff --git a/app/assets/stylesheets/desktop/login.scss b/app/assets/stylesheets/desktop/login.scss index 78a40485093..188f6eff1a6 100644 --- a/app/assets/stylesheets/desktop/login.scss +++ b/app/assets/stylesheets/desktop/login.scss @@ -39,6 +39,7 @@ #modal-alert { max-width: 100%; + margin-bottom: 0; padding: 8px 16px; } diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5d49c7492d9..6fab3f42ede 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -821,6 +821,7 @@ en: user_fields: none: "(select an option)" + required: 'Please enter a value for "%{name}"' user: said: "{{username}}:" @@ -1115,6 +1116,7 @@ en: sso_override_instructions: "Email can be updated from SSO provider." instructions: "Never shown to the public." ok: "We will email you to confirm" + required: "Please enter an email address" invalid: "Please enter a valid email address" authenticated: "Your email has been authenticated by {{provider}}" frequency_immediately: "We'll email you immediately if you haven't read the thing we're emailing you about." @@ -1137,6 +1139,7 @@ en: title: "Name" instructions: "your full name (optional)" instructions_required: "Your full name" + required: "Please enter a name" too_short: "Your name is too short" ok: "Your name looks good" username: @@ -1150,6 +1153,7 @@ en: too_long: "Your username is too long" checking: "Checking username availability..." prefilled: "Email matches this registered username" + required: "Please enter a username" locale: title: "Interface language" @@ -1308,6 +1312,7 @@ en: same_as_email: "Your password is the same as your email." ok: "Your password looks good." instructions: "at least %{count} characters" + required: "Please enter a password" summary: title: "Summary" diff --git a/test/javascripts/acceptance/create-account-user-fields-test.js b/test/javascripts/acceptance/create-account-user-fields-test.js index 476dad902ac..6ddd9469845 100644 --- a/test/javascripts/acceptance/create-account-user-fields-test.js +++ b/test/javascripts/acceptance/create-account-user-fields-test.js @@ -31,10 +31,10 @@ QUnit.test("create account with user fields", async assert => { assert.ok(exists(".create-account"), "it shows the create account modal"); assert.ok(exists(".user-field"), "it has at least one user field"); - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "create account is disabled at first" - ); + + await click(".modal-footer .btn-primary"); + assert.ok(exists("#modal-alert"), "it shows the required field alert"); + assert.equal(find("#modal-alert").text(), "Please enter an email address"); await fillIn("#new-account-name", "Dr. Good Tuna"); await fillIn("#new-account-password", "cool password bro"); @@ -52,28 +52,13 @@ QUnit.test("create account with user fields", async assert => { exists("#account-email-validation.good"), "the email validation is good" ); - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "create account is still disabled due to lack of user fields" - ); + + await click(".modal-footer .btn-primary"); + assert.equal(find("#modal-alert")[0].style.display, ""); await fillIn(".user-field input[type=text]:first", "Barky"); - - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "create account is disabled because field is not checked" - ); - await click(".user-field input[type=checkbox]"); - assert.ok( - !exists(".modal-footer .btn-primary:disabled"), - "create account is enabled because field is checked" - ); - - await click(".user-field input[type=checkbox]"); - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "unchecking the checkbox disables the create account button" - ); + await click(".modal-footer .btn-primary"); + assert.equal(find("#modal-alert")[0].style.display, "none"); }); diff --git a/test/javascripts/acceptance/sign-in-test.js b/test/javascripts/acceptance/sign-in-test.js index 62520e8a629..5ba70ce4a38 100644 --- a/test/javascripts/acceptance/sign-in-test.js +++ b/test/javascripts/acceptance/sign-in-test.js @@ -132,10 +132,6 @@ QUnit.test("create account", async assert => { await click("header .sign-up-button"); assert.ok(exists(".create-account"), "it shows the create account modal"); - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "create account is disabled at first" - ); await fillIn("#new-account-name", "Dr. Good Tuna"); await fillIn("#new-account-password", "cool password bro"); @@ -151,20 +147,14 @@ QUnit.test("create account", async assert => { exists("#username-validation.bad"), "the username validation is bad" ); - assert.ok( - exists(".modal-footer .btn-primary:disabled"), - "create account is still disabled" - ); + await click(".modal-footer .btn-primary"); + assert.ok(exists("#new-account-username:focus"), "username field is focused"); await fillIn("#new-account-username", "goodtuna"); assert.ok( exists("#username-validation.good"), "the username validation is good" ); - assert.not( - exists(".modal-footer .btn-primary:disabled"), - "create account is enabled" - ); await click(".modal-footer .btn-primary"); assert.ok(