Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add password confirmation success #3984

Closed
wants to merge 4 commits into from
Closed

Add password confirmation success #3984

wants to merge 4 commits into from

Conversation

kevinzluo
Copy link
Collaborator

@kevinzluo kevinzluo commented Nov 16, 2018 •
edited
Loading

Fixes #3439 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

When the passwords do not match, the password boxes are bordered in red, and when they match they are bordered green. There is also a confirmation message at the bottom.

passwordconfirmation

Please let me know if you would like me to do it a different way.

@plotsbot
Copy link
Collaborator

plotsbot commented Nov 16, 2018 •
edited
Loading

2 Messages
📖 @kevinzluo Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@grvsachdeva
Copy link
Member

Hi @kevinzluo, as the task you have solved is not claimed by you till now, so I would advice you to close this PR for now. Thanks!

@kevinzluo
Copy link
Collaborator Author

Ok thank you for your advice.

@kevinzluo kevinzluo closed this Nov 16, 2018
@kevinzluo
Copy link
Collaborator Author

Hello.
I have added checks for username, password, email. However, I currently do not know how to check whether a username or email has already been used. I would appreciate it if someone could guide me on this part.
showsignup1

For now I will continue working on a way to check whether the spam filter has been completed.

Thank you,
Kevin Luo

@kevinzluo kevinzluo reopened this Nov 18, 2018
@grvsachdeva
Copy link
Member

You can neglect the checking of username and email as that would be checked once the submission would be done. Regarding captcha you can take some time with it.

@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Nov 21, 2018 •
edited
Loading

I have added a check for spamaway completion as so:
showspamawayvalidation

I have also attempted to add a check for the google recaptcha completion using data-callback and data-expired-callback, but I cannot know if it works as the captcha doesn't appear when run in development mode.

Please let me know if I need to make any changes.
Thank you,
Kevin Luo

@jywarren
Copy link
Member

jywarren commented Nov 21, 2018 via email

@kevinzluo
Copy link
Collaborator Author

I'll try setting it to production right now!
Also, it currently does scroll up to the errors, but maybe I could set it to scroll slower so the user can see all the errors going up?

@kevinzluo
Copy link
Collaborator Author

Leaving this here for anyone else who might want to test the recaptcha:

At first when I changed the line to development, it gave an error saying there was no site key specified. Looking around online, I saw that you need to export the RECAPTCHA_SITE_KEY variable.
I managed to find the site-key for publiclabs by looking at the html on the recaptcha at publiclabs.org/signup and exported the variable. However, it gives the error "Localhost is not in the list of supported domains for this site key."
image

It turns out you need to sign up for a recaptcha api key here: https://www.google.com/recaptcha/admin . In your supported domains add localhost or 127.0.0.1, depending on which you use. Then take the site key given and type export RECAPTCHA_SITE_KEY='[key_given]'. Now the recaptcha should work.

@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Nov 21, 2018 •
edited
Loading

And I believe the check for google recaptcha works:
showrecaptchaworking

The error at the end appears to be a result of me changing the line from production to development mode.

@jywarren
Copy link
Member

This is great, but I'd like to ask that you try pushing it to the unstable branch for final testing, since this is a critical system. You can do this with git push -f git@github.com:publiclab/plots2.git HEAD:unstable and then testing it at https://unstable.publiclab.org. Thank you!

@jywarren
Copy link
Member

I'm also a bit concerned at how long the file is getting. For the section that is just pure JavaScript, do you think you could follow the example: <%= javascript_include_tag 'signin_verification' %> to include a file from /app/assets/javascripts/signin_verification.js? That way the javascript is more isolated and the file is easier to read (not so long). Would that be OK?

@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Nov 22, 2018 •
edited
Loading

Yep. I actually tested including the javascript in a separate file myself, and I can put all the scripts I added into a file. Just to confirm, do I need to add my filename to application.js?

I will try pushing to unstable as soon as I can in the morning.

Thank you,
Kevin Luo

@kevinzluo kevinzluo force-pushed the confirmation-password branch 2 times, most recently from c2b4c9a to f9b7163 Compare November 22, 2018 16:51
@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Nov 22, 2018 •
edited
Loading

@jywarren
I have moved the scripts to an external file, but I don't believe I have permission to push to unstable.
When I try git push -f git@github.com:publiclab/plots2.git HEAD:unstable I get this error:

git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

EDIT: I looked into the Permission denied (publickey) error and setup an SSH key for my account. However, it now gives this error:

ERROR: Permission to publiclab/plots2.git denied to kevinzluo.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@kevinzluo
Copy link
Collaborator Author

@publiclab/reviewers
Could you please help me on pushing to unstable? I'm not quite sure how to do it.

@grvsachdeva
Copy link
Member

grvsachdeva commented Nov 23, 2018 •
edited
Loading

okay, i also use the same command git push -f https://github.com/publiclab/plots2.git HEAD:unstable . I am pushing your PR on unstable, test the changes and let me know if you further need assistance. Thanks!

Update: Use command which I just mentioned, looks like command given by @jywarren is not working.

@kevinzluo
Copy link
Collaborator Author

@gauravano
Thanks for your help.
When I try git push -f https://github.com/publiclab/plots2.git HEAD:unstable, I get this error:

remote: Permission to publiclab/plots2.git denied to kevinzluo.
fatal: unable to access 'https://github.com/publiclab/plots2.git/': The requested URL returned error: 403

I think it is because I do not have write permissions on the repository.

When I go to unstable.publiclab.org, it states that the build has failed. https://jenkins.laboratoriopublico.org/job/Plots-Unstable/343/console

@grvsachdeva
Copy link
Member

yes, actually i just pushed some other branches too on unstable and all are failing. @jywarren @icarito could you look why it's happening. Thanks!

@grvsachdeva
Copy link
Member

I was going through your work in this PR, it's really nice. This PR can be merged only after testing on unstable which would be done after error is resolved.

If you want I can approve this task as I think there wouldn't be much issue on unstable or production. What do you say?

@icarito
Copy link
Member

icarito commented Nov 23, 2018 via email

@grvsachdeva
Copy link
Member

grvsachdeva commented Nov 23, 2018 via email

@kevinzluo
Copy link
Collaborator Author

@gauravano

I was going through your work in this PR, it's really nice. This PR can be merged only after testing on unstable which would be done after error is resolved.

If you want I can approve this task as I think there wouldn't be much issue on unstable or production. What do you say?

That would be great! Thank you so much.

@grvsachdeva
Copy link
Member

@jywarren this task has been approved since most of the work is done on the issue and further testing requires testing on unstable branch which is currently unavailable to us. As Kevin has already spent many days on this one, I think, we can approve it without merging the PR. Thanks!

@icarito
Copy link
Member

icarito commented Nov 25, 2018 via email

@grvsachdeva
Copy link
Member

grvsachdeva commented Nov 25, 2018 via email

@kevinzluo
Copy link
Collaborator Author

@jywarren
My apologies, I just removed the "Sorry" section. I will revert that change now.

I will try fixing the password match part too.

Currently the warnings will stop the submit button from working until the errors are fixed. Would you like me to disable that?

@jywarren
Copy link
Member

jywarren commented Dec 7, 2018 via email

@kevinzluo
Copy link
Collaborator Author

Sure, that sound great! I will try implementing it now.

@kevinzluo
Copy link
Collaborator Author

I have implemented some changes that I believe will work. Pushing to unstable now.

@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Dec 8, 2018 •
edited
Loading

I have tested out the updated form, and everything seems to be working as expected 😄 .

The concerning thing is that an error is given (not by my javascript, but by the server side validation) about being unable to validate the reCaptcha even when the reCaptcha is pressed and the checkmark appears. I tested this out on stable.publiclab.org and the same error occurs, so I do not think this was an issue created by my additions.

Feel free to test it out at https://unstable.publiclab.org/signup .

One more thing -- it starts showing "passwords do not match" when you type into the first box. Maybe we should have it show that only when you start typing in the second box, as people may be confused by seeing that red X and warning so early?

I have set it so that on the first submit, the password confirmation field will not display that error until at least 1 character is in the box.
However, after the submit, it will display the error even if there are none. I did this so that people who forget to fill out the form and try to submit will be notified.

Thanks for sticking with this issue, it's a very cool one and we're just trying to be careful. Thanks a lot for your contributions!

Thank you for allowing me to design it!

@jywarren
Copy link
Member

jywarren commented Dec 8, 2018

Great, all is well except the validation error you mention:

@jywarren
Copy link
Member

jywarren commented Dec 8, 2018

image

Do you think this is because the domain name doesn't match?

@kevinzluo
Copy link
Collaborator Author

I am not sure, but it seems like a definite possibility.

@SidharthBansal
Copy link
Member

@kevinzluo there is a conflict. Please resolve it.

@SidharthBansal SidharthBansal added this to the OAuth milestone Dec 11, 2018
@kevinzluo
Copy link
Collaborator Author

kevinzluo commented Dec 12, 2018 •
edited
Loading

I have resolved the conflicts now. I feel like we should wait for #4209 before proceeding with this PR because it needs to implement these checks on the signin modal. This be somewhat of an issue because the field IDs are different. I will see what I can do once it is merged 👍

@SidharthBansal
Copy link
Member

SidharthBansal commented Dec 12, 2018 via email

@SidharthBansal
Copy link
Member

@kevinzluo can you please make your branch consistent with the current master and try out it again.
There might be some deletions and some additions in this pr too. Please send the changes whenever you get time.
Now you can continue to work on this pr.
Thanks

This was referenced Dec 16, 2018
Closed
Closed
@oorjitchowdhary oorjitchowdhary mentioned this pull request Dec 23, 2018
Closed
@kevinzluo kevinzluo mentioned this pull request Dec 26, 2018
Merged
4 tasks
@SidharthBansal
Copy link
Member

Hi Kevin, it will be appreciated if you will close this PR and open up a new PR for changes. We have moved significantly from the time when you pushed commits for this.

@SidharthBansal
Copy link
Member

Please add the new PR number in this PR after closing so that we can review it again in case of any need.

@kevinzluo kevinzluo closed this Dec 26, 2018
@ghost ghost removed the in progress label Dec 26, 2018
@kevinzluo kevinzluo mentioned this pull request Dec 26, 2018
Open
@SidharthBansal
Copy link
Member

Hi @kevinzluo I cannot find the new pr. Can you please link it to the one you created?

@kevinzluo
Copy link
Collaborator Author

I have not opened it yet. I am trying to finish #4438 👍 .

@kevinzluo kevinzluo mentioned this pull request Dec 29, 2018
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SidharthBansal SidharthBansal SidharthBansal approved these changes

Assignees

@kevinzluo kevinzluo

Labels
None yet
Projects
None yet
Milestone
Login/SignUp & OAuth
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kevinzluo @plotsbot @grvsachdeva @jywarren @icarito @SidharthBansal