-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Reset Hold and Hold For Next Month api #3140
Add Reset Hold and Hold For Next Month api #3140
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 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 |
867c26b
to
0028cf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Co-authored-by: Matiss Janis Aboltins <[email protected]>
Co-authored-by: Matiss Janis Aboltins <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on my end.
Looking for another maintainers review since this impacts the API (which we cannot easily change without introducing a breaking change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! LGTM
@@ -63,6 +63,13 @@ export interface ApiHandlers { | |||
flag: boolean; | |||
}) => Promise<void>; | |||
|
|||
'api/hold-budget-for-next-month': (arg: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought I had is it might be good to call this budget-hold-for-next-month
(and use the same pattern for the other endpoint) vs the current hold-budget-for-next-month
. My thought process is that the former sounds more like you're holding an amount from the budget for next month (the additive nature of the feature), whereas the latter is holding the entire budget. Doing the former also matches previous endpoints in the file (e.g. budget-set-amount
, budget-set-carryover
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Changing it
…odriguestiago0/actual into add_reset_hold_and_hold_for_next_month
b06880f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This also addressed the feature request in #1982 |
Docs