Merge remote dev branch
This commit is contained in:
@@ -14,7 +14,9 @@ logger = logging.getLogger(__name__)
|
||||
router = APIRouter(prefix="/users/me", tags=["user-config"])
|
||||
|
||||
|
||||
async def _get_or_create_config(session: AsyncSession, user_id: uuid.UUID) -> UserConfig:
|
||||
async def _get_or_create_config(
|
||||
session: AsyncSession, user_id: uuid.UUID
|
||||
) -> UserConfig:
|
||||
"""Get or create user config record.
|
||||
|
||||
Args:
|
||||
@@ -24,7 +26,9 @@ async def _get_or_create_config(session: AsyncSession, user_id: uuid.UUID) -> Us
|
||||
Returns:
|
||||
The user's config, creating a new one if it doesn't exist.
|
||||
"""
|
||||
result = await session.execute(select(UserConfig).where(UserConfig.user_id == user_id))
|
||||
result = await session.execute(
|
||||
select(UserConfig).where(UserConfig.user_id == user_id)
|
||||
)
|
||||
config = result.scalar_one_or_none()
|
||||
if config is None:
|
||||
config = UserConfig(user_id=user_id, config={})
|
||||
@@ -42,6 +46,8 @@ class UserConfigResponse(BaseModel):
|
||||
git_user_name: str | None = None
|
||||
git_user_email: str | None = None
|
||||
last_session_id: str | None = None
|
||||
notification_mute_categories: list[str] | None = None
|
||||
notification_toast_level: str | None = None
|
||||
|
||||
|
||||
class UserConfigUpdate(BaseModel):
|
||||
@@ -50,6 +56,8 @@ class UserConfigUpdate(BaseModel):
|
||||
git_user_name: str | None = None
|
||||
git_user_email: str | None = None
|
||||
last_session_id: str | None = None
|
||||
notification_mute_categories: list[str] | None = None
|
||||
notification_toast_level: str | None = None
|
||||
|
||||
|
||||
@router.get(
|
||||
|
||||
@@ -15,6 +15,7 @@ from src.models.tool_instance import ToolInstance
|
||||
from src.services.correlation import get_correlation_id
|
||||
from src.services.docker import check_tunnel_health, get_container_status
|
||||
from src.services.event_bus import InstanceEventBus, InstanceEventPayload
|
||||
from src.services.notification_service import notification_service
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -217,3 +218,36 @@ class HealthMonitor:
|
||||
}
|
||||
|
||||
await self._event_bus.publish(event_type, payload)
|
||||
|
||||
# Create notification for instance owner (fire-and-forget)
|
||||
if new_status == "error":
|
||||
category = "instance"
|
||||
severity = "error"
|
||||
title = "Container failed"
|
||||
else:
|
||||
category = "health"
|
||||
if new_status == "unhealthy":
|
||||
severity = "warning"
|
||||
title = "Container unhealthy"
|
||||
else:
|
||||
severity = "info"
|
||||
title = "Container recovered"
|
||||
|
||||
try:
|
||||
await notification_service.create_notification(
|
||||
session=session,
|
||||
user_id=instance.owner_id,
|
||||
category=category,
|
||||
severity=severity,
|
||||
title=title,
|
||||
message=message,
|
||||
source_type="tool_instances",
|
||||
source_id=instance.id,
|
||||
metadata=metadata,
|
||||
)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Failed to create notification for health event %s",
|
||||
event_type,
|
||||
extra={"correlation_id": correlation_id},
|
||||
)
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
"""Lifecycle hook helpers for instrumenting tool instance transitions."""
|
||||
|
||||
import logging
|
||||
import uuid
|
||||
from datetime import datetime, timezone
|
||||
|
||||
@@ -9,6 +10,25 @@ from src.models.instance_event import InstanceEvent
|
||||
from src.models.tool_instance import ToolInstance
|
||||
from src.services.correlation import get_correlation_id
|
||||
from src.services.event_bus import InstanceEventBus, InstanceEventPayload
|
||||
from src.services.notification_service import notification_service
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _derive_title(event_type: str) -> str:
|
||||
"""Map lifecycle event type to a human-readable notification title."""
|
||||
mapping = {
|
||||
"instance.created": "Container created",
|
||||
"instance.started": "Container started",
|
||||
"instance.stopped": "Container stopped",
|
||||
"instance.restarted": "Container restarted",
|
||||
"instance.deleted": "Container deleted",
|
||||
"instance.error": "Container error",
|
||||
}
|
||||
return mapping.get(
|
||||
event_type,
|
||||
event_type.replace("instance.", "").replace("_", " ").title(),
|
||||
)
|
||||
|
||||
|
||||
def _build_payload(
|
||||
@@ -96,3 +116,26 @@ async def publish_lifecycle_event(
|
||||
|
||||
# Publish to bus
|
||||
await event_bus.publish(event_type, payload)
|
||||
|
||||
# Create notification for instance owner (fire-and-forget)
|
||||
severity = "error" if event_type == "instance.error" else "info"
|
||||
title = _derive_title(event_type)
|
||||
|
||||
try:
|
||||
await notification_service.create_notification(
|
||||
session=session,
|
||||
user_id=instance.owner_id,
|
||||
category="instance",
|
||||
severity=severity,
|
||||
title=title,
|
||||
message=message,
|
||||
source_type="tool_instances",
|
||||
source_id=instance.id,
|
||||
metadata=metadata,
|
||||
)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Failed to create notification for lifecycle event %s",
|
||||
event_type,
|
||||
extra={"correlation_id": payload.get("correlation_id", "unknown")},
|
||||
)
|
||||
|
||||
@@ -0,0 +1,370 @@
|
||||
"""Integration tests for event producer → notification creation flow."""
|
||||
|
||||
import uuid
|
||||
from collections.abc import Generator
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from src.models.git_repository import GitRepository
|
||||
from src.models.notification import Notification
|
||||
from src.models.project import Project
|
||||
from src.models.tool_instance import ToolInstance
|
||||
from src.models.tool_type import ToolType
|
||||
from src.models.user import User
|
||||
from src.services.event_bus import InstanceEventBus, InstanceEventPayload
|
||||
from src.services.health_monitor import HealthSnapshot
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def event_bus() -> Generator[InstanceEventBus, None, None]:
|
||||
"""Provide a fresh EventBus instance."""
|
||||
bus = InstanceEventBus()
|
||||
bus._reset_for_testing()
|
||||
yield bus
|
||||
bus._reset_for_testing()
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def test_instance(db_session: AsyncSession) -> ToolInstance:
|
||||
"""Create a complete tool instance with all required relations."""
|
||||
user = User(
|
||||
id=uuid.uuid4(),
|
||||
email="owner@headquarter.local",
|
||||
name="Owner",
|
||||
authentik_id=f"authentik-{uuid.uuid4()}",
|
||||
avatar_url=None,
|
||||
)
|
||||
db_session.add(user)
|
||||
await db_session.commit()
|
||||
|
||||
project = Project(
|
||||
id=uuid.uuid4(),
|
||||
name="test-project",
|
||||
description="Test",
|
||||
owner_id=user.id,
|
||||
)
|
||||
repo = GitRepository(
|
||||
id=uuid.uuid4(),
|
||||
name="test-repo",
|
||||
path="/tmp/test-repo",
|
||||
project_id=project.id,
|
||||
owner_id=user.id,
|
||||
remote_url="https://github.com/test/repo.git",
|
||||
)
|
||||
tool_type = ToolType(
|
||||
id=uuid.uuid4(),
|
||||
name="test-tool",
|
||||
display_name="Test Tool",
|
||||
category="other",
|
||||
interface_type="web",
|
||||
requires_port=True,
|
||||
default_port=8080,
|
||||
definition_type="legacy",
|
||||
compose_template="version: '3.8'\nservices:\n app:\n image: alpine\n command: sleep 3600\n",
|
||||
)
|
||||
db_session.add_all([project, repo, tool_type])
|
||||
await db_session.commit()
|
||||
|
||||
instance = ToolInstance(
|
||||
id=uuid.uuid4(),
|
||||
name="test-instance",
|
||||
display_name="Test Instance",
|
||||
tool_type_id=tool_type.id,
|
||||
repository_id=repo.id,
|
||||
project_id=project.id,
|
||||
owner_id=user.id,
|
||||
status="running",
|
||||
compose_path="/tmp/test-compose.yml",
|
||||
port=8080,
|
||||
)
|
||||
db_session.add(instance)
|
||||
await db_session.commit()
|
||||
return instance
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_lifecycle_event_creates_notification(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
test_instance: ToolInstance,
|
||||
) -> None:
|
||||
"""Triggering a lifecycle event creates a notification for the instance owner."""
|
||||
received: list[InstanceEventPayload] = []
|
||||
|
||||
def subscriber(payload: InstanceEventPayload) -> None:
|
||||
received.append(payload)
|
||||
|
||||
event_bus.subscribe("instance.started", subscriber)
|
||||
|
||||
from src.services.lifecycle_hooks import publish_lifecycle_event
|
||||
|
||||
await publish_lifecycle_event(
|
||||
event_bus=event_bus,
|
||||
session=db_session,
|
||||
instance=test_instance,
|
||||
event_type="instance.started",
|
||||
status="starting",
|
||||
message="Container started",
|
||||
)
|
||||
|
||||
# Event still published
|
||||
assert len(received) == 1
|
||||
|
||||
# Notification created
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.user_id == test_instance.owner_id)
|
||||
)
|
||||
notifications = list(result.scalars().all())
|
||||
assert len(notifications) == 1
|
||||
n = notifications[0]
|
||||
assert n.category == "instance"
|
||||
assert n.severity == "info"
|
||||
assert n.title == "Container started"
|
||||
assert n.source_type == "tool_instances"
|
||||
assert n.source_id == test_instance.id
|
||||
assert n.message == "Container started"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_health_monitor_error_creates_notification(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
test_instance: ToolInstance,
|
||||
) -> None:
|
||||
"""Simulating a health monitor crash creates an error notification."""
|
||||
from src.services.health_monitor import HealthMonitor
|
||||
|
||||
monitor = HealthMonitor(event_bus)
|
||||
|
||||
received: list[InstanceEventPayload] = []
|
||||
|
||||
def subscriber(payload: InstanceEventPayload) -> None:
|
||||
received.append(payload)
|
||||
|
||||
event_bus.subscribe("instance.error", subscriber)
|
||||
|
||||
with patch(
|
||||
"src.services.health_monitor.get_container_status",
|
||||
return_value={"status": "exited", "exit_code": 137, "health": None},
|
||||
):
|
||||
await monitor._check_instance(db_session, test_instance)
|
||||
|
||||
# Event published
|
||||
assert len(received) == 1
|
||||
|
||||
# Notification created
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.user_id == test_instance.owner_id)
|
||||
)
|
||||
notifications = list(result.scalars().all())
|
||||
assert len(notifications) == 1
|
||||
n = notifications[0]
|
||||
assert n.category == "instance"
|
||||
assert n.severity == "error"
|
||||
assert n.source_type == "tool_instances"
|
||||
assert n.source_id == test_instance.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_notification_failure_does_not_block_event_pipeline(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
test_instance: ToolInstance,
|
||||
) -> None:
|
||||
"""If NotificationService raises, the event is still published and no exception escapes."""
|
||||
received: list[InstanceEventPayload] = []
|
||||
|
||||
def subscriber(payload: InstanceEventPayload) -> None:
|
||||
received.append(payload)
|
||||
|
||||
event_bus.subscribe("instance.started", subscriber)
|
||||
|
||||
from src.services.lifecycle_hooks import publish_lifecycle_event
|
||||
|
||||
with patch(
|
||||
"src.services.lifecycle_hooks.notification_service.create_notification",
|
||||
side_effect=RuntimeError("DB is down"),
|
||||
):
|
||||
# Should not raise
|
||||
await publish_lifecycle_event(
|
||||
event_bus=event_bus,
|
||||
session=db_session,
|
||||
instance=test_instance,
|
||||
event_type="instance.started",
|
||||
status="starting",
|
||||
message="Container started",
|
||||
)
|
||||
|
||||
assert len(received) == 1
|
||||
assert received[0]["event"] == "instance.started"
|
||||
|
||||
# No notification should have been created
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.user_id == test_instance.owner_id)
|
||||
)
|
||||
assert result.scalar_one_or_none() is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_notification_ownership_matches_instance_owner(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
) -> None:
|
||||
"""Notification user_id matches the instance owner, not any caller."""
|
||||
# Create a caller user (simulates the user making an API request)
|
||||
caller = User(
|
||||
id=uuid.uuid4(),
|
||||
email="caller@headquarter.local",
|
||||
name="Caller",
|
||||
authentik_id=f"authentik-{uuid.uuid4()}",
|
||||
avatar_url=None,
|
||||
)
|
||||
db_session.add(caller)
|
||||
await db_session.commit()
|
||||
|
||||
# Create the actual owner
|
||||
owner = User(
|
||||
id=uuid.uuid4(),
|
||||
email="owner@headquarter.local",
|
||||
name="Owner",
|
||||
authentik_id=f"authentik-{uuid.uuid4()}",
|
||||
avatar_url=None,
|
||||
)
|
||||
db_session.add(owner)
|
||||
await db_session.commit()
|
||||
|
||||
project = Project(
|
||||
id=uuid.uuid4(),
|
||||
name="test-project",
|
||||
description="Test",
|
||||
owner_id=owner.id,
|
||||
)
|
||||
repo = GitRepository(
|
||||
id=uuid.uuid4(),
|
||||
name="test-repo",
|
||||
path="/tmp/test-repo",
|
||||
project_id=project.id,
|
||||
owner_id=owner.id,
|
||||
remote_url="https://github.com/test/repo.git",
|
||||
)
|
||||
tool_type = ToolType(
|
||||
id=uuid.uuid4(),
|
||||
name="test-tool",
|
||||
display_name="Test Tool",
|
||||
category="other",
|
||||
interface_type="web",
|
||||
requires_port=True,
|
||||
default_port=8080,
|
||||
definition_type="legacy",
|
||||
compose_template="version: '3.8'\nservices:\n app:\n image: alpine\n command: sleep 3600\n",
|
||||
)
|
||||
db_session.add_all([project, repo, tool_type])
|
||||
await db_session.commit()
|
||||
|
||||
instance = ToolInstance(
|
||||
id=uuid.uuid4(),
|
||||
name="test-instance",
|
||||
display_name="Test Instance",
|
||||
tool_type_id=tool_type.id,
|
||||
repository_id=repo.id,
|
||||
project_id=project.id,
|
||||
owner_id=owner.id,
|
||||
status="running",
|
||||
compose_path="/tmp/test-compose.yml",
|
||||
port=8080,
|
||||
)
|
||||
db_session.add(instance)
|
||||
await db_session.commit()
|
||||
|
||||
from src.services.lifecycle_hooks import publish_lifecycle_event
|
||||
|
||||
await publish_lifecycle_event(
|
||||
event_bus=event_bus,
|
||||
session=db_session,
|
||||
instance=instance,
|
||||
event_type="instance.created",
|
||||
status="pending",
|
||||
message="Instance created",
|
||||
)
|
||||
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.source_id == instance.id)
|
||||
)
|
||||
n = result.scalar_one()
|
||||
assert n.user_id == owner.id
|
||||
assert n.user_id != caller.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_lifecycle_error_creates_error_notification(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
test_instance: ToolInstance,
|
||||
) -> None:
|
||||
"""An instance.error lifecycle event creates a severity=error notification."""
|
||||
from src.services.lifecycle_hooks import publish_lifecycle_event
|
||||
|
||||
await publish_lifecycle_event(
|
||||
event_bus=event_bus,
|
||||
session=db_session,
|
||||
instance=test_instance,
|
||||
event_type="instance.error",
|
||||
status="error",
|
||||
message="Container failed",
|
||||
)
|
||||
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.user_id == test_instance.owner_id)
|
||||
)
|
||||
n = result.scalar_one()
|
||||
assert n.severity == "error"
|
||||
assert n.title == "Container error"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_health_monitor_unhealthy_creates_warning_notification(
|
||||
db_session: AsyncSession,
|
||||
event_bus: InstanceEventBus,
|
||||
test_instance: ToolInstance,
|
||||
) -> None:
|
||||
"""Health monitor marking instance unhealthy creates severity=warning notification."""
|
||||
from src.services.health_monitor import HealthMonitor
|
||||
|
||||
monitor = HealthMonitor(event_bus)
|
||||
monitor._last_known_state[test_instance.id] = HealthSnapshot(
|
||||
container_status="running",
|
||||
container_healthy=None,
|
||||
tunnel_healthy=True,
|
||||
exit_code=None,
|
||||
)
|
||||
test_instance.public_url = "https://example.trycloudflare.com"
|
||||
|
||||
with (
|
||||
patch(
|
||||
"src.services.health_monitor.get_container_status",
|
||||
return_value={"status": "running", "exit_code": None, "health": "healthy"},
|
||||
),
|
||||
patch(
|
||||
"src.services.health_monitor.check_tunnel_health",
|
||||
return_value={"healthy": False, "tunnel_status": "error_response"},
|
||||
),
|
||||
):
|
||||
await monitor._check_instance(db_session, test_instance)
|
||||
|
||||
result = await db_session.execute(
|
||||
select(Notification).where(Notification.user_id == test_instance.owner_id)
|
||||
)
|
||||
n = result.scalar_one()
|
||||
assert n.category == "health"
|
||||
assert n.severity == "warning"
|
||||
assert n.title == "Container unhealthy"
|
||||
@@ -0,0 +1,156 @@
|
||||
# PR-2 Apply Report: Backend Integration for Notification Center
|
||||
|
||||
## Status: COMPLETE
|
||||
|
||||
All 5 tasks for PR-2 (NC-PR2-001 through NC-PR2-005) have been implemented, tested, and validated.
|
||||
|
||||
## What Was Implemented
|
||||
|
||||
### NC-PR2-001: Wire lifecycle_hooks.py to NotificationService
|
||||
|
||||
**File:** `apps/api/src/services/lifecycle_hooks.py`
|
||||
|
||||
- Imported `notification_service` singleton from `src.services.notification_service`
|
||||
- Added `_derive_title(event_type)` helper mapping lifecycle events to human-readable titles:
|
||||
- `instance.created` → "Container created"
|
||||
- `instance.started` → "Container started"
|
||||
- `instance.stopped` → "Container stopped"
|
||||
- `instance.restarted` → "Container restarted"
|
||||
- `instance.deleted` → "Container deleted"
|
||||
- `instance.error` → "Container error"
|
||||
- After `event_bus.publish(...)`, calls `notification_service.create_notification(...)` with:
|
||||
- `user_id = instance.owner_id`
|
||||
- `category = "instance"`
|
||||
- `severity = "error"` for `instance.error`, `"info"` for all others
|
||||
- `source_type = "tool_instances"`, `source_id = instance.id`
|
||||
- Wrapped in `try/except`; logs failure with `correlation_id` and continues
|
||||
- Event bus publish and audit row insert are unaffected by notification failure
|
||||
|
||||
### NC-PR2-002: Wire health_monitor.py to NotificationService
|
||||
|
||||
**File:** `apps/api/src/services/health_monitor.py`
|
||||
|
||||
- Imported `notification_service` singleton
|
||||
- After `self._event_bus.publish(event_type, payload)`, calls `notification_service.create_notification(...)` with:
|
||||
- `user_id = instance.owner_id`
|
||||
- `category = "instance"` for `new_status == "error"`
|
||||
- `category = "health"` for `instance.health_changed`
|
||||
- `severity` mapped:
|
||||
- `"error"` for crash
|
||||
- `"warning"` for unhealthy
|
||||
- `"info"` for recovery (running)
|
||||
- `title` mapped:
|
||||
- "Container failed" for error
|
||||
- "Container unhealthy" for unhealthy
|
||||
- "Container recovered" for running
|
||||
- Wrapped in `try/except`; logs failure with `correlation_id` and continues
|
||||
- Original event bus publish and health check insert are unaffected
|
||||
|
||||
### NC-PR2-003: Extend UserConfig schema for notification preferences
|
||||
|
||||
**File:** `apps/api/src/api/user_config.py`
|
||||
|
||||
- Added `notification_mute_categories: list[str] | None = None` to `UserConfigResponse`
|
||||
- Added `notification_toast_level: str | None = None` to `UserConfigResponse`
|
||||
- Added the same fields to `UserConfigUpdate`
|
||||
- Existing config keys are unaffected; new fields are optional with `None` defaults
|
||||
|
||||
### NC-PR2-004: Event producer integration tests (RED)
|
||||
|
||||
**File:** `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)*
|
||||
|
||||
6 integration tests covering:
|
||||
|
||||
1. `test_lifecycle_event_creates_notification` — lifecycle hook `instance.started` creates `severity="info"` notification for owner
|
||||
2. `test_health_monitor_error_creates_notification` — simulated crash creates `severity="error"` notification for owner
|
||||
3. `test_notification_failure_does_not_block_event_pipeline` — mocked `create_notification` raising `RuntimeError`; event still published, no exception escapes
|
||||
4. `test_notification_ownership_matches_instance_owner` — notification `user_id` equals `instance.owner_id`, not the API caller
|
||||
5. `test_lifecycle_error_creates_error_notification` — `instance.error` maps to `severity="error"`, title="Container error"
|
||||
6. `test_health_monitor_unhealthy_creates_warning_notification` — tunnel failure creating `severity="warning"`, category="health"
|
||||
|
||||
### NC-PR2-005: Verify producer tests and clean up (GREEN / REFACTOR)
|
||||
|
||||
- All 6 new integration tests pass
|
||||
- 13 unit tests for `NotificationService` pass (no regressions)
|
||||
- 10 integration tests for notifications API pass (no regressions)
|
||||
- 6 existing health monitor unit tests pass (no regressions)
|
||||
- 6 existing event integration tests pass (no regressions)
|
||||
- `ruff check` passes on all modified files
|
||||
|
||||
## TDD Cycle Evidence
|
||||
|
||||
| Cycle | Task | Test File | RED | GREEN | Evidence |
|
||||
|-------|------|-----------|-----|-------|----------|
|
||||
| 1 | NC-PR2-004 (producer flow) | `tests/integration/test_notifications_lifecycle.py` | 4 tests written against unwired producers | Wired `lifecycle_hooks.py` and `health_monitor.py` | 4 passed |
|
||||
| 2 | NC-PR2-004 (severity mapping) | `tests/integration/test_notifications_lifecycle.py` | Added error + warning severity tests | Already green from implementation | 6 passed |
|
||||
| 3 | NC-PR2-003 (UserConfig schema) | `src/api/user_config.py` | Schema extended with new optional fields | PATCH/GET endpoints validate correctly | Verified manually |
|
||||
| 4 | NC-PR2-005 (REFACTOR) | All files | — | ruff clean, no regressions across 41 related tests | All pass |
|
||||
|
||||
## Changed Files
|
||||
|
||||
1. `apps/api/src/services/lifecycle_hooks.py` — Wired `NotificationService` after event bus publish
|
||||
2. `apps/api/src/services/health_monitor.py` — Wired `NotificationService` after state change event publish
|
||||
3. `apps/api/src/api/user_config.py` — Added `notification_mute_categories` and `notification_toast_level` to Pydantic schemas
|
||||
4. `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)* — 6 integration tests for event-to-notification flow
|
||||
|
||||
## Test Commands & Exit Codes
|
||||
|
||||
```bash
|
||||
# New integration tests for event producers (6 tests)
|
||||
cd apps/api && python -m pytest tests/integration/test_notifications_lifecycle.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# NotificationService unit tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/unit/test_notification_service.py -v
|
||||
# Exit: 0 — 13 passed
|
||||
|
||||
# Notifications API integration tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v
|
||||
# Exit: 0 — 10 passed
|
||||
|
||||
# Health monitor unit tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/unit/test_health_monitor.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# Event integration tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/integration/test_events.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# Combined relevant test suite
|
||||
cd apps/api && python -m pytest \
|
||||
tests/unit/test_notification_service.py \
|
||||
tests/integration/test_notifications_api.py \
|
||||
tests/integration/test_notifications_lifecycle.py \
|
||||
tests/unit/test_health_monitor.py \
|
||||
tests/integration/test_events.py \
|
||||
-v
|
||||
# Exit: 0 — 41 passed
|
||||
|
||||
# Ruff linting on all modified files
|
||||
cd apps/api && python -m ruff check \
|
||||
src/services/lifecycle_hooks.py \
|
||||
src/services/health_monitor.py \
|
||||
src/api/user_config.py \
|
||||
tests/integration/test_notifications_lifecycle.py
|
||||
# Exit: 0 — All checks passed
|
||||
```
|
||||
|
||||
## Deviations from Design
|
||||
|
||||
None. All mappings and behaviors match the design spec (section 1.3) and task requirements exactly.
|
||||
|
||||
## Surprises / Decisions
|
||||
|
||||
1. **Health monitor `test_health_monitor_unhealthy_creates_warning_notification` required `public_url`:** The health monitor only checks tunnel health when `instance.public_url` is truthy. Without setting it on the test fixture instance, `_derive_status` returned `"running"` instead of `"unhealthy"`, which created an `"info"` notification. Fixed by setting `test_instance.public_url` in the test before calling `_check_instance`.
|
||||
|
||||
2. **Patch target for failure test:** The `test_notification_failure_does_not_block_event_pipeline` patches `src.services.lifecycle_hooks.notification_service.create_notification`. This only works because `lifecycle_hooks.py` imports `notification_service` at module level, making the attribute resolvable by `unittest.mock.patch`.
|
||||
|
||||
3. **No schema migration needed for UserConfig:** Preferences are stored in the existing JSON `config` blob, consistent with the existing pattern (theme, editor, git identity). No Alembic migration required.
|
||||
|
||||
## Risks
|
||||
|
||||
- **None:** All changes are additive. Event producers use `try/except` so notification failures cannot block the event pipeline. No existing test regressions introduced.
|
||||
|
||||
## PR Boundary
|
||||
|
||||
This PR covers PR-2 only (NC-PR2-001 through NC-PR2-005). PR-3 (frontend core) and PR-4 (toast coordination) are out of scope.
|
||||
@@ -1,6 +1,6 @@
|
||||
# Apply Progress: PR-1 Backend Core for Notification Center
|
||||
# Apply Progress: Notification Center
|
||||
|
||||
## TDD Cycle Evidence
|
||||
## TDD Cycle Evidence (PR-1)
|
||||
|
||||
| Cycle | Task | Test File | RED | GREEN | Evidence |
|
||||
|-------|------|-----------|-----|-------|----------|
|
||||
@@ -10,8 +10,18 @@
|
||||
| 4 | NC-PR1-008 (API edge cases) | `tests/integration/test_notifications_api.py` | Already included in cycle 3 | Pagination, 404 ownership, mute categories at API layer | Same 10 tests pass |
|
||||
| 5 | NC-PR1-010 (REFACTOR) | All files | — | ruff clean, no regressions | `ruff check` passes on all new files; existing unit tests 223 passed (4 pre-existing failures unrelated) |
|
||||
|
||||
## TDD Cycle Evidence (PR-2)
|
||||
|
||||
| Cycle | Task | Test File | RED | GREEN | Evidence |
|
||||
|-------|------|-----------|-----|-------|----------|
|
||||
| 1 | NC-PR2-004 (producer flow) | `tests/integration/test_notifications_lifecycle.py` | 4 tests written against unwired producers | Wired `lifecycle_hooks.py` and `health_monitor.py` | 4 passed |
|
||||
| 2 | NC-PR2-004 (severity mapping) | `tests/integration/test_notifications_lifecycle.py` | Added error + warning severity tests | Already green from implementation | 6 passed |
|
||||
| 3 | NC-PR2-003 (UserConfig schema) | `src/api/user_config.py` | Schema extended with new optional fields | PATCH/GET endpoints validate correctly | Verified manually |
|
||||
| 4 | NC-PR2-005 (REFACTOR) | All files | — | ruff clean, no regressions across 41 related tests | All pass |
|
||||
|
||||
## Completed Tasks
|
||||
|
||||
### PR-1: Backend Core
|
||||
- [x] NC-PR1-001: Alembic migration for `notifications` table
|
||||
- [x] NC-PR1-002: SQLAlchemy `Notification` model (`apps/api/src/models/notification.py`)
|
||||
- [x] NC-PR1-003: Export `Notification` in `models/__init__.py`
|
||||
@@ -24,8 +34,16 @@
|
||||
- [x] NC-PR1-010: Register router in `main.py` + import `Notification` for Alembic
|
||||
- [x] NC-PR1-011: Code quality pass — ruff, test regressions, smoke tests (REFACTOR)
|
||||
|
||||
### PR-2: Backend Integration
|
||||
- [x] NC-PR2-001: Wire `lifecycle_hooks.py` to `NotificationService`
|
||||
- [x] NC-PR2-002: Wire `health_monitor.py` to `NotificationService`
|
||||
- [x] NC-PR2-003: Extend `UserConfig` schema for notification preferences
|
||||
- [x] NC-PR2-004: Event producer integration tests (RED)
|
||||
- [x] NC-PR2-005: Verify producer tests pass and clean up (GREEN / REFACTOR)
|
||||
|
||||
## Files Changed
|
||||
|
||||
### PR-1 Files
|
||||
1. `apps/api/alembic/versions/2026_05_29_add_notifications_table.py` *(new)* — Alembic migration
|
||||
2. `apps/api/src/models/notification.py` *(new)* — SQLAlchemy model
|
||||
3. `apps/api/src/models/__init__.py` — Export `Notification`
|
||||
@@ -37,8 +55,15 @@
|
||||
9. `apps/api/tests/integration/test_notifications_api.py` *(new)* — 10 integration tests
|
||||
10. `apps/api/tests/integration/test_models.py` — Updated expected tables list
|
||||
|
||||
### PR-2 Files
|
||||
11. `apps/api/src/services/lifecycle_hooks.py` — Wired `NotificationService` after event bus publish
|
||||
12. `apps/api/src/services/health_monitor.py` — Wired `NotificationService` after state change event publish
|
||||
13. `apps/api/src/api/user_config.py` — Added `notification_mute_categories` and `notification_toast_level` to Pydantic schemas
|
||||
14. `apps/api/tests/integration/test_notifications_lifecycle.py` *(new)* — 6 integration tests for event-to-notification flow
|
||||
|
||||
## Test Commands & Exit Codes
|
||||
|
||||
### PR-1
|
||||
```bash
|
||||
# Unit tests for NotificationService (13 tests)
|
||||
cd apps/api && python -m pytest tests/unit/test_notification_service.py -v
|
||||
@@ -48,48 +73,78 @@ cd apps/api && python -m pytest tests/unit/test_notification_service.py -v
|
||||
cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v
|
||||
# Exit: 0 — 10 passed
|
||||
|
||||
# Combined new tests
|
||||
cd apps/api && python -m pytest tests/unit/test_notification_service.py tests/integration/test_notifications_api.py -v
|
||||
# Exit: 0 — 23 passed
|
||||
|
||||
# Existing unit tests (no regressions in our code)
|
||||
cd apps/api && python -m pytest tests/unit/ -v
|
||||
# Exit: 1 — 223 passed, 4 failed (pre-existing failures in test_config.py and test_git_repository_clone_preflight.py)
|
||||
```
|
||||
|
||||
# Ruff linting on all new/modified files
|
||||
cd apps/api && python -m ruff check \
|
||||
src/models/notification.py \
|
||||
src/models/__init__.py \
|
||||
src/services/notification_service.py \
|
||||
src/api/notifications.py \
|
||||
src/api/__init__.py \
|
||||
src/main.py \
|
||||
alembic/versions/2026_05_29_add_notifications_table.py \
|
||||
### PR-2
|
||||
```bash
|
||||
# New integration tests for event producers (6 tests)
|
||||
cd apps/api && python -m pytest tests/integration/test_notifications_lifecycle.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# NotificationService unit tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/unit/test_notification_service.py -v
|
||||
# Exit: 0 — 13 passed
|
||||
|
||||
# Notifications API integration tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/integration/test_notifications_api.py -v
|
||||
# Exit: 0 — 10 passed
|
||||
|
||||
# Health monitor unit tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/unit/test_health_monitor.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# Event integration tests (no regressions)
|
||||
cd apps/api && python -m pytest tests/integration/test_events.py -v
|
||||
# Exit: 0 — 6 passed
|
||||
|
||||
# Combined relevant test suite (41 tests)
|
||||
cd apps/api && python -m pytest \
|
||||
tests/unit/test_notification_service.py \
|
||||
tests/integration/test_notifications_api.py \
|
||||
tests/integration/test_models.py
|
||||
# Exit: 0 — All checks passed
|
||||
tests/integration/test_notifications_lifecycle.py \
|
||||
tests/unit/test_health_monitor.py \
|
||||
tests/integration/test_events.py \
|
||||
-v
|
||||
# Exit: 0 — 41 passed
|
||||
|
||||
# Smoke tests
|
||||
health: 200
|
||||
notifications unauth: 401
|
||||
# Ruff linting on all PR-2 modified files
|
||||
cd apps/api && python -m ruff check \
|
||||
src/services/lifecycle_hooks.py \
|
||||
src/services/health_monitor.py \
|
||||
src/api/user_config.py \
|
||||
tests/integration/test_notifications_lifecycle.py
|
||||
# Exit: 0 — All checks passed
|
||||
```
|
||||
|
||||
## Deviations from Design
|
||||
|
||||
### PR-1
|
||||
- **SQLAlchemy `metadata` column name conflict:** `Base.metadata` is reserved by SQLAlchemy DeclarativeBase. Used `notification_metadata` as the Python attribute name with DB column name `"metadata"`. In the Pydantic response model, used `Field(serialization_alias="metadata")` so the JSON API still exposes `metadata` as specified.
|
||||
- **`created_at` type in Pydantic:** Used `datetime` instead of `str` to leverage FastAPI's automatic ISO serialization.
|
||||
|
||||
### PR-2
|
||||
- None. All mappings and behaviors match the design spec (section 1.3) and task requirements exactly.
|
||||
|
||||
## Surprises / Decisions
|
||||
|
||||
### PR-1
|
||||
1. **SQLite `func.now()` resolution:** `test_list_notifications_orders_by_created_at_desc` failed because multiple rapid INSERTs got identical timestamps. Fixed by explicitly setting `created_at` offsets in the test after creation.
|
||||
2. **Pre-existing integration test failures:** ~40 integration tests fail due to missing `asyncpg` module and direct PostgreSQL connection attempts in their custom setup code. These are unrelated to our changes.
|
||||
3. **Pre-existing `test_models.py` outdated:** The `test_expected_tables_are_registered` assertion had a hardcoded set missing many newer tables. Updated it to include all current tables (including `notifications`).
|
||||
|
||||
### PR-2
|
||||
1. **Health monitor `test_health_monitor_unhealthy_creates_warning_notification` required `public_url`:** The health monitor only checks tunnel health when `instance.public_url` is truthy. Without setting it on the test fixture instance, `_derive_status` returned `"running"` instead of `"unhealthy"`, which created an `"info"` notification. Fixed by setting `test_instance.public_url` in the test before calling `_check_instance`.
|
||||
2. **Patch target for failure test:** The `test_notification_failure_does_not_block_event_pipeline` patches `src.services.lifecycle_hooks.notification_service.create_notification`. This only works because `lifecycle_hooks.py` imports `notification_service` at module level, making the attribute resolvable by `unittest.mock.patch`.
|
||||
3. **No schema migration needed for UserConfig:** Preferences are stored in the existing JSON `config` blob, consistent with the existing pattern (theme, editor, git identity). No Alembic migration required.
|
||||
|
||||
## Remaining Tasks
|
||||
|
||||
None — PR-1 is complete.
|
||||
- [ ] PR-3: Frontend Core (NC-PR3-001 through NC-PR3-012)
|
||||
- [ ] PR-4: Toast Coordination (NC-PR4-001 through NC-PR4-006)
|
||||
|
||||
## PR Boundary
|
||||
|
||||
This PR covers PR-1 only (NC-PR1-001 through NC-PR1-011). PR-2 (backend integration) and PR-3/PR-4 (frontend) are out of scope.
|
||||
This progress covers PR-1 and PR-2. PR-3 (frontend core — NotificationProvider, useNotifications, NotificationCenter, NotificationItem, styles, AppShell integration) and PR-4 (toast coordination — EventToastBridge preferences, settings UI) are out of scope.
|
||||
|
||||
Reference in New Issue
Block a user