Add Rillet integration import pages#94780
Conversation
This reverts commit cc4a701.
|
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 |
melvin-bot
Bot
requested review from
flaviadefaria and
truph01
and removed request for
a team
melvin-bot
Bot
requested review from
a team and
lakchote
and removed request for
a team
|
@truph01 This is ready for review, the PR is based on R1 so the changes look too much but you can only select the new commits for easy review (once the other PR is merged the changes will look less). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function ConnectToRilletFlow({policyID}: ConnectToRilletFlowProps) { | ||
| const hasReusablePoliciesConnectedToRillet = useHasReusablePoliciesConnectedTo(CONST.POLICY.CONNECTIONS.NAME.RILLET, policyID); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-16 (docs)
This useEffect has an empty dependency array and performs Navigation.navigate(...), which is non-idempotent. React Strict Mode intentionally double-invokes effects in development, so this effect can fire two navigations on mount. There is no guard (module-level flag or ref) preventing the second run — the if (hasReusablePoliciesConnectedToRillet) check validates a precondition but does not stop re-execution if the same precondition still holds on the second invocation.
Guard the one-time initialization so it runs exactly once regardless of rendering mode:
const didInit = useRef(false);
useEffect(() => {
if (didInit.current) {
return;
}
didInit.current = true;
if (hasReusablePoliciesConnectedToRillet) {
Navigation.navigate(ROUTES.POLICY_ACCOUNTING_RILLET_EXISTING_CONNECTIONS.getRoute(policyID));
return;
}
Navigation.navigate(ROUTES.POLICY_ACCOUNTING_RILLET_SETUP.getRoute(policyID));
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);Reviewed at: 2caaa84 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2caaa84f0f
ℹ️ 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".
| CONST.RILLET_CONFIG.SYNC_TAX_RATES, | ||
| ...(policy?.connections?.rillet?.data?.fields.map((field) => `${CONST.RILLET_CONFIG.FIELD_MAPPING_PREFIX}${field.id}`) ?? []), | ||
| ], | ||
| onExportPagePress: () => null, |
There was a problem hiding this comment.
For every connected Rillet workspace, the accounting page still renders the standard configuration rows, including Export, and wires this handler to onPress; returning null makes the row tappable with a chevron but no navigation or feedback. Either add the Rillet export route/page here or omit the export option until it exists so users do not hit a dead-end configuration entry.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| export default withPolicy(RilletImportPage); |
There was a problem hiding this comment.
When the Rillet import page is opened directly or after a reload, this HOC only subscribes to the policy and never calls openPolicyAccountingPage, so policy.connections.rillet.data can remain empty and the dimensions/tax rows derived from rilletData.fields and rilletData.taxRates are omitted. Use withPolicyConnections here, as the connection-data-dependent accounting pages do, so the import settings are populated outside the happy path from the main Accounting page.
Useful? React with 👍 / 👎.
| <ScreenWrapper | ||
| testID="RilletSetupPage" | ||
| enableEdgeToEdgeBottomSafeAreaPadding | ||
| > |
There was a problem hiding this comment.
Opening /workspaces/:policyID/accounting/rillet/setup directly bypasses the upgrade/access checks in startIntegrationFlow, because this page renders the credentials form in a bare ScreenWrapper instead of an AccessOrNotFoundWrapper/ConnectionLayout with the Control-plan and admin requirements. For a non-Control workspace admin with the URL, the client shows and submits the Rillet API-key form instead of the upgrade/not-found flow.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
Fixed Issues
$ #94582
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari