[SAML Configuration] Verify Domain flow#73016
Conversation
|
Hey, I noticed you changed Please look at the code and make sure there are no malicious changes before running the workflow. If you have the K2 extension, you can simply click: [this button] |
| return { | ||
| name: SCREENS.WORKSPACES_LIST, | ||
| path: ROUTES.WORKSPACES_LIST.route, | ||
| path: normalizePath(ROUTES.WORKSPACES_LIST.route), |
There was a problem hiding this comment.
Could you share a bit more about the reasoning for this change? 🤔
There was a problem hiding this comment.
after refreshing an RHP that is shown above the Workspaces list page and then closing it, the url is wrong. in this case it would go back to /workspaces/verify-domain/workspaces instead of /workspaces. it's because react navigation invokes history.replaceState with workspaces instead of /workspaces. so we prepend a slash here. right now this is happening for all RHPs above the workspaces tab, this change fixes it for ones that don't use backTo. consulted this with the navigation team
There was a problem hiding this comment.
Can you add a comment to the code explaining why we need to use the normalize here so its easy to understand for any developer reading it, please?
src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACES_LIST_TO_RHP.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const accountID = route.params?.accountID; | ||
| const [domain] = useOnyx(`${ONYXKEYS.COLLECTION.DOMAIN}${accountID}`, {canBeMissing: true}); | ||
| const domainName = domain ? Str.extractEmailDomain(domain.email) : ''; |
There was a problem hiding this comment.
this is reused couple times, maybe worth to wrap it in an util ? Like getDomainName(domain) ?
There was a problem hiding this comment.
not sure. it would essentially just wrap Str.extractEmailDomain and encourage fetching full domain from onyx, even when email is enough
| : undefined; | ||
|
|
||
| return ( | ||
| <OfflineWithFeedback |
There was a problem hiding this comment.
This Page file is already huge, maybe extract this to a separate file / hook ?
There was a problem hiding this comment.
agreed. I made DomainMenuItem and WorkspacesEmptyStateComponent separate components and moved them to dedicated files. domains and workspaces implementations differ now, but I guess it's okay (?). refactoring the workspaces part is a bigger task
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: The diff is too large to include in this comment (355KB), so I've created a gist for you: 📋 View the translation diff here 📋 Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
Small issue 2025-11-06.11.35.52.mov |
|
LGTM! |
mountiny
left a comment
There was a problem hiding this comment.
Thanks @ZhenjaHorbach @mhawryluk
@NikkiWines wanna do the final blessing?
| const doesDomainExist = !!domain; | ||
|
|
||
| useEffect(() => { | ||
| if (!domain?.validated) { |
|
But wait 2025-11-06.14.28.20.mov |
changed onLinkPress to close the modal. let me know if I should change the link text too, but I think it's a minor issue and we can adjust the text in the following PRs to not block this one |
NikkiWines
left a comment
There was a problem hiding this comment.
Couple NABs otherwise looks good
|
|
||
| /** Whether validation code is currently loading */ | ||
| /** Errors that occurred when validating the domain */ | ||
| domainValidationError?: OnyxCommon.Errors; |
There was a problem hiding this comment.
NAB: Should this be domainVerificationError instead? (Though i guess this would involve changing the backend too if so)
There was a problem hiding this comment.
Just for linguistic consistency, maybe we can do a follow up if we decide to change it
There was a problem hiding this comment.
we use the term "validation" internally, and "verification" for user facing parts. the ideal here imo would be "validationError", to not repeat "domain" unnecessarily, but I think it's fine. we can change it in the future though
| return; | ||
| } | ||
| Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(ROUTES.WORKSPACES_VERIFY_DOMAIN.getRoute(accountID), {forceReplace: true})); | ||
| }, [accountID, domain?.validated, doesDomainExist]); |
There was a problem hiding this comment.
NAB: you don't really need doesDomainExist
| }, [accountID, domain?.validated, doesDomainExist]); | |
| useEffect(() => { | |
| if (!domain || domain?.validated) { | |
| return; | |
| } | |
| Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(ROUTES.WORKSPACES_VERIFY_DOMAIN.getRoute(accountID), {forceReplace: true})); | |
| }, [accountID, domain]); |
There was a problem hiding this comment.
this would make the effect run on every change within domain. not a huge deal, wouldn't really break anything, but I think I prefer !doesDomainExist or domain?.accountID === undefined instead of !domain
There was a problem hiding this comment.
cool, i don't feel too strongly about it, so this is fine
| const domainName = domain ? Str.extractEmailDomain(domain.email) : ''; | ||
| const {isOffline} = useNetwork(); | ||
|
|
||
| const doesDomainExist = !!domain; |
There was a problem hiding this comment.
I first implemented it with domain?.accountID === undefined to not put the whole domain in effect dependencies, in some effects putting it there would actually break stuff, but then I thought doesDomainExist better communicates the intent
|
✋ 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/mountiny in version: 9.2.46-0 🚀
|
|
@mhawryluk @NikkiWines I think the QA team can't execute this PR. Can you check it internally? 2025-11-07.11-54-30.mp4 |
|
@mhawryluk @NikkiWines @mountiny Can this PR be tested internally? Applause testers don’t have access to the private domain required for this test. |
|
I tested it with a real private domain of mine, although it would be good for someone else to test too |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.46-3 🚀
|
| <Text style={styles.webViewStyles.baseFontStyle}> | ||
| <RenderHTML html={translate('domain.verifyDomain.beforeProceeding', {domainName})} /> | ||
| </Text> | ||
|
|
||
| <Text style={styles.webViewStyles.baseFontStyle}> | ||
| <OrderedListRow index={1}> | ||
| <RenderHTML html={translate('domain.verifyDomain.accessYourDNS', {domainName})} /> | ||
| </OrderedListRow> | ||
| </Text> |
There was a problem hiding this comment.
We need a block layout here for renderHTML to correctly calculate parent dimensions for rendering. It caused #77419


Explanation of Change
Creates a domain verification flow, accessible by clicking on a non-verified domain's row in the Workspaces tab.
Fixed Issues
$ #72599
PROPOSAL: N/A
Tests
also:
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Nagranie.z.ekranu.2025-10-31.o.18.26.40.mov
Android: mWeb Chrome
Nagranie.z.ekranu.2025-10-31.o.19.09.35.mov
Nagranie.z.ekranu.2025-10-31.o.19.18.06.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-10-31.at.18.19.07.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-10-31.at.19.11.22.mp4
MacOS: Chrome / Safari
Nagranie.z.ekranu.2025-10-31.o.18.54.44.mov
Nagranie.z.ekranu.2025-10-31.o.19.18.06.mov
MacOS: Desktop
Nagranie.z.ekranu.2025-10-31.o.19.15.53.mov
Nagranie.z.ekranu.2025-10-31.o.19.17.08.mov