chore(): minor fixes around codebase
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user