diff --git a/src/backend/api/v1/events/endpoints.py b/src/backend/api/v1/events/endpoints.py index 8c55217..91f636a 100644 --- a/src/backend/api/v1/events/endpoints.py +++ b/src/backend/api/v1/events/endpoints.py @@ -5,7 +5,6 @@ from django.http import Http404, HttpRequest from ninja import Router from prometheus_client import Counter -from api.v1.auth.endpoints import jwt_bearer from api.v1.events.schemas import ( EventErrorOut, EventTypeCreateIn, @@ -21,6 +20,7 @@ from apps.events.services import ( event_type_update, process_events_batch, ) +from apps.users.auth.bearer import jwt_bearer EVENTS_INGESTED = Counter( "lotty_events_ingested_total", @@ -62,9 +62,7 @@ def list_event_types( is_active: bool | None = None, # noqa: FBT001 ) -> tuple[int, list[EventTypeOut]]: qs = event_type_list(is_active=is_active) - return HTTPStatus.OK, [ - EventTypeOut.model_validate(metric) for metric in qs - ] + return HTTPStatus.OK, [EventTypeOut.model_validate(et) for et in qs] @router.get( diff --git a/src/backend/api/v1/notifications/endpoints.py b/src/backend/api/v1/notifications/endpoints.py index 5928be4..3596547 100644 --- a/src/backend/api/v1/notifications/endpoints.py +++ b/src/backend/api/v1/notifications/endpoints.py @@ -132,7 +132,7 @@ def create_rule( try: experiment = Experiment.objects.get(pk=payload.experiment_id) except Experiment.DoesNotExist: - raise Http404 from Experiment.DoesNotExist + raise Http404 from None r = rule_create( event_type=payload.event_type, diff --git a/src/backend/api/v1/reports/endpoints.py b/src/backend/api/v1/reports/endpoints.py index 8a05d3e..ae98fee 100644 --- a/src/backend/api/v1/reports/endpoints.py +++ b/src/backend/api/v1/reports/endpoints.py @@ -1,14 +1,15 @@ from http import HTTPStatus from uuid import UUID +from django.core.exceptions import ValidationError from django.http import Http404, HttpRequest from django.utils.dateparse import parse_datetime from ninja import Router -from api.v1.auth.endpoints import jwt_bearer from api.v1.reports.schemas import ExperimentReportOut from apps.experiments.models import Experiment from apps.reports.services import build_experiment_report +from apps.users.auth.bearer import jwt_bearer router = Router(tags=["reports"], auth=jwt_bearer) @@ -34,11 +35,15 @@ def get_experiment_report( if start_date: parsed_start = parse_datetime(start_date) if parsed_start is None: - raise Http404 + raise ValidationError( + {"start_date": "Must be a valid ISO 8601 datetime."} + ) if end_date: parsed_end = parse_datetime(end_date) if parsed_end is None: - raise Http404 + raise ValidationError( + {"end_date": "Must be a valid ISO 8601 datetime."} + ) report_data = build_experiment_report( experiment=experiment, diff --git a/src/backend/api/v1/reviews/endpoints.py b/src/backend/api/v1/reviews/endpoints.py index 5d6323b..4a57dfd 100644 --- a/src/backend/api/v1/reviews/endpoints.py +++ b/src/backend/api/v1/reviews/endpoints.py @@ -247,7 +247,7 @@ def remove_approver_from_group( description=( "Retrieve the approver group assigned to a specific experimenter. " "Returns 404 if no explicit group exists. " - "Available to any authenticated user." + "Admin only." ), ) @require_admin diff --git a/src/backend/apps/events/services.py b/src/backend/apps/events/services.py index a1a8e13..629d622 100644 --- a/src/backend/apps/events/services.py +++ b/src/backend/apps/events/services.py @@ -132,7 +132,8 @@ def _process_exposure_event( ) decision = decision_get(decision_id) - if decision: + has_decision = decision is not None + if has_decision: with suppress(ConflictError): Exposure.objects.create( decision_id=decision_id, @@ -149,10 +150,11 @@ def _process_exposure_event( subject_id=subject_id, timestamp=timestamp, properties=event_data.get("properties", {}), - is_attributed=True, + is_attributed=has_decision, ) - _promote_pending_events(decision_id) + if has_decision: + _promote_pending_events(decision_id) def _promote_pending_events(decision_id: str) -> None: diff --git a/src/backend/apps/experiments/services.py b/src/backend/apps/experiments/services.py index e43d49b..50d9bf8 100644 --- a/src/backend/apps/experiments/services.py +++ b/src/backend/apps/experiments/services.py @@ -339,6 +339,7 @@ def experiment_submit_for_review( def experiment_approve( *, experiment: Experiment, approver: User, comment: str = "" ) -> Experiment: + experiment = Experiment.objects.select_for_update().get(pk=experiment.pk) if experiment.status != ExperimentStatus.IN_REVIEW: raise ValidationError( {"status": "Experiment must be in 'in_review' status to approve."} @@ -386,6 +387,15 @@ def experiment_approve( def experiment_reject( *, experiment: Experiment, user: User, comment: str = "" ) -> Experiment: + if not can_user_approve_experimenter(user, experiment.owner): + raise ValidationError( + { + "user": ( + f"User '{user.username}' is not authorized to " + f"reject experiments for '{experiment.owner.username}'." + ) + } + ) experiment = _transition( experiment, ExperimentStatus.REJECTED, @@ -401,6 +411,15 @@ def experiment_reject( def experiment_request_changes( *, experiment: Experiment, user: User, comment: str = "" ) -> Experiment: + if not can_user_approve_experimenter(user, experiment.owner): + raise ValidationError( + { + "user": ( + f"User '{user.username}' is not authorized to " + f"request changes for '{experiment.owner.username}'." + ) + } + ) experiment.approvals.all().delete() return _transition( experiment, diff --git a/src/backend/apps/guardrails/services.py b/src/backend/apps/guardrails/services.py index cced063..e0c9ac1 100644 --- a/src/backend/apps/guardrails/services.py +++ b/src/backend/apps/guardrails/services.py @@ -28,11 +28,6 @@ from apps.notifications.services import ( ) from apps.reports.services import calculate_metric_value -_ACTION_SEVERITY = { - GuardrailAction.ROLLBACK: 0, - GuardrailAction.PAUSE: 1, -} - @transaction.atomic def guardrail_create( @@ -69,6 +64,7 @@ def guardrail_update( guardrail: Guardrail, **fields: Any, ) -> Guardrail: + guardrail.experiment.refresh_from_db(fields=["status"]) if guardrail.experiment.status in STARTED_STATUSES: raise ValidationError( { @@ -96,6 +92,7 @@ def guardrail_update( def guardrail_delete(*, guardrail: Guardrail) -> None: + guardrail.experiment.refresh_from_db(fields=["status"]) if guardrail.experiment.status in STARTED_STATUSES: raise ValidationError( { @@ -171,7 +168,7 @@ def _execute_guardrail_action( guardrail: Guardrail, experiment: Experiment, actual_value: Decimal, -) -> GuardrailTrigger: +) -> GuardrailTrigger | None: now = timezone.now() experiment = Experiment.objects.select_for_update().get(pk=experiment.pk) diff --git a/src/backend/apps/learnings/services.py b/src/backend/apps/learnings/services.py index 3c63d06..204f66b 100644 --- a/src/backend/apps/learnings/services.py +++ b/src/backend/apps/learnings/services.py @@ -8,6 +8,7 @@ from django.contrib.postgres.search import ( SearchRank, SearchVector, ) +from django.core.exceptions import ValidationError from django.db import connection, transaction from django.db.models import Q, QuerySet @@ -60,7 +61,7 @@ def learning_update( changes: dict[str, Any] = {} for key in fields: if key not in allowed: - raise ValueError(f"Field '{key}' cannot be updated.") + raise ValidationError({key: f"Field '{key}' cannot be updated."}) for key, value in fields.items(): if value is not None: old_value = getattr(learning, key) diff --git a/src/backend/apps/learnings/tests/test_learnings.py b/src/backend/apps/learnings/tests/test_learnings.py index 917230a..58d3c87 100644 --- a/src/backend/apps/learnings/tests/test_learnings.py +++ b/src/backend/apps/learnings/tests/test_learnings.py @@ -1,6 +1,7 @@ import uuid from typing import override +from django.core.exceptions import ValidationError from django.test import TestCase from apps.experiments.tests.helpers import ( @@ -146,7 +147,7 @@ class LearningUpdateTest(TestCase): self.assertEqual(edits.count(), 0) def test_update_disallowed_field(self) -> None: - with self.assertRaises(ValueError): + with self.assertRaises(ValidationError): learning_update( learning=self.learning, user=self.user, diff --git a/src/backend/apps/metrics/services.py b/src/backend/apps/metrics/services.py index cbf31af..718696f 100644 --- a/src/backend/apps/metrics/services.py +++ b/src/backend/apps/metrics/services.py @@ -53,6 +53,22 @@ def _validate_calculation_rule( ) } ) + rule_type = rule.get("type") + if rule_type and rule_type != metric_type: + raise ValidationError( + { + "calculation_rule": ( + f"Rule type '{rule_type}' does not match " + f"metric_type '{metric_type}'." + ) + } + ) + if metric_type == MetricType.PERCENTILE: + percentile = rule.get("percentile") + if percentile is not None and not (0 <= percentile <= 100): + raise ValidationError( + {"calculation_rule": ("Percentile must be between 0 and 100.")} + ) @transaction.atomic diff --git a/src/backend/apps/metrics/tests/test_metrics.py b/src/backend/apps/metrics/tests/test_metrics.py index 9fc3816..9aa27ac 100644 --- a/src/backend/apps/metrics/tests/test_metrics.py +++ b/src/backend/apps/metrics/tests/test_metrics.py @@ -1,16 +1,8 @@ -from decimal import Decimal from django.core.exceptions import ValidationError from django.test import TestCase -from apps.experiments.services import ( - experiment_approve, - experiment_start, - experiment_submit_for_review, -) -from apps.experiments.tests.helpers import add_two_variants, make_experiment -from apps.guardrails.models import GuardrailAction -from apps.guardrails.services import guardrail_create +from apps.experiments.tests.helpers import make_experiment from apps.metrics.models import ExperimentMetric, MetricDirection, MetricType from apps.metrics.services import ( experiment_metric_add, @@ -19,8 +11,6 @@ from apps.metrics.services import ( metric_definition_create, metric_definition_update, ) -from apps.reviews.services import review_settings_update -from apps.reviews.tests.helpers import make_approver from config.errors import ConflictError diff --git a/src/backend/apps/reports/tests/test_reports.py b/src/backend/apps/reports/tests/test_reports.py index fbb4fd9..f7f576a 100644 --- a/src/backend/apps/reports/tests/test_reports.py +++ b/src/backend/apps/reports/tests/test_reports.py @@ -244,8 +244,8 @@ class CalculateMetricValueTest(TestCase): variant_id=self.v_treatment.pk, ) self.assertIsNotNone(value) - self.assertGreaterEqual(value, Decimal("90")) - self.assertLessEqual(value, Decimal("100")) + self.assertGreaterEqual(value, Decimal(90)) + self.assertLessEqual(value, Decimal(100)) def test_percentile_no_data_returns_none(self) -> None: metric = metric_definition_create(