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

prevent submission if password is not included #5250

Conversation

jacklynhma
Copy link
Contributor

@jacklynhma jacklynhma commented Nov 19, 2024

PR is ready for review

Objective: Prevent the profile update form from submitting if the user forgets to add their password so they do not have to fill the form out again.

Old behavior:

  • User updates their email
  • User either clicks enter or clicks on "Update" button
  • Page reloads and gets the below error message
  • User needs to re-enter their email

New Behavior:

  • User updates their email
  • User either clicks enter or clicks on "Update" button
  • The page focuses on the password field and user sees this:
Screenshot 2024-11-18 at 11 10 59
  • User types in passwords and DOES NOT need to re-enter email
  • User clicks "Update" button 🥳

More context: This PR opened during the Ruby Conf Hack day. Thanks @martinemde for suggesting this fix :).

@@ -133,6 +133,19 @@ def sign_out
assert page.has_link?("@nick1", href: "https://twitter.com/nick1")
end

test "adding X(formerly Twitter) username without filling in your password" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: I need to write the test in the system folder

fullscreen_headless_chrome_driver

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.16%. Comparing base (a07d05c) to head (f79ad96).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5250      +/-   ##
==========================================
- Coverage   96.85%   94.16%   -2.70%     
==========================================
  Files         456      456              
  Lines        9529     9607      +78     
==========================================
- Hits         9229     9046     -183     
- Misses        300      561     +261     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@martinemde martinemde left a comment

Choose a reason for hiding this comment

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

Looks good once tests are passing. Thanks you for fixing this!

puts page.html


assert_content("please fill in your password")
Copy link
Member

Choose a reason for hiding this comment

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

This may not be readable by capybara because it's browser generated.

What if you click submit, comment about what's happening (browser halted submit because password is required), then filled the password and submitted again and check that the new value is saved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I had to write this as a system test rather than an integration test since the system test runs the test on actual chrome.

@jacklynhma
Copy link
Contributor Author

@martinemde Tests are passing, but looks like the build is failing is the code coverage 😅 .. but I wrote a test for the code change..

Copy link
Member

@martinemde martinemde 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! Don't worry about the project test coverage. We'll need to raise that overall across the project to get it to stop warning. It's not this commit's fault.

@martinemde martinemde merged commit 53ec71e into rubygems:master Nov 20, 2024
18 of 19 checks passed
@jacklynhma jacklynhma deleted the prevent-updating-profile-if-password-not-included branch November 20, 2024 20:24
@martinemde
Copy link
Member

Tested on staging and it works great. Should be available on production soon. Thanks again @jacklynhma and happy to work with you again at the conference.

@jacklynhma
Copy link
Contributor Author

Thanks @martinemde ! Happy to work with you again too :).

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

Successfully merging this pull request may close these issues.

2 participants