Skip to content

Feat: Payments buttons filter#991

Merged
Klakurka merged 11 commits intomasterfrom
payments-filter
May 13, 2025
Merged

Feat: Payments buttons filter#991
Klakurka merged 11 commits intomasterfrom
payments-filter

Conversation

@johnkuney
Copy link
Copy Markdown
Collaborator

Related to #972

Description

Add a multi select button filter on the payments page that will only show payments from those buttons

Test plan

Run the app and check out the payments page and mess with the filters

@johnkuney johnkuney requested a review from chedieck March 27, 2025 15:04
@Klakurka Klakurka self-requested a review March 28, 2025 01:38
Copy link
Copy Markdown
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.

I get an error on this branch when I refresh any of the pages (with F5) but I feel like it's probably some weird local env thing so going to ignore it (will copy it here though just in case):

image

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

Filter isn't apply to CSV generation.

amount: { gt: 0 }
}

const totalCount = await prisma.transaction.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.

this logic should be in the service layer

const orderDescString: Prisma.SortOrder = orderDesc ? 'desc' : 'asc'

if (orderBy === 'buttonDisplayDataList') {
return await getPaymentsByUserIdOrderedByButtonName(
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.

filter by buttons in getPaymentsByUserIdOrderedByButtonName is missing, this way filters will not work in the order by button

@johnkuney
Copy link
Copy Markdown
Collaborator Author

Okay think I got those sorted

lissavxo
lissavxo previously approved these changes Apr 1, 2025
buttonIds = (req.query.buttonIds as string).split(',')
}

if ((buttonIds != null) && buttonIds.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.

Should be !== undefined, not != null.
!= null works but leads the reader to consider that the variable can be null OR undefined, which is not the case, since it can only be undefined.

},
amount: {
gt: 0
// Build the base where clause
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 think any of these comments are useful

}

// If buttonIds is provided, add a nested filter on address.paybuttons
if ((buttonIds != null) && buttonIds.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.

!== undefined

networkId: {
in: networkIds ?? Object.values(NETWORK_IDS)
}
const whereClause: any = {
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.

Prisma.TransactionWhereInput

amount: {
gt: 0
// Build the base where clause
const whereClause: any = {
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.

Better to use Prisma.TransactionWhereInput instead of any.

}
}

if (Array.isArray(buttonIds) && buttonIds.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.

In the function above we had similar code but with if ((buttonIds != null), which was basically checking if buttonIds !== undefined. This Array.isArray(buttonIds) achieves the same result but with in a different way, which makes what it is doing a little less obvious and harder to read.

userId: string,
buttonIds: string[]
): Promise<number> => {
const whereClause: any = {
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.

No need to create this constant

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.

Had some minor code complaints, but I decided to fix them myself.

I noticed that changing the filtered buttons makes the order reset to "by Date": would be nice to have it stay in place (e.g. "by Amount" ascendent, if that was selected)

@johnkuney
Copy link
Copy Markdown
Collaborator Author

Cool thanks for fixing those. Sorting should be persistent now when filtering

@chedieck chedieck self-requested a review April 2, 2025 13:06
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.

Now indeed the order is maintained after changing the paybuttons to filter with, but the page is not reset.

Meaning if I select two buttons: one with a lot of txs and one with only a few, and go to page 25 of payments, and then unselect the one with a lot of txs leaving only the one with a few, the page stays in 25 (which doesn't exist)

image

Would be nice if it would go back to the first one when some change is made to the paybuttons to filter with.

@johnkuney
Copy link
Copy Markdown
Collaborator Author

ah yes good catch, should be good now

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

  • Let's change "Filter by PayButton" to "Filter by Button" to keep the wording consistent in the UI.
  • Maybe we can make a new task for improving the CSV export filename when exporting a filtered set of buttons.

Looks good to me otherwise.

@johnkuney
Copy link
Copy Markdown
Collaborator Author

Okay updated that label
I did also see that error you sent in telegram about "unexpected token '<'...." Yeah, same thing if I create a new button and then try to filter by it right away. May require some investigation...

@johnkuney
Copy link
Copy Markdown
Collaborator Author

Actually think I got it

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

Suggestion on what to do when we run into that error?

@chedieck

@johnkuney
Copy link
Copy Markdown
Collaborator Author

@chedieck any updates here?

@chedieck
Copy link
Copy Markdown
Collaborator

Since I can't seem to reproduce this error, I left a commit with some logs to understand better what is erroring. So far I can't tell if this would happen in prod or not since I am not being able to reproduce.

@Klakurka
Copy link
Copy Markdown
Member

Klakurka commented May 9, 2025

I got this error when sorting a few times right after adding a few new buttons:

image

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

Let's just go ahead with this and if there's issues in prod then we can tackle them separately.

@Klakurka Klakurka merged commit 06e5e3a into master May 13, 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.

4 participants