Skip to content

Feat/invoice#1007

Merged
chedieck merged 10 commits intomasterfrom
feat/invoice
Jun 19, 2025
Merged

Feat/invoice#1007
chedieck merged 10 commits intomasterfrom
feat/invoice

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

Related to #528

Description

added actions column to transactions table in button details page.

  • add invoice, edit invoice, view invoice.

Test plan

Test creating a new invoice, visualize it, try to edit the invoice.

@Klakurka Klakurka requested review from Klakurka and chedieck May 30, 2025 05:11
Copy link
Copy Markdown
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Left some comments. Besides them:

  1. I think this "Action" column should be either the first or the last (probably the last). Having it in the middle is a little weird for me.
  2. You didn't actually implement the API endpoint to edit invoices.

}
}

interface InvoiceData {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is being created twice, just import it


const [invoiceMode, setInvoiceMode] = useState<'create' | 'edit' | 'view'>('create')

const onCreateInvoice = async (transaction: any): Promise<void> => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't like this any here. We do have types for transactions.

package.json Outdated
"jest": "^29.7.0",
"jest-mock-extended": "^2.0.6",
"lint-staged": "^12.4.1",
"lucide-react": "^0.510.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This does not seem necessary. We already have PNGs for what you are using, check the assets/ folder.


export async function createInvoice (params: CreateInvoiceParams): Promise<Invoice> {
// const invoiceNumber = await getNewInvoiceNumber(params.userId);
console.log('params', params)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

debug log here

return { timezone: params.timezone }
}

export interface CreateinvoicePOSTParameters {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wrong UpperCamelCase syntax

return
}
}
const nextNumber = (invoiceWithTheLatestInvoiceNumber != null) ? parseInt(invoiceWithTheLatestInvoiceNumber.invoiceNumber.split('-')[1]) + 1 : 1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the user has created some invoice number like "2025-2Q" it would NaN here. After checking the regex above it would be safe tho

}

export async function createInvoice (params: CreateInvoiceParams): Promise<Invoice> {
// const invoiceNumber = await getNewInvoiceNumber(params.userId);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove this

})
}

export async function getInvoices (userId: string): Promise<Invoice[]> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

too generic name. getUserInvoices, for example, would be better

@lissavxo lissavxo requested a review from chedieck June 6, 2025 17:38
const invoicesWithOurPattern = userInvoices.filter(invoice => defaultPattern.test(invoice.invoiceNumber))

if (invoicesWithOurPattern === null) {
const invoicesCount = await prisma.invoice.count({
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

invoicesCount should be userInvoices.length, no need to access the DB again if you already have that information

userId
}
})
if (invoicesCount > 0) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The logic here is not considering the case where invoicesWithOurPattern.length < userInvoices.length, which in this case (userInvoices.length > 0) should return null.

@lissavxo lissavxo requested a review from chedieck June 10, 2025 21:52
const invoicesWithOurPattern = userInvoices.filter(invoice => defaultPattern.test(invoice.invoiceNumber))

if (invoicesWithOurPattern === null || invoicesWithOurPattern.length < userInvoices.length) {
if (userInvoices.length > 0) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is redundant. This is inside the condition of userInvoices.length being strictly greater than invoicesWithOurPattern.length, which is a non-negative number, so userInvoices.length > 0 is already given.

@lissavxo lissavxo requested a review from chedieck June 18, 2025 15:52
@lissavxo lissavxo mentioned this pull request Jun 18, 2025
1 task
export interface InvoiceData {
id?: string
invoiceNumber: string
amount: number
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a good idea to use number here, can get messy with float imprecision. Best to stick to Prisma.Decimal

Remember to change this also in the other interfaces you're creating.

@chedieck chedieck merged commit c1fcffc into master Jun 19, 2025
2 checks passed
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.

2 participants