Respond to PR feedback.

2026-01-14 03:10:42 -03:30 · 2019-03-05 12:01:06 -05:00 · 2019-03-05 12:01:06 -05:00 · 9f86fc2def
commit 9f86fc2def
parent 35ecd83214
7 changed files with 101 additions and 135 deletions
--- a/tests/components/AccessList.test.jsx
+++ b/tests/components/AccessList.test.jsx
@ -17,20 +17,14 @@ const mockResults = [
        {
          role: {
            name: 'foo',
+            id: 2,
          }
        }
-      ]
+      ],
    }
  }
 ];

-const mockUserRoles = [
-  {
-    id: 2,
-    name: 'bar',
-  }
-];
-
 describe('<AccessList />', () => {
  test('initially renders succesfully', () => {
    mount(
@ -40,7 +34,6 @@ describe('<AccessList />', () => {
            match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '1' } }}
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => {}}
-            getUserRoles={() => {}}
          />
        </MemoryRouter>
      </I18nProvider>
@ -55,7 +48,6 @@ describe('<AccessList />', () => {
            match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }}
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
-            getUserRoles={() => ({ data: { results: mockUserRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
@ -77,7 +69,6 @@ describe('<AccessList />', () => {
            match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }}
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
-            getUserRoles={() => ({ data: { results: mockUserRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
@ -104,7 +95,6 @@ describe('<AccessList />', () => {
            match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }}
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
-            getUserRoles={() => ({ data: { results: mockUserRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
@ -118,4 +108,53 @@ describe('<AccessList />', () => {
      done();
    });
  });
+
+  test('getTeamRoles returns empty array if dataset is missing team_id attribute', (done) => {
+    const mockData = [
+      {
+        id: 1,
+        username: 'boo',
+        url: '/foo/bar/',
+        first_name: 'john',
+        last_name: 'smith',
+        summary_fields: {
+          foo: [
+            {
+              role: {
+                name: 'foo',
+                id: 2,
+              }
+            }
+          ],
+          direct_access: [
+            {
+              role: {
+                name: 'team user',
+                id: 3,
+              }
+            }
+          ]
+        }
+      }
+    ];
+    const wrapper = mount(
+      <I18nProvider>
+        <MemoryRouter>
+          <AccessList
+            match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }}
+            location={{ search: '', pathname: '/organizations/1/access' }}
+            getAccessList={() => ({ data: { count: 1, results: mockData } })}
+          />
+        </MemoryRouter>
+      </I18nProvider>
+    ).find('AccessList');
+
+    setImmediate(() => {
+      const { results } = wrapper.state();
+      results.forEach(result => {
+        expect(result.teamRoles).toEqual([]);
+      });
+      done();
+    });
+  });
 });
--- a/tests/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx
+++ b/tests/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx
@ -7,18 +7,11 @@ import OrganizationAccess from '../../../../../src/pages/Organizations/screens/O
 const mockAPIAccessList = {
  foo: 'bar',
 };
-const mockAPIRoles = {
-  bar: 'baz',
-};

 const mockGetOrganzationAccessList = jest.fn(() => (
  Promise.resolve(mockAPIAccessList)
 ));

-const mockGetUserRoles = jest.fn(() => (
-  Promise.resolve(mockAPIRoles)
-));
-
 describe('<OrganizationAccess />', () => {
  test('initially renders succesfully', () => {
    mount(
@ -29,7 +22,6 @@ describe('<OrganizationAccess />', () => {
          params={{}}
          api={{
            getOrganzationAccessList: jest.fn(),
-            getUserRoles: jest.fn(),
          }}
        />
      </MemoryRouter>
@ -45,14 +37,11 @@ describe('<OrganizationAccess />', () => {
          params={{}}
          api={{
            getOrganzationAccessList: mockGetOrganzationAccessList,
-            getUserRoles: mockGetUserRoles,
          }}
        />
      </MemoryRouter>
    ).find('OrganizationAccess');
    const accessList = await wrapper.instance().getOrgAccessList();
    expect(accessList).toEqual(mockAPIAccessList);
-    const userRoles = await wrapper.instance().getUserRoles();
-    expect(userRoles).toEqual(mockAPIRoles);
  });
 });
--- a/src/api.js
+++ b/src/api.js
@ -5,7 +5,6 @@ const API_V2 = `${API_ROOT}v2/`;
 const API_CONFIG = `${API_V2}config/`;
 const API_ORGANIZATIONS = `${API_V2}organizations/`;
 const API_INSTANCE_GROUPS = `${API_V2}instance_groups/`;
-const API_USERS = `${API_V2}users/`;

 const LOGIN_CONTENT_TYPE = 'application/x-www-form-urlencoded';

@ -130,12 +129,6 @@ class APIClient {
  disassociateInstanceGroup (url, id) {
    return this.http.post(url, { id, disassociate: true });
  }
-
-  getUserRoles (id) {
-    const endpoint = `${API_USERS}${id}/roles/`;
-
-    return this.http.get(endpoint);
-  }
 }

 export default APIClient;
--- a/src/components/AccessList/Access.list.jsx
+++ b/src/components/AccessList/Access.list.jsx
@ -3,7 +3,7 @@ import PropTypes from 'prop-types';

 import {
  DataList, DataListItem, DataListCell, Text,
-  TextContent, TextVariants, Badge
+  TextContent, TextVariants
 } from '@patternfly/react-core';

 import { I18n, i18nMark } from '@lingui/react';
@ -16,7 +16,6 @@ import {
 import BasicChip from '../BasicChip/BasicChip';
 import Pagination from '../Pagination';
 import DataListToolbar from '../DataListToolbar';
-import Tooltip from '../Tooltip';

 import {
  parseQueryString,
@ -47,16 +46,7 @@ const hiddenStyle = {
  display: 'none',
 };

-const badgeStyle = {
-  borderRadius: 'var(--pf-global--BorderRadius--sm)',
-  height: '24px',
-}
-
-const badgeTextStyle = {
-  lineHeight: '24px',
-}
-
-const Detail = ({ label, value, url, isBadge, customStyles }) => {
+const Detail = ({ label, value, url, customStyles }) => {
  let detail = null;
  if (value) {
    detail = (
@ -66,19 +56,29 @@ const Detail = ({ label, value, url, isBadge, customStyles }) => {
            <Text component={TextVariants.h6} style={detailLabelStyle}>{label}</Text>
          </Link>) : (<Text component={TextVariants.h6} style={detailLabelStyle}>{label}</Text>
        )}
-        {isBadge ? (
-          <Badge style={badgeStyle} isRead>
-            <Text component={TextVariants.p} style={{...detailValueStyle, ...badgeTextStyle}}>{value}</Text>
-          </Badge>
-        ) : (
-          <Text component={TextVariants.p} style={detailValueStyle}>{value}</Text>
-        )}
+        <Text component={TextVariants.p} style={detailValueStyle}>{value}</Text>
      </TextContent>
    );
  }
  return detail;
 };

+const UserName = ({ value, url }) => {
+  let username = null;
+  if (value) {
+    username = (
+      <TextContent style={detailWrapperStyle}>
+        {url ? (
+          <Link to={{ pathname: url }}>
+            <Text component={TextVariants.h6} style={detailLabelStyle}>{value}</Text>
+          </Link>) : (<Text component={TextVariants.h6} style={detailLabelStyle}>{value}</Text>
+        )}
+      </TextContent>
+    );
+  }
+  return username;
+};
+
 class AccessList extends React.Component {
  columns = [
    { name: i18nMark('Name'), key: 'first_name', isSortable: true },
@ -112,8 +112,6 @@ class AccessList extends React.Component {
    this.onCompact = this.onCompact.bind(this);
    this.onSort = this.onSort.bind(this);
    this.getQueryParams = this.getQueryParams.bind(this);
-    this.getRoleType = this.getRoleType.bind(this);
-    this.fetchUserRoles = this.fetchUserRoles.bind(this);
    this.getTeamRoles = this.getTeamRoles.bind(this);
  }

@ -173,36 +171,22 @@ class AccessList extends React.Component {
    return Object.assign({}, this.defaultParams, searchParams, overrides);
  }

-  getTeamRoles (arr) {
-    this.arr = arr;
-    const filtered = this.arr.filter(entry => entry.role.team_id);
-    return filtered.reduce((val, item) => {
-      if (item.role.team_id) {
-        const { role } = item;
-        val = role;
+  getRoles = roles => Object.values(roles)
+    .reduce((val, role) => {
+      if (role.length > 0) {
+        val.push(role[0].role);
      }
      return val;
-    }, {});
-  }
+    }, []);

-  getRoleType (arr, index, type) {
-    return Object.values(arr).filter(value => value.length > 0).map(roleType => {
-      if (type === 'user') {
-        return roleType[index].role.name;
+  getTeamRoles = roles => roles
+    .reduce((val, item) => {
+      if (item.role.team_id) {
+        const { role } = item;
+        val.push(role);
      }
-      if (type === 'team') {
-        return this.getTeamRoles(roleType);
-      }
-      return null;
-    });
-  }
-
-  async fetchUserRoles (id) {
-    const { getUserRoles } = this.props;
-    const { data: { results: userRoles = [] } } = await getUserRoles(id);
-
-    return userRoles;
-  }
+      return val;
+    }, []);

  async fetchOrgAccessList (queryParams) {
    const { match, getAccessList } = this.props;
@ -232,25 +216,15 @@ class AccessList extends React.Component {
        sortedColumnKey,
        results,
      };
-
-      results.forEach(async result => {
-        result.userRoles = [];
-        result.teamRoles = [];
-        result.directRole = null;
-
-        // Grab each Role Type and set as a top-level value
-        result.directRole = this.getRoleType(result.summary_fields, 0, 'user') || null;
-        result.teamRoles = this.getRoleType(result.summary_fields, 1, 'team').filter(teamRole => teamRole.id);
-
-        // Grab User Roles and set as a top-level value
-        try {
-          const roles = await this.fetchUserRoles(result.id);
-          roles.map(role => result.userRoles.push(role));
-          this.setState(stateToUpdate);
-        } catch (error) {
-          this.setState({ error });
+      results.forEach((result) => {
+        if (result.summary_fields.direct_access) {
+          result.teamRoles = this.getTeamRoles(result.summary_fields.direct_access);
+        } else {
+          result.teamRoles = [];
        }
+        result.userRoles = this.getRoles(result.summary_fields) || [];
      });
+      this.setState(stateToUpdate);
    } catch (error) {
      this.setState({ error });
    }
@ -301,11 +275,9 @@ class AccessList extends React.Component {
                    {results.map(result => (
                      <DataListItem aria-labelledby={i18n._(t`access-list-item`)} key={result.id}>
                        <DataListCell>
-                          <Detail
-                            label={result.username}
-                            value={result.directRole}
+                          <UserName
+                            value={result.username}
                            url={result.url}
-                            isBadge
                          />
                          {result.first_name || result.last_name ? (
                            <Detail
@ -331,30 +303,12 @@ class AccessList extends React.Component {
                              : userRolesWrapperStyle}
                            >
                              <Text component={TextVariants.h6} style={detailLabelStyle}>{i18n._(t`User Roles`)}</Text>
-                              {result.userRoles.map(role => {
-                                // Show tooltips for associated Org/Team of role name displayed
-                                if (role.summary_fields.resource_name) {
-                                  return (
-                                    <Tooltip
-                                      message={role.summary_fields.resource_name}
-                                      position="top"
-                                      key={role.id}
-                                    >
-                                       <BasicChip
-                                        key={role.id}
-                                        text={role.name}
-                                      />
-                                    </Tooltip>
-                                  )
-                                } else {
-                                  return (
-                                    <BasicChip
-                                      key={role.id}
-                                      text={role.name}
-                                    />
-                                  )
-                                }
-                              })}
+                              {result.userRoles.map(role => (
+                                <BasicChip
+                                  key={role.id}
+                                  text={role.name}
+                                />
+                              ))}
                            </ul>
                          )}
                          {result.teamRoles.length > 0 && (
@ -388,14 +342,12 @@ class AccessList extends React.Component {
          </Fragment>
        )}
      </Fragment>
-
    );
  }
 }

 AccessList.propTypes = {
  getAccessList: PropTypes.func.isRequired,
-  getUserRoles: PropTypes.func.isRequired,
 };

 export default AccessList;
--- a/src/components/DataListToolbar/DataListToolbar.jsx
+++ b/src/components/DataListToolbar/DataListToolbar.jsx
@ -38,7 +38,7 @@ const flexGrowStyling = {
 };

 const ToolbarActiveStyle = {
-  backgroundColor: 'rgb(0, 123, 186)',
+  backgroundColor: '#007bba',
  color: 'white',
  padding: '0 5px',
 };
--- a/src/components/Tooltip/Tooltip.jsx
+++ b/src/components/Tooltip/Tooltip.jsx
@ -4,7 +4,7 @@ import PropTypes from 'prop-types';
 const toolTipContent = {
  padding: '10px',
  minWidth: '100px',
-}
+};

 class Tooltip extends React.Component {
  transforms = {
@ -47,7 +47,7 @@ class Tooltip extends React.Component {
    const {
      isDisplayed
    } = this.state;
-    
+
    if (message === '') {
      return null;
    }
@ -64,7 +64,7 @@ class Tooltip extends React.Component {
              style={{ position: 'absolute', zIndex: '10', ...this.transforms[position] }}
              className={`pf-c-tooltip pf-m-${position}`}
            >
-              <div className="pf-c-tooltip__arrow"/>
+              <div className="pf-c-tooltip__arrow" />
              <div className="pf-c-tooltip__content" style={toolTipContent}>
                { message }
              </div>
--- a/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx
+++ b/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx
@ -6,7 +6,6 @@ class OrganizationAccess extends React.Component {
    super(props);

    this.getOrgAccessList = this.getOrgAccessList.bind(this);
-    this.getUserRoles = this.getUserRoles.bind(this);
  }

  getOrgAccessList (id, params) {
@ -14,11 +13,6 @@ class OrganizationAccess extends React.Component {
    return api.getOrganzationAccessList(id, params);
  }

-  getUserRoles (id) {
-    const { api } = this.props;
-    return api.getUserRoles(id);
-  }
-
  render () {
    const {
      location,
@ -29,7 +23,6 @@ class OrganizationAccess extends React.Component {
    return (
      <AccessList
        getAccessList={this.getOrgAccessList}
-        getUserRoles={this.getUserRoles}
        match={match}
        location={location}
        history={history}