Skip to content
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 recurring expense functionality #263

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

trandall2
Copy link

@trandall2 trandall2 commented Nov 6, 2024

What

  • Closes issue Recurring expenses #5
    • An expense can be created with a recurrence/repeating rule, and subsequent expenses will be created
    • Deleting the most recent expense causes no further repeating expenses to be created
    • Changing the most recent expense to no longer repeat causes no further repeating expenses to be created
    • Changing the recurrence rule of the most recent expense causes all subsequent expenses to adopt the same recurrence

Particular shortcomings to note

  • Creating a monthly expense on the 29th, 30th, or 31st will not cause the expense to consistently be created for that date
    • If an expense is created on any of these 3 dates, it will eventually adopt the largest date available in the next month if the target date is not available
    • Ex 1
      • A monthly expense is initially created on January 31st, 2024
      • The next 3 expenses will be created on: February 29th, March 29th, and April 29th
    • Ex 2
      • A monthly expense is initially created on March 31st, 2024
      • The next 3 expenses will be created on: April 30th, May 30th, and June 30th
    • Ex 3
      • A monthly expense is initially created on July 31st, 2024
      • The next 3 expenses will be created on: August 31st, September 30th, and October 30th

Acknowledged decisions

  • Only supporting recurrence rules (daily, weekly, and "monthly" -- excluding the 29th, 30th, and 31st)
    • This seems like a good starting place for the feature
  • Not indefinitely linking Expenses to their future related Expenses
    • It's still an option to add this linking later. But not adding it now keeps the model more flexible
  • Creating repeating Expenses when a user loads all the Expenses from a group
    • I took inspiration from this earlier PR which chose to do something similar. I think that it's possible/straightforward to add a job to perform this task. But this method was very lightweight and doesn't preclude a later change.

Potential Future improvements

  • Add usage of the rrule library to hopefully avoid the poor end-of-month recurrence behavior mentioned above
    • I think it would be best to add this library before adding more RecurrenceRules (ex. bi-weekly, etc)
  • Add a popup to the Expense page that notifies users about particular deletion behavior:
    • If they are deleting an Expense whose RecurringExpenseLink is pending to create the next Expense, then no further repeating Expenses will be created for that Expense
    • If they are deleting an Expense whose RecurringExpenseLink has already created the next Expense, then only the target Expense will be deleted

@trandall2
Copy link
Author

Test feature in action:
image

@trandall2 trandall2 marked this pull request as ready for review November 6, 2024 05:48
@trandall2
Copy link
Author

image

@Maxco10
Copy link
Contributor

Maxco10 commented Nov 11, 2024

@trandall2 You should add the new labels for all languages not only for es-US.

Copy link

@tasken tasken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding spanish translation strings

messages/es.json Outdated
@@ -136,6 +136,15 @@
"label": "Recibido por",
"description": "Seleccione el participante que recibió los ingresos."
},
"recurrenceRule": {
"label": "Expense Recurrence",
Copy link

@tasken tasken Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recurrencia del gasto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added translations

messages/es.json Outdated Show resolved Hide resolved
messages/es.json Outdated Show resolved Hide resolved
messages/es.json Outdated Show resolved Hide resolved
messages/es.json Outdated Show resolved Hide resolved
messages/es.json Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants