-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
feature: recurring transactions added- migrations, ui, api updated. Closes #5 #114
base: main
Are you sure you want to change the base?
Changes from 3 commits
42eb917
8a2b54d
6dde112
f4f30a7
92dae0e
54507da
1fd7e43
099a03c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
-- CreateTable | ||
CREATE TABLE "RecurringTransactions" ( | ||
"groupId" VARCHAR(255) NOT NULL, | ||
"expenseId" VARCHAR(255) NOT NULL, | ||
"createNextAt" INT NOT NULL, | ||
"lockId" VARCHAR(255) DEFAULT NULL, | ||
|
||
CONSTRAINT "RecurringTransactions_pkey" PRIMARY KEY ("groupId", "expenseId") | ||
); | ||
|
||
CREATE INDEX "idx_recurring_transactions_group_expense_next_create" ON "RecurringTransactions" ("groupId", "expenseId", "createNextAt" DESC); | ||
CREATE UNIQUE INDEX "idx_unq_recurring_transactions_lock_id" ON "RecurringTransactions" ("lockId"); | ||
|
||
-- AlterTable | ||
ALTER TABLE "Expense" ADD COLUMN "recurringDays" TEXT NOT NULL DEFAULT 0; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary change. |
||
|
||
return ( | ||
<ExpenseList | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,6 @@ export function RecentGroupListCard({ | |
}) | ||
|
||
const isStarred = state.starredGroups.includes(group.id) | ||
const isArchived = state.archivedGroups.includes(group.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this change still be part of this PR? |
||
|
||
return ( | ||
<li key={group.id}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import isn't used. |
||
|
||
export type Props = { | ||
group: NonNullable<Awaited<ReturnType<typeof getGroup>>> | ||
|
@@ -164,6 +165,10 @@ export function ExpenseForm({ | |
return field?.value | ||
} | ||
const defaultSplittingOptions = getDefaultSplittingOptions(group) | ||
|
||
const getRecurringField = (field?: { value: string }) => { | ||
return field?.value | ||
} | ||
const form = useForm<ExpenseFormValues>({ | ||
resolver: zodResolver(expenseFormSchema), | ||
defaultValues: expense | ||
|
@@ -182,6 +187,7 @@ export function ExpenseForm({ | |
isReimbursement: expense.isReimbursement, | ||
documents: expense.documents, | ||
notes: expense.notes ?? '', | ||
recurringDays: String(expense.recurringDays), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I started with Number, but I thought about this as well. How to store "1st of every month"? |
||
} | ||
: searchParams.get('reimbursement') | ||
? { | ||
|
@@ -205,6 +211,7 @@ export function ExpenseForm({ | |
saveDefaultSplittingOptions: false, | ||
documents: [], | ||
notes: '', | ||
recurringDays: "0", | ||
} | ||
: { | ||
title: searchParams.get('title') ?? '', | ||
|
@@ -232,7 +239,9 @@ export function ExpenseForm({ | |
] | ||
: [], | ||
notes: '', | ||
recurringDays: "0", | ||
}, | ||
|
||
}) | ||
const [isCategoryLoading, setCategoryLoading] = useState(false) | ||
|
||
|
@@ -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"}] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I'll add a fixes PR later to fix this issue. |
||
return ( | ||
<Form {...form}> | ||
<form onSubmit={form.handleSubmit(submit)}> | ||
|
@@ -413,6 +423,34 @@ export function ExpenseForm({ | |
<FormControl> | ||
<Textarea className="text-base" {...field} /> | ||
</FormControl> | ||
</FormItem> | ||
)} | ||
/> | ||
<FormField | ||
control={form.control} | ||
name="recurringDays" | ||
render={({ field }) => ( | ||
<FormItem className="sm:order-5"> | ||
<FormLabel>Recurring Days</FormLabel> | ||
<Select | ||
onValueChange={field.onChange} | ||
defaultValue={getRecurringField(field)} | ||
> | ||
<SelectTrigger> | ||
<SelectValue placeholder="Never" /> | ||
</SelectTrigger> | ||
<SelectContent> | ||
{recurringDays.map(({ key, value }) => ( | ||
<SelectItem key={key} value={value}> | ||
{key} | ||
</SelectItem> | ||
))} | ||
</SelectContent> | ||
</Select> | ||
<FormDescription> | ||
Select recursive days. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not use "recursive" here, or possibly even "recurrence". "Repeat every 30 days" seems the most natural English to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can have options like 30 days, 7 days etc. Can you please suggest other title? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
</FormDescription> | ||
<FormMessage /> | ||
</FormItem> | ||
)} | ||
/> | ||
|
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.
Indentation/alignment does not match the lines above.