Client side validation of Signup form #1251

krishnak9 · 2017-03-15T13:01:25Z

#861 This implements the client side validation to the signup page.

These are few screenshots:-

Submit button gets disabled when user enters an invalid entry.

The submit button gets enabled after all fields are correct.

vitoravelino · 2017-03-15T14:15:19Z

app/assets/stylesheets/components/layout.scss

+//popover css used in signup page for client side validation
+
+#signup_page .popover{
+    background-color: rgba(0,0,0,0.6);


Please use 2 spaces and spaces after the commas.

like this rgba(0, 0, 0, 0.6);

I will use the 2 spaces for Indent level :)

Yes. Also, move this to a separated file. It could be a registration.scss in the pages folder since you are overriding the default popover style on the sign up page.

One question I have: is it okay to override the popover style only on this page? /cc @mssola

@vitoravelino I don't think it's ok to override it. All popovers should look and feel in the same way throughout the app.

That's what I thought. So, these styling can be removed then.

I didn't see any form validation using popover. It's using the built-in validators. I think you can use setCustomValidity method to achieve the same look and feel.

What do you think, @mssola?

vitoravelino · 2017-03-15T14:15:27Z

app/assets/stylesheets/components/layout.scss

+}
+
+#signup_page .popover.right > .arrow:after{
+    border-right-color: rgba(0,0,0,0.6);


Changing Now..:D

vitoravelino · 2017-03-15T14:18:20Z

app/views/devise/registrations/new.html.slim

          i.fa.fa-check
          - if @admin || !@first_user_admin
            | Create account
          - else
            | Create admin
        - if @have_users
          .text-center = link_to 'I already have an account. Login.', new_user_session_path, class: 'btn btn-link'
+javascript:


Move this out of the slim file.

I suggest you to follow the same structure done on #1233. Since I was responsible for that, I can help you if you need any help.

Should I move the javascript to the Portus/app/assets/javascripts/modules/users/pages/sign-up.js ??

Not only move to that but also restructure the code itself to follow a more component based style.

I agree with @vitoravelino. Please change this to a more component based style.

vitoravelino · 2017-03-15T14:19:22Z

app/views/devise/registrations/new.html.slim

-        = f.email_field :email, class: 'form-control input-lg first', placeholder: 'Email', required: true
-        = f.password_field :password, class: 'form-control input-lg', placeholder: 'Password (8 characters min.)', required: true
-        = f.password_field :password_confirmation, class: 'form-control input-lg last', placeholder: 'Password again', required: true
+        = f.text_field :username, class: 'form-control input-lg', placeholder: 'Username', required: true, autofocus: true,rel: 'popover'


Add a space after comma.

vitoravelino · 2017-03-15T14:19:30Z

app/views/devise/registrations/new.html.slim

-        = f.password_field :password_confirmation, class: 'form-control input-lg last', placeholder: 'Password again', required: true
+        = f.text_field :username, class: 'form-control input-lg', placeholder: 'Username', required: true, autofocus: true,rel: 'popover'
+        = f.email_field :email, class: 'form-control input-lg first', placeholder: 'Email', required: true, rel: 'popover'
+        = f.password_field :password, class: 'form-control input-lg', placeholder: 'Password (8 characters min.)', required: true,rel: 'popover'


vitoravelino · 2017-03-15T14:19:34Z

app/views/devise/registrations/new.html.slim

+        = f.text_field :username, class: 'form-control input-lg', placeholder: 'Username', required: true, autofocus: true,rel: 'popover'
+        = f.email_field :email, class: 'form-control input-lg first', placeholder: 'Email', required: true, rel: 'popover'
+        = f.password_field :password, class: 'form-control input-lg', placeholder: 'Password (8 characters min.)', required: true,rel: 'popover'
+        = f.password_field :password_confirmation, class: 'form-control input-lg last', placeholder: 'Password again', required: true,rel: 'popover'


So the submit button must be disabled at the starting. I will change the popover width too.

vitoravelino · 2017-03-15T14:25:12Z

Popover width too thin.

Submit button disabled if form is empty since is invalid.

vitoravelino · 2017-03-15T15:45:55Z

Tests are very welcome too. You can check the some of the rspec feature tests. 😄

krishnak9 · 2017-03-15T18:44:24Z

I am implementing the changes.... will push them soon...! I will also try to make travis pass.

vitoravelino · 2017-03-15T18:50:11Z

Travis is already passing. That's a false negative, don't worry. It happens sometimes. 😅

krishnak9 · 2017-03-16T11:56:44Z

@vitoravelino I did something similar to password-form.js by making signup-form.js and then using it from signup.js in pages . But when i check for event listener on the user_password and user_password_confirmation and email, Its not showing any. Please help me in this. I am new to this component based style.

vitoravelino · 2017-03-16T11:59:51Z

@krishnak9, could you push the code?

mssola

Thanks for this 👏 Just address @vitoravelino's comments.

mssola · 2017-03-16T12:43:42Z

app/assets/javascripts/modules/users/components/signup-form.js

+// interactions.
+class UsersSignUpForm extends BaseComponent {
+  elements() {
+    this.$Password = this.$el.find(PASSWORD_FIELD);


Could you rename variable to lower case ? (e.g. this.$Password should be this.$password).

Yes . I will do that for sure .

mssola · 2017-03-16T12:44:26Z

app/assets/javascripts/modules/users/components/signup-form.js

+  onKeyup() {
+    const Password = this.$Password.val();
+    const PasswordConfirmation = this.$PasswordConfirmation.val();
+    const PasswordInvalid = !Password;


I don't think this is needed.

mssola · 2017-03-16T12:45:16Z

app/assets/javascripts/modules/users/pages/sign-up.js

  mount() {
-    fadeIn(this.$el);


Please don't remove this 😉

mssola · 2017-03-16T12:46:03Z

app/assets/stylesheets/pages/registration.scss

@@ -0,0 +1,12 @@
+//popover css used in signup page for client side validation


Do you really have to re-style popovers ?

Github hided my comment regarding this.

"That's what I thought. So, these styling can be removed then.

I didn't see any form validation using popover. It's using the built-in validators. I think you can use setCustomValidity method to achieve the same look and feel.

What do you think, @mssola?"

I mean its not much necessary but if we use popover for validation messages then the width of the popover must be increased. But i think i will remove this and use HTMLSelectElement.setCustomValidity() as suggested by @vitoravelino

mssola · 2017-03-16T12:48:06Z

Now you can remove all that jQuery code right ?

vitoravelino · 2017-03-16T12:50:11Z

app/assets/javascripts/modules/users/components/signup-form.js

+  }
+
+  events() {
+    this.$el.on('keyup', PASSWORD_FIELD, e => this.onKeyup(e));


I think the reason nothing is happening on the focus out is because is you listening to the wrong event. You should use blur.

Also rename the method event listener.

Even on the 'keyup' its not listening.

vitoravelino · 2017-03-16T12:51:25Z

app/assets/javascripts/modules/users/components/signup-form.js

+  }
+
+  onKeyup() {
+    const Password = this.$Password.val();


Lowercase for variables.

I am on it :) .!

krishnak9 · 2017-03-16T12:51:39Z

Yeah I will remove all that jquery codes on the slim file, but i still ididnt get this component based style structure to work. I am still doing it. I am new to this so might take some time to learn all.

vitoravelino · 2017-03-16T16:29:47Z

@krishnak9 Let me try to explain. There's a BaseComponent class that is a simple empty js class that tries to avoid some boilerplate of having to call common methods that repeats in every component. There's an order and purpose for those methods.

When you extend BaseComponent, the component inherits its constructor that expects a jQuery wrapped dom element. This element works like the scope of the component. Everything you need to access on DOM should be in its dom tree. We know that we could just use $(selector) and access anything but it's not recommended due lack of control and scope of the component.

Components should do as simple things as possible, if you have many responsibilities in a component, you might need to split into separated components or delegate to a service.

Regarding migrating your initial code to this structure, you can see that you repeat yourself a lot wrapping the same dom elements, right? I guess you've already figured that out and started putting all the elements that you are gonna need to access inside the elements() method.

Second, and really important, thing: events. When you need to listen to any event from any dom element (ideally below this.$el, there will be cases that you are gonna need to listen from another component or service), this is where you are gonna define the event listeners (e.g keyup, blur, focus, etc). To listen to dom elements events, we use the .on() method. I'm not sure if you are aware that

(e) => this.onKeyup(e)

is the same thing as

function (e) {
  this.onKeyUp(e);
}.bind(this)

That's just some syntax suger from ES2015.

This covers the basics of the current component based structure. With that knowledge I think you can do most of the migration. Let me know if you have any question.

krishnak9 · 2017-03-17T10:20:14Z

Finally found what was going wrong. I didn't reload my vagrant environment and thats why the changes didn't take place 😅. I also removed the popover and implemented setCustomValidity() method. Please review @vitoravelino @mssola .

vitoravelino · 2017-03-17T10:27:36Z

app/assets/javascripts/modules/users/components/signup-form.js

+    const passwordInvalid = !this.$password.val() || this.$password.val().length < 8;
+    const passwordConfirmationInvalid = !this.$passwordConfirmation.val() ||
+      this.$password.val() !== this.$passwordConfirmation.val();
+    if (passwordConfirmationInvalid) {


Add an empty line above.

before the if statement ?

Yes. Makes code easier to read.

vitoravelino · 2017-03-17T10:27:41Z

app/assets/javascripts/modules/users/components/signup-form.js

+    } else {
+      this.$passwordConfirmation[0].setCustomValidity('');
+    }
+    if (passwordInvalid) {


vitoravelino · 2017-03-17T10:27:47Z

app/assets/javascripts/modules/users/components/signup-form.js

+      this.$password[0].setCustomValidity('');
+      this.$passwordConfirmation.attr('pattern', this.$password.val());
+    }
+    if (passwordInvalid || passwordConfirmationInvalid || emailInvalid || usernameInvalid) {


vitoravelino · 2017-03-17T10:29:26Z

app/assets/javascripts/modules/users/components/signup-form.js

+    this.$password = this.$el.find(PASSWORD_FIELD);
+    this.$passwordConfirmation = this.$el.find(CONFIRMATION_PASSWORD_FIELD);
+    this.$username = this.$el.find(USERNAME_FIELD);
+    this.$userEmail = this.$el.find(EMAIL_FIELD);


If you are not gonna use the $element, you don't need to cache it here. So, I think you can remove both username and email elements, constants and event listeners.

Oops. My bad! Forgot the disabled button. 😅

Yeah ..they are used in emailInvalid , usernameInvalid and for disabling the submit button. So no need to remove them right ?

No need. I just woke up. Wasn't thinking really straight yet. KKKK.

vitoravelino · 2017-03-17T10:35:31Z

app/assets/javascripts/modules/users/components/signup-form.js

+  }
+
+  onfocusout() {
+    const usernameInvalid = !this.$username;


I think you should do the same as you did for email, no?

Yeah ! I will do that . In that way if we use any pattern based user-name ,it will also get validated.

vitoravelino · 2017-03-17T10:39:10Z

app/assets/javascripts/modules/users/components/signup-form.js

+    this.$el.on('focusout', EMAIL_FIELD, e => this.onfocusout(e));
+  }
+
+  onfocusout() {


I would suggest to rename it to onFocusout. The idea (at least that's what I do) is to start a function/method with on only if it's an event listener/callback. That will highlight that this is probably a a function that is used as an event listener/callback related to the focusout event.

I know that in this case it wouldn't add much value because this component is very simple, but it's good to have consistency around.

Yeah..! Sure 😀.!

krishnak9 · 2017-03-17T11:09:50Z

Did the changes! , Don't know why travis is failing here. In my fork it did passed Travis (commit: changes)

vitoravelino · 2017-03-17T11:12:43Z

It's passing, don't worry. As I said before, it's a false negative.

krishnak9 · 2017-03-17T11:14:30Z

Oh Cool 😀 . Any more changes !

vitoravelino · 2017-03-17T11:19:42Z

app/assets/javascripts/modules/users/components/signup-form.js

+      this.$password.val() !== this.$passwordConfirmation.val();
+
+    if (passwordConfirmationInvalid) {
+      this.$passwordConfirmation[0].setCustomValidity('Please enter same Password as Above');


To lowercase Password and Above.

mssola · 2017-03-17T15:30:37Z

And yeah, please add a feature test 👍

krishnak9 · 2017-03-18T09:05:39Z

To write feature tests for this, I have to execute the javascript using capybara-webkit driver, therefore I need to include this gem too. As I have to trigger the focusout event and check if the button is disabled or not if fields are wrong or Is there any other way to do this ? @vitoravelino @mssola

mssola · 2017-03-18T10:18:13Z

To write feature tests for this, I have to execute the javascript using capybara-webkit driver, therefore I need to include this gem too. As I have to trigger the focusout event and check if the button is disabled or not if fields are wrong or Is there any other way to do this ? @vitoravelino @mssola

There's no need to include any new gem. Notice that whenever you want a feature test to be able to execute JS, you need to add the js: true flag. That being said, maybe you can take a look at the spec/support directory, which includes some helper methods. In that directory, I think that the wait_for_events.rb and the helpers.rb files can be of help (and don't be afraid to extend them if you think it's necessary 😉).

krishnak9 · 2017-03-23T15:50:00Z

Sorry I got busy with the hectic college schedule. For feature test I have done something like this

scenario "Submit Button gets disabled when any field is filled wrong", js: true do
fill_in "user_username", with: user.username
fill_in "user_email", with: "gibberish"
fill_in "user_password", with: "12341234"
fill_in "user_password_confirmation", with: "532"
page.execute_script "$('#user_username').trigger('focusout')"
expect(page).to have_button('submit_btn', disabled: true)

end
but there was an error saying

Capybara::ElementNotFound:
Unable to find field "user_username"

I can't find out what caused this error. @vitoravelino @mssola

mssola · 2017-03-23T16:04:04Z

@krishnak9 iirc, that only worked (as in the other tests of this same file) when js: true was not specified. When JS is needed, you have to take another approach (I still don't fully know why 😅). Check other tests with js: true on how this can be accomplished. If I recall correctly, it could work with something like this:

fill_in "Username", with: user.username

or with something like this (I don't recall if this was the exact way of doing it though):

find("#user_username").fill user.username

You can take a look at the spec/features/namespaces_spec.rb file to check some tests using js: true. Moreover, I'd recommend adding a wait method before this: expect(page).to have_button('submit_btn', disabled: true). Otherwise this might become a flaky test I fear 😃

krishnak9 · 2017-03-23T16:11:40Z

Oh okay 😃 !. I will try to find out the approach for this . Thanks..! I will check by using this find("#user_username").fill user.username . I think this may work.

krishnak9 · 2017-03-27T19:41:20Z

Finally found a way to work through 😅. It's related with the page path, I had to define the path explicitly. Added the feature test for Client side validation. @mssola @vitoravelino Please review. 😃

vitoravelino · 2017-03-27T21:13:31Z

spec/features/auth/signup_feature_spec.rb

+    page.execute_script "$('#user_username').trigger('focusout')"
+    wait_for_effect_on("#user_username")
+    expect(page).to have_button("submit_btn", disabled: true)
+


Remove empty line.

vitoravelino · 2017-03-27T21:13:35Z

spec/features/auth/signup_feature_spec.rb

+    page.execute_script "$('#user_username').trigger('focusout')"
+    wait_for_effect_on("#user_username")
+    expect(page).to have_button("submit_btn", disabled: false)
+


vitoravelino · 2017-03-27T21:14:50Z

spec/features/auth/signup_feature_spec.rb

+    fill_in "user_password", with: "12341234"
+    fill_in "user_password_confirmation", with: "532"
+    page.execute_script "$('#user_username').trigger('focusout')"
+    wait_for_effect_on("#user_username")


Did you test without this? Just curious.

No I didn't test without this. I just want to make sure that focusout get's completed before the check for disabled submit button.

I did it as @mssola mentioned about test might become flaky 😅

You can take a look at the spec/features/namespaces_spec.rb file to check some tests using js: true. Moreover, I'd recommend adding a wait method before this: expect(page).to have_button('submit_btn', disabled: true). Otherwise this might become a flaky test I fear 😃

Maybe I'd wait for the effect on the submit button, since this is the element that will change its state.

krishnak9 · 2017-03-28T17:12:19Z

What more should be done here . Or is this concluded. Please review @mssola @vitoravelino .

mssola · 2017-03-29T15:07:02Z

@krishnak9 fix my nitpick, and then squash your commits into a single one. Then I'd say it will be good to be merged. Thanks 👏

mssola · 2017-03-30T07:45:16Z

Just one final thing, could you sign off your commit as specified in the CONTRIBUTING.md file ? If you have your author & email in your git config, it should be a matter of just using the -s flag in the commit command.

Thanks!

Signed-off-by: Krishna Kumar <[email protected]>

mssola

A couple of considerations before merging (sorry for the trouble...):

The submit button should be disabled when loading the page (this is as simple as adding the disabled="true" element in the HTML code.
This should be done on key press maybe instead of on focus out (maybe with some timer so we don't hit this code a gazillion times). This is because on a usual workflow, when I type the password confirmation in the end, I should see have the button enabled again without me focusing out the element.

What do you think ?

krishnak9 · 2017-03-30T11:10:08Z

Oh okay. I will disable the button at the starting. I see the problem here. Is it okay that if I add a new keyup method which is called only on the confirmation password field and keeping others with focusout only . With this as soon the password matches, the submit button gets enabled. And having a variable set true if all the credentials before the password matching are correct . By checking this variable and the password matching on each keyup inside the keyup method. I think this will solve the problem and also it will call the keyup method only "length of the password number of times". What are your views on this @mssola

mssola · 2017-03-30T11:30:03Z

I will disable the button at the starting

You can even write a very simple feature test that checks that when visiting the signup page this element is disabled.

Is it okay that if I add a new keyup method which is called only on the confirmation password field and keeping others with focusout only . With this as soon the password matches, the submit button gets enabled. And having a variable set true if all the credentials before the password matching are correct . By checking this variable and the password matching on each keyup inside the keyup method. I think this will solve the problem and also it will call the keyup method only "length of the password number of times". What are your views on this

It has to be applied always. I just said password confirmation as an example. Another use case:

I type the email wrong but I continue anyways.
Type the password and password confirmation correctly.
Go back to fix my email

In this case, the button won't be enabled when I've just corrected. So, to be fair, this should be done for each input element.

vitoravelino · 2017-03-30T11:45:58Z

In order to make it more readable, I guess you could have individuals event listeners for each field and store the validation result in a variable. This way you are more flexible to manage the experience of giving a specific feedback back to the user based on the field. In the end of each of these listeners you would need to a common method that will check if all the validations are valid and if yes, enable the button, otherwise disable it.

Since the project is not using a lib/fw with some feature/sugar syntax and BaseComponent is still super simple, this will be a bit verbose and feel not elegant but, remember, it's plain js.

krishnak9 · 2017-03-30T13:41:50Z

Yeah this is true. This should be done on all the input elements .I will do the changes. I will try to do with individual event listeners or the timer based approach, I will check for it. Will push the changes soon.

krishnak9 · 2017-04-11T09:14:04Z

I have implemented this using keyup, having different event handlers for each field. But when writing test, I have to enable js:true in each test where submit button is clicked as the submit button is disabled at start . There is this error showing on tests . I have attached the screenshot
@mssola @vitoravelino . What caused this ??

vitoravelino · 2017-05-21T17:20:13Z

@krishnak9 sorry the delay, any luck so far?

mssola · 2017-07-18T09:46:54Z

@krishnak9 are you still working on this ?

krishnak9 · 2017-07-19T15:28:10Z

@mssola @vitoravelino Sorry Sir ! I got busy with my college campus placements so I m not getting time to actually work on this. If required you can close this PR. When I will resume the work, I will send a new PR.

mssola · 2017-07-20T06:32:33Z

Let's close this then. It can be re-opened if none else has worked on this in the meantime. Thanks for working on this 👍

vitoravelino reviewed Mar 15, 2017

View reviewed changes

mssola suggested changes Mar 16, 2017

View reviewed changes

mssola reviewed Mar 16, 2017

View reviewed changes

vitoravelino reviewed Mar 16, 2017

View reviewed changes

vitoravelino reviewed Mar 17, 2017

View reviewed changes

vitoravelino reviewed Mar 27, 2017

View reviewed changes

krishnak9 force-pushed the Validation branch 3 times, most recently from 05617a2 to 5f90a00 Compare March 29, 2017 21:15

mssola approved these changes Mar 30, 2017

View reviewed changes

Client side validation

d9385a1

Signed-off-by: Krishna Kumar <[email protected]>

krishnak9 force-pushed the Validation branch from 5f90a00 to d9385a1 Compare March 30, 2017 09:25

mssola suggested changes Mar 30, 2017

View reviewed changes

mssola closed this Jul 20, 2017

		@@ -0,0 +1,12 @@
		//popover css used in signup page for client side validation

Client side validation of Signup form #1251

Client side validation of Signup form #1251

Conversation

krishnak9 commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino commented Mar 15, 2017

vitoravelino commented Mar 15, 2017

krishnak9 commented Mar 15, 2017

vitoravelino commented Mar 15, 2017

krishnak9 commented Mar 16, 2017 • edited Loading

vitoravelino commented Mar 16, 2017

mssola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssola commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnak9 commented Mar 16, 2017

vitoravelino commented Mar 16, 2017 • edited Loading

krishnak9 commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnak9 commented Mar 17, 2017

vitoravelino commented Mar 17, 2017

krishnak9 commented Mar 17, 2017

Choose a reason for hiding this comment

mssola commented Mar 17, 2017

krishnak9 commented Mar 18, 2017

mssola commented Mar 18, 2017 • edited Loading

krishnak9 commented Mar 23, 2017

mssola commented Mar 23, 2017

krishnak9 commented Mar 23, 2017

krishnak9 commented Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnak9 Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

krishnak9 Mar 27, 2017 • edited Loading

vitoravelino Mar 16, 2017 •

edited

Loading

krishnak9 commented Mar 16, 2017 •

edited

Loading

vitoravelino Mar 16, 2017 •

edited

Loading

vitoravelino commented Mar 16, 2017 •

edited

Loading

vitoravelino Mar 17, 2017 •

edited

Loading

vitoravelino Mar 17, 2017 •

edited

Loading

mssola commented Mar 18, 2017 •

edited

Loading

krishnak9 commented Mar 27, 2017 •

edited

Loading

krishnak9 Mar 27, 2017 •

edited

Loading

krishnak9 Mar 27, 2017 •

edited

Loading

mssola left a comment •

edited

Loading