fix(experiments): allow sorting experiments list by conclusion#72138
Open
ayaangazali wants to merge 1 commit into
Open
fix(experiments): allow sorting experiments list by conclusion#72138ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
ayaangazali
force-pushed
the
fix/experiments-list-sort-conclusion
branch
from
July 18, 2026 11:31
c7c99a0 to
da43e28
Compare
ayaangazali
marked this pull request as ready for review
July 18, 2026 21:43
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 column breaks the page. The column's sort key is
conclusion, so clicking it makes the frontend requestorder=conclusion, but the list endpoint's order allowlist never included that field. The API responds withInvalid order field: 'conclusion'and the list fails to load.The issue also reports a 500 when sorting by Created By. That looks like the bug already fixed in #60532 (
created_byordering now goes through a display-name annotation), and I could not reproduce it on master, so this PR only fixes the conclusion half.Closes #63621
Changes
conclusion/-conclusiontoEXPERIMENT_ORDER_ALLOWLISTinExperimentServiceCase/Whenrank that matches the frontend Result column's client-side sorter (won, lost, inconclusive, stopped early, invalid, then experiments without a conclusion), the same pattern the existingstatusordering usesorderparameter description on the list endpoint (it was also missingcreated_by) and regenerate the OpenAPI-derived types, which only changes that description string in 3 generated filesHow did you test this code?
test_filter_experiments_queryset_orders_by_conclusion(ascending + descending). The fixture set (won, lost, invalid, no conclusion) is picked so plain alphabetical ordering would fail the assertion, and it checks that experiments without a conclusion sort last ascending. That's the regression no existing test catches: dropping the allowlist entry or the rank mapping breaks Result sorting again.conclusion/-conclusionto the existing parameterizedtest_order_by_valid_fields_works.ValidationError) and pass with it.products/experiments/backend/test/test_experiment_service.pypasses locally (471 tests).hogli ci:preflight --fixgreen, repo-widemypy --cache-fine-grained .clean.Automatic notifications
Docs update
No docs under
docs/describe experiments list ordering; the API parameter description is updated in code and flows into the generated docs.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills invoked:
/writing-testsand/improving-drf-endpoints. Main decision: sort by a rank that mirrors the frontend column's own comparator instead of ordering alphabetically on the raw column, mirroring howstatusordering is implemented. I also considered removing the column's sorter on the frontend instead, but the column is clearly meant to be sortable. The OpenAPI regen was needed because theorderparameter description is embedded in the generated types.small disclaimer: i'm a freshman in college trying to contribute properly to projects i use. claude code did a lot of the heavy lifting here and i went through the logic and the tests myself, so if anything looks off i would honestly love to hear what i missed :)