Add percentage adjustments to schedule templates (#4098) #4257
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 |
MattFaz
changed the title
WalkthroughThe pull request introduces modifications to the budget schedule templates, focusing on enhancing the grammar rules and parsing logic to accommodate percentage-based adjustments for recurring transactions. Key changes include the addition of a new Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4257.mdis excluded by!**/*.md
📒 Files selected for processing (3)
packages/loot-core/src/server/budget/goal-template.pegjs(3 hunks)packages/loot-core/src/server/budget/goalsSchedule.ts(1 hunks)packages/loot-core/src/server/budget/types/templates.d.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/budget/goal-template.pegjs (2)
Learnt from: youngcw
PR: actualbudget/actual#3754
File: packages/loot-core/src/server/budget/goal-template.pegjs:38-43
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/server/budget/goal-template.pegjs`, within the `limit` rule, the optional space `_?` is always placed before the `hold` option, ensuring consistency.
Learnt from: youngcw
PR: actualbudget/actual#3754
File: packages/loot-core/src/server/budget/goal-template.pegjs:38-43
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Validations for limit rules, such as checking start date validity and ensuring amounts are positive, are performed during template processing, not in the parser defined in `goal-template.pegjs`.
🔇 Additional comments (4)
packages/loot-core/src/server/budget/types/templates.d.ts (1)
48-48: LGTM! The type definition aligns with the feature requirements.The optional
adjustmentproperty is correctly typed asnumberand maintains backward compatibility.packages/loot-core/src/server/budget/goal-template.pegjs (3)
31-37: LGTM! Clean implementation of the adjustment modifiers.The implementation correctly:
- Parses both increase/decrease operations
- Handles the percentage value
- Returns the adjustment with the correct sign (+/- based on increase/decrease)
21-22: Add test cases for the schedule rule.While the implementation looks correct, it would be beneficial to add test cases to verify:
- Schedule templates without modifiers still work
- Various percentage values (including decimals)
- Error cases (invalid modifiers)
Would you like me to help create a comprehensive test suite for the grammar changes?
90-100: Verify the regex pattern for schedule names.The complex regex pattern for
rawScheduleNameneeds validation to ensure it correctly handles all edge cases:
- Names with multiple spaces
- Names containing special characters
- Names followed by valid/invalid modifiers
✅ Verification successful
Schedule name pattern is correctly implemented and well-tested
The regex pattern for
rawScheduleNameis verified to handle all required cases:
- Names with multiple spaces (e.g., "Test Schedule", "Mock Schedule 1")
- Special characters and numbers
- Proper handling of [increase]/[decrease] modifiers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Create a test file with various schedule names cat << 'EOF' > test_cases.txt Car Insurance [increase 10%] Rent [decrease 5%] Complex Name With Spaces [increase 15%] Name.With.Dots [decrease 7%] Name-With-Dashes [increase 3%] Invalid[increase 10%] Invalid [invalid 10%] EOF # Use ast-grep to verify the grammar can parse these cases ast-grep --pattern 'schedule $name [$op $value%]'Length of output: 57
Script:
#!/bin/bash # Search for test files related to goal templates echo "=== Test Files ===" fd -e test.js -e test.ts -e spec.js -e spec.ts | rg -i "goal.*template|template.*goal" echo -e "\n=== Usage in Tests ===" rg -l "(\[increase|\[decrease)" $(fd -e test.js -e test.ts -e spec.js -e spec.ts) echo -e "\n=== Documentation ===" fd -e md | rg -i "goal.*template|template.*goal" echo -e "\n=== Real Usage Examples ===" # Look for actual schedule patterns in the codebase rg "(\[increase|\[decrease)" --type ts --type js -A 1 -B 1Length of output: 3368
Script:
#!/bin/bash echo "=== Goal Schedule Tests ===" cat packages/loot-core/src/server/budget/goalsSchedule.test.ts || true echo -e "\n=== Template Notes Tests ===" cat packages/loot-core/src/server/budget/template-notes.test.ts || true echo -e "\n=== Implementation Files ===" fd . packages/loot-core/src/server/budget/ -t f --exclude "*.pegjs" --exclude "*.test.ts" echo -e "\n=== Grammar Usage ===" rg "rawScheduleName" packages/loot-core/src/server/budget/Length of output: 12097
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/server/budget/goalsSchedule.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/loot-core/src/server/budget/goalsSchedule.ts
[error] 41-41: Replace Invalid·adjustment·percentage·for·schedule·${template[ll].name} with ⏎··········Invalid·adjustment·percentage·for·schedule·${template[ll].name},⏎········
(prettier/prettier)
🪛 GitHub Check: lint
packages/loot-core/src/server/budget/goalsSchedule.ts
[failure] 41-41:
Replace Invalid·adjustment·percentage·for·schedule·${template[ll].name} with ⏎··········Invalid·adjustment·percentage·for·schedule·${template[ll].name},⏎········
🪛 GitHub Actions: Test
packages/loot-core/src/server/budget/goalsSchedule.ts
[error] 41-41: Prettier formatting error: Incorrect template literal formatting
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/loot-core/src/server/budget/goalsSchedule.ts (2)
32-36: LGTM! Good use of mutable declaration.The change from
consttoletis appropriate here as it enables the new adjustment feature while maintaining the existing calculation logic.
38-46: Consider additional edge cases in adjustment calculation.While the validation and calculation logic is sound, consider these edge cases:
- When
scheduleAmountis 0, any adjustment will still result in 0- Very small amounts might round to 0 after adjustment
Would you like me to add validation for these edge cases?
🧰 Tools
🪛 ESLint
[error] 41-41: Replace
Invalid·adjustment·percentage·for·schedule·${template[ll].name}with⏎··········Invalid·adjustment·percentage·for·schedule·${template[ll].name},⏎········(prettier/prettier)
🪛 GitHub Check: lint
[failure] 41-41:
ReplaceInvalid·adjustment·percentage·for·schedule·${template[ll].name}with⏎··········Invalid·adjustment·percentage·for·schedule·${template[ll].name},⏎········🪛 GitHub Actions: Test
[error] 41-41: Prettier formatting error: Incorrect template literal formatting
|
Are the brackets necessary? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/loot-core/src/server/budget/goal-template.pegjs (2)
31-37: Consider adding grammar documentation.The modifiers implementation is clean and well-structured. Consider adding a comment explaining the syntax and valid ranges for future maintainers.
+// Modifiers allow percentage adjustments to schedule amounts +// Syntax: [increase X%] or [decrease X%] +// Valid range: -100% to 1000% modifiers = _ '[' modifier:modifier ']' { return modifier }
90-100: Enhance documentation for the complex name parsing logic.While the implementation is correct, the complex lookahead logic would benefit from more detailed documentation explaining the edge cases it handles.
-// Match schedule name including spaces up until we see a [, looking ahead to make sure it's followed by increase/decrease +// Parses schedule names that may contain spaces, stopping at modifier brackets. +// Examples: +// "Car Insurance [increase 10%]" -> name: "Car Insurance" +// "Home Rent [decrease 5%]" -> name: "Home Rent" +// "No Modifier" -> name: "No Modifier" +// The lookahead ensures spaces are included in the name only if they're not +// followed by a modifier bracket, preventing incorrect parsing of modifiers. rawScheduleName = $(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
-
packages/loot-core/src/server/budget/goal-template.pegjs(3 hunks) -
packages/loot-core/src/server/budget/goalsSchedule.ts(1 hunks) -
packages/loot-core/src/server/budget/template-notes.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/budget/goalsSchedule.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/budget/goal-template.pegjs (2)
Learnt from: youngcw
PR: actualbudget/actual#3754
File: packages/loot-core/src/server/budget/goal-template.pegjs:38-43
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/server/budget/goal-template.pegjs`, within the `limit` rule, the optional space `_?` is always placed before the `hold` option, ensuring consistency.
Learnt from: youngcw
PR: actualbudget/actual#3754
File: packages/loot-core/src/server/budget/goal-template.pegjs:38-43
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Validations for limit rules, such as checking start date validity and ensuring amounts are positive, are performed during template processing, not in the parser defined in `goal-template.pegjs`.
🪛 ESLint
packages/loot-core/src/server/budget/template-notes.ts
[error] 46-46: Replace ${name}:·${template.line}${template.error·?·'\nError:·'·+·template.error·:·''} with ⏎··········${name}:·${template.line}${template.error·?·'\nError:·'·+·template.error·:·''},⏎········
(prettier/prettier)
[error] 95-95: Replace parsedTemplate.type·===·'schedule'·&&·parsedTemplate.adjustment·!==·undefined with ⏎··········parsedTemplate.type·===·'schedule'·&&⏎··········parsedTemplate.adjustment·!==·undefined⏎········
(prettier/prettier)
[error] 96-96: Replace parsedTemplate.adjustment·<=·-100·||·parsedTemplate.adjustment·>·1000 with ⏎············parsedTemplate.adjustment·<=·-100·||⏎············parsedTemplate.adjustment·>·1000⏎··········
(prettier/prettier)
[error] 97-97: Replace Invalid·adjustment·percentage·(${parsedTemplate.adjustment}%).·Must·be·between·-100%·and·1000% with ⏎··············Invalid·adjustment·percentage·(${parsedTemplate.adjustment}%).·Must·be·between·-100%·and·1000%,⏎············
(prettier/prettier)
[error] 124-124: Insert ⏎
(prettier/prettier)
🪛 GitHub Check: lint
packages/loot-core/src/server/budget/template-notes.ts
[failure] 46-46:
Replace ${name}:·${template.line}${template.error·?·'\nError:·'·+·template.error·:·''} with ⏎··········${name}:·${template.line}${template.error·?·'\nError:·'·+·template.error·:·''},⏎········
[failure] 95-95:
Replace parsedTemplate.type·===·'schedule'·&&·parsedTemplate.adjustment·!==·undefined with ⏎··········parsedTemplate.type·===·'schedule'·&&⏎··········parsedTemplate.adjustment·!==·undefined⏎········
[failure] 96-96:
Replace parsedTemplate.adjustment·<=·-100·||·parsedTemplate.adjustment·>·1000 with ⏎············parsedTemplate.adjustment·<=·-100·||⏎············parsedTemplate.adjustment·>·1000⏎··········
[failure] 97-97:
Replace Invalid·adjustment·percentage·(${parsedTemplate.adjustment}%).·Must·be·between·-100%·and·1000% with ⏎··············Invalid·adjustment·percentage·(${parsedTemplate.adjustment}%).·Must·be·between·-100%·and·1000%,⏎············
[failure] 124-124:
Insert ⏎
🪛 GitHub Actions: Test
packages/loot-core/src/server/budget/template-notes.ts
[error] 46-46: Prettier formatting error: Incorrect template literal formatting
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/loot-core/src/server/budget/goal-template.pegjs (2)
21-22: LGTM! Well-structured schedule rule update.The schedule rule has been cleanly extended to support modifiers while maintaining backward compatibility.
76-77: LGTM! Clear whitespace handling.The distinction between optional and mandatory whitespace improves the grammar's robustness.
MattFaz
changed the title
It could be done without the square brackets however they are helpful for a few reasons:
I could look at removing the bracket syntax however it would make it more challenging to determine where the schedule name ends and the modifier begins. |
MattFaz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/loot-core/src/server/budget/template-notes.test.ts (1)
230-261: Add test cases for valid adjustment scenarios.The error cases are well covered, but consider adding test cases for:
- Valid adjustments at boundary values (-100%, 1000%)
- Valid adjustments within range (e.g., [increase 50%], [decrease 25%])
Here's a suggested test case structure:
}, + { + description: 'Accepts valid schedule adjustments at boundaries', + mockTemplateNotes: [ + { + id: 'cat1', + name: 'Category 1', + note: '#template schedule Mock Schedule 1 [increase 1000%]', + }, + { + id: 'cat2', + name: 'Category 2', + note: '#template schedule Mock Schedule 1 [decrease 100%]', + }, + ], + mockSchedules: mockSchedules(), + expected: { + type: 'message', + message: 'All templates passed! 🎉', + }, + }, + { + description: 'Accepts valid schedule adjustments within range', + mockTemplateNotes: [ + { + id: 'cat1', + name: 'Category 1', + note: '#template schedule Mock Schedule 1 [increase 50%]', + }, + { + id: 'cat2', + name: 'Category 2', + note: '#template schedule Mock Schedule 1 [decrease 25%]', + }, + ], + mockSchedules: mockSchedules(), + expected: { + type: 'message', + message: 'All templates passed! 🎉', + }, + },packages/loot-core/src/server/budget/template-notes.ts (2)
46-51: Improve error handling robustness.Consider these enhancements:
- Use optional chaining for safer error checking
- Make the adjustment error check more explicit
- if (template.error && template.error.includes('adjustment')) { - errors.push(`${name}: ${template.line}\nError: ${template.error}`); + if (template.error?.includes('Invalid adjustment percentage')) { + errors.push(`${name}: ${template.line}\nError: ${template.error}`);🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
99-113: Extract validation logic to a separate function.Consider moving the validation logic to a dedicated function and using constants for the bounds.
+const MIN_ADJUSTMENT_PERCENTAGE = -100; +const MAX_ADJUSTMENT_PERCENTAGE = 1000; + +function validateScheduleAdjustment(adjustment: number): void { + if (adjustment <= MIN_ADJUSTMENT_PERCENTAGE || adjustment > MAX_ADJUSTMENT_PERCENTAGE) { + throw new Error( + `Invalid adjustment percentage (${adjustment}%). Must be between ${MIN_ADJUSTMENT_PERCENTAGE}% and ${MAX_ADJUSTMENT_PERCENTAGE}%`, + ); + } +} async function getCategoriesWithTemplates(): Promise<CategoryWithTemplates[]> { // ... try { const parsedTemplate: Template = parse(trimmedLine); - if ( - parsedTemplate.type === 'schedule' && - parsedTemplate.adjustment !== undefined - ) { - if ( - parsedTemplate.adjustment <= -100 || - parsedTemplate.adjustment > 1000 - ) { - throw new Error( - `Invalid adjustment percentage (${parsedTemplate.adjustment}%). Must be between -100% and 1000%`, - ); - } + if (parsedTemplate.type === 'schedule' && parsedTemplate.adjustment !== undefined) { + validateScheduleAdjustment(parsedTemplate.adjustment); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loot-core/src/server/budget/template-notes.test.ts(1 hunks)packages/loot-core/src/server/budget/template-notes.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loot-core/src/server/budget/template-notes.ts
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Mostly I would like to avoid special syntax that is unique to just one kind of template outside of the semi-human readable word style. Its not the end of the world if you aren't able to do it without the brackets. |
Addresses feature request #4098
This adds the ability to increase or decrease schedule amounts by a percentage using square bracket modifiers. The syntax allows adjusting schedule amounts without modifying the original schedule.
Example usage:
#template schedule Car Insurance [increase 10%]#template schedule Rent [decrease 5%]Detailed Explanation:
Yearly expenses such as insurance, property rates, etc. increase year on year. You often don't know the new amount until ~1-2 months before it's due. This creates a budget crunch - if your $1,000 insurance jumps 20% ($1,200), you need to make up that extra $200 in just a month or two.
This adds percentage adjustments to templates, letting you gradually save the expected increase throughout the year:
#template schedule Car Insurance [increase 20%].By proactively budgeting a percentage change for these yearly increases, you avoid last-minute scrambling when renewal notices arrive with higher amounts.