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

Add a banner to ask user to validate their email #24172

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

crazytonyli
Copy link
Contributor

Issue on Linear: e0bcdcc1be68

Here are some screenshots.

Init Sent Error
init sent error

To test:

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@crazytonyli crazytonyli added this to the 25.9 milestone Mar 10, 2025
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 10, 2025

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr24172-0760a08
Version25.8
Bundle IDcom.jetpack.alpha
Commit0760a08
App Center Buildjetpack-installable-builds #10682
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 10, 2025

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr24172-0760a08
Version25.8
Bundle IDorg.wordpress.alpha
Commit0760a08
App Center BuildWPiOS - One-Offs #11653
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@jkmassel
Copy link
Contributor

Can we try to understand the error? If their account is already validated, we can probably just dismiss the banner?

@crazytonyli
Copy link
Contributor Author

The error in the screenshot is a very poor example. 🤦‍♂️

My intention is to have an error state to show API errors: an error message (returned by the API endpoint) and a "Try again" button for all errors.

The easiest way for me to debug this state is to update the code to send a link to an already verified account. This should almost never happen in practice, though. Because MeViewController fetched account details quite often, and the banner does not show up if the account is already verified.

Also, the API does not return a specific error code for the "email already verified" error. It's just code: invalid_request. So, it'll be a bit difficult to handle this error. Again, this error should never happen. I don't think it needs special attention.

@kean
Copy link
Contributor

kean commented Mar 10, 2025

Let's try a different color. Something similar to this could work:

Screenshot 2025-03-10 at 5 16 58 PM

@crazytonyli
Copy link
Contributor Author

@kean I thought about that, but didn't reuse the cell because

  1. The "verify email" view should be more prominent. Maybe the current red background is too much, but I'm not sure if there is some existing design in the app we can reuse.
  2. The link/button at the bottom is more like a "more info" than an action button.

@kean
Copy link
Contributor

kean commented Mar 10, 2025

Something like this would be adequate:

Screenshot 2025-03-10 at 6 04 14 PM

It's easy to spot and the icon would draw enough attention. People see "email" – the expect something about verification.

It's not a critical error, and they don't have to address it right away. It just needs to sit somewhere until you do it.

Shouldn't the default state be "Resend Verification Link"? I assume we send the initial verification link right after login.

@kean
Copy link
Contributor

kean commented Mar 10, 2025

Btw, wouldn't it make sense to show it on dashboard? If it's not on dashboard, you need to draw attention to it with a tab bar icon badge.

contentView.addSubview(hostingView)
hostingView.translatesAutoresizingMaskIntoConstraints = false

NSLayoutConstraint.activate([
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use extensions from WordPressUI:

contentView.addSubview(hostingView)
hostingView.pinEdges(to: contentView)

hostingView.translatesAutoresizingMaskIntoConstraints will not be needed if you do.

}

private struct VerifyEmailView: View {
@StateObject var viewModel: VerifyEmailViewModel = .init()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@StateObject var viewModel: VerifyEmailViewModel = .init()
@StateObject private var viewModel = VerifyEmailViewModel()


state = .sending

let accountService = AccountService(coreDataStack: ContextManager.sharedInstance())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let accountService = AccountService(coreDataStack: ContextManager.sharedInstance())
let accountService = AccountService(coreDataStack: ContextManager.shared)


// This value is not an actual "timeout" value of the verification link. It's just an arbitrary value to prevent
// users from sending links repeatedly.
private let verificationLinkTimeout: TimeInterval = 300
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit on a high end. I'd go with 60 seconds, perhaps?

I'd suggest showing the timer in the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think showing a countdown on the UI is not necessary? Showing a countdown feels like it's an action that the user needs to take action right now, but there is no real urgency in clicking the email link.

Copy link
Contributor

Choose a reason for hiding this comment

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

The app could start showing it if you tap "Resend" before it's possible. This way you'll know when you can tap it again. If that's what it already does – please ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The app could start showing it if you tap "Resend" before it's possible.

There are no real restrictions on the endpoint to limit when sending another email is possible.

BTW, I checked the website UI. It neither show a timer nor prevent users from keep resending emails. Once user refresh the webpage, they can send another one.

I pushed another commit to follow the web UI.

Start Sent
start sent
Start Sent
desktop-start

Copy link
Contributor

Choose a reason for hiding this comment

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

It neither show a timer nor prevent users from keep resending emails.

Well, if it's relatively free, it's should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll keep it the same with the web for now.

@crazytonyli
Copy link
Contributor Author

@kean I have updated the UI to be like this. Let me know what you think.

Start Sent
start sent

@nbradbury
Copy link

nbradbury commented Mar 12, 2025

@crazytonyli A couple minor thoughts on this:

Screenshot 2025-03-12 at 8 05 03 AM

  • "Verify Your Email" uses different capitalization than "Verification link sent"
  • Do we need to include the text, "Please check your inbox and click the link?" That seems like unnecessary wording to me.

@jkmassel
Copy link
Contributor

Is there a way to call a bit more attention to this without turning the whole thing red?

@kean
Copy link
Contributor

kean commented Mar 12, 2025

Is there a way to call a bit more attention to this without turning the whole thing red?

I'd suggest the following options:

  • Show it with animation in viewDidAppear the first time it appears
  • Add a badge on the "Me" tab until the user sees it once

@crazytonyli
Copy link
Contributor Author

@nbradbury I have updated the design. See #24172 (comment)

@crazytonyli
Copy link
Contributor Author

@jkmassel @kean I added the Verify Your Email view to the "no sites view" in #24211, which is the first screen after creating a new account from the app. I believe a newly created account is the only scenario where the account email is not verified.

@nbradbury
Copy link

@crazytonyli @kean When one of you has a minute, could you post a video of the validation flow? Thanks!

@crazytonyli
Copy link
Contributor Author

@nbradbury This is how it looks like at the moment (including changes in #24211)

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-03-20.at.12.32.53.mp4

@crazytonyli crazytonyli requested a review from kean March 20, 2025 09:38
@crazytonyli
Copy link
Contributor Author

@nbradbury BTW, the "Create WordPress.com site" button is no longer disabled. The app now shows an alert instead.

@nbradbury
Copy link

This is how it looks like at the moment

Thanks @crazytonyli ! I noticed that it says "you email" instead of "your email" in a couple of places.

Screenshot 2025-03-20 at 7 28 40 AM

@kean
Copy link
Contributor

kean commented Mar 20, 2025

@nbradbury This is how it looks like at the moment (including changes in #24211)

Just wanted to note that I'm pretty confident that "No Sites" should not have the "Verify Your Email" view. I expanded on it here #24211 (review). Wdyt?

@kean
Copy link
Contributor

kean commented Mar 20, 2025

I've tested verification and it worked as expected, but I have a couple of notes.

Steps

  • Create a new account (🟡 forced me to pick my username on the same screen as my email, which felt excessive)
  • Tap "Create Site" and get to the "Pick Plan" step (🔴 as expected, it failed to load asking me to login again)
  • Close the "Site Flow"
  • Open "Me"
  • See the "Verify Email" view saying that the email has been sent
  • Check my email
  • Tap "Confirm Email" (🟡 it opened the web view instead of the app and forced me to login again – annoying. I wish the app could verify me or there was an automatic redirect to the app)
  • Open the app
  • Verify that "Verify Email" view is gone 🟢
  • Verify that "Pick Plan" screen load and you can create a site 🟢

Overall, it does raise questions whether this flow is an improvement over the previous native production version which used your email to login you in and verify it at the same time. The verification step felt natural because after the verification, the web used to redirect you straight back in the app. It was a smoother flow, especially considering that you can't create a site without verification anyway, so it's critical for account creation (on mobile – why doesn't the web require it? what are we missing?).

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

The code also looks good, but noting that the are multiple areas of improvements for the overall flow.

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

Successfully merging this pull request may close these issues.

5 participants