feat(#539): accept agent.id on PATCH /opportunity/{id} to re-link agent#540
Conversation
Resolve agent.id to an existing agent and update opportunity.agentId
instead of mutating the agent record. Legacy { name, address, district }
shape still works for in-place edits; id wins when both are sent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
nadavosa
left a comment
nadavosa
left a comment
There was a problem hiding this comment.
Overall: looks good to merge. Logic is correct, backward-compatible, and well-tested.
One real issue — address and district silently ignored in in-place edit:
The parser only maps agentBody.name → title. The address and district fields exist in OpportunityAgentPatchBody but are never applied to the Agent patch:
```ts
agent: agentBody && agentLinkId === undefined
? ({ title: agentBody.name } as Partial) // address, district dropped
: {},
```
This was the same before this PR, so it is pre-existing — but since we are touching this code it is worth calling out. A coordinator editing the RAC address via the form today would see their change silently lost. Worth a follow-up issue if not fixing here.
Minor observations:
{ agentId: agentLinkId } as Partial<Opportunity>— the cast is needed because TypeORM's partial entity type does not expose FK columns directly. Fine as-is, just worth a comment.- The
OpportunityStatusTypeenum reformatting insdk-types.jsonis cosmetic and harmless. - The local
OpportunityPatchBodyworkaround for the stale SDK type is acceptable short-term — should go into a future SDK update. - All 3 parser test cases are exactly the right ones to have (relink, legacy, id-wins).
Verdict: Approve with the note that the silent address/district drop should be tracked in a follow-up issue. The relink path is correct and production-safe.
| opportunity.id, | ||
| ); | ||
| if (!success) { | ||
| throw new Error("Relinking opportunity agent failed."); |
There was a problem hiding this comment.
throwing new Error returns 500 which is semantically wrong, isn't it? 400 would be more appropriate. wouldn't use custom BadRequestError be more to the point? or maybe NotFoundError?
There was a problem hiding this comment.
Fixed in e020286: missing referenced agent now throws NotFoundError (404), and a failed relink update throws BadRequestError (400). No more generic 500.
| fastify.patch<{ | ||
| Params: ParamsId; | ||
| Body: ApiOpportunityPatch; | ||
| Body: OpportunityPatchBody; |
There was a problem hiding this comment.
I understand that JSON schema used by fastify is the main contract, but SDK lifts shared structures to transpilation phase which is worth bothering IMHO
let's do ApiOpportunityPatch refactoring instead!
There was a problem hiding this comment.
Agreed and done. SDK 0.0.86 adds agent.id (and deprecates name/address/district) on ApiOpportunityPatch. Bumped the dependency, removed the local OpportunityPatchBody workaround, and the route now types its body as ApiOpportunityPatch again.
| opportunity: opportunityObj, | ||
| contact, | ||
| agent, | ||
| agentLinkId, |
There was a problem hiding this comment.
why move agent id outside agent object? wouldn't agent.id do the thing?
There was a problem hiding this comment.
Good call. Removed the hoisted agentLinkId from the parser; the route now reads request.body.agent?.id directly. The parser just suppresses the in-place agent patch when an id is present.
- bump need4deed-sdk to 0.0.86 (agent.id now in ApiOpportunityPatch) - drop local OpportunityPatchBody workaround; route uses ApiOpportunityPatch - read agent.id directly instead of hoisting agentLinkId out of the parser - return NotFoundError when the referenced agent is missing and BadRequestError on relink failure (was a generic 500) - inline the agent id on GET /opportunity/:id so the picker can preselect Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
arturasmckwcz
deleted the
539-accept-agent-reference-agentid-on-patch-opportunityid
branch
3 participants
Closes #539.
Summary
PATCH /opportunity/{id}now accepts{ agent: { id: <existing_id> } }as an explicit re-link reference; the opportunity'sagentIdis updated to point at the existing agent without mutating the agent's name/address/district.{ name, address, district }shape still works for in-place edits (recommendation per the issue: deprecate once the frontend cuts over).agent.idis present, it wins — other fields onagentare ignored./swagger/json) now reflectsidonApiVolunteerOpportunityPatch.agent.Implementation
src/server/schema/sdk-types.json— addedid: integertoApiVolunteerOpportunityPatch.agent.src/services/dto/parser-opportunity-patch-data.ts— new local typesOpportunityAgentPatchBody/OpportunityPatchBody(the SDK type is stale onagentshape); parser surfacesagentLinkIdand skips the in-place agent patch when an id is sent.src/server/routes/opportunity/opportunity.routes.ts— whenagentLinkIdis set, verify the agent exists (400 if not) and updateopportunity.agentId; otherwise fall back to the existing in-place agent patch.src/test/services/dto/parser-opportunity-patch-data.test.ts— 3 new parser tests (relink, legacy in-place edit, id-wins).Test plan
yarn typecheckyarn test:run(161/161 pass)PATCH /opportunity/<id>with{ "agent": { "id": <other_agent_id> } }→ 200; subsequentGETshows the new agent inlined; the previously-linked agent record is untouched.PATCH /opportunity/<id>with{ "agent": { "id": 99999999 } }→ 400Agent (id:99999999) not found.PATCH /opportunity/<id>with the legacy{ "agent": { "name": "..." } }shape still patches the title in place.idfield on the agent body.🤖 Generated with Claude Code