Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions frontend/src/scenes/experiments/experimentsLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
29 changes: 28 additions & 1 deletion products/experiments/backend/experiment_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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:
Expand Down
104 changes: 103 additions & 1 deletion products/experiments/backend/test/test_experiment_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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."""
Expand Down