chore: implemented patch HTTP method for member project affiliations (CM-1041)#3912
chore: implemented patch HTTP method for member project affiliations (CM-1041)#3912
Conversation
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Adds a new public API endpoint to partially update a member’s per-project (segment) affiliation, including DAL helpers for fetching/updating the underlying memberSegmentAffiliations rows and making organization fields nullable.
Changes:
- Made
organizationId/organizationNamenullable inISegmentAffiliationWithOrg. - Added DAL helpers to fetch a single member+project affiliation and partially update it.
- Added
PATCH /:memberId/project-affiliations/:projectIdpublic API route + handler.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| services/libs/data-access-layer/src/members/projectAffiliations.ts | Adds single-affiliation fetch + partial update helpers; adjusts affiliation-with-org typing for null orgs. |
| backend/src/api/public/v1/members/project-affiliations/patchProjectAffiliation.ts | New PATCH handler to update project affiliation fields and trigger affiliation recalculation. |
| backend/src/api/public/v1/members/index.ts | Wires the new PATCH route into the public v1 members router. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface ISegmentAffiliationWithOrg { | ||
| id: string | ||
| segmentId: string | ||
| organizationId: string | ||
| organizationName: string | ||
| organizationId: string | null | ||
| organizationName: string | null | ||
| organizationLogo: string | null | ||
| verified: boolean |
There was a problem hiding this comment.
ISegmentAffiliationWithOrg now allows organizationId/organizationName to be null, but fetchMemberSegmentAffiliationsWithOrg() still does an INNER JOIN on organizations, which will silently drop affiliations where organizationId is NULL (or the org row is missing). That can cause getProjectAffiliations to incorrectly fall back to work experiences even when a manual segment affiliation exists. Consider switching that query to LEFT JOIN organizations (like the single-fetch does) so null-org overrides are returned consistently.
| export async function fetchMemberSegmentAffiliationForProject( | ||
| qx: QueryExecutor, | ||
| memberId: string, | ||
| segmentId: string, | ||
| ): Promise<ISegmentAffiliationWithOrg | null> { | ||
| const rows = await qx.select( | ||
| ` | ||
| SELECT | ||
| msa.id, | ||
| msa."segmentId", | ||
| msa."organizationId", | ||
| o."displayName" AS "organizationName", | ||
| o.logo AS "organizationLogo", | ||
| msa.verified, | ||
| msa."verifiedBy", | ||
| msa."dateStart", | ||
| msa."dateEnd" | ||
| FROM "memberSegmentAffiliations" msa | ||
| LEFT JOIN organizations o ON msa."organizationId" = o.id | ||
| WHERE msa."memberId" = $(memberId) | ||
| AND msa."segmentId" = $(segmentId) | ||
| `, | ||
| { memberId, segmentId }, | ||
| ) | ||
| return rows[0] ?? null |
There was a problem hiding this comment.
fetchMemberSegmentAffiliationForProject returns rows[0] without any ORDER BY/LIMIT. Since the unique constraint on (memberId, segmentId) was dropped when date ranges were introduced (see migration V1691658076), there can be multiple affiliations per member+segment. This function can therefore return an arbitrary row depending on query plan. Either reintroduce uniqueness, or make the selection deterministic (ORDER BY + LIMIT 1) and/or fetch by affiliation id instead of segmentId.
| if ('organizationId' in data) { | ||
| sets.push('"organizationId" = $(organizationId)') | ||
| params.organizationId = data.organizationId | ||
| } | ||
| if ('dateStart' in data) { | ||
| sets.push('"dateStart" = $(dateStart)') | ||
| params.dateStart = data.dateStart | ||
| } | ||
| if ('dateEnd' in data) { | ||
| sets.push('"dateEnd" = $(dateEnd)') | ||
| params.dateEnd = data.dateEnd | ||
| } | ||
| if ('verified' in data) { | ||
| sets.push('"verified" = $(verified)') | ||
| params.verified = data.verified | ||
| } | ||
| if ('verifiedBy' in data) { |
There was a problem hiding this comment.
The partial-update builder uses 'field' in data checks. If a caller passes an object with an explicit undefined value (e.g. { verifiedBy: undefined }), pg-promise formatting will receive an undefined parameter and can throw at runtime. Prefer checking data.field !== undefined for each optional field, and use null explicitly when the intention is to clear a column.
| if ('organizationId' in data) { | |
| sets.push('"organizationId" = $(organizationId)') | |
| params.organizationId = data.organizationId | |
| } | |
| if ('dateStart' in data) { | |
| sets.push('"dateStart" = $(dateStart)') | |
| params.dateStart = data.dateStart | |
| } | |
| if ('dateEnd' in data) { | |
| sets.push('"dateEnd" = $(dateEnd)') | |
| params.dateEnd = data.dateEnd | |
| } | |
| if ('verified' in data) { | |
| sets.push('"verified" = $(verified)') | |
| params.verified = data.verified | |
| } | |
| if ('verifiedBy' in data) { | |
| if (data.organizationId !== undefined) { | |
| sets.push('"organizationId" = $(organizationId)') | |
| params.organizationId = data.organizationId | |
| } | |
| if (data.dateStart !== undefined) { | |
| sets.push('"dateStart" = $(dateStart)') | |
| params.dateStart = data.dateStart | |
| } | |
| if (data.dateEnd !== undefined) { | |
| sets.push('"dateEnd" = $(dateEnd)') | |
| params.dateEnd = data.dateEnd | |
| } | |
| if (data.verified !== undefined) { | |
| sets.push('"verified" = $(verified)') | |
| params.verified = data.verified | |
| } | |
| if (data.verifiedBy !== undefined) { |
| await qx.result( | ||
| ` | ||
| UPDATE "memberSegmentAffiliations" | ||
| SET ${sets.join(', ')} | ||
| WHERE "memberId" = $(memberId) | ||
| AND "segmentId" = $(segmentId) | ||
| `, |
There was a problem hiding this comment.
updateMemberSegmentAffiliation updates rows by (memberId, segmentId). If there are multiple memberSegmentAffiliations rows for the same segment (which is possible after the unique constraint was dropped for date ranges), this UPDATE will modify all of them. If the intent is to patch a single affiliation, update by primary key id (and make the API accept that id). If the intent is bulk-update, the API/response should reflect that (and you should return/verify affected row count).
| export interface ISegmentAffiliationUpdate { | ||
| organizationId?: string | ||
| dateStart?: string | null | ||
| dateEnd?: string | null | ||
| verified?: boolean | ||
| verifiedBy?: string | null | ||
| } |
There was a problem hiding this comment.
ISegmentAffiliationUpdate.organizationId is typed as string (not nullable), but the underlying column is nullable and ISegmentAffiliationWithOrg.organizationId is now string | null. If clearing an affiliation is a supported operation, organizationId should allow null here so callers can explicitly unset it (and the API schema should accept null accordingly).
| const existing = await fetchMemberSegmentAffiliationForProject(qx, memberId, projectId) | ||
|
|
||
| if (!existing) { | ||
| throw new NotFoundError('Project affiliation not found') | ||
| } |
There was a problem hiding this comment.
This endpoint treats (memberId, projectId/segmentId) as identifying a single affiliation, but the DB model supports multiple memberSegmentAffiliations rows per segment (date ranges removed the unique constraint). As written, the code will update potentially multiple rows and then re-fetch an arbitrary one to return. Consider changing the route to patch by affiliation id (or adding enough filters to uniquely identify a record) and making the DAL update/fetch operate on that id.
| if (organizationId) { | ||
| const service = new CommonMemberService(tx, req.temporal, req.log) | ||
| await service.startAffiliationRecalculation(memberId, [organizationId]) | ||
| } |
There was a problem hiding this comment.
Affiliation recalculation is only started when organizationId is truthy. If an existing project affiliation has organizationId = null (supported by the schema), patches to dateStart/dateEnd/verified/verifiedBy will not trigger recalculation and activity affiliations can stay stale. Since the workflow recalculates by memberId anyway, consider starting recalculation unconditionally after a successful update (you can pass an empty organizationIds array when there is no org).
| if (organizationId) { | |
| const service = new CommonMemberService(tx, req.temporal, req.log) | |
| await service.startAffiliationRecalculation(memberId, [organizationId]) | |
| } | |
| const organizationIds = organizationId ? [organizationId] : [] | |
| const service = new CommonMemberService(tx, req.temporal, req.log) | |
| await service.startAffiliationRecalculation(memberId, organizationIds) |
Note
Medium Risk
Adds a new public write endpoint that updates
memberSegmentAffiliationsand triggers affiliation recalculation/audit logging, so mistakes could impact member-organization attribution and downstream computations. Scope is contained to the project-affiliations area with input validation and not-found checks.Overview
Adds a new
PATCH /:memberId/project-affiliations/:projectIdpublic API to partially update a member’s project affiliation (org, dates, verification fields) with zod validation, not-found handling, and audit-log capture.Extends the data-access layer to support this flow by making org fields nullable in
ISegmentAffiliationWithOrg, addingfetchMemberSegmentAffiliationForProject, and addingupdateMemberSegmentAffiliationfor partial SQL updates (includingLEFT JOINsupport when org is missing).Written by Cursor Bugbot for commit 6db1b26. This will update automatically on new commits. Configure here.