townsquare admin stats — show per-connector adoption + query counts

[badoni003shreyansh]

Fixes #4

[SwathiMystery]

Welcome back @badoni003shreyansh — second PR, clean implementation, CI green across all 9 checks. The structure mirrors the issue's mockup, the test covers both empty and populated paths, and the SQLAlchemy aggregations are correct. Nice work.

A few items below. One required, the rest are suggestions that can land in a follow-up PR or here at your discretion.

Required before merge

1. PR body should be Fixes #4, not "Changes #4".

Same fix as PR #6 first round. The current text doesn't auto-close the issue on merge — Linear-style "Fixes #N" is the GitHub keyword that does. Just edit the PR description.

Strongly suggested

2. Don't hardcode the source list.

Lines 246-249 of the new code:

for source in ["gmail", "drive", "calendar", "slack", "github"]:
    count = conn_map.get(source, 0)
    click.echo(f"  {source:<16}{count} users")

Today this works. But we have open issues for Notion (#2), Linear (#7), Confluence (#8), Microsoft 365 (#9) — five new connectors coming. Each will require remembering to update this list, or adoption-stats will silently miss the new connector.

The connector registry already enumerates the source IDs. Use it:

from townsquare.connectors.registry import default_registry

# Show every registered source, even ones with zero connections.
known_sources = sorted(default_registry().keys())
# Plus any source we have rows for that isn't in the registry (defensive).
for source in known_sources + sorted(set(conn_map.keys()) - set(known_sources)):
    count = conn_map.get(source, 0)
    click.echo(f"  {source:<16}{count} users")

That way every new connector PR automatically appears in townsquare admin stats with no follow-up.

Optional nits — your call

3. Style: prefer the select() form used elsewhere in the codebase.

This file currently uses both styles. The existing admin commands use s.execute(select(...)) (modern SQLAlchemy 2.x). The new stats command uses s.query(...) (1.x compat layer). Both work; consistency matters more than which you pick. You can see the pattern in the rest of cli.py (e.g., list_users).

4. getattr defaults are unnecessary on non-nullable columns.

email = getattr(last, "user_email", "—")
query = getattr(last, "query_text", "")

QueryLog.user_email and QueryLog.query_text are both non-nullable in db/models.py. The getattr defaults will never fire. Just last.user_email / last.query_text.

5. The mockup's "2 hours ago" is missing.

The issue spec showed:

LAST QUERY
  alice@…           2 hours ago — "who's working on auth?"

The implementation skips the relative-time. Easy add — humanize is overkill, just inline:

delta = now - last.created_at
mins = int(delta.total_seconds() // 60)
when = f"{mins // 60}h ago" if mins >= 60 else f"{mins}m ago"
click.echo(f"  {last.user_email}  {when} — \"{last.query_text}\"")

Worth adding because the relative-time is the most useful field at a glance.

6. Test fixture: latency_ms=3.5 is misleading.

That seeds 3.5 milliseconds — implausibly fast. Real query latencies are several thousand ms. The test passes because it only asserts the heading appears, not the value. If you want a realistic seed: latency_ms=4200.0 (4.2 seconds, matches what agent/runner.py produces). Cosmetic but helps the next person reading the test.

7. Missing newline at end of tests/test_admin_cli.py.

The diff shows \ No newline at end of file. Most editors auto-add. The repo's CI doesn't lint tests/ (only src/) so it slipped through, but POSIX tools and other linters expect a trailing newline.

8. Two commits with identical messages.

Both commits are titled "townsquare admin stats — show per-connector adoption + query counts". The repo is squash-merge-only so this disappears at merge time. Just FYI for next time — squash locally before pushing keeps the PR history readable to reviewers.

What's working

• CI green across the full matrix (9/9 checks)
• The test covers both empty and populated DB paths
• SQLAlchemy aggregations (count + avg + sum in one query) are correct
• Output format follows the issue mockup
• The (or 0) / (or 0.0) defensive coalesce on aggregates is correct — func.sum returns None on empty tables
• Output is readable and useful

Bottom line

Required: just #1 (PR body → Fixes #4).
Strongly suggested: #2 (dynamic source list).
Rest: at your discretion in this PR or a follow-up.

Once #1 + #2 land I'm ready to approve. Items #3-#8 don't block — if you'd rather ship now and iterate, that's also fine.

[SwathiMystery]

LGTM, approving. Comprehensive turnaround on the round-1 review — went well beyond the bar.

What you addressed

✅ #1 PR body — now Fixes #4. Issue auto-closes on merge.

✅ #2 dynamic source list — switched to default_registry().keys(), AND added the defensive extra_sources handling for any DB rows whose source isn't in the registry. Better than what I suggested:

known_sources = sorted(default_registry().keys())
extra_sources = sorted(set(conn_map.keys()) - set(known_sources))
for source in known_sources + extra_sources:
    ...

✅ #3 SQLAlchemy style — all four queries converted to s.execute(select(...)) form. Consistent with the rest of cli.py now.

✅ #4 getattr defaults — replaced with direct attribute access last.user_email / last.query_text.

✅ #5 relative-time output — added cleanly:

delta = now - last.created_at
mins = int(delta.total_seconds() // 60)
when = f"{mins // 60}h ago" if mins >= 60 else f"{mins}m ago"

✅ #6 test fixture — latency_ms=4200.0 instead of 3.5. Realistic, matches the agent runner's actual values.

Two minor cosmetic items (not blocking, optional follow-up)

These slipped through because the repo's CI lints src/ only, not tests/. If you want to clean them up before merge, fine; if not, that's also fine since CI is green:

• Trailing whitespace on the last line of tests/test_admin_cli.py (the + empty-spaces-only line at the end)
• No newline at end of file still on tests/test_admin_cli.py (line \ No newline at end of file in the diff)

I'm going to add an issue to broaden the lint scope to tests/ so future contributors don't hit this blind spot. Not your problem to fix.

CI

✅ All 9 checks passing (lint + 6 test matrices + CodeQL + analyze (python)).

Approving — ready for squash-merge

Branch protection requires the approval to be from a maintainer, which is happening here. Once this lands, issue #4 auto-closes via Fixes #4. Three of the four oldest pinned good first issues are now done (#3 ✓ examples, #4 ✓ admin stats, #6 ✓ — leaves #2 Notion as the longest-pinned without a taker).

Thank you for the careful follow-through. Two clean PRs in 5 days is a real contributor pace. If you're up for a third, the most-needed issues right now are #13 USD budget enforcement (operationally critical for any real deployment) or #7 Linear connector (closest to PR #6 and PR #16's shape).