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.

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

FIX: signup form validation on page reload #7768

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

shreyaa-s-zz
Copy link
Collaborator

@shreyaa-s-zz shreyaa-s-zz commented Mar 31, 2020

Fixes #7618

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • 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 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

This PR fixes the sign up form validation. Earlier if the form encountered an error like email already registered, then even after changing the email the sign up button would remain disabled unless all input fields were changed. This PR fixes the issue.

Thanks!

@shreyaa-s-zz shreyaa-s-zz requested a review from a team as a code owner March 31, 2020 19:08
@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #7768 into main will increase coverage by 0.24%.
The diff coverage is 84.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7768      +/-   ##
==========================================
+ Coverage   81.96%   82.21%   +0.24%     
==========================================
  Files          97      100       +3     
  Lines        5629     5773     +144     
==========================================
+ Hits         4614     4746     +132     
- Misses       1015     1027      +12     
Impacted Files Coverage Δ
app/controllers/application_controller.rb 92.30% <ø> (ø)
app/controllers/users_controller.rb 82.29% <ø> (ø)
app/mailers/admin_mailer.rb 90.90% <12.50%> (-9.10%) ⬇️
app/models/user.rb 86.44% <12.50%> (-4.61%) ⬇️
app/jobs/digest_spam_job.rb 33.33% <33.33%> (ø)
app/controllers/admin_controller.rb 80.00% <100.00%> (+1.94%) ⬆️
app/controllers/map_controller.rb 50.00% <100.00%> (ø)
app/controllers/spam2_controller.rb 100.00% <100.00%> (ø)
app/controllers/stats_controller.rb 73.62% <100.00%> (-0.57%) ⬇️
app/controllers/tag_controller.rb 81.40% <100.00%> (+0.90%) ⬆️
... and 12 more

@shreyaa-s-zz
Copy link
Collaborator Author

simplescreenrecorder-2020-03-23_18 49 17
These are the implemented changes.

@shreyaa-s-zz
Copy link
Collaborator Author

Please have a look at it when you have a moment. I messed up the last PR because of issues with my laptop and had to use someone else's.

@shreyaa-s-zz
Copy link
Collaborator Author

shreyaa-s-zz commented Apr 1, 2020

@jywarren @cesswairimu @emilyashley @VladimirMikulic please review it and convey the requested changes. I'm aware that I haven't used the best practice to check if email input is valid but I still have queries regarding how to access the returned result of queries. I'll fix this as soon as I get them cleared. Meanwhile, since this was an issue of high priority I've opened a PR with a possible solution.

Copy link
Member

@ananyaarun ananyaarun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Shreyaa-s , can you pls remove the spaces added by mistake on several lines in this PR ?
Would just help maintain coding standards across the community !

@shreyaa-s-zz
Copy link
Collaborator Author

@ananyaarun done! Thanks for mentioning it.

@SidharthBansal
Copy link
Member

Can we write a system test to ensure this?
Thanks for working on this!

@shreyaa-s-zz
Copy link
Collaborator Author

@SidharthBansal I don't have any prior experience with writing system tests. I would love to give it a try though. Could you brief me about it a little? Thanks!

@jywarren
Copy link
Member

jywarren commented Apr 7, 2020

Hi, there are some good examples here; basically you can interact with the page using various commands, and it "autopilots" the browser to run through tests interactively:

https://github.com/publiclab/plots2/blob/master/test/system/sessions_test.rb

There is more documentation here! https://github.com/publiclab/plots2/blob/master/SYSTEM_TESTS.md

@jywarren
Copy link
Member

jywarren commented Apr 7, 2020

Thank you so much!

@shreyaa-s-zz shreyaa-s-zz changed the title fixes signup form validation on page reload FIX: signup form validation on page reload Apr 9, 2020
@shreyaa-s-zz
Copy link
Collaborator Author

I'm learning to write system tests on Ruby. I'm thinking along the lines of writing a system test that would first stimulate a user that is already registered, like 'Jeff', check if error is thrown and if so, it would then stimulate a newcomer and check for successful registration. Is this the right way? Correct me if I am wrong. And what are your expectations with the signup test?
Thanks!

@shreyaa-s-zz shreyaa-s-zz requested review from a team as code owners April 12, 2020 09:21
@shreyaa-s-zz
Copy link
Collaborator Author

@jywarren I've written the system test and tested it locally but the build is failing, can you help me figure out why? Thanks!

* fixes signup form validation on page reload

* adds system test
@shreyaa-s-zz
Copy link
Collaborator Author

@jywarren I found the error and fixed it. Please review. Thanks!

@jywarren
Copy link
Member

Hi @Shreyaa-s this is a really well written PR. Great work. Before merging it, I want to see if we need to think through some of the issues I've written up here: #7816

Thanks, then we can make a call on merging this!

Copy link
Member

@SidharthBansal SidharthBansal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @jywarren can we merge this now?

@shreyaa-s-zz
Copy link
Collaborator Author

To address the issues discussed in #7816 , I've only changed how the individual validation functions are called rather than the function itself. Earlier the functions were called only when the respective input field witnessed a change. As for the above mentioned, not the best practice, since I didn't know how to access the result of server-side validation, I checked if the error messages at top mentioned email.

Copy link
Collaborator

@cesswairimu cesswairimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. 🎉 🎉 .thanks everyone 💯

@cesswairimu cesswairimu requested a review from ananyaarun June 25, 2020 18:40
@jywarren jywarren changed the base branch from master to main June 30, 2020 18:07
Copy link
Member

@ananyaarun ananyaarun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well 🎉

@cesswairimu
Copy link
Collaborator

restarting travis

@cesswairimu cesswairimu closed this Jul 6, 2020
@cesswairimu cesswairimu reopened this Jul 6, 2020
@jywarren
Copy link
Member

jywarren commented Oct 2, 2020

Ack, so sorry I missed this! All good here, many many thanks to everyone! 👍 🎉 🎉 🎉 🎉

@jywarren jywarren merged commit 8c9c39d into publiclab:main Oct 2, 2020
@jywarren
Copy link
Member

jywarren commented Oct 2, 2020

Would someone be able to test this out on https://stable.publiclab.org quickly once it's done building? Just to be sure as it's a critical system. Thanks!!!

@cesswairimu
Copy link
Collaborator

I just tested on stable, its working great. Thanks everyone 🎉

shubhangikori pushed a commit to shubhangikori/plots2 that referenced this pull request Oct 12, 2020
* fixes signup form validation on page reload

* adds system test
alvesitalo pushed a commit to alvesitalo/plots2 that referenced this pull request Oct 14, 2020
* fixes signup form validation on page reload

* adds system test
piyushswain pushed a commit to piyushswain/plots2 that referenced this pull request Oct 22, 2020
* fixes signup form validation on page reload

* adds system test
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* fixes signup form validation on page reload

* adds system test
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* fixes signup form validation on page reload

* adds system test
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* fixes signup form validation on page reload

* adds system test
ampwang pushed a commit to ampwang/plots2 that referenced this pull request Oct 26, 2021
* fixes signup form validation on page reload

* adds system test
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* fixes signup form validation on page reload

* adds system test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signup button gets disabled permanently after errors
5 participants