Skip to content

feat(payments): add filters to the payments list - #4275

Draft
sarkissianraffi wants to merge 5 commits into
mainfrom
feat/payments-list-filters
Draft

sarkissianraffi wants to merge 5 commits into
mainfrom
feat/payments-list-filters

Conversation

@sarkissianraffi

@sarkissianraffi sarkissianraffi commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

The payments list now has eleven URL-persisted filters alongside search: status, amount, receipt number, created date, provider, method, currency, invoice number, customer, payment type and payable type. Filter changes reset pagination and issue one getPaymentsList request.

The pa adapter maps each chip to the API argument. Payment statuses include processing, enum options come from generated GraphQL types, and exact amount strings are preserved through URL parsing and BigInt variables, including 9223372036854775807 cents. Existing invoice/credit-note amount behavior is unchanged via a payment-only precision opt-in. The existing invoice-number selector remains unchanged on other pages; payments use the exact-text variant.

Verification

  • Full regression run: TZ=UTC pnpm exec jest --config jest.config.ts --coverage --maxWorkers=41,023 suites / 12,772 tests pass.
  • 710 tests across the filter system and Payments/Invoices/CreditNotes pages pass; final focused run 241 tests passes.
  • Changed executable lines: 124/127 covered (97.64%), branches 89/94 (94.68%), combined coverage 96.38%; both operation amount variables are asserted to be BigInt.
  • Types pass. Lint: no errors, 57 existing warnings; changed files clean. Translations added through the repository script; inspect and consistency checks pass.
  • Generated GraphQL file rebuilt against the local API.
  • 47 live browser checks: every filter, URL/reload persistence, four amount modes including reversed between bounds, full int64 values, inclusive dates, three filters plus search, clear all, page-two reset, count/row parity with REST, and one request per change. No export button or DataExport code.
  • CLI and all six SDKs match the same seven succeeded EUR payment IDs.

API and client QA, 79 HTTP assertions, cross-client IDs, UI QA and recording

Reproduce on a running development API (synthetic fixtures, no gateway calls):

lago exec api bundle exec rails runner script/seed_payments_filters.rb
python3 script/qa_payments_filters.py
lago exec api bundle exec rails runner script/benchmark_payments_filters.rb

Seed script: 30 payments / 29 visible, three customers, stub Stripe and GoCardless, all statuses/types/methods, receipts, two-invoice PaymentRequest, two currencies, dates at organization-timezone boundaries, zero and amounts through 9223372036854775807. Credentials stay in ignored mode-0600 files.

Screenshots and recording

Before:
Before

After:
After

Filter panel · Screen recording

Compatibility notes

Requires the linked API schema. Feature-branch codegen and TypeScript CI passed; GitHub lint and CodeQL pass. Standard Tests/Cypress/Sonar jobs skip draft PRs, so the full test and coverage results above were also run locally. The default codegen workflow targets API main; merge the API dependency before promoting this PR. An unchanged DatePicker test constructs a historical date before its timezone setup, so the full local suite uses TZ=UTC to match CI; the component and its test are unchanged. The provider method enum is named PaymentProviderMethodTypeEnum to avoid the existing manual/provider enum. The shared payment table's existing display formatter rounds amounts above JS safe integer precision; filter inputs, chips, URLs and query bounds stay exact. That shared display behavior is outside this change. No payment response fields or export added.

Related PRs

Merge order: API, then front; OpenAPI before client/CLI releases. JavaScript CI uses the pinned feature spec during that rollout.

Performance follow-up (2026-09-09)

  • paymentMethodType filter removed, together with its panel item, value labels, translations and tests: the API dropped payment_method_type for performance reasons (getlago/lago-api#6325, "Not shipped for performance reasons"). Generated GraphQL types regenerated from the API schema.
  • The payments collection metadata now carries totalCountCapped and hasNextPage like invoices; the header shows "10,000+" above the API's counting limit and pagination advances on hasNextPage.
  • pnpm code:style green (0 errors), 239 filter tests and the payments page tests green.
  • Depends on getlago/lago-api#6325; indexes ship first in getlago/lago-api#6341.

Raffi added 2 commits September 7, 2026 15:31
Add URL-backed payment filters with exact decimal amount bounds, generated GraphQL arguments and translated enum options. Keep invoice and credit-note filtering behavior unchanged.

Validate with 710 regression tests, 241 final targeted tests, types, lint, translations and 47 live browser checks cross-checked against REST. Include screenshots and a recording of the seeded dev environment.
Raffi added 3 commits September 8, 2026 20:55
Removed with the API filter for performance reasons (see the API change).
Removes the filter from the payments list filters, its panel item, value
labels, translations and tests; GraphQL types regenerated.
The payments collection metadata now carries totalCountCapped and
hasNextPage like invoices; the header count and the pagination use them.
useEffect(() => {
if (!searchPending) return

const timeout = setTimeout(() => {

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — This debounce restarts on every re-render, not just on keystrokes.

goToPage is in the dependency array and is a new function on every render: usePageSearchParam lists navigate in its useCallback deps (src/components/designSystem/Pagination/usePageSearchParam.ts:33), and ~/core/router/useNavigate returns a fresh closure each render with no memoization (src/core/router/useNavigate.ts:38). Any re-render inside the 500 ms window clears the timeout and starts a new one, and with notifyOnNetworkStatusChange: true + fetchPolicy: 'network-only' those renders arrive routinely, so the typed term can stay uncommitted.

We already have two shared solutions, both immune to this because the debounce is created once with empty deps:

  • useDebouncedSearch — for search-only lists (CustomersList, PlansList, CouponsList).
  • CustomerInvoicesTab.tsx:108-117 — the closer precedent, since it has URL filters and search: useMemo(() => debounce((value) => setSearchTerm(value || undefined), DEBOUNCE_SEARCH_MS), []) plus a cancel() on unmount.

Please use one of them rather than a bare setTimeout keyed on render-unstable deps.


if (queryPending || lastQuery.current === signature) return

lastQuery.current = signature

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — The established shape is to put the filters in the query's variables and let Apollo re-execute — InvoicesPage.tsx:159-176 with useGetInvoicesListLazyQuery, and CustomerInvoicesTab.tsx:86-106 with a plain useQuery (fetchPolicy: 'network-only', searchTerm and the formatted filters passed straight in). That also removes the need for queryPending.

Beyond the divergence, this manual dedup has three concrete defects:

  • The signature is stored before the request settles, so a failed getPayments is never retried for that signature — the effect skips it forever.
  • The signature depends on the key insertion order of formatFiltersForPaymentsQuery's reduce, so reordering URL params yields a different signature for identical variables.
  • It loses the loading anti-blink behaviour useDebouncedSearch provides.

Related, on line 87: searchInput.trim().length >= 3 duplicates MIN_SEARCH_CHARS (src/hooks/useDebouncedSearch.ts:8), which isn't exported precisely so callers don't re-derive it.

}}
placeholder={translate('text_17370296250897aidak5kjcg')}
/>
<Filters.Provider

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — The ten filters added here need the list's placeholders updated too. PaymentsList.tsx:305,317 branch only on variables?.searchTerm, so any pa_* combination with no matches renders "No payments have been recorded for invoices. Please record a payment or connect your customer to a payment provider…" — wrong when payments exist but are filtered out.

Two existing ways to fix it: InvoicesList.tsx:105 feeds searchParams into getEmptyStateConfig({ hasSearchTerm, searchParams, translate }), and CustomerInvoicesTab.tsx:119 derives an isFiltering flag from search + filters.

const paymentsIsLoading = loading || queryPending

const paymentsTotalCount = data?.payments?.metadata?.totalCount
// Above the API's counting limit the total is a lower bound: print "10,000+", as invoices do.

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit — This repeats the JSDoc of formatCountToMetadata (src/components/MainHeader/formatCountToMetadata.ts:4-12) and justifies the change rather than explaining the code — that belongs in the commit body, per .agents/docs/typescript-conventions.md → "Comments: Default to None".

Same for // Keep payment amounts exact through URL parsing, interval ordering and cents conversion. in paymentFilterValues.ts:13, which restates what the function names already say. (The jsdom / react-virtual comment in the new test is fine — that's a constraint the reader can't see.)

totalPages
totalCount
totalCountCapped
hasNextPage

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Info — Side note: these two fields, and the new filter arguments above, don't exist on API main — they come from getlago/lago-api#6325. Front codegen CI can't pass until that merges, so the merge order needs to be explicit on the PR.

[AvailableFiltersEnum.paymentProviderType]: 'text_634ea0ecc6147de10ddb6631',
[AvailableFiltersEnum.paymentType]: 'text_1788818972604tg2ogag4h2r',
[AvailableFiltersEnum.payableType]: 'text_17888189726040erbp5xejy9',
[AvailableFiltersEnum.paymentCreatedAt]: 'text_664cb90097bfa800e6efa3f5',

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit — That key is "Date", while the sibling created-at filters use text_1776870266380s3zbpmnfrhj ("Created at") — see quoteCreatedAt / orderFormCreatedAt a few lines below. The chip should read "Created at".

it('does not expose an export action', () => {
render(<PaymentsPage />)
expect(capturedConfig?.actions?.items).toHaveLength(1)
expect(screen.queryByRole('button', { name: /export/i })).not.toBeInTheDocument()

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug — This assertion can never fail: MainHeader.Configure is mocked at :14-17 to render only props.filtersSection, so no action button ever reaches the DOM — it would still pass if an export action were added. It also pins the absence of a feature that was never in scope.

Keep the toHaveLength(1) line and drop this one; or, if the header really should be asserted, render the real MainHeader the way TeamAndSecurity.test.tsx:9-14 does.

expect(setFilterValue).toHaveBeenLastCalledWith('isEqualTo,90071992547409.93,90071992547409.93')
})

it('uses exact invoice text only in the payments panel', () => {

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion — The only half of the name is never exercised: the in-prefix case, which must still get the FiltersItemInvoiceNumber combobox, isn't rendered — and that's precisely the regression the prefix branching risks. Please add it (best case this test disappears along with the prefix branching).

Comment thread src/generated/graphql.tsx
};

/** A product-catalog plan */
export type CatalogPlan = {

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — This regeneration carries schema drift unrelated to payments: CatalogPlan, createCatalogPlan/updateCatalogPlan changing from Plan to CatalogPlan, new AccountTree and CatalogPlan enum members, a taxCodes input field, and ... on CatalogPlan { id } added to the activity-log documents. Please regenerate against the schema this PR targets so the generated diff holds only the payments filter arguments and PaymentCollectionMetadata — otherwise it conflicts with whichever PR owns the catalog changes.

@@ -0,0 +1,67 @@
# Payments filters QA

@domenicofalco domenicofalco Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — This directory adds ~1.37 MB to git history permanently: payments-filters.webm (794 KB), three PNGs (~527 KB), a 1285-line ui-qa.json, plus cli-comparison-ui.json and cross-client-qa.json. The evidence is useful — please move it to the PR description or an attachment and strip the directory from the branch (.gitignore has no rule for qa/ or .webm today, so it's worth adding one). Or just delete such folder unless you have some specific action in mind.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would recommend to melt those test in our existing test structure, following our implementation examples, and completely delete this new qa/* folder.
If you can prove that those assertions cannot be replicated using our jest or cypress test suite for some reasons, we could consider evolving our test structure

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