Introduce blocking modal when user tries to create a workspace from restricted domains group#76094
Conversation
|
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 |
| draft: 'Borrador', | ||
| finished: 'Finalizados', | ||
| upgrade: 'Mejora', | ||
| upgradeWorkspaceWarning: 'No se puede actualizar el espacio de trabajo', |
There was a problem hiding this comment.
asked for confirmation here:
https://expensify.slack.com/archives/C01GTK53T8Q/p1764136775872799
| <ConfirmModal | ||
| isVisible={isUpgradeWarningModalOpen} | ||
| shouldShowCancelButton={false} | ||
| onConfirm={() => setIsUpgradeWarningModalOpen(false)} |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Creating a new arrow function on every render causes ConfirmModal to receive a new function reference each time, potentially triggering unnecessary re-renders even when the modal's visibility hasn't changed.
Suggested fix: Use useCallback to memoize the function:
const handleConfirmUpgradeWarning = useCallback(() => {
setIsUpgradeWarningModalOpen(false);
}, []);
// Then in the JSX:
<ConfirmModal
isVisible={isUpgradeWarningModalOpen}
shouldShowCancelButton={false}
onConfirm={handleConfirmUpgradeWarning}
prompt={translate('common.upgradeWorkspaceWarningForRestrictedPolicyCreationPrompt')}
confirmText={translate('common.buttonConfirm')}
/>|
@situchan @trjExpensify can you please confirm the expected behaviour after Currently it stays on the upgrade modal page, do you want it to navigate to the distance confirmation screen?: Screen.Recording.2025-11-26.at.11.30.51.AM.movAlso @situchan to test this you can use this snippet and paste it in console: I will upload platform recordings once expected behaviour is cleared out |
| draft: 'Draft', | ||
| finished: 'Finished', | ||
| upgrade: 'Upgrade', | ||
| upgradeWorkspaceWarning: `Can't upgrade workspace`, |
There was a problem hiding this comment.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
I don't think the auto-navigation is super intuitive, they can return with the |
|
|
@trjExpensify we automatically dismiss modal when downgrade workspace: Screen.Recording.2025-11-28.at.11.33.37.PM.mov |
|
The action was successful in that case, wasn't it? This is blocking the action. |
The action was unsuccessful. As seen in video, policy plan type is still Control |
|
I am also a fan of staying on the upgrade modal page, not auto-navigation. But just asking for the purpose of consistency. |
|
Ohhhhh, I see. I missed that. I would vote to keep them on the page in both cases of being unsuccessful. 👍 |
|
@situchan i guess you can now continue with your review |
|
@twilight2294 please add screenshots. merge main |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
| upgradeWorkspaceWarning: `Can't upgrade workspace`, | ||
| upgradeWorkspaceWarningForRestrictedPolicyCreationPrompt: 'Your company has restricted workspace creation. Please reach out to an admin for help.', |
There was a problem hiding this comment.
It's not the right fit to add these in common section. Maybe move to workspace.upgrade.commonFeatures
| if (isRestrictedPolicyCreation) { | ||
| setIsUpgradeWarningModalOpen(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This should be called before
App/src/pages/iou/request/step/IOURequestStepUpgrade.tsx
Lines 127 to 130 in fbed79d
|
Looking at the comments |
this too |
|
@twilight2294 sorry for the delay reviewing this! Looks like you need to merge main |
|
@chuckdries all yours 😄 |
|
@twilight2294 conflicts again 😭 |
|
:(, hopefully we don't have any failures now 🤞 |
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.2.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
Explanation of Change
Fixed Issues
$ #73864
PROPOSAL: #73864 (comment)
Tests
Offline tests
QA Steps
Precondition:
If you don't have precondition 1, then you can simply paste the following snippet in console, just replace the domain with the account domain you are logged in with:
Verify that you see the blocking modal:
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari