feat: filter deleted member enrchiment worker (CM-1367) - #4476
Conversation
PR SummaryMedium Risk Overview Adds
Reviewed by Cursor Bugbot for commit 7bbf97e. Bugbot is set up for automated code reviews on this repo. Configure here. |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
572faa1 to
d21e705
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d21e705. Configure here.
There was a problem hiding this comment.
Pull request overview
Refactors member work-experience enrichment to preserve verified affiliations and human-deletion tombstones.
Changes:
- Adds
deletedBytombstones and wires human deletion paths. - Reconciles affiliations in place instead of recreating them.
- Adds Vitest coverage for reconciliation behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/types/src/enrichment.ts |
Extends enrichment affiliation types. |
services/libs/data-access-layer/src/old/apps/members_enrichment_worker/index.ts |
Fetches verification and tombstone data. |
services/libs/data-access-layer/src/members/organizations.ts |
Records deletion actors and update timestamps. |
services/apps/members_enrichment_worker/vitest.config.ts |
Configures Vitest. |
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts |
Implements in-place reconciliation. |
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.test.ts |
Tests reconciliation scenarios. |
services/apps/members_enrichment_worker/src/activities/enrichment.ts |
Integrates reconciliation into enrichment. |
services/apps/members_enrichment_worker/package.json |
Adds the test command and Vitest. |
pnpm-lock.yaml |
Locks the Vitest dependency. |
backend/src/services/member/memberOrganizationsService.ts |
Tombstones UI deletions. |
backend/src/database/migrations/V1786621970__add-deleted-by-to-member-organizations.sql |
Adds the deletedBy column. |
backend/src/api/public/v1/members/work-experiences/verifyMemberWorkExperience.ts |
Tombstones rejected work experiences. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (9)
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:127
- This checks
identity.organizationId, but the enrichment flow resolves an existing organization by setting onlyentry.organizationId(enrichment.ts:421-430). Those identity IDs can remain unset, so a provider row with changed dates bypasses this filter and is inserted beside the verified row. Match the resolved entry organization ID directly.
!verifiedRows.some(
(v) =>
normalizeTitle(v.jobTitle) === normalizeTitle(e.title) &&
e.identities &&
e.identities.some((i) => i.organizationId === v.orgId),
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:139
- This UI conflict check has the same mismatch: existing-org resolution populates
e.organizationIdbut does not populate each identity'sorganizationId. In that normal path, a high-confidence provider entry is not recognized as the existing UI experience and can create a duplicate with different dates.
!uiEntries.some(
(ui) =>
e.title === ui.jobTitle &&
e.identities &&
e.identities.some((i) => i.organizationId === ui.orgId),
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:184
- Existing-organization matching leaves identity-level organization IDs unset, so this lookup can miss a resolved UI row and retain the provider entry. Compare the entry's resolved
organizationId; otherwise the normal-confidence branch can insert a conflicting duplicate instead of suppressing it.
const match = orderedNewVersion.find(
(e) =>
e.title === current.jobTitle &&
e.identities &&
e.identities.some((e) => e.organizationId === current.orgId),
)
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:115
- This comment restates the tombstone filter directly below it. The condition is self-explanatory, so the narration should be removed.
// never recreate an affiliation that a person deleted on purpose — providers keep resupplying it
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:119
- This comment narrates why the following filter exists rather than expressing the rule in the code. Remove it; the verified-row selection and conflict predicate should carry the behavior.
// verified rows are excluded from oldEnrichmentRows above, so a matching provider entry
// must be dropped here too, or it lands in toCreate as a conflicting duplicate
services/libs/data-access-layer/src/old/apps/members_enrichment_worker/index.ts:48
- Selecting
verifiedhere does not protect affiliations whose organization has no identities, because the CTE still inner-joinsorganizationIdentities. Organizations can be created from a name alone, so such verified rows disappear fromoldVersionand a changed-date provider row can be inserted beside them. Use a left join so every active member-organization row is reconciled.
mo.verified,
mo."verifiedBy",
services/apps/members_enrichment_worker/src/activities/enrichment.ts:614
- Title and source changes can alter the affiliation timeline even when dates stay fixed: timeline preparation excludes titles such as
Investor/Mentor/Board Memberand ranks overlapping rows by source (member-organization-affiliation/index.ts:113-123,300-307). Suppressing refreshes for those updates can leaveactivityRelationsassigned using stale eligibility or precedence.
const toUpdateHasTimelineChange = Array.from(results.toUpdate.values()).some(
(fields) => 'dateStart' in fields || 'dateEnd' in fields,
)
services/libs/data-access-layer/src/members/organizations.ts:521
- The optional parameter is tested by truthiness, but the public unverify schema accepts an empty string. Passing
deletedBy: ''therefore performs the delete without recording the tombstone, allowing enrichment to recreate the affiliation. Distinguish omission from an explicitly supplied value.
? deletedBy
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:107
- These lines describe the filter immediately below and do not document an external quirk or a caller-facing invariant. Remove the explanatory comment and let the source predicates express the protected-row rule.
This issue also appears in the following locations of the same file:
- line 115
- line 118
// UI and project-registry rows are manual input; a verified row is a human decision too —
// the worker never deletes or updates any of them.
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:127
- This check relies on identity objects carrying
organizationId, but production-normalized provider identities do not set it when the organization is matched throughorg.organizationId(seeenrichment.ts:421-430). Consequently, a provider entry with changed dates is not filtered and an unverified duplicate can be inserted alongside the verified row. Match the resolved organization ID directly.
!verifiedRows.some(
(v) =>
normalizeTitle(v.jobTitle) === normalizeTitle(e.title) &&
e.identities &&
e.identities.some((i) => i.organizationId === v.orgId),
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:139
- Production provider identities are not stamped with
organizationIdwhen an existing organization is resolved (enrichment.ts:421-430), so this high-confidence filter misses matching UI rows. If the provider dates differ, reconciliation creates an enrichment-owned duplicate instead of preserving the UI-only experience. Compare the already-resolvede.organizationIddirectly.
!uiEntries.some(
(ui) =>
e.title === ui.jobTitle &&
e.identities &&
e.identities.some((i) => i.organizationId === ui.orgId),
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:183
- As in the high-confidence branch, real normalized identities can lack
organizationIdeven aftere.organizationIdhas been resolved. This prevents matching the provider entry to its UI row and can create a second affiliation when dates differ. Use the resolved organization ID for this comparison.
const match = orderedNewVersion.find(
(e) =>
e.title === current.jobTitle &&
e.identities &&
e.identities.some((e) => e.organizationId === current.orgId),
services/libs/data-access-layer/src/members/organizations.ts:521
- The API schema accepts an empty
verifiedBystring, but this truthiness check treats it as if no actor was supplied. Such an unverify operation is soft-deleted without a tombstone and enrichment can recreate it on the next run. Distinguish omission from an explicitly supplied string.
? deletedBy
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:20
- This JSDoc restates the function's implementation and violates the repository guideline that code should be self-explanatory without descriptive comments. The function name and tuple construction already convey this behavior.
/**
* Returns true when the set of (orgId, startDate, endDate) tuples differs
* between deletes and creates. Fields like title or source don't affect
* the affiliation timeline, so they're intentionally ignored.
*/
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
services/apps/members_enrichment_worker/src/activities/workExperienceReconciliation.ts:133
- Entries matched to an existing organization only get
entry.organizationIdpopulated; their identity array may remain empty. In that supported case this predicate misses the verified row, so a provider stint with different dates can be inserted as an unverified duplicate. Match the already-resolved organization ID directly.
!verifiedRows.some(
(v) =>
normalizeTitle(v.jobTitle) === normalizeTitle(e.title) &&
e.identities &&
e.identities.some((i) => i.organizationId === v.orgId),
services/libs/data-access-layer/src/members/organizations.ts:522
- The verification endpoint accepts any
z.string(), including''. With this truthiness check, such an explicit human delete omitsdeletedByand enrichment can recreate the affiliation. Distinguish an omitted actor from an empty value (or reject empty actors at validation).
? deletedBy
? 'UPDATE "memberOrganizations" SET "deletedAt" = NOW(), "deletedBy" = $(deletedBy)'
| if (Object.keys(toUpdateInner).length > 0) { | ||
| toUpdate.set(match, toUpdateInner) | ||
| } |
Summary
Refactors
members_enrichment_worker's org-affiliation reconciliation from delete-and-recreate to update-in-place. Today, every enrichment run deletes every non-UI/non-project-registrymemberOrganizationsrow and recreates whatever the provider supplies, which causes two bugs: (1) an org a human manually deleted because it was wrong (e.g. the "Dusky'z" case — OpenDaylight/Apache/Karaf contributions wrongly attributed to a non-existent company instead of PANTHEON.tech) gets resupplied and recreated by the provider on the next run, and (2)verified/verifiedBystatus is lost every time, since the recreated row is always unverified. This PR makes enrichment update rows in place, so verification and human deletes survive reruns.Changes
"deletedBy"column on"memberOrganizations"(migration, additive, no backfill). Set only on human-initiated deletes;NULLmeans the delete was enrichment's own rebuild and stays recoverable (protects against provider-glitch data loss).deleteMemberOrganizations()(data-access-layer) takes an optionaldeletedBy, wired through the human-delete paths:verifyMemberWorkExperience.ts(unverify) andmemberOrganizationsService.ts#delete(CDP UI org delete). The enrichment worker's own delete path (deleteMemberOrgById) is untouched — it must keep leavingdeletedByNULL.fetchMemberDataForLLMSquashing's tombstone CTE now keys off"deletedAt" IS NOT NULL AND "deletedBy" IS NOT NULLinstead of guessing fromsource; also selectsverified/"verifiedBy"so the worker can protect verified rows.workExperienceReconciliation.tsfromenrichment.ts(pure, unit-testable).prepareWorkExperiencesnow reconciles old vs. new orgs by(organizationId, normalized title): matched rows are updated in place (only the fields that changed), unmatched old rows are soft-deleted (deletedByNULL, recoverable), unmatched new entries are inserted, and orgs withdeletedByset are never recreated. UI, project-registry, andverified = truerows are never touched.affiliationNeedsRefreshnarrowed to fire only on an actual date change, not a cosmetic title-only update — cuts down noise onactivityRelationsrefreshes.updateMemberOrgnow sets"updatedAt" = now()on every update.prepareWorkExperiences/hasMemberOrganizationTimelineChange(14 cases): verified-row protection, tombstone respect, provider-glitch recoverability, no writes on unchanged payload, UI-row conflict handling in both the normal and high-confidence branches.Verified end-to-end locally, not just unit tests: seeded a member with a verified affiliation, a manually-tombstoned org, and a provider-soft-deleted (non-tombstoned) org, then ran the real
processMemberSourcesTemporal workflow against it. Result matched all three expectations — verified row untouched, tombstoned org never recreated, provider-glitch org recreated as a new active row.Known gap, intentionally out of scope — tracked in a separate ticket: the public API v1
DELETE /members/:memberId/work-experiences/:workExperienceIdhas no actor field (it's an M2M token, soreq.actorcarries the calling LFXOne service's client id, not the acting human). Deletes through that endpoint won't be tombstoned yet. Confirmed with Nathan and Joana to handle this separately since it needs LFXOne coordination on the payload contract.Type of change
JIRA ticket
CM-1367