[CSM] feat: add time-cards search, patch change-request, and attachment endpoints - #964
Merged
cloby99 merged 16 commits intoJun 28, 2026
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds reference-based attachment endpoints, change-request patch support, and ServiceNow time-card search across entity-service and csm-portal backend. Routes, handlers, services, DTOs, OpenAPI specs, and backend docs were updated to match the new API shapes. ChangesNew API endpoints and attachment route migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SN returns state.id as a string for time cards; since only the label is used, removing the id field avoids the unmarshal type mismatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rashmika998
added
Type/Improvement
…called Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… both time-card/patch-CR and call-request schemas Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Adds attachment delete to both entity service and CSM portal backend.
Entity service calls DELETE /attachments/{sysid} on Choreo (SN-only).
CSM portal backend passes through as a raw JSON response.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rashmika998
changed the title
… route Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/csm-portal/backend/internal/handler/cases.go`:
- Around line 424-438: The DeleteCaseAttachment flow is not enforcing that the
attachment belongs to the requested case before deletion, so the
/cases/{id}/attachments/{attachmentId} scope is effectively ignored. Update the
cases.go handler’s DeleteCaseAttachment path to validate the case/attachment
relationship in the entity layer before calling the ServiceNow delete, using the
existing DeleteCaseAttachment method and related entity-service lookup logic to
confirm the attachment is associated with caseID. If the association does not
match, return an appropriate error instead of deleting by attachmentID alone.
In `@apps/csm-portal/backend/openapi.yaml`:
- Around line 1910-1918: The TimeCardState public enum is broader than the
supported backend contract because it includes approved. Update the
TimeCardState enum in openapi.yaml to match only the backend-accepted search
states, and remove approved unless there is matching backend support in the
time-card search implementation. Keep the enum aligned with the documented
states used by the portal contract.
- Around line 1888-1897: `PatchChangeRequestPayload` currently allows an empty
object, so update the schema to require at least one property for PATCH
requests. In the `PatchChangeRequestPayload` object definition, add schema
validation that prevents `{}` (for example, a minimum properties constraint or
an equivalent required-at-least-one-of-fields rule) while keeping
`plannedStartOn`, `isCustomerApproved`, and `isCustomerReviewed` optional
individually.
In `@apps/csm-portal/backend/README.md`:
- Around line 214-216: The Time Cards search docs incorrectly say `pagination`
is required, but the backend/OpenAPI request body and time-card payload schema
allow it to be omitted. Update the `POST /time-cards/search` description in the
README so it reflects that the request body is optional and `pagination` is
optional, while keeping the optional `filters` fields (`projectIds`,
`startDate`, `endDate`, `states`) accurate.
In `@entity-service/internal/service/sn_case_service.go`:
- Around line 1107-1115: `DeleteCaseAttachment` currently deletes by
`req.AttachmentID` only and ignores `req.CaseID`, so the route contract is not
enforced. Update this method to first verify the attachment belongs to the
requested case (using both `req.CaseID` and `req.AttachmentID`) before calling
`s.client.Delete`, and fail fast if the association does not match. Keep the fix
localized to `snCaseService.DeleteCaseAttachment` and use the existing request
fields plus the delete call path to locate the logic.
In `@entity-service/internal/service/sn_time_card_service.go`:
- Around line 209-214: The pagination metadata returned from SearchTimeCards is
using the request values instead of the values actually applied by the
downstream response. Update the SearchTimeCards flow in snTimeCardService to
return the parsed pagination from snResp (the same limit/offset already read
from the response) when constructing domain.SearchTimeCardsResponse, so clients
see the real page metadata.
- Around line 61-64: The search payload and response mapping need to preserve
optional filters and actual paging metadata. Update snTimeCardSearchPayload so
Filters is a pointer, allowing omitted filters to stay absent instead of
serializing as an empty object, and adjust SearchTimeCards to populate the
returned pagination from snResp.Limit and snResp.Offset rather than echoing the
requested values. Use the snTimeCardSearchPayload and SearchTimeCards symbols to
locate the changes.
In `@entity-service/openapi.yaml`:
- Around line 2946-2955: The advertised time-card filter enum in TimeCardState
still includes approved, but the supported search states are only pending,
submitted, rejected, processed, and recalled. Update the TimeCardState enum in
the OpenAPI definition to remove approved so the published schema matches
backend validation and the supported filter values.
- Around line 2924-2934: PatchChangeRequestRequest currently allows empty
objects because all properties are optional, so update the schema to enforce the
non-empty PATCH contract. In openapi.yaml, modify the PatchChangeRequestRequest
definition to add a guard such as minProperties or an equivalent oneOf/anyOf
constraint so at least one field must be present while preserving the existing
properties plannedStartOn, isCustomerApproved, and isCustomerReviewed. Ensure
the schema change keeps generated clients and validators from accepting {}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7a4dc2c-717b-4cc1-ad3a-97b779b245b1
📒 Files selected for processing (20)
apps/csm-portal/backend/README.mdapps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/internal/entity/entity.goapps/csm-portal/backend/internal/handler/cases.goapps/csm-portal/backend/internal/handler/change_requests.goapps/csm-portal/backend/internal/handler/helpers_test.goapps/csm-portal/backend/internal/handler/time_cards.goapps/csm-portal/backend/openapi.yamlentity-service/internal/domain/entity.goentity-service/internal/handler/case_handler.goentity-service/internal/handler/change_request_handler.goentity-service/internal/handler/time_card_handler.goentity-service/internal/server/routes.goentity-service/internal/service/case_service.goentity-service/internal/service/interfaces.goentity-service/internal/service/sn_case_service.goentity-service/internal/service/sn_change_request_service.goentity-service/internal/service/sn_time_card_service.goentity-service/internal/servicenow-integration-service/client.goentity-service/openapi.yaml
…ints - Make snTimeCardSearchPayload.Filters a pointer so omitempty skips it when nil - Return actual Limit/Offset from SN response instead of echoing request values - Add minProperties: 1 to PatchChangeRequest schemas in both openapi specs - Fix README to not imply pagination is required for time-card search Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
POST /attachments, POST /attachments/search, GET /attachments/{attachmentId}/content,
and DELETE /attachments/{attachmentId} no longer require caseId as a path param.
caseId moves to the request body for create and search; the get/delete endpoints
only need the attachmentId. Updated entity service domain types, service interface,
handlers, routes, and openapi spec. CSM portal backend entity client, handlers,
mock, routes, openapi, and README updated to match.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Consistent with all other single-resource routes that use {id}.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SearchAttachmentsRequest now uses referenceId (UUID) and referenceType (case|conversation|change_request|deployment) instead of caseId, matching the downstream Ballerina entity service's ReferenceSearchPayload type. The SN service validates and converts both fields before calling the Ballerina service. Added ReferenceType domain type, validReferenceTypes map, and updated both openapi specs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CreateAttachmentRequest now uses referenceId (UUID) and referenceType instead of caseId, matching AttachmentCreatePayload in the downstream Ballerina entity service. Validation added for both fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
entity-service/internal/handler/case_handler.go (1)
163-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate attachment IDs before forwarding to the service.
r.PathValue("id")is passed through directly, while the ServiceNow attachment content/delete paths convert it withuuidToSysidwithout the UUID validation used by create/search. Direct entity-service callers can send malformed IDs and trigger invalid downstream calls; reject non-UUID IDs before invoking the service.🛡️ Suggested shape
func (h *CaseHandler) GetCaseAttachmentContent(w http.ResponseWriter, r *http.Request) { attachmentID := r.PathValue("id") + if attachmentID == "" || !isValidUUID(attachmentID) { + writeServiceError(w, r, &apierror.ValidationError{Msg: "attachmentId must be a valid UUID"}) + return + } content, contentType, err := h.svc.GetCaseAttachmentContent(r.Context(), attachmentID)Apply the same check before constructing
DeleteAttachmentRequest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entity-service/internal/handler/case_handler.go` around lines 163 - 180, Validate the attachment ID in GetCaseAttachmentContent and DeleteCaseAttachment before calling h.svc by rejecting any non-UUID value from r.PathValue("id"), matching the UUID checks already used elsewhere. Apply the same validation in both handlers, then only pass a confirmed UUID into h.svc.GetCaseAttachmentContent and the DeleteAttachmentRequest used by h.svc.DeleteCaseAttachment so malformed direct requests are rejected early.entity-service/openapi.yaml (1)
2915-2925: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDisallow unsupported PATCH fields in the entity-service schema.
minProperties: 1still lets unknown-only objects pass schema validation. AddadditionalProperties: falseso clients must send at least one of the supported fields.🛠️ Suggested fix
PatchChangeRequestRequest: type: object minProperties: 1 + additionalProperties: false properties:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entity-service/openapi.yaml` around lines 2915 - 2925, The PatchChangeRequestRequest schema still allows objects made only of unknown fields because it does not block extra properties. Update the entity-service OpenAPI definition for PatchChangeRequestRequest to set additionalProperties to false alongside minProperties, so validation only accepts at least one of the declared fields plannedStartOn, isCustomerApproved, or isCustomerReviewed.apps/csm-portal/backend/openapi.yaml (1)
1858-1868: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unknown-only PATCH payloads in the schema.
minProperties: 1blocks{}, but{ "foo": true }still validates becauseadditionalPropertiesdefaults to allowed. That still violates “at least one supported field”.🛠️ Suggested fix
PatchChangeRequestPayload: type: object minProperties: 1 + additionalProperties: false properties:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/csm-portal/backend/openapi.yaml` around lines 1858 - 1868, The PatchChangeRequestPayload schema currently allows unknown-only objects because additionalProperties is not restricted, so update that object definition to reject fields other than plannedStartOn, isCustomerApproved, and isCustomerReviewed while keeping minProperties in place. Use the PatchChangeRequestPayload schema in openapi.yaml and add an explicit additionalProperties setting so a payload like { "foo": true } no longer validates.entity-service/internal/service/sn_case_service.go (1)
1114-1120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
attachmentIDbefore converting it to a sys_id.The entity-service handler passes
PathValue("id")through, so invalid UUIDs can reach ServiceNow as malformed attachment IDs instead of returning a local validation error.🛠️ Suggested fix
func (s *snCaseService) GetCaseAttachmentContent(ctx context.Context, attachmentID string) ([]byte, string, error) { + if err := validateUUIDs("attachmentId", []string{attachmentID}); err != nil { + return nil, "", err + } + token := middleware.UserIDTokenFromContext(ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entity-service/internal/service/sn_case_service.go` around lines 1114 - 1120, Validate attachmentID at the start of GetCaseAttachmentContent before calling uuidToSysid or s.client.GetBinary, and return a local validation error if it is not a valid UUID. Use the same validation approach/pattern already used in snCaseService for other ID inputs so malformed PathValue("id") values do not reach ServiceNow.apps/csm-portal/backend/internal/handler/cases_test.go (3)
994-1036: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the “valid” create payload to include the new reference fields.
validPayloadis now missing requiredreferenceIdandreferenceType, so these tests no longer exercise the actual/attachmentscontract.🛠️ Suggested fix
const ( testCaseID = "11111111-1111-1111-1111-111111111111" - validPayload = `{"name":"screenshot.png","type":"image/png","file":"data:image/png;base64,aGVsbG8="}` + validPayload = `{"referenceId":"` + testCaseID + `","referenceType":"case","name":"screenshot.png","type":"image/png","file":"data:image/png;base64,aGVsbG8="}` )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/csm-portal/backend/internal/handler/cases_test.go` around lines 994 - 1036, Update the valid attachment payload in TestCreateCaseAttachment so it matches the current /attachments contract by including the required referenceId and referenceType fields in validPayload. Keep the existing test structure, but ensure the success case and any client expectations still use the updated payload shape so the test exercises CreateCaseAttachment with a valid request.
1115-1134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the unused
testCaseIDconst inTestGetCaseAttachmentContent. Go rejects the local constant block here becausetestCaseIDis never referenced, so this test file won’t compile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/csm-portal/backend/internal/handler/cases_test.go` around lines 1115 - 1134, Remove the unused testCaseID constant from TestGetCaseAttachmentContent so the local const block only contains values that are actually referenced; this test helper is in cases_test.go and the unused symbol is causing the package to fail compilation. Keep testAttachmentID and the existing GetCaseAttachmentContent assertions intact.
1066-1085: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
testCaseIDin the search payload or remove it.testCaseIDis currently unused here, which will fail the test build, and/attachments/searchexpectsreferenceId/referenceTypein the request body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/csm-portal/backend/internal/handler/cases_test.go` around lines 1066 - 1085, The TestSearchCaseAttachments setup defines testCaseID but never uses it, and the /attachments/search request shape needs referenceId/referenceType in the body. Update the SearchCaseAttachments test to either include testCaseID in the valid payload used by the success case or remove the unused constant entirely, and make sure the request body matches what the handler expects so the test builds and exercises the intended path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/csm-portal/backend/openapi.yaml`:
- Around line 3436-3453: The attachment search schema only exposes case-oriented
fields, so update the Attachment-related response/documentation in openapi.yaml
to include the reference identity used by AttachmentSearchPayload. Add the
missing reference fields to the attachment search result schema (or the shared
Attachment schema) so clients can see both referenceId and referenceType, and
keep the existing caseId field only as the case-specific alias if needed. Use
the existing ReferenceType and AttachmentSearchPayload symbols to align the
response shape with the searchable reference types.
In `@entity-service/internal/service/sn_case_service.go`:
- Around line 1068-1070: The attachment search response in snCaseService is
still exposing the returned reference through caseId even though
snSearchAttachmentsPayload now supports conversation, change_request, and
deployment. Update the response model used by the attachment search path to
carry referenceId and referenceType (or otherwise preserve the original
reference scope) so non-case searches do not get misrepresented as case UUIDs;
if that path is intended to be case-only, enforce that restriction in the
snSearchAttachments flow and related request handling.
In `@entity-service/openapi.yaml`:
- Around line 2615-2632: The SearchAttachments response shape still exposes only
Attachment.caseId, which no longer matches the expanded ReferenceType support.
Update the attachment response schema/model used by the search endpoint to
return reference-scoped metadata via referenceId and referenceType (likely in
the Attachment schema and any generated/typesafe response models), or otherwise
restrict SearchAttachments/ReferenceType back to case-only; make sure the
response contract is consistent with SearchAttachmentsRequest and ReferenceType.
---
Outside diff comments:
In `@apps/csm-portal/backend/internal/handler/cases_test.go`:
- Around line 994-1036: Update the valid attachment payload in
TestCreateCaseAttachment so it matches the current /attachments contract by
including the required referenceId and referenceType fields in validPayload.
Keep the existing test structure, but ensure the success case and any client
expectations still use the updated payload shape so the test exercises
CreateCaseAttachment with a valid request.
- Around line 1115-1134: Remove the unused testCaseID constant from
TestGetCaseAttachmentContent so the local const block only contains values that
are actually referenced; this test helper is in cases_test.go and the unused
symbol is causing the package to fail compilation. Keep testAttachmentID and the
existing GetCaseAttachmentContent assertions intact.
- Around line 1066-1085: The TestSearchCaseAttachments setup defines testCaseID
but never uses it, and the /attachments/search request shape needs
referenceId/referenceType in the body. Update the SearchCaseAttachments test to
either include testCaseID in the valid payload used by the success case or
remove the unused constant entirely, and make sure the request body matches what
the handler expects so the test builds and exercises the intended path.
In `@apps/csm-portal/backend/openapi.yaml`:
- Around line 1858-1868: The PatchChangeRequestPayload schema currently allows
unknown-only objects because additionalProperties is not restricted, so update
that object definition to reject fields other than plannedStartOn,
isCustomerApproved, and isCustomerReviewed while keeping minProperties in place.
Use the PatchChangeRequestPayload schema in openapi.yaml and add an explicit
additionalProperties setting so a payload like { "foo": true } no longer
validates.
In `@entity-service/internal/handler/case_handler.go`:
- Around line 163-180: Validate the attachment ID in GetCaseAttachmentContent
and DeleteCaseAttachment before calling h.svc by rejecting any non-UUID value
from r.PathValue("id"), matching the UUID checks already used elsewhere. Apply
the same validation in both handlers, then only pass a confirmed UUID into
h.svc.GetCaseAttachmentContent and the DeleteAttachmentRequest used by
h.svc.DeleteCaseAttachment so malformed direct requests are rejected early.
In `@entity-service/internal/service/sn_case_service.go`:
- Around line 1114-1120: Validate attachmentID at the start of
GetCaseAttachmentContent before calling uuidToSysid or s.client.GetBinary, and
return a local validation error if it is not a valid UUID. Use the same
validation approach/pattern already used in snCaseService for other ID inputs so
malformed PathValue("id") values do not reach ServiceNow.
In `@entity-service/openapi.yaml`:
- Around line 2915-2925: The PatchChangeRequestRequest schema still allows
objects made only of unknown fields because it does not block extra properties.
Update the entity-service OpenAPI definition for PatchChangeRequestRequest to
set additionalProperties to false alongside minProperties, so validation only
accepts at least one of the declared fields plannedStartOn, isCustomerApproved,
or isCustomerReviewed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9639f442-3f1e-4110-b4e7-06c6ad7e950f
📒 Files selected for processing (15)
apps/csm-portal/backend/README.mdapps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/internal/entity/entity.goapps/csm-portal/backend/internal/handler/cases.goapps/csm-portal/backend/internal/handler/cases_test.goapps/csm-portal/backend/internal/handler/helpers_test.goapps/csm-portal/backend/openapi.yamlentity-service/internal/domain/entity.goentity-service/internal/handler/case_handler.goentity-service/internal/server/routes.goentity-service/internal/service/case_service.goentity-service/internal/service/interfaces.goentity-service/internal/service/sn_case_service.goentity-service/internal/service/sn_time_card_service.goentity-service/openapi.yaml
✅ Files skipped from review due to trivial changes (1)
- apps/csm-portal/backend/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- entity-service/internal/service/case_service.go
- entity-service/internal/service/interfaces.go
- entity-service/internal/service/sn_time_card_service.go
…tachmentId} Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…me-card and patch change-request endpoints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onse model Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rashmika998
changed the title
5 tasks
cloby99
approved these changes
Jun 28, 2026
This was referenced Jun 30, 2026
This was referenced Jul 12, 2026
This was referenced Jul 31, 2026
Merged
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.
Summary
Test plan
🤖 Generated with Claude Code