Thread beta overrides into the remaining util call sites - #100061
Thread beta overrides into the remaining util call sites#100061arekm213 wants to merge 25 commits into
Conversation
…-beta-overrides # Conflicts: # src/languages/de.ts # src/languages/es.ts # src/languages/fr.ts # src/languages/it.ts # src/languages/ja.ts # src/languages/nl.ts # src/languages/pl.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts
|
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 bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@codex review |
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75beffc515
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function BetaOverridesPage() { | ||
| const styles = useThemeStyles(); | ||
| const {translate} = useLocalize(); | ||
| const {isBetaEnabled} = usePermissions(); | ||
| const [betaOverrides] = useOnyx(ONYXKEYS.BETA_OVERRIDES); |
There was a problem hiding this comment.
Block the beta override route in production
When an authenticated production user opens /beta-overrides directly, the linking config routes here even though only the Test Tool Menu entry is guarded by !isProduction. This page has no environment check, so it exposes all test switches; because usePermissions ignores overrides in production, toggling a switch writes a hidden override but leaves the effective switch state unchanged. Gate the page or route itself so the production-only restriction cannot be bypassed by a deep link.
Useful? React with 👍 / 👎.
Review: 2 blocking test failures, both caused by this PRThe refactor itself is sound — I verified the 🔴 1. Onyx export keys are out of alphabetical order
🔴 2.
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
|
@MelvinBot review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Re-review of
|
|
@MelvinBot review |
Re-review of
|
Explanation of Change
Follow up to #99444, which added local beta overrides but left three betas resolving from the raw
betasarray, so an override never reached them. This depends on #99444 and must merge after it, since it uses thebetaOverridesparameter added toPermissions.isBetaEnabledthere.The three remaining betas are now override aware:
asapSubmitanddefaultRooms: thebetasparameter is replaced by the resolved boolean (isASAPSubmitBetaEnabled,isDefaultRoomsBetaEnabled), passed down fromusePermissionsat the hook or component boundary. This is the pattern the codebase already uses in about 400 places for nine other betas: resolve once whereusePermissionsis available, then pass the answer down. SinceusePermissionsmerges betas, configuration and overrides, nothing downstream can drift.vendorMatching:ViolationsUtilsresolves this beta itself, so it now readsBETA_OVERRIDESnext to theBETASsubscription it already had. Every caller ofgetViolationsOnyxDatais an optimistic data builder and never runs during render, which is whyOnyx.connectWithoutViewis the right tool here. The same applies toTransactionInlineEdit.Because the boolean replaces the array, the
betasplumbing that existed only to answer these checks is now dead and has been removed, which is why this deletes more lines than it adds.Two changes here go beyond "the utils see overrides", both deliberate:
betaConfigurationis now applied where it was skipped.ViolationsUtilsandTransactionInlineEditresolved their beta without the configuration, while every UI check resolves it with the configuration throughusePermissions. On an account holding theallbeta, if the backend lists that beta underexplicitOnlyorexclusion, the two disagreed, so the app could write an inactive vendor violation for a feature the user cannot see. That predates this PR, but those lines now take the overrides as well, so leaving them configuration blind would have been a strange place to stop. Only accounts on theallbeta are affected.A pre-existing swapped argument pair is fixed. On main,
AttachmentPickerWithMenuItemscallscreateNewReport(personalDetails, isASAPSubmitBetaEnabled, hasViolations, ...)while the signature is(ownerPersonalDetails, hasViolationsParam, isASAPSubmitBetaEnabled, ...). Both are booleans so TypeScript never caught it, and the ten other call sites are correct. Today this only corrupts the next step, because the optimistic report state and status come from the raw betas array further down. This PR moves that resolution to the boolean, which would have made the swap corrupt the state and status as well, so the arguments are put in the right order here rather than left for a separate fix.Two call sites keep a hardcoded
falseon purpose.InSelectorandUnreadIndicatorUpdaterpreviously passedbetas: undefinedandbetas: [], which resolved to false for everyone, so the literal keeps their behaviour identical. Passing the real value there would change what the Search filter and the unread indicator show, which is unrelated to overrides and belongs in its own PR.Fixed Issues
$ #98409
PROPOSAL:
Tests
#adminsand#announceappear in the LHN without a reload. Toggle it off and verify they disappear. Before this PR the LHN ignored the override entirely.The
vendorMatchingoverride needs a QuickBooks Online or Xero workspace with a synced vendor list to demonstrate by hand, so it is covered by unit tests instead:tests/unit/ViolationUtilsTest.tsasserts that pinning the beta on adds the inactive supplier violation when the account does not have the beta, and that pinning it off suppresses the violation when the account does have it.Offline tests
Same as the tests above. Overrides are stored locally and only affect frontend checks, so behaviour is identical offline.
QA Steps
Overrides are hidden in production, so QA is a regression pass rather than a feature test: create, submit, approve and pay expenses, open reports from the LHN, and use the
in:filter in Search. Everything should behave exactly as it does today.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
No UI changes in this PR, the beta overrides screen itself is in #99444.