From 210cfc6367601cc26b318ba1a7d28fc3a017bbcb Mon Sep 17 00:00:00 2001 From: lordspline <74811063+lordspline@users.noreply.github.com> Date: Wed, 17 Jun 2026 22:10:12 +0000 Subject: [PATCH] fix(experiments): sort experiment list by result and creator 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. --- .../experiments/experimentsLogic.test.ts | 46 ++++++++ .../experiments/backend/experiment_service.py | 29 ++++- .../backend/test/test_experiment_service.py | 104 +++++++++++++++++- 3 files changed, 177 insertions(+), 2 deletions(-) diff --git a/frontend/src/scenes/experiments/experimentsLogic.test.ts b/frontend/src/scenes/experiments/experimentsLogic.test.ts index cfcc035c8b7b..3a9944249f06 100644 --- a/frontend/src/scenes/experiments/experimentsLogic.test.ts +++ b/frontend/src/scenes/experiments/experimentsLogic.test.ts @@ -320,6 +320,52 @@ describe('experimentsLogic', () => { }) }) + describe('sorting and order URL state', () => { + beforeEach(() => { + api.get.mockClear() + }) + + // The Result and Created by columns previously broke list loading; cover them + // alongside an unaffected valid column for both sort directions. + it.each(['conclusion', '-conclusion', 'created_by', '-created_by', '-created_at'])( + 'forwards order=%s to the API and resets to page 1', + async (order) => { + api.get.mockClear() + + await expectLogic(logic, () => { + logic.actions.setExperimentsFilters({ order, page: 1 }) + }) + .delay(350) + .toFinishAllListeners() + + expect(logic.values.filters.order).toBe(order) + expect(logic.values.filters.page).toBe(1) + expect(logic.values.paramsFromFilters).toEqual(expect.objectContaining({ order, offset: 0 })) + expect(api.get).toHaveBeenCalledWith(expect.stringContaining(`order=${order}`)) + } + ) + + it('syncs the active order to the URL search params', async () => { + await expectLogic(logic, () => { + logic.actions.setExperimentsFilters({ order: 'conclusion', page: 1 }) + }).toFinishAllListeners() + + expect(router.values.searchParams['order']).toBe('conclusion') + }) + + it('restores the order from the URL on navigation', async () => { + await expectLogic(logic, () => { + router.actions.push(urls.experiments(), { order: '-conclusion' }) + }).toFinishAllListeners() + + expect(logic.values.filters.order).toBe('-conclusion') + }) + + it('omits order from params when no sort is active', () => { + expect(logic.values.paramsFromFilters.order).toBeUndefined() + }) + }) + describe('experiment CRUD operations', () => { beforeEach(() => { router.actions.push = jest.fn() diff --git a/products/experiments/backend/experiment_service.py b/products/experiments/backend/experiment_service.py index ff8fe57aaf1c..49164288220d 100644 --- a/products/experiments/backend/experiment_service.py +++ b/products/experiments/backend/experiment_service.py @@ -10,7 +10,7 @@ from zoneinfo import ZoneInfo from django.db import transaction -from django.db.models import Case, Count, F, Prefetch, Q, QuerySet, Value, When +from django.db.models import Case, CharField, Count, F, Prefetch, Q, QuerySet, Value, When from django.db.models.functions import Coalesce, Now, NullIf from django.utils import timezone @@ -458,8 +458,21 @@ def validate_experiment_metrics(cls, metrics: list | None) -> None: "-duration", "status", "-status", + "conclusion", + "-conclusion", } + # Fixed ranking for the experiment conclusion ("Result" column). Mirrors the + # frontend list sorter so server-side ordering stays consistent across pages: + # won < lost < inconclusive < stopped_early < invalid, with no conclusion last. + CONCLUSION_SORT_RANKING = ( + ("won", 1), + ("lost", 2), + ("inconclusive", 3), + ("stopped_early", 4), + ("invalid", 5), + ) + ELIGIBLE_FLAGS_ORDER_ALLOWLIST = { "created_at", "-created_at", @@ -2531,8 +2544,22 @@ def filter_experiments_queryset( created_by_display=Coalesce( NullIf(F("created_by__first_name"), Value("")), F("created_by__email"), + # first_name is a CharField and email an EmailField; Coalesce + # over mixed field types needs an explicit output_field. + output_field=CharField(), ) ).order_by(f"{prefix}created_by_display") + elif order_value in ["conclusion", "-conclusion"]: + # Order by the fixed conclusion ranking (not alphabetically) so the + # server matches the frontend "Result" sorter; missing conclusions sort + # last ascending (and first descending, mirroring the client sorter). + prefix = "-" if order_value.startswith("-") else "" + queryset = queryset.annotate( + conclusion_sort_key=Case( + *[When(conclusion=value, then=Value(rank)) for value, rank in self.CONCLUSION_SORT_RANKING], + default=Value(len(self.CONCLUSION_SORT_RANKING) + 1), + ) + ).order_by(f"{prefix}conclusion_sort_key") else: queryset = queryset.order_by(order_value) else: diff --git a/products/experiments/backend/test/test_experiment_service.py b/products/experiments/backend/test/test_experiment_service.py index 45007ec49ac5..5e9b18dd682c 100644 --- a/products/experiments/backend/test/test_experiment_service.py +++ b/products/experiments/backend/test/test_experiment_service.py @@ -4226,6 +4226,17 @@ def test_clone_regenerates_uuids_even_when_source_uuid_matches_saved_metric(self def _base_queryset(self): return Experiment.objects.filter(team=self.team) + def _make_ordering_experiment(self, name, *, conclusion=None, created_by="__self__"): + flag = self._create_flag(key=f"order-{name.lower().replace(' ', '-')}") + creator = self.user if created_by == "__self__" else created_by + return Experiment.objects.create( + team=self.team, + feature_flag=flag, + name=name, + conclusion=conclusion, + created_by=creator, + ) + def test_order_by_invalid_field_raises_validation_error(self): """Ordering by a non-allowlisted field should be rejected.""" service = self._service() @@ -4257,7 +4268,98 @@ def test_order_by_invalid_field_raises_validation_error(self): def test_order_by_valid_fields_works(self, order: str): service = self._service() qs = service.filter_experiments_queryset(self._base_queryset(), action="list", query_params={"order": order}) - assert qs is not None + # Force SQL evaluation so annotation errors (e.g. a mixed-type Coalesce missing + # output_field) surface here rather than as a 500 at request time. + assert list(qs.values_list("id", flat=True)) == [] + + @parameterized.expand( + [ + ("ascending", "conclusion", ["Won", "Lost", "Inconclusive", "Stopped", "Invalid", "Undecided"]), + ("descending", "-conclusion", ["Undecided", "Invalid", "Stopped", "Inconclusive", "Lost", "Won"]), + ] + ) + def test_filter_experiments_queryset_orders_by_conclusion( + self, _: str, order: str, expected_order: list[str] + ) -> None: + service = self._service() + self._make_ordering_experiment("Inconclusive", conclusion="inconclusive") + self._make_ordering_experiment("Undecided", conclusion=None) + self._make_ordering_experiment("Won", conclusion="won") + self._make_ordering_experiment("Invalid", conclusion="invalid") + self._make_ordering_experiment("Lost", conclusion="lost") + self._make_ordering_experiment("Stopped", conclusion="stopped_early") + + queryset = service.filter_experiments_queryset( + self._base_queryset(), + action="list", + query_params={"order": order}, + ) + + assert list(queryset.values_list("name", flat=True)) == expected_order + + def test_filter_experiments_queryset_orders_by_conclusion_is_global_across_pages(self) -> None: + service = self._service() + self._make_ordering_experiment("Won", conclusion="won") + self._make_ordering_experiment("Lost", conclusion="lost") + self._make_ordering_experiment("Inconclusive", conclusion="inconclusive") + self._make_ordering_experiment("Stopped", conclusion="stopped_early") + self._make_ordering_experiment("Invalid", conclusion="invalid") + self._make_ordering_experiment("Undecided", conclusion=None) + + queryset = service.filter_experiments_queryset( + self._base_queryset(), + action="list", + query_params={"order": "conclusion"}, + ) + + # Slicing the ordered queryset (how the viewset paginates) must yield a globally + # sorted result, not a per-page client sort. + assert list(queryset.values_list("name", flat=True)[0:3]) == ["Won", "Lost", "Inconclusive"] + assert list(queryset.values_list("name", flat=True)[3:6]) == ["Stopped", "Invalid", "Undecided"] + + @parameterized.expand( + [ + ("ascending", "created_by", ["Alice exp", "Bob exp", "No creator"]), + ("descending", "-created_by", ["No creator", "Bob exp", "Alice exp"]), + ] + ) + def test_filter_experiments_queryset_orders_by_created_by( + self, _: str, order: str, expected_order: list[str] + ) -> None: + service = self._service() + alice = self._create_user("alice@example.com", first_name="Alice") + bob = self._create_user("bob@example.com", first_name="Bob") + self._make_ordering_experiment("Bob exp", created_by=bob) + self._make_ordering_experiment("Alice exp", created_by=alice) + self._make_ordering_experiment("No creator", created_by=None) + + queryset = service.filter_experiments_queryset( + self._base_queryset(), + action="list", + query_params={"order": order}, + ) + + # Force evaluation — the mixed-type Coalesce regression only raised once the SQL ran. + assert list(queryset.values_list("name", flat=True)) == expected_order + + def test_filter_experiments_queryset_orders_by_created_by_falls_back_to_email(self) -> None: + service = self._service() + # A blank first_name must fall back to email instead of collapsing to an empty + # string, matching the frontend `first_name || email` sorter. + blank_name = self._create_user("zzz@example.com", first_name="") + named = self._create_user("a@example.com", first_name="mmm") + self._make_ordering_experiment("Blank exp", created_by=blank_name) + self._make_ordering_experiment("Named exp", created_by=named) + + queryset = service.filter_experiments_queryset( + self._base_queryset(), + action="list", + query_params={"order": "created_by"}, + ) + + # "mmm" < "zzz@example.com"; without the email fallback the blank name would + # collapse to "" and sort first instead. + assert list(queryset.values_list("name", flat=True)) == ["Named exp", "Blank exp"] def test_eligible_flags_order_by_invalid_field_raises(self): """Ordering eligible flags by a non-allowlisted field should be rejected."""