diff --git a/backend/src/apis/app_api/admin/announcements/routes.py b/backend/src/apis/app_api/admin/announcements/routes.py index 5b6eee6d..4e8d3f30 100644 --- a/backend/src/apis/app_api/admin/announcements/routes.py +++ b/backend/src/apis/app_api/admin/announcements/routes.py @@ -1,9 +1,8 @@ """Admin API routes for feature announcements. -Authoring, scheduling, and lifecycle for the notices users see. There is no -user-facing counterpart yet — PR-2 adds ``GET /announcements`` and the ack -endpoint. Until then this surface ships dark: admins can author drafts and -nothing renders anywhere. +Authoring, scheduling, and lifecycle for the notices users see, plus the +reach numbers for one. The user-facing counterpart is ``GET /announcements`` +and its ack endpoint in ``apis/app_api/announcements/``. See ``docs/specs/feature-announcements.md`` §6. """ @@ -18,6 +17,7 @@ AnnouncementState, AnnouncementListResponse, AnnouncementResponse, + AnnouncementStatsResponse, AnnouncementUpdate, ) from apis.shared.announcements.service import get_announcements_service @@ -202,3 +202,31 @@ async def delete_announcement( deleted = await service.delete_announcement(announcement_id) if not deleted: raise _not_found(announcement_id) + + +@router.get( + "/{announcement_id}/stats", + response_model=AnnouncementStatsResponse, + summary="Reach for one announcement", +) +async def get_announcement_stats( + announcement_id: str, + admin_user: User = Depends(require_announcements_admin), +) -> AnnouncementStatsResponse: + """Funnel counts for the announcement's **current** revision. + + ``seen``/``dismissed``/``acknowledged`` are cumulative, not disjoint — a + user who acknowledged is counted in all three, because the stored rank + only ever rises through them (§D2). + + Every number is approximate and the UI must say so (§11): the counters are + incremented on a second write after the ack lands, and ``targeted`` is a + denominator that moves as people join and roles change. ``targeted`` is + null when the audience is role-scoped rather than everyone — that means + "not estimated", not zero. + """ + service = get_announcements_service() + stats = await service.get_stats(announcement_id) + if stats is None: + raise _not_found(announcement_id) + return stats diff --git a/backend/src/apis/shared/announcements/models.py b/backend/src/apis/shared/announcements/models.py index 29431fcb..7ff90ce8 100644 --- a/backend/src/apis/shared/announcements/models.py +++ b/backend/src/apis/shared/announcements/models.py @@ -39,6 +39,9 @@ ANNOUNCEMENT_SK_PREFIX = "ANNOUNCEMENT#" ACK_SK_PREFIX = "ACK#" +#: ``targetRoles`` entry meaning "everyone" (§D9). A display filter, never a grant. +TARGET_EVERYONE = "*" + TITLE_MAX_LENGTH = 140 BODY_MAX_BYTES = 16 * 1024 @@ -58,6 +61,29 @@ ACK_TTL_AFTER_EXPIRY = timedelta(days=90) ACK_TTL_OPEN_ENDED = timedelta(days=730) +#: Ack counters live as **top-level** attributes on the announcement item, +#: one per (revision, action) — ``ackCountsR1Seen`` and friends. +#: +#: Top-level rather than a nested ``ackCounts`` map for one reason: +#: DynamoDB's ``ADD`` only works on top-level attributes, and it creates the +#: attribute (treating a missing one as 0) in the same atomic write. A nested +#: map needs ``SET path = if_not_exists(path, :zero) + :one``, which raises +#: ValidationException until the parent map exists — so every announcement +#: authored before this shipped would need an init-then-retry path around +#: every ack. One atomic write with no fallback beats a tidier shape with a +#: repair branch on the hot path. +#: +#: Keyed by revision because "Show again" (§D4) is a deliberate re-broadcast: +#: rolling its acks into the previous revision's totals would silently inflate +#: them and make the numbers lie about the version people actually saw. +ACK_COUNT_ATTR_PREFIX = "ackCounts" + + +def ack_count_attr(revision: int, action: str) -> str: + """Attribute name holding the count of users who reached ``action``.""" + return f"{ACK_COUNT_ATTR_PREFIX}R{int(revision)}{action.capitalize()}" + + _LOUD_SURFACES = frozenset({"banner", "modal"}) @@ -144,6 +170,20 @@ class Announcement: cta_url: Optional[str] = None revision: int = 1 created_by: Optional[str] = None + #: Ack funnel counters, carried through read → write. + #: + #: **Not domain state — a projection that must survive.** Every admin + #: mutation (``update_announcement``, ``set_state``, ``bump_revision``) + #: is a full ``put_item`` of this dataclass, so any attribute the model + #: does not know about is destroyed by it. Without this field, publishing + #: or archiving an announcement — or hitting "Show again" — would silently + #: zero every stat the feature exists to report. + #: + #: The read-modify-write does mean an ack landing in the same instant as + #: an admin edit can lose its increment. That is the documented cost of + #: approximate O(1) counters (§9); admin writes are rare, and the ack + #: itself is never at risk because it is a different item. + ack_counts: Dict[str, int] = field(default_factory=dict) def to_dynamo_item(self) -> Dict[str, Any]: item: Dict[str, Any] = { @@ -173,6 +213,9 @@ def to_dynamo_item(self) -> Dict[str, Any]: item["ctaUrl"] = self.cta_url if self.created_by: item["createdBy"] = self.created_by + # Carried forward verbatim so a full-item put cannot destroy them. + for attr, value in (self.ack_counts or {}).items(): + item[attr] = int(value) return item @classmethod @@ -205,6 +248,11 @@ def from_dynamo_item(cls, item: Dict[str, Any]) -> "Announcement": created_at=created_at, updated_at=updated_at, created_by=item.get("createdBy"), + ack_counts={ + key: int(value) + for key, value in item.items() + if key.startswith(ACK_COUNT_ATTR_PREFIX) + }, ) def ack_ttl(self, action: str) -> Optional[int]: @@ -422,6 +470,43 @@ class AnnouncementListResponse(BaseModel): total: int +class AnnouncementStatsResponse(BaseModel): + """Reach for one announcement, at its **current** revision. + + The three counts are a **funnel, not a partition**: a user who + acknowledged also counts as dismissed and as seen, because the stored rank + only ever rises through them (§D2). So ``seen >= dismissed >= + acknowledged`` always holds, and "how many only ever saw it" is + ``seen - dismissed``. Reading them as disjoint buckets would understate + every stage. + + Everything here is approximate by construction and must be labelled that + way in the UI (§11): + + - the counts are incremented on a **second** write after the ack itself + lands, so a failure between the two under-counts by one. That is the + documented trade for O(1) stats with no GSI and no scan. + - ``targeted`` is a denominator that moves as people join and roles + change. **Do not build compliance reporting on it.** + - **Nothing is backfilled.** The counters are incremented by the ack write + path, so acks recorded before this shipped are invisible here — an + existing environment starts every announcement at zero on deploy day + even where people have already read and dismissed it. The ack rows + themselves are intact; only the tallies begin at the deploy. There is no + cheap repair for this (counting the existing rows is the scan the design + exists to avoid), so read early numbers as "reach since stats shipped". + """ + + announcement_id: str + revision: int + seen: int + dismissed: int + acknowledged: int + #: Active users this announcement is aimed at, or None when the audience + #: cannot be counted — see ``AnnouncementsService.get_stats``. + targeted: Optional[int] = None + + # ============================================================================= # User-facing response models # diff --git a/backend/src/apis/shared/announcements/repository.py b/backend/src/apis/shared/announcements/repository.py index ee989f8f..1fee9373 100644 --- a/backend/src/apis/shared/announcements/repository.py +++ b/backend/src/apis/shared/announcements/repository.py @@ -14,7 +14,7 @@ import logging import os import uuid -from typing import List, Optional +from typing import Dict, List, Optional import boto3 from botocore.exceptions import ClientError @@ -23,12 +23,14 @@ from .models import ( ACK_SK_PREFIX, + ACTION_RANKS, ANNOUNCEMENT_SK_PREFIX, ANNOUNCEMENTS_PK, Announcement, AnnouncementAck, AnnouncementCreate, AnnouncementUpdate, + ack_count_attr, action_rank, validate_announcement_invariants, ) @@ -318,7 +320,7 @@ async def record_ack( expression_values[":ttl"] = int(ttl) try: - self._table.update_item( + response = self._table.update_item( Key={ "PK": AnnouncementAck.partition_key(user_id), "SK": AnnouncementAck.sort_key(announcement_id, revision), @@ -329,6 +331,11 @@ async def record_ack( ), ExpressionAttributeNames=names, ExpressionAttributeValues=expression_values, + # The rank this user held *before* this write. Absent when the + # item is new, which reads as 0. It is what makes the counters + # count users rather than clicks: without it a `seen` followed + # by a `dismissed` would add two to the seen total. + ReturnValues="UPDATED_OLD", ) except ClientError as e: if e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException": @@ -344,8 +351,109 @@ async def record_ack( return False logger.error("Error recording announcement ack", exc_info=True) raise + + previous_rank = int(response.get("Attributes", {}).get("actionRank", 0)) + self._increment_ack_counts( + announcement_id=announcement_id, + revision=revision, + from_rank=previous_rank, + to_rank=rank, + ) return True + def _increment_ack_counts( + self, + *, + announcement_id: str, + revision: int, + from_rank: int, + to_rank: int, + ) -> None: + """Bump the funnel counters for every rank this write crossed. + + A user landing straight on ``acknowledged`` crosses all three, so all + three rise; one already at ``dismissed`` crosses only the third. That + is what keeps ``seen >= dismissed >= acknowledged`` true without ever + reading the counters back. + + **Best-effort on purpose.** This is a second write after the ack has + already been durably recorded, and the ack is the record that matters + (§D2/§D3). A failure here is logged and swallowed: an under-counted + stat is a worse dashboard, while raising would turn a successful + acknowledgement into a 500 and lose the user's click. The spec asks + for exactly this trade — approximate, O(1), no GSI, no scan. + """ + crossed = [ + action + for action, action_value in ACTION_RANKS.items() + if from_rank < action_value <= to_rank + ] + if not crossed: + return + + names = {} + values = {":one": 1} + clauses = [] + for index, action in enumerate(crossed): + alias = f"#c{index}" + names[alias] = ack_count_attr(revision, action) + clauses.append(f"{alias} :one") + + try: + # ADD, not SET: it creates a missing attribute as 0 in the same + # atomic write, so announcements authored before stats shipped + # need no backfill and no init-then-retry branch. + self._table.update_item( + Key={ + "PK": ANNOUNCEMENTS_PK, + "SK": f"{ANNOUNCEMENT_SK_PREFIX}{announcement_id}", + }, + UpdateExpression="ADD " + ", ".join(clauses), + ExpressionAttributeNames=names, + ExpressionAttributeValues=values, + ) + except ClientError: + logger.warning( + "Ack recorded but its stats counters were not incremented: " + "announcement=%s revision=%s crossed=%s", + announcement_id, + revision, + crossed, + exc_info=True, + ) + + async def get_ack_counts( + self, announcement_id: str, revision: int + ) -> Dict[str, int]: + """Funnel counts for one revision, zero-filled. + + Reads the announcement item itself — the counters live on it, which is + the whole point of the design: no GSI on ``announcementId`` and no + scan of the ack partitions. + """ + zero = {action: 0 for action in ACTION_RANKS} + if not self._enabled: + return zero + + try: + response = self._table.get_item( + Key={ + "PK": ANNOUNCEMENTS_PK, + "SK": f"{ANNOUNCEMENT_SK_PREFIX}{announcement_id}", + } + ) + except ClientError: + logger.error("Error reading announcement ack counts", exc_info=True) + raise + + item = response.get("Item") + if not item: + return zero + return { + action: int(item.get(ack_count_attr(revision, action), 0)) + for action in ACTION_RANKS + } + async def list_acks(self, user_id: str) -> List[AnnouncementAck]: """Every ack this user has ever written, across revisions.""" if not self._enabled: diff --git a/backend/src/apis/shared/announcements/service.py b/backend/src/apis/shared/announcements/service.py index 14dc5e03..aa6c4a45 100644 --- a/backend/src/apis/shared/announcements/service.py +++ b/backend/src/apis/shared/announcements/service.py @@ -15,10 +15,14 @@ from datetime import datetime, timezone from typing import List, Optional, Sequence +from apis.shared.users.repository import UserRepository + from .models import ( + TARGET_EVERYONE, Announcement, AnnouncementAck, AnnouncementCreate, + AnnouncementStatsResponse, AnnouncementUpdate, ) from .repository import AnnouncementsRepository, get_announcements_repository @@ -128,6 +132,69 @@ async def get_ack( ) -> Optional[AnnouncementAck]: return await self._repo.get_ack(user_id, announcement_id, revision) + async def get_stats( + self, announcement_id: str + ) -> Optional[AnnouncementStatsResponse]: + """Reach for one announcement at its current revision, or None if gone. + + Counts come from the counters on the announcement item itself — no GSI + on ``announcementId``, no scan of the ack partitions. They are a + funnel, not a partition (see ``AnnouncementStatsResponse``). + """ + announcement = await self._repo.get_announcement(announcement_id) + if announcement is None: + return None + + counts = await self._repo.get_ack_counts( + announcement_id, announcement.revision + ) + return AnnouncementStatsResponse( + announcement_id=announcement_id, + revision=announcement.revision, + seen=counts.get("seen", 0), + dismissed=counts.get("dismissed", 0), + acknowledged=counts.get("acknowledged", 0), + targeted=await self._estimate_targeted(announcement), + ) + + async def _estimate_targeted( + self, announcement: Announcement + ) -> Optional[int]: + """Roughly how many active users this announcement is aimed at. + + **Only answerable for a ``"*"`` audience, and None otherwise.** The + count comes from a ``Select="COUNT"`` query on the users table's + ``StatusLoginIndex`` — but that index is projected ``INCLUDE`` with + ``userId``/``email``/``name``/``emailDomain`` and **not** ``roles``, so + a role-filtered count cannot be evaluated against it. The alternatives + are both worse than an honest None: widening the projection means + replacing a GSI on the users table (and CFN reporting green well + before the index is ACTIVE), while a filtered table scan is the + option the spec ranks last for exactly this reason. + + Nor is there a membership list to count instead: roles arrive as JWT + claims mapped at login, so nothing stores "who holds this role". + + None means "not estimated" and the UI must say so — it does **not** + mean zero. Even the ``"*"`` number is an estimate that moves as people + join, and §11 is explicit that no compliance reporting should be built + on it. + """ + if TARGET_EVERYONE not in (announcement.target_roles or []): + return None + try: + users = UserRepository() + if not users.enabled: + return None + return await users.count_active_users() + except Exception: + logger.warning( + "Could not estimate the targeted audience for %s", + announcement.announcement_id, + exc_info=True, + ) + return None + # ── User-facing feed ───────────────────────────────────────────────── async def build_feed( diff --git a/backend/src/apis/shared/announcements/visibility.py b/backend/src/apis/shared/announcements/visibility.py index 0d650af6..eb6dce8e 100644 --- a/backend/src/apis/shared/announcements/visibility.py +++ b/backend/src/apis/shared/announcements/visibility.py @@ -21,7 +21,12 @@ from apis.shared.timestamps import from_iso -from .models import SUPPRESSING_RANK, Announcement, AnnouncementAck +from .models import ( + SUPPRESSING_RANK, + TARGET_EVERYONE, + Announcement, + AnnouncementAck, +) logger = logging.getLogger(__name__) @@ -31,9 +36,6 @@ #: goes first. Lower sorts earlier. SEVERITY_ORDER: Dict[str, int] = {"warning": 0, "success": 1, "info": 2} -_TARGET_EVERYONE = "*" - - @dataclass class VisibleAnnouncement: """One announcement plus this user's relationship to it.""" @@ -97,8 +99,8 @@ def _targets_user(announcement: Announcement, roles: Sequence[str]) -> bool: There is no ``can_access_*`` predicate behind this and nothing is inherited; it decides what a notice board shows, not what a user may do. """ - targets = announcement.target_roles or [_TARGET_EVERYONE] - if _TARGET_EVERYONE in targets: + targets = announcement.target_roles or [TARGET_EVERYONE] + if TARGET_EVERYONE in targets: return True return bool(set(targets) & set(roles or [])) diff --git a/backend/src/apis/shared/users/repository.py b/backend/src/apis/shared/users/repository.py index 2f375f07..88b0149b 100644 --- a/backend/src/apis/shared/users/repository.py +++ b/backend/src/apis/shared/users/repository.py @@ -267,6 +267,41 @@ async def list_users_by_status( logger.error(f"Error listing users by status {status}: {e}") return [], None + async def count_active_users(self) -> Optional[int]: + """How many users are active, via a COUNT query on StatusLoginIndex. + + ``Select="COUNT"`` returns only a tally, so nothing is transferred per + user — but DynamoDB still pages, hence the loop. Callers use this as a + denominator for "roughly how many people is this aimed at"; it moves as + people join and sign in, so treat it as an estimate. + + **Cannot be filtered by role.** That index is projected ``INCLUDE`` + with userId/email/name/emailDomain and not ``roles``, so a filter on + roles has nothing to evaluate against. Returns None if the repository + is disabled or the query fails, which means "unknown", never zero. + """ + if not self._enabled: + return None + + try: + total = 0 + kwargs: dict = { + "IndexName": "StatusLoginIndex", + "KeyConditionExpression": "GSI3PK = :pk", + "ExpressionAttributeValues": {":pk": "STATUS#active"}, + "Select": "COUNT", + } + while True: + response = self.table.query(**kwargs) + total += int(response.get("Count", 0)) + last_key = response.get("LastEvaluatedKey") + if not last_key: + return total + kwargs["ExclusiveStartKey"] = last_key + except ClientError as e: + logger.error(f"Error counting active users: {e}") + return None + # ========== Helper Methods ========== def _profile_to_item(self, profile: UserProfile) -> dict: diff --git a/backend/tests/shared/test_announcements_stats.py b/backend/tests/shared/test_announcements_stats.py new file mode 100644 index 00000000..3c6d932d --- /dev/null +++ b/backend/tests/shared/test_announcements_stats.py @@ -0,0 +1,311 @@ +"""Tests for the announcement stats funnel (PR-6, spec §9). + +The counters are the whole subject. They exist because ``/stats`` needs a +count of acks across users and the key shape does not support one — so the +announcement item carries per-(revision, action) tallies that the ack write +bumps. Two properties matter and both are easy to break: + +1. They count **users, not clicks**. A user who goes ``seen`` → ``dismissed`` + adds one to each, not two to ``seen``. +2. They are a **funnel, not a partition**. ``acknowledged`` implies + ``dismissed`` implies ``seen``, so the three are always non-increasing. +""" + +import boto3 +import pytest + +from apis.shared.announcements.models import ( + AnnouncementCreate, + ack_count_attr, +) +from apis.shared.announcements.repository import AnnouncementsRepository +from apis.shared.announcements.service import AnnouncementsService + +AWS_REGION = "us-west-2" +TABLE_NAME = "test-announcements-stats" + +PAST = "2020-01-01T00:00:00Z" +# `expiresAt` is required whenever a loud surface is selected. +FUTURE = "2099-01-01T00:00:00Z" + + +@pytest.fixture() +def announcements_table(aws, monkeypatch): + ddb = boto3.client("dynamodb", region_name=AWS_REGION) + ddb.create_table( + TableName=TABLE_NAME, + KeySchema=[ + {"AttributeName": "PK", "KeyType": "HASH"}, + {"AttributeName": "SK", "KeyType": "RANGE"}, + ], + AttributeDefinitions=[ + {"AttributeName": "PK", "AttributeType": "S"}, + {"AttributeName": "SK", "AttributeType": "S"}, + ], + BillingMode="PAY_PER_REQUEST", + ) + monkeypatch.setenv("DYNAMODB_ANNOUNCEMENTS_TABLE_NAME", TABLE_NAME) + return boto3.resource("dynamodb", region_name=AWS_REGION).Table(TABLE_NAME) + + +@pytest.fixture() +def repo(announcements_table): + return AnnouncementsRepository(table_name=TABLE_NAME, region=AWS_REGION) + + +@pytest.fixture() +def service(repo): + return AnnouncementsService(repo) + + +def _create(**kw) -> AnnouncementCreate: + defaults = dict( + title="Acceptable use policy update", + body_markdown="# Policy", + publish_at=PAST, + expires_at=FUTURE, + surfaces=["panel", "modal"], + ) + defaults.update(kw) + return AnnouncementCreate(**defaults) + + +async def _ack(service, announcement, user_id, action, surface="modal"): + return await service.record_ack( + user_id=user_id, + announcement=announcement, + action=action, + surface=surface, + ) + + +class TestAckCounters: + @pytest.mark.asyncio + async def test_first_ack_counts_one_user(self, service, repo): + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "seen") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 0, "acknowledged": 0} + + @pytest.mark.asyncio + async def test_rising_through_ranks_counts_the_user_once_per_rank( + self, service, repo + ): + """The `UPDATED_OLD` read is what makes this true. + + Without the previous rank, `seen` then `dismissed` would add two to + the seen total — counting clicks instead of people. + """ + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "seen") + await _ack(service, announcement, "u1", "dismissed") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 1, "acknowledged": 0} + + @pytest.mark.asyncio + async def test_jumping_straight_to_acknowledged_fills_the_funnel( + self, service, repo + ): + """A `requiresAck` modal writes `seen` then `acknowledged`, but a user + who never saw the intermediate state must still count at every rung — + otherwise `seen` would understate reach.""" + announcement = await service.create_announcement( + _create(requires_ack=True) + ) + await _ack(service, announcement, "u1", "acknowledged") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 1, "acknowledged": 1} + + @pytest.mark.asyncio + async def test_a_weaker_late_ack_does_not_double_count(self, service, repo): + """§D2's straggler: `seen` arriving after `dismissed` is rejected by + the conditional write, so it must not touch the counters either.""" + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "dismissed") + raised = await _ack(service, announcement, "u1", "seen") + + assert raised is False + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 1, "acknowledged": 0} + + @pytest.mark.asyncio + async def test_repeating_the_same_ack_is_idempotent(self, service, repo): + announcement = await service.create_announcement(_create()) + for _ in range(4): + await _ack(service, announcement, "u1", "seen") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts["seen"] == 1 + + @pytest.mark.asyncio + async def test_counts_are_per_user(self, service, repo): + announcement = await service.create_announcement(_create()) + for user in ("u1", "u2", "u3"): + await _ack(service, announcement, user, "seen") + await _ack(service, announcement, "u2", "dismissed") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 3, "dismissed": 1, "acknowledged": 0} + + @pytest.mark.asyncio + async def test_funnel_is_never_increasing(self, service, repo): + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "acknowledged") + await _ack(service, announcement, "u2", "dismissed") + await _ack(service, announcement, "u3", "seen") + + c = await repo.get_ack_counts(announcement.announcement_id, 1) + assert c["seen"] >= c["dismissed"] >= c["acknowledged"] + assert c == {"seen": 3, "dismissed": 2, "acknowledged": 1} + + +class TestRevisionScoping: + @pytest.mark.asyncio + async def test_revise_starts_a_fresh_count(self, service, repo): + """"Show again" is a deliberate re-broadcast (§D4). + + Rolling the new revision's acks into the old totals would inflate them + and make the numbers lie about the version people actually saw. + """ + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "dismissed") + + revised = await service.revise(announcement.announcement_id) + assert revised.revision == 2 + + assert await repo.get_ack_counts(announcement.announcement_id, 2) == { + "seen": 0, + "dismissed": 0, + "acknowledged": 0, + } + # The old revision's history is untouched and still readable. + assert await repo.get_ack_counts(announcement.announcement_id, 1) == { + "seen": 1, + "dismissed": 1, + "acknowledged": 0, + } + + @pytest.mark.asyncio + async def test_counters_live_on_the_announcement_item( + self, service, announcements_table + ): + """No GSI and no scan — that is the point of the design.""" + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "seen") + + item = announcements_table.get_item( + Key={ + "PK": "ANNOUNCEMENTS", + "SK": f"ANNOUNCEMENT#{announcement.announcement_id}", + } + )["Item"] + assert item[ack_count_attr(1, "seen")] == 1 + + +class TestCountersSurviveAdminWrites: + """Every admin mutation is a full `put_item` of the Announcement model. + + So any attribute the model does not carry is destroyed by it. These are + the regression: without `Announcement.ack_counts`, publishing an + announcement — the single most common admin action — silently zeroed + every stat the feature exists to report. + """ + + @pytest.mark.asyncio + async def test_publishing_preserves_counts(self, service, repo): + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "seen") + + await service.publish(announcement.announcement_id) + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts["seen"] == 1 + + @pytest.mark.asyncio + async def test_archiving_preserves_counts(self, service, repo): + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "acknowledged") + + await service.publish(announcement.announcement_id) + await service.archive(announcement.announcement_id) + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 1, "acknowledged": 1} + + @pytest.mark.asyncio + async def test_editing_the_body_preserves_counts(self, service, repo): + from apis.shared.announcements.models import AnnouncementUpdate + + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "dismissed") + + await service.update_announcement( + announcement.announcement_id, + AnnouncementUpdate(title="Acceptable use policy update (typo fix)"), + ) + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts == {"seen": 1, "dismissed": 1, "acknowledged": 0} + + @pytest.mark.asyncio + async def test_acks_keep_accruing_after_an_admin_write(self, service, repo): + """The counters must stay live, not merely survive once.""" + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "seen") + + published = await service.publish(announcement.announcement_id) + await _ack(service, published, "u2", "seen") + + counts = await repo.get_ack_counts(announcement.announcement_id, 1) + assert counts["seen"] == 2 + + +class TestGetStats: + @pytest.mark.asyncio + async def test_reports_the_current_revision(self, service): + announcement = await service.create_announcement(_create()) + await _ack(service, announcement, "u1", "acknowledged") + + stats = await service.get_stats(announcement.announcement_id) + assert stats.announcement_id == announcement.announcement_id + assert stats.revision == 1 + assert (stats.seen, stats.dismissed, stats.acknowledged) == (1, 1, 1) + + @pytest.mark.asyncio + async def test_unknown_announcement_is_none(self, service): + assert await service.get_stats("nope") is None + + @pytest.mark.asyncio + async def test_zero_filled_before_anyone_acks(self, service): + announcement = await service.create_announcement(_create()) + stats = await service.get_stats(announcement.announcement_id) + assert (stats.seen, stats.dismissed, stats.acknowledged) == (0, 0, 0) + + @pytest.mark.asyncio + async def test_targeted_is_none_for_a_role_scoped_audience(self, service): + """None means "not estimated", never zero. + + The users table's StatusLoginIndex does not project `roles`, so a + role-filtered count has nothing to evaluate against — and the UI must + say the audience is unknown rather than imply nobody is targeted. + """ + announcement = await service.create_announcement( + _create(target_roles=["faculty"]) + ) + stats = await service.get_stats(announcement.announcement_id) + assert stats.targeted is None + + @pytest.mark.asyncio + async def test_targeted_is_none_when_the_user_directory_is_unavailable( + self, service, monkeypatch + ): + """A directory blip must not be reported as an audience of zero.""" + monkeypatch.delenv("DYNAMODB_USERS_TABLE_NAME", raising=False) + announcement = await service.create_announcement( + _create(target_roles=["*"]) + ) + stats = await service.get_stats(announcement.announcement_id) + assert stats.targeted is None diff --git a/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.spec.ts b/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.spec.ts index 78c7db3a..ab65e7cf 100644 --- a/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.spec.ts +++ b/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.spec.ts @@ -3,7 +3,11 @@ import { TestBed } from '@angular/core/testing'; import { signal } from '@angular/core'; import { ManageAnnouncementsPage } from './manage-announcements.page'; import { AnnouncementsAdminService } from './services/announcements-admin.service'; -import { Announcement, AnnouncementState } from './models/announcement.model'; +import { + Announcement, + AnnouncementState, + AnnouncementStats, +} from './models/announcement.model'; function makeAnnouncement(overrides: Partial = {}): Announcement { return { @@ -29,17 +33,33 @@ function makeAnnouncement(overrides: Partial = {}): Announcement { }; } +function makeStats(overrides: Partial = {}): AnnouncementStats { + return { + announcement_id: 'a1', + revision: 1, + seen: 12, + dismissed: 8, + acknowledged: 3, + targeted: 40, + ...overrides, + }; +} + describe('ManageAnnouncementsPage', () => { let items: ReturnType>; + let statsById: ReturnType>>; let service: any; let confirmSpy: ReturnType; beforeEach(() => { TestBed.resetTestingModule(); items = signal([]); + statsById = signal>(new Map()); service = { ensureLoaded: vi.fn(), announcements: items, + loadStats: vi.fn(async () => undefined), + statsFor: (id: string) => statsById().get(id) ?? null, announcementsResource: { isLoading: () => false, error: () => null }, publish: vi.fn(async () => makeAnnouncement({ state: 'published' })), archive: vi.fn(async () => makeAnnouncement({ state: 'archived' })), @@ -198,4 +218,79 @@ describe('ManageAnnouncementsPage', () => { expect(new Set(classes).size).toBe(states.length); }); }); + + describe('reach', () => { + it('says nothing for a draft — zeroes would read as "nobody engaged"', () => { + items.set([makeAnnouncement({ state: 'draft' })]); + statsById.set(new Map([['a1', makeStats()]])); + const page = createPage(); + expect(page.rows()[0].reach).toBeNull(); + }); + + it('says nothing until the stats have arrived', () => { + items.set([makeAnnouncement({ state: 'published' })]); + const page = createPage(); + expect(page.rows()[0].reach).toBeNull(); + }); + + it('renders the funnel once stats land', () => { + items.set([makeAnnouncement({ state: 'published' })]); + const page = createPage(); + statsById.set(new Map([['a1', makeStats()]])); + + const reach = page.rows()[0].reach!; + expect(reach).toContain('12 seen'); + expect(reach).toContain('8 dismissed'); + expect(reach).toContain('~40 targeted'); + expect(reach).toContain('estimate'); + }); + + it('omits acknowledged unless one was ever asked for', () => { + items.set([ + makeAnnouncement({ state: 'published', requires_ack: false }), + ]); + const page = createPage(); + statsById.set(new Map([['a1', makeStats()]])); + expect(page.rows()[0].reach).not.toContain('acknowledged'); + }); + + it('shows acknowledged for a requiresAck announcement', () => { + items.set([makeAnnouncement({ state: 'published', requires_ack: true })]); + const page = createPage(); + statsById.set(new Map([['a1', makeStats()]])); + expect(page.rows()[0].reach).toContain('3 acknowledged'); + }); + + it('says the audience is not estimated rather than implying zero', () => { + // A role-scoped announcement cannot be counted — `targeted` is null, and + // rendering that as "of ~0" would read as nobody being targeted. + items.set([ + makeAnnouncement({ state: 'published', target_roles: ['faculty'] }), + ]); + const page = createPage(); + statsById.set(new Map([['a1', makeStats({ targeted: null })]])); + + const reach = page.rows()[0].reach!; + expect(reach).toContain('audience not estimated'); + expect(reach).not.toContain('~0'); + }); + + it('still reports reach for an archived announcement', () => { + // Archiving stops it showing but keeps the acks — the record is the + // reason to archive rather than delete. + items.set([makeAnnouncement({ state: 'archived' })]); + const page = createPage(); + statsById.set(new Map([['a1', makeStats()]])); + expect(page.rows()[0].reach).toContain('12 seen'); + }); + + it('requests stats once the list resolves', () => { + items.set([makeAnnouncement({ state: 'published' })]); + createPage(); + // The fetch is an effect, so it needs a flush — the page is constructed + // directly here rather than through a fixture. + TestBed.tick(); + expect(service.loadStats).toHaveBeenCalled(); + }); + }); }); diff --git a/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.ts b/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.ts index c7dc988c..78fb4070 100644 --- a/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.ts +++ b/frontend/ai.client/src/app/admin/manage-announcements/manage-announcements.page.ts @@ -1,4 +1,11 @@ -import { ChangeDetectionStrategy, Component, computed, inject, signal } from '@angular/core'; +import { + ChangeDetectionStrategy, + Component, + computed, + effect, + inject, + signal, +} from '@angular/core'; import { RouterLink } from '@angular/router'; import { NgIcon, provideIcons } from '@ng-icons/core'; import { @@ -146,6 +153,16 @@ import { parseIso } from '../../utils/date'; · {{ item.audience }} }

+ + @if (item.reach; as reach) { +

+ Reach + {{ reach }} +

+ }
@@ -219,6 +236,16 @@ export class ManageAnnouncementsPage { constructor() { this.service.ensureLoaded(); + + // Reach is a second endpoint per announcement, so it is fetched once the + // list resolves rather than blocking it. `loadStats` skips ids it has + // already requested, so re-running on every list change is cheap and the + // effect cannot feed itself. + effect(() => { + const announcements = this.announcements(); + if (announcements.length === 0) return; + void this.service.loadStats(announcements); + }); } protected readonly announcements = this.service.announcements; @@ -239,9 +266,15 @@ export class ManageAnnouncementsPage { announcement, timing: this.describeTiming(announcement), audience: this.describeAudience(announcement), + reach: this.describeReach(announcement), })), ); + protected readonly reachHint = + 'Approximate. Counts are cumulative — anyone who acknowledged also ' + + 'counts as dismissed and as seen. The audience size is an estimate that ' + + 'moves as people join.'; + protected canPublish(a: Announcement): boolean { // Archived is terminal; the server refuses to publish out of it, so do not // offer a button that returns a 400. @@ -283,6 +316,32 @@ export class ManageAnnouncementsPage { return stripped.length > 140 ? stripped.slice(0, 140) + '…' : stripped; } + /** + * One line of reach, or null when there is nothing honest to say. + * + * Null for a draft (nothing has been shown, so a row of zeroes would read + * as "nobody engaged" rather than "not sent yet") and while the fetch is + * still in flight. + * + * The counts are a funnel, not a partition — see `AnnouncementStats`. They + * are rendered as such: "12 seen · 8 dismissed" means 8 of those 12, not 20 + * people. + */ + private describeReach(a: Announcement): string | null { + if (!AnnouncementsAdminService.hasReach(a)) return null; + const stats = this.service.statsFor(a.announcement_id); + if (!stats) return null; + + const parts = [`${stats.seen} seen`, `${stats.dismissed} dismissed`]; + // Only meaningful where an acknowledgement was ever asked for. + if (a.requires_ack) parts.push(`${stats.acknowledged} acknowledged`); + + const line = parts.join(' · '); + return stats.targeted != null + ? `${line} — of ~${stats.targeted} targeted (estimate)` + : `${line} (audience not estimated)`; + } + private describeTiming(a: Announcement): string { const published = this.formatDate(a.publish_at); const verb = a.state === 'published' ? 'Live since' : 'Publishes'; diff --git a/frontend/ai.client/src/app/admin/manage-announcements/models/announcement.model.ts b/frontend/ai.client/src/app/admin/manage-announcements/models/announcement.model.ts index fef4ffd6..09504e8b 100644 --- a/frontend/ai.client/src/app/admin/manage-announcements/models/announcement.model.ts +++ b/frontend/ai.client/src/app/admin/manage-announcements/models/announcement.model.ts @@ -72,3 +72,26 @@ export interface AnnouncementCreateRequest { export type AnnouncementUpdateRequest = Partial< Omit >; + + +/** + * `GET /admin/announcements/{id}/stats` — reach for the **current** revision. + * + * The three counts are a **funnel, not a partition**: the stored rank only + * ever rises through seen → dismissed → acknowledged (§D2), so someone who + * acknowledged is counted in all three and `seen >= dismissed >= + * acknowledged` always holds. "Only ever saw it" is `seen - dismissed`. + * Rendering them as disjoint buckets would understate every stage. + * + * All of it is approximate and the UI must say so (§11). `targeted` is null + * when the audience is role-scoped rather than everyone — that means **not + * estimated, not zero**. + */ +export interface AnnouncementStats { + announcement_id: string; + revision: number; + seen: number; + dismissed: number; + acknowledged: number; + targeted?: number | null; +} diff --git a/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.spec.ts b/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.spec.ts new file mode 100644 index 00000000..838cfdb2 --- /dev/null +++ b/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.spec.ts @@ -0,0 +1,188 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { TestBed } from '@angular/core/testing'; +import { + HttpTestingController, + provideHttpClientTesting, +} from '@angular/common/http/testing'; +import { provideHttpClient } from '@angular/common/http'; +import { ConfigService } from '../../../services/config.service'; +import { AnnouncementsService } from '../../../services/announcements/announcements.service'; +import { AnnouncementsAdminService } from './announcements-admin.service'; +import { Announcement } from '../models/announcement.model'; + +const API = 'http://api.test'; + +function makeAnnouncement(overrides: Partial = {}): Announcement { + return { + announcement_id: 'a1', + title: 'Skills are here', + body_markdown: '# Skills', + summary: null, + surfaces: ['panel'], + severity: 'info', + state: 'published', + publish_at: '2026-01-01T00:00:00Z', + expires_at: null, + target_roles: ['*'], + show_to_new_users: false, + requires_ack: false, + cta_label: null, + cta_url: null, + revision: 1, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + created_by: 'admin@example.com', + ...overrides, + }; +} + +describe('AnnouncementsAdminService — reach', () => { + let service: AnnouncementsAdminService; + let http: HttpTestingController; + + beforeEach(() => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + provideHttpClient(), + provideHttpClientTesting(), + { provide: ConfigService, useValue: { appApiUrl: () => API } }, + { provide: AnnouncementsService, useValue: { reload: vi.fn() } }, + ], + }); + service = TestBed.inject(AnnouncementsAdminService); + http = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + http.verify(); + TestBed.resetTestingModule(); + }); + + function statsUrl(id: string) { + return `${API}/admin/announcements/${id}/stats`; + } + + it('fetches reach for a published announcement', async () => { + const pending = service.loadStats([makeAnnouncement()]); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 1, + seen: 12, + dismissed: 8, + acknowledged: 3, + targeted: 40, + }); + await pending; + + expect(service.statsFor('a1')?.seen).toBe(12); + }); + + it('does not ask about a draft — nothing has been shown', async () => { + await service.loadStats([makeAnnouncement({ state: 'draft' })]); + http.expectNone(statsUrl('a1')); + expect(service.statsFor('a1')).toBeNull(); + }); + + it('asks once per announcement, however often the list re-renders', async () => { + const announcements = [makeAnnouncement()]; + const first = service.loadStats(announcements); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 1, + seen: 1, + dismissed: 0, + acknowledged: 0, + targeted: null, + }); + await first; + + await service.loadStats(announcements); + http.expectNone(statsUrl('a1')); + }); + + it('re-asks after a revision bump — the counters restart', async () => { + // "Show again" starts a fresh count, so a cached entry from the previous + // revision would report stale reach for a broadcast that just went out. + const first = service.loadStats([makeAnnouncement({ revision: 1 })]); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 1, + seen: 9, + dismissed: 9, + acknowledged: 0, + targeted: null, + }); + await first; + + const second = service.loadStats([makeAnnouncement({ revision: 2 })]); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 2, + seen: 1, + dismissed: 0, + acknowledged: 0, + targeted: null, + }); + await second; + + expect(service.statsFor('a1')?.revision).toBe(2); + expect(service.statsFor('a1')?.seen).toBe(1); + }); + + it('fails soft — a broken stats endpoint leaves the list usable', async () => { + const pending = service.loadStats([makeAnnouncement()]); + http.expectOne(statsUrl('a1')).flush('boom', { + status: 500, + statusText: 'Server Error', + }); + + await expect(pending).resolves.toBeUndefined(); + expect(service.statsFor('a1')).toBeNull(); + }); + + it('retries a failed fetch on the next pass rather than caching the failure', async () => { + const first = service.loadStats([makeAnnouncement()]); + http.expectOne(statsUrl('a1')).flush('boom', { + status: 500, + statusText: 'Server Error', + }); + await first; + + const second = service.loadStats([makeAnnouncement()]); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 1, + seen: 4, + dismissed: 0, + acknowledged: 0, + targeted: null, + }); + await second; + + expect(service.statsFor('a1')?.seen).toBe(4); + }); + + it('drops cached reach when a mutation lands', async () => { + const first = service.loadStats([makeAnnouncement()]); + http.expectOne(statsUrl('a1')).flush({ + announcement_id: 'a1', + revision: 1, + seen: 5, + dismissed: 0, + acknowledged: 0, + targeted: null, + }); + await first; + expect(service.statsFor('a1')).not.toBeNull(); + + const archived = service.archive('a1'); + http.expectOne(`${API}/admin/announcements/a1/archive`).flush( + makeAnnouncement({ state: 'archived' }), + ); + await archived; + + // Publishing/archiving/revising all change what reach means. + expect(service.statsFor('a1')).toBeNull(); + }); +}); diff --git a/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.ts b/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.ts index 948470e3..a731feca 100644 --- a/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.ts +++ b/frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.ts @@ -7,6 +7,7 @@ import { Announcement, AnnouncementCreateRequest, AnnouncementListResponse, + AnnouncementStats, AnnouncementUpdateRequest, } from '../models/announcement.model'; @@ -119,9 +120,89 @@ export class AnnouncementsAdminService { this.refresh(); } + // ── Reach (§9) ──────────────────────────────────────────────────────── + // + // Stats are a separate endpoint per announcement rather than a field on the + // list, so they are fetched into this map and read by id. Only announcements + // that have actually been shown are worth a request: a draft has by + // definition reached nobody, and asking would be N wasted round trips on the + // rows an admin is still writing. + + private readonly statsById = signal>( + new Map(), + ); + /** Ids already requested, so the loader is not re-entered on every render. */ + private readonly requestedStats = new Set(); + + readonly stats = this.statsById.asReadonly(); + + statsFor(id: string): AnnouncementStats | null { + return this.statsById().get(id) ?? null; + } + + /** Whether reach is meaningful — nothing has been shown before publication. */ + static hasReach(announcement: Announcement): boolean { + return ( + announcement.state === 'published' || announcement.state === 'archived' + ); + } + + /** + * Fetch reach for any of these that has been live and is not already + * loaded. Fails soft per id: a stats endpoint erroring must not blank the + * admin list, which is the page's actual job. + */ + async loadStats(announcements: Announcement[]): Promise { + const pending = announcements + .filter(a => AnnouncementsAdminService.hasReach(a)) + .filter(a => !this.requestedStats.has(this.statsKey(a))); + if (pending.length === 0) return; + + for (const a of pending) this.requestedStats.add(this.statsKey(a)); + + const results = await Promise.all( + pending.map(async a => { + try { + return await firstValueFrom( + this.http.get( + `${this.baseUrl()}/${a.announcement_id}/stats`, + ), + ); + } catch { + // Leave it absent; the row renders without a reach line. + this.requestedStats.delete(this.statsKey(a)); + return null; + } + }), + ); + + this.statsById.update(prev => { + const next = new Map(prev); + for (const stats of results) { + if (stats) next.set(stats.announcement_id, stats); + } + return next; + }); + } + + /** + * Keyed by revision, not just id. + * + * "Show again" bumps the revision and the counters restart, so a cached + * entry from the previous revision would show stale reach for a broadcast + * that has only just gone out. + */ + private statsKey(a: Announcement): string { + return `${a.announcement_id}#R${a.revision}`; + } + private refresh(): void { this.announcementsResource.reload(); // The admin is also a user: keep their own What's-New in step. this.userFeed.reload(); + // Publishing, archiving and revising all change what reach means, so drop + // the cache and let the list re-request it. + this.requestedStats.clear(); + this.statsById.set(new Map()); } }