Add schedule end date/count field #1899
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 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
|
jfdoming
force-pushed
the
jfdoming/schedule-end-date
branch
from
1625ea4 to
8e7a947
Compare
upcoming-release-notes/1899.md
Outdated
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| category: Bugfix | |||
There was a problem hiding this comment.
I'd tag this as a feature personally!
There was a problem hiding this comment.
Oops totally my bad! I was in bugfixing mode in my mind 😆
| @@ -25,6 +25,9 @@ export interface ScheduleEntity { | |||
| }[]; | |||
| skipWeekend: boolean; | |||
| start: string; | |||
| endMode: 'never' | 'after_n_occurrences' | 'on_date'; | |||
| endOccurrences: number; | |||
There was a problem hiding this comment.
Would be be possible to only store endDate regardless of endMode?
So when the user selects '5 occurrences' we work out the date of the 5th occurrence and store that? Just thinking about saving a column but maybe its not worth the effort.
There was a problem hiding this comment.
I think it makes showing the future UI a lot easier. For example, if you add a schedule and then go edit the rule later, having a dedicated endOccurrences field means we can just use that for hydrating the UI. If we don't have that field, we have to have logic like "generate dates until we pass the end date" which could get pretty inefficient if someone inputs a large # of occurrences. Let me know if this makes sense!
There was a problem hiding this comment.
I'm not too fussed either way, Just a suggestion! But I wouldn't necessarily have thought it would hurt perf too badly even for v large dates, particularly as you only see one at a time.
There was a problem hiding this comment.
Yeah, thanks for the suggestion! I think I should have been more clear: I'm talking about the rules page which displays the same selector:
If we don't store endOccurrences, I think we'd have to recalculate it on this page by manually iterating until we hit endDate, no?
There was a problem hiding this comment.
Yeh you're correct; you'd probably have to iterate through the schedule executions till you reach 'end date'. Even then I dont think it would be too slow.
but given what you've got now works I wouldn't worry too much about it!
| id="repeat_end_dropdown" | ||
| bare | ||
| options={[ | ||
| ['never', 'forever'], |
There was a problem hiding this comment.
Can we use 'indefinitely' here instead?
|
@shall0pass This will need testing against templates when one or both of us get a chance. |
I tried this really quick and it caused the template script to get into some sort of infinite loop. Thats likely not a reason to hold this PR back, but will need fixed soon. |
|
#1917 is for something else. The schedule template processor needs updated to handle the schedules ending |
Got it. I'll try to replicate this later today |
I think this is fixed now! Semi-related to the previous issue, just needed to add a further fallback to the logic |
Cool it looks to be working for me. Ill try some more to see if I can break it, but so far so good! |
jfdoming
force-pushed
the
jfdoming/schedule-end-date
branch
from
3f51914 to
cca80a0
Compare
|
(sorry, rebased on master due to merge conflicts) |
jfdoming
force-pushed
the
jfdoming/schedule-end-date
branch
from
cca80a0 to
cd1451b
Compare
| @@ -90,6 +89,10 @@ export async function goalsSchedule( | |||
| dateConditions, | |||
| monthUtils._parse(next_date), | |||
| ); | |||
| if (!diffDays) { | |||
There was a problem hiding this comment.
I don't think this works correctly. If you have a schedule that has two occurrences in the same month, the function could exit prematurely. I think diffDays needs to be reevaluated before doing this check.
There was a problem hiding this comment.
I don't think this works correctly. If you have a schedule that has two occurrences in the same month, the function could exit prematurely. I think diffDays needs to be reevaluated before doing this check.
Oh, nice catch! This should definitely have been reevaluated. I decided to slightly update how the logic works to be more straightforward/correct in general, let me know your thoughts! Also fixed another bug I found incidentally, that one can move to a separate PR if you like.
There was a problem hiding this comment.
My test budget is crashing with the current build. Here it is if you want to try and debug it. I believe it was working before these most recent changes, so its likely somewhere in there.
template_test_export.zip
There was a problem hiding this comment.
Ah yikes, sorry about that. I'll look into this later today and add what I find to my own manual tests.
There was a problem hiding this comment.
Alright, sorry for the pain here. I was able to run your test budget with no issues in the latest commit.
There was a problem hiding this comment.
I've attached a test budget.
2023-12-05-My-Finances-564f13f.zip
In this test file if you use the "Savings" category that has a schedule called "breakWeekend", the months of December and March aren't right. Those days both fall on a Saturday those months and only budget for one of the two schedules.
I haven't tried it, but I think this only occurs on the 'before' style schedules. What if instead of passing the weekendSolveMode variable of the actual schedule, we force it to 'after' so diffDays won't automatically be evaluated as 0 and will then look for a second schedule for the month? Thanks for all the cleanup you've done!
There was a problem hiding this comment.
Oh, nice catch! This was just one place I missed in my refactor, I believe this should be fixed in that budget now.
trafico-bot
bot
added
⚠️ Changes requested
|
Looks to be working with the template test file! I tried both number of occurrences and end date and it seems to work right. @shall0pass You understand the schedule template a lot better than I do so Ill defer reviewing the code to you. |
|
Fixes #506 as well |
|
(just rebased on master again due to merge conflicts, and fixed one let -> const) |
Awesome, thanks! Are we good to merge, then? (I don't have merge permissions, not sure how this normally works) |
trafico-bot
bot
added
✨ Merged
|
I have two instances of Actual, one updated today in “edge” and the other using the “latest” tag. In addition I had four scheduled payments auto posting this morning. They are set up for “after the weekend” but that was not relevant today as it was a Friday. This is what I observed:
It may be a coincidence but the merge of the above PR seems to be the difference between the behaviour on my two files. I wonder therefore if there may still be a bug here to be hunted down? |
|
Yes, I see the same as @Kidglove57 . Any schedules that were created before this PR are given a "null" for "endMode" which means it doesn't run. Is there a catch for this case in the code? I haven't read through it, so it may already exist, but something like if "isRecurring === true" and "endMode == null" then assume it's indefinite. Sorry again if you've already got this in there but it may not be working properly. |
Yikes, sorry about that. I thought I handled those cases but clearly not. I'll look into it now |
|
Hmm, @Kidglove57 I'm struggling to reproduce this issue. Do you have a redacted sample budget you can share where this occurs? I tried making a schedule with the endMode initially |
|
I’ve got two more scheduled transactions due to post tomorrow (Sunday). They are useful for this trial as, unlike the previous failed posts, these are without “after the weekend” enabled. I’ll see what happens and if they auto post and report back here. The problem with test budgets is that the only way to reproduce this is when real future scheduled transactions are due to post. I will add in a brand new test schedule as a further trial. |
I was currently testing things by altering my system time. So I think I would be able to fast forward things that way. But maybe I'm misunderstanding! At any rate, awaiting your test results 🙏 |
|
Thank you - I will. Bearing in mind @carkom comment, and as an extra check, may I ask if the schedules you were testing were created prior to this release? |
|
Good morning. I am glad to bring good news. Both my two pre existing scheduled transactions and my test transaction posted successfully this morning (Sunday) using the latest Edge. I have no way of assessing why all four scheduled payments due on Friday failed (unless it was the fact that they were marked “after the weekend”). I’ll set up another test transaction today with the “after the weekend” marker and see what happens on Monday. Fingers crossed! |
|
All posted successfully this morning! I’ll keep an eye on this and thank you for your patience. Hopefully my experience on Friday was a one off! |
Glad to hear it worked! It is definitely strange that it didn't work on Friday though... Maybe something with "after the weekend" on a Friday. I'll keep following this thread just in case! |
* Add "end" field with date/count options * Use "end" field to generate schedule * Show "end" field in recurring description * Disable weekend before/after picker when not enabled * Add release notes * Fix failing typechecks * Add some description tests * PR feedback * 'Features', not 'Feature' * Fix goal templates infinite loop * Empty commit to bump ci * Fix bug where schedule templates in the past would apply incorrectly For example, if you had a schedule which started in November 2023 for 1.00, and you applied the schedule in October 2023, then you would end up with a value of 0.50 applied in October. * Fix handling of schedules with an end date This commit also includes a refactor of the skip-weekend logic: rather than referring only to dates with skipped weekends (which requires checking whether the "next date" request worked correctly), we track a "base date" which is the previous value of the schedule according to the rrule, excluding any weekend-skipping. This lets us use `addDays(baseDate, 1)` to get the next occurrence, regardless of the weekend behaviour. Doing things this way ensures that the loop will always make progress. * Only compute skipped weekend if weekend skips were requested * Fix typo in iterate-schedule-occurrences code We should be using `nextBaseDate` to derive the next base date, not `nextDate`; this is because we want the base date to be guaranteed to make progress in each loop iteration, so we can finish in at most 30 iterations without duplicate base dates. * Use const * Revert const -> let for one mutable variable
Resurrecting #407! This PR adds an end date/max occurrences field to the schedule creator. Closes #1568 and #241.
Screenshots
Current popup:
New popup: