Allow early payment of scheduled transactions #4433
Conversation
✅ 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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
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 |
actual-github-bot
bot
changed the title
WalkthroughThe changes add a new function, Possibly related PRs
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
🧹 Nitpick comments (1)
packages/loot-core/src/shared/schedules.ts (1)
37-39: LGTM! Consider enhancing the comments.The new date filtering logic correctly implements early payment support. The comments are helpful but could be more precise.
- // A transaction belong to a schedule occurence if + // A transaction belongs to a schedule occurrence if // 1. the transaction is associated with the schedule AND - // 2. the transaction's date is between this occurence and the previous one. + // 2. the transaction's date is between this occurrence and the previous one (inclusive).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4433.mdis excluded by!**/*.md
📒 Files selected for processing (1)
packages/loot-core/src/shared/schedules.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/loot-core/src/shared/schedules.ts (2)
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/shared/schedules.ts:278-311
Timestamp: 2025-01-16T14:30:20.991Z
Learning: The `getNextDate` function in `packages/loot-core/src/shared/schedules.ts` cannot return null because the `Condition` class validates date values during construction and recurring schedules always have at least one occurrence (either future or past).
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/client/data-hooks/transactions.ts:173-181
Timestamp: 2025-01-16T14:29:03.337Z
Learning: The `getNextDate` function in `packages/loot-core/src/shared/schedules.ts` is designed to always return a date value, either directly from the condition value or after processing schedule occurrences. It does not return null.
| export function getPreviousDate(dateCond): string | null { | ||
| const cond = new Condition(dateCond.op, 'date', dateCond.value, null); | ||
| const value = cond.getValue(); | ||
|
|
||
| if (value.type === 'recur') { | ||
| const previous = value.schedule | ||
| .occurrences({ reverse: true, end: monthUtils.currentDay() }) | ||
| .toArray()[0]; | ||
| if (previous) { | ||
| const date = value.schedule.data.skipWeekend | ||
| ? getDateWithSkippedWeekend( | ||
| previous.date, | ||
| value.schedule.data.weekendSolve, | ||
| ) | ||
| : previous.date; | ||
| return monthUtils.dayFromDate(date); | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align getPreviousDate with getNextDate for consistency.
The new function should match the behavior and parameters of getNextDate for consistency:
- Handle 'date' type values like
getNextDate. - Add
startparameter for custom start dates. - Add
noSkipWeekendparameter for consistency. - Return type should be
stringnotstring | nullas per learnings.
-export function getPreviousDate(dateCond): string | null {
+export function getPreviousDate(
+ dateCond,
+ start = new Date(monthUtils.currentDay()),
+ noSkipWeekend = false,
+): string {
const cond = new Condition(dateCond.op, 'date', dateCond.value, null);
const value = cond.getValue();
+ if (value.type === 'date') {
+ return value.date;
+ }
if (value.type === 'recur') {
const previous = value.schedule
- .occurrences({ reverse: true, end: monthUtils.currentDay() })
+ .occurrences({ reverse: true, end: start })
.toArray()[0];
if (previous) {
- const date = value.schedule.data.skipWeekend
+ let date = previous.date;
+ if (value.schedule.data.skipWeekend && !noSkipWeekend)
+ {
+ date = getDateWithSkippedWeekend(
+ date,
+ value.schedule.data.weekendSolve,
+ );
+ }
- ? getDateWithSkippedWeekend(
- previous.date,
- value.schedule.data.weekendSolve,
- )
- : previous.date;
return monthUtils.dayFromDate(date);
}
}
- return null;
+ return value.date;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getPreviousDate(dateCond): string | null { | |
| const cond = new Condition(dateCond.op, 'date', dateCond.value, null); | |
| const value = cond.getValue(); | |
| if (value.type === 'recur') { | |
| const previous = value.schedule | |
| .occurrences({ reverse: true, end: monthUtils.currentDay() }) | |
| .toArray()[0]; | |
| if (previous) { | |
| const date = value.schedule.data.skipWeekend | |
| ? getDateWithSkippedWeekend( | |
| previous.date, | |
| value.schedule.data.weekendSolve, | |
| ) | |
| : previous.date; | |
| return monthUtils.dayFromDate(date); | |
| } | |
| } | |
| return null; | |
| } | |
| export function getPreviousDate( | |
| dateCond, | |
| start = new Date(monthUtils.currentDay()), | |
| noSkipWeekend = false, | |
| ): string { | |
| const cond = new Condition(dateCond.op, 'date', dateCond.value, null); | |
| const value = cond.getValue(); | |
| if (value.type === 'date') { | |
| return value.date; | |
| } | |
| if (value.type === 'recur') { | |
| const previous = value.schedule | |
| .occurrences({ reverse: true, end: start }) | |
| .toArray()[0]; | |
| if (previous) { | |
| let date = previous.date; | |
| if (value.schedule.data.skipWeekend && !noSkipWeekend) | |
| { | |
| date = getDateWithSkippedWeekend( | |
| date, | |
| value.schedule.data.weekendSolve, | |
| ); | |
| } | |
| return monthUtils.dayFromDate(date); | |
| } | |
| } | |
| return value.date; | |
| } |
How about late payments? We also need to take that into consideration. With the above criteria, wouldn't the late payments count towards the next schedule date instead of the current |
Good point, this was an oversight. In that case, a natural way to do it would be to define a schedule occurrence as paid like suggested in this PR unless there is a missed schedule occurrence (in which case the old definition would apply). This means that missed occurrences would always be paid first, unless manually skipped by the user. Given how schedule currently work, this might require some rather important modifications. I will see what I can do. |
Fixes #1957
Currently, a scheduled transaction is considered "Paid" iff there is an actual transaction such that
This makes it impossible for a schedule transaction to be paid in advance, causing the behaviour described in the issue.
This PR proposes to solve this by changing the conditions under which a scheduled transaction is considered paid. More precisely, a scheduled transaction will now be paid iff there is an actual transaction such that