diff --git a/frontend/src/scenes/experiments/experimentsLogic.test.ts b/frontend/src/scenes/experiments/experimentsLogic.test.ts index 119291b7a397..297a5e0a46b4 100644 --- a/frontend/src/scenes/experiments/experimentsLogic.test.ts +++ b/frontend/src/scenes/experiments/experimentsLogic.test.ts @@ -320,6 +320,38 @@ describe('experimentsLogic', () => { archived: false, }) }) + + it.each(['conclusion', '-conclusion', 'created_by', '-created_by', 'name', '-name'])( + 'sends %s ordering globally and writes it to the URL', + (order) => { + const replace = jest.fn() + router.actions.replace = replace + + logic.actions.setExperimentsFilters({ order, page: 1 }) + + expect(logic.values.filters).toEqual(expect.objectContaining({ order, page: 1 })) + expect(logic.values.paramsFromFilters).toEqual(expect.objectContaining({ order, page: 1, offset: 0 })) + expect(replace).toHaveBeenLastCalledWith(expect.stringContaining(`order=${encodeURIComponent(order)}`)) + } + ) + + it('restores ordering and pagination from the URL', async () => { + router.actions.push(urls.experiments(), { order: '-conclusion', page: '3' }) + + await expectLogic(logic) + .toMatchValues({ + filters: expect.objectContaining({ + order: '-conclusion', + page: 3, + }), + paramsFromFilters: expect.objectContaining({ + order: '-conclusion', + page: 3, + offset: 200, + }), + }) + .toFinishAllListeners() + }) }) describe('experiment CRUD operations', () => { diff --git a/products/experiments/backend/experiment_service.py b/products/experiments/backend/experiment_service.py index 14d56b81eca1..b94d72a470d7 100644 --- a/products/experiments/backend/experiment_service.py +++ b/products/experiments/backend/experiment_service.py @@ -865,6 +865,8 @@ def validate_experiment_metrics(cls, metrics: list | None) -> None: "-duration", "status", "-status", + "conclusion", + "-conclusion", } @classmethod @@ -4460,11 +4462,27 @@ def filter_experiments_queryset( created_by_display=Coalesce( NullIf(F("created_by__first_name"), Value("")), F("created_by__email"), + Value(""), # first_name is a CharField and email an EmailField; Django refuses to # infer a type across the two, so set it explicitly. output_field=CharField(), ) ).order_by(f"{prefix}created_by_display") + elif order_value in ["conclusion", "-conclusion"]: + queryset = queryset.annotate( + conclusion_sort_key=Case( + When(conclusion="won", then=Value(1)), + When(conclusion="lost", then=Value(2)), + When(conclusion="inconclusive", then=Value(3)), + When(conclusion="stopped_early", then=Value(4)), + When(conclusion="invalid", then=Value(5)), + default=Value(6), + ) + ) + if order_value.startswith("-"): + queryset = queryset.order_by(F("conclusion_sort_key").desc()) + else: + queryset = queryset.order_by(F("conclusion_sort_key").asc()) else: queryset = queryset.order_by(order_value) else: diff --git a/products/experiments/backend/presentation/views.py b/products/experiments/backend/presentation/views.py index 2481895fbce5..cb57bb9f3e79 100644 --- a/products/experiments/backend/presentation/views.py +++ b/products/experiments/backend/presentation/views.py @@ -344,8 +344,8 @@ def _accessible_session_ids( location=OpenApiParameter.QUERY, type=str, description=( - "Field to order by. Prefix with '-' for descending. Allowlisted fields include name, " - "created_at, updated_at, start_date, end_date, duration, and status." + "Field to order by. Prefix with '-' for descending. Supported fields are name, created_at, " + "created_by, updated_at, start_date, end_date, duration, status, and conclusion." ), required=False, ), diff --git a/products/experiments/backend/test/test_experiment_service.py b/products/experiments/backend/test/test_experiment_service.py index d1166fb48361..004b1ab4ac89 100644 --- a/products/experiments/backend/test/test_experiment_service.py +++ b/products/experiments/backend/test/test_experiment_service.py @@ -5231,6 +5231,133 @@ def test_filter_experiments_queryset_filters_by_multiple_created_by_ids( ) assert set(queryset.values_list("name", flat=True)) == expected_names + @parameterized.expand( + [ + ( + "ascending", + "conclusion", + ["Won", "Lost", "Inconclusive", "Stopped early", "Invalid", "No conclusion"], + ), + ( + "descending", + "-conclusion", + ["No conclusion", "Invalid", "Stopped early", "Inconclusive", "Lost", "Won"], + ), + ] + ) + def test_filter_experiments_queryset_orders_by_conclusion( + self, _: str, order: str, expected_order: list[str] + ) -> None: + service = self._service() + conclusions = [ + ("No conclusion", None), + ("Invalid", "invalid"), + ("Won", "won"), + ("Stopped early", "stopped_early"), + ("Lost", "lost"), + ("Inconclusive", "inconclusive"), + ] + for index, (name, conclusion) in enumerate(conclusions): + service.create_experiment( + name=name, + feature_flag_key=f"order-conclusion-{index}", + conclusion=conclusion, + ) + + queryset = service.filter_experiments_queryset( + Experiment.objects.filter(team=self.team), + action="list", + query_params={"order": order}, + ) + + assert list(queryset.values_list("name", flat=True)) == expected_order + + @parameterized.expand( + [ + ("ascending", "created_by", ["Email fallback", "Named creator"]), + ("descending", "-created_by", ["Named creator", "Email fallback"]), + ] + ) + def test_filter_experiments_queryset_orders_by_creator_display_name( + self, _: str, order: str, expected_named_order: list[str] + ) -> None: + service = self._service() + email_user = self._create_user("alpha@example.com") + email_user.first_name = "" + email_user.save(update_fields=["first_name"]) + named_user = self._create_user("zulu@example.com") + named_user.first_name = "bravo" + named_user.save(update_fields=["first_name"]) + empty_user = self._create_user("empty@example.com") + empty_user.email = "" + empty_user.save(update_fields=["email"]) + + without_creator = service.create_experiment(name="No creator", feature_flag_key="order-no-creator") + without_creator.created_by = None + without_creator.save(update_fields=["created_by"]) + ExperimentService(team=self.team, user=empty_user).create_experiment( + name="Empty creator", feature_flag_key="order-empty-creator" + ) + ExperimentService(team=self.team, user=email_user).create_experiment( + name="Email fallback", feature_flag_key="order-email-creator" + ) + ExperimentService(team=self.team, user=named_user).create_experiment( + name="Named creator", feature_flag_key="order-named-creator" + ) + + queryset = service.filter_experiments_queryset( + Experiment.objects.filter(team=self.team), + action="list", + query_params={"order": order}, + ) + ordered_names = list(queryset.values_list("name", flat=True)) + + if order == "created_by": + assert set(ordered_names[:2]) == {"No creator", "Empty creator"} + assert ordered_names[2:] == expected_named_order + else: + assert ordered_names[:2] == expected_named_order + assert set(ordered_names[2:]) == {"No creator", "Empty creator"} + + def test_filter_experiments_queryset_orders_before_pagination(self) -> None: + service = self._service() + conclusions = ["invalid", "won", None, "stopped_early", "lost", "inconclusive"] + for index, conclusion in enumerate(conclusions): + service.create_experiment( + name=f"Experiment {conclusion}", + feature_flag_key=f"order-pagination-{index}", + conclusion=conclusion, + ) + + queryset = service.filter_experiments_queryset( + Experiment.objects.filter(team=self.team), + action="list", + query_params={"order": "conclusion"}, + ) + + assert list(queryset.values_list("conclusion", flat=True)[2:4]) == ["inconclusive", "stopped_early"] + + @parameterized.expand( + [ + ("ascending", "name", ["Alpha", "Zulu"]), + ("descending", "-name", ["Zulu", "Alpha"]), + ] + ) + def test_filter_experiments_queryset_keeps_ordering_by_name( + self, _: str, order: str, expected_order: list[str] + ) -> None: + service = self._service() + service.create_experiment(name="Zulu", feature_flag_key="order-name-zulu") + service.create_experiment(name="Alpha", feature_flag_key="order-name-alpha") + + queryset = service.filter_experiments_queryset( + Experiment.objects.filter(team=self.team), + action="list", + query_params={"order": order}, + ) + + assert list(queryset.values_list("name", flat=True)) == expected_order + @parameterized.expand( [ ("ascending", "duration", ["Short", "Long"]), @@ -6041,12 +6168,14 @@ def test_order_by_invalid_field_raises_validation_error(self): ("-duration",), ("status",), ("-status",), + ("conclusion",), + ("-conclusion",), ] ) 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 + assert list(qs[:1]) == [] def test_launch_with_deleted_flag_raises(self): """Launching an experiment whose flag is soft-deleted should fail.""" diff --git a/products/experiments/frontend/generated/api.schemas.ts b/products/experiments/frontend/generated/api.schemas.ts index ec5f6cd9a108..83140bf85cd6 100644 --- a/products/experiments/frontend/generated/api.schemas.ts +++ b/products/experiments/frontend/generated/api.schemas.ts @@ -2775,7 +2775,7 @@ export type ExperimentsListParams = { */ offset?: number /** - * Field to order by. Prefix with '-' for descending. Allowlisted fields include name, created_at, updated_at, start_date, end_date, duration, and status. + * Field to order by. Prefix with '-' for descending. Supported fields are name, created_at, created_by, updated_at, start_date, end_date, duration, status, and conclusion. */ order?: string /** diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index 1340f786de4c..ce4e827d03b5 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -85534,7 +85534,7 @@ export namespace Schemas { */ offset?: number; /** - * Field to order by. Prefix with '-' for descending. Allowlisted fields include name, created_at, updated_at, start_date, end_date, duration, and status. + * Field to order by. Prefix with '-' for descending. Supported fields are name, created_at, created_by, updated_at, start_date, end_date, duration, status, and conclusion. */ order?: string; /** diff --git a/services/mcp/src/generated/experiments/api.ts b/services/mcp/src/generated/experiments/api.ts index 735a75c1ac38..d82d9be9b0ce 100644 --- a/services/mcp/src/generated/experiments/api.ts +++ b/services/mcp/src/generated/experiments/api.ts @@ -718,7 +718,7 @@ export const ExperimentsListQueryParams = /* @__PURE__ */ zod.object({ .string() .optional() .describe( - "Field to order by. Prefix with '-' for descending. Allowlisted fields include name, created_at, updated_at, start_date, end_date, duration, and status." + "Field to order by. Prefix with '-' for descending. Supported fields are name, created_at, created_by, updated_at, start_date, end_date, duration, status, and conclusion." ), prompt_name: zod .string()