fix first occurrence of some schedules moved after the weekend not showing in preview #4256
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. |
matt-fidd
changed the title
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 No assets were 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 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
|
WalkthroughThe pull request modifies the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
|
Do you have steps to test this? |
matt-fidd
force-pushed
the
schedules-fix
branch
from
456edd8 to
ab75b1e
Compare
Yep, sorry. If you upload this to edge you'll see the schedule preview doesn't show in the account, but it does in the preview build. |
There was a problem hiding this comment.
This almost seems to work, but if I set my computer's time to Tuesday and check the linked test, the date on the schedule is 'March 3' even though it's missed and should show 'Feb 2' per the schedules page. I'd imagine we need to take the next date and "un-move" it after the weekend, somehow (or just get the base next date directly).
Separately, any chance you could write some unit tests for this file? The logic is looking pretty complex and I think it would help give more confidence about edge cases like this
I think this is a bug outside of this PR. I can see the same behaviour on edge where if the date is moved by 2 days because it hits a weekend that date is skipped entirely. Screen.Recording.2025-02-02.at.17.45.50.mov
Yes, good idea. I don't know if I'll be able to before release but I'll look into how's best. |
matt-fidd
force-pushed
the
schedules-fix
branch
from
ab75b1e to
ebc079a
Compare
|
I'll look into the bug above when I get a chance but for now I've altered the logic here to always include the next date, regardless, and then calculate the others. That should alleviate all bugs where the next date is missed because of getNextDate weirdness. |
![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 comments (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
177-195: Add unit tests for weekend schedule scenarios.As suggested in the PR comments, we should add unit tests to cover weekend schedule scenarios, especially edge cases where schedules are moved after weekends.
Would you like me to help generate unit tests that cover:
- Regular weekday schedules
- Schedules that fall on weekends
- Edge cases for schedules moved after weekends
🧰 Tools
🪛 ESLint
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date(prettier/prettier)
🪛 GitHub Check: lint
[failure] 177-177:
Replace·schedule.next_date·withschedule.next_date🪛 GitHub Actions: Test
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date
🧹 Nitpick comments (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
177-177: Fix formatting issue.Remove extra spaces around
schedule.next_date.- const dates: string[] = [ schedule.next_date ]; + const dates: string[] = [schedule.next_date];🧰 Tools
🪛 ESLint
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date(prettier/prettier)
🪛 GitHub Check: lint
[failure] 177-177:
Replace·schedule.next_date·withschedule.next_date🪛 GitHub Actions: Test
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4256.mdis excluded by!**/*.md
📒 Files selected for processing (1)
packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4166
File: packages/loot-core/src/client/data-hooks/transactions.ts:200-200
Timestamp: 2025-01-16T14:29:13.188Z
Learning: In the scheduled transactions implementation within `packages/loot-core/src/client/data-hooks/transactions.ts`, the `upcoming` flag is set based on `schedules.length > 0` to act as an override, where the first occurrence gets `false` and subsequent occurrences get `true`. This is intentional and should not be changed to date-based comparison.
🪛 ESLint
packages/loot-core/src/client/data-hooks/transactions.ts
[error] 177-177: Replace ·schedule.next_date· with schedule.next_date
(prettier/prettier)
🪛 GitHub Check: lint
packages/loot-core/src/client/data-hooks/transactions.ts
[failure] 177-177:
Replace ·schedule.next_date· with schedule.next_date
🪛 GitHub Actions: Test
packages/loot-core/src/client/data-hooks/transactions.ts
[error] 177-177: Replace ·schedule.next_date· with schedule.next_date
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
177-195: Verify weekend schedule handling.The fix ensures first occurrences are included by initializing
dateswithschedule.next_date. However, based on the PR comments about dates being incorrectly moved after weekends (showing March 3 instead of Feb 2), we should verify the weekend adjustment logic.Let's verify the weekend adjustment behavior:
✅ Verification successful
Weekend Schedule Handling Verification Confirmed
The weekend adjustment behavior is implemented through the centralized logic in
getDateWithSkippedWeekend(inpackages/loot-core/src/shared/schedules.ts), which correctly handles solving weekend dates based on theweekendSolveModeconfiguration. The schedules and tests (as seen in various files likegoalsSchedule.test.tsand settings mocks) consistently reference and use these configurations. In the transactions hook, initializingdateswithschedule.next_dateensures that the first occurrence (whether adjusted or not) is preserved, and subsequent dates are computed usinggetNextDatethat relies on the existing weekend adjustment logic.
- The weekend adjustment implementation in
getDateWithSkippedWeekendproperly returns the next Monday when using'after'or the previous Friday for'before'modes.- Related tests and configurations in the codebase (across schedules, reports, and mocks) confirm that weekend handling is in place and working as intended.
- The logic in
transactions.tscorrectly preserves the starting date and computes subsequent dates without inadvertently shifting them.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for weekend adjustment logic in schedules # Expected: Find code that handles weekend date adjustments # Search for weekend-related date adjustments rg -A 5 -i "weekend|saturday|sunday" --type ts # Search for getNextDate implementations ast-grep --pattern 'function getNextDate($_) { $$$ }'Length of output: 23416
🧰 Tools
🪛 ESLint
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date(prettier/prettier)
🪛 GitHub Check: lint
[failure] 177-177:
Replace·schedule.next_date·withschedule.next_date🪛 GitHub Actions: Test
[error] 177-177: Replace
·schedule.next_date·withschedule.next_date
matt-fidd
force-pushed
the
schedules-fix
branch
from
ebc079a to
f99f76c
Compare
|
/update-vrt |
matt-fidd
force-pushed
the
schedules-fix
branch
3 times, most recently
from
5d4fc0c to
e3a273d
Compare
matt-fidd
force-pushed
the
schedules-fix
branch
from
e3a273d to
7cbdbfe
Compare
|
@jfdoming VRT was failing because non-recurring schedules were added twice, fixed now. |
Testing:
If you upload this to edge you'll see the schedule preview doesn't show in the account, but it does in the preview build.
schedules_test.zip