feat: server-side pagination and search for contract codes#3530
Conversation
OpenAPI Changes3 changes: 0 error, 3 warning, 0 info Unexpected changes? Ensure your branch is up-to-date with |
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
@daniellefrappier18 this worked well. I left some small suggestions—I think they are all worth doing [with caveat on backend CSV generation...could be followup, and maybe you and Dan discussed it already.]
| setTimeout(() => { | ||
| setSearchAnnouncement(`${count} result${count !== 1 ? "s" : ""}`) | ||
| }, 0) | ||
| const handleExportCsv = async () => { |
There was a problem hiding this comment.
Two comments:
- I had thought the plan was to build the CSV on the backend. Is that still the long term plan, or did we change that? IMO, backend generation is still the better long-term plan:
- with the backend implementation, I'm imagining a single link
<a href="/mitxonline/api/v0/b2b/manager/organizations/{org_id}/contracts/{contract_id}/codes/export.csv" download> Export CSV </a>... very simple on the frontend side, and Python has good builtin tools for building the CSV - We discussed an org with several thousand members...9k was the number I remember. This would be 18 serial frontend queries. That's not fast, plus any one of them could fail for transient network issues.
- with the backend implementation, I'm imagining a single link
- If you leave it here, I would lean toward doing
queryClient.fetchQuery(managerOrganizationQueries.managerContractCodes(...params))rather than usingb2bApiclient directly.- This is the only direct use of an axios API client outside the react-query layer in the entire codebase, so it's a bit of a pattern change
- going through react query has some advantages, like retrying with exponential backoff (up to max 3 retries) if a query fails due to transient network errors
@dsubak what would you think of shifting the CSV generation to the backend? [Alternatively, I think I missed a meeting on the admin dashboard, so if you two decided to put it here, OK!]
There was a problem hiding this comment.
@ChristopherChudzicki @daniellefrappier18
Danielle and I discussed it in person - I'm happy to give a bespoke export CSV projection if we'd like but I think it'd end up being pretty close to syntactic sugar over similar functionality to /codes w/ filtering and an arbitrarily large page_size.
- I believe we are assuming that export would want to respect filters. It might be weird if you had it filtered to a specific status and search term and you hit export and got a huge superset of records in your export. Please correct me if this is not the case.
- On the backend I deliberately don't enforce a maximum page size in the pagination configuration to allow for pulling every row in one request, so my thinking was that we could specify a page size == max_learners (as returned by the summary stats in the
/contract/{id}request we use elsewhere on the page).
With those in mind, we were basically like "It's essentially just /codes with a different serialization format" when you look at the inputs, outputs and behaviors needed. I do think we ought to strike the pagination from this function - as far as I understand it there shouldn't be the need for more than one request - but otherwise I don't think a bespoke endpoint for csv export is hugely valuable at this time.
That all having been said, if either 1 or 2 does not hold anymore, that'd push me towards a bespoke endpoint. Absolutely let me know if my assumptions are now invalid!
There was a problem hiding this comment.
(Also, ALL of this assumes an export fits nicely in memory in a single web request - if we needed to do the 10 ton solve of an async job to export stuff to s3 and allow ya'll to retrieve it by subsequent API request, whole thing would need to look different. I think this is understood by everyone at this point, but wanted to be explicit)
There was a problem hiding this comment.
Discussed offline. Key takeaways are as follows!
- For now, the exported CSV will provide an unfiltered output. We might revisit this in the future, so any backend APIs we stand up should consider retaining filtering, but for now we're just gonna hand em everything.
- We're going to keep CSV export on the frontend for now. I'll file a ticket to stand up a tiny endpoint to move it to the backend; that endpoint should function really similarly to /codes (potentially including filtering), but we'll be able to drop the page_size arg and can use python csv builtins for actual payload construction. We can adopt that at any point in the future without introducing a breaking change.
There was a problem hiding this comment.
@ChristopherChudzicki @dsubak As discussed for now we are going to keep the CSV export on the frontend with the plan to move the CSV generation to the backend in the future. I have swapped using the b2bApi client directly in favor of queryClient.fetchQuery
dsubak
left a comment
There was a problem hiding this comment.
Looks pretty good to me - I was able to test everything listed and didn't find any issues.
I think we ought to sync up and make sure we're on the same page w/r/t CSV export (per Chris' comment thread), but once we've chased that to ground, I'd be happy to run this back through its paces and provide an approval!
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Working well! Thanks for changing the query client
One tiny request... https://github.com/mitodl/mit-learn/pull/3530/changes#r3499608315 IMO, no need for a re-review after client bump + that change.
What are the relevant tickets?
Description (What does it do?)
Summary
contractDetailAPI query for per-status seat counts (total, unassigned, assigned, redeemed) instead of deriving them by filtering the full codes list client-sideScreen Recording (if appropriate):
Screen.Recording.2026-06-26.at.2.03.14.PM.mov
Using NVDA
pagination_NVDA.mp4
How can this be tested?
Additional Context