From 3b65068258e34d7469b0d2e4caad64381bcb0921 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Fri, 8 Mar 2019 13:56:27 -0500 Subject: [PATCH 1/9] Add remove role functionality. --- __tests__/api.test.js | 4 +- __tests__/components/AccessList.test.jsx | 37 +++++ .../Organization/OrganizationAccess.test.jsx | 11 ++ .../Organization/OrganizationEdit.test.jsx | 2 +- src/api.js | 2 +- src/app.scss | 33 +++- src/components/AccessList/Access.list.jsx | 141 ++++++++++++++---- src/components/BasicChip/basicChip.scss | 7 + .../screens/Organization/Organization.jsx | 2 +- .../Organization/OrganizationAccess.jsx | 7 + .../screens/Organization/OrganizationEdit.jsx | 2 +- 11 files changed, 202 insertions(+), 46 deletions(-) diff --git a/__tests__/api.test.js b/__tests__/api.test.js index a6e12ecdba..c61bd980ea 100644 --- a/__tests__/api.test.js +++ b/__tests__/api.test.js @@ -155,14 +155,14 @@ describe('APIClient (api.js)', () => { done(); }); - test('disassociateInstanceGroup calls expected http method with expected data', async (done) => { + test('disassociate calls expected http method with expected data', async (done) => { const createPromise = () => Promise.resolve(); const mockHttp = ({ post: jest.fn(createPromise) }); const api = new APIClient(mockHttp); const url = 'foo/bar/'; const id = 1; - await api.disassociateInstanceGroup(url, id); + await api.disassociate(url, id); expect(mockHttp.post).toHaveBeenCalledTimes(1); expect(mockHttp.post.mock.calls[0][0]).toEqual(url); diff --git a/__tests__/components/AccessList.test.jsx b/__tests__/components/AccessList.test.jsx index d7ab1862fb..1cb93ac9da 100644 --- a/__tests__/components/AccessList.test.jsx +++ b/__tests__/components/AccessList.test.jsx @@ -34,6 +34,7 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '1' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => {}} + removeRole={() => {}} /> @@ -48,6 +49,7 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => ({ data: { count: 1, results: mockResults } })} + removeRole={() => {}} /> @@ -69,6 +71,7 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => ({ data: { count: 1, results: mockResults } })} + removeRole={() => {}} /> @@ -95,6 +98,7 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => ({ data: { count: 1, results: mockResults } })} + removeRole={() => {}} /> @@ -144,6 +148,7 @@ describe('', () => { match={{ path: '/organizations/:id', url: '/organizations/1', params: { id: '0' } }} location={{ search: '', pathname: '/organizations/1/access' }} getAccessList={() => ({ data: { count: 1, results: mockData } })} + removeRole={() => {}} /> @@ -157,4 +162,36 @@ describe('', () => { done(); }); }); + + test('test handleWarning, confirmDelete, and removeRole methods for Alert component', async (done) => { + const handleWarning = jest.spyOn(AccessList.prototype, 'handleWarning'); + const confirmDelete = jest.spyOn(AccessList.prototype, 'confirmDelete'); + const removeRole = jest.spyOn(AccessList.prototype, 'removeRole'); + const wrapper = mount( + + + ({ data: { count: 1, results: mockResults } })} + removeRole={() => {}} + /> + + + ).find('AccessList'); + expect(handleWarning).not.toHaveBeenCalled(); + expect(confirmDelete).not.toHaveBeenCalled(); + expect(removeRole).not.toHaveBeenCalled(); + + setImmediate(() => { + const rendered = wrapper.update().find('ChipButton'); + rendered.find('button[aria-label="close"]').simulate('click'); + expect(handleWarning).toHaveBeenCalled(); + const alert = wrapper.update().find('Alert'); + alert.find('button[aria-label="confirm-delete"]').simulate('click'); + expect(confirmDelete).toHaveBeenCalled(); + expect(removeRole).toHaveBeenCalled(); + done(); + }); + }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx index 4a42274379..3ba80d596f 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx @@ -12,6 +12,14 @@ const mockGetOrganzationAccessList = jest.fn(() => ( Promise.resolve(mockAPIAccessList) )); +const mockResponse = { + status: 'success', +}; + +const mockRemoveRole = jest.fn(() => ( + Promise.resolve(mockResponse) +)); + describe('', () => { test('initially renders succesfully', () => { mount( @@ -37,11 +45,14 @@ describe('', () => { params={{}} api={{ getOrganzationAccessList: mockGetOrganzationAccessList, + disassociate: mockRemoveRole }} /> ).find('OrganizationAccess'); const accessList = await wrapper.instance().getOrgAccessList(); expect(accessList).toEqual(mockAPIAccessList); + const resp = await wrapper.instance().removeRole(2, 3, 'users'); + expect(resp).toEqual(mockResponse); }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationEdit.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationEdit.test.jsx index 39c638bba6..1160f6ed4d 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationEdit.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationEdit.test.jsx @@ -183,7 +183,7 @@ describe('', () => { getOrganizationInstanceGroups: getOrganizationInstanceGroupsFn, updateOrganizationDetails: updateOrganizationDetailsFn, associateInstanceGroup: associateInstanceGroupFn, - disassociateInstanceGroup: disassociateInstanceGroupFn + disassociate: disassociateInstanceGroupFn }; const wrapper = mount( diff --git a/src/api.js b/src/api.js index 41cdb7525d..f87c51703e 100644 --- a/src/api.js +++ b/src/api.js @@ -126,7 +126,7 @@ class APIClient { return this.http.post(url, { id }); } - disassociateInstanceGroup (url, id) { + disassociate (url, id) { return this.http.post(url, { id, disassociate: true }); } } diff --git a/src/app.scss b/src/app.scss index 934240376c..b768b036e4 100644 --- a/src/app.scss +++ b/src/app.scss @@ -220,14 +220,6 @@ // and bem style, as well as moved into component-based scss files // -.awx-lookup { - min-height: 36px; - - .pf-c-form-control { - --pf-c-form-control--Height: auto; - } -} - .awx-c-list { border-bottom: 1px solid #d7d7d7; } @@ -238,3 +230,28 @@ --pf-c-card__body--PaddingX: 0; --pf-c-card__body--PaddingY: 0; } + + +.awx-c-card { + position: relative; +} + +.awx-c-alert { + position: absolute; + top: 0; + left: 0; + width: 100%; +} + +.pf-c-alert { + position: absolute; + width: 100%; + + & button { + margin-right: 20px; + } +} + +.pf-c-alert__icon > svg { + fill: white; +} \ No newline at end of file diff --git a/src/components/AccessList/Access.list.jsx b/src/components/AccessList/Access.list.jsx index c8d3aaec35..cb0e9a09f9 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 + TextContent, TextVariants, Chip, Alert, AlertActionCloseButton, Button } from '@patternfly/react-core'; import { I18n, i18nMark } from '@lingui/react'; @@ -13,7 +13,6 @@ import { Link } from 'react-router-dom'; -import BasicChip from '../BasicChip/BasicChip'; import Pagination from '../Pagination'; import DataListToolbar from '../DataListToolbar'; @@ -46,6 +45,10 @@ const hiddenStyle = { display: 'none', }; +const buttonGroupStyle = { + float: 'right', +}; + const Detail = ({ label, value, url, customStyles }) => { let detail = null; if (value) { @@ -104,6 +107,7 @@ class AccessList extends React.Component { sortOrder: 'ascending', sortedColumnKey: 'username', isCompact: false, + showWarning: false, }; this.fetchOrgAccessList = this.fetchOrgAccessList.bind(this); @@ -112,7 +116,10 @@ class AccessList extends React.Component { this.onCompact = this.onCompact.bind(this); this.onSort = this.onSort.bind(this); this.getQueryParams = this.getQueryParams.bind(this); - this.getTeamRoles = this.getTeamRoles.bind(this); + this.removeRole = this.removeRole.bind(this); + this.handleWarning = this.handleWarning.bind(this); + this.hideWarning = this.hideWarning.bind(this); + this.confirmDelete = this.confirmDelete.bind(this); } componentDidMount () { @@ -171,23 +178,6 @@ class AccessList extends React.Component { return Object.assign({}, this.defaultParams, searchParams, overrides); } - getRoles = roles => Object.values(roles) - .reduce((val, role) => { - if (role.length > 0) { - val.push(role[0].role); - } - return val; - }, []); - - getTeamRoles = roles => roles - .reduce((val, item) => { - if (item.role.team_id) { - const { role } = item; - val.push(role); - } - return val; - }, []); - async fetchOrgAccessList (queryParams) { const { match, getAccessList } = this.props; @@ -216,13 +206,27 @@ class AccessList extends React.Component { sortedColumnKey, results, }; + 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) || []; + // Separate out roles into user roles or team roles + // based on whether or not a team_id attribute is present + const teamRoles = []; + const userRoles = []; + Object.values(result.summary_fields).forEach(field => { + if (field.length > 0) { + field.forEach(item => { + const { role } = item; + if (role.team_id) { + teamRoles.push(role); + } else { + userRoles.push(role); + } + }); + } + }); + + result.teamRoles = teamRoles || []; + result.userRoles = userRoles || []; }); this.setState(stateToUpdate); } catch (error) { @@ -230,6 +234,55 @@ class AccessList extends React.Component { } } + async removeRole (roleId, resourceId, type) { + const { removeRole } = this.props; + const url = `/api/v2/${type}/${resourceId}/roles/`; + await removeRole(url, roleId); + const queryParams = this.getQueryParams(); + try { + this.fetchOrgAccessList(queryParams); + } catch (error) { + this.setState({ error }); + } + this.setState({ showWarning: false }); + } + + handleWarning (roleName, roleId, resourceName, resourceId, type) { + let warningTitle; + let warningMsg; + + if (type === 'users') { + warningTitle = i18nMark('User Access Removal'); + warningMsg = i18nMark(`Please confirm that you would like to remove ${roleName} + access from ${resourceName}.`); + } + if (type === 'teams') { + warningTitle = i18nMark('Team Access Removal'); + warningMsg = i18nMark(`Please confirm that you would like to remove ${roleName} + access from the team ${resourceName}. This will affect all + members of the team. If you would like to only remove access + for this particular user, please remove them from the team.`); + } + + this.setState({ + showWarning: true, + warningMsg, + warningTitle, + deleteType: type, + deleteRoleId: roleId, + deleteResourceId: resourceId + }); + } + + hideWarning () { + this.setState({ showWarning: false }); + } + + confirmDelete () { + const { deleteType, deleteResourceId, deleteRoleId } = this.state; + this.removeRole(deleteRoleId, deleteResourceId, deleteType); + } + render () { const { results, @@ -241,6 +294,9 @@ class AccessList extends React.Component { sortedColumnKey, sortOrder, isCompact, + warningMsg, + warningTitle, + showWarning } = this.state; return ( @@ -268,6 +324,20 @@ class AccessList extends React.Component { isCompact={isCompact} showExpandCollapse /> + {showWarning && ( + } + > + {warningMsg} + + + + + + )} + {({ i18n }) => ( @@ -304,10 +374,13 @@ class AccessList extends React.Component { > {i18n._(t`User Roles`)} {result.userRoles.map(role => ( - + className="awx-c-chip" + onClick={() => this.handleWarning(role.name, role.id, result.username, result.id, 'users')} + > + {role.name} + ))} )} @@ -318,10 +391,13 @@ class AccessList extends React.Component { > {i18n._(t`Team Roles`)} {result.teamRoles.map(role => ( - + className="awx-c-chip" + onClick={() => this.handleWarning(role.name, role.id, role.team_name, role.team_id, 'teams')} + > + {role.name} + ))} )} @@ -348,6 +424,7 @@ class AccessList extends React.Component { AccessList.propTypes = { getAccessList: PropTypes.func.isRequired, + removeRole: PropTypes.func.isRequired, }; export default AccessList; diff --git a/src/components/BasicChip/basicChip.scss b/src/components/BasicChip/basicChip.scss index 11b6601ec7..41e7e8f5df 100644 --- a/src/components/BasicChip/basicChip.scss +++ b/src/components/BasicChip/basicChip.scss @@ -7,4 +7,11 @@ .pf-c-button { display: none; } +} + +.awx-c-chip { + padding: 3px 0; + height: 24px; + margin-right: 10px; + margin-bottom: 10px; } \ No newline at end of file diff --git a/src/pages/Organizations/screens/Organization/Organization.jsx b/src/pages/Organizations/screens/Organization/Organization.jsx index dde9a44621..85b090ee73 100644 --- a/src/pages/Organizations/screens/Organization/Organization.jsx +++ b/src/pages/Organizations/screens/Organization/Organization.jsx @@ -112,7 +112,7 @@ class Organization extends Component { return ( - + { cardHeader } { - await api.disassociateInstanceGroup(url, id); + await api.disassociate(url, id); })); } catch (err) { this.setState({ error: err }); From 198dfe7f2e1e2d92ee0938e0ca2dc625ed9a4fdd Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Wed, 13 Mar 2019 14:44:12 -0400 Subject: [PATCH 2/9] Add unit test to check state when user tries to delete a role. --- __tests__/components/AccessList.test.jsx | 89 ++++++++++++------- .../Organization/OrganizationAccess.test.jsx | 8 +- src/components/AccessList/Access.list.jsx | 4 +- 3 files changed, 59 insertions(+), 42 deletions(-) diff --git a/__tests__/components/AccessList.test.jsx b/__tests__/components/AccessList.test.jsx index 1cb93ac9da..ce9a49c132 100644 --- a/__tests__/components/AccessList.test.jsx +++ b/__tests__/components/AccessList.test.jsx @@ -5,7 +5,7 @@ import { I18nProvider } from '@lingui/react'; import AccessList from '../../src/components/AccessList'; -const mockResults = [ +const mockData = [ { id: 1, username: 'boo', @@ -48,7 +48,7 @@ describe('', () => { ({ data: { count: 1, results: mockResults } })} + getAccessList={() => ({ data: { count: 1, results: mockData } })} removeRole={() => {}} /> @@ -56,7 +56,7 @@ describe('', () => { ).find('AccessList'); setImmediate(() => { - expect(wrapper.state().results).toEqual(mockResults); + expect(wrapper.state().results).toEqual(mockData); done(); }); }); @@ -70,7 +70,7 @@ describe('', () => { ({ data: { count: 1, results: mockResults } })} + getAccessList={() => ({ data: { count: 1, results: mockData } })} removeRole={() => {}} /> @@ -97,7 +97,7 @@ describe('', () => { ({ data: { count: 1, results: mockResults } })} + getAccessList={() => ({ data: { count: 1, results: mockData } })} removeRole={() => {}} /> @@ -114,33 +114,7 @@ describe('', () => { }); 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( @@ -163,7 +137,7 @@ describe('', () => { }); }); - test('test handleWarning, confirmDelete, and removeRole methods for Alert component', async (done) => { + test('test handleWarning, confirmDelete, and removeRole methods for Alert component', (done) => { const handleWarning = jest.spyOn(AccessList.prototype, 'handleWarning'); const confirmDelete = jest.spyOn(AccessList.prototype, 'confirmDelete'); const removeRole = jest.spyOn(AccessList.prototype, 'removeRole'); @@ -173,7 +147,7 @@ describe('', () => { ({ data: { count: 1, results: mockResults } })} + getAccessList={() => ({ data: { count: 1, results: mockData } })} removeRole={() => {}} /> @@ -194,4 +168,51 @@ describe('', () => { done(); }); }); + + test('state is set appropriately when a user tries deleting a role', (done) => { + const wrapper = mount( + + + ({ data: { count: 1, results: mockData } })} + removeRole={() => {}} + /> + + + ).find('AccessList'); + + expect(wrapper.state().warningMsg).not.toBeDefined; + expect(wrapper.state().warningTitle).not.toBeDefined; + expect(wrapper.state().deleteType).not.toBeDefined; + expect(wrapper.state().deleteRoleId).not.toBeDefined; + expect(wrapper.state().deleteResourceId).not.toBeDefined; + + setImmediate(() => { + const expected = [ + { + deleteType: 'users' + }, + { + deleteRoleId: mockData[0].summary_fields.foo[0].role.id + }, + { + deleteResourceId: mockData[0].id + } + ]; + const rendered = wrapper.update().find('ChipButton'); + rendered.find('button[aria-label="close"]').simulate('click'); + const alert = wrapper.update().find('Alert'); + alert.find('button[aria-label="confirm-delete"]').simulate('click'); + expect(wrapper.state().warningTitle).not.toBe(null); + expect(wrapper.state().warningMsg).not.toBe(null); + expected.forEach(criteria => { + for (const prop in criteria) { + expect(wrapper.state()[prop]).toEqual(criteria[prop]); + } + }) + done(); + }); + }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx index 3ba80d596f..915c077364 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx @@ -8,17 +8,13 @@ const mockAPIAccessList = { foo: 'bar', }; -const mockGetOrganzationAccessList = jest.fn(() => ( - Promise.resolve(mockAPIAccessList) -)); +const mockGetOrganzationAccessList = () => Promise.resolve(mockAPIAccessList); const mockResponse = { status: 'success', }; -const mockRemoveRole = jest.fn(() => ( - Promise.resolve(mockResponse) -)); +const mockRemoveRole = () => Promise.resolve(mockResponse); describe('', () => { test('initially renders succesfully', () => { diff --git a/src/components/AccessList/Access.list.jsx b/src/components/AccessList/Access.list.jsx index cb0e9a09f9..01b0f085b7 100644 --- a/src/components/AccessList/Access.list.jsx +++ b/src/components/AccessList/Access.list.jsx @@ -103,7 +103,7 @@ class AccessList extends React.Component { this.state = { page, page_size, - count: null, + count: 0, sortOrder: 'ascending', sortedColumnKey: 'username', isCompact: false, @@ -193,7 +193,7 @@ class AccessList extends React.Component { try { const { data: - { count = null, results = null } + { count = 0, results = [] } } = await getAccessList(match.params.id, queryParams); const pageCount = Math.ceil(count / page_size); From 0495214f475b6d695c5ba3a616349ee7058fe5b2 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Wed, 13 Mar 2019 15:59:42 -0400 Subject: [PATCH 3/9] Address PR feedback. --- __tests__/components/AccessList.test.jsx | 19 ++++++------------- src/app.scss | 7 +++++++ src/components/AccessList/Access.list.jsx | 16 ++++++++++------ src/components/BasicChip/basicChip.scss | 7 ------- 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/__tests__/components/AccessList.test.jsx b/__tests__/components/AccessList.test.jsx index ce9a49c132..77aff16344 100644 --- a/__tests__/components/AccessList.test.jsx +++ b/__tests__/components/AccessList.test.jsx @@ -114,7 +114,6 @@ describe('', () => { }); test('getTeamRoles returns empty array if dataset is missing team_id attribute', (done) => { - const wrapper = mount( @@ -140,7 +139,7 @@ describe('', () => { test('test handleWarning, confirmDelete, and removeRole methods for Alert component', (done) => { const handleWarning = jest.spyOn(AccessList.prototype, 'handleWarning'); const confirmDelete = jest.spyOn(AccessList.prototype, 'confirmDelete'); - const removeRole = jest.spyOn(AccessList.prototype, 'removeRole'); + const removeRole = jest.spyOn(AccessList.prototype, 'removeAccessRole'); const wrapper = mount( @@ -182,19 +181,13 @@ describe('', () => { ).find('AccessList'); - - expect(wrapper.state().warningMsg).not.toBeDefined; - expect(wrapper.state().warningTitle).not.toBeDefined; - expect(wrapper.state().deleteType).not.toBeDefined; - expect(wrapper.state().deleteRoleId).not.toBeDefined; - expect(wrapper.state().deleteResourceId).not.toBeDefined; setImmediate(() => { const expected = [ { deleteType: 'users' }, - { + { deleteRoleId: mockData[0].summary_fields.foo[0].role.id }, { @@ -208,10 +201,10 @@ describe('', () => { expect(wrapper.state().warningTitle).not.toBe(null); expect(wrapper.state().warningMsg).not.toBe(null); expected.forEach(criteria => { - for (const prop in criteria) { - expect(wrapper.state()[prop]).toEqual(criteria[prop]); - } - }) + Object.keys(criteria).forEach(key => { + expect(wrapper.state()[key]).toEqual(criteria[key]); + }); + }); done(); }); }); diff --git a/src/app.scss b/src/app.scss index 9655d532b8..637bcdb636 100644 --- a/src/app.scss +++ b/src/app.scss @@ -275,3 +275,10 @@ .at-u-textRight { text-align: right; } + +.awx-c-chip { + padding: 3px 0; + height: 24px; + margin-right: 10px; + margin-bottom: 10px; +} \ No newline at end of file diff --git a/src/components/AccessList/Access.list.jsx b/src/components/AccessList/Access.list.jsx index 01b0f085b7..7a55688d8a 100644 --- a/src/components/AccessList/Access.list.jsx +++ b/src/components/AccessList/Access.list.jsx @@ -116,7 +116,7 @@ class AccessList extends React.Component { this.onCompact = this.onCompact.bind(this); this.onSort = this.onSort.bind(this); this.getQueryParams = this.getQueryParams.bind(this); - this.removeRole = this.removeRole.bind(this); + this.removeAccessRole = this.removeAccessRole.bind(this); this.handleWarning = this.handleWarning.bind(this); this.hideWarning = this.hideWarning.bind(this); this.confirmDelete = this.confirmDelete.bind(this); @@ -225,8 +225,8 @@ class AccessList extends React.Component { } }); - result.teamRoles = teamRoles || []; - result.userRoles = userRoles || []; + result.teamRoles = teamRoles; + result.userRoles = userRoles; }); this.setState(stateToUpdate); } catch (error) { @@ -234,10 +234,14 @@ class AccessList extends React.Component { } } - async removeRole (roleId, resourceId, type) { + async removeAccessRole (roleId, resourceId, type) { const { removeRole } = this.props; const url = `/api/v2/${type}/${resourceId}/roles/`; - await removeRole(url, roleId); + try { + await removeRole(url, roleId); + } catch (error) { + this.setState({ error }); + } const queryParams = this.getQueryParams(); try { this.fetchOrgAccessList(queryParams); @@ -280,7 +284,7 @@ class AccessList extends React.Component { confirmDelete () { const { deleteType, deleteResourceId, deleteRoleId } = this.state; - this.removeRole(deleteRoleId, deleteResourceId, deleteType); + this.removeAccessRole(deleteRoleId, deleteResourceId, deleteType); } render () { diff --git a/src/components/BasicChip/basicChip.scss b/src/components/BasicChip/basicChip.scss index 41e7e8f5df..8cea76d049 100644 --- a/src/components/BasicChip/basicChip.scss +++ b/src/components/BasicChip/basicChip.scss @@ -8,10 +8,3 @@ display: none; } } - -.awx-c-chip { - padding: 3px 0; - height: 24px; - margin-right: 10px; - margin-bottom: 10px; -} \ No newline at end of file From c6d810621fb3caef3023db4807e2d7c5c9558531 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Wed, 13 Mar 2019 16:37:03 -0400 Subject: [PATCH 4/9] Move AccessList out of components directory. - The AccessList component is pretty specific to the Organization component, and given the structure and methods within it, is pretty context-specific and not a good candidate to be a re-useable component. --- src/components/AccessList/index.js | 3 --- .../Organizations/components/OrganizationAccessList.jsx} | 0 2 files changed, 3 deletions(-) delete mode 100644 src/components/AccessList/index.js rename src/{components/AccessList/Access.list.jsx => pages/Organizations/components/OrganizationAccessList.jsx} (100%) diff --git a/src/components/AccessList/index.js b/src/components/AccessList/index.js deleted file mode 100644 index f435e8bb1f..0000000000 --- a/src/components/AccessList/index.js +++ /dev/null @@ -1,3 +0,0 @@ -import AccessList from './Access.list'; - -export default AccessList; diff --git a/src/components/AccessList/Access.list.jsx b/src/pages/Organizations/components/OrganizationAccessList.jsx similarity index 100% rename from src/components/AccessList/Access.list.jsx rename to src/pages/Organizations/components/OrganizationAccessList.jsx From 390832cc1ad2c8eaddd107fa7df5e67b63e5d0ad Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Wed, 13 Mar 2019 16:46:50 -0400 Subject: [PATCH 5/9] Rename AccessList to OrganizationAccessList and fix paths. --- .../components/OrganizationAccessList.jsx | 12 ++++++------ .../screens/Organization/OrganizationAccess.jsx | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/pages/Organizations/components/OrganizationAccessList.jsx b/src/pages/Organizations/components/OrganizationAccessList.jsx index 7a55688d8a..f94a90699e 100644 --- a/src/pages/Organizations/components/OrganizationAccessList.jsx +++ b/src/pages/Organizations/components/OrganizationAccessList.jsx @@ -13,12 +13,12 @@ import { Link } from 'react-router-dom'; -import Pagination from '../Pagination'; -import DataListToolbar from '../DataListToolbar'; +import Pagination from '../../../components/Pagination'; +import DataListToolbar from '../../../components/DataListToolbar'; import { parseQueryString, -} from '../../qs'; +} from '../../../qs'; const userRolesWrapperStyle = { display: 'flex', @@ -82,7 +82,7 @@ const UserName = ({ value, url }) => { return username; }; -class AccessList extends React.Component { +class OrganizationAccessList extends React.Component { columns = [ { name: i18nMark('Name'), key: 'first_name', isSortable: true }, { name: i18nMark('Username'), key: 'username', isSortable: true }, @@ -426,9 +426,9 @@ class AccessList extends React.Component { } } -AccessList.propTypes = { +OrganizationAccessList.propTypes = { getAccessList: PropTypes.func.isRequired, removeRole: PropTypes.func.isRequired, }; -export default AccessList; +export default OrganizationAccessList; diff --git a/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx b/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx index 6fde705fdf..e8cb7630c5 100644 --- a/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx +++ b/src/pages/Organizations/screens/Organization/OrganizationAccess.jsx @@ -1,5 +1,5 @@ import React from 'react'; -import AccessList from '../../../../components/AccessList'; +import OrganizationAccessList from '../../components/OrganizationAccessList'; class OrganizationAccess extends React.Component { constructor (props) { @@ -27,7 +27,7 @@ class OrganizationAccess extends React.Component { } = this.props; return ( - Date: Wed, 13 Mar 2019 17:12:16 -0400 Subject: [PATCH 6/9] Move test for AccessList. --- .../OrganizationAccessList.test.jsx} | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) rename __tests__/{components/AccessList.test.jsx => pages/Organizations/components/OrganizationAccessList.test.jsx} (84%) diff --git a/__tests__/components/AccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx similarity index 84% rename from __tests__/components/AccessList.test.jsx rename to __tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 77aff16344..82a4c452c6 100644 --- a/__tests__/components/AccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -3,7 +3,7 @@ import { mount } from 'enzyme'; import { MemoryRouter } from 'react-router-dom'; import { I18nProvider } from '@lingui/react'; -import AccessList from '../../src/components/AccessList'; +import OrganizationAccessList from '../../../../src/pages/Organizations/components/OrganizationAccessList'; const mockData = [ { @@ -25,12 +25,12 @@ const mockData = [ } ]; -describe('', () => { +describe('', () => { test('initially renders succesfully', () => { mount( - {}} @@ -45,7 +45,7 @@ describe('', () => { const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -53,7 +53,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); setImmediate(() => { expect(wrapper.state().results).toEqual(mockData); @@ -62,12 +62,12 @@ describe('', () => { }); test('onExpand and onCompact methods called when user clicks on Expand and Compact icons respectively', async (done) => { - const onExpand = jest.spyOn(AccessList.prototype, 'onExpand'); - const onCompact = jest.spyOn(AccessList.prototype, 'onCompact'); + const onExpand = jest.spyOn(OrganizationAccessList.prototype, 'onExpand'); + const onCompact = jest.spyOn(OrganizationAccessList.prototype, 'onCompact'); const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -75,7 +75,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); expect(onExpand).not.toHaveBeenCalled(); expect(onCompact).not.toHaveBeenCalled(); @@ -90,11 +90,11 @@ describe('', () => { }); test('onSort being passed properly to DataListToolbar component', async (done) => { - const onSort = jest.spyOn(AccessList.prototype, 'onSort'); + const onSort = jest.spyOn(OrganizationAccessList.prototype, 'onSort'); const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -102,7 +102,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); expect(onSort).not.toHaveBeenCalled(); setImmediate(() => { @@ -117,7 +117,7 @@ describe('', () => { const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -125,7 +125,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); setImmediate(() => { const { results } = wrapper.state(); @@ -137,13 +137,13 @@ describe('', () => { }); test('test handleWarning, confirmDelete, and removeRole methods for Alert component', (done) => { - const handleWarning = jest.spyOn(AccessList.prototype, 'handleWarning'); - const confirmDelete = jest.spyOn(AccessList.prototype, 'confirmDelete'); - const removeRole = jest.spyOn(AccessList.prototype, 'removeAccessRole'); + const handleWarning = jest.spyOn(OrganizationAccessList.prototype, 'handleWarning'); + const confirmDelete = jest.spyOn(OrganizationAccessList.prototype, 'confirmDelete'); + const removeRole = jest.spyOn(OrganizationAccessList.prototype, 'removeAccessRole'); const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -151,7 +151,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); expect(handleWarning).not.toHaveBeenCalled(); expect(confirmDelete).not.toHaveBeenCalled(); expect(removeRole).not.toHaveBeenCalled(); @@ -172,7 +172,7 @@ describe('', () => { const wrapper = mount( - ({ data: { count: 1, results: mockData } })} @@ -180,7 +180,7 @@ describe('', () => { /> - ).find('AccessList'); + ).find('OrganizationAccessList'); setImmediate(() => { const expected = [ From 518ecee53e8fbbea0d05dcef8d9ef51f1393bc2f Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Thu, 14 Mar 2019 10:35:54 -0400 Subject: [PATCH 7/9] Apply padding to Alert component. --- src/app.scss | 28 ++++++++++++++++--- .../components/OrganizationAccessList.jsx | 6 +--- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/app.scss b/src/app.scss index 637bcdb636..7726c4131b 100644 --- a/src/app.scss +++ b/src/app.scss @@ -258,14 +258,24 @@ position: relative; } +// PF Alert notification component overrides +.pf-c-alert__title { + --pf-c-alert__title--PaddingTop: 20px; + --pf-c-alert__title--PaddingRight: 20px; + --pf-c-alert__title--PaddingBottom: 20px; + --pf-c-alert__title--PaddingLeft: 20px; +} + +.pf-c-alert__description { + --pf-c-alert__description--PaddingRight: 20px; + --pf-c-alert__description--PaddingBottom: 20px; + --pf-c-alert__description--PaddingLeft: 20px; +} + .pf-c-alert { position: absolute; width: 100%; z-index: 20; - - button { - margin-right: 20px; - } } .pf-c-alert__icon { @@ -281,4 +291,14 @@ height: 24px; margin-right: 10px; margin-bottom: 10px; +} + +.awx-c-form-action-group { + float: right; + display: block; + + .pf-m-danger { + margin-top: 20px; + margin-right: 20px; + } } \ No newline at end of file diff --git a/src/pages/Organizations/components/OrganizationAccessList.jsx b/src/pages/Organizations/components/OrganizationAccessList.jsx index f94a90699e..f4a9663f3d 100644 --- a/src/pages/Organizations/components/OrganizationAccessList.jsx +++ b/src/pages/Organizations/components/OrganizationAccessList.jsx @@ -45,10 +45,6 @@ const hiddenStyle = { display: 'none', }; -const buttonGroupStyle = { - float: 'right', -}; - const Detail = ({ label, value, url, customStyles }) => { let detail = null; if (value) { @@ -335,7 +331,7 @@ class OrganizationAccessList extends React.Component { action={} > {warningMsg} - + From a5683fb3549f35132eed04700b798a02cf276009 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Thu, 14 Mar 2019 10:41:28 -0400 Subject: [PATCH 8/9] Clear mocked methods after each test to prevent overlaps with other tests. --- .../Organizations/components/OrganizationAccessList.test.jsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 82a4c452c6..588ff8df7a 100644 --- a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -26,6 +26,10 @@ const mockData = [ ]; describe('', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + test('initially renders succesfully', () => { mount( From a4493cd02b59825baf4e5b5b49cd1045095922b1 Mon Sep 17 00:00:00 2001 From: Kia Lam Date: Mon, 18 Mar 2019 09:32:11 -0400 Subject: [PATCH 9/9] Add empty initial states and adjust rendering logic. --- .../components/OrganizationAccessList.jsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/pages/Organizations/components/OrganizationAccessList.jsx b/src/pages/Organizations/components/OrganizationAccessList.jsx index f4a9663f3d..60616756c8 100644 --- a/src/pages/Organizations/components/OrganizationAccessList.jsx +++ b/src/pages/Organizations/components/OrganizationAccessList.jsx @@ -104,6 +104,12 @@ class OrganizationAccessList extends React.Component { sortedColumnKey: 'username', isCompact: false, showWarning: false, + warningTitle: '', + warningMsg: '', + deleteType: '', + deleteRoleId: null, + deleteResourceId: null, + results: [], }; this.fetchOrgAccessList = this.fetchOrgAccessList.bind(this); @@ -300,10 +306,10 @@ class OrganizationAccessList extends React.Component { } = this.state; return ( - {!error && !results && ( + {!error && results.length <= 0 && (

Loading...

// TODO: replace with proper loading state )} - {error && !results && ( + {error && results.length <= 0 && (
{error.message}
{error.response && ( @@ -311,7 +317,7 @@ class OrganizationAccessList extends React.Component { )} // TODO: replace with proper error handling )} - {results && ( + {results.length > 0 && (