-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Frontend: Private Domain Onboarding Check #54186
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
Conversation
Signed-off-by: allgandalf <[email protected]>
Signed-off-by: allgandalf <[email protected]>
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@blimpich 🟢 Issue was fixed, latest testing looks good 👍 Screen.Recording.2025-04-14.at.20.57.52.mov |
@ikevin127 , this is a BE issue, for the second time the BE returns empty object for onyx data: {"jsonCode":200,"requestID":"93095480dd724483-SJC","onyxData":[]} Screen.Recording.2025-04-15.at.11.49.02.AM.movWe need to know the @blimpich the request id for the first request was : And for the second request was: I honestly think what kevin found is a edge case and can be dealt in a follow up BE PR without blocking this one. the core flow works here, what do you guys think ? |
@blimpich with the latest backend update, the previous issue was resolved, onboarding is completed successfully , the looks good to be merged ! |
Yeah I agree. The main flows work here. This is a huge PR, lets deal with other edge case issues / bugs as follow up. I think this is ready to merge 👍. @allgandalf can you fix the latest merge conflict and then I'll approve and merge? |
It was a trivially simple merge conflict to resolve so I just resolved it myself. Once this goes green lets merge! |
Ugh, linting changed as a result of merging in the latest main. Will let @allgandalf handle that. |
@blimpich extremely sorry but i cannot access laptop till tomorrow, i checked slack and perhaps this rule was newly added https://expensify.enterprise.slack.com/archives/C01GTK53T8Q/p1744735887734139, i think merging this as is wont break main as this affects changed file only, so i guess we can still merge this without breaking main, else i will take a look at it tomorrow |
All good @allgandalf! One more day won't hurt us 🙂. We could probably merge but lets just wait till you can update the code to meet the new linter requirements and get a full green build. |
hmm, since there is no clear instructions on how to resolve this till now, I went through the PR which introduced the rule and it mentioned that most of the calls would be set to true: Following the same appoarch here, since it was also done at other places: https://github.com/search?q=repo%3AExpensify%2FApp%20canBeMissing&type=code in Update: Okay made some more sense to me now, this should work! |
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.
WOOOOOOHHOOOO LETS MERGE!!!! 🎉
✋ 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 https://github.com/blimpich in version: 9.1.29-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.29-10 🚀
|
Explanation of Change
Check whether public domain and SMS sign-ups have a work email in the New Expensify onboarding flow. If they’re new accounts, we'll merge them, and if the work email is tied to an existing account, we’ll redirect them to OldDot.
Fixed Issues
$ #54075
PROPOSAL: N/A
Tests
Same as QA
Offline tests
QA Steps
Test Scenarios
1. Add Private Domain When Account Doesn’t Already Exist (Scenario A)
Login:
Onboarding Modal:
Entering Email:
Adding Work Email:
Post-Onboarding Verification:
2. Add Private Domain When Account Already Exist (Scenario B)
Initial Account Setup:
[email protected]
), validate it, then log out.Onboarding Modal:
Linking Existing Private Domain:
Post-Onboarding Verification:
3. Don’t Ask for Work Email if Domain Is Private
Account Creation:
Verification:
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2025-04-09.at.1.22.02.PM.mov
iOS: mWeb Safari
Screen.Recording.2025-04-09.at.1.23.55.PM.mov
Screen.Recording.2025-04-09.at.1.29.22.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-04-09.at.12.41.21.PM.mov
MacOS: Desktop