feat(SITES-44690): add task-management controller and POST /tickets route#2661
feat(SITES-44690): add task-management controller and POST /tickets route#2661prithipalpatwal wants to merge 25 commits into
Conversation
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @prithipalpatwal,
Verdict: Request changes - CI-breaking test mock issues plus a batch-mode bug that silently burns API calls after auth failure.
Complexity: HIGH - large diff; API surface + dependency signal.
Changes: adds a task-management controller with connection CRUD, ticket creation (single/batch/grouped), idempotency enforcement, and Jira project listing (8 files).
Note: CI checks are currently failing - resolve before merge.
Must fix before merge
- [Critical] New route params
:connectionIdand:providernot classified infacs-capabilities.js- likely CI failure cause -src/routes/facs-capabilities.js(details inline) - [Critical]
listProjectsmissing from mock controller in route tests -test/routes/index.test.js:548(details inline) - [Critical]
Suggestion: {}in index test mock causes controller constructor to throw -test/index.test.js:176(details inline) - [Important] Individual batch mode continues calling Jira API after 401 auth failure -
src/controllers/task-management.js:823(details inline) - [Important] No integration tests in
test/it/for new endpoints - repo CLAUDE.md explicitly requires these - [Important] Batch idempotency marks "completed" even when all items fail, blocking retries -
src/controllers/task-management.js:942
Non-blocking (6): minor issues and suggestions
- nit:
deleteConnectionreturnscreateResponse({}, STATUS_NO_CONTENT)which sends a body with 204 -src/controllers/task-management.js:374 - suggestion: consider short-circuiting
listTicketsbridge loading into a single query rather than N+1TicketSuggestion.allByTicketIdcalls per ticket -src/controllers/task-management.js:402 - suggestion: add
log.warninside the barecatch {}blocks inlistTicketsandlistTicketsByOpportunity- silent failure with zero observability -src/controllers/task-management.js:411 - suggestion: the controller at 1265 lines is the largest in the repo; consider extracting the three
createTicketcode paths into helper functions - nit: no OpenAPI spec in
docs/openapi/for the new endpoints - the repo's adding-an-endpoint checklist lists this as step 1 - suggestion: include orphaned Jira ticket metadata (ticketKey, ticketUrl) in the 500 error response when
Ticket.createfails after Jira creation succeeds, so ops can recover -src/controllers/task-management.js:1115
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 6m 34s | Cost: $8.84 | Commit: 3bb1e05d375df29d4a04863a2a0d038defed495e
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| 'GET /organizations/:organizationId/suggestions/:suggestionId/ticket', | ||
| 'GET /organizations/:organizationId/opportunities/:opportunityId/tickets', | ||
| 'POST /organizations/:organizationId/task-management/:provider/tickets', | ||
| 'GET /organizations/:organizationId/task-management/:provider/projects', |
There was a problem hiding this comment.
issue (blocking): The new routes introduce :connectionId and :provider as dynamic params not yet classified in src/routes/facs-capabilities.js. The repo CLAUDE.md states: "The routeFacsCapabilities test fails the build if a param is left unclassified." This is the likely cause of the CI build failure.
Fix: add 'connectionId' and 'provider' to the FACS_NON_RESOURCE_PARAMS array in src/routes/facs-capabilities.js (they are not ReBAC entities). :suggestionId and :opportunityId already exist in that array.
| @@ -548,6 +548,16 @@ describe('getRouteHandlers', () => { | |||
| getPreview: sinon.stub(), | |||
There was a problem hiding this comment.
issue (blocking): mockTaskManagementController defines 7 stubs but is missing listProjects. The route GET /organizations/:organizationId/task-management/:provider/projects references taskManagementController.listProjects which will be undefined, causing the route registration test to fail.
Fix: add listProjects: sinon.stub() to the mock object.
| } | ||
| } | ||
|
|
||
| if (batchSuggestionOk) { |
There was a problem hiding this comment.
issue (blocking): In the individual batch loop, when a 401/reauth error occurs (lines 863-870), the code marks the connection as requires_reauth but then continues the for...of loop. Subsequent iterations will call ticketClient.createTicket again with a revoked token - burning API calls and producing N-1 redundant 409 results.
Fix: after detecting a reauth-needed error, break out of the loop and mark all remaining suggestions as failed with the reauth message. This matches the single-ticket and grouped paths which both return immediately on 401.
Implements POST /organizations/:organizationId/task-management/:provider/tickets as defined in mysticat-architecture PR #150. Flow: 1. Validates organizationId, provider, and required body field (summary). 2. Calls TaskManagementConnection.findActiveByOrganizationAndProvider() to resolve the org's active Jira OAuth connection (404 if none, 409 if degraded). 3. Delegates to TicketClientFactory (spacecat-shared-ticket-client) which handles OAuth token refresh, ADF description conversion, and the Jira API call. 4. On 401 from Jira, marks the connection as requires_reauth so the UI can prompt reconnect without waiting for a GC cycle. 5. Persists a Ticket entity with ticketId / ticketKey / ticketUrl. 6. Returns 201 with the persisted ticket data. Adds @adobe/spacecat-shared-ticket-client dependency (activate after PR #1701 merges). Co-authored-by: Cursor <cursoragent@cursor.com>
…emove dead code Three bug fixes in the task-management controller: 1. TicketClientFactory.create() call was completely wrong: - 'provider' string passed where a connection object is expected - 'env' object passed where an AWS SecretsManagerClient is expected Fix: build a plain connectionObj from entity getters, construct SecretsManagerClient (region auto-detected from Lambda env) and an httpClient wrapping globalThis.fetch, then call TicketClientFactory.create(connectionObj, smClient, httpClient, log). 2. projectKey was never validated or passed to createTicket(): Jira rejects every ticket without a project key. Added required body validation and threaded projectKey through createTicket(). Also removed priority (not supported by JiraCloudClient in v1 — deferred to v2 once Jira field configuration is in place). 3. Dead-code isActive() guard removed: findActiveByOrganizationAndProvider only returns connections with status='active', so the 409 branch after a non-null result was unreachable and created a misleading narrative. Also documents v1 intentional deviations in the controller JSDoc. Co-authored-by: Cursor <cursoragent@cursor.com>
… Ticket Extracts the IMS user ID from the JWT via authInfo.getProfile().getImsUserId() and passes it as createdBy to Ticket.create(). Also passes the provider path param as ticketProvider (denormalized for audit/query purposes). Both fields are now required by the Ticket schema (spacecat-shared PR #1702) and the architecture spec (mysticat-architecture PR #150). Also includes ticketProvider in the 201 response body so callers can confirm which provider created the ticket without a follow-up lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
…reation When suggestionId is provided in the request body, creates a TicketSuggestion record after persisting the Ticket. The DB UNIQUE (suggestion_id) constraint prevents the same suggestion from being ticketed twice; a constraint violation returns 409 Conflict so the UI can surface a clear error. Also extracts TicketSuggestion from dataAccess and validates it at controller construction time, consistent with Ticket and TaskManagementConnection. Co-authored-by: Cursor <cursoragent@cursor.com>
…150) Four routes added to align with the architecture spec: GET /organizations/:orgId/task-management/connections List all connections for an org; optional ?provider= filter. GET /organizations/:orgId/task-management/connections/:connectionId Fetch a single connection; 404 on org mismatch (prevents enumeration). DELETE /organizations/:orgId/task-management/connections/:connectionId Delete the connection from DB and its OAuth secret from AWS Secrets Manager (7-day recovery window). ResourceNotFoundException on the SM secret is treated as a soft success so stale DB records can still be cleaned up. Does not revoke the Atlassian-side authorization in v1 (accepted risk — revoking via Atlassian's token revocation endpoint is a v2 enhancement). GET /organizations/:orgId/task-management/tickets List all tickets for an org. Also: - Rename suggestionId → suggestionIds (array) to match the spec field name. v1 processes only the first element; batch 207 is deferred to v2. The controller accepts both forms for forward-compat. - Extract serializeConnection() and serializeTicket() helpers to keep the response shape consistent across all endpoints. - Document Idempotency-Key v1 scope deviation in the JSDoc: header accepted but not cached; UNIQUE DB constraint prevents duplicates. Co-authored-by: Cursor <cursoragent@cursor.com>
1. mode validation: 'grouped' returns 400 in v1; unknown modes also 400. 2. suggestionIds max 10 validation (400 if exceeded). 3. Two new read endpoints: GET /organizations/:orgId/suggestions/:suggestionId/ticket GET /organizations/:orgId/opportunities/:opportunityId/tickets 4. Response shape: connectionId + statusSyncedAt:null + suggestions array. 5. Structured audit event log.info on ticket creation success. Co-authored-by: Cursor <cursoragent@cursor.com>
…ady isolates envs
NODE_ENV is "production" on all Lambda environments so the env segment was
misleading. New path: /mysticat/task-management/{orgId}/{connectionId}.
Mirrors the fix in spacecat-shared ticket-client-factory.js.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ODULE_NOT_FOUND failures - Add 61-test suite covering all 7 TaskManagementController methods - Fix task-management.js: top-level await/catch for shared-ticket-client import so module loads cleanly via test/utils.js bare import of src/index.js - Add task-management routes to INTERNAL_ROUTES in required-capabilities.js - Add mockTaskManagementController to routes/index.test.js - Add TaskManagementConnection/Ticket/TicketSuggestion stubs to index.test.js dataAccess Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… Ticket.create() - deleteConnection: replace connection.remove() with connection.markDisconnected() per PR #1702 v1 design — soft-delete preserves audit history, GC job cleans up later - Ticket.create(): pass ticketStatus from JiraCloudClient result rather than relying solely on schema default (aligns with PR #1701 createTicket() return shape) - Update tests: markDisconnected stubs + ticketStatus in all createTicket mock results Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… listProjects route - Replace null TicketClientFactory fallback with explicit error object so any code path reaching the fallback throws clearly instead of NPE; only swallow ERR_MODULE_NOT_FOUND (package absent in dev/test env without PR #1701) - Enforce Idempotency-Key header on POST tickets (400 if absent); full dedup via idempotency_keys table — lookup cached response, insert processing record, mark completed/failed on each exit path - Validate suggestion existence before bridge creation (404 if not found) per spec §7 - Add GET /organizations/:organizationId/task-management/:provider/projects route calling ticketClient.listProjects() with reauth detection - Add Suggestion to controller constructor injection and guard - Add 25 new unit tests covering all new paths; update routes snapshot Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop connectionId/createdBy from getTicketBySuggestion spread — already included by serializeTicket() - Drop ?? undefined from three opportunityId assignments — nullish coalesce to undefined is a no-op - Use connection.getInstanceUrl() (not ?.) in createTicket and listProjects connectionObj — instanceUrl is required by TicketClientFactory; silent undefined would be caught at Jira call time, not at construction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… + expand v1 deviation docs - createTicket now accepts optional connectionId from request body; when provided, the specified connection is loaded directly (loadConnectionForOrg + provider/status check); when absent, findActiveByOrganizationAndProvider is used as before. - Controller JSDoc expanded to explain why multi-connection 400 guard is not needed in v1 (DB partial unique index prevents multiple active connections per cloudId) and documents the client-provided summary/description deviation from spec §7 step 5. - Soft-delete comment expanded with FK preservation rationale: hard delete would cascade-delete associated tickets; disconnected status keeps FK target intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…s merged Co-authored-by: Cursor <cursoragent@cursor.com>
…-44690) Implements spec §30 attachment pipeline: optional base64 attachment in request body is decoded, size-validated (<= 3 MB), then proxied to Jira via ticketClient.uploadAttachment() after the ticket is persisted. Attachment failures are treated as partial success per spec — ticket is not rolled back, response includes attachmentWarning so the UI can prompt retry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…creation (SITES-44690)
- Add TICKET_MODE_GROUPED support: M suggestionIds → 1 Jira ticket (201)
- Add individual batch support: N suggestionIds → N Jira tickets (207 Multi-Status)
- Replace SUGGESTION_IDS_MAX=10 with mode-aware limits:
individual: max 10 suggestions per request
grouped: max 400 suggestions per request
- Move size check after mode resolution so grouped callers get the correct limit
- Block attachments in individual batch mode; upload per-ticket via attachment endpoint
- Validate all suggestionIds upfront in grouped mode (fail-fast before Jira call)
- Link all suggestions to single ticket in grouped mode; non-fatal on duplicate bridge row
- Add/update tests for new mode paths, attachment guard, and per-mode size limits
Co-authored-by: Cursor <cursoragent@cursor.com>
… in v1 (SITES-44690) - Remove "grouped is v2-only" note now that both individual and grouped are shipped - Update mode JSDoc: remove stale "returns 400 in v1" annotation - Document attachment, idempotency, and mode caps as v1 scope decisions Co-authored-by: Cursor <cursoragent@cursor.com>
…(SITES-44690) - Filter expired idempotency keys with expires_at >= now() on lookup - Handle unique constraint race (23505) on idempotency key insert with 409 - Pass priority, dueDate, components, and parent fields through to ticketClient.createTicket on all three code paths (individual batch, grouped, and single) Co-authored-by: Cursor <cursoragent@cursor.com>
…sedAt/errorMessage, fill test gaps - Rename ticketId to externalTicketId in Ticket.create calls and serializeTicket to match the ORM PK collision fix in data-access - Wire connection.setLastUsedAt/setErrorMessage on ticket creation success/failure across single, batch, and grouped paths - Add tests: expired idempotency key re-processing, explicit connectionId resolution (success/404/400), grouped and batch field pass-through (priority/dueDate/components/parent) Co-authored-by: Cursor <cursoragent@cursor.com>
…d test fixes (SITES-44690) - Short-circuit batch loop on 401 auth failure: remaining suggestions marked connection_reauth_required instead of calling Jira with a known-bad token - Batch idempotency key marked 'failed' when zero tickets succeed (was always 'completed' regardless of outcome) - Add connectionId UUID validation in index.js param checks - Add listProjects to mock task-management controller in route tests - Fix Suggestion mock in index.test.js (empty object fails isNonEmptyObject guard) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cket responses - connectionId is now required in POST /tickets request body (removes implicit resolution via findActiveByOrganizationAndProvider) - Add createdAt to serializeTicket output per spec response contract Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…POST /tickets
Change request body field from singular `attachment: {}` to plural
`attachments: [{}]` for consistency and future extensibility. Max 1
item per request enforced (Lambda 6MB sync payload limit).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-44690) Connection disconnect (SM DeleteSecret + DB soft-delete) is owned by auth-service PR #595. Removes the duplicate handler, route, and tests from api-service to align with the single-owner design. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aaf34ce to
a827a91
Compare
…ticketUrl on persistence failure (SITES-44690) - Name caught errors in bridge-load catch blocks and log warn with context - Include ticketKey and ticketUrl in 500 body when Jira succeeds but DB persist fails Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GET /organizations/:organizationId/task-management/:provider/projects was missing from INTERNAL_ROUTES. Without it, the route requires a FACS capability that doesn't exist, causing 403 for all callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Jira: SITES-44690
Summary
Adds the task-management controller and routes to spacecat-api-service (SITES-44690).
Ticket creation (POST /organizations/:organizationId/task-management/:provider/tickets)
Flow:
Additional routes
Merge order
v1 scope notes
DIP note
`SecretsManagerClient` is constructed inside the controller function rather than injected. This matches the existing pattern in the api-service (e.g. `SFNClient` in `agent-workflow.js`) and is acceptable for v1. The AWS SDK auto-detects region from the Lambda execution environment.