[dev] [carhartlewis] lewis/comp-findings-fixes#2598
Merged
Conversation
The unified_findings migration (20260419120000) mapped every legacy `scope IN (people, people_tasks, people_devices, people_chart)` row onto `area = 'people'` so they stayed queryable. That's misleading in the new model — a finding that used to have scope `people_chart` was about the org chart, not the people directory. Reading the table now makes those rows look like first-class people-area findings. Fix: - Add `'other'` to the `FindingArea` enum (20260419140000) — separate migration because Postgres 12+ disallows using a newly-added enum value in the same transaction it was added. - Reclassify `area='people' AND createdAt < 2026-04-19 12:00:00 UTC` rows to `area='other'` (20260419140001). The cutoff matches the unified_findings migration timestamp: before that point the `area` column didn't exist, so any `area='people'` row pre-dating it can only be a backfill artefact. Genuine `area='people'` findings created after the cutoff are preserved. Prod safety: migrations run back-to-back during a single deploy with no app activity in between, so there is no window for a real `area='people'` finding to be created before the reclass runs. For staging, which has already applied the unified migration, any finding created between then and the reclass deploy will have `createdAt` after the cutoff and will not be touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…eserve origin
Previous version used a hard-coded `createdAt < 2026-04-19 12:00:00`
cutoff to decide which `area='people'` rows were backfilled legacy
rows vs. genuine post-migration people-area findings. That's broken
for prod:
- Prod will keep using the old `FindingScope` code path until the
deploy that runs 20260419120000.
- Findings created on prod between now and that deploy have
`createdAt > cutoff` but are still legacy scope rows.
- Migration 1 backfills them to `area='people'`. Migration 3 would
leave them alone because their `createdAt` is past the cutoff —
wrong.
Switch the signal to the creation AuditLog. The old
finding-audit.service always wrote the source scope into
`data.findingScope` on the creation entry. Presence of that field is
a precise, environment-agnostic marker of "created via the old
scope-based flow". The new service writes `targetKind`/`area` instead,
so genuine post-migration findings won't match and will be left alone.
Also appends a human-readable footer to `content` so the detail sheet
still shows the original scope ("Originally logged against People ›
Devices"). The content field is preserved verbatim and renders in the
existing read-only paragraph for non-auditors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…migration The previous reclass migration (20260419140001) would have moved legacy people-scope findings to area='other' and appended a content footer. That reclass breaks the /people deep-link that owners/admins currently rely on (targetHref returns null for area='other'), so it's worse for operability than leaving rows at area='people'. Do it in the UI instead: - `FindingHistoryEntry.data` typed with optional `findingScope`. - `legacyScopeLabelFromHistory` reads the creation AuditLog entry and maps `findingScope` → "People › Devices" / "Directory" / "Tasks" / "Org chart". - `FindingDetailSheet` renders a muted "Originally logged against …" line below the target label when a legacy scope is present. No DB migration required. The `FindingArea` enum retains `other` for future use / manual categorisation, but no rows are force-reclassified. Legacy rows keep area='people' and their /people link, so owners/admins can still drill in to fix what the auditor found. Drops migration 20260419140001 (never applied). Keeps 20260419140000 (just adds the `other` enum value — safe no-op for existing rows). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ctivity Two issues Paul flagged: 1. "Create Finding" sheet required picking a specific Risk / Vendor / Policy — no way to log "no risks tracked" or "missing policy" when the row doesn't exist yet. Auditors were forced to pick a random target just to get past the "Please select a target" validation. 2. Every finding's activity log showed two "created" entries at the same timestamp — "created a finding" (from the global AuditLogInterceptor) and "created this finding" (from FindingsService.create calling findingAuditService.logFindingCreated). Same for deletions. Fixes: - Extend `FindingArea` enum with `risks` / `vendors` / `policies` (migration 20260419150000). Each new value is its own ALTER TYPE ADD VALUE statement because Postgres 12+ disallows using a newly- added enum value in the same transaction. - Surface the new values in `CreateFindingSheet` as "Risks (general)", "Vendors (general)", "Policies (general)" under the existing "Area" target type. Detail sheet + list row render the label verbatim and link general findings to `/risks`, `/vendors`, `/policies` so the owner/admin can drill in. - Remove the explicit `findingAuditService.logFindingCreated` and `logFindingDeleted` calls from `FindingsService`. The global `AuditLogInterceptor` already writes those via `extractFindingDescription`. Status / type / content-change audits are kept — the interceptor can't derive those deltas. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pickers) - create-finding.dto.ts: add @isnotempty() to every optional target ID (taskId, evidenceSubmissionId, policyId, vendorId, riskId, memberId, deviceId, templateId). Empty strings now get rejected with 400 before reaching resolveTarget. - finding-notifier.service.ts: - normalize evidenceFormType via toExternalEvidenceFormType in findingLabel so email/Novu labels show "board-meeting" instead of the DB enum "board_meeting". - When a finding was created by a platform admin (createdById null, createdByAdminId set), fall back to notifying org owners/admins on ready_for_review transitions so the review isn't silently dropped. - includeAdmins now resolves the primary recipient through db.member.findFirst with deactivated=false + isActive=true so deactivated/ex-members are excluded, matching getOwnersAndAdmins. - FindingDetailSheet: correct risk deep link — route is /risk/:id (singular), not /risks. Covers both specific risk targets and the general "Risks" area. - CreateFindingSheet: new `endpointOverrides` and `disabledTargetKinds` props so the admin surface can point pickers at admin-scoped endpoints and hide target types with no admin equivalent. - admin FindingsTab: supplies admin endpoints for task/policy/vendor/ evidenceFormType and hides risk/member/device until admin equivalents exist. Pickers now show the correct org's data. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tab switches on /people were 700ms-2.2s because the RSC for page.tsx blocked on four sequential awaits (session, currentUserMember, complianceMembers findMany, filterComplianceMembers) before returning the shell. Every query-param change — including switching to Chart or Devices, which don't need compliance data — ran the whole chain. Move the two heavy queries into <TeamMembers>, which is already Suspense-wrapped. The page shell now only waits on session + currentUserMember (needed for tab/permission gating), so tab navigation paints immediately and compliance bookkeeping streams in underneath. Drops the `complianceMemberIds` prop on <TeamMembers> — it already computes the ID list from its own members fetch via filterComplianceMembers, so the page-level query was duplicated work. Also removes the `showEmployeeTasks` gate; the Tasks tab now always renders (empty state handles zero-employee orgs), which avoided the prior need to count employees server-side on every navigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/page.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/page.tsx:55">
P2: The Tasks tab is now unconditionally shown to all organizations. Previously it was hidden when there were no compliance-obligated employees (`employees.length > 0`). If this is intentional, no action needed — but if orgs with zero employees shouldn't see an empty Tasks tab, consider computing the visibility signal here (e.g. a lightweight count query) or deferring it with Suspense.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| showRoleMapping={false} | ||
| roleMappingContent={null} | ||
| showEmployeeTasks={showEmployeeTasks} | ||
| showEmployeeTasks |
Contributor
There was a problem hiding this comment.
P2: The Tasks tab is now unconditionally shown to all organizations. Previously it was hidden when there were no compliance-obligated employees (employees.length > 0). If this is intentional, no action needed — but if orgs with zero employees shouldn't see an empty Tasks tab, consider computing the visibility signal here (e.g. a lightweight count query) or deferring it with Suspense.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/people/page.tsx, line 55:
<comment>The Tasks tab is now unconditionally shown to all organizations. Previously it was hidden when there were no compliance-obligated employees (`employees.length > 0`). If this is intentional, no action needed — but if orgs with zero employees shouldn't see an empty Tasks tab, consider computing the visibility signal here (e.g. a lightweight count query) or deferring it with Suspense.</comment>
<file context>
@@ -62,17 +43,16 @@ export default async function PeoplePage({ params }: { params: Promise<{ orgId:
showRoleMapping={false}
roleMappingContent={null}
- showEmployeeTasks={showEmployeeTasks}
+ showEmployeeTasks
canInviteUsers={canInviteUsers}
canManageMembers={canManageMembers}
</file context>
- Add `evidenceSubmission` to CreateFindingSheet target dropdown so users can actually reach the free-text submission-ID picker the form already supports. - Add `FindingSeverity` to the `@db` jest mock; CreateFindingDto imports it at module load time, so its absence risked breaking module initialization in tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
🎉 This PR is included in version 3.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated pull request to merge lewis/comp-findings-fixes into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Adds general risk/vendor/policy findings, exposes evidence submission as a target, surfaces the original legacy people-scope in Finding details, and removes duplicate create/delete activity entries. Also tightens validation, fixes notifications and risk links, improves admin pickers, and speeds up the People page.
New Features
risks,vendors,policies. Shown inCreateFindingSheet; list/detail label them and link to/risk,/vendors,/policies.CreateFindingSheetadds “Evidence submission” to the target dropdown.FindingDetailSheetshows “Originally logged against …” usingfindingScopefrom the creation AuditLog;FindingHistoryEntry.data.findingScopetyped optional.CreateFindingSheetsupports endpoint overrides and hidden target kinds; adminFindingsTabpoints pickers at org-scoped endpoints and hides kinds without admin APIs.Bug Fixes
@IsNotEmpty()in the create finding DTO./risk/:id.TeamMembersfor faster tab switches; Tasks tab always renders.FindingSeverityto the@dbjest mock to stabilize DTO initialization.Written for commit 320d053. Summary will update on new commits.