Manage user groups in UserEdit

[gabrieldutra]

Description

The idea here is to improve UX for editing user groups by adding an option to manage this in the user edit page.

Some discussion

Changes

• Update user handler to modify groups (groups seemed not be be a user attr, group_ids was instead)
• Introduce SelectField in DynamicForm (it may be useful in the future as well)
• Manage User Groups in UserEdit - 👌

Possible stuff to change depending on comments

• Refactor Field proptypes (it seems to work, but it's not creating much better check than an Object propType)
• Separate DynamicForm Fields into smaller components (DynamicFormUploadField, DynamicFormSelectField) instead of render methods

Preview (Admin editing other user)

[arikfr]

Nice!

• When we don't render the select field, we should render the list of groups the user belongs to.
• We shouldn't allow removing all groups from a user, a user should always have at least 1 group association.
• How hard will it be to add this to the create user form?

Besides that, see comments inline.

Thanks :)

[arikfr]

Need to check that the group ids belong to the current org.

[gabrieldutra]

We should rather abort with an error message or simply remove the non-belonging groups? (in frontend side I guess those groups won't even appear, this is more to guarantee the behavior)

• We shouldn't allow removing all groups from a user, a user should always have at least 1 group association.

I guess I should add the rule here in the backend as well (frontend it was as easy as putting the field required)

[gabrieldutra]

@arikfr, please check and consider me inexperienced in those parts 😂

[arikfr]

Considering that it might take some time to load the groups, maybe have some loading state for the data? I know Ant's Select components have support for this, so it's only a matter of communicating this between the components.

[gabrieldutra]

Cool, locally it's already done, I also added a "Loading..." placeholder. (better than a whole blank select imo)

[gabrieldutra]

• When we don't render the select field, we should render the list of groups the user belongs to.

I don't think this should be in the form, but I'll try to come up with a place to put it. (perhaps below or beside the heading as tags).

• How hard will it be to add this to the create user form?

In case the route for creating a user is pretty similar to this one I would say this will be easy once we migrate the page. I would separate the renderUserInfoForm method into a new component that could be used in both places. (Even without this new component, the use of DynamicForm brings it only a bit more of effort in case there will be different fields)

[ranbena]

I tested the feature on netlify and found the following:

1. Save not working
   Add group -> save -> refresh page - changes not saved.

2. Autocomplete not working
   Focus on select ->"admin" group shows in suggestions -> type "a" -> no suggestions (should show "admin")

[gabrieldutra]

1. Save not working
   Add group -> save -> refresh page - changes not saved.

I suppose this won't work on netlify since there are some backend changes on the user handler.

2. Autocomplete not working
   Focus on select ->"admin" group shows in suggestions -> type "a" -> no suggestions (should show "admin")

Good catch, I didn't notice this one, by default Ant Select filter works with Option values (in this case they are the group ids). Changing optionFilterProp="children" fixed that though.

Once I fix Arik's comments I'll comment here 🙂

[gabrieldutra]

A disabled select looks bad, but why not just a tag list?

Can we just render the labels there like you do on the show component?

I had assumed this would be in the form (and considering I'm currently using DynamicForm, that would require a bigger change) 😅 .

Percy view for that:

BTW, I don't know exactly how to handle UserShow jest tests now (after this, it now requires Angular 🤔).

[arikfr]

I had assumed this would be in the form (and considering I'm currently using DynamicForm, that would require a bigger change) 😅 .

Two options:

1. Add a simple content type field, which takes a render function, and renders whatever the function returns.
2. Just skip DynamicForm here and implement the form directly. Although option 1 sounds simple enough.

BTW, I don't know exactly how to handle UserShow jest tests now (after this, it now requires Angular 🤔).

Why?

[gabrieldutra]

1. Add a simple content type field, which takes a render function, and renders whatever the function returns.

Indeed not a bad idea, I totally forgot about those tricks... Thanks! 🙂

Why?

Because of this. I'm not sure if I'm missing something, but my first guess was that the Group was null because the ngModule didn't init.

[arikfr]

Because of this. I'm not sure if I'm missing something, but my first guess was that the Group was null because the ngModule didn't init.

Oh, right. Maybe stub it until we have a better solution?

[arikfr]

Is this ready?

[gabrieldutra]

Is this ready?

Yes, I've just updated with master :)

[ranbena]

LGTM 👍

One thing perhaps is to swap

<div class="ant-tag other-classnames">
  <a href="">Group Name</a>
</div>

for

<a href="" class="ant-tag other-classnames">Group Name</a>

so that it could have a hover state to better indicate that it's linkable.

[gabrieldutra]

One thing perhaps is to swap [...]

I'm currently using the Tag component for that, so I guess in this case I'd have to stop using it or use css to style it. The first option doesn't seem good as we would use an Antd component class without the component wrapping it. The css option sounds better to me, so how about I add an override to ant.less?

.ant-tag {
  a:hover {
    color: @somecolor
  }
}

[ranbena]

@gabrieldutra in that case never mind. Leave as is. Not worth the complication. Feel free to merge.