[CSM Portal] User profile, role and team lookups, plus project/account detail rework - #1287
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds user, role, team, and project-contact APIs across the entity service and CSM Portal backend. It adds canonical user references, ABT team resolution, ID-based profile links, project lifecycle presentation, and account deactivation updates. ChangesEntity-service APIs and data contracts
CSM Portal backend integration
CSM Portal frontend
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
rksk
changed the title
CS engineers belong to one of 15 named ServiceNow groups (Account-Based
Teams and a few adjacent operational teams). GET /users/me now resolves
the caller's team by forwarding their own id token to a name-based SN
group-membership lookup, entirely at request time — no team data is
cached or stored locally.
Team names are matched by string, not ServiceNow sys_id, since several
of these groups have different sys_ids (or don't exist at all) between
ServiceNow environments; resolving by name keeps this working correctly
regardless of which environment is behind the call. A team that doesn't
resolve in a given environment is simply absent from the result, not an
error.
Response is additive: `team: {teamKey, teamName, family}` on the
existing GET /users/me payload, omitted entirely when no team resolves.
Failure to resolve a team (lookup error, no match) degrades gracefully —
identity and roles are still returned.
The project search response carried no start date, so every consumer showed it as absent on every row. The backing-data-source adapter never declared the field, so it was discarded at unmarshal, and the view had nowhere to put it. Add it as a nullable field, since the backing data source can legitimately leave it blank and a zero time would serialise as a nonsense date. Note this is a distinct fact from createdOn: createdOn is when the project record was created and never moves, while the start date is the beginning of the current renewed period and moves forward on each renewal. Also documents the field in the component's published schema alongside the two dates it is easily confused with. The alternate (relational) data source populates this field on the same view but is deliberately left untouched here.
The list's Start column read a field the search response never returns, so it showed an em dash on every row. It now reads the start date the search response carries, and the Subscription column is replaced by the project's closure state, rendered as a colour-coded chip so projects in a closure process stand out. The project detail page drops the duplicate project key under the title, and its Overview card drops Account tier and the project id while gaining State. Its date labels now share one form: Created on, Updated on, and a pair whose tense is derived rather than hardcoded. The start-date label reads "Renewed on" when the start date falls on a later calendar day than the creation date, and "Started on" otherwise, because the start date is the beginning of the current renewed period and advances on each renewal. The comparison is on calendar days rather than instants: the creation date is a full timestamp while the start date is effectively midnight, so comparing instants would report a renewal for every project created and started on the same day. Tabs are reordered to Overview, Deployments, Project contacts, Work items. "Issues" is renamed to "Work items" and the contacts tab is a placeholder. The create menu gains an engagement entry, wired to the existing route that already accepts and locks a project. Service requests are offered only for a managed cloud subscription. Also removes a stale comment asserting no engagement route existed. The search-row type is corrected to match what the endpoint actually returns: it had declared a start date, a created-at, and an updated-at that never arrive, and was missing the closure fields.
Replace the two bare owner-id cells with the named account manager, renewal account manager, and technical owner, which the account detail response already carries on the alternate response shape. Drop the Account ID cell (redundant with the URL) and the duplicate Salesforce ID line under the account name. Relabel the two capability toggles to "AI Chat Assistant (Novera)" and "Smart KB Suggestions", and make the deactivation cell tense-aware: "Deactivated on" for a past date, "Deactivates on" for a future one. The header's "Deactivated" chip now renders only for a past date, driven by the same comparison so the chip and the label cannot disagree. Every date label in the card now follows one form, "<verb> on", so the card no longer mixes bare participles with "<noun> date". The card holds twelve cells, which packs into four exact rows on the three-column layout with no trailing gaps.
The project schemas had drifted from the code and were misdescribing the responses in ways that would send the next consumer down the wrong path. The gateway spec used a single schema for both the project search row and the project detail response, so it was wrong for both: it named the project key and the account reference incorrectly, advertised an identifier and an updated timestamp that the search projection does not return, and omitted the closure fields entirely. Split into a search-row schema and a detail schema that match the two views, with the closure fields factored into a shared component so the pair cannot drift apart again. The entity service spec was missing the account reference on the project search view. Also documents the caller's team on the current-user response, which the handler already returns. Docs only: neither spec drives code generation, and no runtime behaviour changes.
…rvice
Serves the CSM portal's user profile page and its role/group/team directory pages. Mirrors
the additive ServiceNow update set 1262-S1222-T90-CST-SajithE and the corresponding
entity-service (Ballerina) changes.
New endpoints:
- GET /users/{id} — one user's full profile: the row plus every group they belong to, the
subset of those that are teams, and for external contacts their per-project access with
the reason access is or is not granted. Built on the search's userIds filter since there
is no get-by-id upstream. A deactivated user is still returned, because that state is
usually the answer the caller wants.
- POST /roles/search — the curated role catalogue. Served from the same map that validates
the roleIds filter, so the dropdown and the filter cannot disagree. Deliberately not a
query against the backing source: it carries dozens of product-shipped roles that mean
nothing here, with ids that differ per environment.
- POST /teams/search — the team registry, fetched upstream at runtime. A team's id is its
registry key, not the backing group's id, because group ids differ between environments.
POST /users/search gains userIds, groupIds and teamIds filters, and renames roles to
roleIds for consistency. Group and team membership cannot be expressed as an upstream
user-search filter — that source cannot join users against group membership in one query —
so both resolve to a user-id set here before a single upstream call, keeping paging and
totals server-side. When a membership filter resolves to nobody the response is an empty
page: sending no id filter instead would return every user, which reads as "everyone
matched".
SNUser gains mobilePhone and userType (internal/external). Only the mobile number is
exposed — it is the number the on-call escalation flow dials. userType is derived upstream
from role membership. Note the two data sources name the non-staff value differently:
postgres emits "customer", ServiceNow emits "external"; callers branching on staff-vs-not
must treat both alike.
The registry fetch moves from /abt-teams to /teams, matching the renamed upstream
resource, and the role enum gains timecard_approver, which the upstream accepts but this
side could not filter on.
Profile enrichments are best-effort: a failed group or project-contact lookup yields an
empty list rather than failing the whole profile, matching how the caller's own team is
resolved on GET /users/me.
Verified: go build, go vet and go test all clean. Seven new service tests cover the
empty-page-on-no-match rule, the intersect-not-union behaviour of userIds with a
membership filter, unknown team keys erroring rather than silently matching nothing,
group/team enrichment, external project access, graceful degradation, and 404 on an
unknown id. Route precedence for /users/{id} against /users/me confirmed separately.
rksk
changed the title
rksk
marked this pull request as ready for review
July 31, 2026 18:35
Passthroughs for the entity-service endpoints the user profile page and the role / group /
team directory pages need. No business logic here — the entity service owns the
aggregation; this layer authenticates, validates and forwards.
- GET /users/{id} — one user's profile: the row plus group and team membership, and for
external contacts their per-project access. Registered after /users/me, which is the more
specific pattern and still wins for that exact path.
- POST /roles/search and POST /teams/search — the role catalogue and team registry that back
the directory filters. Both accept an absent body, meaning "no filters, default page", so
an empty request is a 200 rather than a 400.
- POST /users/search forwards the new userIds, groupIds and teamIds filters, and roles is
renamed roleIds to match the layer below.
- The user shape carries mobilePhone and userType. Only the mobile number is exposed: it is
the number the on-call escalation flow dials.
Both search handlers share one forward helper. They differ solely in which client call they
make and what they are named in logs, so duplicating the read-body / validate / map-error
sequence per endpoint would have been three copies of the same thing.
openapi.yaml updated to match: the three new paths, the four new filters, mobilePhone and
userType on SNUser, and the SNUserDetail / Role / Team / UserProjectAccess schemas. Every
$ref resolves. Note for consumers of userType: the two data sources name the non-staff value
differently — postgres emits "customer", ServiceNow emits "external" — so anything branching
on staff-vs-not must treat both alike; the enum documents this.
Verified: go build and go test ./internal/... clean. Nine new handler subtests cover
authentication, verbatim body forwarding, the optional-body case, malformed JSON, upstream
failure mapping, roles-vs-teams call routing, and path-id passthrough including a missing id.
rksk
marked this pull request as draft
July 31, 2026 18:54
…act lookup
The project contacts search now surfaces each contact's user id, so a contacts list can link
a row to that user's profile. Without it a list can render a name but has nothing to build a
link from.
- `ProjectContact` gains `ID`. The upstream field is optional and may be null — absent on an
instance that predates it, null for a row with no linked contact record — so a nil or blank
upstream value maps to an empty id rather than a converted-garbage one. Callers use
emptiness to decide whether the row is clickable.
- New `GET /projects/{id}/contacts/{contactId}` returns one contact's attributes for a single
project: their roles on it, registration state and notification preference. Implemented by
reading the project's contacts and picking the matching row — there is no by-id contact
resource upstream, and a project's contact list is a handful of people, so paging to find
one is cheaper than adding a by-id filter to a shared upstream resource. That keeps the
whole feature inside this layer, and the row shape stays identical to the list's, so a
caller showing contact detail renders fields it already knows.
- The scan is bounded. If a project ever exceeds the bound the lookup reports not-found and
logs the shortfall, rather than silently searching a partial list and calling it absent.
- Backend passthrough plus both OpenAPI specs. The portal spec's inline contact item is
promoted to a named `ProjectContact` schema so the list and the new detail endpoint cannot
drift apart.
Depends on a corresponding entity-service change in the private repo, tracked separately,
which adds the field to the contacts response. That change is deliberately both optional and
tolerant of unknown fields, so the two sides can deploy in any order.
Verified: go build, go vet and go test clean in both modules. A new entity-service test proves
that a null upstream id and an absent one both yield an empty id rather than a bogus one, and
three backend subtests cover authentication, both path ids being forwarded, and a missing
contact id. The repo's route/spec contract test caught the new route before it was declared —
now satisfied.
Every actor in a case, comment, attachment, activity or watcher response now
carries a sibling user reference with exactly id, email and name, so a consumer
has one shape for "who did this" instead of a different set of flat fields per
endpoint. Nothing existing moves: the incumbent createdBy/assignedEngineer/
watchList fields are byte-for-byte what they were, because the live customer
portal and the current webapp read them.
The id is a pointer and serializes as an explicit null, deliberately. The
backing data source hands over a user id only where it already had to resolve
the user record for another reason: comment and attachment rows, plus the
assignee id that already arrives with a case. Filling the id in at the remaining
sites (case creator, activity-feed actors, watchers) would mean an extra per-row
user lookup on hot list endpoints, in a data-source layer shared with a live
production consumer. That cost was weighed and rejected, so the id stays null
there and a consumer resolves it from the email through its own cached lookup.
An explicit null says "no id available here"; an omitted key would be
indistinguishable from a producer too old to send one, which is why the field is
never omitted.
Ids coming from the data source go through the same sysid-to-UUID conversion as
every other id this service emits, so the value is accepted as-is by
GET /users/{id}. An empty or absent upstream user object can never produce an
id: it yields a null one, with the email and name that are present at every
site. That makes this deployable before the corresponding data-source change,
which is tracked separately, and there is a test for exactly that case.
Watcher references keep a null id even though the entry carries one: the watch
list collapses several upstream lists and an entry is not guaranteed to point at
a user record, so emitting that id could hand a consumer an id that is not a
user's.
The entity service now hangs a UserReference — exactly id, email and name — off every person-valued field it returns, and this backend forwards those responses byte-for-byte, so the field already reaches consumers. This is the published contract catching up with what the service actually returns; no handler code changes, because every affected response path (case get and search, comment, attachment and activity search, watch lists) passes the upstream bytes straight to the client, and the one place that does touch a case response merges into a map of raw JSON, which preserves keys it does not know about. Purely additive: createdByUser, assignedEngineerUser, assignedToUser and the watcher's user sit alongside the incumbent createdBy/assignedEngineer/watchList properties, which are unchanged because the live customer portal and the current webapp read them. The id inside the reference is nullable, and that is deliberate — a future reader should not "fix" it. The backing data source supplies a user id only where it already had to resolve the user record for another reason: comment and attachment rows, plus the assignee id that arrives with a case. Filling it in at the remaining sites (case creator, activity-feed actors, watchers) would mean an extra per-row user lookup on hot list endpoints, in a data-source layer shared with a live production consumer; that cost was weighed and rejected. email and name are populated everywhere, so a consumer that needs the id resolves it from the email through its own cached user lookup. The key is always present: an explicit null means "no id available here", whereas an omitted key would be indistinguishable from a producer too old to send one.
…rence
The backend now sends a UserReference ({id, email, name}) alongside every
person-valued field, but id is only populated where the backing data source
already resolved that actor to a user record (comment/attachment authors,
the case assignee) — populating it everywhere else would mean a per-row
user lookup on hot list endpoints shared with the live customer portal, a
cost that was explicitly rejected. So the frontend owns resolving the rest:
UserRefLink now takes an optional userId and, when it's null or absent,
resolves it from the actor's email through useResolvedUserId. Distinct
emails requested in a short window are coalesced into one POST
/users/search call (see userEmailResolutionLoader) rather than one request
per actor, and each result is cached under its own react-query key so a
resolved id is reused across the rest of the session, not just the page
that first needed it. A person's profile route moves from /people/:email
to /people/:id to match, backed by GET /users/{id}; a missing UserReference
(old backend) or an email that never resolves both degrade to plain text,
never a blocked render or a surfaced error.
rksk
marked this pull request as ready for review
July 31, 2026 21:48
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
entity-service/internal/server/routes.go (1)
205-228: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegister
GET /users/{id}for the non-ServiceNow data source too.When
cfg.DataSource != config.DataSourceServiceNow, onlyPOST /users/searchis registered; every other dual-mode entity router registers the same path set for both ServiceNow and non-ServiceNow sources. Add a non-ServiceNowGetUserhandler forGET /users/{id}, or remove the ServiceNow-only path if this endpoint is intentionally ServiceNow-specific.🤖 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/server/routes.go` around lines 205 - 228, Update the non-ServiceNow branch in the route setup around snUserHandler so GET /users/{id} is also registered with the appropriate non-ServiceNow userHandler.GetUser; preserve the existing ServiceNow registrations and POST /users/search behavior.
🧹 Nitpick comments (3)
apps/csm-portal/backend/internal/handler/reference_test.go (1)
143-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse real UUIDs and
upstreamErrorsin these subtests.Two points:
- Lines 149-150 set
idto"x", and lines 176-177 setid/contactIdto"pid"/"cid". Both parameters are UUID path parameters. Real UUIDs are required, and they become mandatory once the handlers applyuuidRe.- Lines 146 and 74 build the upstream failure with
errors.New, so the assertions can only check "not 200".upstreamErrors(fallback)lets each subtest assert the mapped status directly.Based on learnings from the coding guidelines: "Use
upstreamErrors(fallback),withUser(), anddecodeJSON[T]()for standard handler tests, and use real UUIDs for UUID path parameters."Also applies to: 187-194
🤖 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/reference_test.go` around lines 143 - 157, Update the affected handler subtests around GetUser and the additional cases at the referenced symbols to use valid UUID strings for all UUID path parameters, including id, pid, and cid. Replace generic errors.New upstream failures with upstreamErrors using the intended fallback, then assert the exact mapped HTTP status instead of only checking that the response is not 200; preserve withUser and decodeJSON[T]() conventions where applicable.Source: Coding guidelines
entity-service/internal/repository/case_repo.go (1)
484-499: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPopulate the full
CreatedByUserreference inSearchCases, matchingGetCaseByID.The
SearchCasesquery already joinsusers uonu.id = c.created_by(see thejoinsstring). The SELECT list only projectsu.email, soCreatedByUseris built with an empty id and empty name at Line 561.GetCaseByID, which uses the same join, selectsu.idand the composed name and fully resolvesCreatedByUserat Line 176-177.This creates an inconsistency for the same Postgres-backed data source: a case detail view resolves its creator by id, but the same case in a search-result list always carries a null-id creator reference. Add
u.idandu.first_name || ' ' || u.last_nameto the data query's SELECT list, scan them alongsidecv.CreatedBy, and pass them intoNewUserReferenceso search results carry the same canonical reference completeness as case detail views.♻️ Proposed fix
dataQuery := fmt.Sprintf( `SELECT c.id, c.number, c.internal_id, c.type, c.subject, c.description, c.severity, c.issue_type, c.state, c.engagement_type, c.work_state, c.created_at, - u.email, + u.id, u.first_name || ' ' || u.last_name, u.email, p.id, p.name, d.id, d.name, dp.id, prod.name || COALESCE(' ' || pv.version, ''), prod.id, prod.name, ae.id, ae.first_name || ' ' || ae.last_name, pc.id, pc.number, rc.id, rc.number FROM cases c %s %s ORDER BY %s %s NULLS LAST, c.id LIMIT $%d OFFSET $%d`, joins, where, sortCol, sortDir, argIdx, argIdx+1, )+ var creatorID, creatorName string if err := rows.Scan( &cv.ID, &cv.Number, &cv.InternalID, &caseType, &subject, &description, &severity, &issueType, &cv.State, &engagementType, &workState, &createdAt, - &cv.CreatedBy, + &creatorID, &creatorName, &cv.CreatedBy, &cv.Project.ID, &cv.Project.Name, ... ); err != nil { return fmt.Errorf("scan case: %w", err) } ... - cv.CreatedByUser = domain.NewUserReference("", cv.CreatedBy, "") + cv.CreatedByUser = domain.NewUserReference(creatorID, cv.CreatedBy, creatorName)Also applies to: 559-564
🤖 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/repository/case_repo.go` around lines 484 - 499, Update the SearchCases query and result mapping to select u.id and the composed u.first_name || ' ' || u.last_name alongside u.email, scan these values into the CreatedBy fields, and pass the id and name to NewUserReference so CreatedByUser matches the complete reference returned by GetCaseByID.entity-service/internal/service/sn_user_directory_test.go (1)
48-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
strconv.Itoainstead of a custom digit-building loop.
itoamanually builds the decimal string forn. func itoa(n int) string { if n == 0 { return "0" } digits := "" for n > 0 { digits = string(rune('0'+n%10)) + digits n /= 10 } return digits } Replace it withstrconv.Itoa(n).♻️ Proposed fix
-func itoa(n int) string { - if n == 0 { - return "0" - } - digits := "" - for n > 0 { - digits = string(rune('0'+n%10)) + digits - n /= 10 - } - return digits -} +func itoa(n int) string { + return strconv.Itoa(n) +}🤖 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_user_directory_test.go` around lines 48 - 58, Replace the custom digit-building logic in itoa with strconv.Itoa, adding the required strconv import and preserving the helper’s existing string conversion behavior.
🤖 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/users.go`:
- Around line 259-263: Validate UUID path parameters with the package-level
uuidRe before upstream calls: in
apps/csm-portal/backend/internal/handler/users.go lines 259-263, update GetUser
to reject empty or non-UUID id values with ErrMsgInvalidUUID; in
apps/csm-portal/backend/internal/handler/projects.go lines 163-168, apply the
same validation to both id and contactId in GetProjectContact, matching
SearchProjectContacts. Update both affected subtests in
apps/csm-portal/backend/internal/handler/reference_test.go to use real UUID
values instead of "x", "pid", and "cid".
In
`@apps/csm-portal/webapp/src/features/csm-accounts/pages/CsmAccountDetailPage.tsx`:
- Around line 283-285: Update the deactivation MetaCell in CsmAccountDetailPage
so deactivationState "none" is not labeled "Deactivated on": hide the field for
that state or use the neutral "Deactivation date" label, while preserving
"Deactivates on" for "future" and the existing behavior for valid deactivated
states.
In `@apps/csm-portal/webapp/src/features/csm-accounts/types/csmAccounts.ts`:
- Around line 30-34: Update the PersonRef interface so its id property accepts
string or null, while preserving the existing name and optional email typings.
In
`@apps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.tsx`:
- Around line 281-285: The user-reference links currently prefer legacy email
fields without falling back to canonical UserReference.email. Update
CsmCaseCommentBubble.tsx:281-285, CaseActivitiesFeed.tsx:428-432 and 549-553,
and CaseDetailWidgets.tsx:551-555 and 845-849 to pass the canonical nested email
when the existing email is empty, preserving the existing ID values. Add a
regression test covering a null canonical ID, absent legacy email, and valid
canonical email.
In
`@apps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectDetailPage.tsx`:
- Around line 119-131: Extract the duplicated ClosureStateChip component into a
shared component at
apps/csm-portal/webapp/src/features/csm-projects/components/ClosureStateChip.tsx,
preserving its closureStatePresentation logic and using the Typography “—”
fallback. Remove the local definitions and import the shared component in
apps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectDetailPage.tsx
(lines 119-131) and
apps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectsPage.tsx
(lines 56-67).
In `@entity-service/internal/domain/abt_team_test.go`:
- Around line 40-49: Update resetAbtRegistry to register a t.Cleanup callback
that calls SetAbtTeamsFetcher(nil) after the initial registry reset, ensuring
abtFetcher, abtLoaded, and abtTeams are cleared again when each test finishes.
In `@entity-service/internal/domain/abt_team.go`:
- Around line 132-169: Update ensureAbtRegistryLoaded so abtLoaded is set to
true only after both fetching and parsing the ABT team registry succeed.
Preserve the empty-registry result for a successful empty response, but leave
abtLoaded false on missing fetchers, fetch errors, or parse errors so later
callers retry; ensure the final commit logic in ensureAbtRegistryLoaded does not
mark failed attempts as loaded.
In `@entity-service/internal/domain/entity.go`:
- Around line 291-294: Update the OpenAPI schemas in openapi.yaml to match
GetUserMeResponse by adding a nullable team property referencing a new UserTeam
schema. Define UserTeam with required teamKey, teamName, and family string
properties, preserving the documented nullable behavior for unresolved or failed
best-effort team resolution.
- Around line 2191-2195: The optional contact ID field ID in the entity domain
model should use a string pointer, with nil representing no linked contact.
Update the mapping in sn_project_service.go to assign nil when no contact record
exists, and update GetProjectContact to guard c.ID != nil before dereferencing
it for the contactID comparison.
In `@entity-service/internal/service/role_service.go`:
- Around line 63-71: Update clampCatalogPagination and its call sites in
SearchRoles and SearchTeams so they return the clamped requested limit
separately from the slice length, and populate SearchRolesResponse.Limit and the
corresponding team response with that effective page size while continuing to
use the length for slicing.
In `@entity-service/internal/service/sn_project_service.go`:
- Around line 582-626: The GetProjectContact scan window does not match the
effective limit enforced by SearchProjectContacts. Update
projectContactScanLimit and its surrounding documentation/warning semantics so
the configured limit reflects the normalized pagination window of 100, or
consistently raise the normalized limit to 200; ensure the fallback logging
reports the actual maximum scanned window.
In `@entity-service/internal/service/sn_user_service.go`:
- Around line 417-421: Update GetUser to distinguish an empty id from a
non-empty id that uuidToSysid cannot convert: retain the “id is required”
validation message only for empty input, and return a clear validation message
such as “id must be a valid UUID” for malformed input.
- Around line 226-253: In the request validation flow before ID conversion or
membership resolution, add validateUUIDs("userIds", req.Filters.UserIDs) after
the existing user-ID count check. Add the corresponding group-ID count limit and
validateUUIDs("groupIds", req.Filters.GroupIDs), using the established filter
limit and validation error conventions.
In `@entity-service/openapi.yaml`:
- Around line 3193-3202: Define a dedicated CatalogPagination schema with limit
constrained to 1–200, defaulting to 50, and offset constrained to non-negative
values with default 0, matching clampCatalogPagination and its
catalogDefaultLimit/catalogMaxLimit settings. Update both SearchRolesRequest and
SearchTeamsRequest to reference CatalogPagination instead of the shared
Pagination schema, preserving the documented runtime limits.
- Around line 3073-3087: Add matching maxItems constraints to the groupIds and
teamIds array schemas in the OpenAPI definition, consistent with roleIds,
userNames, emails, and userIds. Update SNUserService to reject arrays exceeding
those limits before resolving group or team membership, ensuring runtime
validation matches the documented contract.
---
Outside diff comments:
In `@entity-service/internal/server/routes.go`:
- Around line 205-228: Update the non-ServiceNow branch in the route setup
around snUserHandler so GET /users/{id} is also registered with the appropriate
non-ServiceNow userHandler.GetUser; preserve the existing ServiceNow
registrations and POST /users/search behavior.
---
Nitpick comments:
In `@apps/csm-portal/backend/internal/handler/reference_test.go`:
- Around line 143-157: Update the affected handler subtests around GetUser and
the additional cases at the referenced symbols to use valid UUID strings for all
UUID path parameters, including id, pid, and cid. Replace generic errors.New
upstream failures with upstreamErrors using the intended fallback, then assert
the exact mapped HTTP status instead of only checking that the response is not
200; preserve withUser and decodeJSON[T]() conventions where applicable.
In `@entity-service/internal/repository/case_repo.go`:
- Around line 484-499: Update the SearchCases query and result mapping to select
u.id and the composed u.first_name || ' ' || u.last_name alongside u.email, scan
these values into the CreatedBy fields, and pass the id and name to
NewUserReference so CreatedByUser matches the complete reference returned by
GetCaseByID.
In `@entity-service/internal/service/sn_user_directory_test.go`:
- Around line 48-58: Replace the custom digit-building logic in itoa with
strconv.Itoa, adding the required strconv import and preserving the helper’s
existing string conversion behavior.
🪄 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 Plus
Run ID: 41abcabd-b2e4-4d44-8c6f-1cf21942f7c6
📒 Files selected for processing (59)
apps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/internal/entity/customer.goapps/csm-portal/backend/internal/handler/helpers_test.goapps/csm-portal/backend/internal/handler/projects.goapps/csm-portal/backend/internal/handler/reference.goapps/csm-portal/backend/internal/handler/reference_test.goapps/csm-portal/backend/internal/handler/users.goapps/csm-portal/backend/internal/handler/users_test.goapps/csm-portal/backend/openapi.yamlapps/csm-portal/webapp/src/App.tsxapps/csm-portal/webapp/src/api/backend/mappers.tsapps/csm-portal/webapp/src/api/backend/types.tsapps/csm-portal/webapp/src/components/UserRefLink.test.tsxapps/csm-portal/webapp/src/components/UserRefLink.tsxapps/csm-portal/webapp/src/features/csm-accounts/pages/CsmAccountDetailPage.tsxapps/csm-portal/webapp/src/features/csm-accounts/types/csmAccounts.test.tsapps/csm-portal/webapp/src/features/csm-accounts/types/csmAccounts.tsapps/csm-portal/webapp/src/features/csm-cases/api/useGetCsmCaseDetail.tsapps/csm-portal/webapp/src/features/csm-cases/components/CaseActivitiesFeed.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseActivitiesFeed.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseMetaBand.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.tsxapps/csm-portal/webapp/src/features/csm-cases/types/csmCases.tsapps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectDetailPage.tsxapps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectsPage.tsxapps/csm-portal/webapp/src/features/csm-projects/types/csmProjects.tsapps/csm-portal/webapp/src/features/csm-projects/utils/projectLifecycle.test.tsapps/csm-portal/webapp/src/features/csm-projects/utils/projectLifecycle.tsapps/csm-portal/webapp/src/features/csm-users/api/useGetUserById.tsapps/csm-portal/webapp/src/features/csm-users/api/useResolvedUserId.tsapps/csm-portal/webapp/src/features/csm-users/api/userEmailResolutionLoader.tsapps/csm-portal/webapp/src/features/csm-users/pages/UserProfilePage.tsxapps/csm-portal/webapp/src/features/csm-users/utils/isPlausibleEmail.tsapps/csm-portal/webapp/src/types/userReference.tsentity-service/internal/domain/abt_team.goentity-service/internal/domain/abt_team_test.goentity-service/internal/domain/entity.goentity-service/internal/handler/project_handler.goentity-service/internal/handler/reference_handler.goentity-service/internal/handler/user_handler.goentity-service/internal/repository/case_repo.goentity-service/internal/server/routes.goentity-service/internal/service/interfaces.goentity-service/internal/service/role_service.goentity-service/internal/service/sn_case_service.goentity-service/internal/service/sn_comment_service.goentity-service/internal/service/sn_project_service.goentity-service/internal/service/sn_project_service_test.goentity-service/internal/service/sn_user_directory_test.goentity-service/internal/service/sn_user_reference.goentity-service/internal/service/sn_user_reference_test.goentity-service/internal/service/sn_user_service.goentity-service/internal/service/sn_user_service_test.goentity-service/internal/service/team_service.goentity-service/internal/service/user_service.goentity-service/openapi.yaml
Comment on lines
+3073
to
+3087
| groupIds: | ||
| type: array | ||
| items: | ||
| type: string | ||
| format: uuid | ||
| description: > | ||
| Restrict to members of these groups. Resolved to a user-id set before the | ||
| upstream call, because the data source cannot join users against group | ||
| membership in one query. ServiceNow data source only. | ||
| teamIds: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: > | ||
| Restrict to members of these teams, by team key. ServiceNow data source only. |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find validation of groupIds/teamIds in the ServiceNow user service.
set -euo pipefail
fd -t f 'sn_user' entity-service/internal/service --exec rg -n -C6 'GroupIDs|TeamIDs|resolveUserGroupsAndTeams|group-members'Repository: wso2-open-operations/cs-tools
Length of output: 16925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== openapi bounds around user filter schemas =="
rg -n -C8 'userIds:|groupIds:|teamIds:|maxItems' entity-service/openapi.yaml | sed -n '1,220p'
echo
echo "== ServiceNow user resolve implementation =="
sed -n '290,430p' entity-service/internal/service/sn_user.go
echo
echo "== SearchUsers path and domain filters =="
rg -n -C6 'type SearchUsers|SearchUsersFilters|GroupIDs|TeamIDs|len\(filters\.(Group|Team)IDs\)' entity-service/internal/service entity-service/internal/domainRepository: wso2-open-operations/cs-tools
Length of output: 3105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ServiceNow user service files =="
fd -a 'sn|service.*user|user.*service|entity-service' . | sed 's#^\./##' | rg -i 'sn|user' | head -200
echo
echo "== locate ServiceNow references =="
rg -n -C8 'snUserService|ServiceNowUserService|resolveUserGroupsAndTeams|resolveAbtTeam|resolve\.(User|Group)|TeamIDs|GroupIDs|groupNames:|teamIds contains unknown team|SearchUsersFilters' entity-service/internal entity-service/openapi.yaml | head -400
echo
echo "== git status files changed in entity-service/internal =="
git status --short entity-service/internal | head -100 || trueRepository: wso2-open-operations/cs-tools
Length of output: 39865
Bound groupIds and teamIds.
roleIds, userNames, emails, and userIds already declare maxItems, but groupIds and teamIds do not. Add matching maxItems here and reject oversized arrays in SNUserService before resolving membership, so the openapi contract and runtime enforcement agree.
🤖 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 3073 - 3087, Add matching maxItems
constraints to the groupIds and teamIds array schemas in the OpenAPI definition,
consistent with roleIds, userNames, emails, and userIds. Update SNUserService to
reject arrays exceeding those limits before resolving group or team membership,
ensuring runtime validation matches the documented contract.
Contributor
Author
There was a problem hiding this comment.
Fixed in f9cadd8. Added maxItems: 50 to both groupIds and teamIds, with matching runtime rejection in SearchUsers (snGroupIDFilterLimit, snTeamIDFilterLimit), so contract and enforcement agree in both directions.
50 rather than the 200 used for userIds: each groupId widens the membership query that resolves the filter into a user-id set, and that set then feeds the same IN clause snUserIDFilterLimit exists to protect, so the multiplication argues for a tighter bound. Also added validateUUIDs("groupIds", ...), since a malformed group id had the same pass-through problem.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== user-filter contract =="
rg -n -A12 -B2 '^( (userIds|groupIds|teamIds):| maxItems:)' entity-service/openapi.yaml | sed -n '1,180p'
echo
echo "== ServiceNow user-filter limits and validation =="
rg -n -C5 'sn(Group|Team|User)IDFilterLimit|validateUUIDs\("groupIds"|validateUUIDs\("teamIds"|GroupIDs|TeamIDs' \
entity-service/internal/service --glob '*.go' | sed -n '1,300p'Length of output: 15993
@rksk, verification is inconclusive because the checked PR branch does not contain the reported changes.
entity-service/openapi.yaml still has no maxItems for groupIds or teamIds. SNUserService.SearchUsers still validates only userIds; it does not contain snGroupIDFilterLimit, snTeamIDFilterLimit, or validateUUIDs("groupIds", ...).
Please retry after commit f9cadd891 is available on this PR branch.
🧠 Learnings used
Learnt from: rksk
Repo: wso2-open-operations/cs-tools PR: 1303
File: entity-service/openapi.yaml:0-0
Timestamp: 2026-07-31T12:25:50.500Z
Learning: In entity-service/openapi.yaml, review every request schema against the entity service's runtime validation rules, especially when an endpoint uses a subset of a shared enum. Route-to-contract validation detects registered routes missing from the OpenAPI document but may not detect schema values that differ from service validation, so keep OpenAPI enum values and other constraints synchronized with the endpoint-specific runtime validation sets.
You are interacting with an AI system.
Team registry no longer caches a failed load: a fetch or parse failure left the
registry permanently empty for the process lifetime, so one transient upstream
outage broke team resolution on /users/me and team search until a restart. Only a
successful load is committed; a failure degrades that call and the next retries.
Project-contact lookup was returning a validation error unconditionally. Its scan
window was 200 while the pagination it goes through rejects anything over 50, so
GET /projects/{id}/contacts/{contactId} failed before reaching upstream. The scan
limit is now pinned to that cap.
Catalogue searches echoed the page length as the limit, so a 10-row result for a
requested page size of 50 reported limit 10 and told a caller paging by limit that
the page size had shrunk. Offset, effective page size and slice length are now
returned separately.
User-search filters reject malformed ids and oversized group and team arrays at
the boundary instead of forwarding them upstream, and a malformed user id is no
longer reported as a missing one.
The optional project-contact id is a pointer, so no linked contact record is nil
rather than an empty string. The wire shape is unchanged: both are omitted.
Spec catches up with the code: the resolved team on the identity response, bounds
on the group and team filters, and a catalogue pagination schema carrying the
defaults the catalogue endpoints actually apply.
Registry tests clear package state on exit as well as entry, with a test that
asserts a clean start so the cleanup stays load-bearing under -shuffle.
GetUser and GetProjectContact only rejected empty path ids. Both endpoints declare every path parameter as format: uuid and the entity service rejects non-UUID ids, so a malformed id cost a pointless upstream round trip and came back as an upstream-mapped status instead of a local 400. Both now use the package-level uuidRe guard already used by every other path-scoped handler, returning ErrMsgInvalidUUID. Tests assert the 400, the error message, and that the entity stub was never called.
…tions#1287 - Fall back to the canonical UserReference email at every author/uploader link site (comment author, attachment uploader x3, case watcher) so a person with a null canonical id but no legacy email field still resolves a profile link; add a regression test for the null-id/no-legacy-email path. - Make PersonRef.id nullable to match the documented contract for unresolved account manager/technical owner/renewal manager references. - Hide the deactivation-date field on the account detail page when the account isn't deactivated, instead of mislabeling it "Deactivated on". - Extract the duplicated ClosureStateChip into a shared component under csm-projects/components, parameterising the empty-state fallback so each page keeps its own markup convention.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/csm-portal/backend/internal/handler/reference_test.go (1)
168-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen the not-found mapping assertion.
This test is named "maps an upstream not-found," but it returns a bare
errors.New("not found")and only assertsw.Code != http.StatusOK.mapUpstreamErrorlikely type-switches on typed error values; a plainerrors.Newmay not map tohttp.StatusNotFoundat all, yet this assertion still passes for any non-200 status, including a generic 500.Use the
upstreamErrors(fallback)helper referenced by path instructions for standard handler tests, and assert the specific expected status.🔧 Proposed fix
t.Run("maps an upstream not-found", func(t *testing.T) { + notFoundErr := upstreamErrors(t, "Failed to fetch the user.") h := NewUsersHandler(&mockSCIMClient{}, &mockEntityUserClient{ getUserFn: func(_ context.Context, _ string) ([]byte, error) { - return nil, errors.New("not found") + return nil, notFoundErr.notFound }, }) r := withUser(httptest.NewRequest(http.MethodGet, "/users/"+testUserID, nil)) r.SetPathValue("id", testUserID) w := httptest.NewRecorder() h.GetUser(w, r) - if w.Code == http.StatusOK { - t.Fatal("status = 200, want an error status") - } + assertStatus(t, w, http.StatusNotFound) })Adjust field access to match the actual
upstreamErrorsreturn shape inhelpers_test.go.As per path instructions: "Use
upstreamErrors(fallback),withUser(), anddecodeJSON[T]()for standard handler tests, and use real UUIDs for UUID path parameters."🤖 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/reference_test.go` around lines 168 - 181, Update the “maps an upstream not-found” test to return the typed not-found error from upstreamErrors(fallback) instead of errors.New("not found"), matching the helper’s actual return shape. Assert that h.GetUser responds with http.StatusNotFound rather than merely checking it is not OK, while preserving the existing withUser request setup.Source: Path instructions
🤖 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 `@entity-service/openapi.yaml`:
- Around line 2964-2982: Update the UserTeam.family schema to match
normalizeAbtFamily’s domain output by widening its enum to include every
accepted family value, or removing the enum while retaining type: string. Keep
the existing optional/empty behavior and description intact.
---
Outside diff comments:
In `@apps/csm-portal/backend/internal/handler/reference_test.go`:
- Around line 168-181: Update the “maps an upstream not-found” test to return
the typed not-found error from upstreamErrors(fallback) instead of
errors.New("not found"), matching the helper’s actual return shape. Assert that
h.GetUser responds with http.StatusNotFound rather than merely checking it is
not OK, while preserving the existing withUser request setup.
🪄 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 Plus
Run ID: abf03946-41d5-49bd-9e3b-9f0af45a2f6f
📒 Files selected for processing (23)
apps/csm-portal/backend/internal/handler/projects.goapps/csm-portal/backend/internal/handler/reference_test.goapps/csm-portal/backend/internal/handler/users.goapps/csm-portal/webapp/src/features/csm-accounts/pages/CsmAccountDetailPage.tsxapps/csm-portal/webapp/src/features/csm-accounts/types/csmAccounts.tsapps/csm-portal/webapp/src/features/csm-cases/components/CaseActivitiesFeed.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.tsxapps/csm-portal/webapp/src/features/csm-projects/components/ClosureStateChip.tsxapps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectDetailPage.tsxapps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectsPage.tsxentity-service/internal/domain/abt_team.goentity-service/internal/domain/abt_team_test.goentity-service/internal/domain/entity.goentity-service/internal/service/catalog_pagination_test.goentity-service/internal/service/role_service.goentity-service/internal/service/sn_project_service.goentity-service/internal/service/sn_project_service_test.goentity-service/internal/service/sn_user_directory_test.goentity-service/internal/service/sn_user_service.goentity-service/internal/service/team_service.goentity-service/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.tsx
- apps/csm-portal/backend/internal/handler/projects.go
- apps/csm-portal/webapp/src/features/csm-accounts/pages/CsmAccountDetailPage.tsx
- entity-service/internal/service/team_service.go
- apps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectsPage.tsx
- apps/csm-portal/webapp/src/features/csm-cases/components/CaseActivitiesFeed.tsx
- entity-service/internal/service/role_service.go
- apps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.test.tsx
- apps/csm-portal/webapp/src/features/csm-accounts/types/csmAccounts.ts
- apps/csm-portal/webapp/src/features/csm-projects/pages/CsmProjectDetailPage.tsx
- entity-service/internal/service/sn_project_service.go
- apps/csm-portal/backend/internal/handler/users.go
- apps/csm-portal/webapp/src/features/csm-cases/components/CsmCaseCommentBubble.tsx
- entity-service/internal/domain/entity.go
- entity-service/internal/service/sn_user_service.go
The registry's family vocabulary is owned upstream and open: the parser passes any unrecognised value through lowercased so a new family cannot break registry parsing. The spec's two-value enum contradicted that, so it would have rejected data the service is designed to emit. Dropped the enum rather than widening it, since a fixed list would recreate the same problem on the next new value.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Rashmika998
approved these changes
Aug 1, 2026
This was referenced Aug 1, 2026
Merged
This was referenced Aug 3, 2026
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.
Purpose
Backend groundwork for the CSM portal's user profile page and its role / group / team
directory pages, plus the project and account detail rework that landed alongside it.
Depends on a corresponding entity-service change in the private repo, tracked separately: the
team registry endpoint, group-membership lookup by user, a project-contact diagnostic view,
and the
userIds/mobilePhone/userTypeadditions to the user shapes. Those areadditive at the backing data source — no existing response field or behaviour changed.
New endpoints
GET /users/{id}— one user's full profile: the row plus every group they belong to,the subset of those that are teams, and for external contacts their per-project access with
the reason access is or is not granted. Built on the search's
userIdsfilter becausethere is no get-by-id upstream. A deactivated user is still returned: that state is usually
the answer the caller is looking for.
POST /roles/search— the curated role catalogue, served from the same map thatvalidates the
roleIdsfilter, so the dropdown and the filter cannot disagree.Deliberately not a query against the backing source: it carries dozens of
product-shipped roles that mean nothing here, with ids that differ per environment.
POST /teams/search— the team registry, fetched upstream at runtime. A team's id isits registry key, not the backing group's id, because group ids differ between
environments while the key does not.
Changed endpoints
POST /users/searchgainsuserIds,groupIdsandteamIds, and renamesrolestoroleIds. Group and team membership cannot be expressed as an upstream user-search filter— that source cannot join users against group membership in one query — so both resolve to
a user-id set here before a single upstream call, keeping paging and totals server-side.
When a membership filter resolves to nobody the response is an empty page: sending no
id filter instead would return every user, which reads as "everyone matched".
GET /users/meresolves the caller's team via a live group-membership lookup. Teamnames are never hardcoded here — the registry is owned upstream and fetched at runtime,
since it is org vocabulary that does not belong in this repo.
mobilePhoneanduserType. Only the mobile number is surfaced: itis the number the on-call escalation flow dials.
userTypeis derived upstream from rolemembership. Note for consumers: the two data sources name the non-staff value
differently — postgres emits
customer, the other source emitsexternal. Anythingbranching on staff-vs-not must treat both alike.
timecard_approver, which the upstream accepts but this side could notpreviously filter on.
Canonical user reference (
{id, email, name})Every person-valued field across the stack now carries a canonical user reference as a
sibling of whatever it returned before:
Motivation: the webapp needs to link a byline to that person's profile page, and it had no id
to link with. The incumbent
createdBy.idholds an email, not an id, so it could not berepurposed without silently flipping its meaning under every existing reader.
Ten sites across eight schemas:
createdByUseron the case creator, case and generic comments,conversation messages, attachments and activity-feed entries;
assignedEngineerUseron the caseread and search;
assignedToUseron the casePATCHresponse; anduseron watch-list entries.idis nullable, on purposeThe backing data source supplies a real id only where it already resolved the user record for
some other reason. Filling in the rest would mean an extra per-row lookup on hot list
endpoints, in a layer shared with a live production consumer — a cost that was weighed and
rejected. So:
null: case creator, activity-feed actors, watch-list entries.The id is a pointer serialized without
omitempty, so an unknown id is an explicitnullrather than a missing key — an absent key would be ambiguous with "the producer is older".
Watch-list entries keep a null id even though the row carries one: that list collapses several
upstream lists, so an entry is not provably a user (it may name a group), and handing a
consumer a possibly-non-user id is worse than handing it nothing.
The webapp closes the gap itself: unresolved emails on a page are collected and resolved
through one batched
POST /users/search, cached per email so every byline for the sameperson shares one result. Non-email actors (a bot,
system) are filtered out before the requestand cost nothing. A name renders immediately as plain text and becomes a link only once an id is
known, so resolution never blocks or breaks a byline.
Every layer deploys independently, in any order
Nothing is removed or retyped anywhere — the object is always additive. So this side works
against a data source that does not yet return it (field absent →
id: null→ the webappresolves it), and a data source that returns it works against an older consumer (unknown key,
ignored). No coordinated deploy, and no regression surface on the shared production path.
Also in this branch
UserReferenceschema on both specs./people/:emailto/people/:id, and every userreference in the UI now links through a single component so there is one place that decides
what a person link is.
Design notes
empty list rather than failing the whole profile, matching how the caller's own team is
resolved on
GET /users/me. A partial profile is more useful than an error page.projectIdsfilter on the user search and no publicproject-contact or group-membership endpoint here. "Who is on this project" is already
answered by the project contacts search; the membership and contact-diagnostic calls are
internal, used to assemble
GET /users/{id}.Verification
go build,go vetandgo test ./...clean, before and after the rebase ontomain.userIdsintersectingrather than unioning with a membership filter, an unknown team key erroring instead of
silently matching nothing, group/team enrichment, external project access including rows
that grant nothing, graceful degradation when an enrichment call fails, and 404 on an
unknown id.
GET /users/{id}againstGET /users/meverified against a real muxrather than assumed.
was written.
Full-stack round for the canonical user reference
Run against the real DEV instance through the whole vertical (webapp → backend →
entity-service → upstream), not just unit tests:
GET /users/{id}returns 200 for ids taken from three different actor sites (a commentauthor, an attachment uploader, a case assignee). This was the check most likely to expose a
wrong id space — the id embedded in a byline is exactly what the profile route accepts.
createdByUser.idisnullresolves from the email and its/people/<id>link loads the right profile.POST /users/searchper case-detail load, verified off the wire on twoindependent cases — the batching holds rather than degrading to one request per actor.
system) renders as plain text with no link and triggers no lookup.case-detail load returned 200.
Unit coverage for the new field includes the upstream-supplies-it, upstream-says-null and
upstream-omits-it-entirely cases (the backward-compatibility guarantee), the id conversion
asserted against its expected value rather than just non-empty, and a wire-format assertion that
"id":nullliterally appears in the serialized response — a struct-level test would not provethe contract.
Not covered: the postgres data source's path for this field has unit coverage but was not
exercised live; only the other vertical was.
Summary by CodeRabbit
New Features
Bug Fixes