Conversation
lissavxo
force-pushed
the
feat/export-csv-payments
branch
from
d1389fd to
ee415cf
Compare
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
It needs to change to the dropdown when there's more than 1 currency involved - same behaviour as on the existing transactions one.
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
The exported CSV filename should be the same format as the transaction page ones but without the button name on the front.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
Left a minor request, but my biggest concern is:
Is it necessary to mantain both API endpoints pages/api/payments/download/index.ts and pages/api/paybutton/download/transactions/[paybuttonId].ts?
They have some differences but most code is repeated. My concern is because this separation may make future maintenance harder. In any case, I think they share some utility functions which could be imported from utils/index.ts, like isNetworkValid
A less obvious example is: sortPaymentsByNetworkId, which in the other view is called sortTransactionsByNetworkId and it does exactly the same thing, except that it takes Transactions, not Payments.
Couldn't both just take payments? In the end payments are what are used to generate the CSV anyway.
It's an honest question, I haven't dug that deep into it, maybe there is a good reason to keep it separate like this but seeing this amount of really similar but not identical code being repeated does strike me as something odd.
| const getDataAndSetUpCurrencyCSV = async (): Promise<void> => { | ||
| const paybuttons = await fetchPaybuttons() | ||
| const networkIds: number[] = [] | ||
| paybuttons.forEach((p: { addresses: any[] }) => { |
There was a problem hiding this comment.
Better to use new Set here already and down where this array is used with .includes just use the Set method .has which does the same
lissavxo
force-pushed
the
feat/export-csv-payments
branch
from
f754178 to
d81ff00
Compare
3 participants
Related to #740
Description
Added export csv functionality to payments page
Test plan
Run the server with
docker compose upGo to payments page click
Export as CSVbutton it should download the payments in a csv