Skip to content

Conversation

@lissavxo
Copy link
Collaborator

@lissavxo lissavxo commented Jun 18, 2025

Related to #528

Depends on

Description

Added download pdf function in view invoice modal.

Test plan

Go to a transaction in button details, create an invoice, click view invoice>Download as PDF

@Klakurka Klakurka requested review from Klakurka and chedieck June 19, 2025 21:18
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • This should be part of the Payments page, not the Transactions table. The transactions table in the button details isn't really a place we want people having to go to often unless they're really curious about what's going on in their wallets.
  • On the 'Amount' field in the Create Invoice dialog, let's include the currency icon somewhere so it's clear what units it uses.
  • Maybe add a slight 'greyed out' style to the inputs that are read-only? @johnkuney can suggest.
  • If the user has an org set, auto-fill 'Customer Name' field with the org name.

<button
onClick={() => onSeeInvoice(cellProps.row.original.invoices[0])}
onClick={() => onSeeInvoice(invoice)}
title="See Invoice"
Copy link
Member

Choose a reason for hiding this comment

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

Change to 'View Invoice'.

<div className={style.see_invoice_ctn}>
<button
onClick={() => onSeeInvoice(cellProps.row.original.invoices[0])}
onClick={() => onSeeInvoice(invoice)}
Copy link
Member

Choose a reason for hiding this comment

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

Change onSeeInvoice to onViewInvoice (more standard language).

Copy link
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.

This magic number indexing of invoices from a tx (transaction.invoices[0]) is very unsafe. Right now if two users add the same address to one of their buttons, and one of them creates an invoice for one of the address' txs, the other will be able to see that invoice.

minute: '2-digit',
hour12: true
}).replace(',', '')
const url = transactionNetworkId === 1 ? XEC_TX_EXPLORER_URL : BCH_TX_EXPLORER_URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't use a magic number here, we have XEC_NETWORK_ID in constants/index

transactionHash: transaction.hash,
transactionDate: transaction.timestamp,
transactionNetworkId: transaction.address.networkId,
...transaction.invoices[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unsafe

@johnkuney
Copy link
Collaborator

Im getting this error if I try to view a button details page on this branch. Master seems to work fine for me...not sure what the issue is
Screen Shot 2025-06-22 at 9 46 09 PM

@lissavxo
Copy link
Collaborator Author

This magic number indexing of invoices from a tx (transaction.invoices[0]) is very unsafe. Right now if two users add the same address to one of their buttons, and one of them creates an invoice for one of the address' txs, the other will be able to see that invoice.

we are fetching the transactions filtering by the userId, so that wont happen

@lissavxo lissavxo requested review from Klakurka and chedieck June 25, 2025 23:05
@lissavxo
Copy link
Collaborator Author

This magic number indexing of invoices from a tx (transaction.invoices[0]) is very unsafe. Right now if two users add the same address to one of their buttons, and one of them creates an invoice for one of the address' txs, the other will be able to see that invoice.

This magic number indexing of invoices from a tx (transaction.invoices[0]) is very unsafe. Right now if two users add the same address to one of their buttons, and one of them creates an invoice for one of the address' txs, the other will be able to see that invoice.

Added verification to only return invoices from the user that is requesting

@lissavxo lissavxo mentioned this pull request Jul 3, 2025
1 task
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • Being able to sort the 'Actions' column doesn't make any sense and also throws an error.
    image
  • We need to figure out a more clear way for people to know what the buttons do as it is not at all obvious without hovering and the tooltip takes a second to appear. We could possibly change 'Actions' to 'Invoice' in the table header.

@Klakurka Klakurka requested a review from Copilot July 7, 2025 05:01

This comment was marked as outdated.

@lissavxo lissavxo requested review from Klakurka and Copilot July 8, 2025 15:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds invoice support and PDF download functionality to the payments UI and backend services.

  • Extends transaction queries and cache generation to include invoice data
  • Defines new Redis types for invoices and updates payment transformation functions
  • Implements create/edit/view invoice modal with "Download as PDF" feature on the Payments page

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
services/transactionService.ts Includes invoices in raw SQL query and adds JSON parsing
services/invoiceService.ts Adds null-check for missing invoice in update flow
redis/types.ts Defines InvoiceData and extends Payment interface
redis/paymentCache.ts Implements generatePaymentFromTxWithInvoices
pages/payments/index.tsx Adds invoice modal handlers, props and table action column
components/Transaction/InvoiceModal.tsx Integrates react-to-print and hidden printable receipt
Comments suppressed due to low confidence (1)

pages/payments/index.tsx:67

  • The organization prop can be null when no organization is found. Update the type to Organization | null to reflect that possibility and avoid runtime errors.
  organization: Organization

redis/types.ts Outdated
providerUserId?: string
}

export interface InvoiceData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed if is not used in the cache? Why not use just Prisma.Invoice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed InvoiceData and created prisma type for InvoiceWithTransaction

chedieck
chedieck previously approved these changes Jul 17, 2025
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Minor nit but let's move the dialog buttons around a bit:

image
  • Change 'Close' to 'Cancel' to match the other buttons.
  • Move the 'Cancel' button to the left.
  • Have the Download PDF button be on the right.

For reference:
image
image

  • Have the 'Cancel' button be the darker button. All of them should be like this throughout the site - not sure why some aren't.

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Forgot to mention - the submit (for creation) button should use the same loading spinner as throughout the rest of the site.

Klakurka

This comment was marked as duplicate.

@lissavxo lissavxo requested review from Klakurka and chedieck July 31, 2025 20:59
@Klakurka Klakurka merged commit 1557a44 into master Aug 1, 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.

5 participants