User Groups: Add ability to manage users directly from the group workspace#22215
Conversation
There was a problem hiding this comment.
I've only given this a quick look-over @NguyenThuyLan, so you have some feedback for the start of your working day. Please see what you think on my comments. I'll try to pull it down for some testing when I'm in tomorrow.
…7/feature/manage-users-from-group
…7/feature/manage-users-from-group
|
Done, I separated the user list into a separate element, so when adding or removing users from the list, it will be saved instantly |
AndyButland
left a comment
There was a problem hiding this comment.
Looks great and works correctly @NguyenThuyLan.
I see now that adding or removing users from the list happens immediately - I don't have to press "Save". From the discussions I've seen you've had with @nielslyngsoe , that looks as expected. Longer term I think it would be better if this occurred on "Save" though, as the "Users" list is within the workspace and looks like it should be persisted only when you save.
Comments to consider are mostly around the edge-case of > 100 users in the group.
…com/umbraco/Umbraco-CMS into v17/feature/manage-users-from-group
…com/umbraco/Umbraco-CMS into v17/feature/manage-users-from-group
…7/feature/manage-users-from-group
AndyButland
left a comment
There was a problem hiding this comment.
Just one tiny thing then I would say this looks good to me, on the understanding that the intention is to be able to provide a paged list further down the line. It will already meet the need for most projects.
I'll approve but think this needs @nielslyngsoe to look over too before merging.
…com/umbraco/Umbraco-CMS into v17/feature/manage-users-from-group
…7/feature/manage-users-from-group
|
Thanks @nielslyngsoe ,everything has been updated. Please let me know if anything else is needed |
|
Changes look good to me, so please have someone verifying that it works as expected :-) |
|
I'll take one further look over from a testing perspective @NguyenThuyLan - than I take it from your comment above @nielslyngsoe that this is OK to merge. |
|
@AndyButland I was giving this a quick review, (@NguyenThuyLan asked if I had time). Overall, I'm happy with it, but had the same thoughts as you outlined about the "Save" (#22215 (review)) and pagination (#22215 (review)). If these are going to be address later, then all good. |
|
OK, if you've tested it out @leekelleher we can merge this in my view. As I understand we need a bit more plumbing to allow other actions to hook into the "Save" for a workspace before we can support delegating the save to this operation. |
Prerequisites
If there's an existing issue for this PR then this fixes
Description
Test plan