From 1fb397324a744c516415bb6a7f95f83b139b160c Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 6 Sep 2026 09:17:45 -0600 Subject: [PATCH 1/2] feat(announcements): ack funnel counters + GET /{id}/stats (PR-6 backend) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/stats` needs a count of acks across users, which the key shape does not support: acks live under `USER#` partitions, so counting them per announcement means a GSI on `announcementId` or a scan. The spec ranks those second and third and says start with atomic counters on the announcement item (§9). This does. The counters are top-level attributes — `ackCountsR1Seen` and friends — not a nested `ackCounts` map, because DynamoDB's `ADD` only works on top-level attributes and creates 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 exists, so every announcement authored before this shipped would need an init-then-retry branch on the ack hot path. They count users, not clicks. `record_ack` now reads the previous rank via `ReturnValues="UPDATED_OLD"` and bumps only the ranks the write crossed, so `seen` then `dismissed` adds one to each rather than two to `seen`. They are a funnel, not a partition: acknowledged implies dismissed implies seen, so `seen >= dismissed >= acknowledged` holds without ever reading them back. Keyed by revision, because "Show again" (§D4) is a deliberate re-broadcast and rolling its acks into the previous revision's totals would inflate them and make the numbers lie about the version people actually saw. **The bug worth reading twice:** every admin mutation — `update_announcement`, `set_state`, `bump_revision` — is a full `put_item` of the `Announcement` dataclass, so any attribute the model does not carry is destroyed by it. Publishing an announcement, the most common admin action there is, silently zeroed every counter. `Announcement.ack_counts` now carries them through read → write. Four regression tests cover publish, archive, edit, and continued accrual afterwards. `targeted` is answerable only for a `"*"` audience, via a COUNT query on the users table's StatusLoginIndex. That index is projected INCLUDE without `roles`, so a role-filtered count has nothing to evaluate against, and the alternatives are worse than an honest null: replacing a GSI on the users table (CFN reports green well before an index is ACTIVE), or the scan the spec ranks last. Nor is there a membership list to count — roles arrive as JWT claims mapped at login. Null means "not estimated", never zero. Increments are best-effort by design: a second write after the ack is already durable, logged and swallowed on failure. An under-counted stat beats turning a successful acknowledgement into a 500. 18 new tests; full backend suite 2329 passed. Co-Authored-By: Claude Opus 5 --- .../app_api/admin/announcements/routes.py | 36 +- .../src/apis/shared/announcements/models.py | 78 +++++ .../apis/shared/announcements/repository.py | 112 ++++++- .../src/apis/shared/announcements/service.py | 67 ++++ .../apis/shared/announcements/visibility.py | 14 +- backend/src/apis/shared/users/repository.py | 35 ++ .../tests/shared/test_announcements_stats.py | 311 ++++++++++++++++++ 7 files changed, 641 insertions(+), 12 deletions(-) create mode 100644 backend/tests/shared/test_announcements_stats.py diff --git a/backend/src/apis/app_api/admin/announcements/routes.py b/backend/src/apis/app_api/admin/announcements/routes.py index 5b6eee6d8..4e8d3f303 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 29431fcb5..f1014213f 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,36 @@ 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.** + """ + + 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 ee989f8fc..1fee93730 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 14dc5e033..aa6c4a451 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 0d650af6e..eb6dce8e9 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 2f375f077..88b0149bf 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 000000000..3c6d932da --- /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 From 18d33ccef2533b780982bc4d451a1d290e782f69 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 6 Sep 2026 09:31:12 -0600 Subject: [PATCH 2/2] feat(announcements): show reach on the admin list (PR-6 frontend) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes PR-6. The admin list now carries a reach line per announcement — "2 seen · 0 dismissed — of ~68 targeted (estimate)" — which is the point of the whole surface: it tells you whether any of this works. Rendered as a funnel, not a partition. "12 seen · 8 dismissed" means 8 of those 12, because the stored rank only ever rises through them (§D2). `acknowledged` appears only where one was actually asked for; on an announcement without `requiresAck` the number is real but meaningless, and showing a third figure that is always equal to the second reads as a bug. Two cases render nothing rather than a zero: - **A draft.** Nothing has been shown, so "0 seen" would read as "nobody engaged" instead of "not sent yet". `hasReach` gates on published/archived, which also keeps the fetch off every row an admin is still writing. - **A role-scoped audience.** `targeted` is null there — the users table's StatusLoginIndex does not project `roles` — and "of ~0" would imply nobody is targeted. It says "audience not estimated" instead. Stats are a second endpoint per announcement, so they load after the list rather than blocking it, and only for rows that have been live. The cache is keyed by **id plus revision**: "Show again" restarts the counters, so an entry from the previous revision would report stale reach for a broadcast that has only just gone out. A failed fetch is dropped from the requested set so the next pass retries, and leaves the row without a reach line rather than blanking the list — the page's actual job is CRUD. The hover text and the "(estimate)" suffix carry the §11 caveat. One more is now documented on the response model: **nothing is backfilled.** The counters are incremented by the ack write path, so acks recorded before this ships are invisible — an existing environment starts every announcement at zero on deploy day even where people have already read and dismissed it. Verified against dev, where four ack rows predate the counters and only the two written since are tallied. 7 new service specs, 8 new page specs; full frontend suite 2486 passed. Co-Authored-By: Claude Opus 5 --- .../src/apis/shared/announcements/models.py | 7 + .../manage-announcements.page.spec.ts | 97 ++++++++- .../manage-announcements.page.ts | 61 +++++- .../models/announcement.model.ts | 23 +++ .../announcements-admin.service.spec.ts | 188 ++++++++++++++++++ .../services/announcements-admin.service.ts | 81 ++++++++ 6 files changed, 455 insertions(+), 2 deletions(-) create mode 100644 frontend/ai.client/src/app/admin/manage-announcements/services/announcements-admin.service.spec.ts diff --git a/backend/src/apis/shared/announcements/models.py b/backend/src/apis/shared/announcements/models.py index f1014213f..7ff90ce8b 100644 --- a/backend/src/apis/shared/announcements/models.py +++ b/backend/src/apis/shared/announcements/models.py @@ -488,6 +488,13 @@ class AnnouncementStatsResponse(BaseModel): 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 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 78c7db3a2..ab65e7cf3 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 c7dc988c7..78fb40703 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 fef4ffd6a..09504e8bf 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 000000000..838cfdb27 --- /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 948470e30..a731feca4 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()); } }