Refactor Access List.

- Derive Team Roles without making extra API call. - Consistent variable naming convention use camelCase. - More informative error display.
2024-11-01 08:21:15 +03:00 · 2019-03-01 10:12:36 -05:00 · 2019-03-01 10:12:36 -05:00 · bf7e6201a2
commit bf7e6201a2
parent a3a80bc23e
5 changed files with 62 additions and 114 deletions
--- a/tests/components/AccessList.test.jsx
+++ b/tests/components/AccessList.test.jsx
@ -31,19 +31,6 @@ const mockUserRoles = [
  }
 ];

-const mockUserTeams = [
-  {
-    id: 3,
-  }
-];
-
-const mockTeamRoles = [
-  {
-    id: 4,
-    name: 'baz',
-  }
-];
-
 describe('<AccessList />', () => {
  test('initially renders succesfully', () => {
    mount(
@ -54,8 +41,6 @@ describe('<AccessList />', () => {
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => {}}
            getUserRoles={() => {}}
-            getUserTeams={() => {}}
-            getTeamRoles={() => {}}
          />
        </MemoryRouter>
      </I18nProvider>
@ -71,8 +56,6 @@ describe('<AccessList />', () => {
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
            getUserRoles={() => ({ data: { results: mockUserRoles } })}
-            getUserTeams={() => ({ data: { results: mockUserTeams } })}
-            getTeamRoles={() => ({ data: { results: mockTeamRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
@ -95,8 +78,6 @@ describe('<AccessList />', () => {
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
            getUserRoles={() => ({ data: { results: mockUserRoles } })}
-            getUserTeams={() => ({ data: { results: mockUserTeams } })}
-            getTeamRoles={() => ({ data: { results: mockTeamRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
@ -124,8 +105,6 @@ describe('<AccessList />', () => {
            location={{ search: '', pathname: '/organizations/1/access' }}
            getAccessList={() => ({ data: { count: 1, results: mockResults } })}
            getUserRoles={() => ({ data: { results: mockUserRoles } })}
-            getUserTeams={() => ({ data: { results: mockUserTeams } })}
-            getTeamRoles={() => ({ data: { results: mockTeamRoles } })}
          />
        </MemoryRouter>
      </I18nProvider>
--- a/tests/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx
+++ b/tests/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx
@ -10,12 +10,6 @@ const mockAPIAccessList = {
 const mockAPIRoles = {
  bar: 'baz',
 };
-const mockAPITeams = {
-  qux: 'quux',
-};
-const mockAPITeamRoles = {
-  quuz: 'quuz',
-};

 const mockGetOrganzationAccessList = jest.fn(() => (
  Promise.resolve(mockAPIAccessList)
@ -25,14 +19,6 @@ const mockGetUserRoles = jest.fn(() => (
  Promise.resolve(mockAPIRoles)
 ));

-const mockGetUserTeams = jest.fn(() => (
-  Promise.resolve(mockAPITeams)
-));
-
-const mockGetTeamRoles = jest.fn(() => (
-  Promise.resolve(mockAPITeamRoles)
-));
-
 describe('<OrganizationAccess />', () => {
  test('initially renders succesfully', () => {
    mount(
@ -44,8 +30,6 @@ describe('<OrganizationAccess />', () => {
          api={{
            getOrganzationAccessList: jest.fn(),
            getUserRoles: jest.fn(),
-            getUserTeams: jest.fn(),
-            getTeamRoles: jest.fn(),
          }}
        />
      </MemoryRouter>
@ -62,8 +46,6 @@ describe('<OrganizationAccess />', () => {
          api={{
            getOrganzationAccessList: mockGetOrganzationAccessList,
            getUserRoles: mockGetUserRoles,
-            getUserTeams: mockGetUserTeams,
-            getTeamRoles: mockGetTeamRoles,
          }}
        />
      </MemoryRouter>
@ -72,9 +54,5 @@ describe('<OrganizationAccess />', () => {
    expect(accessList).toEqual(mockAPIAccessList);
    const userRoles = await wrapper.instance().getUserRoles();
    expect(userRoles).toEqual(mockAPIRoles);
-    const userTeams = await wrapper.instance().getUserTeams();
-    expect(userTeams).toEqual(mockAPITeams);
-    const teamRoles = await wrapper.instance().getTeamRoles();
-    expect(teamRoles).toEqual(mockAPITeamRoles);
  });
 });
--- a/src/api.js
+++ b/src/api.js
@ -6,7 +6,6 @@ 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 API_TEAMS = `${API_V2}teams/`;

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

@ -127,18 +126,6 @@ class APIClient {

    return this.http.get(endpoint);
  }
-
-  getUserTeams (id) {
-    const endpoint = `${API_USERS}${id}/teams/`;
-
-    return this.http.get(endpoint);
-  }
-
-  getTeamRoles (id) {
-    const endpoint = `${API_TEAMS}${id}/roles/`;
-
-    return this.http.get(endpoint);
-  }
 }

 export default APIClient;
--- a/src/components/AccessList/Access.list.jsx
+++ b/src/components/AccessList/Access.list.jsx
@ -71,15 +71,15 @@ const Detail = ({ label, value, url, isBadge, customStyles }) => {

 class AccessList extends React.Component {
  columns = [
+    { name: i18nMark('Name'), key: 'first_name', isSortable: true },
    { name: i18nMark('Username'), key: 'username', isSortable: true },
-    { name: i18nMark('First Name'), key: 'first_name', isSortable: true, isNumeric: true },
-    { name: i18nMark('Last Name'), key: 'last_name', isSortable: true, isNumeric: true },
+    { name: i18nMark('Last Name'), key: 'last_name', isSortable: true },
  ];

  defaultParams = {
    page: 1,
    page_size: 5,
-    order_by: 'username',
+    order_by: 'first_name',
  };

  constructor (props) {
@ -102,6 +102,9 @@ 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);
  }

  componentDidMount () {
@ -127,6 +130,7 @@ class AccessList extends React.Component {
    const page_size = parseInt(pageSize, 10);
    let order_by = sortedColumnKey;

+    // Preserve sort order when paginating
    if (sortOrder === 'descending') {
      order_by = `-${order_by}`;
    }
@ -159,8 +163,39 @@ 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;
+      }
+      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;
+      }
+      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;
+  }
+
  async fetchOrgAccessList (queryParams) {
-    const { match, getAccessList, getUserRoles, getTeamRoles, getUserTeams } = this.props;
+    const { match, getAccessList } = this.props;

    const { page, page_size, order_by } = queryParams;

@ -189,44 +224,22 @@ class AccessList extends React.Component {
      };

      results.forEach(async result => {
-        result.user_roles = [];
-        result.team_roles = [];
+        result.userRoles = [];
+        result.teamRoles = [];
+        result.directRole = null;
+
        // Grab each Role Type and set as a top-level value
-        Object.values(result.summary_fields).filter(value => value.length > 0).forEach(roleType => {
-          result.roleType = roleType[0].role.name;
-        });
+        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 resp = await getUserRoles(result.id);
-          const roles = resp.data.results || [];
-          roles.forEach(role => {
-            result.user_roles.push(role);
-          });
+          const roles = await this.fetchUserRoles(result.id);
+          roles.map(role => result.userRoles.push(role));
          this.setState(stateToUpdate);
        } catch (error) {
          this.setState({ error });
        }
-
-        // Grab Team Roles and set as a top-level value
-        try {
-          const team_data = await getUserTeams(result.id);
-          const teams = team_data.data.results || [];
-          teams.forEach(async team => {
-            try {
-              let team_roles = await getTeamRoles(team.id);
-              team_roles = team_roles.data.results || [];
-              team_roles.forEach(role => {
-                result.team_roles.push(role);
-              });
-              this.setState(stateToUpdate);
-            } catch (error) {
-              this.setState({ error });
-            }
-          });
-        } catch (error) {
-          this.setState({ error });
-        }
      });
    } catch (error) {
      this.setState({ error });
@ -247,8 +260,16 @@ class AccessList extends React.Component {
    } = this.state;
    return (
      <Fragment>
-        {!results && (
-          <h1>Loading...</h1> // TODO: replace with something nicer
+        {!error && !results && (
+          <h1>Loading...</h1> // TODO: replace with proper loading state
+        )}
+        {error && !results && (
+          <Fragment>
+            <div>{error.message}</div>
+            {error.response && (
+              <div>{error.response.data.detail}</div>
+            )}
+          </Fragment> // TODO: replace with proper error handling
        )}
        {results && (
          <Fragment>
@ -272,7 +293,7 @@ class AccessList extends React.Component {
                        <DataListCell>
                          <Detail
                            label={result.username}
-                            value={result.roleType}
+                            value={result.directRole}
                            url={result.url}
                            isBadge
                          />
@ -294,13 +315,13 @@ class AccessList extends React.Component {
                            url={null}
                            customStyles={isCompact ? hiddenStyle : null}
                          />
-                          {result.user_roles.length > 0 && (
+                          {result.userRoles.length > 0 && (
                            <ul style={isCompact
                              ? { ...userRolesWrapperStyle, ...hiddenStyle }
                              : userRolesWrapperStyle}
                            >
                              <Text component={TextVariants.h6} style={detailLabelStyle}>{i18n._(t`User Roles`)}</Text>
-                              {result.user_roles.map(role => (
+                              {result.userRoles.map(role => (
                                <BasicChip
                                  key={role.id}
                                  text={role.name}
@ -308,13 +329,13 @@ class AccessList extends React.Component {
                              ))}
                            </ul>
                          )}
-                          {result.team_roles.length > 0 && (
+                          {result.teamRoles.length > 0 && (
                            <ul style={isCompact
                              ? { ...userRolesWrapperStyle, ...hiddenStyle }
                              : userRolesWrapperStyle}
                            >
                              <Text component={TextVariants.h6} style={detailLabelStyle}>{i18n._(t`Team Roles`)}</Text>
-                              {result.team_roles.map(role => (
+                              {result.teamRoles.map(role => (
                                <BasicChip
                                  key={role.id}
                                  text={role.name}
@ -336,7 +357,6 @@ class AccessList extends React.Component {
              page_size={page_size}
              onSetPage={this.onSetPage}
            />
-            {error ? <div>{error}</div> : ''}
          </Fragment>
        )}
      </Fragment>
@ -348,8 +368,6 @@ class AccessList extends React.Component {
 AccessList.propTypes = {
  getAccessList: PropTypes.func.isRequired,
  getUserRoles: PropTypes.func.isRequired,
-  getUserTeams: PropTypes.func.isRequired,
-  getTeamRoles: PropTypes.func.isRequired,
 };

 export default AccessList;
--- a/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx
+++ b/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx
@ -7,8 +7,6 @@ class OrganizationAccess extends React.Component {

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

  getOrgAccessList (id, params) {
@ -21,16 +19,6 @@ class OrganizationAccess extends React.Component {
    return api.getUserRoles(id);
  }

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