From 9f97b98e19d56b2347373f0b8161234764f448d0 Mon Sep 17 00:00:00 2001 From: Sandro Date: Mon, 20 Jul 2026 10:09:04 +0100 Subject: [PATCH 1/6] feat(accounts): personal throughput Hero + wired range switcher (Story 3.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace the console-hero skeleton with a live, personal, server-rendered Hero: this-range headline + vs-last delta, range-invariant all-time odometer, copyable Slack one-liner, and a demoted (hidden-in-v1) estimate slot. - Count = merged DAIV MRs for the user via MergeMetric ⋈ Run (session__user), a read-only Exists-join; refactor get_velocity_data to take an optional queryset so the personal Hero and org Manager Lens share one counting body. - Wire the disabled range switcher (today/7d/30d/90d/all) to scope #console-main via hx-get + href fallback + Alpine active pill; flip DEFAULT_PERIOD today -> 7d. - Externalise all new strings (en + pt); a11y aria-live headline, CSS-only odometer. - Extend test_dashboard_console.py with the I/O matrix + ACs. --- daiv/accounts/locale/pt/LC_MESSAGES/django.po | 77 ++++- .../templates/accounts/_console_body.html | 13 +- daiv/accounts/templates/accounts/_hero.html | 107 +++++++ .../templates/accounts/dashboard.html | 64 +++-- daiv/accounts/views.py | 85 +++++- .../accounts/test_dashboard_console.py | 272 +++++++++++++++++- 6 files changed, 577 insertions(+), 41 deletions(-) create mode 100644 daiv/accounts/templates/accounts/_hero.html diff --git a/daiv/accounts/locale/pt/LC_MESSAGES/django.po b/daiv/accounts/locale/pt/LC_MESSAGES/django.po index 56043613..09238de4 100644 --- a/daiv/accounts/locale/pt/LC_MESSAGES/django.po +++ b/daiv/accounts/locale/pt/LC_MESSAGES/django.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: PACKAGE VERSION\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2026-07-19 13:16+0100\n" +"POT-Creation-Date: 2026-07-20 03:55-0500\n" "PO-Revision-Date: 2026-04-04 22:34+0100\n" "Last-Translator: \n" "Language-Team: Portuguese \n" @@ -83,8 +83,8 @@ msgstr "Introduza este código na janela do navegador para concluir a autentica msgid "Your DAIV Sign-In Code" msgstr "O Seu Código de Autenticação DAIV" -msgid "Today" -msgstr "Hoje" +msgid "Throughput" +msgstr "Entregas" msgid "Needs-me queue" msgstr "Fila de pendências" @@ -157,6 +157,74 @@ msgstr "próxima verificação %(sweep)s" msgid "No scheduled runs yet." msgstr "Ainda não há execuções agendadas." +msgid "today" +msgstr "hoje" + +msgid "vs yesterday" +msgstr "vs ontem" + +msgid "this week" +msgstr "esta semana" + +msgid "vs last week" +msgstr "vs semana passada" + +msgid "in the last 30 days" +msgstr "nos últimos 30 dias" + +msgid "vs last 30 days" +msgstr "vs últimos 30 dias" + +msgid "in the last 90 days" +msgstr "nos últimos 90 dias" + +msgid "vs last 90 days" +msgstr "vs últimos 90 dias" + +msgid "all time" +msgstr "desde sempre" + +#, python-format +msgid "%(n)s change shipped %(range_label)s" +msgid_plural "%(n)s changes shipped %(range_label)s" +msgstr[0] "%(n)s alteração entregue %(range_label)s" +msgstr[1] "%(n)s alterações entregues %(range_label)s" + +msgid "shipped since day one" +msgstr "entregues desde o início" + +#, python-format +msgid "~%(n)s dev-hours saved" +msgstr "~%(n)s horas de dev poupadas" + +msgid "est." +msgstr "est." + +#, python-format +msgid "DAIV shipped %(n)s changes %(range)s (%(total)s all-time)" +msgstr "A DAIV entregou %(n)s alterações %(range)s (%(total)s no total)" + +msgid "Copy" +msgstr "Copiar" + +msgid "Copied to clipboard" +msgstr "Copiado para a área de transferência" + +msgid "Copy failed" +msgstr "Falha ao copiar" + +msgid "Copy Slack summary" +msgstr "Copiar resumo para o Slack" + +msgid "Copied" +msgstr "Copiado" + +msgid "No changes shipped yet" +msgstr "Ainda sem alterações entregues" + +msgid "Your throughput appears here once DAIV merges its first change." +msgstr "As tuas entregas aparecem aqui assim que o DAIV fizer o primeiro merge." + msgid "Org impact" msgstr "Impacto da organização" @@ -282,6 +350,9 @@ msgstr "Nenhum utilizador encontrado." msgid "Time range" msgstr "Intervalo de tempo" +msgid "Today" +msgstr "Hoje" + msgid "7 days" msgstr "7 dias" diff --git a/daiv/accounts/templates/accounts/_console_body.html b/daiv/accounts/templates/accounts/_console_body.html index 86ed44d7..0952b3e1 100644 --- a/daiv/accounts/templates/accounts/_console_body.html +++ b/daiv/accounts/templates/accounts/_console_body.html @@ -8,20 +8,19 @@ shimmer skeletons are aria-hidden so assistive tech is not read empty rows. {% endcomment %}
- {# ── Hero ─────────────────────────────────────────────────────────────── #} -

- {% translate "Today" %} + {% translate "Throughput" %}

- + {% include "accounts/_hero.html" %}
{# ── Needs-me queue ───────────────────────────────────────────────────── #} diff --git a/daiv/accounts/templates/accounts/_hero.html b/daiv/accounts/templates/accounts/_hero.html new file mode 100644 index 00000000..49861d98 --- /dev/null +++ b/daiv/accounts/templates/accounts/_hero.html @@ -0,0 +1,107 @@ +{% load i18n icon_tags %} +{% comment %} +Personal Throughput Hero body (Story 3.1) — the live, personal, server-rendered replacement for +the 2.1 ``console-hero`` skeleton. Rendered INSIDE the ``console-hero`` section card in +``_console_body.html`` (flat surface-2, hairline border), so this partial carries no card chrome +of its own. Four honest parts on the 2.1 token layer: + + 1. This-range headline fact — "N changes shipped this week" in display mono/tabular; the NUMBER is + the single text-text-strong at display size, so a fact reads solid/white and can't be mistaken + for the demoted estimate. + 2. ``vs last`` delta chip — this-range minus the preceding equal-length window; green/red/muted; + hidden for the "all time" range (no prior window). Wrapped with the headline in a polite + aria-live region so a range change announces the new value. + 3. All-time odometer — range-invariant, mono, entering via the CSS-only ``.animate-number`` + (auto-neutralised under prefers-reduced-motion by the global kill-switch in input.css — no JS). + 4. Copyable Slack one-liner — mono, in a ``ground`` inset inside its OWN overflow-x-auto container + so a long line never scrolls the body; copied via the existing Alpine clipboard idiom (no new JS). + +The demoted estimate slot is OMITTED in v1 (D1: no defensible dev-hours field), rendered only +``{% if hero.estimate %}`` — the markup + demotion styling are retained so a later story enables it +by supplying a value. When ``hero`` is falsy (nothing shipped all-time) we render an honest empty +line, never a ``0`` dressed as a headline fact (AC13). +{% endcomment %} +{% if hero %} +{# Range-adaptive labels — externalised here (not in the view) so the i18n source lives with the markup. #} +{% if hero.period == "today" %}{% translate "today" as range_label %}{% translate "vs yesterday" as delta_label %} +{% elif hero.period == "7d" %}{% translate "this week" as range_label %}{% translate "vs last week" as delta_label %} +{% elif hero.period == "30d" %}{% translate "in the last 30 days" as range_label %}{% translate "vs last 30 days" as delta_label %} +{% elif hero.period == "90d" %}{% translate "in the last 90 days" as range_label %}{% translate "vs last 90 days" as delta_label %} +{% else %}{% translate "all time" as range_label %}{% endif %} + +
+ {# ── This-range headline + delta (announced politely on a range change) ── #} +
+

+ {% blocktranslate count n=hero.this_range.total_merges %}{{ n }} change shipped {{ range_label }}{% plural %}{{ n }} changes shipped {{ range_label }}{% endblocktranslate %} +

+ {% if hero.delta is not None %} +

+ {% if hero.delta > 0 %}+{{ hero.delta }}{% elif hero.delta < 0 %}{{ hero.delta }}{% else %}0{% endif %} {{ delta_label }} +

+ {% endif %} +
+ + {# ── All-time odometer (range-invariant) ─────────────────────────────── #} +

+ {{ hero.all_time }} + {% translate "shipped since day one" %} +

+ + {# ── Demoted estimate (D1: OMITTED in v1 — falsy ``estimate`` renders nothing) ── #} + {% if hero.estimate %} +

+ {% blocktranslate with n=hero.estimate %}~{{ n }} dev-hours saved{% endblocktranslate %} + {% translate "est." %} +

+ {% endif %} + + {# ── Copyable Slack one-liner (own overflow-x-auto container; Alpine clipboard, no new JS) ── #} + {% blocktranslate asvar slack_line with n=hero.this_range.total_merges range=range_label total=hero.all_time %}DAIV shipped {{ n }} changes {{ range }} ({{ total }} all-time){% endblocktranslate %} + {{ slack_line|json_script:"hero-slack-line" }} +
+
+ {{ slack_line }} +
+ +
+
+{% else %} +{# Honest empty state (AC13): nothing shipped all-time — a quiet onboarding line, never a fake 0. #} +
+

{% translate "No changes shipped yet" %}

+

{% translate "Your throughput appears here once DAIV merges its first change." %}

+
+{% endif %} diff --git a/daiv/accounts/templates/accounts/dashboard.html b/daiv/accounts/templates/accounts/dashboard.html index aff3bf37..0aef6b6a 100644 --- a/daiv/accounts/templates/accounts/dashboard.html +++ b/daiv/accounts/templates/accounts/dashboard.html @@ -14,32 +14,56 @@ {# Admin-gated console surface toggle, active on "Personal" (the default); absent for non-admins (AC2). #} {% include "accounts/_manager_lens_toggle.html" with surface="personal" %} - {% comment %} - Range-switcher — console-only, static placeholder in 2.1; wired in Epic 3. - Active pill is teal; labels use the mono face + tabular figures. - {% endcomment %} - {% endblock topbar_start %} diff --git a/daiv/accounts/views.py b/daiv/accounts/views.py index a4797814..28221daf 100644 --- a/daiv/accounts/views.py +++ b/daiv/accounts/views.py @@ -5,7 +5,7 @@ from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.db import IntegrityError -from django.db.models import Avg, Count, DurationField, ExpressionWrapper, F, Q, Sum +from django.db.models import Avg, Count, DurationField, Exists, ExpressionWrapper, F, OuterRef, Q, Sum from django.http import Http404 from django.shortcuts import redirect, render from django.urls import reverse, reverse_lazy @@ -83,7 +83,10 @@ def get_context_data(self, **kwargs): ("all", "All time", None), ] PERIOD_DAYS = {key: days for key, _, days in PERIOD_CHOICES} -DEFAULT_PERIOD = "today" +# The personal console lands on the week (Story 3.1 / D4): a weekly throughput number is +# screenshot-worthy where a daily one is not, and it satisfies the FR-16 "this week" headline. +# ``ManagerLensView`` is unaffected — it passes ``default="all"`` explicitly. +DEFAULT_PERIOD = "7d" def _raw_pct(numerator: int, denominator: int) -> int | None: @@ -117,7 +120,7 @@ def _resolve_period(request, default: str = DEFAULT_PERIOD) -> tuple[str, date | """Resolve the stateless ``?period=`` querystring to a ``(period_key, cutoff_date)`` pair. Falls back to ``default`` for a missing/invalid value — the personal console uses the module - ``DEFAULT_PERIOD`` ("today"), while the org ``ManagerLensView`` passes ``default="all"`` because + ``DEFAULT_PERIOD`` ("7d"), while the org ``ManagerLensView`` passes ``default="all"`` because org velocity is cumulative. ``cutoff_date`` is ``None`` for the "all time" period, today's date for the zero-day "today" period, and ``today - days`` otherwise. Shared by both console surfaces so they honour the same range semantics with no persisted state. @@ -132,15 +135,35 @@ def _resolve_period(request, default: str = DEFAULT_PERIOD) -> tuple[str, date | return period, cutoff_date -def get_velocity_data(cutoff_date: date | None) -> dict | None: - """Aggregate org-wide code-velocity + DAIV-attribution from ``MergeMetric``. +def _shipped_metrics_for(user: User): + """Return the ``MergeMetric`` queryset of merges opened by DAIV **for this user** (AR9 / AD-10). - Org-wide by design — ``MergeMetric.objects.all()`` (never user-scoped, never derived from - ``RunEnvelope`` per AD-10); an optional ``cutoff_date`` restricts to merges on/after that date. - Returns ``None`` when there are zero matching rows so the caller can render an honest cold-load - state instead of a misleading zero reading. Single source of truth for the Manager Lens. + A merge counts as "shipped by DAIV, by this user" iff a ``Run`` in one of the user's own + sessions shares the merge's ``(repo_id, merge_request_iid)`` — the value-based ``Exists`` join + (there is no FK) *is* the attribution predicate, so human-authored MRs (no matching DAIV ``Run``) + are excluded. Uses the literal ``session__user=user`` — never ``Run.objects.by_owner`` / + ``visible_to``, which short-circuit to ``.all()`` for admins and would break personal scope (AC10). """ - merges = MergeMetric.objects.all() + daiv_user_runs = Run.objects.filter( + session__user=user, + merge_request_iid__isnull=False, + repo_id=OuterRef("repo_id"), + merge_request_iid=OuterRef("merge_request_iid"), + ) + return MergeMetric.objects.filter(Exists(daiv_user_runs)) + + +def get_velocity_data(cutoff_date: date | None, queryset=None) -> dict | None: + """Aggregate code-velocity + DAIV-attribution over a ``MergeMetric`` queryset. + + The single shared counting body (AC11): the Manager Lens passes nothing (defaulting to the + org-wide ``MergeMetric.objects.all()``, never derived from ``RunEnvelope`` per AD-10), while the + personal Hero passes the user-scoped ``_shipped_metrics_for(user)`` queryset — so the org number + and the personal number can never disagree in method. An optional ``cutoff_date`` restricts to + merges on/after that date. Returns ``None`` when there are zero matching rows so the caller can + render an honest cold-load state instead of a misleading zero reading. + """ + merges = MergeMetric.objects.all() if queryset is None else queryset if cutoff_date is not None: merges = merges.filter(merged_at__date__gte=cutoff_date) @@ -320,6 +343,7 @@ def get_context_data(self, **kwargs): # (``ManagerLensView``); every read here is personal-scoped via ``visible_to`` / ``user=``. user = self.request.user context["activity"] = self._get_activity_data(cutoff_date, user) + context["hero"] = self._get_hero_data(user, cutoff_date, period) context["active_api_keys"] = APIKey.objects.filter(user=user, revoked=False).count() context["periods"] = [{"key": key, "label": label} for key, label, _ in PERIOD_CHOICES] context["current_period"] = period @@ -328,6 +352,47 @@ def get_context_data(self, **kwargs): return context + def _get_hero_data(self, user: User, cutoff_date: date | None, period: str) -> dict | None: + """Compose the personal Throughput Hero from the user-scoped ``MergeMetric ⋈ Run`` join. + + Read-only (``.count()`` / ``.aggregate()`` only — AC12): nothing is written on the render + path. Returns ``None`` when the user has shipped nothing all-time so the template renders the + honest empty state rather than a ``0`` dressed as a headline fact (AC13); a genuine "0 this + range" with a non-zero all-time is a true fact and *is* rendered. + + - ``this_range`` — the range-scoped count through the SHARED ``get_velocity_data`` body + (never a forked count, AC11); ``0`` when the window is empty but all-time is not. + - ``delta`` — ``this_range − prev_equal_window`` where the previous window is + ``[cutoff − window, cutoff)``; ``None`` for the "all time" range (no prior window) so the + template hides the chip. + - ``all_time`` — the range-invariant odometer count. + - ``estimate`` — ``None`` in v1 (D1: no defensible dev-hours field exists); the key is kept + so the template's ``{% if hero.estimate %}`` demotion path stays exercisable. + """ + shipped = _shipped_metrics_for(user) + all_time = shipped.count() + if not all_time: + return None + + this_range = get_velocity_data(cutoff_date, queryset=shipped) + this_count = this_range["total_merges"] if this_range else 0 + + delta = None + if period != "all" and cutoff_date is not None: + # "today" has a zero-day window; compare it against the single preceding day. + window_days = PERIOD_DAYS[period] or 1 + prev_start = cutoff_date - timedelta(days=window_days) + prev_count = shipped.filter(merged_at__date__gte=prev_start, merged_at__date__lt=cutoff_date).count() + delta = this_count - prev_count + + return { + "this_range": {"total_merges": this_count}, + "period": period, + "delta": delta, + "all_time": all_time, + "estimate": None, + } + def _get_feed_data(self, user: User) -> dict: """Build the console Feed: RUN_FEED rows newest-first, each rendered from its live envelope. diff --git a/tests/unit_tests/accounts/test_dashboard_console.py b/tests/unit_tests/accounts/test_dashboard_console.py index 6bf795f4..5d38d1be 100644 --- a/tests/unit_tests/accounts/test_dashboard_console.py +++ b/tests/unit_tests/accounts/test_dashboard_console.py @@ -6,17 +6,20 @@ job-creation launcher in the console content. """ +import uuid from datetime import timedelta from pathlib import Path -from django.template.loader import get_template +from django.template.loader import get_template, render_to_string from django.test import Client from django.urls import reverse from django.utils import timezone import pytest +from sessions.models import Run, RunStatus, Session, SessionOrigin from accounts.models import Role +from accounts.views import get_velocity_data from codebase.models import MergeMetric, PlatformType # Repo root: tests/unit_tests/accounts/ -> parents[3]. @@ -395,3 +398,270 @@ def test_lens_strings_are_translated(self): assert '{% translate "MRs involving DAIV" %}' in src # The cold-load aria-busy label (single-quoted inside the double-quoted attribute). assert "{% translate 'Computing org impact…' %}" in src + + +# --------------------------------------------------------------------------- +# Story 3.1 — The personal Throughput Hero + wired range switcher +# --------------------------------------------------------------------------- + +# ``merged_at`` is *now* in ``_make_merge_metric``; distinct ``iid`` keeps the +# (repo_id, merge_request_iid, platform) uniqueness happy. The Hero counts a merge only when a +# ``Run`` in one of the USER's own sessions shares its (repo_id, merge_request_iid) — the value-based +# ``MergeMetric ⋈ Run`` attribution join. This helper builds that owning Session+Run inline (no +# factory exists), mirroring how ``_make_merge_metric`` builds the MergeMetric directly. + + +def _make_shipping_run(user, *, iid, repo_id="daiv/test"): + """Create a Session owned by ``user`` + a Run sharing ``(repo_id, iid)`` — the DAIV-opened, + by-this-user attribution the Hero join asserts. Pairs with a ``_make_merge_metric(iid=iid)``.""" + session = Session.objects.create( + thread_id=str(uuid.uuid4()), origin=SessionOrigin.MR_WEBHOOK, repo_id=repo_id, user=user + ) + return Run.objects.create( + session=session, + trigger_type=SessionOrigin.MR_WEBHOOK, + status=RunStatus.SUCCESSFUL, + repo_id=repo_id, + merge_request_iid=iid, + ) + + +@pytest.mark.django_db +class TestHeroCountAndAttribution: + """AC2/AC3: 'changes shipped' = merged MRs opened by DAIV for THIS user (MergeMetric ⋈ Run).""" + + def test_counts_only_merges_with_a_matching_user_run(self, member_client, member_user): + # iid=1 → member's own run (counted); iid=2 → human MR, no run (excluded). + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1) + _make_merge_metric(iid=2) # no run → human-authored → excluded + + response = member_client.get(reverse("dashboard")) + assert response.status_code == 200 + hero = response.context["hero"] + assert hero is not None + assert hero["all_time"] == 1 + assert hero["this_range"]["total_merges"] == 1 + assert b'data-testid="hero-headline"' in response.content + + def test_another_users_merge_is_excluded(self, member_client, member_user, admin_user): + # A merge matched only by ANOTHER user's run must not count for member. + _make_merge_metric(iid=3) + _make_shipping_run(admin_user, iid=3) + + response = member_client.get(reverse("dashboard")) + # member has zero matched merges → honest empty state, not a 0-headline. + assert response.context["hero"] is None + assert b'data-testid="hero-empty"' in response.content + + +@pytest.mark.django_db +class TestHeroAdminPersonalScope: + """AC10: the admin Hero counts only the admin's OWN merges — never the by_owner/.all() short-circuit.""" + + def test_admin_hero_is_personal_not_org_wide(self, admin_client, admin_user, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(admin_user, iid=1) # admin's own → counted + _make_merge_metric(iid=2) + _make_shipping_run(member_user, iid=2) # another user's → excluded for admin + _make_merge_metric(iid=3) # human MR, no run → excluded + + response = admin_client.get(reverse("dashboard")) + hero = response.context["hero"] + assert hero is not None + # Personal scope: only the admin's own 1 merge, NOT the org-wide 3 (or 2). + assert hero["all_time"] == 1 + + def test_personal_by_default_still_holds_with_hero(self, admin_client, admin_user): + # AC10 belt-and-braces: even with a live personal Hero, the default carries no org content. + _make_merge_metric(iid=1) + _make_shipping_run(admin_user, iid=1) + response = admin_client.get(reverse("dashboard")) + assert "velocity" not in response.context + assert "total_users" not in response.context + assert b'data-testid="manager-lens"' not in response.content + + +@pytest.mark.django_db +class TestHeroRangeScopingAndDelta: + """AC1/AC6 + D3/D4: range scopes the headline, delta vs the prior equal window, hidden for 'all'.""" + + def _seed_windows(self, user): + # this-week ×2 (now), prev-window ×1 (10d ago), older ×1 (40d ago); all owned by ``user``. + for iid in (1, 2, 3, 4): + _make_merge_metric(iid=iid) + _make_shipping_run(user, iid=iid) + MergeMetric.objects.filter(merge_request_iid=3).update(merged_at=timezone.now() - timedelta(days=10)) + MergeMetric.objects.filter(merge_request_iid=4).update(merged_at=timezone.now() - timedelta(days=40)) + + def test_default_view_resolves_to_7d_this_week_with_delta(self, member_client, member_user): + self._seed_windows(member_user) + response = member_client.get(reverse("dashboard")) # no ?period= → D4 default + assert response.context["current_period"] == "7d" + hero = response.context["hero"] + assert hero["this_range"]["total_merges"] == 2 # the two this-week merges + assert hero["delta"] == 1 # 2 this week − 1 in the preceding 7-day window + assert hero["all_time"] == 4 + assert b"this week" in response.content + assert b'data-testid="hero-delta"' in response.content + + def test_30d_range_label_and_scope(self, member_client, member_user): + self._seed_windows(member_user) + response = member_client.get(reverse("dashboard"), {"period": "30d"}) + hero = response.context["hero"] + assert hero["this_range"]["total_merges"] == 3 # excludes the 40-day-old merge + assert b"in the last 30 days" in response.content + + def test_all_time_range_has_no_delta_chip(self, member_client, member_user): + self._seed_windows(member_user) + response = member_client.get(reverse("dashboard"), {"period": "all"}) + hero = response.context["hero"] + assert hero["this_range"]["total_merges"] == 4 # every merge + assert hero["delta"] is None + assert b'data-testid="hero-delta"' not in response.content + + def test_htmx_fragment_rescopes_hero(self, member_client, member_user): + self._seed_windows(member_user) + response = member_client.get(reverse("dashboard"), {"period": "all"}, HTTP_HX_REQUEST="true") + assert response.status_code == 200 + assert b'data-testid="hero-headline"' in response.content + assert b'data-testid="app-sidebar"' not in response.content # fragment, not the shell + assert response.context["hero"]["this_range"]["total_merges"] == 4 + + +@pytest.mark.django_db +class TestHeroOdometerInvariance: + """AC1: the all-time odometer is invariant to the selected range.""" + + def test_all_time_identical_across_ranges(self, member_client, member_user): + for iid in (1, 2, 3): + _make_merge_metric(iid=iid) + _make_shipping_run(member_user, iid=iid) + MergeMetric.objects.filter(merge_request_iid=3).update(merged_at=timezone.now() - timedelta(days=40)) + + seven = member_client.get(reverse("dashboard"), {"period": "7d"}).context["hero"]["all_time"] + thirty = member_client.get(reverse("dashboard"), {"period": "30d"}).context["hero"]["all_time"] + assert seven == thirty == 3 + + +@pytest.mark.django_db +class TestHeroDefaultPeriod: + """D4: the personal console default period is now '7d' (was 'today').""" + + def test_default_period_is_7d(self, member_client): + response = member_client.get(reverse("dashboard")) + assert response.context["current_period"] == "7d" + + +class TestHeroEstimateDemotion: + """AC4/D1: no estimate line in v1; a truthy estimate renders demoted (never fact-styled).""" + + def _render(self, *, estimate): + hero = {"this_range": {"total_merges": 5}, "period": "7d", "delta": 1, "all_time": 100, "estimate": estimate} + return render_to_string("accounts/_hero.html", {"hero": hero}) + + def test_v1_estimate_none_renders_no_estimate_line(self): + html = self._render(estimate=None) + assert 'data-testid="hero-estimate"' not in html + + def test_truthy_estimate_is_visually_demoted(self): + html = self._render(estimate=48) + assert 'data-testid="hero-estimate"' in html + start = html.index('data-testid="hero-estimate"') + block = html[start : html.index("

", start)] + assert "~48 dev-hours saved" in html + # Demotion: italic + faint + dotted underline + an ``est.`` tag ... + assert "italic" in block + assert "text-text-faint" in block + assert "decoration-dotted" in block + assert "est." in block + # ... and NEVER the fact styling (solid white display mono). + assert "text-text-strong" not in block + + +@pytest.mark.django_db +class TestHeroSlackLine: + """AC5: the copyable Slack one-liner sits in its own overflow-x-auto inset with a Copy control.""" + + def test_slack_line_overflow_and_copy_control(self, member_client, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1) + response = member_client.get(reverse("dashboard")) + content = response.content.decode() + assert 'data-testid="hero-slack"' in content + # The inset owns its overflow container so a long line never scrolls the body. + slack_start = content.index('data-testid="hero-slack"') + assert "overflow-x-auto" in content[slack_start - 120 : slack_start + 120] + assert 'data-testid="hero-copy"' in content + # The clipboard idiom's json_script payload is present with the referenced id. + assert 'id="hero-slack-line"' in content + assert "DAIV shipped 1 changes this week (1 all-time)" in content + + +@pytest.mark.django_db +class TestHeroEmptyState: + """AC13: nothing shipped all-time → honest empty state, never a 0-headline fact.""" + + def test_empty_hero_is_honest(self, member_client): + response = member_client.get(reverse("dashboard")) + assert response.context["hero"] is None + assert b'data-testid="hero-empty"' in response.content + assert b"No changes shipped yet" in response.content + # No 0 dressed up as a headline fact. + assert b'data-testid="hero-headline"' not in response.content + + +@pytest.mark.django_db +class TestGetVelocityDataQuerysetRefactor: + """AC11: one shared counting body — the queryset arg scopes it; the default stays org-wide.""" + + def test_default_is_org_wide_and_queryset_scopes(self, member_user): + _make_merge_metric(iid=1) + _make_merge_metric(iid=2) + # Default (no queryset) aggregates every MergeMetric — protects the org-wide ManagerLensView. + assert get_velocity_data(None)["total_merges"] == 2 + # A passed subset aggregates only that subset — the personal Hero's user-scoped path. + subset = MergeMetric.objects.filter(merge_request_iid=1) + assert get_velocity_data(None, queryset=subset)["total_merges"] == 1 + + def test_empty_queryset_returns_none(self): + assert get_velocity_data(None, queryset=MergeMetric.objects.none()) is None + + +@pytest.mark.django_db +class TestHeroReadOnly: + """AC12: rendering the Hero performs read-only queries — no row is created or mutated.""" + + def test_render_writes_nothing(self, member_client, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1) + before = (MergeMetric.objects.count(), Run.objects.count(), Session.objects.count()) + member_client.get(reverse("dashboard")) + member_client.get(reverse("dashboard"), {"period": "all"}) + after = (MergeMetric.objects.count(), Run.objects.count(), Session.objects.count()) + assert before == after + + +class TestHeroI18n: + """AC9: every new Hero string is {% translate %}/{% blocktranslate %}-wrapped (no bare literal).""" + + def test_hero_strings_are_translated(self): + src = Path(get_template("accounts/_hero.html").origin.name).read_text(encoding="utf-8") + assert "{% blocktranslate count n=hero.this_range.total_merges %}" in src + assert "changes shipped" in src + assert '{% translate "shipped since day one" %}' in src + assert '{% translate "est." %}' in src + assert "DAIV shipped" in src # the locked Slack line (D2), blocktranslate asvar + assert "{% translate 'Copy' %}" in src + assert '{% translate "No changes shipped yet" %}' in src + # A range-adaptive label is wrapped too (the ``as`` form). + assert '{% translate "this week" as range_label %}' in src + + def test_hero_eyebrow_is_range_agnostic_and_mounts_the_partial(self): + # The reconciled eyebrow can no longer hard-code "Today" and contradict the active range, + # the live partial is mounted, and the hero section no longer carries the placeholder + # aria-busy (its content is live now). + src = Path(get_template("accounts/_console_body.html").origin.name).read_text(encoding="utf-8") + assert '{% translate "Throughput" %}' in src + assert '{% include "accounts/_hero.html" %}' in src + assert 'data-testid="console-hero" aria-labelledby="console-hero-heading" aria-busy="true"' not in src From 7298e7736991059a60674d9c4818507baf55a0c1 Mon Sep 17 00:00:00 2001 From: Sandro Date: Mon, 20 Jul 2026 10:36:23 +0100 Subject: [PATCH 2/6] fix(accounts): equal-length delta window in the throughput Hero (Story 3.1) Review found the vs-last delta compared an inclusive [cutoff, today] current window (window_days + 1 days) against a window_days-long previous window, biasing the delta positive on a flat cadence (one merge/day showed a spurious +1 on the default 7d view). Make the previous window span the same number of days; add a regression test asserting delta == 0 for a uniform one-merge-per-day cadence. --- daiv/accounts/views.py | 15 +++++++++------ .../unit_tests/accounts/test_dashboard_console.py | 13 +++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/daiv/accounts/views.py b/daiv/accounts/views.py index 28221daf..1dec6623 100644 --- a/daiv/accounts/views.py +++ b/daiv/accounts/views.py @@ -362,9 +362,9 @@ def _get_hero_data(self, user: User, cutoff_date: date | None, period: str) -> d - ``this_range`` — the range-scoped count through the SHARED ``get_velocity_data`` body (never a forked count, AC11); ``0`` when the window is empty but all-time is not. - - ``delta`` — ``this_range − prev_equal_window`` where the previous window is - ``[cutoff − window, cutoff)``; ``None`` for the "all time" range (no prior window) so the - template hides the chip. + - ``delta`` — ``this_range − prev_equal_window`` where the previous window spans the same + number of days as the current [cutoff, today] window (``PERIOD_DAYS[period] + 1``); + ``None`` for the "all time" range (no prior window) so the template hides the chip. - ``all_time`` — the range-invariant odometer count. - ``estimate`` — ``None`` in v1 (D1: no defensible dev-hours field exists); the key is kept so the template's ``{% if hero.estimate %}`` demotion path stays exercisable. @@ -379,9 +379,12 @@ def _get_hero_data(self, user: User, cutoff_date: date | None, period: str) -> d delta = None if period != "all" and cutoff_date is not None: - # "today" has a zero-day window; compare it against the single preceding day. - window_days = PERIOD_DAYS[period] or 1 - prev_start = cutoff_date - timedelta(days=window_days) + # The current window ``merged_at__date >= cutoff_date`` is [cutoff, today] *inclusive*, + # spanning ``PERIOD_DAYS[period] + 1`` days ("7d" cutoff = today−7 ⇒ 8 days; "today" ⇒ 1). + # The preceding window must span the SAME number of days, else a flat one-merge-per-day + # cadence reports a spurious +1 (an 8-day current window vs a 7-day previous one). + window_span = (PERIOD_DAYS[period] or 0) + 1 + prev_start = cutoff_date - timedelta(days=window_span) prev_count = shipped.filter(merged_at__date__gte=prev_start, merged_at__date__lt=cutoff_date).count() delta = this_count - prev_count diff --git a/tests/unit_tests/accounts/test_dashboard_console.py b/tests/unit_tests/accounts/test_dashboard_console.py index 5d38d1be..c1a5a202 100644 --- a/tests/unit_tests/accounts/test_dashboard_console.py +++ b/tests/unit_tests/accounts/test_dashboard_console.py @@ -505,6 +505,19 @@ def test_default_view_resolves_to_7d_this_week_with_delta(self, member_client, m assert b"this week" in response.content assert b'data-testid="hero-delta"' in response.content + def test_delta_uses_equal_length_windows(self, member_client, member_user): + # AC1/D3 regression: the current "7d" window is [today−7, today] = 8 inclusive days, so the + # preceding window must also span 8 days. With a flat one-merge-per-day cadence a 7-day + # previous window would report a spurious +1; an equal 8-day window reports 0. + for offset in range(16): # today, today−1, … today−15 (one merge per day) + iid = offset + 1 + _make_merge_metric(iid=iid) + _make_shipping_run(member_user, iid=iid) + MergeMetric.objects.filter(merge_request_iid=iid).update(merged_at=timezone.now() - timedelta(days=offset)) + hero = member_client.get(reverse("dashboard"), {"period": "7d"}).context["hero"] + assert hero["this_range"]["total_merges"] == 8 # offsets 0..7 + assert hero["delta"] == 0 # preceding equal 8-day window (offsets 8..15) also holds 8 + def test_30d_range_label_and_scope(self, member_client, member_user): self._seed_windows(member_user) response = member_client.get(reverse("dashboard"), {"period": "30d"}) From 6dc5e9e744d96eade971f68b242c10b8ff433f50 Mon Sep 17 00:00:00 2001 From: Sandro Date: Mon, 20 Jul 2026 11:52:24 +0100 Subject: [PATCH 3/6] feat(accounts): Hero click-through breakdown + how-computed disclosure (Story 3.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the Throughput Hero's this-range headline and all-time odometer each click through to a breakdown of the exact merged DAIV MRs the count summed, plus a plain-prose "how computed" (AD-10) disclosure — so the number is auditable in place (FR-19, NFR1). Breakdown rows are annotated onto the SAME _shipped_metrics_for(user) queryset the count aggregates, with the identical range window, so the revealed list can never diverge from the number. Delivered as a generic, hero-agnostic _clickthrough.html partial that Epic 4's Needs-me Queue count will reuse. Presentation-only: no live reads, no new model/migration/URL/cache/JS. Also close the AC12 gap where an expired LangGraph checkpoint dead-ended the Feed drill-through: session_detail's expired block now links each successful run's durable prose report (Run.response_text) via the existing session_run_download_md route. Strings externalised (en + pt); inline Alpine disclosure with aria-expanded/ aria-controls, Esc + click-outside, teal focus ring and >=44px targets. --- daiv/accounts/locale/pt/LC_MESSAGES/django.po | 24 +- .../templates/accounts/_clickthrough.html | 64 ++++ daiv/accounts/templates/accounts/_hero.html | 70 ++++- daiv/accounts/views.py | 42 ++- .../templates/sessions/session_detail.html | 15 +- .../accounts/test_dashboard_console.py | 282 +++++++++++++++++- .../unit_tests/sessions/test_views_detail.py | 82 +++++ 7 files changed, 557 insertions(+), 22 deletions(-) create mode 100644 daiv/accounts/templates/accounts/_clickthrough.html diff --git a/daiv/accounts/locale/pt/LC_MESSAGES/django.po b/daiv/accounts/locale/pt/LC_MESSAGES/django.po index 09238de4..bda9b8c1 100644 --- a/daiv/accounts/locale/pt/LC_MESSAGES/django.po +++ b/daiv/accounts/locale/pt/LC_MESSAGES/django.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: PACKAGE VERSION\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2026-07-20 03:55-0500\n" +"POT-Creation-Date: 2026-07-20 05:23-0500\n" "PO-Revision-Date: 2026-04-04 22:34+0100\n" "Last-Translator: \n" "Language-Team: Portuguese \n" @@ -83,6 +83,22 @@ msgstr "Introduza este código na janela do navegador para concluir a autentica msgid "Your DAIV Sign-In Code" msgstr "O Seu Código de Autenticação DAIV" +msgid "What this counts" +msgstr "O que isto conta" + +msgid "Changes shipped = merged MRs opened by DAIV, counted from GitLab merges and attributed to you because each one links to one of your runs." +msgstr "Alterações entregues = MRs integrados abertos pelo DAIV, contados a partir das integrações do GitLab e atribuídos a ti porque cada um está ligado a uma das tuas execuções." + +#, python-format +msgid "open MR !%(iid)s on GitLab" +msgstr "abrir MR !%(iid)s no GitLab" + +msgid "view run" +msgstr "ver execução" + +msgid "No changes shipped in this range." +msgstr "Sem alterações entregues neste intervalo." + msgid "Throughput" msgstr "Entregas" @@ -190,6 +206,12 @@ msgid_plural "%(n)s changes shipped %(ra msgstr[0] "%(n)s alteração entregue %(range_label)s" msgstr[1] "%(n)s alterações entregues %(range_label)s" +msgid "counted — every merged change" +msgstr "contado — cada alteração integrada" + +msgid "how this is computed" +msgstr "como isto é calculado" + msgid "shipped since day one" msgstr "entregues desde o início" diff --git a/daiv/accounts/templates/accounts/_clickthrough.html b/daiv/accounts/templates/accounts/_clickthrough.html new file mode 100644 index 00000000..98fe9af1 --- /dev/null +++ b/daiv/accounts/templates/accounts/_clickthrough.html @@ -0,0 +1,64 @@ +{% load i18n icon_tags %} +{% comment %} +Generic click-through breakdown panel (Story 3.2) — reused by Epic 4's Queue count (AC9). +HERO-AGNOSTIC: reads ONLY its include params, never a ``hero.``-prefixed context var, so Epic 4 +wires Queue rows in with no rework. + + {% include "accounts/_clickthrough.html" with rows=… number=… how_computed=… panel_id=… %} + +Params: + rows — iterable of row objects. Row contract (each item exposes, as attr or dict key): + · repo_id — repo tag (raw, mono chip) + · merge_request_iid — the MR/PR number (rendered ``!{iid}``, mono) + · title — human title (Geist Sans, truncate-tolerant) + · merged_at — merged timestamp (mono, tabular) + · web_url — best-effort out-link URL; blank → NO out-link, never ``#`` (AC11) + · thread_id — originating session for the same-tab drill-through + number — the count/label this panel is behind (contract param; not force-rendered). + how_computed — truthy renders the "how computed" AD-10 formula disclosure (AC2). + panel_id — the unique id the trigger's ``aria-controls`` points at (a11y, AC10). + +Flat INLINE surface (surface-2 + hairline) — NOT a floating popover, so no overlay shadow. Links +are teal ``accent`` NAVIGATION — never a ``button-*`` action, never a status colour. Icons via +``{% templatetag openblock %} icon {% templatetag closeblock %}`` only (no inline ````). +{% endcomment %} +
+ {% if how_computed %} +
+

+ {% translate "What this counts" %} +

+

+ {% blocktranslate %}Changes shipped = merged MRs opened by DAIV, counted from GitLab merges and attributed to you because each one links to one of your runs.{% endblocktranslate %} +

+
+ {% endif %} +
+ +
+
diff --git a/daiv/accounts/templates/accounts/_hero.html b/daiv/accounts/templates/accounts/_hero.html index 49861d98..cd816efd 100644 --- a/daiv/accounts/templates/accounts/_hero.html +++ b/daiv/accounts/templates/accounts/_hero.html @@ -30,25 +30,63 @@ {% else %}{% translate "all time" as range_label %}{% endif %}
- {# ── This-range headline + delta (announced politely on a range change) ── #} -
-

- {% blocktranslate count n=hero.this_range.total_merges %}{{ n }} change shipped {{ range_label }}{% plural %}{{ n }} changes shipped {{ range_label }}{% endblocktranslate %} -

- {% if hero.delta is not None %} -

- {% if hero.delta > 0 %}+{{ hero.delta }}{% elif hero.delta < 0 %}{{ hero.delta }}{% else %}0{% endif %} {{ delta_label }} + {# ── This-range headline (click-through trigger) + delta + how-computed disclosure ── #} + {# The headline number, the muted "how this is computed" link, and the inline breakdown panel share + ONE Alpine scope so the trigger's aria-controls and the panel stay in sync (mirrors + _user_menu.html; this repo uses ``@click.outside``, not ``@click.away``). Esc closes; the global + teal ``:focus-visible`` ring is inherited (not re-implemented). The existing polite + ``role=status`` region is PRESERVED so a range change still announces the new value — the + deferred aria-live-across-swap gap (3.1) is left untouched, not worsened. #} +

+
+ {# A + {% if hero.delta is not None %} + {# Delta chip stays INERT — a plain

, never a click target (never wrapped in the trigger). #} +

+ {% if hero.delta > 0 %}+{{ hero.delta }}{% elif hero.delta < 0 %}{{ hero.delta }}{% else %}0{% endif %} {{ delta_label }} +

+ {% endif %} +
+ + {# Fact caption + the mockup's muted "how this is computed" entry point — toggles the SAME panel. #} +

+ {% translate "counted — every merged change" %} +

- {% endif %} + + {# Inline breakdown panel — flat surface below the fact (no overlay shadow), reflows on <768px, + re-renders with #console-main on a range change. One panel serves AC1 (rows) + AC2 (how). #} +
+ {% include "accounts/_clickthrough.html" with rows=hero.this_range_rows number=hero.this_range.total_merges how_computed=True panel_id="hero-breakdown-range" %} +
- {# ── All-time odometer (range-invariant) ─────────────────────────────── #} -

- {{ hero.all_time }} - {% translate "shipped since day one" %} -

+ {# ── All-time odometer (range-invariant) — click-through trigger ─────────── #} + {# Its OWN Alpine scope + panel (range-invariant all-time rows). The odometer element is itself the + button (keeps the ``hero-odometer`` testid + the ``.animate-number`` span). ``how_computed`` is + omitted here — the headline panel already carries the AD-10 disclosure. #} +
+ +
+ {% include "accounts/_clickthrough.html" with rows=hero.all_time_rows number=hero.all_time how_computed=False panel_id="hero-breakdown-alltime" %} +
+
{# ── Demoted estimate (D1: OMITTED in v1 — falsy ``estimate`` renders nothing) ── #} {% if hero.estimate %} diff --git a/daiv/accounts/views.py b/daiv/accounts/views.py index 1dec6623..09b34352 100644 --- a/daiv/accounts/views.py +++ b/daiv/accounts/views.py @@ -5,7 +5,7 @@ from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.db import IntegrityError -from django.db.models import Avg, Count, DurationField, Exists, ExpressionWrapper, F, OuterRef, Q, Sum +from django.db.models import Avg, Count, DurationField, Exists, ExpressionWrapper, F, OuterRef, Q, Subquery, Sum from django.http import Http404 from django.shortcuts import redirect, render from django.urls import reverse, reverse_lazy @@ -153,6 +153,38 @@ def _shipped_metrics_for(user: User): return MergeMetric.objects.filter(Exists(daiv_user_runs)) +def _hero_breakdown_rows(shipped, user: User, cutoff_date: date | None) -> tuple[list, list]: + """Annotate the exact ``MergeMetric`` rows behind the Hero counts (AC1/AC5, read-only). + + Derived from the SAME ``shipped`` base ``get_velocity_data`` aggregates, so the revealed list + can never diverge from the number (NFR1): a ``.count()`` and this ``.annotate()`` over one base + queryset return the identical set. Each row carries its native ``MergeMetric`` fields + (``repo_id`` / ``merge_request_iid`` / ``title`` / ``merged_at``) plus two annotation-only names + joined from the most-recent attributing ``Run`` in one of the USER's own sessions: + + - ``web_url`` — the already-persisted ``Run.merge_request_web_url`` (no live client, AC6/AC11); + may be ``""`` (blank, best-effort) → the template degrades to the session link. + - ``thread_id`` — the representative session for the drill-through. + + The correlation is the literal ``session__user=user`` scope — never ``by_owner`` / ``visible_to`` + (which broaden to ``.all()`` for admins, AC7). The this-range list applies the IDENTICAL window + filter ``get_velocity_data`` uses (``merged_at__date__gte=cutoff_date`` when dated; unfiltered for + "all"), so it matches the headline count; the unfiltered list matches the odometer (AC8). + """ + attributing_run = Run.objects.filter( + session__user=user, repo_id=OuterRef("repo_id"), merge_request_iid=OuterRef("merge_request_iid") + ).order_by("-created_at", "-pk") # ``-pk`` is a deterministic tiebreak on a created_at collision + rows = shipped.annotate( + # Prefer an attributing run that actually PERSISTED a url: a blank best-effort url on the + # newest re-run must not hide a good url carried by an older run (AC11 degrades to the session + # link only when NO attributing run has one). ``thread_id`` (the session PK) is always present. + web_url=Subquery(attributing_run.exclude(merge_request_web_url="").values("merge_request_web_url")[:1]), + thread_id=Subquery(attributing_run.values("session__thread_id")[:1]), + ).order_by("-merged_at") + this_range_rows = rows.filter(merged_at__date__gte=cutoff_date) if cutoff_date is not None else rows + return list(this_range_rows), list(rows) + + def get_velocity_data(cutoff_date: date | None, queryset=None) -> dict | None: """Aggregate code-velocity + DAIV-attribution over a ``MergeMetric`` queryset. @@ -388,12 +420,20 @@ def _get_hero_data(self, user: User, cutoff_date: date | None, period: str) -> d prev_count = shipped.filter(merged_at__date__gte=prev_start, merged_at__date__lt=cutoff_date).count() delta = this_count - prev_count + # Story 3.2 — attach the exact underlying rows behind each number, derived from the SAME + # ``shipped`` base with the identical window filter, so the click-through breakdown always + # reconciles with the count (``len(this_range_rows) == this_count``, ``len(all_time_rows) == + # all_time`` by construction). Read-only annotate/list only — no live client (AC5/AC6/AC11). + this_range_rows, all_time_rows = _hero_breakdown_rows(shipped, user, cutoff_date) + return { "this_range": {"total_merges": this_count}, "period": period, "delta": delta, "all_time": all_time, "estimate": None, + "this_range_rows": this_range_rows, + "all_time_rows": all_time_rows, } def _get_feed_data(self, user: User) -> dict: diff --git a/daiv/sessions/templates/sessions/session_detail.html b/daiv/sessions/templates/sessions/session_detail.html index 7291517d..8ae233a6 100644 --- a/daiv/sessions/templates/sessions/session_detail.html +++ b/daiv/sessions/templates/sessions/session_detail.html @@ -84,7 +84,20 @@ {% if expired %}
{% translate "This session's state has expired. Start a new session to continue." %} - {% translate "New session" %} + {# AC12: the transcript is gone with the checkpoint, but ``Run.response_text`` is persisted — + surface a link to the durable prose report for every successful run so the drill-through + never dead-ends. ``session_run_download_md`` serves it verbatim from the DB. #} +
+ {% for run in runs %} + {% if run.status == "SUCCESSFUL" and run.response_text %} + {# Label each report with its run time so multiple runs are distinguishable (a11y). #} + {% translate "Download report" %} {{ run.created_at|date:"Y-m-d H:i" }} + {% endif %} + {% endfor %} + {% translate "New session" %} +
{% endif %} diff --git a/tests/unit_tests/accounts/test_dashboard_console.py b/tests/unit_tests/accounts/test_dashboard_console.py index c1a5a202..4342f362 100644 --- a/tests/unit_tests/accounts/test_dashboard_console.py +++ b/tests/unit_tests/accounts/test_dashboard_console.py @@ -9,6 +9,7 @@ import uuid from datetime import timedelta from pathlib import Path +from unittest.mock import patch from django.template.loader import get_template, render_to_string from django.test import Client @@ -16,10 +17,13 @@ from django.utils import timezone import pytest +from notifications.choices import EventType +from notifications.models import Notification from sessions.models import Run, RunStatus, Session, SessionOrigin from accounts.models import Role from accounts.views import get_velocity_data +from codebase.clients import RepoClient from codebase.models import MergeMetric, PlatformType # Repo root: tests/unit_tests/accounts/ -> parents[3]. @@ -27,7 +31,7 @@ INPUT_CSS = REPO_ROOT / "daiv" / "static_src" / "css" / "input.css" -def _make_merge_metric(*, iid=1, daiv_commits=1, total_commits=2, lines_added=10, lines_removed=3): +def _make_merge_metric(*, iid=1, daiv_commits=1, total_commits=2, lines_added=10, lines_removed=3, title=""): """Create a ``MergeMetric`` row directly (no factory exists). Fills the required non-default fields (``merged_at``/``target_branch``/``source_branch``/ @@ -37,6 +41,7 @@ def _make_merge_metric(*, iid=1, daiv_commits=1, total_commits=2, lines_added=10 return MergeMetric.objects.create( repo_id="daiv/test", merge_request_iid=iid, + title=title, lines_added=lines_added, lines_removed=lines_removed, total_commits=total_commits, @@ -411,9 +416,12 @@ def test_lens_strings_are_translated(self): # factory exists), mirroring how ``_make_merge_metric`` builds the MergeMetric directly. -def _make_shipping_run(user, *, iid, repo_id="daiv/test"): +def _make_shipping_run(user, *, iid, repo_id="daiv/test", web_url=""): """Create a Session owned by ``user`` + a Run sharing ``(repo_id, iid)`` — the DAIV-opened, - by-this-user attribution the Hero join asserts. Pairs with a ``_make_merge_metric(iid=iid)``.""" + by-this-user attribution the Hero join asserts. Pairs with a ``_make_merge_metric(iid=iid)``. + + ``web_url`` carries onto ``Run.merge_request_web_url`` (the best-effort MR out-link the 3.2 + breakdown surfaces through the join; blank exercises the AC11 degrade-to-session-link path).""" session = Session.objects.create( thread_id=str(uuid.uuid4()), origin=SessionOrigin.MR_WEBHOOK, repo_id=repo_id, user=user ) @@ -423,6 +431,7 @@ def _make_shipping_run(user, *, iid, repo_id="daiv/test"): status=RunStatus.SUCCESSFUL, repo_id=repo_id, merge_request_iid=iid, + merge_request_web_url=web_url, ) @@ -678,3 +687,270 @@ def test_hero_eyebrow_is_range_agnostic_and_mounts_the_partial(self): assert '{% translate "Throughput" %}' in src assert '{% include "accounts/_hero.html" %}' in src assert 'data-testid="console-hero" aria-labelledby="console-hero-heading" aria-busy="true"' not in src + + +# --------------------------------------------------------------------------- +# Story 3.2 — Click-through to source (the auditable-in-place breakdown) +# --------------------------------------------------------------------------- + + +@pytest.mark.django_db +class TestBreakdownReconcilesExactly: + """AC1/AC5 (NFR1, load-bearing): the revealed rows are exactly the rows the count summed.""" + + def test_this_range_rows_equal_headline_count_and_exclude_the_rest(self, member_client, member_user, admin_user): + # K=3 matched, in-range (now) → counted this range AND all-time. + for iid in (1, 2, 3): + _make_merge_metric(iid=iid) + _make_shipping_run(member_user, iid=iid) + # matched but out-of-range (40d ago) → excluded this range, present all-time. + _make_merge_metric(iid=4) + _make_shipping_run(member_user, iid=4) + MergeMetric.objects.filter(merge_request_iid=4).update(merged_at=timezone.now() - timedelta(days=40)) + # matched by ANOTHER user's run → excluded from both (personal scope). + _make_merge_metric(iid=5) + _make_shipping_run(admin_user, iid=5) + # human MR, no matching run → excluded from both (attribution join). + _make_merge_metric(iid=6) + + response = member_client.get(reverse("dashboard")) # 7d default + hero = response.context["hero"] + # Reconcile-exactly by construction: len(this_range_rows) == the headline count. + assert hero["this_range"]["total_merges"] == 3 + assert len(hero["this_range_rows"]) == 3 + # all-time carries the out-of-range matched row too, still excludes other-user/human. + assert hero["all_time"] == 4 + assert len(hero["all_time_rows"]) == 4 + + range_iids = {row.merge_request_iid for row in hero["this_range_rows"]} + assert range_iids == {1, 2, 3} + all_time_iids = {row.merge_request_iid for row in hero["all_time_rows"]} + assert all_time_iids == {1, 2, 3, 4} + # Excluded rows appear in neither breakdown. + assert 5 not in all_time_iids + assert 6 not in all_time_iids + # The rendered panel carries one row per underlying merge. + assert b'data-testid="hero-breakdown"' in response.content + assert b'data-testid="breakdown-row"' in response.content + + def test_rows_carry_the_row_contract_fields(self, member_client, member_user): + _make_merge_metric(iid=7, title="Fix the flaky auth test") + _make_shipping_run(member_user, iid=7, web_url="https://gitlab.example.com/daiv/test/-/merge_requests/7") + hero = member_client.get(reverse("dashboard")).context["hero"] + row = hero["this_range_rows"][0] + assert row.repo_id == "daiv/test" + assert row.merge_request_iid == 7 + assert row.title == "Fix the flaky auth test" + assert row.merged_at is not None + assert row.web_url == "https://gitlab.example.com/daiv/test/-/merge_requests/7" + assert row.thread_id # a representative session for the drill-through + + +@pytest.mark.django_db +class TestBreakdownAdminPersonalScope: + """AC7: the admin breakdown lists ONLY the admin's own rows — never the by_owner/.all() path.""" + + def test_admin_breakdown_is_personal_not_org_wide(self, admin_client, admin_user, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(admin_user, iid=1) # admin's own → listed + _make_merge_metric(iid=2) + _make_shipping_run(member_user, iid=2) # another user's → excluded for admin + _make_merge_metric(iid=3) # human MR, no run → excluded + + hero = admin_client.get(reverse("dashboard"), {"period": "all"}).context["hero"] + assert len(hero["all_time_rows"]) == 1 + assert hero["all_time_rows"][0].merge_request_iid == 1 + assert len(hero["this_range_rows"]) == 1 + assert hero["this_range_rows"][0].merge_request_iid == 1 + + +@pytest.mark.django_db +class TestBreakdownRangeAndOdometer: + """AC8: headline rows track the active range; odometer rows are range-invariant.""" + + def _seed(self, user): + # two this-week (now) + one 20 days ago; all owned by ``user``. + for iid in (1, 2, 3): + _make_merge_metric(iid=iid) + _make_shipping_run(user, iid=iid) + MergeMetric.objects.filter(merge_request_iid=3).update(merged_at=timezone.now() - timedelta(days=20)) + + def test_headline_rows_match_counted_range(self, member_client, member_user): + self._seed(member_user) + seven = member_client.get(reverse("dashboard"), {"period": "7d"}).context["hero"] + assert len(seven["this_range_rows"]) == seven["this_range"]["total_merges"] == 2 + thirty = member_client.get(reverse("dashboard"), {"period": "30d"}).context["hero"] + assert len(thirty["this_range_rows"]) == thirty["this_range"]["total_merges"] == 3 + + def test_odometer_rows_identical_across_ranges(self, member_client, member_user): + self._seed(member_user) + seven = member_client.get(reverse("dashboard"), {"period": "7d"}).context["hero"] + thirty = member_client.get(reverse("dashboard"), {"period": "30d"}).context["hero"] + assert len(seven["all_time_rows"]) == len(thirty["all_time_rows"]) == 3 + + def test_htmx_fragment_carries_the_rescoped_breakdown(self, member_client, member_user): + self._seed(member_user) + response = member_client.get(reverse("dashboard"), {"period": "7d"}, HTTP_HX_REQUEST="true") + assert response.status_code == 200 + assert b'data-testid="app-sidebar"' not in response.content # fragment, not the shell + assert b'data-testid="hero-breakdown"' in response.content + assert len(response.context["hero"]["this_range_rows"]) == 2 + + def test_empty_range_shows_honest_empty_state(self, member_client, member_user): + # A non-zero all-time but nothing in the active range must not open a blank panel: the + # breakdown renders an honest "nothing here" row rather than an empty reveal (Edge-A). + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1) + MergeMetric.objects.filter(merge_request_iid=1).update(merged_at=timezone.now() - timedelta(days=20)) + response = member_client.get(reverse("dashboard"), {"period": "7d"}) + ctx = response.context["hero"] + assert ctx["all_time"] == 1 + assert len(ctx["this_range_rows"]) == 0 + assert b"No changes shipped in this range." in response.content + + +@pytest.mark.django_db +class TestBreakdownHowComputed: + """AC2: an honest AD-10 disclosure — no envelope category error, no NON-GOAL promises.""" + + def test_how_computed_present_and_honest(self, member_client, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1) + # Use the console-body fragment (no shell chrome), so the envelope guard reads the console + # content only — the shared sidebar renders an unrelated ``{% icon "envelope" %}`` glyph. + content = member_client.get(reverse("dashboard"), HTTP_HX_REQUEST="true").content + assert b'data-testid="hero-howcomputed"' in content + assert b"Changes shipped = merged MRs opened by DAIV" in content + # The Hero never reads RunEnvelope — the word must not appear in the disclosure (category error). + assert b"RunEnvelope" not in content + assert b"envelope" not in content + # No NON-GOAL metric is promised (diff-survival / clean-vs-edited / sparkline). + for word in (b"diff-survival", b"sparkline", b"clean-vs", b"dev-hours"): + assert word not in content + + +@pytest.mark.django_db +class TestBreakdownMrLinkHonesty: + """AC11/AC6: link out via the persisted URL only; blank degrades; no live client on render.""" + + def test_web_url_links_out_blank_degrades_to_session(self, member_client, member_user): + _make_merge_metric(iid=1) + run_linked = _make_shipping_run( + member_user, iid=1, web_url="https://gitlab.example.com/daiv/test/-/merge_requests/1" + ) + _make_merge_metric(iid=2) + run_blank = _make_shipping_run(member_user, iid=2, web_url="") # blank → no out-link + + content = member_client.get(reverse("dashboard"), {"period": "all"}).content.decode() + # The set URL renders a real out-link opening in a new tab. + assert 'href="https://gitlab.example.com/daiv/test/-/merge_requests/1"' in content + assert 'target="_blank"' in content + # Never a broken/placeholder link when the URL is blank. + assert 'href="#"' not in content + # Both rows still carry a same-tab session drill-through. + for run in (run_linked, run_blank): + assert reverse("session_detail", kwargs={"thread_id": run.session_id}) in content + + def test_no_repo_client_instantiated_on_render(self, member_client, member_user): + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1, web_url="https://gitlab.example.com/daiv/test/-/merge_requests/1") + with patch.object(RepoClient, "create_instance") as mock_create_instance: + member_client.get(reverse("dashboard")) + member_client.get(reverse("dashboard"), {"period": "all"}) + mock_create_instance.assert_not_called() + + def test_prefers_a_run_with_a_url_over_a_newer_blank_one(self, member_client, member_user): + # A merge attributed by two of the user's runs: the OLDER persisted the url, a newer re-run + # left it blank (the field default). The good url must win — a blank newest run must not hide + # an out-link that genuinely exists (AC11 degrades only when NO attributing run has a url). + _make_merge_metric(iid=1) + _make_shipping_run(member_user, iid=1, web_url="https://gitlab.example.com/daiv/test/-/merge_requests/1") + _make_shipping_run(member_user, iid=1, web_url="") # newer re-run, blank url + content = member_client.get(reverse("dashboard"), {"period": "all"}).content.decode() + assert 'href="https://gitlab.example.com/daiv/test/-/merge_requests/1"' in content + + +@pytest.mark.django_db +class TestBreakdownA11y: + """AC10: the trigger is a native
diff --git a/daiv/accounts/views.py b/daiv/accounts/views.py index 09b34352..9ed8f2e8 100644 --- a/daiv/accounts/views.py +++ b/daiv/accounts/views.py @@ -9,6 +9,7 @@ from django.http import Http404 from django.shortcuts import redirect, render from django.urls import reverse, reverse_lazy +from django.utils import timezone from django.utils.decorators import method_decorator from django.utils.timezone import localdate from django.views import View @@ -19,6 +20,7 @@ from notifications.choices import EventType from notifications.models import Notification from sessions.models import EnvelopeStatus, Run, RunEnvelope, RunStatus, SessionOrigin +from sessions.reconcile import still_actionable from accounts.context_processors import running_jobs_count from accounts.emails import send_welcome_email @@ -381,6 +383,9 @@ def get_context_data(self, **kwargs): context["current_period"] = period context["active_schedules"] = ScheduledJob.objects.filter(user=user, is_enabled=True).count() context.update(self._get_feed_data(user)) + # Reconciliation happens at render time (AD-6 live/cached read), so the "last checked" stamp + # is the request time. Read-only / presentation-only: passed into context, never stored. + context["reconciled_at"] = timezone.now() return context @@ -461,7 +466,16 @@ def _get_feed_data(self, user: User) -> dict: if run is None: # The run was deleted out from under a stale Feed row; skip it rather than 500. continue - item = build_feed_item(run, notification, envelope=envelopes_by_run.get(str(run.id))) + envelope = envelopes_by_run.get(str(run.id)) + # Reconcile against live MR state (Story 3.3, AC4/AC5). A run that WAS actionable but + # whose MR resolved externally (merged/closed) leaves the surface — it no longer counts + # toward attention nor renders as awaiting. An ``all-clear`` run (never actionable) is + # NOT dropped; it stays as a quiet card, exactly as before. A read failure keeps the item + # visible (AC6, fail-safe) since ``still_actionable`` then resolves to actionable. + classification_actionable = envelope is None or envelope.is_actionable + if classification_actionable and not still_actionable(run, envelope): + continue + item = build_feed_item(run, notification, envelope=envelope) items.append(item) if item["status_slug"] == "classifying": in_flight_ids.append(str(run.id)) @@ -503,20 +517,21 @@ def _get_feed_data(self, user: User) -> dict: def _feed_attention_summary(feed_qs) -> tuple[bool, int]: """Return ``(has_attention, all_clear_count)`` over the FULL feed set. - ``has_attention`` is True when any feed run is classifying (no envelope yet) or has a - non-all-clear envelope. Called only when the rendered page is entirely all-clear, to avoid a - false zero-state seal that would hide a finding paged past ``FEED_PAGE_SIZE`` and to count - all-clear runs across the whole feed (not just the page). + ``has_attention`` routes every existing feed run through the shared ``still_actionable`` + predicate (Story 3.3, AC2) — a classifying run, a non-all-clear envelope, or an open/unknown + live MR read all mean attention; an all-clear envelope or an externally-resolved MR does not + — so the zero-state seal decision can never diverge from the badge or the per-item render. + Called only when the rendered page is entirely quiet, to avoid a false seal that would hide a + finding paged past ``FEED_PAGE_SIZE`` and to count all-clear runs across the whole feed. """ run_ids = list(feed_qs.values_list("source_id", flat=True)) if not run_ids: return False, 0 - statuses = list(RunEnvelope.objects.filter(run_id__in=run_ids).values_list("status", flat=True)) - all_clear_count = sum(1 for status in statuses if status == EnvelopeStatus.ALL_CLEAR) - # Fewer envelopes than feed runs ⇒ at least one run is still classifying (no envelope) — don't seal. - classifying = len(statuses) < len(run_ids) - has_non_all_clear = any(status != EnvelopeStatus.ALL_CLEAR for status in statuses) - return (classifying or has_non_all_clear), all_clear_count + runs_by_id = Run.objects.filter(pk__in=run_ids).in_bulk() + envelopes_by_run = {str(env.run_id): env for env in RunEnvelope.objects.filter(run_id__in=run_ids)} + all_clear_count = sum(1 for env in envelopes_by_run.values() if env.status == EnvelopeStatus.ALL_CLEAR) + has_attention = any(still_actionable(run, envelopes_by_run.get(str(run.id))) for run in runs_by_id.values()) + return has_attention, all_clear_count @staticmethod def _next_sweep(user: User): diff --git a/daiv/codebase/base.py b/daiv/codebase/base.py index 764336df..1a26c036 100644 --- a/daiv/codebase/base.py +++ b/daiv/codebase/base.py @@ -14,6 +14,31 @@ class GitPlatform(StrEnum): SWE = "swe" +class MergeRequestState(StrEnum): + """Live lifecycle state of a merge/pull request, normalized across platforms. + + Presentation-only (AC1): read live from the platform for render-time reconciliation and + never persisted — so this is a plain ``StrEnum``, NOT a ``models.TextChoices`` with a + DB ``CheckConstraint``. The ``resolved()`` / ``live()`` groupings mirror the + ``RunStatus.terminal()`` classmethod idiom (AR14), returning ``frozenset[str]``. + """ + + OPEN = "open" + MERGED = "merged" + CLOSED = "closed" + DRAFT = "draft" + + @classmethod + def resolved(cls) -> frozenset[str]: + """States that mean the MR was settled externally — an item in one leaves the console.""" + return frozenset({cls.MERGED, cls.CLOSED}) + + @classmethod + def live(cls) -> frozenset[str]: + """States that mean the MR is still open work — an item in one stays on the console.""" + return frozenset({cls.OPEN, cls.DRAFT}) + + class Scope(StrEnum): """ Scope of the conversation. diff --git a/daiv/codebase/clients/base.py b/daiv/codebase/clients/base.py index 25ead1c1..77a4943c 100644 --- a/daiv/codebase/clients/base.py +++ b/daiv/codebase/clients/base.py @@ -20,6 +20,7 @@ MergeRequest, MergeRequestCommit, MergeRequestDiffStats, + MergeRequestState, RepoAccessLevel, RepoMember, Repository, @@ -419,6 +420,24 @@ def get_bot_commit_email(self) -> str: def get_merge_request(self, repo_id: str, merge_request_id: int) -> MergeRequest: pass + @abc.abstractmethod + def get_merge_request_state(self, repo_id: str, merge_request_id: int) -> MergeRequestState: + """Read the live lifecycle state (open / merged / closed / draft) of a merge/pull request. + + Presentation-only trust-baseline read (AC1): returns the current platform state so the + console can reconcile against source of truth at render time. Providers RAISE their native + API error on failure (``GitlabError`` / ``GithubException``); the cached wrapper in + :mod:`codebase.mr_state` owns the fail-safe (AC6), not this method. + + Args: + repo_id: The repository ID. + merge_request_id: The merge request IID (GitLab) or pull request number (GitHub). + + Returns: + The live :class:`~codebase.base.MergeRequestState`. + """ + pass + @abc.abstractmethod def get_merge_request_by_branches( self, repo_id: str, source_branch: str, target_branch: str diff --git a/daiv/codebase/clients/github/api/callbacks.py b/daiv/codebase/clients/github/api/callbacks.py index 84996f48..93c5788e 100644 --- a/daiv/codebase/clients/github/api/callbacks.py +++ b/daiv/codebase/clients/github/api/callbacks.py @@ -2,6 +2,8 @@ from functools import cached_property from typing import Any, Literal +from django.core.cache import cache + from asgiref.sync import sync_to_async from github.GithubException import GithubException from sandbox_envs.services import resolve_env_for_run @@ -13,6 +15,7 @@ from codebase.base import Scope from codebase.clients import RepoClient from codebase.clients.base import Emoji +from codebase.mr_state import _mr_state_cache_key from codebase.repo_config import RepositoryConfig from codebase.tasks import address_issue_task, address_mr_comments_task from codebase.utils import compute_thread_id, note_mentions_daiv @@ -319,6 +322,10 @@ async def process_callback(self): merged_at=self.pull_request.merged_at or "", platform="github", ) + # Eagerly drop the cached live MR state so the merged PR leaves the console near-instantly + # rather than after the TTL (AC7). Pure cache side effect — no accept-gate change, no new + # stored state; the same posture as ``PushCallback`` invalidating the repo-config cache. + cache.delete(_mr_state_cache_key(self.repository.full_name, self.pull_request.number)) class PushCallback(GitHubCallback): diff --git a/daiv/codebase/clients/github/client.py b/daiv/codebase/clients/github/client.py index 37413acf..1c88f21f 100644 --- a/daiv/codebase/clients/github/client.py +++ b/daiv/codebase/clients/github/client.py @@ -20,6 +20,7 @@ MergeRequest, MergeRequestCommit, MergeRequestDiffStats, + MergeRequestState, Note, NoteableType, NoteDiffPosition, @@ -755,6 +756,24 @@ def get_merge_request(self, repo_id: str, merge_request_id: int) -> MergeRequest author=User(id=mr.user.id, username=mr.user.login, name=mr.user.name), ) + def get_merge_request_state(self, repo_id: str, merge_request_id: int) -> MergeRequestState: + """Read the live lifecycle state of a pull request (AC1, AC5). + + GitHub layers ``merged`` / ``draft`` booleans on top of ``state ∈ {open, closed}``: a merged + PR is ``MERGED``; a non-merged closed PR is ``CLOSED``; an open draft is ``DRAFT``; else + ``OPEN``. Raises ``GithubException`` on API failure — the cached wrapper in + :mod:`codebase.mr_state` owns the fail-safe. + """ + repo = self.client.get_repo(repo_id, lazy=True) + pr = repo.get_pull(merge_request_id) + if pr.merged: + return MergeRequestState.MERGED + if pr.state == "closed": + return MergeRequestState.CLOSED + if pr.draft: + return MergeRequestState.DRAFT + return MergeRequestState.OPEN + def get_merge_request_comment(self, repo_id: str, merge_request_id: int, comment_id: str) -> Discussion: """ Get a comment from a merge request. diff --git a/daiv/codebase/clients/gitlab/api/callbacks.py b/daiv/codebase/clients/gitlab/api/callbacks.py index eee61dec..921ea07d 100644 --- a/daiv/codebase/clients/gitlab/api/callbacks.py +++ b/daiv/codebase/clients/gitlab/api/callbacks.py @@ -2,6 +2,8 @@ from functools import cached_property from typing import Any, Literal +from django.core.cache import cache + from gitlab.exceptions import GitlabError from sandbox_envs.services import resolve_env_for_run from sessions.models import SessionOrigin @@ -12,6 +14,7 @@ from codebase.base import Scope from codebase.clients import RepoClient from codebase.clients.base import Emoji +from codebase.mr_state import _mr_state_cache_key from codebase.repo_config import RepositoryConfig from codebase.tasks import address_issue_task, address_mr_comments_task from codebase.utils import compute_thread_id, note_mentions_daiv @@ -337,6 +340,10 @@ async def process_callback(self): merged_at=self.object_attributes.merged_at or "", platform="gitlab", ) + # Eagerly drop the cached live MR state so the merged MR leaves the console near-instantly + # rather than after the TTL (AC7). Pure cache side effect — no accept-gate change, no new + # stored state; the same posture as ``PushCallback`` invalidating the repo-config cache. + cache.delete(_mr_state_cache_key(self.project.path_with_namespace, self.object_attributes.iid)) class PushCallback(BaseCallback): diff --git a/daiv/codebase/clients/gitlab/client.py b/daiv/codebase/clients/gitlab/client.py index c81c3548..c6819c71 100644 --- a/daiv/codebase/clients/gitlab/client.py +++ b/daiv/codebase/clients/gitlab/client.py @@ -23,6 +23,7 @@ MergeRequest, MergeRequestCommit, MergeRequestDiffStats, + MergeRequestState, Note, NoteDiffPosition, NotePosition, @@ -901,6 +902,25 @@ def get_merge_request(self, repo_id: str, merge_request_id: int) -> MergeRequest author=User(id=mr.author.get("id"), username=mr.author.get("username"), name=mr.author.get("name")), ) + def get_merge_request_state(self, repo_id: str, merge_request_id: int) -> MergeRequestState: + """Read the live lifecycle state of a merge request (AC1, AC5). + + Maps GitLab's raw ``state`` (``opened`` / ``merged`` / ``closed`` / ``locked``) plus the + ``work_in_progress`` flag (the same flag ``_serialize_merge_request`` maps to ``draft``) onto + the normalized :class:`MergeRequestState`: a ``locked`` MR is settled like ``closed``, and an + ``opened`` WIP MR is a ``DRAFT``. Raises ``GitlabError`` on API failure — the cached wrapper + in :mod:`codebase.mr_state` owns the fail-safe. + """ + project = self.client.projects.get(repo_id, lazy=True) + mr = project.mergerequests.get(merge_request_id) + if mr.state == "merged": + return MergeRequestState.MERGED + if mr.state in ("closed", "locked"): + return MergeRequestState.CLOSED + if mr.work_in_progress: + return MergeRequestState.DRAFT + return MergeRequestState.OPEN + def get_merge_request_comment(self, repo_id: str, merge_request_id: int, comment_id: str) -> Discussion: """ Get a comment from a merge request. diff --git a/daiv/codebase/clients/swe.py b/daiv/codebase/clients/swe.py index 517c0dc9..50461090 100644 --- a/daiv/codebase/clients/swe.py +++ b/daiv/codebase/clients/swe.py @@ -17,6 +17,7 @@ MergeRequest, MergeRequestCommit, MergeRequestDiffStats, + MergeRequestState, RepoMember, Repository, User, @@ -335,6 +336,15 @@ def get_merge_request(self, repo_id: str, merge_request_id: int) -> MergeRequest """Not supported for SWE client.""" raise NotImplementedError("SWERepoClient does not support merge requests") + def get_merge_request_state(self, repo_id: str, merge_request_id: int) -> MergeRequestState: + """SWE runs have no live MR lifecycle: report ``OPEN`` (unresolved → keep visible, AC6). + + Returning a benign default rather than raising keeps AC6's fail-safe intact without the + cached wrapper logging an exception on every reconcile of a SWE install — the same graceful + posture as :meth:`get_merge_request_by_branches` returning ``None``. Genuine GitLab/GitHub + API failures still raise from their clients and are caught by the wrapper.""" + return MergeRequestState.OPEN + def get_merge_request_by_branches( self, repo_id: str, source_branch: str, target_branch: str ) -> MergeRequest | None: diff --git a/daiv/codebase/mr_state.py b/daiv/codebase/mr_state.py new file mode 100644 index 00000000..4cfc7fe4 --- /dev/null +++ b/daiv/codebase/mr_state.py @@ -0,0 +1,65 @@ +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +from django.core.cache import cache + +from codebase.base import MergeRequestState + +if TYPE_CHECKING: + from collections.abc import Iterable + +logger = logging.getLogger("daiv.clients") + +MR_STATE_CACHE_KEY_PREFIX = "mr_state:" +# Short TTL (W2 render-latency/cache decision): a refresh reflects reality within this window. +# Eager webhook invalidation (codebase/clients/{gitlab,github}/api/callbacks.py) is the primary +# freshness path; this TTL is the fallback for a close-without-merge or a missed webhook. +MR_STATE_CACHE_TTL = 60 # seconds + + +def _mr_state_cache_key(repo_id: str, iid: int) -> str: + """Build the cache key for one MR's live state. + + The single source of the key (AC7), used by BOTH the read-through below and the webhook + invalidation ``cache.delete`` — a divergent key would leave a merged MR cached-stale until the + TTL, so the read and the invalidation must never drift. + """ + return f"{MR_STATE_CACHE_KEY_PREFIX}{repo_id}:{iid}" + + +def get_merge_request_state(repo_id: str, iid: int) -> MergeRequestState: + """Cached read-through for one MR's live lifecycle state (AC3, AC6, AC7). + + Cache-first, mirroring the ``codebase/repo_config.py`` idiom: a warm key returns the rehydrated + enum with NO client call; a cold key calls the provider and caches the enum ``.value`` (a + ``str``) for :data:`MR_STATE_CACHE_TTL`. This wrapper OWNS the fail-safe: any exception, timeout, + or unconfigured client resolves to :attr:`MergeRequestState.OPEN` (logged at ``warning``) so a + read failure keeps the item visible (NFR1) and never raises up into a render path. + """ + key = _mr_state_cache_key(repo_id, iid) + try: + if (cached := cache.get(key)) is not None: + return MergeRequestState(cached) + + # Lazy import: keep this module importable without pulling the whole client graph, and + # let the mocked factory take over in tests. + from codebase.clients import RepoClient + + state = RepoClient.create_instance().get_merge_request_state(repo_id, merge_request_id=iid) + cache.set(key, state.value, MR_STATE_CACHE_TTL) + return state + except Exception: + logger.warning("Live MR-state read failed for %s!%s; keeping item visible", repo_id, iid, exc_info=True) + return MergeRequestState.OPEN + + +def get_merge_request_states(repo_id: str, iids: Iterable[int]) -> dict[int, MergeRequestState]: + """Batch entry point: resolve many MRs' live states in one call (AC7). + + Loops the per-item cached read (warm items cost nothing), so Epic 4's Needs-me Queue can + reconcile a list of MRs without N cold sequential round-trips. Each item is independently + fail-safe (a per-item failure resolves to ``OPEN``). + """ + return {iid: get_merge_request_state(repo_id, iid) for iid in iids} diff --git a/daiv/sessions/reconcile.py b/daiv/sessions/reconcile.py new file mode 100644 index 00000000..01eb2314 --- /dev/null +++ b/daiv/sessions/reconcile.py @@ -0,0 +1,46 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from codebase.base import MergeRequestState +from sessions.models import RunEnvelope + +if TYPE_CHECKING: + from sessions.models import Run + +# Sentinel distinguishing "caller passed no envelope, look it up" (``_UNSET``) from "caller asserts +# there is no envelope — the run is still classifying" (an explicit ``None``). Letting ``None`` mean +# classifying lets the badge/Feed pass their already-batched per-run envelope directly (``None`` for +# a run with no envelope) with no re-query, so no N+1 is introduced. Typed ``Any`` (matching the +# house sentinel idiom) so the default is assignable to the ``RunEnvelope | None`` parameter. +_UNSET: Any = object() + + +def still_actionable(run: Run, envelope: RunEnvelope | None = _UNSET) -> bool: + """The single shared liveness predicate (AC2) — read-only (AC3), fail-safe (AC6). + + Composes two checks and NEVER writes (no ``.save()`` / ``aupdate`` / signal): + + 1. **Classification actionability.** ``envelope is _UNSET`` → look it up via + ``RunEnvelope.objects.for_run(run)``. An explicit ``None`` (or a resolved ``None``) means the + run is still *classifying* → actionable until it resolves. Otherwise defer to + ``envelope.is_actionable`` (an ``all-clear`` envelope is not actionable). Because + ``RunEnvelope.clean()`` enforces ``is_actionable ⟺ status != all-clear`` for every persisted + envelope, this preserves the badge's existing counted cases. + 2. **Live MR resolution.** Only when the item references an MR (``run.merge_request_iid`` is set) + do we consult the Task-2 cached live read: a confirmed ``MERGED`` / ``CLOSED`` state means the + MR was resolved externally → *not* actionable (AC4/AC5); ``OPEN`` / ``DRAFT``, an unknown + state, or a failed read keeps it actionable (AC6, the fail-safe: an item leaves only on a + confirmed resolution). A run with no ``merge_request_iid`` skips the live read entirely. + """ + env = RunEnvelope.objects.for_run(run) if envelope is _UNSET else envelope + actionable = True if env is None else env.is_actionable + if not actionable or run.merge_request_iid is None: + return actionable + # Lazy import: keeps the sessions <-> codebase dependency acyclic at import time. + from codebase.mr_state import get_merge_request_state + + # ``actionable`` is necessarily True here (the guard above returned otherwise), so liveness is + # decided solely by the MR state: a confirmed resolution drops the item, anything else keeps it. + state = get_merge_request_state(run.repo_id, run.merge_request_iid) + return state not in MergeRequestState.resolved() diff --git a/tests/unit_tests/accounts/test_context_processors.py b/tests/unit_tests/accounts/test_context_processors.py index d6d1c006..3ef8acac 100644 --- a/tests/unit_tests/accounts/test_context_processors.py +++ b/tests/unit_tests/accounts/test_context_processors.py @@ -187,6 +187,8 @@ def rf(): def _make_feed_run(user, *, envelope_status=None, repo_id="group/project"): """Build a SCHEDULE run for ``user``, optionally with a ``RunEnvelope`` of the given status.""" + from sessions.envelopes import build_actionable_item + session = Session.objects.create( thread_id=str(uuid.uuid4()), origin=SessionOrigin.SCHEDULE, repo_id=repo_id, user=user ) @@ -199,7 +201,14 @@ def _make_feed_run(user, *, envelope_status=None, repo_id="group/project"): finished_at=timezone.now(), ) if envelope_status is not None: - RunEnvelope.objects.create(run=run, status=envelope_status) + # A found-issues envelope MUST carry ≥1 actionable item (RunEnvelope.clean() / FR-5), so its + # ``is_actionable`` is True — the invariant the badge's predicate routing relies on. + actionable = ( + [build_actionable_item(id="1", kind="bug", label="issue", ref="a.py")] + if envelope_status == EnvelopeStatus.FOUND_ISSUES + else [] + ) + RunEnvelope.objects.create(run=run, status=envelope_status, actionable=actionable) return run @@ -320,3 +329,68 @@ def test_bell_mark_all_read_leaves_feed_count(self, member_user): _feed_notif(member_user, run.pk) Notification.mark_all_read_for(member_user, exclude_event_types=(EventType.RUN_FEED,)) assert feed_unread_attention_count(member_user) == 1 + + +def _make_mr_feed_run(user, *, envelope_status, merge_request_iid, repo_id="group/project"): + """An attention run that references an MR (so the badge reconciles it against live MR state).""" + from sessions.envelopes import build_actionable_item + + session = Session.objects.create( + thread_id=str(uuid.uuid4()), origin=SessionOrigin.MR_WEBHOOK, repo_id=repo_id, user=user + ) + run = Run.objects.create( + session=session, + trigger_type=SessionOrigin.MR_WEBHOOK, + repo_id=repo_id, + status=RunStatus.SUCCESSFUL, + user=user, + merge_request_iid=merge_request_iid, + finished_at=timezone.now(), + ) + actionable = ( + [build_actionable_item(id="1", kind="bug", label="issue", ref="a.py")] + if envelope_status == EnvelopeStatus.FOUND_ISSUES + else [] + ) + RunEnvelope.objects.create(run=run, status=envelope_status, actionable=actionable) + return run + + +@pytest.mark.django_db +class TestFeedUnreadAttentionCountReconcile: + """Story 3.3 — the badge routes liveness through ``still_actionable`` (AC2, AC4, AC5, AC6).""" + + _LIVE_READ = "codebase.mr_state.get_merge_request_state" + + def test_externally_merged_mr_run_is_excluded(self, member_user): + from codebase.base import MergeRequestState + + run = _make_mr_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + _feed_notif(member_user, run.pk) + with patch(self._LIVE_READ, return_value=MergeRequestState.MERGED): + assert feed_unread_attention_count(member_user) == 0 + + def test_open_mr_run_is_counted(self, member_user): + from codebase.base import MergeRequestState + + run = _make_mr_feed_run(member_user, envelope_status=EnvelopeStatus.FOUND_ISSUES, merge_request_iid=7) + _feed_notif(member_user, run.pk) + with patch(self._LIVE_READ, return_value=MergeRequestState.OPEN): + assert feed_unread_attention_count(member_user) == 1 + + def test_read_failure_keeps_the_run_counted(self, member_user): + # The wrapper resolves a failed read to OPEN → the item stays counted (AC6, fail-safe). + from codebase.base import MergeRequestState + + run = _make_mr_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + _feed_notif(member_user, run.pk) + with patch(self._LIVE_READ, return_value=MergeRequestState.OPEN): + assert feed_unread_attention_count(member_user) == 1 + + def test_all_clear_mr_run_never_triggers_a_live_read(self, member_user): + # An all-clear run is quiet regardless of its MR: no live read, not counted. + run = _make_mr_feed_run(member_user, envelope_status=EnvelopeStatus.ALL_CLEAR, merge_request_iid=7) + _feed_notif(member_user, run.pk) + with patch(self._LIVE_READ) as read: + assert feed_unread_attention_count(member_user) == 0 + read.assert_not_called() diff --git a/tests/unit_tests/accounts/test_dashboard_console.py b/tests/unit_tests/accounts/test_dashboard_console.py index 4342f362..0b843576 100644 --- a/tests/unit_tests/accounts/test_dashboard_console.py +++ b/tests/unit_tests/accounts/test_dashboard_console.py @@ -19,7 +19,7 @@ import pytest from notifications.choices import EventType from notifications.models import Notification -from sessions.models import Run, RunStatus, Session, SessionOrigin +from sessions.models import EnvelopeStatus, Run, RunEnvelope, RunStatus, Session, SessionOrigin from accounts.models import Role from accounts.views import get_velocity_data @@ -954,3 +954,116 @@ def test_clickthrough_strings_are_translated(self): "{% blocktranslate with iid=row.merge_request_iid %}open MR !{{ iid }} on GitLab{% endblocktranslate %}" ) assert aria_tag in src + + +# --------------------------------------------------------------------------- +# Story 3.3 — reconcile with source of truth (live MR read + freshness stamp) +# --------------------------------------------------------------------------- + +_LIVE_READ = "codebase.mr_state.get_merge_request_state" + + +def _make_scheduled_feed_run(user, *, envelope_status, merge_request_iid=None, repo_id="daiv/test"): + """A scheduled RUN_FEED run (+ envelope + notification) for ``user``, optionally MR-referencing.""" + session = Session.objects.create( + thread_id=str(uuid.uuid4()), origin=SessionOrigin.SCHEDULE, repo_id=repo_id, user=user + ) + run = Run.objects.create( + session=session, + trigger_type=SessionOrigin.SCHEDULE, + repo_id=repo_id, + status=RunStatus.SUCCESSFUL, + user=user, + merge_request_iid=merge_request_iid, + finished_at=timezone.now(), + ) + RunEnvelope.objects.create(run=run, status=envelope_status) + Notification.objects.create( + recipient=user, + event_type=EventType.RUN_FEED, + source_type="sessions.Run", + source_id=str(run.pk), + subject="nightly", + body="", + link_url=reverse("session_detail", kwargs={"thread_id": session.thread_id}), + ) + return run + + +@pytest.mark.django_db +class TestConsoleReconcile: + """AC4/AC5/AC8 — the console reflects live MR state and stamps a freshness time.""" + + def test_externally_merged_run_leaves_the_feed(self, member_client, member_user): + from codebase.base import MergeRequestState + + # A lone needs-attention run whose MR merged externally must drop out — the "nothing needs + # you." seal resolves instead of an awaiting item. + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + with patch(_LIVE_READ, return_value=MergeRequestState.MERGED): + response = member_client.get(reverse("dashboard")) + content = response.content.decode() + assert response.status_code == 200 + assert 'data-status="needs-attention"' not in content + assert 'data-testid="feed-zero-state"' in content + + def test_open_mr_run_still_shows_as_awaiting(self, member_client, member_user): + from codebase.base import MergeRequestState + + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + with patch(_LIVE_READ, return_value=MergeRequestState.OPEN): + response = member_client.get(reverse("dashboard")) + content = response.content.decode() + assert 'data-status="needs-attention"' in content + + def test_read_failure_keeps_the_run_visible(self, member_client, member_user): + # The wrapper resolves a failed read to OPEN → the item stays (AC6, fail-safe). + from codebase.base import MergeRequestState + + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + with patch(_LIVE_READ, return_value=MergeRequestState.OPEN): + response = member_client.get(reverse("dashboard")) + assert 'data-status="needs-attention"' in response.content.decode() + + def test_all_clear_stays_a_quiet_card_in_a_mixed_feed(self, member_client, member_user): + # An all-clear run is NOT dropped by reconciliation — only externally-resolved actionable + # runs leave. A mixed feed keeps the quiet all-clear card. + from codebase.base import MergeRequestState + + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.ALL_CLEAR) + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.FAILED) + with patch(_LIVE_READ, return_value=MergeRequestState.OPEN): + response = member_client.get(reverse("dashboard")) + content = response.content.decode() + assert 'data-status="all-clear"' in content + assert 'data-status="failed"' in content + + def test_reconciled_at_in_context_and_last_checked_rendered(self, member_client, member_user): + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.ALL_CLEAR) + response = member_client.get(reverse("dashboard")) + assert response.context["reconciled_at"] is not None + content = response.content.decode() + assert 'data-testid="feed-reconciled-meta"' in content + assert "last checked" in content + + def test_last_checked_string_is_translated(self): + src = Path(get_template("accounts/_feed.html").origin.name).read_text(encoding="utf-8") + assert '{% blocktranslate with checked=reconciled_at|date:"H:i" %}last checked {{ checked }}' in src + + def test_render_performs_no_writes(self, member_client, member_user): + from codebase.base import MergeRequestState + + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=7) + before = (MergeMetric.objects.count(), Run.objects.count(), RunEnvelope.objects.count()) + with patch(_LIVE_READ, return_value=MergeRequestState.MERGED): + member_client.get(reverse("dashboard")) + after = (MergeMetric.objects.count(), Run.objects.count(), RunEnvelope.objects.count()) + assert before == after + + def test_no_client_instantiated_for_non_mr_feed(self, member_client, member_user): + # A feed of runs with no MR reference must not reach the live read at all. + _make_scheduled_feed_run(member_user, envelope_status=EnvelopeStatus.NEEDS_ATTENTION, merge_request_iid=None) + with patch(_LIVE_READ) as read: + response = member_client.get(reverse("dashboard")) + assert response.status_code == 200 + read.assert_not_called() diff --git a/tests/unit_tests/codebase/clients/github/test_client.py b/tests/unit_tests/codebase/clients/github/test_client.py index 8173920e..25a263fa 100644 --- a/tests/unit_tests/codebase/clients/github/test_client.py +++ b/tests/unit_tests/codebase/clients/github/test_client.py @@ -6,7 +6,7 @@ from github.IssueComment import IssueComment from github.PullRequestComment import PullRequestComment -from codebase.base import GitPlatform, MergeRequestCommit, Repository, User +from codebase.base import GitPlatform, MergeRequestCommit, MergeRequestState, Repository, User from codebase.clients.base import Emoji, GitAuthEnv from codebase.clients.github.client import GitHubClient @@ -424,6 +424,38 @@ def test_get_merge_request_by_branches_returns_first_open_match(self, github_cli assert result.labels == ["enhancement"] mock_repo.get_pulls.assert_called_once_with(state="open", base="main", head="feat-x") + @pytest.mark.parametrize( + ("merged", "state", "draft", "expected"), + [ + (True, "closed", False, MergeRequestState.MERGED), + # A merged PR reports state == "closed"; the ``merged`` boolean wins. + (True, "open", False, MergeRequestState.MERGED), + (False, "closed", False, MergeRequestState.CLOSED), + (False, "open", True, MergeRequestState.DRAFT), + (False, "open", False, MergeRequestState.OPEN), + ], + ) + def test_get_merge_request_state_maps_platform_state(self, github_client, merged, state, draft, expected): + mock_repo = Mock() + mock_pr = Mock() + mock_pr.merged = merged + mock_pr.state = state + mock_pr.draft = draft + mock_repo.get_pull.return_value = mock_pr + github_client.client.get_repo.return_value = mock_repo + + assert github_client.get_merge_request_state("owner/repo", 7) == expected + mock_repo.get_pull.assert_called_once_with(7) + + def test_get_merge_request_state_propagates_github_exception(self, github_client): + """Providers RAISE their native error — the cached wrapper owns the fail-safe, not the client.""" + mock_repo = Mock() + mock_repo.get_pull.side_effect = GithubException(500, {}, {}) + github_client.client.get_repo.return_value = mock_repo + + with pytest.raises(GithubException): + github_client.get_merge_request_state("owner/repo", 7) + def test_is_branch_protected_returns_true_when_branch_protected(self, github_client): mock_repo = Mock() mock_repo.get_branch.return_value = Mock(protected=True) diff --git a/tests/unit_tests/codebase/clients/gitlab/test_client.py b/tests/unit_tests/codebase/clients/gitlab/test_client.py index 85f727f0..a33f9566 100644 --- a/tests/unit_tests/codebase/clients/gitlab/test_client.py +++ b/tests/unit_tests/codebase/clients/gitlab/test_client.py @@ -7,7 +7,7 @@ from git import GitCommandError from gitlab.exceptions import GitlabCreateError, GitlabGetError -from codebase.base import GitPlatform, MergeRequestCommit, MergeRequestDiffStats, Repository, User +from codebase.base import GitPlatform, MergeRequestCommit, MergeRequestDiffStats, MergeRequestState, Repository, User from codebase.clients.base import Emoji, GitAuthEnv from codebase.clients.gitlab.client import ( MERGE_REQUEST_BRANCH_VISIBILITY_RETRY_BACKOFF_SECONDS, @@ -342,6 +342,38 @@ def test_create_merge_request_inline_discussion_returns_discussion_id(self, gitl assert result == "unique-id-xyz" + @pytest.mark.parametrize( + ("raw_state", "work_in_progress", "expected"), + [ + ("merged", False, MergeRequestState.MERGED), + ("closed", False, MergeRequestState.CLOSED), + ("locked", False, MergeRequestState.CLOSED), + ("opened", True, MergeRequestState.DRAFT), + ("opened", False, MergeRequestState.OPEN), + # A merged MR whose stale WIP flag is set is still MERGED (merged wins over draft). + ("merged", True, MergeRequestState.MERGED), + ], + ) + def test_get_merge_request_state_maps_platform_state(self, gitlab_client, raw_state, work_in_progress, expected): + mock_project = Mock() + mock_mr = Mock() + mock_mr.state = raw_state + mock_mr.work_in_progress = work_in_progress + mock_project.mergerequests.get.return_value = mock_mr + gitlab_client.client.projects.get.return_value = mock_project + + assert gitlab_client.get_merge_request_state("group/repo", 5) == expected + mock_project.mergerequests.get.assert_called_once_with(5) + + def test_get_merge_request_state_propagates_gitlab_error(self, gitlab_client): + """Providers RAISE their native error — the cached wrapper owns the fail-safe, not the client.""" + mock_project = Mock() + mock_project.mergerequests.get.side_effect = GitlabGetError("boom", response_code=500) + gitlab_client.client.projects.get.return_value = mock_project + + with pytest.raises(GitlabGetError): + gitlab_client.get_merge_request_state("group/repo", 5) + @staticmethod def _merge_request_mock(source_branch="feat/x"): """A GitLab merge request object shaped for `_serialize_merge_request`.""" diff --git a/tests/unit_tests/codebase/clients/test_swe.py b/tests/unit_tests/codebase/clients/test_swe.py index c4979449..b6bb7eb6 100644 --- a/tests/unit_tests/codebase/clients/test_swe.py +++ b/tests/unit_tests/codebase/clients/test_swe.py @@ -211,6 +211,13 @@ def test_current_user(self, swe_client): assert user.username == "swe-bench" assert user.name == "SWE Bench" + def test_get_merge_request_state_returns_open(self, swe_client): + """SWE has no live MR lifecycle: report OPEN (unresolved → keep visible) rather than raise, + so AC6's fail-safe stays intact without the wrapper logging on every reconcile.""" + from codebase.base import MergeRequestState + + assert swe_client.get_merge_request_state("psf/requests", 1) == MergeRequestState.OPEN + def test_unsupported_methods_raise_not_implemented(self, swe_client): """Test that unsupported methods raise NotImplementedError.""" unsupported_methods = [ diff --git a/tests/unit_tests/codebase/test_mr_state.py b/tests/unit_tests/codebase/test_mr_state.py new file mode 100644 index 00000000..5a889c2d --- /dev/null +++ b/tests/unit_tests/codebase/test_mr_state.py @@ -0,0 +1,106 @@ +"""Story 3.3 — the cached, batch-capable, fail-safe MR-state read-through (AC3, AC6, AC7). + +Exercises the cache read-through (cold hits the client + ``cache.set``; warm skips the client), +webhook-style invalidation (``cache.delete`` via the centralized key), the fail-safe (any error → +``OPEN``, logged, cache not poisoned), and the batch entry point. +""" + +import logging + +from django.core.cache import cache + +import pytest + +from codebase.base import MergeRequestState +from codebase.mr_state import MR_STATE_CACHE_TTL, _mr_state_cache_key, get_merge_request_state, get_merge_request_states + + +@pytest.fixture(autouse=True) +def _clear_mr_state_cache(): + # LocMemCache is not cleared between tests; the mr_state keys would otherwise leak. + cache.clear() + yield + cache.clear() + + +class TestCacheKey: + def test_key_is_prefixed_and_scoped(self): + assert _mr_state_cache_key("group/repo", 7) == "mr_state:group/repo:7" + + def test_ttl_is_short_window(self): + assert MR_STATE_CACHE_TTL == 60 + + +@pytest.mark.django_db +class TestCachedReadThrough: + def test_cold_read_calls_client_and_caches_the_value(self, mock_repo_client): + mock_repo_client.get_merge_request_state.return_value = MergeRequestState.MERGED + + result = get_merge_request_state("group/repo", 5) + + assert result == MergeRequestState.MERGED + mock_repo_client.get_merge_request_state.assert_called_once_with("group/repo", merge_request_id=5) + # The enum VALUE (a str) is cached, not the enum instance. + assert cache.get(_mr_state_cache_key("group/repo", 5)) == "merged" + + def test_warm_read_skips_the_client(self, mock_repo_client): + cache.set(_mr_state_cache_key("group/repo", 5), "closed", MR_STATE_CACHE_TTL) + + result = get_merge_request_state("group/repo", 5) + + assert result == MergeRequestState.CLOSED + mock_repo_client.get_merge_request_state.assert_not_called() + + def test_warm_read_rehydrates_the_enum(self, mock_repo_client): + cache.set(_mr_state_cache_key("group/repo", 5), "draft", MR_STATE_CACHE_TTL) + + result = get_merge_request_state("group/repo", 5) + + assert result is MergeRequestState.DRAFT + + def test_invalidation_deletes_the_key(self, mock_repo_client): + mock_repo_client.get_merge_request_state.return_value = MergeRequestState.OPEN + get_merge_request_state("group/repo", 5) + assert cache.get(_mr_state_cache_key("group/repo", 5)) is not None + + # The webhook path deletes via the same centralized helper. + cache.delete(_mr_state_cache_key("group/repo", 5)) + assert cache.get(_mr_state_cache_key("group/repo", 5)) is None + + def test_read_failure_returns_open_and_logs_without_poisoning_cache(self, mock_repo_client, caplog): + mock_repo_client.get_merge_request_state.side_effect = RuntimeError("gitlab down") + + with caplog.at_level(logging.WARNING, logger="daiv.clients"): + result = get_merge_request_state("group/repo", 5) + + assert result == MergeRequestState.OPEN # fail-safe: keep the item visible (AC6) + assert "keeping item visible" in caplog.text + # A failed read must NOT persist a state — the next read retries. + assert cache.get(_mr_state_cache_key("group/repo", 5)) is None + + +@pytest.mark.django_db +class TestBatchRead: + def test_batch_returns_dict_keyed_by_iid(self, mock_repo_client): + mock_repo_client.get_merge_request_state.side_effect = [MergeRequestState.MERGED, MergeRequestState.OPEN] + + result = get_merge_request_states("group/repo", [1, 2]) + + assert result == {1: MergeRequestState.MERGED, 2: MergeRequestState.OPEN} + + def test_batch_serves_warm_items_without_a_client_call(self, mock_repo_client): + cache.set(_mr_state_cache_key("group/repo", 1), "merged", MR_STATE_CACHE_TTL) + mock_repo_client.get_merge_request_state.return_value = MergeRequestState.OPEN + + result = get_merge_request_states("group/repo", [1, 2]) + + assert result == {1: MergeRequestState.MERGED, 2: MergeRequestState.OPEN} + # Only the cold item (iid 2) hit the client. + mock_repo_client.get_merge_request_state.assert_called_once_with("group/repo", merge_request_id=2) + + def test_batch_is_per_item_fail_safe(self, mock_repo_client): + mock_repo_client.get_merge_request_state.side_effect = [MergeRequestState.MERGED, RuntimeError("boom")] + + result = get_merge_request_states("group/repo", [1, 2]) + + assert result == {1: MergeRequestState.MERGED, 2: MergeRequestState.OPEN} diff --git a/tests/unit_tests/sessions/test_reconcile.py b/tests/unit_tests/sessions/test_reconcile.py new file mode 100644 index 00000000..021756b3 --- /dev/null +++ b/tests/unit_tests/sessions/test_reconcile.py @@ -0,0 +1,145 @@ +"""Story 3.3 — the single shared ``still_actionable`` predicate (AC2, AC3, AC4, AC6). + +Full matrix: envelope actionability (all-clear / found-issues / classifying), the live MR read +(open / draft / merged / closed), the ``UNSET`` vs explicit-``None`` envelope contract, the no-MR +short-circuit, the fail-safe (a read resolving to ``OPEN`` keeps the item), and the no-write +guarantee. +""" + +import uuid +from unittest.mock import patch + +import pytest +from sessions.envelopes import build_actionable_item +from sessions.models import EnvelopeStatus, Run, RunEnvelope, RunStatus, Session, SessionOrigin +from sessions.reconcile import still_actionable + +from codebase.base import MergeRequestState + +_LIVE_READ = "codebase.mr_state.get_merge_request_state" + + +def _make_run(*, merge_request_iid=None, repo_id="group/project"): + session = Session.objects.create(thread_id=str(uuid.uuid4()), origin=SessionOrigin.SCHEDULE, repo_id=repo_id) + return Run.objects.create( + session=session, + trigger_type=SessionOrigin.SCHEDULE, + repo_id=repo_id, + status=RunStatus.SUCCESSFUL, + merge_request_iid=merge_request_iid, + ) + + +def _make_envelope(run, status, *, n_actionable=0): + actionable = [ + build_actionable_item(id=str(i), kind="bug", label=f"issue {i}", ref="a.py") for i in range(n_actionable) + ] + return RunEnvelope.objects.create(run=run, status=status, actionable=actionable) + + +@pytest.mark.django_db +class TestClassificationActionability: + """Layer 1: with no MR to reconcile against, the predicate mirrors envelope actionability.""" + + def test_all_clear_is_not_actionable(self): + run = _make_run() + env = _make_envelope(run, EnvelopeStatus.ALL_CLEAR) + assert still_actionable(run, env) is False + + def test_found_issues_is_actionable(self): + run = _make_run() + env = _make_envelope(run, EnvelopeStatus.FOUND_ISSUES, n_actionable=2) + assert still_actionable(run, env) is True + + def test_needs_attention_is_actionable(self): + run = _make_run() + env = _make_envelope(run, EnvelopeStatus.NEEDS_ATTENTION) + assert still_actionable(run, env) is True + + def test_failed_is_actionable(self): + run = _make_run() + env = _make_envelope(run, EnvelopeStatus.FAILED) + assert still_actionable(run, env) is True + + def test_explicit_none_is_classifying_and_actionable(self): + # An explicit ``None`` asserts "no envelope exists" (classifying) — no re-query. + run = _make_run() + assert still_actionable(run, None) is True + + def test_unset_looks_up_the_envelope(self): + # The default (``_UNSET``) resolves the envelope via ``for_run``. + run = _make_run() + _make_envelope(run, EnvelopeStatus.ALL_CLEAR) + assert still_actionable(run) is False + + def test_unset_with_no_envelope_is_classifying(self): + run = _make_run() + assert still_actionable(run) is True + + +@pytest.mark.django_db +class TestLiveMrResolution: + """Layer 2: an actionable, MR-referencing run reconciles against the live cached read.""" + + @pytest.mark.parametrize("state", [MergeRequestState.OPEN, MergeRequestState.DRAFT]) + def test_open_or_draft_mr_stays_actionable(self, state): + run = _make_run(merge_request_iid=5) + env = _make_envelope(run, EnvelopeStatus.NEEDS_ATTENTION) + with patch(_LIVE_READ, return_value=state) as read: + assert still_actionable(run, env) is True + read.assert_called_once_with("group/project", 5) + + @pytest.mark.parametrize("state", [MergeRequestState.MERGED, MergeRequestState.CLOSED]) + def test_resolved_mr_is_not_actionable(self, state): + # Merged/closed externally → resolved, leaves the console (AC4/AC5). + run = _make_run(merge_request_iid=5) + env = _make_envelope(run, EnvelopeStatus.FOUND_ISSUES, n_actionable=1) + with patch(_LIVE_READ, return_value=state): + assert still_actionable(run, env) is False + + def test_classifying_run_with_merged_mr_is_not_actionable(self): + run = _make_run(merge_request_iid=9) + with patch(_LIVE_READ, return_value=MergeRequestState.MERGED): + assert still_actionable(run, None) is False + + def test_failed_read_resolves_to_open_and_keeps_the_item(self): + # The wrapper owns the fail-safe: a failed read surfaces here as ``OPEN`` → stays (AC6). + run = _make_run(merge_request_iid=5) + env = _make_envelope(run, EnvelopeStatus.NEEDS_ATTENTION) + with patch(_LIVE_READ, return_value=MergeRequestState.OPEN): + assert still_actionable(run, env) is True + + def test_all_clear_never_triggers_a_live_read(self): + # A non-actionable envelope short-circuits before the MR read, even with an MR reference. + run = _make_run(merge_request_iid=5) + env = _make_envelope(run, EnvelopeStatus.ALL_CLEAR) + with patch(_LIVE_READ) as read: + assert still_actionable(run, env) is False + read.assert_not_called() + + def test_no_merge_request_iid_skips_the_live_read(self): + run = _make_run(merge_request_iid=None) + env = _make_envelope(run, EnvelopeStatus.NEEDS_ATTENTION) + with patch(_LIVE_READ) as read: + assert still_actionable(run, env) is True + read.assert_not_called() + + +@pytest.mark.django_db +class TestNoWrites: + """AC3: the predicate is presentation-only — it mutates no stored state.""" + + def test_predicate_performs_no_writes(self): + run = _make_run(merge_request_iid=5) + env = _make_envelope(run, EnvelopeStatus.FOUND_ISSUES, n_actionable=1) + + with ( + patch(_LIVE_READ, return_value=MergeRequestState.MERGED), + patch.object(Run, "save") as run_save, + patch.object(RunEnvelope, "save") as env_save, + ): + still_actionable(run, env) + still_actionable(run) # also the UNSET/for_run path + + run_save.assert_not_called() + env_save.assert_not_called() From eaf9b01337877b8504accb71521b905419c939b9 Mon Sep 17 00:00:00 2001 From: Sandro Date: Mon, 20 Jul 2026 14:49:38 +0100 Subject: [PATCH 5/6] fix(templates): Use {% comment %} for multi-line comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Django's {# … #} comment is single-line only — the lexer's tag_re has no DOTALL flag, so a {# … #} spanning a newline is not recognised as a comment: the text renders literally and any {{ … }} / {% … %} inside it gets evaluated. Convert the multi-line {# … #} blocks in the review-console templates to {% comment %} … {% endcomment %}, and record the convention in AGENTS.md. --- AGENTS.md | 2 ++ .../templates/accounts/_console_body.html | 8 ++++---- daiv/accounts/templates/accounts/_feed.html | 4 ++-- daiv/accounts/templates/accounts/_feed_item.html | 8 ++++---- daiv/accounts/templates/accounts/_hero.html | 16 ++++++++-------- daiv/accounts/templates/accounts/dashboard.html | 4 ++-- .../templates/sessions/session_detail.html | 4 ++-- 7 files changed, 24 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f9e43e53..ae2ccdaa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,6 +84,8 @@ uv run --all-extras python scripts/dump_schemas.py \ **Code-review detector output** — the `cr-*` detectors defer their `{"findings":[...]}` to `/workspace/tmp/subagent-output/-.json` (via `DeferredOutputMiddleware`, added in `_build_detector_middleware`); the review orchestrator passes those paths to `scripts/findings.py merge` instead of re-typing the JSON. The detector charters are unaware of this — they still just return the structured object. (A detector with no structured response — e.g. one the `LoopBreakerMiddleware` stopped — defers a `.txt` error file instead, which `findings.py merge` counts as a `skipped`/failed detector, never as empty findings.) +**Template comments** — Django's `{# … #}` comment is **single-line only** (the lexer's `tag_re` has no `DOTALL` flag). A `{# … #}` that spans a newline is NOT recognised as a comment: the text renders literally and any `{{ … }}` / `{% … %}` inside it gets evaluated. Use `{% comment %} … {% endcomment %}` for anything multi-line; reserve `{# … #}` for one-liners. + **Icons in templates** — never hand-roll an inline `` for a UI icon. Use `{% load icon_tags %}{% icon "name" "css-classes" %}`; see `DESIGN.md` §Icon System for the mechanism and the icon directory. Exceptions (keep inline): animated spinners, SVGs that need ``/Alpine `:class` on the element itself, and brand/logo `` tags. **Views split by content type** — server-rendered HTML (dashboard pages, forms) lives in `daiv//views.py` as **CBVs** subclassing `View` / `TemplateView` / `ListView` / `UpdateView` with `LoginRequiredMixin` / `AdminRequiredMixin`. JSON endpoints (including those consumed by dashboard JS like autocompletes and the agent-picker catalog) live in `daiv//api/views.py` (or `api/router.py` — both names exist) as a **django-ninja `Router`** with `auth=django_auth` for session callers, registered on the central `NinjaAPI` in `daiv/daiv/api.py` (`api.add_router("/", _router)`). Set `url_name="..."` on each route and reverse via `{% url 'api:' %}` from templates (or pass the URL into JS as an init prop instead of hardcoding `/api/...` paths); see `daiv/automation/api/views.py` + `_agent_picker.html` for the reference pair. diff --git a/daiv/accounts/templates/accounts/_console_body.html b/daiv/accounts/templates/accounts/_console_body.html index 0952b3e1..af6faac8 100644 --- a/daiv/accounts/templates/accounts/_console_body.html +++ b/daiv/accounts/templates/accounts/_console_body.html @@ -9,8 +9,8 @@ {% endcomment %}
{# ── Hero (Story 3.1) ─────────────────────────────────────────────────── #} - {# Range-agnostic section heading: the active range is carried honestly by the Hero headline - itself ("… this week"), so this label can never contradict the selected range. #} + {% comment %} Range-agnostic section heading: the active range is carried honestly by the Hero headline + itself ("… this week"), so this label can never contradict the selected range. {% endcomment %}
@@ -49,8 +49,8 @@

{% translate "what happened" %} - {# Persistent aria-live container: only its inner fragment swaps on ``feed:seen`` so the live - region survives and the change (including to 0) is announced. #} + {% comment %} Persistent aria-live container: only its inner fragment swaps on ``feed:seen`` so the live + region survives and the change (including to 0) is announced. {% endcomment %} {% include "accounts/_feed_unread_badge.html" %} diff --git a/daiv/accounts/templates/accounts/_feed.html b/daiv/accounts/templates/accounts/_feed.html index a3408b7e..64c521ee 100644 --- a/daiv/accounts/templates/accounts/_feed.html +++ b/daiv/accounts/templates/accounts/_feed.html @@ -20,9 +20,9 @@ {% else %} {% include "accounts/_feed_zero_state.html" with zero=feed_zero %} {% endif %} - {# Reconcile freshness (Story 3.3, AC8): the console reconciles against live GitLab/GitHub MR + {% comment %} Reconcile freshness (Story 3.3, AC8): the console reconciles against live GitLab/GitHub MR state at render time, so this "last checked" stamp is the render time. Presentation-only, - mirroring the zero-state audit meta (_feed_zero_state.html). #} + mirroring the zero-state audit meta (_feed_zero_state.html). {% endcomment %} {% if reconciled_at %}

{% blocktranslate with checked=reconciled_at|date:"H:i" %}last checked {{ checked }}{% endblocktranslate %} diff --git a/daiv/accounts/templates/accounts/_feed_item.html b/daiv/accounts/templates/accounts/_feed_item.html index 7a970878..6ffa5f6b 100644 --- a/daiv/accounts/templates/accounts/_feed_item.html +++ b/daiv/accounts/templates/accounts/_feed_item.html @@ -22,9 +22,9 @@ {% if item.status_slug == "classifying" %}aria-live="polite" aria-busy="true"{% endif %} class="flex items-stretch rounded-md border border-l-2 border-border bg-surface-2{% if item.status_slug == 'classifying' %} border-dashed{% endif %}{% if not item.read_at %} bg-white/[0.02]{% endif %}" style="{% if item.status_slug == 'all-clear' %}opacity: 0.92; border-left-color: var(--color-status-clear);{% else %}{% if item.read_at %}opacity: 0.7;{% endif %}{% if item.accent_var %}border-left-color: var({{ item.accent_var }});{% endif %}{% endif %}"> - {# Only an unmodified left-click marks-seen-then-navigates via htmx; ctrl/cmd/shift/middle-click + {% comment %} Only an unmodified left-click marks-seen-then-navigates via htmx; ctrl/cmd/shift/middle-click fall through to the native ``href`` (open in a new tab) WITHOUT marking seen — restoring 2.3's - plain- behaviour that the bare ``hx-post`` (fires on every click) regressed. #} + plain- behaviour that the bare ``hx-post`` (fires on every click) regressed. {% endcomment %} - {# Dismiss (mark-seen) — resolved-only, unread-only. A seen-state control, NOT an Epic 5 action. + {% comment %} Dismiss (mark-seen) — resolved-only, unread-only. A seen-state control, NOT an Epic 5 action. Single accessible name (aria-label); 44px touch target; inherits the global teal focus ring. The swap replaces this

with its seen version (which has NO dismiss button), so focus would fall to ; after the swap, move focus back onto the swapped-in article (same ``#feed-item-{id}``, made focusable by ``tabindex="-1"``) so keyboard/AT users keep their place - (WCAG 2.4.3). The global ``:focus-visible`` ring supplies the visible indicator. #} + (WCAG 2.4.3). The global ``:focus-visible`` ring supplies the visible indicator. {% endcomment %} {% if not item.read_at and item.status_slug != "classifying" %}

- {# Inline breakdown panel — flat surface below the fact (no overlay shadow), reflows on <768px, - re-renders with #console-main on a range change. One panel serves AC1 (rows) + AC2 (how). #} + {% comment %} Inline breakdown panel — flat surface below the fact (no overlay shadow), reflows on <768px, + re-renders with #console-main on a range change. One panel serves AC1 (rows) + AC2 (how). {% endcomment %}
{% include "accounts/_clickthrough.html" with rows=hero.this_range_rows number=hero.this_range.total_merges how_computed=True panel_id="hero-breakdown-range" %}

{# ── All-time odometer (range-invariant) — click-through trigger ─────────── #} - {# Its OWN Alpine scope + panel (range-invariant all-time rows). The odometer element is itself the + {% comment %} Its OWN Alpine scope + panel (range-invariant all-time rows). The odometer element is itself the button (keeps the ``hero-odometer`` testid + the ``.animate-number`` span). ``how_computed`` is - omitted here — the headline panel already carries the AD-10 disclosure. #} + omitted here — the headline panel already carries the AD-10 disclosure. {% endcomment %}