feat(xo-server/rest-api): ability to update group and its users #8278
Conversation
| json(), | ||
| wrap(async (req, res) => { | ||
| await collections.groups.actions.update(req.body, req) | ||
| res.sendStatus(200) |
There was a problem hiding this comment.
If you try to update a group with an ID that doesn't exist, does it return an error code?
| collections.groups.actions = { | ||
| update: withParams( | ||
| async ({ id, name, userIds }, req) => { | ||
| const group = await app.getGroup(id) | ||
| if (group.provider !== undefined) { | ||
| throw new Error('Cannot edit synchronized group') | ||
| } | ||
|
|
||
| if (name !== undefined) { | ||
| await app.updateGroup(id, { name }) | ||
| } | ||
|
|
||
| if (Array.isArray(userIds)) { | ||
| await app.setGroupUsers(id, userIds) | ||
| } | ||
| }, | ||
| { | ||
| id: { type: 'string' }, | ||
| name: { type: 'string', optional: true }, | ||
| userIds: { type: 'array', items: { type: 'string' }, optional: true }, | ||
| } | ||
| ), | ||
| } |
There was a problem hiding this comment.
Don't create action like that. Same as: #8276 (comment)
| collections.groups.actions = { | |
| update: withParams( | |
| async ({ id, name, userIds }, req) => { | |
| const group = await app.getGroup(id) | |
| if (group.provider !== undefined) { | |
| throw new Error('Cannot edit synchronized group') | |
| } | |
| if (name !== undefined) { | |
| await app.updateGroup(id, { name }) | |
| } | |
| if (Array.isArray(userIds)) { | |
| await app.setGroupUsers(id, userIds) | |
| } | |
| }, | |
| { | |
| id: { type: 'string' }, | |
| name: { type: 'string', optional: true }, | |
| userIds: { type: 'array', items: { type: 'string' }, optional: true }, | |
| } | |
| ), | |
| } |
| api.patch( | ||
| '/groups/update/:id', | ||
| json(), | ||
| wrap(async (req, res) => { | ||
| await collections.groups.actions.update(req.body, req) | ||
| res.sendStatus(200) | ||
| }) | ||
| ) |
There was a problem hiding this comment.
IMHO this can be split into 2 routes.
Update group name:
PATCH /:collection(groups)/:id
request body: {name?: string}
Add user to group:
POST /:collection(groups)/:id/users/:id
First route we update a property of the group collection
Second we add a user to the users subresource of the group collection
| const group = await app.getGroup(id) | ||
| if (group.provider !== undefined) { | ||
| throw new Error('Cannot edit synchronized group') | ||
| } | ||
|
|
||
| if (name !== undefined) { | ||
| await app.updateGroup(id, { name }) | ||
| } |
There was a problem hiding this comment.
Avoid adding business logic into the REST API code.
consider the REST API as much as possible as an interface that exposes xo-servers methods via HTTP requests.
IMHO, you can move this logic to xo-mixins/subjects.mjs#updateGroup.
@fbeauchamp What do you think?
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| > Users must be able to say: “Nice enhancement, I'm eager to test it” | |||
|
|
|||
| - [REST API] Ability to update a group's name and its users (PR [#8278](https://github.com/vatesfr/xen-orchestra/pull/8278)) | |||
There was a problem hiding this comment.
I don't know if this message is clear enough. It can be confusing, for example: "you can update the properties of users in the group".
this PR expose the possibility to ADD a user in a group
There was a problem hiding this comment.
What about "Allow updating a group's name and manage its users list (add/remove users)" ?
Description
Short explanation of this PR (feel free to re-use commit message)
Checklist
Fixes #007,See xoa-support#42,See https://...)Introduced byCHANGELOG.unreleased.mdReview process
Notes: