fix(experiments): sort experiment list by result and creator#17
Open
lordspline wants to merge 1 commit into
Open
fix(experiments): sort experiment list by result and creator#17lordspline wants to merge 1 commit into
lordspline wants to merge 1 commit into
Conversation
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> Sorting the Experiments list by the Result column sent order=conclusion, which was not allowlisted (400 Invalid order field), and sorting by Created by sent order=created_by, whose Coalesce annotation mixed CharField and EmailField without an explicit output_field (500 FieldError). Allow conclusion ordering via a fixed Case ranking that mirrors the frontend Result sorter (won < lost < inconclusive < stopped_early < invalid, no conclusion last) so server-side ordering stays consistent across paginated pages, and give the created_by Coalesce an explicit output_field. Strengthen the order regression test to evaluate the queryset so annotation errors surface, and add asc/desc, null-creator/conclusion, pagination, and URL-state coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Sorting the Experiments list by the Result or Created by column broke list loading, as reported in PostHog/posthog#63621:
order=conclusion, which was not on the experiment order allowlist →400 Invalid order field: 'conclusion'.order=created_by, whose ordering annotation built aCoalesce(first_name, email)over aCharFieldand anEmailFieldwithout an explicitoutput_field→FieldError: Expression contains mixed types: CharField, EmailField→500("A server error occurred").The
created_byfailure only surfaced when the queryset was actually evaluated, so the existing allowlist test (which never evaluated the queryset) passed while the endpoint 500'd. On small datasets the 500 was further masked in the UI becauseLemonTable's client-sidesorterre-sorts the already-loaded page — i.e. it looked sorted while the global, server-side request had failed.Changes
All changes are in the experiment service ordering logic (no viewset/serializer or schema change, so no generated-type regeneration is required):
created_byordering: addoutput_field=CharField()to theCoalesce, fixing the mixed-typeFieldError.conclusion("Result") ordering: allowlistconclusion/-conclusionand order by a fixedCaseranking (won<lost<inconclusive<stopped_early<invalid, with no conclusion last). This mirrors the frontend Result sorter so server-side ordering is correct globally across pages, rather than only re-sorting the current page client-side.Other valid sortable columns (name, created/updated/start/end dates, duration, status) are unchanged.
No visual UI change — the table renders identically; it simply stops erroring and now returns a globally-correct order.
How did you test this code?
I'm an agent (Captain Capy). I ran the following locally and verified behavior end to end:
Automated tests (
hogli test):products/experiments/backend/test/test_experiment_service.py— new/updated coverage: conclusion asc/desc by ranking, conclusion ordering global across pagination slices,created_byasc/desc (forcing queryset evaluation to catch the mixed-type regression), null creator and null conclusion handling, blank-first_name→ email fallback, and the existingtest_order_by_valid_fields_worksstrengthened to evaluate the queryset.frontend/src/scenes/experiments/experimentsLogic.test.ts— new coverage:orderforwarded to the API and reset to page 1 forconclusion,-conclusion,created_by,-created_by, and an unaffected-created_at; order synced to / restored from URL search params; order omitted from params when unset. 46/46 suite tests pass.rufflint/format andty checkpass (via pre-commit) on the changed Python;oxlint/oxfmton the changed TS.Manual browser verification against the local stack with seeded experiments (varied conclusions and two creators):
Load experiments failed: Invalid order field: 'conclusion'; the API returned400fororder=conclusionand500fororder=created_by.200fororder=conclusion,-conclusion,created_by,-created_by, with the ranking/creator ordering verified directly.Automatic notifications
Docs update
No docs change required.
🤖 Agent context
Autonomy: Human-driven (agent-assisted) — directed by Miles Morales.
Authored by Captain Capy. Investigated the LemonTable sorting contract (columns provide client-side
sorters and a server-sideonSortthat pushesorder=<columnKey>), the experiments logic/URL state, and the service-layer order allowlist. Confirmed both failures by evaluating the queryset directly (400 forconclusion,FieldError/500 forcreated_by).Chose to fix server-side ordering (rather than dropping the columns to client-only sort) so ordering stays correct across the 100/page server pagination — explicitly avoiding the "sort only the current page but present it as global" trap, which is what masked the
created_by500 in the UI. Kept the change in the service layer to avoid an OpenAPI/types regeneration; theorderparameter description already enumerates fields non-exhaustively. Verified before/after in a real browser session in addition to the automated tests above.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.