Custom Reports: Include current

[carkom]

Testing an option to add/remove current interval to "live" ranges.

WARNING: Do not use live budget files with this PR as it contains a migration. Please use a copy or a test budget ONLY.

I still need to add a migration so that it can be saved as part of the report. Holding off on that right now as I don't want it to mess up anyone's budget file.

[netlify]

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	191019f
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/6662097e9407540008c5a010
😎 Deploy Preview	https://deploy-preview-2684.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions]

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
9	4.74 MB → 4.74 MB (+2.61 kB)	+0.05%

Changeset

File	Δ	Size
src/components/reports/disabledList.ts	📈 +731 B (+21.28%)	3.35 kB → 4.07 kB
src/components/reports/getLiveRange.ts	📈 +94 B (+9.54%)	985 B → 1.05 kB
src/components/reports/ReportSidebar.tsx	📈 +1.27 kB (+8.06%)	15.71 kB → 16.97 kB
home/runner/work/actual/actual/packages/loot-core/src/client/data-hooks/reports.ts	📈 +57 B (+4.82%)	1.16 kB → 1.21 kB
src/components/reports/reports/CustomReport.tsx	📈 +298 B (+1.49%)	19.52 kB → 19.81 kB
src/components/reports/ReportOptions.ts	📈 +82 B (+1.25%)	6.42 kB → 6.5 kB
home/runner/work/actual/actual/packages/loot-core/src/shared/months.ts	📈 +85 B (+1.10%)	7.53 kB → 7.61 kB
src/components/reports/reports/GetCardData.tsx	📈 +31 B (+0.72%)	4.2 kB → 4.23 kB
src/components/reports/reportRanges.ts	📉 -1 B (-0.03%)	2.8 kB → 2.8 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/ReportRouter.js	1.23 MB → 1.23 MB (+2.47 kB)	+0.20%
static/js/index.js	3.01 MB → 3.01 MB (+142 B)	+0.00%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/resize-observer.js	18.37 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/narrow.js	75.5 kB	0%
static/js/usePreviewTransactions.js	790 B	0%
static/js/AppliedFilters.js	20.41 kB	0%
static/js/wide.js	263.37 kB	0%

[github-actions]

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
1	1.2 MB → 1.2 MB (+160 B)	+0.01%

Changeset

File	Δ	Size
packages/loot-core/src/server/reports/app.ts	📈 +131 B (+2.99%)	4.27 kB → 4.4 kB
packages/loot-core/src/shared/months.ts	📈 +89 B (+0.77%)	11.36 kB → 11.44 kB
packages/loot-core/src/server/aql/schema/index.ts	📈 +75 B (+0.53%)	13.9 kB → 13.97 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
kcab.worker.js	1.2 MB → 1.2 MB (+160 B)	+0.01%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

[Teprifer]

The 'current interval' controlled by the option is 'month' rather than what is defined by the interval option? if so I'd suggest renaming it to Include current month for clarity. e.g. interval is weekly, with a live range of last 3 months, this option toggles the 4 weeks starting this month.
How it is now vs making it go by the interval option both makes sense to me (as the budgeting is monthly).

Noticed option is disabled when using a static range(makes obvious sense). Were you thinking to disable it for the interval + live ranges that it doesn't make sense for, or not worry about that?

[carkom]

The 'current interval' controlled by the option is 'month' rather than what is defined by the interval option? if so I'd suggest renaming it to Include current month for clarity. e.g. interval is weekly, with a live range of last 3 months, this option toggles the 4 weeks starting this month. How it is now vs making it go by the interval option both makes sense to me (as the budgeting is monthly).

Noticed option is disabled when using a static range(makes obvious sense). Were you thinking to disable it for the interval + live ranges that it doesn't make sense for, or not worry about that?

It's not always monthly. It's based on the range interval. Example: choosing "Last week" and Interval = daily. The toggle would give you all the days from this week as well.

If we changed it to match the interval instead and you chose "last month" and interval = weekly. You'd see all 4 weeks of March. Select the new toggle and you'd get the 4th week of April - the first three weeks would be missing. Not sure that makes sense to ever have a view like that. Also, it makes the code needlessly complicated.

I see your point with labeling, because it's not actually "current interval". I've changed the wording to follow the "range" selection. So it says "Include current month" when a monthly range is chosen and "Include current week" when a weekly range is chosen.

[Teprifer]

Ah! I see where I was getting confused, thanks. :) (there was an extra layer of misunderstanding on my part too)

[carkom]

@Teprifer, can you check this meets your expectations?

Make sure to only use a copied budget file or a test file as the migration in this PR will cause the budget file to not work with edge.

[Teprifer]

Awww <3

Yeah I always test in a browser data cleared chrome window with a freshly exported budget.

• Year to Date, includes up to current date regardless of the include current option setting.
  Not sure this is incorrect, since by definition 'to date' means, well, to the current day. But also there's the option to include/exclude the current X (see next point). Eh, thoughts? For what it's worth I can see it both ways reasonably equally.
  Mayyyybe it could be a thing that if 'year to date' or 'all time' is selected this option is forced to On automatically(to keep the current behaviour), and then users could optionally turn it off?

• The option 'include current <week/month>' has no time unit when using a range of 'year to date' 'last year' or 'all time', and so just reads 'include current', wonder if for these it should be 'month' for year to date, and 'year' for all time. For 'last year' eh, disable/grey out?? Have a think on this and the above and what you think makes sense - happy to discuss further :)

Besides those minor points it looks pretty good.

[carkom]

@Teprifer So there's a regression from a previous PR that broke filters. This PR is not focused on fixing those at all. You can check #2785 for the fix for filters corrections.

Can you edit your review (above) and take out anything that relates to filters or show activity clicks? It would be much easier if we could keep the issues separate. Cheers!

[Teprifer]

@carkom Sorry didn't realise the filter correction stuff included the dates bit (only aware of amounts) so thought maybe related as this has a date affecting option. And the first part was linked to the new option in this PR, anyway, removed.

[carkom]

Awww <3

Yeah I always test in a browser data cleared chrome window with a freshly exported budget.

* Year to Date, includes up to current date regardless of the include current option setting.
  Not sure this is incorrect, since by definition 'to date' means, well, to the current day. But also there's the option to include/exclude the current X (see next point). Eh, thoughts? For what it's worth I can see it both ways reasonably equally.
  Mayyyybe it could be a thing that if 'year to date' or 'all time' is selected this option is forced to On automatically(to keep the current behaviour), and then users could optionally turn it off?

* The option 'include current <week/month>' has no time unit when using a range of 'year to date' 'last year' or 'all time', and so just reads 'include current', wonder if for these it should be 'month' for year to date, and 'year' for all time. For 'last year' eh, disable/grey out?? Have a think on this and the above and what you think makes sense - happy to discuss further  :)

Besides those minor points it looks pretty good.

I think I've fixed both issues. Have a look.

Making "Year to date" and "All time" function within the "include current" logic is messy because the interval type can change. Example: when interval = Monthly currentInterval would be current month, when interval = weekly currentInterval would be current week. Making this change dynamically adds complexity that I don't see as neccessary.

"Year to date" is meant to be Jan 1st through today. If someone wants Jan of this year through this month minus one then they can utilize the static filters. Same with "All time" range. It's one extra click to use these static fields.

[Teprifer]

I think I've fixed both issues. Have a look.

Making "Year to date" and "All time" function within the "include current" logic is messy because the interval type can change. Example: when interval = Monthly currentInterval would be current month, when interval = weekly currentInterval would be current week. Making this change dynamically adds complexity that I don't see as neccessary.

"Year to date" is meant to be Jan 1st through today. If someone wants Jan of this year through this month minus one then they can utilize the static filters. Same with "All time" range. It's one extra click to use these static fields.

Ah yep, I was thinking it'd be static as month for YTD and year for all time(regardless of interval setting), but as I mentioned I don't think there's an incorrect approach here, so the simplest makes sense. Besides which I imagine it's fairly niche anyhoo.

Had a look, seems fine, my brain is a bit fried, but if you're happy with it, onwards! :)

[youngcw]

Would it make sense to turn this option on by default? I feel like including the current would be wanted more often than not.

[youngcw]

I just realized, that the time range options probably need updated to consider this setting.
For example, if you select last 6 months with "include current" you get 7 months in the report.

[carkom]

I just realized, that the time range options probably need updated to consider this setting. For example, if you select last 6 months with "include current" you get 7 months in the report.

That's as intended right? "Last 6 months" (eg previous 6 months) would be the 6 full (completed) months before the current month. If you tick "include current" then you'd get those same 6 months plus this month (6 + 1 = 7).

The other option would be to make the "Last 6 months" (and other options like it) dynamic. So with includeCurrent selected it would give you Dec '23 to May '24 with it unselected you'd get Nov '23 to Apr '24.

It could make sense either way. I find it a bit confusing that the start date changes based on if "include current" is selected. In my mind "last 6 months" is (current month - 7) through (current month - 1). The status of "includeCurrent" wouldn't change that.

[youngcw]

Your second option is what I expected to happen. For me the "6" part takes precedent and "last x months" could easily mean either before current or with current, so I then expect 6 months in the report. Maybe we can see what others think.

[Teprifer]

With the option off by default the logic is correct.
But I see the point of having it on by default as that matches current behaviour.

I think it could also be approached as a discoverabilty thing, if the include current option was right under the range then it should be obvious what's going on.

Or as you say, update the range text to reference there current month being included(or not), either by changing the number, or appending "(plus current X)" or some variation of.
Then users could discover the option in the options menu, is easy to find.

[twk3]

Just to add my opinion. It's currently working how I would expect. I think it's behaviour is clear when you use "last month" as the example.

If I select "Last month" from the live time range (and I'm graphing by time). And include current month is on. I see last month and this month. Not just this month (I think that would be worse). If I toggle current month off, I just see last month. This seems correct, and based on that, how it currently works for 'last 6 months' also makes sense to me. I get 7 months listed, just the current probably being incomplete. (Which I can remove thanks to this toggle).

[carkom]

Just to add my opinion. It's currently working how I would expect. I think it's behaviour is clear when you use "last month" as the example.

If I select "Last month" from the live time range (and I'm graphing by time). And include current month is on. I see last month and this month. Not just this month (I think that would be worse). If I toggle current month off, I just see last month. This seems correct, and based on that, how it currently works for 'last 6 months' also makes sense to me. I get 7 months listed, just the current probably being incomplete. (Which I can remove thanks to this toggle).

Good explanation of how my mind was working. Thanks! @youngcw, are you okay to merge as is?