feature: recurring transactions added- migrations, ui, api updated. Closes #5 #114

neonshobhit · 2024-02-25T23:29:06Z

This Pull requests solves recurring transactions adding feature.
Closes #5

neonshobhit · 2024-02-25T23:44:40Z

If recurring transaction is selected from the drop down menu while adding an expense, that expense id will be inserted in the recurring transaction table once.
When listing for the groups, it will check if any pending recursive transaction needed to be added, if yes, then same transaction will be copied.

WIP:

Need to add same support while editing the expense.
Delete only latest transaction in the recursive transaction is yet to be supported. This will be done by giving options for archiving the expense/ delete this and all future expenses.

neonshobhit · 2024-03-08T11:27:01Z

Update the PR.
Features:

Set the recurring expense schedule
While fetching Group's Expenses, checks if any pending expense exists. If yes, adds into the DB and then lists all the expenses.
Edit the expense schedule.
Delete the expense schedule (Deleting the last added expense deletes the future entries.)
Archive the expense which needs to be shown in history but not needed while calculating.
Archiving the last expense doesn't add to calculations, but will occur in the next cycle.

rgov · 2024-04-10T19:37:16Z

src/app/groups/[groupId]/expenses/page.tsx

@@ -100,7 +100,7 @@ export default async function GroupExpensesPage({
 async function Expenses({ groupId }: { groupId: string }) {
  const group = await cached.getGroup(groupId)
  if (!group) notFound()
-  const expenses = await getGroupExpenses(group.id)
+  const expenses = (await getGroupExpenses(group.id))// .filter(e => !e.isArchive)


Can you revert this no-op change?

rgov · 2024-04-10T19:39:59Z

package.json

  },
  "devDependencies": {
    "@total-typescript/ts-reset": "^0.5.1",
    "@types/content-disposition": "^0.5.8",
-    "@types/node": "^20",
+    "@types/node": "^20.11.20",


This file (and presumably package-lock.json) is virtually unchanged except for sorting the packages and bumping the @types/node version. Is the version bump necessary? If not, I recommend removing both package.json and package-lock.json from this PR.

rgov · 2024-04-10T19:41:14Z

src/app/groups/[groupId]/expenses/[expenseId]/edit/page.tsx

+    redirect(`/groups/${groupId}`)
+  }
+
+  async function archiveExpenseStateUpdateAction(state: boolean) {


Is this stuff about archiving related to this commit "added locks to ensure only 1 user can get the group expenses created"? If no, can it be factored out of this commit / pull request and submitted separately?

No
This feature was added here for removing a recurring transction temporarily.
For example:
I have a monthly subscription of Netflix. It was there for 6 months but we cancelled for 1 month.
It will automatically add in the recurring expense section.
If I delete the transaction, future recurring transactions will not be created. Hence Archiving a transaction will result it to be there, but won't add in the split calculations.

Please check this image for archived expense in the fetched expenses

I see. I recommend simplifying the pull request to remove archived expenses for now, and propose that as a separate feature. There may be disagreement over the user experience here and it will delay acceptance of the recurring expense feature. (The word "archive" to me does not connote the behavior of the feature, for example.)

Splitwise behavior is: The most recent expense is the one marked "recurring." Older expenses can be deleted without affecting the recurrence. If you delete the most recent expense, the recurrence stops.

That makes it easy to delete transactions 1...n-1 without affecting the recurrence. But I agree it is useful to have a way to delete the most recent transaction without affecting the recurrence, too.

For version 1 of this feature I suggest just stopping the recurrence if the most recent transaction is deleted. Maybe pop up an alert warning the user about this.

sure. Can do. I'll raise that feature as another PR and add an alert.
Archive seemed best to me as it just serves as a historical preserved record for reference. Any other suggestion?

rgov · 2024-04-10T19:43:04Z

src/lib/api.ts

@@ -27,6 +35,134 @@ export async function createGroup(groupFormValues: GroupFormValues) {
  })
 }

+export async function acquireLockRecurringTransaction(groupId:string, expenseId:string, lockId: string): Promise<RecurringTransactions[]> {


Could you elaborate on why we need to implement locking and retrying here? What is the issue if multiple users edit a recurring expense concurrently? Some comments would go a long way towards explaining the logic.

When someone fetches group expenses, only then I'm checking if there's a pending recurring transaction. If yes, then I insert in the DB as new transaction.

Locking is necessary to avoid race condition:
if two people fetch expenses at the same time, APIs of both will result in fetching of pending transactions and inserting in DB. Hence Double insertions for the pending transaction.

Same condition will be there for update. One person is updating the transaction and other is inserting in DB.

Locking ensures consistency and correctness.

Retrying is there to retry acquiring locks after some time if it was acquired by someone else at that instant.

Is it possible to delegate this concern to the database? Suppose each expense ID in a recurrence followed the pattern ffffff01, ffffff02, ffffff03, .... When expenses are fetched, you determine the need to create expense ffffff04. You do some kind of INSERT IF NOT EXISTS query (as you mention — createIfNotExists is what you're looking for). If there is a race and this expense already got inserted by another request, it doesn't really affect anything.

Worst case scenario I think is if, before your request completes, the expense has already been created and then subsequently updated. You might have a stale view of the totals. But it would only be a temporary issue.

Umm, not.
Let's take an example:

1 Jan 2024 (monthly txn) ffffff01 was added. And the group wasn't opened until now.
If Person P1 and P2 both open it at the same moment - both will determine that they need to insert 3 new Txns. How will I verify if P1 is already adding the transaction and P2 doesn't need to?
ffffff04 can be a copy of ffffff02 (determined by P1) or ffffff02 (determined by P2).
For realising that someone else is working on this resource, I need to insert a reference id in DB.
That Id can be in the same Expense table, or a new table that I created.
New table made it easy to manage it as I can keep track which txn needs to be inserted without looking into Expense table + separation of concern.

So if lock is not there, in worst case both will insert the same records.

Another case:
P1 didn't call fetch groups (consider he didn't close tab for 4 months). P2 fetches now and he realizes he has to insert 3 new records.

P1 edits the amount from 100 to 200.
What shall happen now? Either P1 should successfully edit first and P2 shall recursively add 3 txns with updated amount OR P2 should insert all 3 txns and then only P1 should update the amount.
It shouldn't happen that P2 inserted 1 record with old amount and later 2 with updated amount.

I agree, chances of happening while edit is very hard, but it's possible that two people open the group at the same time and double inserts can happen.
I made lock so that this issue doesn't happen.

rgov · 2024-04-10T19:43:51Z

src/lib/api.ts

+          ON CONFLICT("groupId", "expenseId")
+          DO NOTHING;
+      `
+      await prisma.$queryRawUnsafe(query)


I don't know Prisma but I do not recommend building SQL queries via string interpolation. This is how SQL injection vulnerabilities arise, note the "unsafe" in the function name.

Agreed. I searched for Prisma docs but there's no option for 'createIfNotExists' hence used this method. I need this functionality for avoiding race conditions in between of inserting and fetching.

I also don't like building SQL queries like this - can you please help here for the alternatives for the above requirement?

Could something like this work? reference. I'm not following the issue with createIfNotExists.

const query = await prisma.$queryRaw` INSERT INTO "RecurringTransactions" ("groupId", "expenseId", "createNextAt", "lockId") VALUES (${groupId}, ${expenseId}, 0, ${lockId}) ON CONFLICT ("groupId", "expenseId") DO NOTHING; `

You mean to use queryRaw instead of queryRawUnsafe?

justcallmelarry · 2024-04-17T07:51:18Z

what happens if no one opens the group for over a month? will it add two recurring entries? or will it skip one?

I generally think that there should be some sort of scheduler that takes care of adding recurring expenses rather than doing some magic when people are loading the group

neonshobhit · 2024-04-17T08:11:19Z

what happens if no one opens the group for over a month? will it add two recurring entries? or will it skip one?

It recursively adds all the pending txns, which means it won't skip any.

I generally think that there should be some sort of scheduler that takes care of adding recurring expenses rather than doing some magic when people are loading the group

Agreed. I wanted to make a scheduler only but decided against it for the following reasons:

Some groups can just a txn scheduler but never open that group. Since it's not requiring even logins, abuse can be high. So I felt if someone is opening the group, they are showing the intent that they want all the "pending expenses". Those who are not opening, can be avoided.
This is open source + allows redistribution. So, if someone wants to set Spliit up in their own server, they will need to set up crons scheduler separately if they want to use serverless. Having server in EC2 machines would be easier though, but I didn't want to limit serverless possibility becasue of just one small feature.

rgov · 2024-04-17T16:11:21Z

The approach is appealing because it also works with a scheduler, you can simply have a cron task that polls the transaction list API, right?

neonshobhit · 2024-04-18T19:13:34Z

Yes. But we will have to build a scheduler also. For example, we wouldn't fetch all the groups all the time. We would rather fetch only the transactions that needs to be copied (Fetch only those groups).
But yes, internally, scheduler needs to call only this function.

…esolves issue spliit-app#5

…or locking

neonshobhit · 2024-05-26T14:28:53Z

@rgov Have removed archive feature from this PR and opened another PR for the same. Resolved all the comments also.

rgov

Thanks @neonshobhit for splitting the PR. I think this is a great feature and I hope @scastiel will provide feedback now. I have some style nitpicks here.

rgov · 2024-05-26T19:39:19Z

prisma/schema.prisma

@@ -53,6 +53,7 @@ model Expense {
  createdAt       DateTime          @default(now())
  documents       ExpenseDocument[]
  notes           String?
+  recurringDays  Int               @default(0)


Indentation/alignment does not match the lines above.

rgov · 2024-05-26T19:39:42Z

src/app/groups/[groupId]/expenses/page.tsx

@@ -104,7 +104,7 @@ export default async function GroupExpensesPage({
 async function Expenses({ groupId }: { groupId: string }) {
  const group = await cached.getGroup(groupId)
  if (!group) notFound()
-  const expenses = await getGroupExpenses(group.id)
+  const expenses = (await getGroupExpenses(group.id))


Unnecessary change.

rgov · 2024-05-26T19:40:03Z

src/app/groups/recent-group-list-card.tsx

@@ -54,7 +54,6 @@ export function RecentGroupListCard({
    })

  const isStarred = state.starredGroups.includes(group.id)
-  const isArchived = state.archivedGroups.includes(group.id)


Should this change still be part of this PR?

rgov · 2024-05-26T19:40:19Z

src/components/expense-form.tsx

@@ -51,6 +51,7 @@ import { match } from 'ts-pattern'
 import { DeletePopup } from './delete-popup'
 import { extractCategoryFromTitle } from './expense-form-actions'
 import { Textarea } from './ui/textarea'
+import { AsyncButton } from './async-button'


This import isn't used.

rgov · 2024-05-26T19:41:05Z

src/lib/api.ts

 import { nanoid } from 'nanoid'
+import * as z from 'zod';
+// import { setTimeout } from "timers/promises";


Commented out code should be removed from the PR.

rgov · 2024-05-26T19:45:07Z

src/lib/api.ts

@@ -243,7 +413,69 @@ export async function getCategories() {
 }

 export async function getGroupExpenses(groupId: string) {
+  const now: number = Math.floor((new Date()).getTime() / 1000)


This calculation Math.floor(date.getTime() / 1000 appears in a few places, it would probably improve readability to factor this into a separate function (and the inverse *1000 calculation) since it's not super obvious what the calculation is for, I'm guessing converting a JS timestamp from milliseconds to seconds.

rgov · 2024-05-26T19:49:41Z

src/components/expense-form.tsx

@@ -241,6 +250,7 @@ export function ExpenseForm({
    return onSubmit(values)
  }

+  const recurringDays = [{ "key": "Never","value": "0"}, { "key":"weekly", "value": "7"}, {"key": "fortnightly", "value": "14"}, {"key": "monthly", "value": "30"}, {"key": "bimonthly", "value": "60"}]


This supports recurrences on an interval of days, but not other typical calendar occurrences like "on the first day of every month". I think it's fine for that to be version 1 of this feature, but rather than display "monthly" I think it should display "every 30 days" so that it doesn't lead to any surprises.

Yes. I'll add a fixes PR later to fix this issue.
Right now, changing to every 30 days.

rgov · 2024-05-26T19:52:39Z

src/components/expense-form.tsx

+                    </SelectContent>
+                  </Select>
+                  <FormDescription>
+                    Select recursive days.


I would not use "recursive" here, or possibly even "recurrence". "Repeat every 30 days" seems the most natural English to me.

We can have options like 30 days, 7 days etc. Can you please suggest other title?
or
"Repeat after:" this seems okay?

The Apple Calendar app uses "Repeat: <never|every day|every week|...>" which seems like a decent way to display this and a set of choices.

rgov · 2024-05-26T19:54:44Z

src/components/expense-form.tsx

@@ -182,6 +187,7 @@ export function ExpenseForm({
          isReimbursement: expense.isReimbursement,
          documents: expense.documents,
          notes: expense.notes ?? '',
+          recurringDays: String(expense.recurringDays),


Why does this need to be a String here rather than a number?

I started with Number, but I thought about this as well. How to store "1st of every month"?
Cron makes more sense. Please check here.
Most probably I will have to change every thing from days to cron - which I can add in v2 PR.

rgov · 2024-05-26T20:01:23Z

src/lib/api.ts

+  let recTxn;
+  if (!receivedLockId) return recTxn
+  const epochTime = Math.floor((new Date(expenseDate)).getTime() / 1000)
+  const nextAt: number = epochTime + (+expenseFormValues.recurringDays * 86400)


This creates the transaction exactly 24 hours after the previous transaction. It might be ever-so-slightly better to use Math.floor(epochTime / 86400) + (1 + expenseFormValues.recurringDays) * 86400 to get midnight of the day of the next occurrence.

If there is a JS library that does these calculations, like moment.js or whatever, I would suggest using it.

Sure.
Better way to do this is by using crons as mentioned in this comment. I can add this here or in V2 PR where I'll be changing everything from Days to crons?

neonshobhit · 2024-05-26T22:32:30Z

@rgov Resolved comments. Please check.

sir-argupta · 2024-08-01T16:24:51Z

@neonshobhit Can you please resolve conflicts..!?

ankit-kapur · 2024-08-09T00:35:27Z

Would love to see this merged @neonshobhit

…rringtransactions

neonshobhit · 2024-09-02T19:59:18Z

@sir-argupta @ankit-kapur
Apologies, Completely missed the notifications.

Anyways it's resolved and working now.
@scastiel can you please review this PR?

trandall2 · 2024-11-06T05:57:49Z

It looked like this PR has been inactive for a bit and has several merge conflicts that I was having issues resolving. I took inspiration from this PR for my take on the feature^.

(Note: This was also the last feature I was really waiting for to be able to switch off of SplitWise, so I have some extra motivation to try to get it through)

neonshobhit changed the title ~~[WIP] feature: recurring transactions added- migrations, ui, api updated #5~~ [WIP] feature: recurring transactions added- migrations, ui, api updated. Closes #5 Feb 25, 2024

neonshobhit force-pushed the feature/recurringtransactions branch from e511ed3 to 10263bb Compare February 25, 2024 23:35

neonshobhit changed the title ~~[WIP] feature: recurring transactions added- migrations, ui, api updated. Closes #5~~ feature: recurring transactions added- migrations, ui, api updated. Closes #5 Mar 8, 2024

neonshobhit force-pushed the feature/recurringtransactions branch from 42797c2 to 4001fc4 Compare March 13, 2024 19:53

rgov suggested changes Apr 10, 2024

View reviewed changes

feature: recurring transactions added- migrations, ui, api updated. R…

42eb917

…esolves issue spliit-app#5

neonshobhit force-pushed the feature/recurringtransactions branch from 4001fc4 to deae321 Compare May 25, 2024 20:08

removed package(-lock).json diff, no-op code removed, addd comments f…

8a2b54d

…or locking

neonshobhit force-pushed the feature/recurringtransactions branch from dbffcb3 to 8a2b54d Compare May 26, 2024 08:51

removed archive expense feature

6dde112

neonshobhit mentioned this pull request May 26, 2024

Feature/archive #157

Open

rgov suggested changes May 26, 2024

View reviewed changes

neonshobhit added 2 commits May 27, 2024 01:44

remove formatting changes

f4f30a7

updated repeat transactions form description

92dae0e

removed conflicts

54507da

neonshobhit force-pushed the feature/recurringtransactions branch from a5e6e04 to 54507da Compare September 2, 2024 19:53

neonshobhit added 2 commits September 3, 2024 01:25

Merge branch 'main' of github.com:spliit-app/spliit into feature/recu…

1fd7e43

…rringtransactions

undoed package json

099a03c

trandall2 mentioned this pull request Nov 6, 2024

Add recurring expense functionality #263

Open

feature: recurring transactions added- migrations, ui, api updated. Closes #5 #114

Are you sure you want to change the base?

feature: recurring transactions added- migrations, ui, api updated. Closes #5 #114

Conversation

neonshobhit commented Feb 25, 2024 • edited Loading

neonshobhit commented Feb 25, 2024 • edited Loading

neonshobhit commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonshobhit Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

rgov Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgov Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

neonshobhit Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

rgov Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcallmelarry commented Apr 17, 2024

neonshobhit commented Apr 17, 2024

rgov commented Apr 17, 2024

neonshobhit commented Apr 18, 2024

neonshobhit commented May 26, 2024

rgov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgov May 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonshobhit May 26, 2024 • edited Loading

Choose a reason for hiding this comment

rgov May 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonshobhit commented May 26, 2024

sir-argupta commented Aug 1, 2024 • edited Loading

ankit-kapur commented Aug 9, 2024

neonshobhit commented Sep 2, 2024

trandall2 commented Nov 6, 2024 • edited Loading

neonshobhit commented Feb 25, 2024 •

edited

Loading

neonshobhit commented Feb 25, 2024 •

edited

Loading

neonshobhit commented Mar 8, 2024 •

edited

Loading

neonshobhit Apr 10, 2024 •

edited

Loading

rgov Apr 10, 2024 •

edited

Loading

rgov Apr 10, 2024 •

edited

Loading

neonshobhit Apr 10, 2024 •

edited

Loading

rgov Apr 10, 2024 •

edited

Loading

rgov May 26, 2024 •

edited

Loading

neonshobhit May 26, 2024 •

edited

Loading

rgov May 26, 2024 •

edited

Loading

sir-argupta commented Aug 1, 2024 •

edited

Loading

trandall2 commented Nov 6, 2024 •

edited

Loading