Repository navigation
Use one export rule across the to-do, the sidebar and the report page - #102953
dukenv0307 wants to merge 18 commits into
Conversation
dukenv0307
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
|
![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: 6c927d9ab2
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920bf86e10
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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: 0b3aa80d9f
ℹ️ 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".
dukenv0307
changed the title
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
bug.movI set |
|
So the bug is that Cash is selected as default in UI, despite 
bug.mov |
|
Looks good. Just waiting on #102953 (comment) |
|
@codex review |
|
@MelvinBot review |
ReviewThe logic looks right overall, and every CI check passes except the PR Author checklist. I found two things to fix before merge, plus the cash-basis item you already raised. 1. Cash basis: the report preview still offers Export. 2. The file's doc comment now sits on an import. In 3. Card-only reports on cash basis (your open thread). You're still waiting on the backend fix. When it lands, the cash branch at Checked with no issues
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d72b9d07ba
ℹ️ 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".
Regression checkI found one likely regression and three smaller risks. I reviewed the code at 1. Likely regression: "queued for export" actions now show in chat and the LHN. 2. The More menu still offers Export when the header hides it. The secondary 3. Card-only reports lose Export when transactions aren't loaded. 4. A stale Checked with no regression found
|
|
Case 1 is wrong since the backend sends QUEUEDFOREXPORT with an empty message Please review again |
Re-reviewAll four points are settled, and I found no blocking issues in
Two optional follow-ups:
|
|
@situchan All yours! |
| /** | ||
| * Which accounting integration is connected, whether it is syncing, and who exports. | ||
| * Extracted from PolicyUtils/index.ts to keep that file smaller. | ||
| */ |
There was a problem hiding this comment.
This comment was wrongly moved
| /** Needed to skip archived reports, which the backend never returns as exportable */ | ||
| reportNameValuePairs?: ReportNameValuePairs; | ||
|
|
||
| /** Needed for the cash-basis carve-out on card-only reports; Search snapshot rows aren't reliably in Onyx */ |
There was a problem hiding this comment.
Avoid semicolon in inline comment
|
Conflicts! |
Regression check (round 3)I found no new regressions in Merging main: none of the 1015 commits touch the export logic. Only two import lists conflict, and both are easy to fix:
Two behaviors are worth confirming against the backend. Neither is a confirmed bug:
Checked with no regression found
I didn't run the app. These cases need a connected accounting integration, which the test account doesn't have. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@MelvinBot Why is the test failing? |
|
The All the errors are in two files this PR doesn't touch:
Your branch has no conflicts with main, so CI typechecks the merge with the latest main and picks up these errors. You don't need to change anything. Once main is fixed, merge main again or re-run the check. Evidence
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
(Vit's agent) I compared this PR with the backend rules in https://github.com/Expensify/Auth/pull/25129 and https://github.com/Expensify/Auth/pull/25628, one clause at a time. These all match Auth: the export failure check (only the latest integration message counts, and not one from before the last approval reset), the queued check, the archive check, and the cash basis rule for card only reports, including closed and empty reports. I found no blockers in the code.
Please fix these before merge:
- The QA steps fail on a new connection. A newly connected QBO, Xero or Intacct has no
accountingMethodkey. The settings page shows Cash for that state (DynamicQuickbooksOnlineAccountingMethodPage.tsx:37).getIntegrationAccountingMethodand Auth both read it as Accrual. A tester who sees Cash already selected does not change it, so step 5 shows the to-do. @situchan hit this. Please change step 2 to: select Accrual, then select Cash again, so the key is saved. - The card only rule has no tests. See the inline comment on
isCardOnlyReport. - The file docblock in
connections.tsstill sits on theexpensify-commonimport (from @situchan's thread). Move it back to line 1. - The exporter list does not match Auth for Zoho Books and Business Central. See the inline comment on
getConnectionExporters.
Follow-up, not for this PR: an unset accountingMethod has three defaults across the stack.
- The App settings pages show Cash.
- Integration-Server uses Cash when ACH reimbursement is on, and Accrual otherwise (QuickbooksOnlineReportExporterHelper.java:4427; the NetSuite, QBD, Xero and Intacct exporters do the same).
- Auth search and this PR use Accrual.
This PR keeps the to-do and the search in step with each other, so it is not a regression. But the settings page, the exporter and the to-do can still disagree about which method a workspace uses. I think that answers @situchan's question about fixing it separately: yes, in its own issue.
| * Whether a report is card-only (has non-reimbursable spend and nothing reimbursable). Prefers the per-transaction answer when | ||
| * expenses are loaded, and falls back to the stored report totals otherwise, matching the backend's "no reimbursable transaction" check. | ||
| */ | ||
| function isCardOnlyReport(report: OnyxEntry<Report>, transactionsParam?: Transaction[]): boolean { |
There was a problem hiding this comment.
(Vit's agent) The last two commits added this function and the card only branch in both isExportAction and canExport. That fixed the bug @situchan found, but no test covers it. Please add these cases:
- Cash basis, approved, every expense non reimbursable: Export shows.
- Cash basis, approved, one reimbursable and one non reimbursable expense: Export waits for payment.
- Cash basis, approved, no transactions passed,
totalequal to a non zerononReimbursableTotal: the fallback to report totals returns true. - Case 1 again through
getReportPreviewAction, which reachescanExport.
Auth tests cases 1 and 2 in SearchFiltersTest2::testExportActionAutoSyncAndReportState, so these tests keep both sides in step.
| policy?.connections?.rillet?.config?.export?.exporter, | ||
| policy?.connections?.dualEntry?.config?.export?.exporter, | ||
| policy?.connections?.campfire?.config?.export?.exporter, | ||
| policy?.connections?.financialforce?.config?.export?.exporter, |
There was a problem hiding this comment.
(Vit's agent) Adding financialforce matches Auth. Two connections still differ from Auth's FilterUtils::getConnectionExporterFields:
- Zoho Books: Auth reads
zohoBooks.config.export.exporter, and App types that field, but this list has no Zoho Books entry. Before this PR, the to-do and the sidebar suggestion checkedpolicy.exporter. Now they checkisPreferredExporter, so a Zoho Books exporter gets no Export to-do, whileaction:export exporter:mestill returns their reports. One line here fixes it. - Business Central: this is the reverse. App counts the Business Central exporter, but Auth leaves
businessCentralout of its exporter fields,getConnectionConfigQueryandINTEGRATIONS_SUPPORTING_AUTO_EXPORT. A Business Central exporter sees "Export N reports" on Home, and the search returns nothing. This needs an Auth change or a follow-up issue.
Both integrations are behind betas, so few users see this.
| UNREPORTED_TRANSACTION: 'UNREPORTEDTRANSACTION', | ||
| UNDELETED_TRANSACTION: 'UNDELETEDTRANSACTION', | ||
| OUTDATED_BANK_ACCOUNT: 'OUTDATEDBANKACCOUNT', // OldDot Action | ||
| QUEUED_FOR_EXPORT: 'QUEUEDFOREXPORT', |
There was a problem hiding this comment.
(Vit's agent) Non blocking. This new type puts QUEUEDFOREXPORT into supportedActionTypes, so the type check in shouldReportActionBeVisible (ReportActionsUtils.ts:1340) lets it through. It stays out of chat only because its message is empty, which makes isDeletedAction return true. Auth lists this action in Report::HIDDEN_ACTIONS. If the backend ever gives it message text, it shows up in chat. An explicit hide in shouldReportActionBeVisible would make that safe.
This branch has not been deployed
5 participants
Explanation of Change
Read the exporter from the connections like the server does, and skip reports that are archived, queued for export, or cash-basis and not yet paid.
Fixed Issues
$ #102868
BE PR: https://github.com/Expensify/Auth/pull/25129, https://github.com/Expensify/Auth/pull/25628
Tests
type:expense-report action:export exported:never exporter:me.Offline tests
Same as above
QA Steps
Same as above
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