From 9f86fc2def5d2cef346de0373949b42eb3926fb6 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Tue, 5 Mar 2019 12:01:06 -0500 Subject: [PATCH] Respond to PR feedback. --- __tests__/components/AccessList.test.jsx | 63 ++++++-- .../Organization/OrganizationAccess.test.jsx | 11 -- src/api.js | 7 - src/components/AccessList/Access.list.jsx | 140 ++++++------------ .../DataListToolbar/DataListToolbar.jsx | 2 +- src/components/Tooltip/Tooltip.jsx | 6 +- .../Organization/OrganizationAccess.jsx | 7 - 7 files changed, 101 insertions(+), 135 deletions(-) diff --git a/__tests__/components/AccessList.test.jsx b/__tests__/components/AccessList.test.jsx index 624ae78fa2..d7ab1862fb 100644 --- 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('', () => { test('initially renders succesfully', () => { mount( @@ -40,7 +34,6 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '1' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => {}} - getUserRoles={() => {}} /> @@ -55,7 +48,6 @@ describe('', () => { 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 } })} /> @@ -77,7 +69,6 @@ describe('', () => { 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 } })} /> @@ -104,7 +95,6 @@ describe('', () => { 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 } })} /> @@ -118,4 +108,53 @@ describe('', () => { 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( + + + ({ data: { count: 1, results: mockData } })} + /> + + + ).find('AccessList'); + + setImmediate(() => { + const { results } = wrapper.state(); + results.forEach(result => { + expect(result.teamRoles).toEqual([]); + }); + done(); + }); + }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx index 9f6c63accd..4a42274379 100644 --- 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('', () => { test('initially renders succesfully', () => { mount( @@ -29,7 +22,6 @@ describe('', () => { params={{}} api={{ getOrganzationAccessList: jest.fn(), - getUserRoles: jest.fn(), }} /> @@ -45,14 +37,11 @@ describe('', () => { params={{}} api={{ getOrganzationAccessList: mockGetOrganzationAccessList, - getUserRoles: mockGetUserRoles, }} /> ).find('OrganizationAccess'); const accessList = await wrapper.instance().getOrgAccessList(); expect(accessList).toEqual(mockAPIAccessList); - const userRoles = await wrapper.instance().getUserRoles(); - expect(userRoles).toEqual(mockAPIRoles); }); }); diff --git a/src/api.js b/src/api.js index 54fe242f5e..41cdb7525d 100644 --- 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; diff --git a/src/components/AccessList/Access.list.jsx b/src/components/AccessList/Access.list.jsx index 1df8377ddf..c8d3aaec35 100644 --- 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 }) => { {label} ) : ({label} )} - {isBadge ? ( - - {value} - - ) : ( - {value} - )} + {value} ); } return detail; }; +const UserName = ({ value, url }) => { + let username = null; + if (value) { + username = ( + + {url ? ( + + {value} + ) : ({value} + )} + + ); + } + 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 => ( - {result.first_name || result.last_name ? ( {i18n._(t`User Roles`)} - {result.userRoles.map(role => { - // Show tooltips for associated Org/Team of role name displayed - if (role.summary_fields.resource_name) { - return ( - - - - ) - } else { - return ( - - ) - } - })} + {result.userRoles.map(role => ( + + ))} )} {result.teamRoles.length > 0 && ( @@ -388,14 +342,12 @@ class AccessList extends React.Component { )} - ); } } AccessList.propTypes = { getAccessList: PropTypes.func.isRequired, - getUserRoles: PropTypes.func.isRequired, }; export default AccessList; diff --git a/src/components/DataListToolbar/DataListToolbar.jsx b/src/components/DataListToolbar/DataListToolbar.jsx index 3968d4742d..2810a52435 100644 --- 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', }; diff --git a/src/components/Tooltip/Tooltip.jsx b/src/components/Tooltip/Tooltip.jsx index c3d8a2ac4f..c2309b443d 100644 --- 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}`} > -
+
{ message }
diff --git a/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx b/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx index 5301fcb05b..f1b8cb98b2 100644 --- 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 (