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

task(recovery-phone): Create new phone number setup method #18007

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

dschom
Copy link
Contributor

@dschom dschom commented Nov 13, 2024

Because

  • We want to be able to setup a new phone number

This pull request

  • Creates a recovery phone service
  • Stubs out storeUnconfirmed in RecoveryPhoneManager
  • Adds config for recovery phone service
  • Adds tests for recovery phone service
  • Adds new error type RecoveryNumberNotSupportedError

Issue that this pull request solves

Closes: FXA-10347

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)

Waiting on blocker to land, put a stub in place for now.

@dschom dschom requested a review from a team as a code owner November 13, 2024 01:14
@dschom dschom requested a review from vbudhram November 13, 2024 01:14
setup: boolean
) {
// TODO: Provide real implementation FXA-10346
console.log('TODO: implement storeUnconfirmed', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vbudhram Did this for now. Let me know when FXA-10346 is ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dschom Just pushed #18012 if you wanted to take a look.

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@dschom The code seems reasonable. I think there might be some confusion on how/when to use the OtpManager? In #18012 I use a Redis instance to store the lookupData and isSetup flags.

setup: boolean
) {
// TODO: Provide real implementation FXA-10346
console.log('TODO: implement storeUnconfirmed', {
Copy link
Contributor

Choose a reason for hiding this comment

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

@dschom Just pushed #18012 if you wanted to take a look.

}
}

const code = await this.otpCode.create(`${uid}:${phoneNumber}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would want to use getCode?

Copy link
Contributor Author

@dschom dschom Nov 13, 2024

Choose a reason for hiding this comment

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

getCode is private. Seems like create is the public facing function.

@dschom
Copy link
Contributor Author

dschom commented Nov 13, 2024

@dschom The code seems reasonable. I think there might be some confusion on how/when to use the OtpManager? In #18012 I use a Redis instance to store the lookupData and isSetup flags.

@vbudhram I noticed this as well. It seem redundant to store the code twice; however, the getCode method is private. Perhaps it should be public, or there should be an option to store the code, or the otp manager should not be concerned with storage of the code?

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@dschom Should be good to rebase and merge latest.

…r setup method

Because:
- We want to be able to setup a new phone number

This Commit:
- Creates a recovery phone service
- Stubs out storeUnconfirmed in RecoveryPhoneManager
- Adds config for recovery phone service
- Adds tests for recovery phone service
- Adds new error type RecoveryNumberNotSupportedError
- Public exposes generateCode on OtpManager.
@dschom dschom merged commit 448265b into main Nov 14, 2024
25 checks passed
@dschom dschom deleted the FXA-10347 branch November 14, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants