Skip to content

Commit

Permalink
fix: ensure create-password.js doesn't attempt to change routes twi…
Browse files Browse the repository at this point in the history
…ce (#24874)

## **Description**

Multiple onboarding tests are flaky, failing with the error
`TimeoutError: Waiting for element to be located By(css selector,
[data-testid="recovery-phrase-reveal"])`

The problem is that the test can get to the page where that selector is
present, but then be re-routed back to the previous screen. This is due
to a race condition.

 in create-password.js  there is this code:
 ```
useEffect(() => {
    if (currentKeyring) {
      if (firstTimeFlowType === FirstTimeFlowType.import) {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_COMPLETION_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
        ///: END:ONLY_INCLUDE_IF
      } else {
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      }
    }
  }, [currentKeyring, history, firstTimeFlowType]);
  ```
and there is this also this code inside handleCreate():
```
} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }
```

What is happening is:

1. user clicks submit/confirm on the create password screen
2. await createNewAccount(password); occurs
3. before that fully resolves, at least a new keyring is created, so the
useEffect fires and
history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test
is taken to the next route and continues with the clicks on the "Secure
your wallet" and then "Write down your Secret Recovery Phrase" screen
4. then `await createNewAccount(password);` resolves, and the user/test
is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the
`history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE)` call in the
`useEffect` if `handleCreate` has already started to create a new
account

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24874?quickstart=1)

## **Related issues**

Fixes: #24608

## **Manual testing steps**

e2e tests should pass

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
danjm committed Jun 4, 2024
1 parent 0db44f1 commit eb44460
Showing 1 changed file with 10 additions and 2 deletions.
12 changes: 10 additions & 2 deletions ui/pages/onboarding-flow/create-password/create-password.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export default function CreatePassword({
const [confirmPasswordError, setConfirmPasswordError] = useState('');
const [termsChecked, setTermsChecked] = useState(false);
const [showPassword, setShowPassword] = useState(false);
const [newAccountCreationInProgress, setNewAccountCreationInProgress] =
useState(false);
const history = useHistory();
const firstTimeFlowType = useSelector(getFirstTimeFlowType);
const trackEvent = useContext(MetaMetricsContext);
Expand All @@ -91,7 +93,7 @@ export default function CreatePassword({
)}`;

useEffect(() => {
if (currentKeyring) {
if (currentKeyring && !newAccountCreationInProgress) {
if (firstTimeFlowType === FirstTimeFlowType.import) {
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
history.replace(ONBOARDING_COMPLETION_ROUTE);
Expand All @@ -110,7 +112,12 @@ export default function CreatePassword({
///: END:ONLY_INCLUDE_IF
}
}
}, [currentKeyring, history, firstTimeFlowType]);
}, [
currentKeyring,
history,
firstTimeFlowType,
newAccountCreationInProgress,
]);

const isValid = useMemo(() => {
if (!password || !confirmPassword || password !== confirmPassword) {
Expand Down Expand Up @@ -219,6 +226,7 @@ export default function CreatePassword({
// Otherwise we are in create new wallet flow
try {
if (createNewAccount) {
setNewAccountCreationInProgress(true);
await createNewAccount(password);
}
///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
Expand Down

0 comments on commit eb44460

Please sign in to comment.