[NoQA] Feature: Travel Invoicing – Release 2.8: Travel Mapping Section in Integration Settings#82736
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❌ 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.
|
melvin-bot
Bot
requested review from
JmillsExpensify and
parasharrajat
and removed request for
a team
|
@parasharrajat 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] |
|
@parasharrajat 🟢 Ready for review! Please checkout the latest Slack discussion for context. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: e17e556b8e
ℹ️ 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".
| route: 'workspaces/:policyID/accounting/quickbooks-online/travel-invoicing', | ||
|
|
||
| // eslint-disable-next-line no-restricted-syntax -- Legacy route generation | ||
| getRoute: (policyID: string) => `workspaces/${policyID}/accounting/quickbooks-online/travel-invoicing` as const, |
There was a problem hiding this comment.
The travel-invoicing configuration route builder drops backTo, unlike neighboring QBO export routes, so callers cannot preserve navigation context when entering this screen. This breaks flows that rely on that context (for example src/pages/workspace/companyCards/utils.tsx passes a backTo into the export page): after visiting Travel Invoicing and navigating back, the export screen no longer has the original backTo, so subsequent back navigation can no longer return to the originating company-cards route.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ I updated ROUTES.ts so getRoute now accepts the backTo?: string argument and appends ?backTo=... to the generated route path, just like the other QBO routes.
|
Note Not sure why prettier is failing, I ran it on the branch and there are no changes locally 🤔 |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Part of an existing product initiative and in line with spec.
…7-travel-invoicing-28
|
Looks good. |
|
@codex review |
| [SCREENS.WORKSPACE.ACCOUNTING.QUICKBOOKS_ONLINE_TRAVEL_INVOICING_CONFIGURATION]: { | ||
| policyID: string; | ||
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo?: Routes; | ||
| }; | ||
| [SCREENS.WORKSPACE.ACCOUNTING.QUICKBOOKS_ONLINE_TRAVEL_INVOICING_VENDOR_SELECT]: { | ||
| policyID: string; | ||
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo?: Routes; | ||
| }; | ||
| [SCREENS.WORKSPACE.ACCOUNTING.QUICKBOOKS_ONLINE_TRAVEL_INVOICING_PAYABLE_ACCOUNT_SELECT]: { | ||
| policyID: string; | ||
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo?: Routes; | ||
| }; |
There was a problem hiding this comment.
Can we avoid adding backto here?
If not, please add a comment explaining why.
| POLICY_ACCOUNTING_QUICKBOOKS_ONLINE_TRAVEL_INVOICING_CONFIGURATION: { | ||
| route: 'workspaces/:policyID/accounting/quickbooks-online/travel-invoicing', | ||
|
|
||
| // eslint-disable-next-line no-restricted-syntax -- Legacy route generation | ||
| getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`workspaces/${policyID}/accounting/quickbooks-online/travel-invoicing` as const, backTo), | ||
| }, | ||
| POLICY_ACCOUNTING_QUICKBOOKS_ONLINE_TRAVEL_INVOICING_VENDOR_SELECT: { | ||
| route: 'workspaces/:policyID/accounting/quickbooks-online/travel-invoicing/vendor', | ||
|
|
||
| // eslint-disable-next-line no-restricted-syntax -- Legacy route generation | ||
| getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`workspaces/${policyID}/accounting/quickbooks-online/travel-invoicing/vendor` as const, backTo), | ||
| }, | ||
| POLICY_ACCOUNTING_QUICKBOOKS_ONLINE_TRAVEL_INVOICING_PAYABLE_ACCOUNT_SELECT: { | ||
| route: 'workspaces/:policyID/accounting/quickbooks-online/travel-invoicing/payable-account', | ||
|
|
||
| // eslint-disable-next-line no-restricted-syntax -- Legacy route generation | ||
| getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`workspaces/${policyID}/accounting/quickbooks-online/travel-invoicing/payable-account` as const, backTo), |
There was a problem hiding this comment.
Same here, can we avoid the disable? If not please comment why
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b626b21a81
ℹ️ 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".
| ? [ | ||
| { | ||
| description: translate('workspace.qbo.travelInvoicing'), | ||
| onPress: !policyID ? undefined : () => Navigation.navigate(ROUTES.POLICY_ACCOUNTING_QUICKBOOKS_ONLINE_TRAVEL_INVOICING_CONFIGURATION.getRoute(policyID)), |
There was a problem hiding this comment.
This onPress is the only export submenu navigation here that does not pass Navigation.getActiveRoute() as backTo, so if the export page was opened with its own backTo param, entering Travel Invoicing and returning drops that context and can send users to an unexpected screen on the next back action (including bypassing the existing goBackFromExportConnection handling). Use getRoute(policyID, Navigation.getActiveRoute()) like the neighboring menu items to preserve the navigation chain.
Useful? React with 👍 / 👎.
…7-travel-invoicing-28
@rlinoz I used With your requests, this will be pushed back at least 1-2 days as I need to refactor the entire navigation for the Travel Mapping section and the 2 routes to use Dynamic Routes, which I did not use before - not familiar with so I have to:
Warning Dynamic Routes currently have a limitation: they do not support stacking multiple suffixes on a single path. If we make Travel Invoicing Configuration a dynamic route (travel-invoicing), any direct sub-pages (e.g. Vendor and Payable Account) cannot also be dynamic routes stacked on top of it. Therefore, this plan treats Travel Invoicing Configuration itself as the dynamic route appended to Export, while keeping Vendor Select and Payable Account Select as static routes that fall back gracefully. |
|
I am ok with merging this as is if we create a clean up issue to fix routing. |
|
Sure, that would be best given the timeline 🙌 |
blimpich
left a comment
blimpich
left a comment
There was a problem hiding this comment.
Sounds good to me. @rlinoz can you make a GH for follow up and assign Kevin and @parasharrajat?
|
Issue is here #83572 |
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ 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/rlinoz in version: 9.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
6 participants
Explanation of Change
UX Flow
On the workspace’s accounting integration configuration page (e.g., QuickBooks, NetSuite):
Data & Onyx
We extend the existing integration config stored on the policy object, for example:
policy.connections?.quickbooksOnline?.config.travelInvoicingVendorIDpolicy.connections?.quickbooksOnline?.config.travelInvoicingPayableAccountIDOn render:
useOnyx(ONYXKEYS.COLLECTION.POLICY)to get the policy.Actions
Reuse existing integration settings action in
src/libs/actions/Policy/PolicyConnections.ts:PolicyConnections.updateConnectionConfig(policyID, connectionName, partialConfig):{travelInvoicingVendorID: selectedVendorID}{travelInvoicingPayableAccountID: selectedAccountID}The backend updates the integration config and returns the updated policy, which Onyx merges into
ONYXKEYS.COLLECTION.POLICY.Components & reuse
WorkspaceAccountingDetailsPage, to add a new section:MenuItemor similar to show:SelectionListfor list of vendors/accounts.MenuItem/RadioListItemfor rows.Testing
PolicyConnections.updateConnectionConfigwith the expectedtravelInvoicingVendorID/travelInvoicingPayableAccountID.Fixed Issues
$ #78680
PROPOSAL:
Tests
Preconditions
contributor_plusQBO)1. Verify Section Rendering
Objective: Ensure the "Travel Invoicing" section renders only when the feature is enabled.
Steps:
2. Verify Selection Interaction
Objective: Ensure each row is selectable and opens the correct picker.
Steps:
SelectionList) showing available vendors.3. Verify Updating Vendor/Account
Objective: Ensure selection updates the integration config via the backend.
Steps:
Offline tests
B - Optimistic With Feedback: Config change with pending stateQA 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
Screen.Recording.2026-02-19.at.16.52.19.mov