Skip to content

fix: button name csv notes#978

Closed
lissavxo wants to merge 3 commits intomasterfrom
fix/button_name_csv_notes
Closed

fix: button name csv notes#978
lissavxo wants to merge 3 commits intomasterfrom
fix/button_name_csv_notes

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

@lissavxo lissavxo commented Mar 2, 2025

Related to #974

Description

Fixed button name in csv buildind
added name of button to notes

Test plan

Check both payments csv and button page csv building

@lissavxo lissavxo requested a review from chedieck March 3, 2025 18:17
utils/files.ts Outdated
.filter(pb => pb.paybutton.providerUserId === userId)
.map(pb => pb.paybutton.name)
)
if (uniqueButtonName.size > 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.

this does not look right. If a tx belongs to more than one button, which is something that we allow, we silently show an error in the logs and just pick one of the button names at random?

I think the expected behavior here is that when generating the CSV from the button page we get THAT button name here, and when generating it from the payments page we get all the names here comma separated, or something of the kind.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed, separated by semicolon so it does not mess the csv that is comma separated

@lissavxo lissavxo requested a review from chedieck March 7, 2025 15:21
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.

Works well, just a minor request.

Nonetheless, this collapseSmallPayments function has 150+ lines of code, 2 different functions are defined inside it and still some routines that could be separated into a different function (such as the paybutton name extraction). I think in its current state it is hard to read it and hard to mantain it, but refactoring could be done in a separate PR, so I'll open a separate issue for that.

const values = getTransactionValue(tx)
const value = Number(values[currency])
const rate = tx.prices.find(p => p.price.quoteId === QUOTE_IDS[currency.toUpperCase()])!.price.value
const buttonNames = groupKey.split('_').slice(2).join(' ; ')
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.

any reason for this spaces around the ;? It's somewhat counter intuitive and uncommon, so I'd take it out if there is no particular reason for it.

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.

3 participants