-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Unify expense column pickers via COLUMN_AVAILABILITY and align report details columns #94124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
73f2e4b
Add COLUMN_AVAILABILITY source of truth and align report details colu…
MelvinBot 588ef2c
Fix spellcheck: add taxrate, exportedto, toggleable to cspell dictionary
MelvinBot 73b9617
Revert keeping Posted/Original amount visible without values in repor…
MelvinBot 25418e7
Make Posted and Original amount data-driven in report view + add regr…
MelvinBot 67c9e76
Gate POSTED and ORIGINAL_AMOUNT behind explicit selection in report view
MelvinBot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| /** | ||
| * Guard tests for the single source of truth that drives the two expense column pickers. | ||
| * | ||
| * `CONST.SEARCH.COLUMN_AVAILABILITY` declares, per transaction-level column, whether it is available in the | ||
| * Search picker (`search`) and/or the single-report picker (`reportView`). Both | ||
| * `CONST.SEARCH.TYPE_CUSTOM_COLUMNS.EXPENSE` and `CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS` are derived from it. | ||
| * | ||
| * These tests fail if a new `TABLE_COLUMNS` value is added without a deliberate availability decision (either an | ||
| * entry in `COLUMN_AVAILABILITY` or an explicit listing under another surface), preventing the two pickers from | ||
| * silently drifting apart the way they did historically. | ||
| */ | ||
| describe('Column availability single source of truth', () => { | ||
| const TABLE_COLUMNS = CONST.SEARCH.TABLE_COLUMNS; | ||
| const allTableColumns = new Set<string>(Object.values(TABLE_COLUMNS)); | ||
| const availabilityColumns: string[] = Object.values(CONST.SEARCH.COLUMN_AVAILABILITY).map((availability) => availability.column); | ||
|
|
||
| // Columns that intentionally belong to surfaces other than the two expense column pickers. Every | ||
| // TABLE_COLUMNS value must be either classified in COLUMN_AVAILABILITY or listed here, so adding a new | ||
| // column forces a deliberate decision instead of silently doing nothing. | ||
| const OTHER_SURFACE_COLUMNS = new Set<string>([ | ||
| // Structural / row-chrome columns rendered implicitly, never user-toggleable. | ||
| TABLE_COLUMNS.AVATAR, | ||
| TABLE_COLUMNS.TYPE, | ||
| TABLE_COLUMNS.COMMENTS, | ||
| // Task search view. | ||
| TABLE_COLUMNS.ASSIGNEE, | ||
| TABLE_COLUMNS.IN, | ||
| // Expense-report (report list) view. | ||
| TABLE_COLUMNS.REIMBURSABLE_TOTAL, | ||
| TABLE_COLUMNS.NON_REIMBURSABLE_TOTAL, | ||
| // Grouped search views. | ||
| TABLE_COLUMNS.EXPENSES, | ||
| TABLE_COLUMNS.FEED, | ||
| TABLE_COLUMNS.WITHDRAWN, | ||
| TABLE_COLUMNS.BANK_ACCOUNT, | ||
| TABLE_COLUMNS.GROUP_FROM, | ||
| TABLE_COLUMNS.GROUP_EXPENSES, | ||
| TABLE_COLUMNS.GROUP_TOTAL, | ||
| TABLE_COLUMNS.GROUP_CARD, | ||
| TABLE_COLUMNS.GROUP_FEED, | ||
| TABLE_COLUMNS.GROUP_BANK_ACCOUNT, | ||
| TABLE_COLUMNS.GROUP_WITHDRAWN, | ||
| TABLE_COLUMNS.GROUP_WITHDRAWAL_ID, | ||
| TABLE_COLUMNS.GROUP_CATEGORY, | ||
| TABLE_COLUMNS.GROUP_MERCHANT, | ||
| TABLE_COLUMNS.GROUP_TAG, | ||
| TABLE_COLUMNS.GROUP_MONTH, | ||
| TABLE_COLUMNS.GROUP_WEEK, | ||
| TABLE_COLUMNS.GROUP_YEAR, | ||
| TABLE_COLUMNS.GROUP_QUARTER, | ||
| TABLE_COLUMNS.GROUP_WITHDRAWAL_STATUS, | ||
| ]); | ||
|
|
||
| test('every TABLE_COLUMNS value is classified in COLUMN_AVAILABILITY or owned by another surface', () => { | ||
| const unclassified = [...allTableColumns].filter((column) => !availabilityColumns.includes(column) && !OTHER_SURFACE_COLUMNS.has(column)); | ||
|
|
||
| // If this fails, a new TABLE_COLUMNS value was added without deciding its picker availability. | ||
| // Add it to CONST.SEARCH.COLUMN_AVAILABILITY (with search/reportView flags) or, if it belongs to | ||
| // another surface (grouped/report-list/task view), to OTHER_SURFACE_COLUMNS above. | ||
| expect(unclassified).toEqual([]); | ||
| }); | ||
|
|
||
| test('COLUMN_AVAILABILITY and OTHER_SURFACE_COLUMNS are disjoint', () => { | ||
| const overlap = availabilityColumns.filter((column) => OTHER_SURFACE_COLUMNS.has(column)); | ||
| expect(overlap).toEqual([]); | ||
| }); | ||
|
|
||
| test('every value in COLUMN_AVAILABILITY is a valid TABLE_COLUMNS value', () => { | ||
| const invalid = availabilityColumns.filter((column) => !allTableColumns.has(column)); | ||
| expect(invalid).toEqual([]); | ||
| }); | ||
|
|
||
| test('TYPE_CUSTOM_COLUMNS.EXPENSE is exactly the search-enabled columns in declaration order', () => { | ||
| const expectedSearchColumns = Object.values(CONST.SEARCH.COLUMN_AVAILABILITY) | ||
| .filter((availability) => availability.search) | ||
| .map((availability) => availability.column); | ||
| expect(Object.values(CONST.SEARCH.TYPE_CUSTOM_COLUMNS.EXPENSE)).toEqual(expectedSearchColumns); | ||
| }); | ||
|
|
||
| test('REPORT_DETAILS_CUSTOM_COLUMNS is exactly the reportView-enabled columns in declaration order', () => { | ||
| const expectedReportColumns = Object.values(CONST.SEARCH.COLUMN_AVAILABILITY) | ||
| .filter((availability) => availability.reportView) | ||
| .map((availability) => availability.column); | ||
| expect(Object.values(CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS)).toEqual(expectedReportColumns); | ||
| }); | ||
|
|
||
| test('every derived picker column is a valid TABLE_COLUMNS value', () => { | ||
| const expenseColumns = Object.values(CONST.SEARCH.TYPE_CUSTOM_COLUMNS.EXPENSE); | ||
| const reportColumns = Object.values(CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS); | ||
| expect(expenseColumns.filter((column) => !allTableColumns.has(column))).toEqual([]); | ||
| expect(reportColumns.filter((column) => !allTableColumns.has(column))).toEqual([]); | ||
| }); | ||
|
|
||
| test('the four transaction-level columns are aligned into the single-report picker', () => { | ||
| const reportColumns = Object.values(CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS); | ||
| expect(reportColumns).toContain(TABLE_COLUMNS.POSTED); | ||
| expect(reportColumns).toContain(TABLE_COLUMNS.ATTENDEES); | ||
| expect(reportColumns).toContain(TABLE_COLUMNS.TOTAL_PER_ATTENDEE); | ||
| expect(reportColumns).toContain(TABLE_COLUMNS.ORIGINAL_AMOUNT); | ||
| }); | ||
|
|
||
| test('report-level columns stay out of the single-report picker', () => { | ||
| const reportColumns = Object.values(CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS); | ||
| const reportLevelColumns = [ | ||
| TABLE_COLUMNS.STATUS, | ||
| TABLE_COLUMNS.SUBMITTED, | ||
| TABLE_COLUMNS.APPROVED, | ||
| TABLE_COLUMNS.EXPORTED, | ||
| TABLE_COLUMNS.EXPORTED_TO, | ||
| TABLE_COLUMNS.FROM, | ||
| TABLE_COLUMNS.TO, | ||
| TABLE_COLUMNS.POLICY_NAME, | ||
| TABLE_COLUMNS.REPORT_ID, | ||
| TABLE_COLUMNS.BASE_62_REPORT_ID, | ||
| TABLE_COLUMNS.TITLE, | ||
| TABLE_COLUMNS.ACTION, | ||
| ]; | ||
| for (const column of reportLevelColumns) { | ||
| expect(reportColumns).not.toContain(column); | ||
| } | ||
| }); | ||
|
|
||
| test('the derived picker lists match their snapshots', () => { | ||
| // Snapshots make any future change to either picker list a deliberate, reviewed update. | ||
| expect(Object.values(CONST.SEARCH.TYPE_CUSTOM_COLUMNS.EXPENSE)).toMatchSnapshot('TYPE_CUSTOM_COLUMNS.EXPENSE'); | ||
| expect(Object.values(CONST.SEARCH.REPORT_DETAILS_CUSTOM_COLUMNS)).toMatchSnapshot('REPORT_DETAILS_CUSTOM_COLUMNS'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
Postedis added toREPORT_DETAILS_CUSTOM_COLUMNS, the report column settings page lets users save it, butMoneyRequestReportTransactionListstill callsgetColumnsToShow(..., isExpenseReportView: true), whose custom-column path filters the saved list down tononDataColumns.has(col) || columns[col].POSTEDis never set in the report-viewcolumnsmap and is not innonDataColumns(same for the newly exposedORIGINAL_AMOUNTbelow), so selecting these new options inr/:reportID/settings/columnssilently drops them and they never render.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — implemented the data-driven approach in
25418e7.Instead of
nonDataColumns,getColumnsToShownow 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-5864Both are also seeded as
falsein the report-viewcolumnsmap so they default to hidden and are only kept by the existingcolumns[col]presence check (nononDataColumnsentry). 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:⚠️ I couldn't run ESLint locally — this environment's
SearchUIUtilsTest351/351 passed (incl. the 4 new tests);typecheckandprettierclean.lint-changedrelies ongit merge-base origin/main HEAD, which fails on the shallow PR checkout, and directeslintinvocation isn't available here. The change adds no new imports and follows the existing data-presence patterns, andtsgotypecheck passed, but a CI lint pass should confirm.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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):
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.