ADD: Client-side Sign Up form validation #7086
Conversation
|
Please add comments. Thanks for working on it |
Codecov Report
@@ Coverage Diff @@
## master #7086 +/- ##
=======================================
Coverage 80.81% 80.81%
=======================================
Files 97 97
Lines 5567 5567
=======================================
Hits 4499 4499
Misses 1068 1068 |
|
Thank you @SidharthBansal. I've added the comments for most 'complicated' functions. |
|
Cool thanks. I have left some comments. Kindly read them.
Lets discuss things before implementation details in that issue
…On Thu, Jan 2, 2020 at 11:34 PM Vladimir Mikulic ***@***.***> wrote:
Thank you @SidharthBansal <https://github.com/SidharthBansal>. I've added
the comments for most 'complicated' functions.
Code is pretty declarative, so there shouldn't be any issues with the rest.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7086?email_source=notifications&email_token=AFAAEQ26HNSVWDDFIK5S3J3Q3YUDZA5CNFSM4KCEYUQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH664OI#issuecomment-570289721>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAAEQ7T56FMR3BXPJ5BPFTQ3YUDZANCNFSM4KCEYUQQ>
.
|
|
Does this pr handles both sign up form and sign up modal? OR do we need two separate prs for that? |
|
@SidharthBansal the PR handles both 😄 |
|
Can you please resolve code climate? |
|
I will request Jeff or cess to review once more as changes are huge and merge it |
|
Yeah, changes are huge 😄 |
|
Also, notices should be in request tone.
Please make sure password is atleast 8 character long with minimum one
numeric value.
Kindly change in all of them
Bonus tip: create ftos for other notices apart from this pr 🎉
…On Fri, 3 Jan 2020, 5:58 pm Vladimir Mikulic, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/assets/javascripts/validation.js
<#7086 (comment)>:
> - });
+ }
+
+ function validateEmail(e) {
+ var email = e.target.value;
+ var regexp = /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;
+ var isValid = regexp.test(email);
+
+ updateUI(emailElement, isValid, 'Invalid email');
+ }
+
+ function validatePassword(e) {
+ var password = e.target.value;
+
+ if (!isPasswordValid(password)) {
+ updateUI(this, false, 'Minimum 8 characters including 1 number');
Got it 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7086?email_source=notifications&email_token=AFAAEQ434Q5KXYWFXT3LOBLQ34VQNA5CNFSM4KCEYUQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQTGC3Y#discussion_r362793067>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAAEQ4L3F2ONB4OEXCOD2TQ34VQNANCNFSM4KCEYUQQ>
.
|
|
@SidharthBansal I implemented the changes that you've requested. |
|
Ignore codeclimate then |
app/assets/javascripts/validation.js
Outdated
|
|
||
| function validateEmail(obj) { | ||
| var email = this.value; | ||
| var regexp = /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/; |
There was a problem hiding this comment.
Try to use explicit variable names like emailRegex. I can't understand which regex are you talking about if I am new developer. I hope you understand. Do similarly at other places
There was a problem hiding this comment.
Please ensure that the regex used here and at the backend are SAME.
There was a problem hiding this comment.
I am unable to find the email regex in RoR. Maybe it's not there? Some functionality is not in this repo, but in a "database-backed something" as @jywarren described it.
There was a problem hiding this comment.
Hi! I hope to catch up on this PR today, but need to go eat some lunch first! For now i hope this helps: the regex is generally in the Authlogic Ruby gem, but we've overridden it here:
Line 10 in d251b5f
I actually am not sure why we did that. But we did. :-)
We could look back in the history to see what the justification was, but it seems out of scope for this use case. Let's just be sure people seeing that line can find their way to the JS code, and vice versa, so that if either one is ever changed, the developer knows to change the corresponding one. Thanks!
There was a problem hiding this comment.
This the username validation regex, but I need a regex that validates the email address.
It's simply not there :(
|
@SidharthBansal it is already modular. Making separate files wouldn't make sense. |
|
I think the broader issue of JS maintainability in plots2 is a really critical one. Over the past few years we've spun out quite a few critical JS systems into internally-tested independent modules/libraries, as described in https://publiclab.org/notes/warren/05-22-2019/draft-of-a-public-lab-software-roadmap-comments-welcome -- this includes:
etc etc! However, the code remaining is not well organized and, while this would be out of the scope of this PR, a bigger project to organize and properly test smaller bits of JS within the app seems very worthwhile. Just don't let it distract too much from the narrower issue at stake here. Let's make good pragmatic decisions here and just leave things a little better than they were. As to maintainability of this code, one thing i strongly recommend is to write a system test (such as in /test/system/) to demonstrate this working. That'll ensure it doesn't get broken by future coders, and will be a backstop protection if we do a bigger JS refactor in the future. If our JS were really thoroughly tested, we would not be as hesitant to tear out a lot and refactor it! Thanks, all, for your hard work! |
|
And, just to share my thinking - client-side validation is really nice, but it does affect a critical system -- login -- and so the stakes are very high. We want to be sure this is well protected with tests. And, that it remains maintainable and readable, and doesn't create undue maintenance burden in the future. Those are the metrics I keep in mind when reviewing code that affects this area of functionality! |
|
@SidharthBansal @jywarren what should be my next step? |
|
Also @jywarren this would need JS test, we can't write Ruby to test this because we manipulate the DOM and we need to access it via JS to be able to verify that form is displaying errors. |
|
Actually system tests can interact with JavaScript, CSS and the DOM! Check
out the existing ones, they're very powerful! Full-stack.
…On Fri, Jan 3, 2020 at 1:27 PM Vladimir Mikulic ***@***.***> wrote:
Also @jywarren <https://github.com/jywarren> this would need JS test, we
can't write Ruby to test this because we manipulate the DOM and we need to
access it via JS to be able to verify that form is displaying errors.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7086?email_source=notifications&email_token=AAAF6J7GZ4AM7QMUPJYJHX3Q3566BA5CNFSM4KCEYUQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBYD4Q#issuecomment-570655218>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J3TTEQDXPKUTRCUZK3Q3566BANCNFSM4KCEYUQQ>
.
|
|
@SidharthBansal @jywarren I've added the test.
|
|
OK, pending #7106 we should be able to merge this too! We're going to lower the thresholds for both project and patch and this should pass then. Thanks for your patience!! |
|
@jywarren no problem, thank you :) |
|
Thanks all for working on thid |
|
OK, with #7106 merged, I /believe/ we should be able to get this to pass by rebasing it! Can you give a try? Thank you! |
Changes: - Removed realtime_usernmae_validation.js - CSS classes for valid and invalid input element - Minor changes in _create_form.html.erb The old sign up form would only validate a username. This change provides a nice UI to the user while filling the form. Resolves #3439
Changes: - Comments for most important functions in form validation - Refactor isPasswordValid to be pure function(no UI updates)
|
@jywarren done! |
|
Awesome, fingers 🤞 |
|
Awesome!!! |
|
Wohoo 🚀 |
|
Hi @jywarren. I've tested the form and noticed a bug. It's nothing serious though, only 5 lines changed. |
|
Ah, ok! Let's get it patched quick then! Thanks! And, perhaps we can add to
the tests to watch for such a bug in the future? Awesome!
…On Sat, Jan 4, 2020, 5:46 PM Vladimir Mikulic ***@***.***> wrote:
Hi @jywarren <https://github.com/jywarren>. I've tested the form and
noticed a bug. It's nothing serious though, only 5 lines changed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7086?email_source=notifications&email_token=AAAF6J6QEBASV2D3J5GOWCTQ4EGTFA5CNFSM4KCEYUQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIDB5DA#issuecomment-570826380>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J7DGZYZ2CFWYAHNIOLQ4EGTFANCNFSM4KCEYUQQ>
.
|
|
@jywarren I'll add the test for this 👍 |
* ADD: Client-side Sign Up form validation Changes: - Removed realtime_usernmae_validation.js - CSS classes for valid and invalid input element - Minor changes in _create_form.html.erb The old sign up form would only validate a username. This change provides a nice UI to the user while filling the form. Resolves publiclab#3439 * ADD: Comments for SignUp form validation Changes: - Comments for most important functions in form validation - Refactor isPasswordValid to be pure function(no UI updates) * ADD: Support for pl.org/register form validation * ADD: Tests for signup modal form validation
* ADD: Client-side Sign Up form validation Changes: - Removed realtime_usernmae_validation.js - CSS classes for valid and invalid input element - Minor changes in _create_form.html.erb The old sign up form would only validate a username. This change provides a nice UI to the user while filling the form. Resolves publiclab#3439 * ADD: Comments for SignUp form validation Changes: - Comments for most important functions in form validation - Refactor isPasswordValid to be pure function(no UI updates) * ADD: Support for pl.org/register form validation * ADD: Tests for signup modal form validation
Changes:
The old sign up form would only validate a username.
This change provides a nice UI to the user while filling the form.
Demo
Resolves #3439 & #4268