Skip to content
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

Goals: Schedule multi month forecasting #1452

Merged
merged 12 commits into from
Aug 5, 2023

Conversation

shall0pass
Copy link
Contributor

References discord discussion starting here: https://discord.com/channels/937901803608096828/940290142579605514/1133523705063030824

Currently the schedule keyword won't fill any future budget cells if the category balance already satisfies the schedule. This PR is an attempt to improve the behavior by allowing budget fills regardless of the category balance.

This is a drastic rewrite of the schedule keyword. Though I've tried not to have any regressions, it is possible because of how different the logic is. I've tested compounding using a simple template, so a small change in the 'by' keyword was also made.

@netlify
Copy link

netlify bot commented Aug 3, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit d35fdaa
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/64ce4cfe167c350008463a8d
😎 Deploy Preview https://deploy-preview-1452.demo.actualbudget.org/
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Contributor

github-actions bot commented Aug 3, 2023

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
16 2.08 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/main.js 851.69 kB 0%
static/js/457.chunk.js 388.22 kB 0%
static/media/Inter-italic.var.woff2 239.29 kB 0%
static/media/Inter-roman.var.woff2 221.86 kB 0%
static/js/wide-components.chunk.js 159.17 kB 0%
static/js/383.chunk.js 117.35 kB 0%
static/js/reports.chunk.js 33.23 kB 0%
static/js/narrow-components.chunk.js 32.06 kB 0%
static/js/281.chunk.js 28.55 kB 0%
static/js/876.chunk.js 26.2 kB 0%
static/js/969.chunk.js 12.94 kB 0%
static/js/resize-observer-polyfill.chunk.js 8.12 kB 0%
static/css/main.css 5.82 kB 0%
asset-manifest.json 2.07 kB 0%
index.html 1.66 kB 0%
static/media/browser-server.js 963 B 0%

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

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
2 1.97 MB → 1.97 MB (+1.31 kB) +0.06%
Changeset
File Δ Size
packages/loot-core/src/server/budget/goaltemplates.ts 📈 +3.25 kB (+14.19%) 22.94 kB → 26.19 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
kcab.worker.js 1010.32 kB → 1011.63 kB (+1.31 kB) +0.13%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
xfo.xfo.kcab.worker.js 1004.04 kB 0%

@kyrias
Copy link
Contributor

kyrias commented Aug 3, 2023

I've done some testing of this now and it seems to work exactly like I expected it to work! :)

@youngcw
Copy link
Member

youngcw commented Aug 3, 2023

Using the full flag seems broken. It will work well when the previous months have fully allocated funds, but if the category is at 0 then it will act like its a regular schedule.

@shall0pass
Copy link
Contributor Author

Using the full flag seems broken. It will work well when the previous months have fully allocated funds, but if the category is at 0 then it will act like its a regular schedule.

Thanks for checking. I'll try to fix that up.

@youngcw
Copy link
Member

youngcw commented Aug 3, 2023

something broke with my priority test. It ends up getting negative values if the available funds are negative at time of priority pass (I think)

@shall0pass
Copy link
Contributor Author

something broke with my priority test. It ends up getting negative values if the available funds are negative at time of priority pass (I think)

I think I have it resolved. Could you run your test again? Thanks!

@youngcw
Copy link
Member

youngcw commented Aug 3, 2023

Its a little better. It works well if started in the right month, but still budgets the whole thing in wrong months when the balance is 0 when applying the template.

Also still getting the negative value in the priority test.

@joel-jeremy
Copy link
Contributor

Haven't done an in depth review of the code yet but the current implementation for schedule templates allocate a budget even though the schedule is already completed. Is it still the same here? I think we change that and just ignore the template if it's schedule is already completed. Thoughts?

@shall0pass
Copy link
Contributor Author

Currently it's the same. I'll look at doing a check for a completed schedule though, that's a great point.

I'm still checking for bugs. I found a few more tonight that I hope I resolved with the latest commit, but I don't think I've addressed @youngcw comment about the negative value in a priority level. I'm still trying to replicate the conditions for that to occur so I can fix it.

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again 🔍 Ready for Review and removed 🔍 Ready for Review ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Aug 4, 2023
@shall0pass
Copy link
Contributor Author

I've also noticed that I'm not getting error or warning pop ups. The errors still seem to be passed back to the calling function correctly. Was there a change in the notification system that we need to address from this file?

@youngcw
Copy link
Member

youngcw commented Aug 4, 2023

Priority thing is fixed. With the full flag it seems to be that after 3 broken months future months fill right. I can shift which month I start and it looks to be the same, 3 broken then good. Maybe that's just an artifact of my test.

@shall0pass
Copy link
Contributor Author

I think I broke the error notifications when doing the speed up improvements. The functions are async and those are the only functions not passing the Notifications back. The Check templates and End of month cleanup notifications still come up. I've been trying things, but I'm poking at something I don't fully understand. Could use a hint. 🙏

@youngcw
Copy link
Member

youngcw commented Aug 5, 2023

Beautiful. All my tests are working now.

I can't tell what happened to the regular messages, but the messages for the template check still work.

@MatissJanis
Copy link
Member

@youngcw would you mind doing a code review here? I could do it myself, but since I don't really use the feature.. it would be mostly a rubber stamp.

Copy link
Member

@youngcw youngcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for figuring this out.

@joel-jeremy
Copy link
Contributor

Looks good! Tested and it no longer applies completed schedules. Thanks!

@MatissJanis MatissJanis added this to the v23.8.1 milestone Aug 5, 2023
@shall0pass shall0pass merged commit c581a80 into actualbudget:master Aug 5, 2023
15 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Aug 5, 2023
TomAFrench added a commit to TomAFrench/actual that referenced this pull request Aug 6, 2023
* master: (34 commits)
  ♻️ (crdt) adding more strict typings (actualbudget#1461)
  Goals: Schedule multi month forecasting (actualbudget#1452)
  Fix titlebar transparent background (actualbudget#1460)
  🐛 (import) fix YNAB (and other) importers (actualbudget#1462)
  Fix the mobile footer color (actualbudget#1456)
  🐛 (schedules) fix creating schedules with the same name (actualbudget#1463)
  Move big input component into Input.js, port some of the manager app to TS (actualbudget#1431)
  Bugfix: remove select component inner scrollbar (actualbudget#1458)
  ⬆️ (crdt) upgrade murmurhash (actualbudget#1438)
  ♻️ (crdt) adding more strict typings (actualbudget#1437)
  Add options on Import transactions screen to mark transactions as cleared/uncleared (actualbudget#1451)
  Reports privacy filter (actualbudget#1447)
  Sentence case in menus (actualbudget#1446)
  Goals:  Allow up to to consider spent values (actualbudget#1448)
  Release note README link update (actualbudget#1449)
  fix eye coloring (actualbudget#1450)
  🔖 (23.8.0) stability improvements and new experimental features (actualbudget#1444)
  Update to the latest version of the size compare action (actualbudget#1435)
  🔥  remove unused budgetMonth (actualbudget#1432)
  🐛  fix incorrect state slice usage (actualbudget#1433)
  ...
@shall0pass shall0pass deleted the goalScheduleMultiMonth branch August 18, 2023 16:30
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
References discord discussion starting here:
https://discord.com/channels/937901803608096828/940290142579605514/1133523705063030824

Currently the schedule keyword won't fill any future budget cells if the
category balance already satisfies the schedule. This PR is an attempt
to improve the behavior by allowing budget fills regardless of the
category balance.

This is a drastic rewrite of the schedule keyword. Though I've tried not
to have any regressions, it is possible because of how different the
logic is. I've tested compounding using a simple template, so a small
change in the 'by' keyword was also made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants