Repository navigation
[No QA] Add Day to the Insights Spend over time group-by options - #102636
Conversation
Co-authored-by: daledah <daledah@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Web test steps verification results: ❌ fail The Insights Group by menu lists Day first, but selecting Day breaks the Spend over time chart because the backend rejects groupBy:day (GetInsights returns jsonCode 400). After that, the Group by control disappears, so there is no way to switch back to Week or Month without reloading.
|
|
@MelvinBot please unhold this PR since Backend is in prod and open this PR |
|
I removed I couldn't mark the PR ready for review because that action was blocked in my run. Before you ask for review, fill out the Tests and QA Steps sections. They are still TODO placeholders. The PR body's backend dependency note also still says not to merge. You can remove it now that the backend change is in production. |
Reviewer Checklist
Screenshots/VideosiOS: HybridAppios-mweb.moviOS: mWeb Safariios.movMacOS: Chrome / Safariweb.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a62c8bc57
ℹ️ 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 INSIGHTS_GROUP_BY_OPTIONS = [CONST.SEARCH.GROUP_BY.WEEK, CONST.SEARCH.GROUP_BY.MONTH, CONST.SEARCH.GROUP_BY.QUARTER, CONST.SEARCH.GROUP_BY.YEAR] as const; | ||
| const INSIGHTS_GROUP_BY_OPTIONS = [CONST.SEARCH.GROUP_BY.DAY, CONST.SEARCH.GROUP_BY.WEEK, CONST.SEARCH.GROUP_BY.MONTH, CONST.SEARCH.GROUP_BY.QUARTER, CONST.SEARCH.GROUP_BY.YEAR] as const; |
There was a problem hiding this comment.
Wait to expose daily grouping until GetInsights supports it
Until the server-side GetInsights change is deployed, selecting this new item persists groupBy:day; the Insights query builder forwards that value to GetInsights, which currently returns 400 Invalid groupBy for insights (as noted in this commit). Any online user who chooses Day will therefore see the full-page error instead of the dashboard. Gate this option on backend availability or land it together with the backend support.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The BE PR is now on production, and I've tested it thoroughly. The groupBy:day flow is working correctly.
|
🚧 JmillsExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
JmillsExpensify
left a comment
There was a problem hiding this comment.
LGTM to me as well.
|
🚧 luacmartins 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/luacmartins in version: 9.5.2-0 🚀
|
|
No help site update needed. The Day option only appears in the group-by dropdown on the Insights page, which is behind the |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |







Explanation of Change
Adds Day to the Group by options on the Insights "Spend over time" graph.
Insights keeps its own list of allowed group-by values in
INSIGHTS_GROUP_BY_OPTIONS. The earlier daily grouping work added Day to Search, but not to this list. Because the dropdown builds its menu from this list, and URL parsing drops anygroupByvalue that is not in it, Day never showed up in Insights.This PR:
CONST.SEARCH.GROUP_BY.DAYto the start ofINSIGHTS_GROUP_BY_OPTIONS, so the menu reads Day, Week, Month, Quarter, Year.DAYto theInsightsFilters['groupBy']type.groupBy:dayround-trips throughbuildInsightsQueryStringandparseInsightsFilters, and the headline chart is grouped by day while supporting charts keep their own group-by.The default stays Month. Translations, chart config, and
SearchUIUtilsalready handle Day, so no other App changes are needed.Backend dependency: the backend
GetInsightscommand does not supportgroupBy:dayfor this graph yet, and returns400 Invalid groupBy for insights. A companion internal issue tracks that change. Do not merge this PR until the backend change is deployed.Fixed Issues
$ #102571
PROPOSAL: #102571 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
AI Tests
Melvin ran these locally. All passed:
npm teston the Insights-related suites (8 suites, 644 tests):insightsFilterParsingTest,insightsQueriesTest,resolveInsightsChartDataTest,resolveDashboardStateTest,useInsightsFilters.test,useInsightDataTest,SearchUIUtilsTest,InsightsControlsTestnpm run typechecknpm run linton changed filesnpm run fmt(oxfmt) on changed files, with no diffnpm run spell-changedNot run: the full
npm testsuite and the Storybook smoke test. CI covers them.A browser check was attempted, but the tester opened the Search "Group by" menu instead of the Insights one, so it does not confirm this change.