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

Login Flow: Splitting Username / Password Interfaces #1614

Merged
merged 20 commits into from
Jul 16, 2024

Conversation

jleandroperez
Copy link
Contributor

@jleandroperez jleandroperez commented Jul 12, 2024

Details:

Our new Authentication UX involves a multi staged flow, where Username and Password aren't necessarily entered in the same UI.

In this PR we're updating SPAuthViewController as follows:

  • AuthenticationMode has been enhanced, to support new fields
  • AuthenticationMode can no longer be modified, which helps us drop several methods
  • Implemented AuthenticationState, which allows us to pass on the State across different screens
  • And finally, new UI elements have been added to the nib itself

Plus the Onboarding UI no longer displays a sheet when you tap Log In.

Ref. #1611

Test

  1. Fresh install Simplenote
  2. Press on Log In
  • Verify that if you enter an invalid email, you get an error
  • Verify that if you enter a valid email, the Log in with email button turns blue
  • Verify that if you press on it, you get into the Password UI
  • Verify that pressing Forgot your Password opens the PW recovery website
  • Verify that if you enter your password, and pres son Log in, you get authenticated

Test: WPCOM SSO

  1. Fresh install Simplenote
  2. Press on Log In
  3. Press on Log in with WordPress.com
  • Verify you can auth with your WPCOM account

Release

RELEASE-NOTES.txt was updated in 05869f2 with

Login UI has been overhauled

@jleandroperez jleandroperez added the enhancement Improve existing functionality. label Jul 12, 2024
@jleandroperez jleandroperez self-assigned this Jul 12, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Jul 12, 2024

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 12, 2024

You can test the changes in simplenote-ios from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr1614-fbec117-0190b7fb-594e-42ad-8052-ebc6b54f15bb on your iPhone

If you need access to App Center, please ask a maintainer to add you.

let backButton = app.navigationBars[UID.NavBar.logIn].buttons.element(boundBy: 0)
if backButton.isHittable {
backButton.tap()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We account for the fact that the Auth Flow may be multi-layered

EmailLogin.logIn(email: "", password: "")
app.assertLabelExists(withText: Text.loginEmailInvalid)
app.assertLabelExists(withText: Text.loginPasswordShort)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is no longer relevant: Email + Password never show up simultaneously

@jleandroperez jleandroperez added this to the 4.52 milestone Jul 15, 2024
@jleandroperez jleandroperez changed the title [WIP] Login Flow: Splitting Username / Password Interfaces Login Flow: Splitting Username / Password Interfaces Jul 15, 2024
@jleandroperez jleandroperez marked this pull request as ready for review July 15, 2024 13:04
Base automatically changed from lantean/1611-updates-magic-link-endpoints to trunk July 15, 2024 19:57
Comment on lines +637 to +639
if mode.isUsernameHidden {
return .success
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little odd to me. It may be nit picky, but if you are performing a username validation and the username is hidden, should we be returning success?

Comment on lines 647 to 649
func performPasswordValidation() -> AuthenticationValidator.Result {
guard !mode.isPasswordHidden else {
if mode.isPasswordHidden {
return .success
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, maybe the validator result should be able to return a not needed?

Copy link
Contributor

@charliescheer charliescheer 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, works as intended. Some naming comments below

@jleandroperez
Copy link
Contributor Author

@charliescheer I'll switch the return .success to return nil (when the field isn't visible), in one of the follow up PRs.

Changing things here will break something down the line, and it'll be waaaay easier to patch in a standalone.

Thank youuu!!

@jleandroperez jleandroperez merged commit c25422a into trunk Jul 16, 2024
11 of 17 checks passed
@jleandroperez jleandroperez deleted the lantean/1611-updates-auth-flow branch July 16, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants