Manage user groups in UserEdit (getredash#3450)

client/app/assets/less/ant.less

@@ -20,6 +20,7 @@
 @import '~antd/lib/tag/style/index';
 @import '~antd/lib/grid/style/index';
 @import '~antd/lib/switch/style/index';
+@import '~antd/lib/empty/style/index';
 @import '~antd/lib/drawer/style/index';
 @import '~antd/lib/divider/style/index';
 @import '~antd/lib/dropdown/style/index';

client/app/components/dynamic-form/DynamicForm.jsx

@@ -7,6 +7,7 @@ import Checkbox from 'antd/lib/checkbox';
 import Button from 'antd/lib/button';
 import Upload from 'antd/lib/upload';
 import Icon from 'antd/lib/icon';
+import Select from 'antd/lib/select';
 import notification from '@/services/notification';
 import { includes } from 'lodash';
 import { react2angular } from 'react2angular';
@@ -132,6 +133,25 @@ export const DynamicForm = Form.create()(class DynamicForm extends React.Compone
     return getFieldDecorator(name, fileOptions)(upload);
   }
 
+  renderSelect(field, props) {
+    const { getFieldDecorator } = this.props.form;
+    const { name, options, mode, initialValue, readOnly, loading } = field;
+    const { Option } = Select;
+
+    const decoratorOptions = {
+      rules: fieldRules(field),
+      initialValue,
+    };
+
+    return getFieldDecorator(name, decoratorOptions)(
+      <Select {...props} optionFilterProp="children" loading={loading || false} mode={mode}>
+        {options && options.map(({ value, title }) => (
+          <Option key={`${value}`} value={value} disabled={readOnly}>{ title || value }</Option>
+        ))}
+      </Select>,
+    );
+  }
+
   renderField(field, props) {
     const { getFieldDecorator } = this.props.form;
     const { name, type, initialValue } = field;
@@ -147,6 +167,10 @@ export const DynamicForm = Form.create()(class DynamicForm extends React.Compone
       return getFieldDecorator(name, options)(<Checkbox {...props}>{fieldLabel}</Checkbox>);
     } else if (type === 'file') {
       return this.renderUpload(field, props);
+    } else if (type === 'select') {
+      return this.renderSelect(field, props);
+    } else if (type === 'content') {
+      return field.content;
     } else if (type === 'number') {
       return getFieldDecorator(name, options)(<InputNumber {...props} />);
     }

client/app/components/proptypes.js

@@ -34,12 +34,30 @@ export const RefreshScheduleDefault = {
 export const Field = PropTypes.shape({
   name: PropTypes.string.isRequired,
   title: PropTypes.string,
-  type: PropTypes.oneOf(['text', 'email', 'password', 'number', 'checkbox', 'file']).isRequired,
-  initialValue: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool]),
+  type: PropTypes.oneOf([
+    'text',
+    'email',
+    'password',
+    'number',
+    'checkbox',
+    'file',
+    'select',
+    'content',
+  ]).isRequired,
+  initialValue: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.number,
+    PropTypes.bool,
+    PropTypes.arrayOf(PropTypes.string),
+    PropTypes.arrayOf(PropTypes.number),
+  ]),
+  content: PropTypes.node,
+  mode: PropTypes.string,
   required: PropTypes.bool,
   readOnly: PropTypes.bool,
   minLength: PropTypes.number,
   placeholder: PropTypes.string,
+  loading: PropTypes.bool,
   props: PropTypes.object, // eslint-disable-line react/forbid-prop-types
 });
 

client/app/components/users/UserEdit.jsx

@@ -1,9 +1,12 @@
 import React, { Fragment } from 'react';
+import { includes } from 'lodash';
 import Alert from 'antd/lib/alert';
 import Button from 'antd/lib/button';
 import Form from 'antd/lib/form';
 import Modal from 'antd/lib/modal';
+import Tag from 'antd/lib/tag';
 import { User } from '@/services/user';
+import { Group } from '@/services/group';
 import { currentUser } from '@/services/auth';
 import { absoluteUrl } from '@/services/utils';
 import { UserProfile } from '../proptypes';
@@ -20,13 +23,24 @@ export default class UserEdit extends React.Component {
     super(props);
     this.state = {
       user: this.props.user,
+      groups: [],
+      loadingGroups: true,
       regeneratingApiKey: false,
       sendingPasswordEmail: false,
       resendingInvitation: false,
       togglingUser: false,
     };
   }
 
+  componentDidMount() {
+    Group.query((groups) => {
+      this.setState({
+        groups: groups.map(({ id, name }) => ({ value: id, title: name })),
+        loadingGroups: false,
+      });
+    });
+  }
+
   changePassword = () => {
     ChangePasswordDialog.showModal({ user: this.props.user });
   };
@@ -102,8 +116,9 @@ export default class UserEdit extends React.Component {
     });
   };
 
-  renderBasicInfoForm() {
-    const { user } = this.state;
+  renderUserInfoForm() {
+    const { user, groups, loadingGroups } = this.state;
+
     const formFields = [
       {
         name: 'name',
@@ -117,7 +132,22 @@ export default class UserEdit extends React.Component {
         type: 'email',
         initialValue: user.email,
       },
-    ].map(field => ({ ...field, readOnly: user.isDisabled, required: true }));
+      (!user.isDisabled && currentUser.id !== user.id) ? {
+        name: 'group_ids',
+        title: 'Groups',
+        type: 'select',
+        mode: 'multiple',
+        options: groups,
+        initialValue: groups.filter(group => includes(user.groupIds, group.value)).map(group => group.value),
+        loading: loadingGroups,
+        placeholder: loadingGroups ? 'Loading...' : '',
+      } : {
+        name: 'group_ids',
+        title: 'Groups',
+        type: 'content',
+        content: this.renderUserGroups(),
+      },
+    ].map(field => ({ readOnly: user.isDisabled, required: true, ...field }));
 
     return (
       <DynamicForm
@@ -128,6 +158,20 @@ export default class UserEdit extends React.Component {
     );
   }
 
+  renderUserGroups() {
+    const { user, groups, loadingGroups } = this.state;
+
+    return loadingGroups ? 'Loading...' : (
+      <div data-test="Groups">
+        {groups.filter(group => includes(user.groupIds, group.value)).map((group => (
+          <Tag className="m-b-5 m-r-5" key={group.value}>
+            <a href={`groups/${group.value}`}>{group.title}</a>
+          </Tag>
+        )))}
+      </div>
+    );
+  }
+
   renderApiKey() {
     const { user, regeneratingApiKey } = this.state;
 
@@ -227,7 +271,7 @@ export default class UserEdit extends React.Component {
         />
         <h3 className="profile__h3">{user.name}</h3>
         <hr />
-        {this.renderBasicInfoForm()}
+        {this.renderUserInfoForm()}
         {!user.isDisabled && (
           <Fragment>
             {this.renderApiKey()}

client/app/components/users/UserShow.jsx

@@ -1,30 +1,66 @@
 import React from 'react';
+import { includes } from 'lodash';
+import Tag from 'antd/lib/tag';
+import { Group } from '@/services/group';
 import { UserProfile } from '../proptypes';
 
-export default function UserShow({ user: { name, email, profileImageUrl } }) {
-  return (
-    <div className="col-md-4 col-md-offset-4 profile__container">
-      <img
-        alt="profile"
-        src={profileImageUrl}
-        className="profile__image"
-        width="40"
-      />
-
-      <h3 className="profile__h3">{name}</h3>
-
-      <hr />
-
-      <dl className="profile__dl">
-        <dt>Name:</dt>
-        <dd>{name}</dd>
-        <dt>Email:</dt>
-        <dd>{email}</dd>
-      </dl>
-    </div>
-  );
-}
+export default class UserShow extends React.Component {
+  static propTypes = {
+    user: UserProfile.isRequired,
+  };
+
+  constructor(props) {
+    super(props);
+    this.state = { groups: [], loadingGroups: true };
+  }
+
+  componentDidMount() {
+    Group.query((groups) => {
+      this.setState({ groups, loadingGroups: false });
+    });
+  }
+
+  renderUserGroups() {
+    const { groupIds } = this.props.user;
+    const { groups } = this.state;
+
+    return (
+      <div>
+        {groups.filter(group => includes(groupIds, group.id)).map((group => (
+          <Tag className="m-t-5 m-r-5" key={group.id}>
+            <a href={`groups/${group.id}`}>{group.name}</a>
+          </Tag>
+        )))}
+      </div>
+    );
+  }
 
-UserShow.propTypes = {
-  user: UserProfile.isRequired,
-};
+  render() {
+    const { name, email, profileImageUrl } = this.props.user;
+    const { loadingGroups } = this.state;
+
+    return (
+      <div className="col-md-4 col-md-offset-4 profile__container">
+        <img
+          alt="profile"
+          src={profileImageUrl}
+          className="profile__image"
+          width="40"
+        />
+
+        <h3 className="profile__h3">{name}</h3>
+
+        <hr />
+
+        <dl className="profile__dl">
+          <dt>Name:</dt>
+          <dd>{name}</dd>
+          <dt>Email:</dt>
+          <dd>{email}</dd>
+          <dt>Groups:</dt>
+          <dd>{loadingGroups ? 'Loading...' : this.renderUserGroups()}</dd>
+        </dl>
+      </div>
+    );
+  }
+}

client/app/components/users/UserShow.test.js

@@ -1,12 +1,18 @@
 import React from 'react';
 import renderer from 'react-test-renderer';
+import { Group } from '@/services/group';
 import UserShow from './UserShow';
 
+beforeEach(() => {
+  Group.query = jest.fn(dataCallback => dataCallback([]));
+});
+
 test('renders correctly', () => {
   const user = {
     id: 2,
     name: 'John Doe',
     email: 'john@doe.com',
+    groupIds: [],
     profileImageUrl: 'http://www.images.com/llama.jpg',
   };
 

client/app/components/users/__snapshots__/UserShow.test.js.snap

@@ -31,6 +31,12 @@ exports[`renders correctly 1`] = `
     <dd>
       john@doe.com
     </dd>
+    <dt>
+      Groups:
+    </dt>
+    <dd>
+      <div />
+    </dd>
   </dl>
 </div>
 `;

client/app/services/group.js

@@ -1,4 +1,4 @@
-export let Group = null; // eslint-disable-line import/no-mutable-exports
+export let Group = {}; // eslint-disable-line import/no-mutable-exports
 
 function GroupService($resource) {
   const actions = {

client/app/services/user.js

@@ -66,6 +66,7 @@ function convertUserInfo(user) {
     email: user.email,
     profileImageUrl: user.profile_image_url,
     apiKey: user.api_key,
+    groupIds: user.groups,
     isDisabled: user.is_disabled,
     isInvitationPending: user.is_invitation_pending,
   };

client/cypress/integration/user/edit_profile_spec.js

@@ -41,6 +41,8 @@ describe('Edit Profile', () => {
       $apiKey.val('secret');
     });
 
+    cy.getByTestId('Groups').should('contain', 'admin');
+
     cy.percySnapshot('User Profile');
   });
 

redash/handlers/users.py

@@ -4,6 +4,7 @@
 from flask_restful import abort
 from flask_login import current_user, login_user
 from funcy import project
+from sqlalchemy.orm.exc import NoResultFound
 from sqlalchemy.exc import IntegrityError
 from disposable_email_domains import blacklist
 from funcy import partial
@@ -207,7 +208,7 @@ def post(self, user_id):
 
         req = request.get_json(True)
 
-        params = project(req, ('email', 'name', 'password', 'old_password', 'groups'))
+        params = project(req, ('email', 'name', 'password', 'old_password', 'group_ids'))
 
         if 'password' in params and 'old_password' not in params:
             abort(403, message="Must provide current password to update password.")
@@ -219,8 +220,18 @@ def post(self, user_id):
             user.hash_password(params.pop('password'))
             params.pop('old_password')
 
-        if 'groups' in params and not self.current_user.has_permission('admin'):
-            abort(403, message="Must be admin to change groups membership.")
+        if 'group_ids' in params:
+            if not self.current_user.has_permission('admin'):
+                abort(403, message="Must be admin to change groups membership.")
+
+            for group_id in params['group_ids']:
+                try:
+                    models.Group.get_by_id_and_org(group_id, self.current_org)
+                except NoResultFound:
+                    abort(400, message="Group id {} is invalid.".format(group_id))
+
+            if len(params['group_ids']) == 0:
+                params.pop('group_ids')
 
         if 'email' in params:
             _, domain = params['email'].split('@', 1)

tests/handlers/test_users.py

@@ -307,6 +307,15 @@ def test_changing_email_does_not_end_current_session(self):
         # make sure the session's `user_id` has changed to reflect the new identity, thus not logging the user out
         self.assertNotEquals(previous, current)
 
+    def test_admin_can_change_user_groups(self):
+        admin_user = self.factory.create_admin()
+        other_user = self.factory.create_user(group_ids=[1])
+
+        rv = self.make_request('post', "/api/users/{}".format(other_user.id), data={"group_ids": [1, 2]}, user=admin_user)
+
+        self.assertEqual(rv.status_code, 200)
+        self.assertEqual(models.User.query.get(other_user.id).group_ids, [1,2])
+
     def test_admin_can_delete_user(self):
         admin_user = self.factory.create_admin()
         other_user = self.factory.create_user(is_invitation_pending=True)