Add pagination to credentials and project credentials page#4672
Add pagination to credentials and project credentials page#4672sharleenawinja wants to merge 5 commits into
Conversation
|
@midigofrank could you please review this PR when you have a moment? Thanks! |
|
Hey @sharleenawinja , thank you. I will review this today |
|
Hi @sharleenawinja, thank you for this! There are two tests failing in CI, I think just because some of the HTML structures have changed. Would you mind taking a look before Frank reviews this properly? You can see the errors here: https://app.circleci.com/pipelines/github/OpenFn/lightning/17326/workflows/2b9ed83e-0bac-4f14-9180-2958451d29ac/jobs/93782/tests |
midigofrank
left a comment
midigofrank
left a comment
There was a problem hiding this comment.
Hey @sharleenawinja , thank you for picking this up, you've picked a tough one, pagination in a page with multiple lists is an unchartered territory in lightning.
The credentials page is gnarly, it has multiple lists and the lists are shared in both the credentials page and the project settings page.
I see that you've chosen to do the pagination in the UI, I think that's understandable given the complexity of the page. I'm just not sure if that's the direction we want to take given that every other place does pagination using db queries. @elias-ba what do you think about this approach?
I also noticed that you created a new pagination component, could you try and reuse the one in https://github.com/OpenFn/lightning/blob/main/lib/lightning_web/pagination.ex ?
Also, test this in both pages (project settings and credentials page)
|
@midigofrank Thanks for the context. I'll rework the pagination to use DB queries to stay consistent with the rest of the app, and I'll reuse the existing pagination component instead of the one I added. I'll update the PR once that's done. |
sharleenawinja
force-pushed
the
credentials-pagination
branch
from
d0a4068 to
a4ab2f0
Compare
|
@josephjclark @midigofrank @elias-ba I have switched to DB-level pagination and reused the existing pagination_bar component. Each table now paginates via URL params. For the pipeline, the elixir tests job is failing due to pre-existing flaky tests. |
elias-ba
left a comment
elias-ba
left a comment
There was a problem hiding this comment.
Hey @sharleenawinja, really nice work on this 🙌 I pulled it down, ran the credentials and oauth suites and read through it carefully. It reads well, and the scoping and tests look right to me.
Nothing blocking from me. A few things I'd love us to look at, mostly questions:
- A bit of duplication I'd like to consolidate (inline). The display-mapping helpers are now copy-pasted into both
index.exandsettings.ex; they used to live once inside the shared component. This is the one I'd most like tidied. - The paginater change (inline). Works, but I think we can name the actual Ecto struct instead of matching a bare map, and it's worth a test since it's a shared file.
- A couple of smaller things (inline): the repeated empty-page maps, the
Map.putdisplay fields, and a question about whether create/edit reliably refreshes the list now thatclose_active_modalno longer reloads.
Honestly it's close. Lift the duplicated helpers into one home and I'm happy. Thanks for taking this one on 🙏
| end | ||
| end | ||
|
|
||
| defp map_credentials(%Scrivener.Page{} = page) do |
There was a problem hiding this comment.
This block (map_credentials / add_credential_display_fields / map_oauth_clients / add_oauth_client_display_fields) is now identical to the one in credential_live/index.ex. It lived in one place inside CredentialIndexComponent before the refactor pulled loading up into the two parents. Could we lift these into a shared home so they don't drift, maybe Credentials.with_display_fields/1 in the context, or a small helper both LiveViews call? Non-blocking, but it's the thing I'd most like sorted before merge.
| active_menu_item: :settings, | ||
| can_receive_failure_alerts: can_receive_failure_alerts, | ||
| collaborators_to_invite: [], | ||
| credentials_page: %{ |
There was a problem hiding this comment.
Small one: these placeholder pages are hand-written maps (and there's another as @empty_page in CredentialIndexComponent), while the real values from apply_action are %Scrivener.Page{} structs. Safe today since :index always overwrites before render, but it's easy for the shapes to drift apart. Maybe one empty_page() helper, or just %Scrivener.Page{page_number: 1}, so there's a single source of truth?
| %{ | ||
| group_bys: [ | ||
| %Ecto.Query.QueryExpr{ | ||
| %{ |
There was a problem hiding this comment.
This one I'd like to think through with you. Loosening %Ecto.Query.QueryExpr{} to a bare %{expr: ...} works, but it now matches any map with an expr key rather than the actual struct. I think the precise fix is %Ecto.Query.ByExpr{}, that's what group_by builds in Ecto 3.13, which keeps the type guard. It's also a shared file every grouped-paginated query flows through, with no test hitting it directly. Could we name the struct and add a small test for the grouped-count path? More a robustness worry than a blocker.
| socket | ||
| |> assign(active_modal: nil, credential: nil, oauth_client: nil) | ||
| |> load_credentials()} | ||
| |> assign(active_modal: nil, credential: nil, oauth_client: nil)} |
There was a problem hiding this comment.
More of a question than a problem. close_active_modal used to reload the lists; now it just clears the modal, so a create/edit only appears if the save handler remembers to push_patch to return_to (the delete paths now do). It works in the tests, I just want to be sure the create-then-close path always routes back through handle_params so the new row shows up, rather than leaning on each handler to remember. Did the new-credential flow refresh cleanly when you tested it?
| credential |> Map.get(:credential_bodies, []) |> Enum.map(& &1.name) | ||
|
|
||
| credential | ||
| |> Map.put(:project_names, project_names) |
There was a problem hiding this comment.
Non-blocking, and carried over from the old component rather than something you introduced: we're adding :project_names/:environment_names onto the %Credential{} struct with Map.put, which quietly turns it into a struct with keys that aren't in the schema. Fine for display, but since we're already reshaping this, deriving them in the template (or a small view struct) would keep the schema honest. Happy to leave it for a follow-up.
|
Oh, and the red |
4 participants
Description
This PR adds pagination to the credentials, keychain credentials, and OAuth clients tables on the credentials list pages (10 items per page, with Previous/Next control). OAuth clients are now in a collapsible section, collapsed by default. Tables are ordered: credentials first, keychain credentials second, OAuth clients last.
Closes #4301
Validation steps
Go to
/credentialswith more than 10 credentials — confirm the table paginatesConfirm the OAuth Clients section is collapsed by default and the toggle shows/hides it.
If there are more than 10 OAuth clients, expand the section and confirm pagination works.
Verify the same behaviour on Project Settings → Credentials.
Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)