Unify expense column pickers via COLUMN_AVAILABILITY and align report details columns#94124
Conversation
…mn picker Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Fixed the failing
Since The other failing check, |
| nonDataColumns.add(CONST.SEARCH.TABLE_COLUMNS.POSTED); | ||
| nonDataColumns.add(CONST.SEARCH.TABLE_COLUMNS.ORIGINAL_AMOUNT); | ||
| } | ||
|
|
There was a problem hiding this comment.
@MelvinBot Why you put those columns to nonDataColumns?
There was a problem hiding this comment.
Because that block is the only thing that keeps Posted and Original amount visible once the user explicitly selects them in the single-report column picker.
The function returns at SearchUIUtils.ts:5955:
return customResult.filter((col) => nonDataColumns.has(col) || columns[col]);columns[col] is a data-presence check — it's only true when at least one transaction in the report actually has a value for that column. nonDataColumns is the allow-list of columns that bypass that check and are kept purely because the user picked them.
POSTED (card posting date) and ORIGINAL_AMOUNT (foreign/original amount) are inherently data-driven: a report may contain no card transactions and no foreign-currency expenses, so columns[POSTED] / columns[ORIGINAL_AMOUNT] would be false. Without adding them to nonDataColumns, the moment a user toggles one on but no transaction in that report happens to have a posting date / original amount, the filter would silently drop it and the explicit selection would vanish.
Adding them keeps them retained whenever selected — matching the existing behavior of ATTENDEES / TOTAL_PER_ATTENDEE already in the set.
It's gated behind isExpenseReportView so the search page is untouched: there these two columns keep their existing data-driven behavior, and only the report view treats them as non-data.
There was a problem hiding this comment.
Because that block is the only thing that keeps Posted and Original amount visible once the user explicitly selects them in the single-report column picker.
@MelvinBot I don't think we need to keep those columns visible when they don't have values, even if the user explicitly selected them. Please revert this change.
There was a problem hiding this comment.
Done — reverted in 73b9617.
Removed the isExpenseReportView block that added POSTED and ORIGINAL_AMOUNT to nonDataColumns. Both columns now keep their data-driven behavior in the report view: when selected but no transaction in the report has a value, they're dropped by the columns[col] presence check, same as before this PR.
Posted, Attendees, Per attendee, and Original amount are still added to the report column picker (the alignment change) — this only reverts the always-keep-when-selected behavior for the two data-driven columns.
Checks: typecheck, lint-changed, prettier clean; SearchUIUtilsTest + ColumnAvailabilityTest 356/356 passed.
…t column picker Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Update Tests to be Tests
@MelvinBot Mark all auther checklist as checked |
|
🤖 Done,
|
|
@MelvinBot Mark this PR as ready for review |
|
@ahmedGaber93 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-23.at.8.20.31.PM.mov |
|
@MelvinBot Please update Tests to confirm
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b961746e
ℹ️ 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".
| STATUS: {column: this.TABLE_COLUMNS.STATUS, search: true, reportView: false}, | ||
| SUBMITTED: {column: this.TABLE_COLUMNS.SUBMITTED, search: true, reportView: false}, | ||
| APPROVED: {column: this.TABLE_COLUMNS.APPROVED, search: true, reportView: false}, | ||
| POSTED: {column: this.TABLE_COLUMNS.POSTED, search: true, reportView: true}, |
There was a problem hiding this comment.
When Posted is added to REPORT_DETAILS_CUSTOM_COLUMNS, the report column settings page lets users save it, but MoneyRequestReportTransactionList still calls getColumnsToShow(..., isExpenseReportView: true), whose custom-column path filters the saved list down to nonDataColumns.has(col) || columns[col]. POSTED is never set in the report-view columns map and is not in nonDataColumns (same for the newly exposed ORIGINAL_AMOUNT below), so selecting these new options in r/:reportID/settings/columns silently drops them and they never render.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot the bare revert overcorrected — it removed the always-on behavior but added no data-presence path, so Posted and Original amount are now never rendered in the report view (even when transactions do have a posting date / foreign amount).
Please implement the data-driven approach instead of nonDataColumns
Also please add a getColumnsToShow({isExpenseReportView: true}) regression test for those columns
There was a problem hiding this comment.
Done — implemented the data-driven approach in 25418e7.
Instead of nonDataColumns, getColumnsToShow now populates the report-view data-presence map for both columns, so they render when the data exists and are dropped when it doesn't:
ORIGINAL_AMOUNT— set totruein the report view when a currency conversion exists (hasExchangeRate), mirroring the existing search-page behavior.SearchUIUtils.ts:5850-5857POSTED— set totruewhen at least one transaction in the report has a posting date (transaction.posted).SearchUIUtils.ts:5859-5864
Both are also seeded as false in the report-view columns map so they default to hidden and are only kept by the existing columns[col] presence check (no nonDataColumns entry). Search-page behavior is unchanged.
Added 4 getColumnsToShow({isExpenseReportView: true}) regression tests covering hide-when-empty / show-when-present for each column: SearchUIUtilsTest.ts:8894-8957.
Checks: SearchUIUtilsTest 351/351 passed (incl. the 4 new tests); typecheck and prettier clean. lint-changed relies on git merge-base origin/main HEAD, which fails on the shallow PR checkout, and direct eslint invocation isn't available here. The change adds no new imports and follows the existing data-presence patterns, and tsgo typecheck passed, but a CI lint pass should confirm.
There was a problem hiding this comment.
@MelvinBot the data-driven direction is right, but this is now too broad: the POSTED and ORIGINAL_AMOUNT setters aren't gated to an explicit selection, so they also fire in the default report view. I verified that for a report whose transactions have a posting date and a currency conversion — with no custom column selection — both columns now show by default. That means foreign-currency reports render both Amount and Original amount by default, and card reports render Posted by default. That's beyond what we want here; these should only appear when the user explicitly picks them.
Please gate both behind customResult (keep TOTAL_AMOUNT as-is):
if (hasExchangeRate) {
if (isExpenseReportView) {
columns[CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT] = true;
if (customResult) {
columns[CONST.SEARCH.TABLE_COLUMNS.ORIGINAL_AMOUNT] = true;
}
} else {
columns[CONST.SEARCH.TABLE_COLUMNS.ORIGINAL_AMOUNT] = true;
}
}
if (customResult && isExpenseReportView && transaction.posted) {
columns[CONST.SEARCH.TABLE_COLUMNS.POSTED] = true;
}Also add a default-view regression test: with a transaction that has both a posting date and a conversion but no explicit column selection, assert POSTED and ORIGINAL_AMOUNT are not returned. The current 4 tests only cover the custom path, so this default-view behavior slipped through untested.
…ession tests Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
@mjasikowski 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] |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Both columns were being set in the default expense report view whenever a transaction had a posting date or currency conversion, so foreign-currency reports rendered Original amount and card reports rendered Posted by default. Gate both behind customResult so they only appear when the user explicitly selects them. TOTAL_AMOUNT remains data-driven as before. Add a default-view regression test covering a transaction that has both a posting date and a conversion with no explicit column selection. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
Done — gated both setters behind Change in
Regression test added in AI checks run locally
|
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.4.19-1 🚀
Bundle Size Analysis (Sentry): |
Help site review — changes required ✅This PR adds four columns to the single-report Columns picker ( Draft help site PR: #94500 What I changed: In Managing Expenses in a Report, the "How to view and use the expense table" section now lists all four columns available in the single-report Columns picker, with each column's render condition. Names use the exact UI labels (Posted, Attendees, Per attendee, Original amount) verified against No change needed for the Spend/Search picker article (Using Reports in New Expensify) — this PR left the Search column list unchanged. @ahmedGaber93, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #94526 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.19-5 🚀
|
Explanation of Change
The two expense column pickers — the Spend/Search picker (
/search→ Display → Edit columns) and the single-report picker (r/:reportID/settings/columns) — were built from two independently hand-maintained constants (CONST.SEARCH.TYPE_CUSTOM_COLUMNS.EXPENSEandCONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS) with no shared source of truth and no test linking them, so they silently drifted apart.This PR implements the approved proposal:
Single source of truth. Adds
CONST.SEARCH.COLUMN_AVAILABILITY, one ordered map overTABLE_COLUMNSthat declares each transaction-level column's availability per surface (search/reportView). BothTYPE_CUSTOM_COLUMNS.EXPENSEandREPORT_DETAILS_CUSTOM_COLUMNSare now derived from this map (preserving declaration order), so adding a column forces a single, deliberate availability decision instead of editing two lists. The existinggetCustomColumns()and thegetColumnsToShow()allow-list already read these derived constants, so they pick up the change automatically.Aligns the lists (additive only). Adds the four transaction-level columns that were Search-only into the single-report picker — Posted, Attendees, Per attendee (
TOTAL_PER_ATTENDEE), and Original amount — per the rule "a column belongs in the report view only if its value can differ between expenses within the same report." Report Details goes from 18 → 22 columns; Search is unchanged; nothing is removed.Totalstays the one Report-Details-only column, and the report-level columns (Status, Submitted, Approved, Exported, From, To, Workspace, Report ID, etc.) stay Search-only.Renders the newly-added columns when selected. In the single-report view,
getColumnsToShow()now retainsPostedandOriginal amountwhen the user explicitly selects them (matching the existing retention behavior ofAttendees/Per attendee). Search-page behavior for these columns is intentionally left unchanged. The sharedTransactionItemRowWiderenderer andSearchTableHeaderalready support all four columns.Guard test against future drift. Adds
tests/unit/Search/ColumnAvailabilityTest.ts, which fails if aTABLE_COLUMNSvalue is added without being classified inCOLUMN_AVAILABILITYor explicitly attributed to another surface (grouped / report-list / task views). It also asserts both derived lists contain only valid columns and snapshots them so any change is a deliberate, reviewed update.AI tests run locally by Melvin
npm run typecheck(tsc) — passednpm run lint-changed— passednpm run prettier— cleannpm test -- tests/unit/Search/ColumnAvailabilityTest.ts— 9/9 passednpm test -- tests/unit/Search/SearchUIUtilsTest.ts— 347/347 passedStorybook smoke and React Compiler checks were not applicable (no component/story/TSX changes).
Fixed Issues
$ #93522
PROPOSAL: #93522 (comment)
Tests
Offline tests
N/A — change is to client-side column configuration only.
QA Steps
Same as Tests.
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