feat(pair/CAD): Implement signed-in '/connect_another_device' and '/pair' mods + redirects #17660
Conversation
| @@ -148,7 +148,7 @@ test.describe('severity-1 #smoke', () => { | |||
| const code = await target.emailClient.getVerifyShortCode(email); | |||
| await confirmSignupCode.fillOutCodeForm(code); | |||
|
|
|||
| await expect(page).toHaveURL(/connect_another_device/); | |||
| await expect(page).toHaveURL(/pair/); | |||
There was a problem hiding this comment.
I wouldn't be surprised if some of the mobile-based tests fail since /pair will take mobile users to /pair/unsupported, if you're seeing that I'd just check for /pair/unsupported. Though, does pair/unsupported pass on this test since the URL contains /pair? 😅
There was a problem hiding this comment.
I don't think we have functional tests for the pair flow so this is probably ok. I did see a bunch of unit test failures though.
| !this.getUserAgent().isMobile() | ||
| ) { | ||
| this.navigate('/pair'); | ||
| } |
There was a problem hiding this comment.
I think there is one other case we want to account for here, and it's accessing "Connect another device" in the desktop browser menu after the user is signed in. The query params are connect_another_device?context=fx_desktop_v3&entrypoint=fxa_avatar_menu&service=sync... Maybe we just need something like this (we probably want a new constant since I don't see this one)?
if (this._isSignedIn() && !this.getUserAgent().isMobile() && (window.location.search === '' || this.getSearchParam('entrypoint') === 'fxa_avatar_menu'))
There was a problem hiding this comment.
@vbudhram Looking at Figma again, I actually think all we need to check here is if (this._isSignedIn() && !this.getUserAgent().isMobile()) and then redirect, and not bother with the parameters. We always want to take signed on desktop users here.
| if (needsMobileConfirmed) { | ||
| GleanMetrics.cadFirefox.view(); | ||
| } else { | ||
| GleanMetrics.cadFirefox.choiceView(); | ||
| } | ||
|
|
||
| context.set({ |
There was a problem hiding this comment.
We need to check for showSuccessMessage=true here like CAD and display the message now that users are being redirected here (this is from Figma):
On the bright side I tested with 2FA and I'm signed into Sync when redirected here as you said \o/
There was a problem hiding this comment.
I added the header but was spending way to much time trying to get the checkmark styled correctly, will file a follow up.
There was a problem hiding this comment.
@vbudhram Sounds good, I think in that same ticket we should change this to not be a div instead of an h1 especially since there's an h1/h2 right after this.
| sinon.stub(view, 'getSearchParam').callsFake(() => undefined); | ||
| assert.equal(view.showDownloadFirefoxQrCode(), false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
So much code removal ❤️
vbudhram
force-pushed
the
FXA-10138
branch
3 times, most recently
from
38bb333
to
56738ab
Compare
|
@LZoog I'm going to approve this since its built on top of your initial PR. If all looks good, I can merge. |
| <p class="mt-4 text-base">Open Firefox on your computer, visit <b class="whitespace-nowrap">firefox.com/pair</b>, and | ||
| follow the on-screen instructions to connect your mobile device.</p> |
There was a problem hiding this comment.
Does this need a translation tag?
There was a problem hiding this comment.
Ah yes it does, I'll update.
There was a problem hiding this comment.
@vbudhram This is looking great, there's two things that would be great to get in here to fully satisfy the ticket:
- "Do it later" needs to go to
/pairwithout success messaging:fxa/packages/fxa-settings/src/pages/InlineRecoveryKeySetup/index.tsx
Lines 38 to 39 in 8e7c494
- Desktop signed in users reaching CAD should be redirected to
/pair: https://github.com/mozilla/fxa/pull/17660/files#r1775160447
but consider this r+ because it's late and if we can land what we have here already in the tag I think it'll be a win and then we can prioritize these last fixes early next sprint.
| if (needsMobileConfirmed) { | ||
| GleanMetrics.cadFirefox.view(); | ||
| } else { | ||
| GleanMetrics.cadFirefox.choiceView(); | ||
| } | ||
|
|
||
| context.set({ |
There was a problem hiding this comment.
@vbudhram Sounds good, I think in that same ticket we should change this to not be a div instead of an h1 especially since there's an h1/h2 right after this.
…air' mods + redirects
This PR is for FXA-10138, but someone else will need to pick it up since I will be in all-day workshops Mon-Wed next week. Please feel free to modify this PR description with the commit message and push over my changes.
This currently covers the first two bullet points in that ticket. See this note about the iPad comments here.
You'll need to run
yarn firefoxof course to look at these locally. You can modify the user agent in dev tools to see different views.TODO:
pair/unsupportedinto a partial, and conditionally display itif this.getUserAgent().isMobile() && this._isSignedIn()getSyncNavigatefunction when we send a message or not on the React side.connect_another_deviceor the CAD header a lot in our functional tests after sync signin/signup.