refactor(content): Remove deprecated recovery key creation flows #15934

vpomerleau · 2023-10-17T22:12:16Z

Because

We have turned on the new account recovery key creation flow to 100% and now want to remove the old unused settings and content-server (post_verify/account_recovery) flows
There are no links remaining to post_verify/account_recovery - after using up a key in the account recovery flow, navigates to /settings/account_recovery

This pull request

Remove feature flag set up for old vs new account recovery key flow
Remove all code related to old PageAddRecoveryKey in fxa-settings
Remove all code related to old post_verify/acccount_recovery in content-server
Test fixes

Issue that this pull request solves

Closes: #FXA-7419

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Verified events to post_verify/account_recovery via Looker/Big Query. Few events (<20 over 6 months) that are not within a legitimate user flow. Also, conversion of this flow to React was cancelled as documented in https://mozilla-hub.atlassian.net/browse/FXA-6510?focusedCommentId=680801

vbudhram

@vpomerleau Just a few nits otherwise, LGTM

vbudhram · 2023-10-19T18:35:19Z

packages/functional-tests/pages/login.ts

@@ -639,7 +637,7 @@ export class LoginPage extends BaseLayout {
    const metaConfig = this.page.locator(
      'meta[name="fxa-content-server/config"]'
    );
-    const config = await metaConfig.getAttribute('content');
+    const config = (await metaConfig.getAttribute('content')) || '';


I added a ConfigPage, so we might be able to remove it from here.

vbudhram · 2023-10-19T18:37:47Z

packages/functional-tests/tests/react-conversion/recoveryKey.spec.ts

-          key = await recoveryKey.getKey();
-          await recoveryKey.clickClose();
-        }
+        await settings.goto();


Do we have a follow up to remove the reset password routes flags? Looks like this still performs the check.

We do - it's https://mozilla-hub.atlassian.net/browse/FXA-8267

vbudhram · 2023-10-19T18:41:06Z

packages/fxa-content-server/app/scripts/lib/router.js

-        type: VerificationReasons.RECOVERY_KEY,
-      }
-    ),
+    // 'post_verify/account_recovery/add_recovery_key': createViewHandler(


I say we remove these

vbudhram · 2023-10-19T18:43:19Z

packages/fxa-content-server/app/scripts/views/post_verify/verified.js

-        account
-      );
-    }
+    // if (this.type === 'recovery_key') {


I think we can remove this vs comment out.

bcolsson · 2023-10-23T18:32:16Z

@vpomerleau - Does this need to be rebased? I'm trying to review which gettext strings are changing here, but I'm seeing lots of differences from this PR and main that were changes merged last week in different PRs.

Because: * We have turned on the new account recovery key creation flow to 100% and now want to remove the old unused settings and content-server (post_verify/account_recovery) flows This commit: * Remove feature flag set up for old vs new account recovery key flow * Remove all code related to old PageAddRecoveryKey in fxa-settings * Remove all code related to old post_verify/acccount_recovery in content-server * Test fixes Closes #FXA-7419

LZoog

Just a couple of functional test questions. I think it may also be worth linking to this Jira comment in your original PR comment too. But this PR is:

LZoog · 2023-10-24T19:47:41Z

packages/functional-tests/pages/login.ts

@@ -14,7 +14,7 @@ export const selectors = {
  EMAIL_PREFILLED: '#prefillEmail',
  EMAIL_HEADER: '#fxa-enter-email-header',
  ERROR: '.error',
-  LINK_LOST_RECOVERY_KEY: 'a:has-text("Don’t have an account recovery key?")',
+  LINK_LOST_RECOVERY_KEY: '.lost-recovery-key',


Looks like this selector only works with Backbone - why was this changed?

The previous selector didn't actually work, because the string in use for backbone has a straight quote (Don't) vs (Don’t) in the selector. I considered updating the original string, but it didn't feel worth triggering a re-translation since we're so close to stripping out the backbone flow for reset password anyway. And it seems fine for this to only work for backbone - we should be using a selector in the resetPasswordReact page (vs login) for this moving forward.

LZoog · 2023-10-24T19:49:35Z

packages/functional-tests/pages/login.ts

-      await this.page.locator(selectors.PASSWORD).fill(password);
-      await this.page.locator(selectors.VPASSWORD).fill(password);
-    }
+    await this.page.locator(selectors.PASSWORD).fill(password);


Similar question as above, don't we need if (config.showReactApp.resetPasswordRoutes === true) { here for React...?

This was the only spot where we were doing a config check in the page vs in the test. Also, I felt the react tests should use a selector from the resetPasswordReact page, not login.

LZoog · 2023-10-24T19:54:09Z

packages/functional-tests/pages/login.ts

-      'meta[name="fxa-content-server/config"]'
-    );
-    const config = await metaConfig.getAttribute('content');
-    this.page.goBack();


We might want this.page.goBack(); in the getConfig function in config.ts. I ran into failing tests at some point without it (I think this relates to my questions above, since I think I added this when I added the if (config.showReactApp.resetPasswordRoutes === true) { check.

config.ts has page.close(), which may work just as well?

LZoog · 2023-10-24T19:57:32Z

packages/fxa-settings/src/components/Settings/index.test.tsx

@@ -211,7 +200,7 @@ describe('App component', () => {

    await navigate(HomePath + '/two_step_authentication');

-    expect(getByTestId('recovery-key-input')).toBeInTheDocument();
+    expect(getByTestId('totp-input')).toBeInTheDocument();


shakes fist at when we used to use data-testids everywhere

clouserw requested a review from a team October 17, 2023 22:19

vpomerleau changed the title ~~DRAFT refactor(content): Remove deprecated recovery key creation flows~~ refactor(content): Remove deprecated recovery key creation flows Oct 18, 2023

vpomerleau marked this pull request as ready for review October 18, 2023 20:48

vpomerleau requested a review from a team as a code owner October 18, 2023 20:48

vbudhram approved these changes Oct 19, 2023

View reviewed changes

vpomerleau force-pushed the FXA-7419 branch from 54694e8 to bc321cb Compare October 23, 2023 22:27

bcolsson approved these changes Oct 24, 2023

View reviewed changes

vpomerleau force-pushed the FXA-7419 branch from bc321cb to d6c798d Compare October 24, 2023 17:11

LZoog reviewed Oct 24, 2023

View reviewed changes

vpomerleau merged commit ba515e8 into main Oct 24, 2023
6 checks passed

vpomerleau deleted the FXA-7419 branch October 24, 2023 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(content): Remove deprecated recovery key creation flows #15934

refactor(content): Remove deprecated recovery key creation flows #15934

vpomerleau commented Oct 17, 2023 •

edited

Loading

vbudhram left a comment

vbudhram Oct 19, 2023

vbudhram Oct 19, 2023

vpomerleau Oct 23, 2023

vbudhram Oct 19, 2023

vbudhram Oct 19, 2023

bcolsson commented Oct 23, 2023

LZoog left a comment

LZoog Oct 24, 2023

vpomerleau Oct 24, 2023

LZoog Oct 24, 2023

vpomerleau Oct 24, 2023

LZoog Oct 24, 2023 •

edited

Loading

vpomerleau Oct 24, 2023

LZoog Oct 24, 2023

refactor(content): Remove deprecated recovery key creation flows #15934

refactor(content): Remove deprecated recovery key creation flows #15934

Conversation

vpomerleau commented Oct 17, 2023 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

vbudhram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcolsson commented Oct 23, 2023

LZoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpomerleau commented Oct 17, 2023 •

edited

Loading

LZoog Oct 24, 2023 •

edited

Loading