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

fix(auth, android)!: correct error messages for finalizeMultiFactorEnrollment #7567

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

mnahkies
Copy link
Contributor

Description

Use promiseRejectAuthException and add logging consistent with other methods.

Before

Error: [auth/unknown] The verification code from SMS/TOTP is invalid. Please check and enter the correct verification code again.
NativeFirebaseError: [auth/unknown] The verification code from SMS/TOTP is invalid. Please check and enter the correct verification code again.

After

Error: [auth/invalid-verification-code] The verification code from SMS/TOTP is invalid. Please check and enter the correct verification code again.
NativeFirebaseError: [auth/invalid-verification-code] The verification code from SMS/TOTP is invalid. Please check and enter the correct verification code again.

Arguably a breaking change since the errror codes thrown change.

Release Summary

Errors thrown by finalizeMultiFactorEnrollment on Android are no longer of code auth/unknown

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Tested in Android emulator


🔥

Copy link

vercel bot commented Jan 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 26, 2024 0:08am
react-native-firebase-next 🛑 Canceled (Inspect) Feb 26, 2024 0:08am

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Feb 11, 2024
@mikehardy
Copy link
Collaborator

I have a breaking change release pending right now, you marked this as a breaking change which is one of the reasons it stuck in the queue (thank you for your patience!)
I'll review this before releasing the current pending break so it goes together as v19.0.0 here

@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. and removed Stale labels Feb 25, 2024
…rollment

BREAKING CHANGE: multifactor error messages were auth/unknown before on android
Now they will correctly come through as auth/invalid-verification-code
If you were relying on the previous auth/unknown codes you
will need to update your error handling code
@mikehardy mikehardy force-pushed the mn/fix/error-handling branch from 95dd587 to f715192 Compare February 26, 2024 00:03
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks great, thank you, and thanks again for your patience.

I've rebased the commit to the current main and I reworded the commit message to add a prescriptive "BREAKING CHANGE" block for developers - this is consumed by our automated release process and goes into the CHANGELOG file for people

No code changes were made on your patch

I pushed those changes and once CI does it's thing I can merge this. Hoping for release almost immediately

@mikehardy mikehardy changed the title fix!: improve error handling for finalizeMultiFactorEnrollment fix(auth, android)!: correct error messages for finalizeMultiFactorEnrollment Feb 26, 2024
@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. labels Feb 26, 2024
@mikehardy
Copy link
Collaborator

Once this is merged, working through the rest of the MFA / TOTP stuff is the priority. I believe I have mentioned it elsewhere but this is more difficult than it seems, as the auth emulator we carefully switched to has no TOTP support. So we need to re-add support in the e2e harness for direct use of cloud auth vs the auth emulator

@mikehardy
Copy link
Collaborator

android e2e finally made it past the auth part of the suite, and it passes locally. Ready to go!

@mikehardy mikehardy merged commit b0be508 into invertase:main Feb 26, 2024
16 of 18 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Feb 26, 2024
@mnahkies mnahkies deleted the mn/fix/error-handling branch February 26, 2024 21:20
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