Move enforcement settings to Workflows section#75117
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 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@dukenv0307 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I noticed that the subtitle of the feature doesn’t suit the page after we moved
I changed the subtitle to
Let me know your thoughts on this @dukenv0307 |
|
@nyomanjyotisa Bug:
Screen.Recording.2025-11-17.at.00.00.35.mov |
|
@nyomanjyotisa Can you please ask for ES translation confirmation in Slack? Then add it in
|
|
PR updated @dukenv0307 We now wrap When a workspace is created in offline mode, we optimistically enable New-Expensify.mp4So based on this, I think we need to disable the toggle if the policy has a pending |
|
@nyomanjyotisa I can't enable these features in offline mode. But in online mode, I can
Screen.Recording.2025-11-19.at.10.53.48.mov |
Yeah, that's the new behavior after the change. My previous comment shows the behavior before the change. Do you think the current behavior is better? |
There was a problem hiding this comment.
Wow, I'm not sure how we got this far, though this is exactly why we have product review on PRs. This changes a bunch of logic that we shouldn't be touching, and in particular, is a contradiction of several on-going initiatives in #migrate. For instance:
- All the enforcement settings in this PR are Control features. That's why they live in
Rules, because that entire feature is behind the paywall. - Any customer using prevent-self approval, auto-approve or auto-reimburse is being upgraded to Control as part of the "Collect v2" initiative.
As a result, I honestly would prefer that we just pay our the contributor and close this PR. What problem are we solving here?
An alternative would be to continue with moving these to Workflows, though if we do that then we need to add additional logic where anyone on Collect that toggles any of these settings individually will need to be prompted in situ to upgrade to Control.
|
@JmillsExpensify Thanks for weighing in. Let's discuss this in the original slack thread that started this. |
|
We decided to go with this alt solution.
Accordingly, can you please update this PR with the upgrade considerations for anyone that toggles these settings on Collect. |
Is this the expected behavior? New-Expensify.mp4When user with a Collect workspace tries to enable an enforcement setting, we can saves their chosen setting in Onyx, then navigates them to the upgrade page. After upgrading, when the users click |
|
@tgolen Thanks for the update! |
|
@dukenv0307 For MacOS-Chrome.mp4I think we need to do this because if we wrap
Aside from this, I've tested the functionality in both online and offline mode, and could not find any other issue. Could you please check and review this again? Thanks! @dukenv0307 |
|
@nyomanjyotisa Thanks for you update, but I think we shouldn't do that
it can cause an inconsistency in this page where all page is greyed out but
|
|
@dukenv0307 Got it. I updated the
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-08.at.16.21.44.movAndroid: mWeb ChromeScreen.Recording.2025-12-08.at.16.18.59.moviOS: HybridAppScreen.Recording.2025-12-08.at.16.21.16.moviOS: mWeb SafariScreen.Recording.2025-12-08.at.16.15.07.movMacOS: Chrome / SafariScreen.Recording.2025-12-08.at.16.10.19.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #74752 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
LGTM |
| preventSelfApproval: { | ||
| id: 'preventSelfApproval' as const, | ||
| alias: 'prevent-self-approval', | ||
| name: 'Advanced Approvals' as const, |
There was a problem hiding this comment.
Should this name also be a translated string? I think it should.
There was a problem hiding this comment.
Other entries in UPGRADE_FEATURE_INTRO_MAPPING use hardcoded strings for the name field, so I followed the same pattern for consistency. Would you prefer that I update it to use translation keys instead?
There was a problem hiding this comment.
OK, I think we can leave it as-is for this PR, but maybe we should open up a new GH to make them consistent and I do think we want them to be translated keys. I don't know why we wouldn't.
| enableAutoApprovalOptions(policyID, true); | ||
| Navigation.goBack(); | ||
| if (route.params.backTo) { | ||
| Navigation.navigate(route.params.backTo); |
There was a problem hiding this comment.
Won't this cause navigation to happen twice? Once for goBack() and then again navigate()? Is that for sure what we want? It kind of feels like it should be more like this:
if (route.params.backTo) {
Navigation.navigate(route.params.backTo);
return;
}
Navigation.goBack();
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@nyomanjyotisa This PR picked up a conflict. |
|
@nyomanjyotisa kindly bump |
|
Sorry for the delayed response, I’ve merged with the latest main I also moved the enforcement setting enabling logic from Previously, clicking outside the RHP after upgrading wouldn't toggle the feature |
| preventSelfApproval: { | ||
| id: 'preventSelfApproval' as const, | ||
| alias: 'prevent-self-approval', | ||
| name: 'Advanced Approvals' as const, |
There was a problem hiding this comment.
OK, I think we can leave it as-is for this PR, but maybe we should open up a new GH to make them consistent and I do think we want them to be translated keys. I don't know why we wouldn't.
|
✋ 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/tgolen in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|





Explanation of Change
Relocate
ExpenseReportRulesSectionfromPolicyRulesPagetoWorkspaceWorkflowsPageFixed Issues
$ #74752
PROPOSAL: #74752 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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
MacOS: Desktop