From ca0c4568624ec4586c206e1eb0d1f9637d5c8056 Mon Sep 17 00:00:00 2001 From: ITQ Date: Mon, 23 Feb 2026 11:46:52 +0300 Subject: [PATCH] refactor(); project refactor --- src/backend/api/v1/conflicts/schemas.py | 12 ++- .../v1/conflicts/tests/test_conflicts_api.py | 39 ++++++---- src/backend/api/v1/experiments/endpoints.py | 4 +- src/backend/api/v1/learnings/endpoints.py | 42 +++++----- src/backend/api/v1/learnings/schemas.py | 31 ++++---- src/backend/apps/conflicts/services.py | 10 ++- .../apps/conflicts/tests/test_conflicts.py | 27 ++----- src/backend/apps/decision/services.py | 5 +- src/backend/apps/experiments/services.py | 13 +++- .../apps/experiments/tests/test_services.py | 5 +- src/backend/apps/learnings/models.py | 4 +- src/backend/apps/learnings/services.py | 78 ++++++++++++------- .../apps/learnings/tests/test_learnings.py | 66 +++++++++------- src/backend/apps/notifications/models.py | 4 +- src/backend/apps/notifications/services.py | 46 ++--------- .../notifications/tests/test_notifications.py | 6 +- 16 files changed, 198 insertions(+), 194 deletions(-) diff --git a/src/backend/api/v1/conflicts/schemas.py b/src/backend/api/v1/conflicts/schemas.py index dbd3771..d09f341 100644 --- a/src/backend/api/v1/conflicts/schemas.py +++ b/src/backend/api/v1/conflicts/schemas.py @@ -4,7 +4,11 @@ from uuid import UUID from ninja import Field, ModelSchema, Schema -from apps.conflicts.models import ConflictDomain, ConflictPolicy, ExperimentConflictDomain +from apps.conflicts.models import ( + ConflictDomain, + ConflictPolicy, + ExperimentConflictDomain, +) class ConflictDomainOut(ModelSchema): @@ -54,7 +58,8 @@ class MembershipOut(Schema): @classmethod def from_membership( - cls, m: ExperimentConflictDomain, + cls, + m: ExperimentConflictDomain, ) -> "MembershipOut": return cls( id=m.pk, @@ -76,7 +81,8 @@ class ExperimentDomainOut(Schema): @classmethod def from_membership( - cls, m: ExperimentConflictDomain, + cls, + m: ExperimentConflictDomain, ) -> "ExperimentDomainOut": return cls( id=m.pk, diff --git a/src/backend/api/v1/conflicts/tests/test_conflicts_api.py b/src/backend/api/v1/conflicts/tests/test_conflicts_api.py index a0890a4..80e99e2 100644 --- a/src/backend/api/v1/conflicts/tests/test_conflicts_api.py +++ b/src/backend/api/v1/conflicts/tests/test_conflicts_api.py @@ -1,13 +1,18 @@ import json +import uuid from typing import override from django.test import Client, TestCase from django.urls import reverse from apps.conflicts.tests.helpers import make_domain -from apps.experiments.tests.helpers import add_two_variants, make_flag from apps.experiments.services import experiment_create -from apps.reviews.tests.helpers import make_admin, make_experimenter, make_viewer +from apps.experiments.tests.helpers import add_two_variants, make_flag +from apps.reviews.tests.helpers import ( + make_admin, + make_experimenter, + make_viewer, +) from apps.users.tests.helpers import auth_header @@ -23,12 +28,14 @@ class ConflictDomainAPITest(TestCase): def test_create_domain(self) -> None: resp = self.client.post( reverse("api-1:create_domain"), - data=json.dumps({ - "name": "checkout", - "description": "Checkout zone", - "policy": "mutual_exclusion", - "max_concurrent": 1, - }), + data=json.dumps( + { + "name": "checkout", + "description": "Checkout zone", + "policy": "mutual_exclusion", + "max_concurrent": 1, + } + ), content_type="application/json", HTTP_AUTHORIZATION=self.admin_auth, ) @@ -86,8 +93,6 @@ class ConflictDomainAPITest(TestCase): self.assertEqual(resp.status_code, 204) def test_get_nonexistent_domain(self) -> None: - import uuid - resp = self.client.get( reverse("api-1:get_domain", args=[uuid.uuid4()]), HTTP_AUTHORIZATION=self.admin_auth, @@ -114,10 +119,12 @@ class DomainExperimentAPITest(TestCase): def test_add_experiment_to_domain(self) -> None: resp = self.client.post( reverse("api-1:add_experiment_to_domain", args=[self.domain.pk]), - data=json.dumps({ - "experiment_id": str(self.exp.pk), - "priority": 5, - }), + data=json.dumps( + { + "experiment_id": str(self.exp.pk), + "priority": 5, + } + ), content_type="application/json", HTTP_AUTHORIZATION=self.admin_auth, ) @@ -134,9 +141,7 @@ class DomainExperimentAPITest(TestCase): HTTP_AUTHORIZATION=self.admin_auth, ) resp = self.client.get( - reverse( - "api-1:list_experiment_domains", args=[self.exp.pk] - ), + reverse("api-1:list_experiment_domains", args=[self.exp.pk]), HTTP_AUTHORIZATION=self.admin_auth, ) self.assertEqual(resp.status_code, 200) diff --git a/src/backend/api/v1/experiments/endpoints.py b/src/backend/api/v1/experiments/endpoints.py index 040f386..5c55ff0 100644 --- a/src/backend/api/v1/experiments/endpoints.py +++ b/src/backend/api/v1/experiments/endpoints.py @@ -214,7 +214,9 @@ def update_variant( ) -> tuple[HTTPStatus, VariantOut]: user = _get_user(request) v = get_object_or_404( - Variant.objects.select_related("experiment__flag", "experiment__owner"), + Variant.objects.select_related( + "experiment__flag", "experiment__owner" + ), pk=variant_id, experiment_id=experiment_id, ) diff --git a/src/backend/api/v1/learnings/endpoints.py b/src/backend/api/v1/learnings/endpoints.py index c3f25f8..c10d35d 100644 --- a/src/backend/api/v1/learnings/endpoints.py +++ b/src/backend/api/v1/learnings/endpoints.py @@ -36,11 +36,13 @@ def create_learning( payload: LearningCreateIn, ) -> tuple[int, LearningOut]: try: - experiment = Experiment.objects.select_related("flag").get(pk=payload.experiment_id) + experiment = Experiment.objects.select_related("flag").get( + pk=payload.experiment_id + ) except Experiment.DoesNotExist: raise Http404 from Experiment.DoesNotExist - l = learning_create( + learning = learning_create( experiment=experiment, hypothesis=payload.hypothesis, findings=payload.findings, @@ -48,8 +50,8 @@ def create_learning( context_summary=payload.context_summary, user=request.auth, ) - l = learning_get(l.pk) - return HTTPStatus.CREATED, LearningOut.from_learning(l) + learning = learning_get(learning.pk) + return HTTPStatus.CREATED, LearningOut.from_learning(learning) @router.get( @@ -72,7 +74,9 @@ def list_learnings( tag=tag, search=search, ) - return HTTPStatus.OK, [LearningOut.from_learning(l) for l in learnings] + return HTTPStatus.OK, [ + LearningOut.from_learning(learning) for learning in learnings + ] @router.get( @@ -84,10 +88,10 @@ def get_learning( request: HttpRequest, learning_id: UUID, ) -> tuple[int, LearningOut]: - l = learning_get(learning_id) - if not l: + learning = learning_get(learning_id) + if not learning: raise Http404 - return HTTPStatus.OK, LearningOut.from_learning(l) + return HTTPStatus.OK, LearningOut.from_learning(learning) @router.patch( @@ -100,16 +104,16 @@ def update_learning( learning_id: UUID, payload: LearningUpdateIn, ) -> tuple[int, LearningOut]: - l = learning_get(learning_id) - if not l: + learning = learning_get(learning_id) + if not learning: raise Http404 - l = learning_update( - learning=l, + learning = learning_update( + learning=learning, user=request.auth, **payload.dict(exclude_unset=True), ) - l = learning_get(l.pk) - return HTTPStatus.OK, LearningOut.from_learning(l) + learning = learning_get(learning.pk) + return HTTPStatus.OK, LearningOut.from_learning(learning) @router.delete( @@ -121,10 +125,10 @@ def delete_learning( request: HttpRequest, learning_id: UUID, ) -> tuple[int, None]: - l = learning_get(learning_id) - if not l: + learning = learning_get(learning_id) + if not learning: raise Http404 - learning_delete(learning=l) + learning_delete(learning=learning) return HTTPStatus.NO_CONTENT, None @@ -137,8 +141,8 @@ def list_learning_edits( request: HttpRequest, learning_id: UUID, ) -> tuple[int, list[EditOut]]: - l = learning_get(learning_id) - if not l: + learning = learning_get(learning_id) + if not learning: raise Http404 edits = learning_edit_list(learning_id) return HTTPStatus.OK, [EditOut.from_edit(e) for e in edits] diff --git a/src/backend/api/v1/learnings/schemas.py b/src/backend/api/v1/learnings/schemas.py index c1df152..877390f 100644 --- a/src/backend/api/v1/learnings/schemas.py +++ b/src/backend/api/v1/learnings/schemas.py @@ -1,4 +1,3 @@ -from datetime import datetime from typing import Any, ClassVar from uuid import UUID @@ -51,26 +50,26 @@ class LearningOut(ModelSchema): ) @classmethod - def from_learning(cls, l: Learning) -> "LearningOut": + def from_learning(cls, learning: Learning) -> "LearningOut": created_by_out = None - if l.created_by: + if learning.created_by: created_by_out = CreatedByOut( - id=l.created_by.pk, - username=l.created_by.username, + id=learning.created_by.pk, + username=learning.created_by.username, ) return cls( - id=l.pk, - hypothesis=l.hypothesis, - findings=l.findings, - tags=l.tags, - context_summary=l.context_summary, - created_at=l.created_at, - updated_at=l.updated_at, + id=learning.pk, + hypothesis=learning.hypothesis, + findings=learning.findings, + tags=learning.tags, + context_summary=learning.context_summary, + created_at=learning.created_at, + updated_at=learning.updated_at, experiment=ExperimentBriefOut( - id=l.experiment.pk, - name=l.experiment.name, - status=l.experiment.status, - flag_key=l.experiment.flag.key, + id=learning.experiment.pk, + name=learning.experiment.name, + status=learning.experiment.status, + flag_key=learning.experiment.flag.key, ), created_by=created_by_out, ) diff --git a/src/backend/apps/conflicts/services.py b/src/backend/apps/conflicts/services.py index d62ffb3..83643e1 100644 --- a/src/backend/apps/conflicts/services.py +++ b/src/backend/apps/conflicts/services.py @@ -133,9 +133,7 @@ def validate_domain_conflicts(experiment: Experiment) -> None: active_count = active.count() if active_count >= domain.max_concurrent: - active_names = ", ".join( - m.experiment.name for m in active[:3] - ) + active_names = ", ".join(m.experiment.name for m in active[:3]) raise ValidationError( { "conflict_domain": ( @@ -174,7 +172,11 @@ def resolve_domain_conflict( if domain.policy == ConflictPolicy.PRIORITY: current = next( - (m for m in active_memberships if str(m.experiment_id) == str(experiment_id)), + ( + m + for m in active_memberships + if str(m.experiment_id) == str(experiment_id) + ), None, ) if not current: diff --git a/src/backend/apps/conflicts/tests/test_conflicts.py b/src/backend/apps/conflicts/tests/test_conflicts.py index c4a84de..21c0227 100644 --- a/src/backend/apps/conflicts/tests/test_conflicts.py +++ b/src/backend/apps/conflicts/tests/test_conflicts.py @@ -15,14 +15,12 @@ from apps.conflicts.selectors import ( experiment_conflict_domains, ) from apps.conflicts.services import ( - conflict_domain_create, conflict_domain_delete, conflict_domain_update, experiment_add_to_domain, experiment_remove_from_domain, experiment_update_domain_priority, resolve_domain_conflict, - validate_domain_conflicts, ) from apps.conflicts.tests.helpers import make_domain from apps.experiments.models import ExperimentStatus @@ -35,6 +33,7 @@ from apps.experiments.services import ( from apps.experiments.tests.helpers import add_two_variants, make_flag from apps.reviews.services import review_settings_update from apps.reviews.tests.helpers import make_approver, make_experimenter +from config.errors import ConflictError class ConflictDomainCRUDTest(TestCase): @@ -55,7 +54,7 @@ class ConflictDomainCRUDTest(TestCase): def test_duplicate_name_fails(self) -> None: make_domain(suffix="_dup") - with self.assertRaises(Exception): + with self.assertRaises(ConflictError): make_domain(suffix="_dup") def test_updates_domain(self) -> None: @@ -128,7 +127,7 @@ class ExperimentDomainMembershipTest(TestCase): def test_duplicate_membership_fails(self) -> None: exp = self._make_ready_experiment("_dup2") experiment_add_to_domain(experiment=exp, domain=self.domain) - with self.assertRaises(Exception): + with self.assertRaises(ConflictError): experiment_add_to_domain(experiment=exp, domain=self.domain) def test_remove_experiment_from_domain(self) -> None: @@ -307,12 +306,8 @@ class ResolveDomainConflictTest(TestCase): ) exp_low = self._make_and_start("_pr1", domain, priority=1) exp_high = self._make_and_start("_pr2", domain, priority=10) - self.assertTrue( - resolve_domain_conflict(exp_high.pk, domain.pk, "u1") - ) - self.assertFalse( - resolve_domain_conflict(exp_low.pk, domain.pk, "u1") - ) + self.assertTrue(resolve_domain_conflict(exp_high.pk, domain.pk, "u1")) + self.assertFalse(resolve_domain_conflict(exp_low.pk, domain.pk, "u1")) def test_priority_tie_first_created_wins(self) -> None: domain = make_domain( @@ -322,16 +317,10 @@ class ResolveDomainConflictTest(TestCase): ) exp1 = self._make_and_start("_tie1", domain, priority=5) exp2 = self._make_and_start("_tie2", domain, priority=5) - self.assertTrue( - resolve_domain_conflict(exp1.pk, domain.pk, "u1") - ) - self.assertFalse( - resolve_domain_conflict(exp2.pk, domain.pk, "u1") - ) + self.assertTrue(resolve_domain_conflict(exp1.pk, domain.pk, "u1")) + self.assertFalse(resolve_domain_conflict(exp2.pk, domain.pk, "u1")) def test_single_experiment_always_wins(self) -> None: domain = make_domain(suffix="_single") exp = self._make_and_start("_s", domain) - self.assertTrue( - resolve_domain_conflict(exp.pk, domain.pk, "u1") - ) + self.assertTrue(resolve_domain_conflict(exp.pk, domain.pk, "u1")) diff --git a/src/backend/apps/decision/services.py b/src/backend/apps/decision/services.py index fd3fe95..1563ebf 100644 --- a/src/backend/apps/decision/services.py +++ b/src/backend/apps/decision/services.py @@ -8,6 +8,7 @@ from django.core.cache import cache from django.utils import timezone from prometheus_client import Counter +from apps.conflicts.models import ExperimentConflictDomain from apps.conflicts.services import resolve_domain_conflict from apps.events.models import Decision from apps.events.services import decision_create @@ -145,8 +146,6 @@ def _check_participation_limits( def _check_domain_conflicts(experiment: Experiment) -> bool: - from apps.conflicts.models import ExperimentConflictDomain - memberships = ExperimentConflictDomain.objects.filter( experiment=experiment, ).select_related("conflict_domain") @@ -271,7 +270,7 @@ def decide_for_flag( "variant", ) total_weight = sum(v.weight for v in variants) - normalized_hash = variant_hash * total_weight / Decimal("100") + normalized_hash = variant_hash * total_weight / Decimal(100) selected = _select_variant(variants, normalized_hash) DECIDE_REQUESTS.labels(reason="experiment_assigned").inc() diff --git a/src/backend/apps/experiments/services.py b/src/backend/apps/experiments/services.py index c61eccf..b2a5594 100644 --- a/src/backend/apps/experiments/services.py +++ b/src/backend/apps/experiments/services.py @@ -4,6 +4,7 @@ from typing import Any from django.core.exceptions import ValidationError from django.db import transaction +from apps.conflicts.services import validate_domain_conflicts from apps.experiments.models import ( ACTIVE_STATUSES, ALLOWED_TRANSITIONS, @@ -16,7 +17,6 @@ from apps.experiments.models import ( OutcomeType, Variant, ) -from apps.conflicts.services import validate_domain_conflicts from apps.flags.models import FeatureFlag, validate_value_for_type from apps.notifications.services import ( NotificationPayload, @@ -30,12 +30,19 @@ from apps.users.models import User from config.errors import ForbiddenError -def _notify(event_type: str, experiment: Experiment, extra: dict[str, Any] | None = None) -> None: +def _notify( + event_type: str, + experiment: Experiment, + extra: dict[str, Any] | None = None, +) -> None: notification_enqueue( event_type, NotificationPayload( title=f"{event_type.replace('_', ' ').title()}", - body=f"Experiment '{experiment.name}' — {event_type.replace('_', ' ')}.", + body=( + f"Experiment '{experiment.name}' — " + f"{event_type.replace('_', ' ')}." + ), event_type=event_type, experiment_id=str(experiment.pk), experiment_name=experiment.name, diff --git a/src/backend/apps/experiments/tests/test_services.py b/src/backend/apps/experiments/tests/test_services.py index ac263e9..7532fa5 100644 --- a/src/backend/apps/experiments/tests/test_services.py +++ b/src/backend/apps/experiments/tests/test_services.py @@ -14,7 +14,6 @@ from apps.experiments.models import ( OutcomeType, ) from apps.experiments.services import ( - ensure_owner_or_admin, experiment_approve, experiment_archive, experiment_complete, @@ -530,9 +529,7 @@ class OwnershipPermissionTest(TestCase): def test_other_experimenter_cannot_submit_for_review(self) -> None: with self.assertRaises(ForbiddenError): - experiment_submit_for_review( - experiment=self.exp, user=self.other - ) + experiment_submit_for_review(experiment=self.exp, user=self.other) def test_admin_can_submit_for_review(self) -> None: exp = experiment_submit_for_review( diff --git a/src/backend/apps/learnings/models.py b/src/backend/apps/learnings/models.py index 2d8cb0a..d608c17 100644 --- a/src/backend/apps/learnings/models.py +++ b/src/backend/apps/learnings/models.py @@ -59,7 +59,9 @@ class Learning(BaseModel): ordering = ["-created_at"] indexes = [ GinIndex(fields=["search_vector"], name="idx_learning_search"), - models.Index(fields=["experiment"], name="idx_learning_experiment"), + models.Index( + fields=["experiment"], name="idx_learning_experiment" + ), ] @override diff --git a/src/backend/apps/learnings/services.py b/src/backend/apps/learnings/services.py index 7288ad2..3c63d06 100644 --- a/src/backend/apps/learnings/services.py +++ b/src/backend/apps/learnings/services.py @@ -1,11 +1,20 @@ import logging +import operator from typing import Any from uuid import UUID +from django.contrib.postgres.search import ( + SearchQuery, + SearchRank, + SearchVector, +) from django.db import connection, transaction from django.db.models import Q, QuerySet -from apps.experiments.models import Experiment, ExperimentOutcome, ExperimentStatus +from apps.experiments.models import ( + Experiment, + ExperimentOutcome, +) from apps.guardrails.models import GuardrailTrigger from apps.learnings.models import Learning, LearningEdit from apps.metrics.models import ExperimentMetric @@ -111,11 +120,12 @@ def learning_list( qs = qs.filter(tags__icontains=tag) if search is not None: if _is_postgres(): - from django.contrib.postgres.search import SearchQuery, SearchRank query = SearchQuery(search, config="english") - qs = qs.filter(search_vector=query).annotate( - rank=SearchRank("search_vector", query) - ).order_by("-rank") + qs = ( + qs.filter(search_vector=query) + .annotate(rank=SearchRank("search_vector", query)) + .order_by("-rank") + ) else: qs = qs.filter( Q(hypothesis__icontains=search) @@ -138,7 +148,9 @@ def find_similar_learnings( limit: int = 5, ) -> list[dict[str, Any]]: try: - experiment = Experiment.objects.select_related("flag").get(pk=experiment_id) + experiment = Experiment.objects.select_related("flag").get( + pk=experiment_id + ) except Experiment.DoesNotExist: return [] @@ -159,13 +171,17 @@ def find_similar_learnings( experiment_id=experiment_id ).exists() - candidates = Learning.objects.select_related( - "experiment__flag", - "experiment__owner", - "experiment__outcome", - ).exclude( - experiment_id=experiment_id, - ).order_by("-created_at")[:100] + candidates = ( + Learning.objects.select_related( + "experiment__flag", + "experiment__owner", + "experiment__outcome", + ) + .exclude( + experiment_id=experiment_id, + ) + .order_by("-created_at")[:100] + ) scored: list[tuple[float, Learning]] = [] for candidate in candidates: @@ -179,7 +195,7 @@ def find_similar_learnings( if score > 0: scored.append((score, candidate)) - scored.sort(key=lambda x: x[0], reverse=True) + scored.sort(key=operator.itemgetter(0), reverse=True) results: list[dict[str, Any]] = [] for score, candidate in scored[:limit]: outcome_data = None @@ -189,7 +205,9 @@ def find_similar_learnings( outcome_data = { "outcome": exp_outcome.outcome, "rationale": exp_outcome.rationale, - "winning_variant": str(exp_outcome.winning_variant_id) if exp_outcome.winning_variant_id else None, + "winning_variant": str(exp_outcome.winning_variant_id) + if exp_outcome.winning_variant_id + else None, } except ExperimentOutcome.DoesNotExist: pass @@ -198,19 +216,21 @@ def find_similar_learnings( experiment_id=candidate.experiment_id ).count() - results.append({ - "learning_id": str(candidate.pk), - "experiment_id": str(candidate.experiment_id), - "experiment_name": candidate.experiment.name, - "flag_key": candidate.experiment.flag.key, - "hypothesis": candidate.hypothesis, - "findings": candidate.findings, - "tags": candidate.tags, - "outcome": outcome_data, - "had_guardrail_triggers": trigger_count > 0, - "guardrail_trigger_count": trigger_count, - "similarity_score": round(score, 3), - }) + results.append( + { + "learning_id": str(candidate.pk), + "experiment_id": str(candidate.experiment_id), + "experiment_name": candidate.experiment.name, + "flag_key": candidate.experiment.flag.key, + "hypothesis": candidate.hypothesis, + "findings": candidate.findings, + "tags": candidate.tags, + "outcome": outcome_data, + "had_guardrail_triggers": trigger_count > 0, + "guardrail_trigger_count": trigger_count, + "similarity_score": round(score, 3), + } + ) return results @@ -258,7 +278,7 @@ def _jaccard_score( def _update_search_vector(learning: Learning) -> None: if not _is_postgres(): return - from django.contrib.postgres.search import SearchVector + Learning.objects.filter(pk=learning.pk).update( search_vector=( SearchVector("hypothesis", weight="A", config="english") diff --git a/src/backend/apps/learnings/tests/test_learnings.py b/src/backend/apps/learnings/tests/test_learnings.py index 0b7e40d..917230a 100644 --- a/src/backend/apps/learnings/tests/test_learnings.py +++ b/src/backend/apps/learnings/tests/test_learnings.py @@ -1,12 +1,12 @@ +import uuid from typing import override from django.test import TestCase -from apps.experiments.models import ExperimentOutcome, ExperimentStatus, OutcomeType -from apps.experiments.services import experiment_complete -from apps.experiments.tests.helpers import add_two_variants, make_experiment, make_flag -from apps.guardrails.models import Guardrail, GuardrailTrigger -from apps.learnings.models import Learning, LearningEdit +from apps.experiments.tests.helpers import ( + make_experiment, + make_flag, +) from apps.learnings.services import ( find_similar_learnings, learning_create, @@ -16,9 +16,9 @@ from apps.learnings.services import ( learning_list, learning_update, ) -from apps.metrics.models import ExperimentMetric, MetricDefinition, MetricType from apps.reviews.tests.helpers import make_admin from apps.users.tests.helpers import make_user +from config.errors import ConflictError class LearningCRUDTest(TestCase): @@ -28,7 +28,7 @@ class LearningCRUDTest(TestCase): self.exp = make_experiment(suffix="_lcrud", owner=self.user) def test_create_learning(self) -> None: - l = learning_create( + learning = learning_create( experiment=self.exp, hypothesis="Changing button color increases CTR", findings="Blue variant showed +5% CTR improvement", @@ -36,21 +36,25 @@ class LearningCRUDTest(TestCase): context_summary="Checkout page button color test", user=self.user, ) - self.assertEqual(l.experiment, self.exp) - self.assertEqual(l.hypothesis, "Changing button color increases CTR") - self.assertEqual(l.findings, "Blue variant showed +5% CTR improvement") - self.assertEqual(l.tags, ["ui", "ctr", "button"]) - self.assertEqual(l.created_by, self.user) + self.assertEqual(learning.experiment, self.exp) + self.assertEqual( + learning.hypothesis, "Changing button color increases CTR" + ) + self.assertEqual( + learning.findings, "Blue variant showed +5% CTR improvement" + ) + self.assertEqual(learning.tags, ["ui", "ctr", "button"]) + self.assertEqual(learning.created_by, self.user) def test_create_learning_minimal(self) -> None: - l = learning_create( + learning = learning_create( experiment=self.exp, hypothesis="Test hypothesis", findings="Test findings", ) - self.assertEqual(l.tags, []) - self.assertEqual(l.context_summary, "") - self.assertIsNone(l.created_by) + self.assertEqual(learning.tags, []) + self.assertEqual(learning.context_summary, "") + self.assertIsNone(learning.created_by) def test_create_duplicate_learning_fails(self) -> None: learning_create( @@ -58,7 +62,7 @@ class LearningCRUDTest(TestCase): hypothesis="First", findings="First findings", ) - with self.assertRaises(Exception): + with self.assertRaises(ConflictError): learning_create( experiment=self.exp, hypothesis="Second", @@ -66,28 +70,27 @@ class LearningCRUDTest(TestCase): ) def test_get_learning(self) -> None: - l = learning_create( + learning = learning_create( experiment=self.exp, hypothesis="Test", findings="Test findings", ) - fetched = learning_get(l.pk) + fetched = learning_get(learning.pk) self.assertIsNotNone(fetched) - self.assertEqual(fetched.pk, l.pk) + self.assertEqual(fetched.pk, learning.pk) def test_get_nonexistent_learning(self) -> None: - import uuid result = learning_get(uuid.uuid4()) self.assertIsNone(result) def test_delete_learning(self) -> None: - l = learning_create( + learning = learning_create( experiment=self.exp, hypothesis="Test", findings="Test findings", ) - learning_delete(learning=l) - self.assertIsNone(learning_get(l.pk)) + learning_delete(learning=learning) + self.assertIsNone(learning_get(learning.pk)) class LearningUpdateTest(TestCase): @@ -108,12 +111,12 @@ class LearningUpdateTest(TestCase): username="editor_lupd", email="editor_lupd@lotty.local", ) - l = learning_update( + learning = learning_update( learning=self.learning, user=editor, hypothesis="Updated hypothesis", ) - self.assertEqual(l.hypothesis, "Updated hypothesis") + self.assertEqual(learning.hypothesis, "Updated hypothesis") def test_update_creates_audit_trail(self) -> None: editor = make_user( @@ -193,7 +196,7 @@ class LearningListTest(TestCase): def test_filter_by_tag(self) -> None: learnings = learning_list(tag="ui") - self.assertTrue(all("ui" in l.tags for l in learnings)) + self.assertTrue(all("ui" in learning.tags for learning in learnings)) def test_filter_by_tag_no_match(self) -> None: learnings = learning_list(tag="nonexistent") @@ -214,7 +217,9 @@ class SimilarLearningsTest(TestCase): self.user = make_admin("_sim") self.flag = make_flag(suffix="_sim1") - self.exp1 = make_experiment(flag=self.flag, owner=self.user, suffix="_sim1") + self.exp1 = make_experiment( + flag=self.flag, owner=self.user, suffix="_sim1" + ) self.learning1 = learning_create( experiment=self.exp1, hypothesis="Test button color", @@ -223,7 +228,9 @@ class SimilarLearningsTest(TestCase): ) flag2 = make_flag(suffix="_sim2") - self.exp2 = make_experiment(flag=flag2, owner=self.user, suffix="_sim2") + self.exp2 = make_experiment( + flag=flag2, owner=self.user, suffix="_sim2" + ) self.learning2 = learning_create( experiment=self.exp2, hypothesis="Test font size", @@ -262,7 +269,6 @@ class SimilarLearningsTest(TestCase): self.assertNotIn(str(self.exp1.pk), result_exp_ids) def test_find_similar_nonexistent_experiment(self) -> None: - import uuid results = find_similar_learnings(experiment_id=uuid.uuid4()) self.assertEqual(results, []) diff --git a/src/backend/apps/notifications/models.py b/src/backend/apps/notifications/models.py index e897a04..5f44f0d 100644 --- a/src/backend/apps/notifications/models.py +++ b/src/backend/apps/notifications/models.py @@ -41,7 +41,9 @@ class NotificationChannel(BaseModel): default=dict, blank=True, verbose_name=_("configuration"), - help_text=_("Provider-specific settings (tokens, chat IDs, SMTP host, etc.)"), + help_text=_( + "Provider-specific settings (tokens, chat IDs, SMTP host, etc.)" + ), ) is_active = models.BooleanField( default=True, diff --git a/src/backend/apps/notifications/services.py b/src/backend/apps/notifications/services.py index cbbd4b2..760becd 100644 --- a/src/backend/apps/notifications/services.py +++ b/src/backend/apps/notifications/services.py @@ -5,13 +5,12 @@ from typing import Any import requests from django.core.mail import send_mail from django.db import transaction -from django.db.models import QuerySet +from django.db.models import Q, QuerySet from django.utils import timezone from apps.notifications.models import ( ChannelType, NotificationChannel, - NotificationEventType, NotificationLog, NotificationRule, NotificationStatus, @@ -30,11 +29,6 @@ class NotificationPayload: extra: dict[str, Any] = field(default_factory=dict) -# --------------------------------------------------------------------------- -# Channel CRUD -# --------------------------------------------------------------------------- - - @transaction.atomic def channel_create( *, @@ -82,11 +76,6 @@ def channel_get(channel_id: Any) -> NotificationChannel | None: return None -# --------------------------------------------------------------------------- -# Rule CRUD -# --------------------------------------------------------------------------- - - @transaction.atomic def rule_create( *, @@ -130,11 +119,6 @@ def rule_list(channel_id: Any | None = None) -> QuerySet[NotificationRule]: return qs -# --------------------------------------------------------------------------- -# Log selectors -# --------------------------------------------------------------------------- - - def log_list( *, status: str | None = None, @@ -146,11 +130,6 @@ def log_list( return qs[:limit] -# --------------------------------------------------------------------------- -# Notification enqueue (called from integration points) -# --------------------------------------------------------------------------- - - def notification_enqueue( event_type: str, payload: NotificationPayload, @@ -163,8 +142,7 @@ def notification_enqueue( if payload.experiment_id: rules = rules.filter( - models_Q(experiment__isnull=True) - | models_Q(experiment_id=payload.experiment_id) + Q(experiment__isnull=True) | Q(experiment_id=payload.experiment_id) ) else: rules = rules.filter(experiment__isnull=True) @@ -203,17 +181,6 @@ def _build_event_key(event_type: str, payload: NotificationPayload) -> str: return f"{event_type}:{payload.experiment_id}:{bucket}" -def models_Q(**kwargs): - from django.db.models import Q - - return Q(**kwargs) - - -# --------------------------------------------------------------------------- -# Senders -# --------------------------------------------------------------------------- - - def _send_telegram(config: dict[str, Any], payload: dict[str, Any]) -> None: bot_token = config.get("bot_token", "") chat_id = config.get("chat_id", "") @@ -260,11 +227,6 @@ def _send_smtp(config: dict[str, Any], payload: dict[str, Any]) -> None: ) -# --------------------------------------------------------------------------- -# Flush pending (called from Celery task) -# --------------------------------------------------------------------------- - - def flush_pending_notifications() -> dict[str, int]: pending = NotificationLog.objects.filter( status=NotificationStatus.PENDING, @@ -288,7 +250,9 @@ def flush_pending_notifications() -> dict[str, int]: sender = senders.get(log.channel.channel_type) if not sender: log.status = NotificationStatus.FAILED - log.error = f"No sender for channel type '{log.channel.channel_type}'." + log.error = ( + f"No sender for channel type '{log.channel.channel_type}'." + ) log.save(update_fields=["status", "error"]) results["failed"] += 1 continue diff --git a/src/backend/apps/notifications/tests/test_notifications.py b/src/backend/apps/notifications/tests/test_notifications.py index 129e8d8..c61146a 100644 --- a/src/backend/apps/notifications/tests/test_notifications.py +++ b/src/backend/apps/notifications/tests/test_notifications.py @@ -6,10 +6,8 @@ from django.test import TestCase from apps.experiments.tests.helpers import make_experiment from apps.notifications.models import ( ChannelType, - NotificationChannel, NotificationEventType, NotificationLog, - NotificationRule, NotificationStatus, ) from apps.notifications.services import ( @@ -85,7 +83,9 @@ class RuleCRUDTest(TestCase): event_type=NotificationEventType.GUARDRAIL_TRIGGERED, channel=self.channel, ) - self.assertEqual(r.event_type, NotificationEventType.GUARDRAIL_TRIGGERED) + self.assertEqual( + r.event_type, NotificationEventType.GUARDRAIL_TRIGGERED + ) self.assertIsNone(r.experiment) self.assertTrue(r.is_active)