Make TeamResourceService sole owner of google_resources (#491)

[peterdrier]

Summary

Phase 1 of consolidating google_resources ownership under TeamResourceService, per .claude/DESIGN_RULES.md. Consumers stop touching DbSet<GoogleResource> directly and go through ITeamResourceService instead.

• Add 7 read methods to ITeamResourceService: GetResourcesByTeamIdsAsync, GetTeamResourceSummariesAsync, GetActiveResourceCountsByTeamAsync, GetUserTeamResourcesAsync, GetActiveDriveFoldersAsync, GetResourceCountAsync, plus existing GetTeamResourcesAsync / GetResourceByIdAsync.
• Migrate callers: HumansMetricsService, DriveActivityMonitorService, GoogleAdminService, and TeamService (email-send read, admin-list .Include, CreateAdminTeamSummary counts, and the relocated GetUserTeamGoogleResourcesAsync — now on ITeamResourceService and consumed directly by MyGoogleResourcesViewComponent).
• Delete TeamResourcePersistence (static helper is gone; each impl does its own DbContext access).
• Remove Team.GoogleResources EF navigation property and configure the relationship from the GoogleResource side only. Model snapshot updated; dotnet ef migrations has-pending-model-changes confirms no schema diff (this PR is db:no).
• TeamService ↔ TeamResourceService now have a mutual read-path dependency (GetUserTeamResourcesAsync needs GetUserTeamsAsync); resolved lazily through IServiceProvider on both sides to avoid the DI cycle.
• Guardrail: scripts/check-google-resource-ownership.sh (wired into the code-quality CI job) fails if anything outside TeamResourceService/StubTeamResourceService/TeamConfiguration/HumansDbContext touches DbSet<GoogleResource>. GoogleWorkspaceSyncService, SystemTeamSyncJob, ProcessGoogleSyncOutboxJob, and GoogleController stay on the Phase 2 exception list.

Phase 2 — decomposing GoogleWorkspaceSyncService's own writes/reads to the table, the return-tuple pattern for Google state, and removing the remaining exception-list entries — is out of scope for this PR and should get its own issue.

Test plan

• dotnet build Humans.slnx — clean
• dotnet test Humans.slnx (non-integration) — 1055 passing, 0 failing
• dotnet format Humans.slnx --verify-no-changes — clean
• dotnet ef migrations has-pending-model-changes — no changes
• bash scripts/check-google-resource-ownership.sh — passes locally
• CI green on the PR
• QA smoke: dashboard "My Google Resources" widget still renders; /Admin/Teams admin list still shows correct HasMailGroup / DriveResourceCount per team; added-to-team email still lists the team's drive resources.

Closes nobodies-collective#491 (tracked upstream on nobodies-collective/Humans; auto-close will fire when this batch promotes to upstream main).

🤖 Generated with Claude Code

[peterdrier]

PR Review — 2026-04-14

PR: #225 — Make TeamResourceService sole owner of google_resources (nobodies-collective#491)
Branch: fix/491-teamresource-owns-google-resources → main
Scope: 21 files, 510 insertions / 145 deletions, 1 commit

---

Spec Compliance — Issue nobodies-collective#491

Verdict: SUBSTANTIALLY ADDRESSED (7 of 8 criteria met)

#	Criterion	Verdict
1	All 7 new read methods on ITeamResourceService	FULLY ADDRESSED (with naming substitution)
2	TeamResourcePersistence deleted	FULLY ADDRESSED
3	Consumer services no longer touch DbSet<GoogleResource>	FULLY ADDRESSED
4	Team.GoogleResources nav removed; no .Include remains	FULLY ADDRESSED
5	Grep returns only expected hits	FULLY ADDRESSED
6	Guardrail in place	FULLY ADDRESSED (regex limitation noted)
7	Tests pass, no behavior changes	LIKELY ADDRESSED
8	Phase 2 tracked in separate issue with link back	NOT ADDRESSED

Notes:

• Issue named GetActiveResourcesWithTeamAsync; PR shipped GetActiveResourceCountsByTeamAsync (IReadOnlyDictionary<Guid,int>). The narrower count-only shape matches the actual consumer (GoogleAdminService.cs:447) — acceptable substitution.
• The issue body claimed GoogleController was "clean"; it is not (GoogleController.cs:739). The PR correctly added it to the Phase 2 exception list rather than silently leaving it broken.
• The DI cycle resolution (mutual lazy IServiceProvider.GetRequiredService<T>() on both sides) is correct and safe for scoped services.
• Gap: No GitHub issue exists yet for Phase 2 (decomposing GoogleWorkspaceSyncService writes). Should be filed before/at merge so the work is tracked.

---

Code Quality

No CRITICAL issues.

IMPORTANT:

1. OnDelete(DeleteBehavior.SetNull) on non-nullable Guid TeamId — TeamConfiguration.cs:116 / GoogleResource.cs:39. Pre-existing latent bug, but this PR touches the exact configuration line. SetNull on a non-nullable FK column will throw a Postgres FK violation if a Team is ever hard-deleted. Fix to DeleteBehavior.Restrict (or Cascade) while here.

2. GetUserTeamResourcesAsync ordering change — TeamResourceService.cs:143-185. Old single-JOIN query ordered by Postgres collation; new two-step impl re-sorts in memory with StringComparer.OrdinalIgnoreCase. Behavior change for accented names (e.g. "Équipo"). Edge case at 500-user scale, but undocumented.

3. GetResourceCountAsync missing AsNoTracking — TeamResourceService.cs:196 and StubTeamResourceService.cs:178. Inconsistent with every other new read method.

4. GetUserTeamResourcesAsync silently drops null Team — TeamResourceService.cs:154-158. The .Where(t => t is not null) masks data integrity issues. Either remove (fail loudly) or log a warning before filtering.

5. Guardrail regex is variable-name-bound — scripts/check-google-resource-ownership.sh:51. Catches _dbContext|dbContext|db|_db only. A future ctx/context/humansDb would slip through. Also doesn't catch context.Set<GoogleResource>(). Acceptable as a first line, but document the limitation.

MINOR:

6. Lazy IServiceProvider pattern resolves on every property access — minor smell. Lazy<T> initialized in constructor would be slightly more idiomatic. Not required.
7. Zero direct test coverage for the 6 new ITeamResourceService read methods. Tests only stub them with empty dicts; no behavior tests.
8. EnsureTeamGroupAsync in GoogleWorkspaceSyncService now does 2 queries instead of 1 (Phase 2 file, intentional, irrelevant at scale).
9. GetActiveDriveFoldersAsync lacks OrderBy — fine for anomaly detection, inconsistent with sibling methods.
10. UserTeamGoogleResource record moved between interface files in the same namespace — harmless.

What looks good:

• Lazy DI cycle resolution is correct for scoped services.
• All new read methods (except GetResourceCountAsync) use AsNoTracking() and efficient projections (GetTeamResourceSummariesAsync projects to anonymous type).
• StubTeamResourceService faithfully mirrors the real service, including the UnlinkResourceAsync SaveChangesAsync fix vs. the old persistence helper.
• TeamConfiguration change to HasMany<GoogleResource>() is the correct pattern for a relationship without a principal-side nav. Snapshot confirms zero schema diff.
• AdminTeamList behavior is preserved exactly (HasMailGroup / DriveResourceCount semantics match).
• Guardrail script is wired into build.yml CI and currently passes against HEAD.
• TeamResourcePersistence deletion removes a layer of indirection cleanly.

---

Bottom Line

Well-executed Phase 1. Architecture intent is correct, EF patterns are sound, and the CI guardrail is a meaningful regression gate. The actionable items before merge are: (a) file the Phase 2 tracking issue (criterion #8); (b) decide whether to fix the pre-existing OnDelete(SetNull) latent bug while you're touching that configuration line; (c) add AsNoTracking to GetResourceCountAsync. Items (b) and (c) are quick. Nothing blocks merging.

---

Review by Claude Code /pr-review

[peterdrier]

Fixes Applied — 2026-04-14

Commit: 8ecf6b1

Addressed review findings 1–4:

1. OnDelete(SetNull) on non-nullable FK → changed to DeleteBehavior.Restrict. GoogleResource.TeamId is Guid (non-nullable), so the previous SetNull would have thrown a Postgres NOT NULL violation if a team were ever hard-deleted. Restrict is the correct behavior: if resources exist, deletion is blocked and the caller must unlink first. Added migration 20260414215135_RestrictGoogleResourceTeamDelete — this PR is now db:yes (was db:no). EF migration reviewer gate ran clean.

2. GetUserTeamResourcesAsync ordering + behavior change → rewritten as a single DB-sorted JOIN in both TeamResourceService and StubTeamResourceService. Restores the pre-PR query shape exactly (orderby t.Name, r.Name in SQL, using Postgres collation). No more in-memory OrdinalIgnoreCase sort — matches old behavior for accented names.

3. GetResourceCountAsync missing AsNoTracking → added in both services, consistent with every other read method.

4. Silent null-Team filter in GetUserTeamResourcesAsync → removed entirely. The JOIN guarantees Team is present, so no guard is needed. As a bonus, this eliminates the service-locator hop through ITeamService for this code path (the DI cycle is still required for CanManageTeamResourcesAsync elsewhere).

Also filed: Phase 2 tracking issue at nobodies-collective#492, covering the remaining GoogleWorkspaceSyncService / SystemTeamSyncJob / ProcessGoogleSyncOutboxJob / GoogleController direct writes — closes the last open acceptance criterion from nobodies-collective#491.

Verification:

• dotnet build Humans.slnx — clean, 0 warnings
• dotnet test Humans.slnx (non-integration) — 1055 passing, 0 failing
• dotnet format Humans.slnx --verify-no-changes — clean
• dotnet ef migrations has-pending-model-changes — no changes
• bash scripts/check-google-resource-ownership.sh — passes
• EF migration reviewer gate — no CRITICAL issues

Not addressed (minor, deferred):

• Test preview deploy #5 (guardrail regex is variable-name-bound) — documented limitation; fine for first-line defense.
• Minor feat: add in-app feedback system with admin triage and API #6–10 — not requested in this fix pass.

---

Fix & re-review by Claude Code /pr-review