Fix yearly schedule templates #3511
Conversation
actual-github-bot
bot
changed the title
WalkthroughThis pull request adds a new test case to the Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
JukeboxRhino
force-pushed
the
fix/yearly-schedule-templates
branch
from
5fc4561
to
5eb6d4a
Compare
Bundle Stats — desktop-clientHey 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
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey 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
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/server/budget/goals/goalsSchedule.test.ts (2)
86-148: Good addition of a test case for yearly recurring schedules.This test case effectively verifies the behavior of the
goalsSchedulefunction for yearly recurring schedules when the balance meets the target. The setup and assertions are clear and align with the expected behavior.Consider the following improvements to enhance test coverage:
- Add test cases for different months within the yearly cycle to ensure consistent behavior throughout the year.
- Include edge cases such as:
- The first month of the cycle (August in this case)
- The last month before the next cycle (July)
- A month where the balance is slightly below or above the target
These additional tests will help ensure the function behaves correctly across various scenarios.
Consider updating the test description to be more specific:
- it('should return correct budget when yearly recurring schedule set and balance is greater than target', async () => { + it('should return 1/12 of yearly budget when balance meets the target in a non-starting month', async () => {This description more accurately reflects the specific scenario being tested.
86-148: Overall assessment: Good progress, but consider expanding test coverageThe new test case aligns well with the PR objectives, addressing the issue of yearly schedule templates when the category is fully funded. It verifies that the budgeting is correctly distributed (1/12 of the amount) across the year instead of allocating the full yearly amount in a single month.
To further strengthen the implementation and ensure robustness:
- Consider adding more test cases as suggested earlier to cover various scenarios throughout the yearly cycle.
- Ensure that the
goalsSchedulefunction itself (not visible in this file) correctly handles all edge cases and variations of yearly schedules.- If possible, include a test case that demonstrates the previous incorrect behavior (full yearly amount allocated in a single month) to clearly show the fix's impact.
These additions would provide more comprehensive coverage and increase confidence in the fix for yearly schedule templates.
packages/loot-core/src/server/budget/goals/goalsSchedule.ts (1)
169-173: LGTM! Consider adding a comment for clarity.The changes correctly address the issue with yearly schedule templates by converting yearly amounts to monthly contributions. This fix aligns well with the PR objectives.
Consider adding a brief comment explaining the reasoning behind dividing by 12 for yearly frequencies. This would improve code readability and maintainability. For example:
let monthlyAmount = t[ll].target / t[ll].target_interval; if (t[ll].target_frequency === 'yearly') { + // Convert yearly amount to monthly by dividing by 12 monthlyAmount /= 12; } total += monthlyAmount;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3511.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/loot-core/src/server/budget/goals/goalsSchedule.test.ts (1 hunks)
- packages/loot-core/src/server/budget/goals/goalsSchedule.ts (1 hunks)
🔇 Additional comments (2)
packages/loot-core/src/server/budget/goals/goalsSchedule.ts (2)
Line range hint
1-253: Summary of changes and impactThe modifications in this file are focused and effective:
- The
getSinkingBaseContributionTotalfunction has been updated to handle yearly schedules correctly.- These changes address the PR objective of fixing yearly schedule templates when the associated category is fully funded.
- The fix ensures that yearly amounts are properly distributed as monthly contributions.
- No other functions were modified, maintaining the overall structure and functionality of the file.
The changes appear to be minimal, targeted, and should resolve the reported issue without introducing new complexities or dependencies.
169-173: Verify the impact on thegoalsSchedulefunctionThe changes in
getSinkingBaseContributionTotalcorrectly address the issue with yearly schedule templates. This fix ensures that thetotalSinkingBaseContributioncalculated in thegoalsSchedulefunction will now be more accurately distributed for yearly schedules.To confirm the correct implementation and usage, let's verify the
goalsSchedulefunction:This will help ensure that the
goalsSchedulefunction correctly utilizes the updatedgetSinkingBaseContributionTotalfor both yearly and non-yearly frequencies.✅ Verification successful
Impact on the
goalsScheduleFunction VerifiedThe updates to
getSinkingBaseContributionTotalcorrectly integrate with thegoalsSchedulefunction, ensuring that yearly schedule templates now distribute the budget accurately across all months.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of getSinkingBaseContributionTotal in goalsSchedule # Test: Check if goalsSchedule uses getSinkingBaseContributionTotal rg --type typescript -A 10 'getSinkingBaseContributionTotal' packages/loot-core/src/server/budget/goals/goalsSchedule.ts # Test: Verify if goalsSchedule handles both yearly and non-yearly frequencies rg --type typescript -A 20 'goalsSchedule' packages/loot-core/src/server/budget/goals/goalsSchedule.tsLength of output: 293
Script:
#!/bin/bash # Description: Verify the usage of getSinkingBaseContributionTotal in goalsSchedule without specifying file type # Test: Check if goalsSchedule uses getSinkingBaseContributionTotal rg 'getSinkingBaseContributionTotal' packages/loot-core/src/server/budget/goals/goalsSchedule.ts --context 10 # Test: Verify if goalsSchedule handles both yearly and non-yearly frequencies rg 'goalsSchedule' packages/loot-core/src/server/budget/goals/goalsSchedule.ts --context 20Length of output: 2519
JukeboxRhino
changed the title
JukeboxRhino
force-pushed
the
fix/yearly-schedule-templates
branch
2 times, most recently
from
d7d99f5
to
bb2e08e
Compare
JukeboxRhino
force-pushed
the
fix/yearly-schedule-templates
branch
from
bb2e08e
to
97932c3
Compare
|
As far as I can tell this works fine, and doesn't break anything else. We will have to wait and see if any complaints come up about issues. |
Current behavior of template goals set to a schedule with frequency
yearlyare broken once the category is fully funded. This means budgeting into future months does not work as expected, in this case the full yearly amount is budgeted in the month following the transaction (instead of 1/12 the amount). The function that handles calculating the base savings amount has been updated to take yearly schedules into account.Example of previous broken behavior:
The same budget file with the fix applied and templates re-ran:
