feat(identity): add updateById, deleteById and create to Users service#617
feat(identity): add updateById, deleteById and create to Users service#617Sarath1018 wants to merge 2 commits into
Conversation
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| updateById: vi.fn(), | ||
| deleteById: vi.fn(), | ||
| create: vi.fn(), | ||
| invite: vi.fn(), |
There was a problem hiding this comment.
invite is not part of UserServiceModel in this PR — it arrives in PR #609. Keeping it here is a dead mock that the conventions explicitly forbid:
NEVER leave unused mock methods in mock objects — dead mocks obscure what the test actually exercises and accumulate as the API evolves.
| invite: vi.fn(), | |
| }; |
| * }); | ||
| * ``` | ||
| */ | ||
| updateById(userId: string, options: UserUpdateOptions): Promise<UserUpdateResponse>; |
There was a problem hiding this comment.
Per convention, {Entity}{Operation}Options bags — whose fields are all optional — must also have the parameter itself marked ?:
{Entity}{Operation}Options— bag of optional fields. Always the last parameter. Always marked?. Contains only optional fields.
Same applies to UserMethods.update(options: UserUpdateOptions) at line 163, and the implementation signature in users.ts.
| updateById(userId: string, options: UserUpdateOptions): Promise<UserUpdateResponse>; | |
| updateById(userId: string, options?: UserUpdateOptions): Promise<UserUpdateResponse>; |
| IDENTITY_TEST_USER_ID= | ||
| # Organization GUID (from UiPath Cloud > Admin > Organization Settings) | ||
| # Required by the Identity Users tests (user create operations) | ||
| UIPATH_ORGANIZATION_ID=3aa10965-a82d-4d9e-8366-0eff8e87bf7a |
There was a problem hiding this comment.
A real organization GUID is committed to the example env file (same value also ends up in tests/utils/constants/users.ts). Example files are templates — this should be blank or use an obvious placeholder so no real tenant identifier is embedded in the public repo.
| UIPATH_ORGANIZATION_ID=3aa10965-a82d-4d9e-8366-0eff8e87bf7a | |
| UIPATH_ORGANIZATION_ID= |
Review findingsThree issues found this run:
|
5e31db9 to
3e38f55
Compare
| * | ||
| * `result` reflects the request as a whole; `users` contains the created users. | ||
| */ | ||
| export interface UserCreateResponse { |
There was a problem hiding this comment.
Per convention, response types must use type, not interface:
Always use
typefor response types (intersections, unions, composed types). The only placeinterface extendsis required is single-type aliases (type X = Y), which break TypeDoc.
UserCreateResponse is a plain multi-field object type — it's not a single-type alias extending another type, so the TypeDoc exception doesn't apply.
| export interface UserCreateResponse { | |
| export type UserCreateResponse = { |
and close with }; on line 30.
|
One new issue found this run: UserCreateResponse should use type not interface in src/models/identity/users.models.ts line 25. Convention says to always use type for response types. |
3e38f55 to
b94fcec
Compare
| }); | ||
|
|
||
| it('should throw when the user ID is undefined', async () => { | ||
| const user = createUserWithMethods(createBasicUser({ id: undefined as any }), mockService); |
There was a problem hiding this comment.
as any is unnecessary here. createBasicUser already accepts Partial<RawUserGetResponse>, and Partial<> makes every field optional, so id?: string | undefined — TypeScript accepts undefined without a cast. Convention: "No any type".
Same applies to line 69 (the user.delete() guard test).
| const user = createUserWithMethods(createBasicUser({ id: undefined as any }), mockService); | |
| const user = createUserWithMethods(createBasicUser({ id: undefined }), mockService); |
| } | ||
|
|
||
| /** | ||
| * Error entry from a failed user operation. |
There was a problem hiding this comment.
Convention: "Always use type for response types." UserOperationError and UserOperationResult (line 87) both describe API response shapes and should be type, not interface.
(Note: UserUpdateResponse extends UserOperationResult {} is the correct TypeDoc single-alias exception — that pattern works whether UserOperationResult is an interface or a type.)
| * Error entry from a failed user operation. | |
| export type UserOperationError = { |
|
|
||
| // Organization (partition) GUID — required by create() | ||
| ORGANIZATION_ID: '3aa10965-a82d-4d9e-8366-0eff8e87bf7a', | ||
|
|
There was a problem hiding this comment.
This is the same real production GUID noted in the open thread on tests/.env.integration.example:90. Unit test constants should use invented placeholder UUIDs (like USER_ID and GROUP_ID above), not real tenant/organization identifiers.
| ORGANIZATION_ID: '00000000-0000-0000-0000-000000000001', |
Review findingsThree new issues this run:
|
Second slice of the Identity user management onboarding (follows the getById
scaffold PR):
- updateById(userId, options) PUT /api/User/{userId}
- deleteById(userId) DELETE /api/User/{userId}
- create(users, organizationId, opts?) POST /api/User/BulkCreate
Introduces bound entity methods (user.update(), user.delete()) attached to
getById() and create() results via createUserWithMethods, with model tests
for the delegation.
Live-API finding encoded: BulkCreate returns { result, users[] } with the
full created users, not the bare IdentityResult the Swagger spec declares;
userName is required (email alone is rejected).
The integration suite now self-provisions its fixture user via create() and
cleans up with deleteById() — the transient IDENTITY_TEST_USER_ID env var
from the previous slice is replaced by UIPATH_ORGANIZATION_ID.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The identity suite now self-provisions its fixture user via create(), so the IDENTITY_TEST_USER_ID mapping is replaced by the organization GUID. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b94fcec to
7288fc2
Compare
| const response = await this.post<RawUserCreateResponse>(IDENTITY_USER_ENDPOINTS.BULK_CREATE, { | ||
| users, | ||
| partitionGlobalId: organizationId, | ||
| groupIDs: options?.groupIds, |
There was a problem hiding this comment.
Convention: Outbound requests (SDK → API) must use transformRequest(data, {Entity}Map) to reverse the field map — not inline manual renames. updateById in the same file does this correctly:
transformRequest(options, UserMap)Here the groupIds → groupIDs rename is hard-coded directly. If the map ever changes (or additional options fields are added that need renaming) create() silently diverges.
| groupIDs: options?.groupIds, | |
| ...transformRequest(options ?? {}, UserMap), |
(remove groupIDs: options?.groupIds, on this line and spread the transform result instead)
Review findingsOne new issue this run: create() bypasses transformRequest for outbound mapping (src/services/identity/users.ts:71) — groupIDs: options?.groupIds hard-codes the API field name inline. updateById in the same file correctly delegates to transformRequest(options, UserMap); create should do the same via ...transformRequest(options ?? {}, UserMap) so any future additions to UserMap are picked up automatically. |
1 participant
Summary
PR 2 of 3 splitting the Identity user management onboarding (stacked on #616; #609 is the final slice). Adds the write operations and bound entity methods:
updateById(userId, options)PUT /api/User/{userId}deleteById(userId)DELETE /api/User/{userId}create(users, organizationId, options?)POST /api/User/BulkCreategetById()andcreate()now return users with bound entity methods (user.update(),user.delete()) viacreateUserWithMethods.Design notes
BulkCreatereturns{ result, users[] }with the full created users, not a bareIdentityResult;userNameis required for create (emailalone is rejected withInvalidUserName).transformRequest:groupIds→groupIDs,groupIdsToAdd/Remove→groupIDsToAdd/Remove.organizationIdis a required positional param oncreate()(SDK term for the API'spartitionGlobalId).Testing
tests/unit/models/identity/)create()and cleans up withdeleteById()(replaces the transientIDENTITY_TEST_USER_IDenv var from PR 1 withUIPATH_ORGANIZATION_ID)Stack
getByIdupdateById,deleteById,create+ bound entity methodsfeat/identity-users(feat(identity): add invite to Users service #609) —invite🤖 Generated with Claude Code