-
Notifications
You must be signed in to change notification settings - Fork 0
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
Strip dashes from accountPin in porting embed for Verizon portings #20
base: main
Are you sure you want to change the base?
Strip dashes from accountPin in porting embed for Verizon portings #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks!! Just a few tiny minuscule comments 🍓 🍍 🪅
.fn() | ||
.mockImplementation(async () => porting) | ||
|
||
;(useUpdatePorting as jest.Mock).mockReturnValue(useUpdatePortingMock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray ;? 👀 something weird with the formatting perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually formatted this way by prettier/eslint. I think if we were to drop the semicolon it would be complied as currying
"@tashi-iu/react-native-paper-dates": "^0.18.5", | ||
"jest": "^29.2.1", | ||
"jest-expo": "~50.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would have preferred to make a separate chore PR for this, this way e.g. if for some reason we have to revert the upgrade we don't also lose the PIN changes and vice versa,
but I'm also ok including the upgrade here 💖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. In this case not updating will break the Expo preview (they dropped support our SDK version completely), so I figured there might be little need to go back a version.
.fn() | ||
.mockImplementation(async () => porting) | ||
|
||
;(useUpdatePorting as jest.Mock).mockReturnValue(useUpdatePortingMock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too 👀
We have recently learned that Verizon will display their users account PINs with dashes (e.g.
123-456
) for better readability, but actually requires the PIN to be sent without dashes for port outs.Naturally, this cause users to copy the PIN in the format provided by Verizon, causing failed portings.
We have adapted Connect to automatically strip dashes from the
accountPin
field in the porting form for Verizon portings and decided to backport this functionality to our other products as well.Changes
<CarrierInfoForm />
accepts a new prop (forbiddenPinChars
) that defines an array of characters that the component strips from theaccountPin
field before submitting<PortingFormContainer />
checks wether the current porting'sdonorProvider
is Verizon and configures<CarrierInfoForm />
to remove dashes from theaccountPin
if that is the case.Unrelated Changes
51
as Expo GO dropped support for SKD 49 & 50