-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[No QA] Fix mobile transitions for some emails #9469
Conversation
Updated to fix some of the logic and all the tests are passing. Ready for another review. |
|
||
// If the email param matches what is stored in the session then we are | ||
// definitely not logging in as a new user | ||
if (paramsEmail === sessionEmail) { |
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.
Can you confirm this handles all four cases?
- Unencoded new user (OldApp new user)
- Encoded new user (OldDot new user)
- Unencoded existing user (OldApp existing user)
- Encoded existing user (OldDot existing user)
Specifically interested in case 2? e.g. when do we return true when paramsEmail !== sessionEmail
?
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.
-
Unencoded new user (OldApp new user)
The params and session emails don't match, then the linked and session emails also don't match so we correctly returntrue
. -
Encoded new user (OldDot new user)
The params and session emails don't match so we check the linked and session emails which
also don't match so we correctly returntrue
. -
Unencoded existing user (OldApp existing user)
The params and session emails don't match, but then the linked email matches the session email so we correctly returnfalse
. -
Encoded existing user (OldDot existing user)
The params and session emails match so we correctly returnfalse
.
Code looks good to me, but I need to test it on Monday. If you've tested the flow @AndrewGable , you can merge it! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @neil-marcellini in version: 1.1.79-0 🚀
|
@neil-marcellini , Our beta version OldDot is expired. Should we try in production app? |
@kavimuru yes try with the AppStore version, the oldDot TestFlight is no longer active (there are nochanges and the build has eventuallly expired from its 90 days) |
Sorry @kavimuru, OldDot mobile will create a link to NewDot production so this will need to be QAed once this fix is in production. I don't think we do production QA for NewDot right? In that case we can skip QA and I will update the title to include [No QA]. |
🚀 Deployed to production by @roryabraham in version: 1.1.79-17 🚀
|
Details
OldDot mobile does not encode URL parameters for the transition link. We can't fix it on OldDot mobile because we don't deploy it anymore, so this PR is a work around.
When determining if the transitioning user is logged in we compare the email in the URL param to the session email, and we also compare it to the un-encoded email extracted from the link with a regex. If both of those do not match, then we know that a new user is logging in.
Fixed Issues
$ #9426
Tests
Check out the Web-Expensify branch neil-test-mobile-transition which creates the transition link without encoding the URL params like OldDot mobile.
+
in the email E.x[email protected]
Checkout Web-Expensify branch
main
+
.PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Production QA / Regression Tests
Test the OldDot mobile app flows:
A. Get started inbox task
+
in it and @gmail.com. E.x[email protected]
B. Edit a free policy
+
in the email E.x[email protected]
Screenshots
N/A