From 5ae7cbb43ad24080fb55923c995140728ff635e9 Mon Sep 17 00:00:00 2001 From: mabashian Date: Thu, 18 Apr 2019 13:31:03 -0400 Subject: [PATCH 1/6] Add RBAC to org views --- src/api.js | 9 ++ .../DataListToolbar/DataListToolbar.jsx | 60 ++++++---- src/components/DataListToolbar/styles.scss | 5 +- .../NotificationListItem.jsx | 4 + .../NotificationsList/Notifications.list.jsx | 3 + src/contexts/Config.jsx | 59 +++++++++- src/index.jsx | 4 +- src/pages/Login.jsx | 6 +- src/pages/Organizations/Organizations.jsx | 16 ++- .../components/OrganizationAccessList.jsx | 28 +++-- .../screens/Organization/Organization.jsx | 89 +++++++++++++-- .../Organization/OrganizationDetail.jsx | 15 ++- .../OrganizationNotifications.jsx | 11 +- .../screens/OrganizationsList.jsx | 104 ++++++++++++------ 14 files changed, 315 insertions(+), 98 deletions(-) diff --git a/src/api.js b/src/api.js index bda5fd5319..9767632c6a 100644 --- a/src/api.js +++ b/src/api.js @@ -3,6 +3,7 @@ const API_LOGIN = `${API_ROOT}login/`; const API_LOGOUT = `${API_ROOT}logout/`; const API_V2 = `${API_ROOT}v2/`; const API_CONFIG = `${API_V2}config/`; +const API_ME = `${API_V2}me/`; const API_ORGANIZATIONS = `${API_V2}organizations/`; const API_INSTANCE_GROUPS = `${API_V2}instance_groups/`; const API_USERS = `${API_V2}users/`; @@ -58,6 +59,10 @@ class APIClient { return this.http.get(API_CONFIG); } + getMe () { + return this.http.get(API_ME); + } + destroyOrganization (id) { const endpoint = `${API_ORGANIZATIONS}${id}/`; return (this.http.delete(endpoint)); @@ -71,6 +76,10 @@ class APIClient { return this.http.post(API_ORGANIZATIONS, data); } + callOrganizations () { + return this.http.options(API_ORGANIZATIONS); + } + getOrganizationAccessList (id, params = {}) { const endpoint = `${API_ORGANIZATIONS}${id}/access_list/`; diff --git a/src/components/DataListToolbar/DataListToolbar.jsx b/src/components/DataListToolbar/DataListToolbar.jsx index 738f396b3f..a4e6428253 100644 --- a/src/components/DataListToolbar/DataListToolbar.jsx +++ b/src/components/DataListToolbar/DataListToolbar.jsx @@ -1,6 +1,6 @@ import React, { Fragment } from 'react'; import PropTypes from 'prop-types'; -import { I18n } from '@lingui/react'; +import { I18n, i18nMark } from '@lingui/react'; import { t } from '@lingui/macro'; import { Button, @@ -28,23 +28,27 @@ import VerticalSeparator from '../VerticalSeparator'; class DataListToolbar extends React.Component { render () { const { + add, addUrl, columns, + deleteTooltip, disableTrashCanIcon, - onSelectAll, - sortedColumnKey, - sortOrder, - showDelete, - showSelectAll, isAllSelected, isCompact, noLeftMargin, onSort, - onSearch, + onSearch onCompact, onExpand, - add, - onOpenDeleteModal + onOpenDeleteModal, + onSearch, + onSelectAll, + onSort, + showAdd, + showDelete, + showSelectAll, + sortOrder, + sortedColumnKey } = this.props; const showExpandCollapse = (onCompact && onExpand); @@ -112,21 +116,23 @@ class DataListToolbar extends React.Component { { showDelete && ( - + + + )} - {addUrl && ( + {showAdd && addUrl && ( - - + {summary_fields.user_capabilities.edit && ( +
+ + + +
+ )} {error ? 'error!' : ''} )} diff --git a/src/pages/Organizations/screens/Organization/OrganizationNotifications.jsx b/src/pages/Organizations/screens/Organization/OrganizationNotifications.jsx index 4f6cf13d6d..491071bfee 100644 --- a/src/pages/Organizations/screens/Organization/OrganizationNotifications.jsx +++ b/src/pages/Organizations/screens/Organization/OrganizationNotifications.jsx @@ -41,13 +41,18 @@ class OrganizationNotifications extends Component { } render () { + const { + canToggleNotifications + } = this.props; + return ( ); } diff --git a/src/pages/Organizations/screens/OrganizationsList.jsx b/src/pages/Organizations/screens/OrganizationsList.jsx index a3746ebaab..f0d0054715 100644 --- a/src/pages/Organizations/screens/OrganizationsList.jsx +++ b/src/pages/Organizations/screens/OrganizationsList.jsx @@ -62,8 +62,7 @@ class OrganizationsList extends Component { loading: true, results: [], selected: [], - isModalOpen: false, - orgsToDelete: [], + isModalOpen: false }; @@ -74,14 +73,16 @@ class OrganizationsList extends Component { this.onSelectAll = this.onSelectAll.bind(this); this.onSelect = this.onSelect.bind(this); this.updateUrl = this.updateUrl.bind(this); + this.callOrganizations = this.callOrganizations.bind(this); this.fetchOrganizations = this.fetchOrganizations.bind(this); this.handleOrgDelete = this.handleOrgDelete.bind(this); this.handleOpenOrgDeleteModal = this.handleOpenOrgDeleteModal.bind(this); - this.handleClearOrgsToDelete = this.handleClearOrgsToDelete.bind(this); + this.handleCloseOrgDeleteModal = this.handleCloseOrgDeleteModal.bind(this); } componentDidMount () { const queryParams = this.getQueryParams(); + this.callOrganizations(); this.fetchOrganizations(queryParams); } @@ -117,20 +118,20 @@ class OrganizationsList extends Component { onSelectAll (isSelected) { const { results } = this.state; - const selected = isSelected ? results.map(o => o.id) : []; + const selected = isSelected ? results : []; this.setState({ selected }); } - onSelect (id) { + onSelect (row) { const { selected } = this.state; - const isSelected = selected.includes(id); + const isSelected = selected.some(s => s.id === row.id); if (isSelected) { - this.setState({ selected: selected.filter(s => s !== id) }); + this.setState({ selected: selected.filter(s => s.id !== row.id) }); } else { - this.setState({ selected: selected.concat(id) }); + this.setState({ selected: selected.concat(row) }); } } @@ -143,43 +144,35 @@ class OrganizationsList extends Component { return Object.assign({}, this.defaultParams, searchParams, overrides); } - handleClearOrgsToDelete () { + handleCloseOrgDeleteModal () { this.setState({ - isModalOpen: false, - orgsToDelete: [] + isModalOpen: false }); - this.onSelectAll(); } handleOpenOrgDeleteModal () { - const { results, selected } = this.state; + const { selected } = this.state; const warningTitle = selected.length > 1 ? i18nMark('Delete Organization') : i18nMark('Delete Organizations'); const warningMsg = i18nMark('Are you sure you want to delete:'); - - const orgsToDelete = []; - results.forEach((result) => { - selected.forEach((selectedOrg) => { - if (result.id === selectedOrg) { - orgsToDelete.push({ name: result.name, id: selectedOrg }); - } - }); - }); this.setState({ - orgsToDelete, isModalOpen: true, warningTitle, warningMsg, - loading: false }); + loading: false + }); } - async handleOrgDelete (event) { - const { orgsToDelete } = this.state; + async handleOrgDelete () { + const { selected } = this.state; const { api, handleHttpError } = this.props; let errorHandled; try { - await Promise.all(orgsToDelete.map((org) => api.destroyOrganization(org.id))); - this.handleClearOrgsToDelete(); + await Promise.all(selected.map((org) => api.destroyOrganization(org.id))); + this.setState({ + isModalOpen: false, + selected: [] + }); } catch (err) { errorHandled = handleHttpError(err); } finally { @@ -188,7 +181,6 @@ class OrganizationsList extends Component { this.fetchOrganizations(queryParams); } } - event.preventDefault(); } updateUrl (queryParams) { @@ -248,16 +240,35 @@ class OrganizationsList extends Component { } } + async callOrganizations () { + const { api } = this.props; + + try { + const { data } = await api.callOrganizations(); + const { actions } = data; + + const stateToUpdate = { + canAdd: Object.prototype.hasOwnProperty.call(actions, 'POST') + }; + + this.setState(stateToUpdate); + } catch (err) { + this.setState({ error: true }); + } finally { + this.setState({ loading: false }); + } + } + render () { const { medium, } = PageSectionVariants; const { + canAdd, count, error, loading, noInitialResults, - orgsToDelete, page, pageCount, page_size, @@ -270,6 +281,12 @@ class OrganizationsList extends Component { warningMsg, } = this.state; const { match } = this.props; + + const disableDelete = ( + selected.length === 0 + || selected.some(row => !row.summary_fields.user_capabilities.delete) + ); + return ( {({ i18n }) => ( @@ -280,15 +297,15 @@ class OrganizationsList extends Component { variant="danger" title={warningTitle} isOpen={isModalOpen} - onClose={this.handleClearOrgsToDelete} + onClose={this.handleCloseOrgDeleteModal} actions={[ , - + ]} > {warningMsg}
- {orgsToDelete.map((org) => ( + {selected.map((org) => ( {org.name} @@ -321,9 +338,24 @@ class OrganizationsList extends Component { onSort={this.onSort} onSelectAll={this.onSelectAll} onOpenDeleteModal={this.handleOpenOrgDeleteModal} - disableTrashCanIcon={selected.length === 0} + disableTrashCanIcon={disableDelete} + deleteTooltip={ + selected.some(row => !row.summary_fields.user_capabilities.delete) ? ( +
+ + You dont have permission to delete the following Organizations: + + {selected.map(row => ( +
+ {row.name} +
+ ))} +
+ ) : undefined + } showDelete showSelectAll + showAdd={canAdd} />
    { results.map(o => ( @@ -334,8 +366,8 @@ class OrganizationsList extends Component { detailUrl={`${match.url}/${o.id}`} memberCount={o.summary_fields.related_field_counts.users} teamCount={o.summary_fields.related_field_counts.teams} - isSelected={selected.includes(o.id)} - onSelect={() => this.onSelect(o.id, o.name)} + isSelected={selected.some(row => row.id === o.id)} + onSelect={() => this.onSelect(o)} onOpenOrgDeleteModal={this.handleOpenOrgDeleteModal} /> ))} From 38bb4f3f3c24829a0760b527d6a33e5c83e0bd7f Mon Sep 17 00:00:00 2001 From: mabashian Date: Tue, 23 Apr 2019 13:27:00 -0400 Subject: [PATCH 2/6] Fix merge conflicts --- .../DataListToolbar/DataListToolbar.jsx | 22 ++++++------------- src/contexts/Config.jsx | 2 +- .../components/OrganizationAccessList.jsx | 4 ---- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/src/components/DataListToolbar/DataListToolbar.jsx b/src/components/DataListToolbar/DataListToolbar.jsx index a4e6428253..3d8992c69f 100644 --- a/src/components/DataListToolbar/DataListToolbar.jsx +++ b/src/components/DataListToolbar/DataListToolbar.jsx @@ -37,13 +37,11 @@ class DataListToolbar extends React.Component { isCompact, noLeftMargin, onSort, - onSearch + onSearch, onCompact, onExpand, onOpenDeleteModal, - onSearch, onSelectAll, - onSort, showAdd, showDelete, showSelectAll, @@ -160,7 +158,10 @@ DataListToolbar.propTypes = { columns: PropTypes.arrayOf(PropTypes.object).isRequired, deleteTooltip: PropTypes.node, isAllSelected: PropTypes.bool, + isCompact: PropTypes.bool, noLeftMargin: PropTypes.bool, + onCompact: PropTypes.func, + onExpand: PropTypes.func, onSearch: PropTypes.func, onSelectAll: PropTypes.func, onSort: PropTypes.func, @@ -168,11 +169,7 @@ DataListToolbar.propTypes = { showDelete: PropTypes.bool, showSelectAll: PropTypes.bool, sortOrder: PropTypes.string, - sortedColumnKey: PropTypes.string, - onCompact: PropTypes.func, - onExpand: PropTypes.func, - isCompact: PropTypes.bool, - add: PropTypes.node + sortedColumnKey: PropTypes.string }; DataListToolbar.defaultProps = { @@ -181,6 +178,7 @@ DataListToolbar.defaultProps = { deleteTooltip: i18nMark('Delete'), isAllSelected: false, isCompact: false, + noLeftMargin: false, onCompact: null, onExpand: null, onSearch: null, @@ -190,13 +188,7 @@ DataListToolbar.defaultProps = { showDelete: false, showSelectAll: false, sortOrder: 'ascending', - sortedColumnKey: 'name', - isAllSelected: false, - onCompact: null, - onExpand: null, - isCompact: false, - add: null, - noLeftMargin: false + sortedColumnKey: 'name' }; export default DataListToolbar; diff --git a/src/contexts/Config.jsx b/src/contexts/Config.jsx index 10595a7d90..ab4bfed202 100644 --- a/src/contexts/Config.jsx +++ b/src/contexts/Config.jsx @@ -124,7 +124,7 @@ class Provider extends Component { return ( Date: Tue, 23 Apr 2019 14:55:06 -0400 Subject: [PATCH 3/6] Fix existing test failures --- .../components/NotificationList.test.jsx | 7 ++++ .../components/NotificationListItem.test.jsx | 5 +++ .../OrganizationAccessList.test.jsx | 34 ++++++++++++++++++- .../Organization/Organization.test.jsx | 6 +++- .../Organization/OrganizationAccess.test.jsx | 21 +++++++----- .../Organization/OrganizationDetail.test.jsx | 7 +++- .../OrganizationNotifications.test.jsx | 4 +-- .../components/OrganizationAccessList.jsx | 2 ++ 8 files changed, 73 insertions(+), 13 deletions(-) diff --git a/__tests__/components/NotificationList.test.jsx b/__tests__/components/NotificationList.test.jsx index 47ae953746..2261292ee0 100644 --- a/__tests__/components/NotificationList.test.jsx +++ b/__tests__/components/NotificationList.test.jsx @@ -11,6 +11,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={() => {}} onCreateSuccess={() => {}} + canToggleNotifications /> ); }); @@ -24,6 +25,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={() => {}} onCreateSuccess={() => {}} + canToggleNotifications /> ); expect(spy).toHaveBeenCalled(); @@ -38,6 +40,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={() => {}} onCreateSuccess={() => {}} + canToggleNotifications /> ).find('Notifications'); wrapper.instance().toggleNotification(1, true, 'success'); @@ -56,6 +59,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={() => {}} onCreateSuccess={onCreateSuccess} + canToggleNotifications /> ).find('Notifications'); wrapper.setState({ successTemplateIds: [44] }); @@ -76,6 +80,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={() => {}} onCreateSuccess={() => {}} + canToggleNotifications /> ).find('Notifications'); wrapper.instance().toggleNotification(1, true, 'error'); @@ -94,6 +99,7 @@ describe('', () => { onReadSuccess={() => {}} onCreateError={onCreateError} onCreateSuccess={() => {}} + canToggleNotifications /> ).find('Notifications'); wrapper.setState({ errorTemplateIds: [44] }); @@ -144,6 +150,7 @@ describe('', () => { onReadSuccess={onReadSuccess} onCreateError={() => {}} onCreateSuccess={() => {}} + canToggleNotifications /> ).find('Notifications'); wrapper.instance().updateUrl = jest.fn(); diff --git a/__tests__/components/NotificationListItem.test.jsx b/__tests__/components/NotificationListItem.test.jsx index de5303c318..3ce89b72ba 100644 --- a/__tests__/components/NotificationListItem.test.jsx +++ b/__tests__/components/NotificationListItem.test.jsx @@ -20,6 +20,7 @@ describe('', () => { toggleNotification={toggleNotification} detailUrl="/foo" notificationType="slack" + canToggleNotifications /> ); expect(wrapper.length).toBe(1); @@ -33,6 +34,7 @@ describe('', () => { toggleNotification={toggleNotification} detailUrl="/foo" notificationType="slack" + canToggleNotifications /> ); wrapper.find('Switch').first().find('input').simulate('change'); @@ -47,6 +49,7 @@ describe('', () => { toggleNotification={toggleNotification} detailUrl="/foo" notificationType="slack" + canToggleNotifications /> ); wrapper.find('Switch').first().find('input').simulate('change'); @@ -61,6 +64,7 @@ describe('', () => { toggleNotification={toggleNotification} detailUrl="/foo" notificationType="slack" + canToggleNotifications /> ); wrapper.find('Switch').at(1).find('input').simulate('change'); @@ -75,6 +79,7 @@ describe('', () => { toggleNotification={toggleNotification} detailUrl="/foo" notificationType="slack" + canToggleNotifications /> ); wrapper.find('Switch').at(1).find('input').simulate('change'); diff --git a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 42836b436e..88eded387a 100644 --- a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -16,13 +16,31 @@ const mockData = [ role: { name: 'foo', id: 2, + user_capabilities: { + unattach: true + } } } - ], + ] } } ]; +const organization = { + id: 1, + name: 'Default', + summary_fields: { + object_roles: {}, + user_capabilities: { + edit: true + } + } +}; + +const api = { + foo: () => {} +}; + describe('', () => { afterEach(() => { jest.restoreAllMocks(); @@ -33,6 +51,8 @@ describe('', () => { {}} removeRole={() => {}} + api={api} + organization={organization} /> ); }); @@ -42,9 +62,13 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} + api={api} + organization={organization} /> ).find('OrganizationAccessList'); + // expect(wrapper.debug()).toBe(false); + setImmediate(() => { expect(wrapper.state().results).toEqual(mockData); done(); @@ -57,6 +81,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} + api={api} + organization={organization} /> ).find('OrganizationAccessList'); expect(onSort).not.toHaveBeenCalled(); @@ -74,6 +100,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} + api={api} + organization={organization} /> ).find('OrganizationAccessList'); @@ -94,6 +122,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} + api={api} + organization={organization} /> ).find('OrganizationAccessList'); expect(handleWarning).not.toHaveBeenCalled(); @@ -117,6 +147,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} + api={api} + organization={organization} /> ).find('OrganizationAccessList'); diff --git a/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx b/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx index 25a9da73fd..cd7e7a5dbc 100644 --- a/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx @@ -3,7 +3,11 @@ import { mountWithContexts } from '../../../../enzymeHelpers'; import Organization from '../../../../../src/pages/Organizations/screens/Organization/Organization'; describe('', () => { + const me = { + is_super_user: true, + is_system_auditor: false + }; test('initially renders succesfully', () => { - mountWithContexts(); + mountWithContexts(); }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx index fd8748d9e8..a998402345 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationAccess.test.jsx @@ -3,8 +3,12 @@ import { mountWithContexts } from '../../../../enzymeHelpers'; import OrganizationAccess from '../../../../../src/pages/Organizations/screens/Organization/OrganizationAccess'; describe('', () => { + const organization = { + id: 1, + name: 'Default' + }; test('initially renders succesfully', () => { - mountWithContexts(); + mountWithContexts(); }); test('passed methods as props are called appropriately', async () => { @@ -14,13 +18,14 @@ describe('', () => { const mockResponse = { status: 'success', }; - const wrapper = mountWithContexts(, { context: { network: { - api: { - getOrganizationAccessList: () => Promise.resolve(mockAPIAccessList), - disassociate: () => Promise.resolve(mockResponse) - }, - handleHttpError: () => {} - } } }).find('OrganizationAccess'); + const wrapper = mountWithContexts(, + { context: { network: { + api: { + getOrganizationAccessList: () => Promise.resolve(mockAPIAccessList), + disassociate: () => Promise.resolve(mockResponse) + }, + handleHttpError: () => {} + } } }).find('OrganizationAccess'); const accessList = await wrapper.instance().getOrgAccessList(); expect(accessList).toEqual(mockAPIAccessList); const resp = await wrapper.instance().removeRole(2, 3, 'users'); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx index 177ff714b1..8636e3bce9 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx @@ -8,7 +8,12 @@ describe('', () => { description: 'Bar', custom_virtualenv: 'Fizz', created: 'Bat', - modified: 'Boo' + modified: 'Boo', + summary_fields: { + user_capabilities: { + edit: true + } + } }; test('initially renders succesfully', () => { diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationNotifications.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationNotifications.test.jsx index 560565c931..5b10cda0c9 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationNotifications.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationNotifications.test.jsx @@ -18,7 +18,7 @@ describe('', () => { test('initially renders succesfully', () => { mountWithContexts( - , { context: { network: { + , { context: { network: { api, handleHttpError: () => {} } } } @@ -26,7 +26,7 @@ describe('', () => { }); test('handles api requests', () => { const wrapper = mountWithContexts( - , { context: { network: { + , { context: { network: { api, handleHttpError: () => {} } } } diff --git a/src/pages/Organizations/components/OrganizationAccessList.jsx b/src/pages/Organizations/components/OrganizationAccessList.jsx index 53b3dc4d5d..6085d31e8e 100644 --- a/src/pages/Organizations/components/OrganizationAccessList.jsx +++ b/src/pages/Organizations/components/OrganizationAccessList.jsx @@ -476,7 +476,9 @@ class OrganizationAccessList extends React.Component { } OrganizationAccessList.propTypes = { + api: PropTypes.shape().isRequired, getAccessList: PropTypes.func.isRequired, + organization: PropTypes.shape().isRequired, removeRole: PropTypes.func.isRequired }; From 82db7df6b35de234cf1d8cabd5f0fec12a506897 Mon Sep 17 00:00:00 2001 From: mabashian Date: Tue, 23 Apr 2019 15:19:46 -0400 Subject: [PATCH 4/6] Remove errant comment --- .../Organizations/components/OrganizationAccessList.test.jsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 88eded387a..2cf1c3a24e 100644 --- a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -67,8 +67,6 @@ describe('', () => { /> ).find('OrganizationAccessList'); - // expect(wrapper.debug()).toBe(false); - setImmediate(() => { expect(wrapper.state().results).toEqual(mockData); done(); From e5dda696d7305c941c911b001be81f60575ebc12 Mon Sep 17 00:00:00 2001 From: mabashian Date: Wed, 24 Apr 2019 10:09:17 -0400 Subject: [PATCH 5/6] Add new tests for rbac on some of the org pages --- .../OrganizationAccessList.test.jsx | 34 +++++++++++++++++++ .../Organization/Organization.test.jsx | 10 ++++++ .../Organization/OrganizationDetail.test.jsx | 24 +++++++++++++ .../screens/Organization/Organization.jsx | 21 ++++++------ 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 2cf1c3a24e..24473e3453 100644 --- a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -176,4 +176,38 @@ describe('', () => { done(); }); }); + + test('add role button visible for user that can edit org', () => { + const wrapper = mountWithContexts( + ({ data: { count: 1, results: mockData } })} + removeRole={() => {}} + api={api} + organization={organization} + /> + ).find('OrganizationAccessList'); + + setImmediate(() => { + const addRole = wrapper.update().find('DataListToolbar').find('PlusIcon'); + expect(addRole.length).toBe(1); + }); + }); + + test('add role button hidden for user that cannot edit org', () => { + const readOnlyOrg = { ...organization }; + readOnlyOrg.summary_fields.user_capabilities.edit = false; + const wrapper = mountWithContexts( + ({ data: { count: 1, results: mockData } })} + removeRole={() => {}} + api={api} + organization={readOnlyOrg} + /> + ).find('OrganizationAccessList'); + + setImmediate(() => { + const addRole = wrapper.update().find('DataListToolbar').find('PlusIcon'); + expect(addRole.length).toBe(0); + }); + }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx b/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx index cd7e7a5dbc..21e69ac58f 100644 --- a/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/Organization.test.jsx @@ -10,4 +10,14 @@ describe('', () => { test('initially renders succesfully', () => { mountWithContexts(); }); + test('notifications tab shown/hidden based on permissions', () => { + const wrapper = mountWithContexts(); + expect(wrapper.find('.pf-c-tabs__item').length).toBe(3); + expect(wrapper.find('.pf-c-tabs__button[children="Notifications"]').length).toBe(0); + wrapper.find('Organization').setState({ + isNotifAdmin: true + }); + expect(wrapper.find('.pf-c-tabs__item').length).toBe(4); + expect(wrapper.find('.pf-c-tabs__button[children="Notifications"]').length).toBe(1); + }); }); diff --git a/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx b/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx index 8636e3bce9..3b7ddda352 100644 --- a/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx +++ b/__tests__/pages/Organizations/screens/Organization/OrganizationDetail.test.jsx @@ -88,4 +88,28 @@ describe('', () => { expect(modifiedDetail.find('h6').text()).toBe('Last Modified'); expect(modifiedDetail.find('p').text()).toBe('Boo'); }); + + test('should show edit button for users with edit permission', () => { + const wrapper = mountWithContexts( + + ).find('OrganizationDetail'); + + const editLink = wrapper.findWhere(node => node.props().to === '/organizations/undefined/edit'); + expect(editLink.length).toBe(1); + }); + + test('should hide edit button for users without edit permission', () => { + const readOnlyOrg = { ...mockDetails }; + readOnlyOrg.summary_fields.user_capabilities.edit = false; + const wrapper = mountWithContexts( + + ).find('OrganizationDetail'); + + const editLink = wrapper.findWhere(node => node.props().to === '/organizations/undefined/edit'); + expect(editLink.length).toBe(0); + }); }); diff --git a/src/pages/Organizations/screens/Organization/Organization.jsx b/src/pages/Organizations/screens/Organization/Organization.jsx index da0c526f25..e4d8a837ec 100644 --- a/src/pages/Organizations/screens/Organization/Organization.jsx +++ b/src/pages/Organizations/screens/Organization/Organization.jsx @@ -135,14 +135,18 @@ class Organization extends Component { || isAdminOfThisOrg ); - const tabElements = [ - { name: i18nMark('Details'), link: `${match.url}/details` }, - { name: i18nMark('Access'), link: `${match.url}/access` }, - { name: i18nMark('Teams'), link: `${match.url}/teams` } + const tabsArray = [ + { name: i18nMark('Details'), link: `${match.url}/details`, id: 0 }, + { name: i18nMark('Access'), link: `${match.url}/access`, id: 1 }, + { name: i18nMark('Teams'), link: `${match.url}/teams`, id: 2 } ]; if (canSeeNotificationsTab) { - tabElements.push({ name: i18nMark('Notifications'), link: `${match.url}/notifications` }); + tabsArray.push({ + name: i18nMark('Notifications'), + link: `${match.url}/notifications`, + id: 3 + }); } let cardHeader = ( @@ -158,12 +162,7 @@ class Organization extends Component { match={match} history={history} labeltext={i18n._(t`Organization detail tabs`)} - tabsArray={[ - { name: i18nMark('Details'), link: `${match.url}/details`, id: 0 }, - { name: i18nMark('Access'), link: `${match.url}/access`, id: 1 }, - { name: i18nMark('Teams'), link: `${match.url}/teams`, id: 2 }, - { name: i18nMark('Notifications'), link: `${match.url}/notifications`, id: 3 }, - ]} + tabsArray={tabsArray} /> Date: Fri, 26 Apr 2019 11:02:16 -0400 Subject: [PATCH 6/6] Code cleanup, renaming functions, use .all() on config promises --- CONTRIBUTING.md | 1 + .../OrganizationAccessList.test.jsx | 24 +++++--------- src/api.js | 2 +- src/contexts/Config.jsx | 31 +++++++------------ src/pages/Login.jsx | 2 +- .../screens/Organization/Organization.jsx | 3 -- .../screens/OrganizationsList.jsx | 21 +++++++------ 7 files changed, 34 insertions(+), 50 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d6d958fe92..8ec52296db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -171,6 +171,7 @@ Here are the guidelines for how to name functions. |`replace`| Use for methods that make API `PUT` requests | |`disassociate`| Use for methods that pass `{ disassociate: true }` as a data param to an endpoint | |`associate`| Use for methods that pass a resource id as a data param to an endpoint | +|`can`| Use for props dealing with RBAC to denote whether a user has access to something | ### Default State Initialization When declaring empty initial states, prefer the following instead of leaving them undefined: diff --git a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx index 24473e3453..1ea9dc9868 100644 --- a/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx +++ b/__tests__/pages/Organizations/components/OrganizationAccessList.test.jsx @@ -51,9 +51,8 @@ describe('', () => { {}} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ); }); @@ -62,9 +61,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); setImmediate(() => { @@ -79,9 +77,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); expect(onSort).not.toHaveBeenCalled(); @@ -98,9 +95,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); setImmediate(() => { @@ -120,9 +116,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); expect(handleWarning).not.toHaveBeenCalled(); expect(confirmDelete).not.toHaveBeenCalled(); @@ -145,9 +140,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); setImmediate(() => { @@ -182,9 +176,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={organization} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); setImmediate(() => { @@ -200,9 +193,8 @@ describe('', () => { ({ data: { count: 1, results: mockData } })} removeRole={() => {}} - api={api} organization={readOnlyOrg} - /> + />, { context: { network: { api } } } ).find('OrganizationAccessList'); setImmediate(() => { diff --git a/src/api.js b/src/api.js index 9767632c6a..96842e64b6 100644 --- a/src/api.js +++ b/src/api.js @@ -76,7 +76,7 @@ class APIClient { return this.http.post(API_ORGANIZATIONS, data); } - callOrganizations () { + optionsOrganizations () { return this.http.options(API_ORGANIZATIONS); } diff --git a/src/contexts/Config.jsx b/src/contexts/Config.jsx index ab4bfed202..c5dfa6ed8f 100644 --- a/src/contexts/Config.jsx +++ b/src/contexts/Config.jsx @@ -78,28 +78,19 @@ class Provider extends Component { const { api, handleHttpError } = this.props; try { - const { - data: { - ansible_version, - custom_virtualenvs, - version - } - } = await api.getConfig(); - const { - data: { - custom_logo, - custom_login_info - } - } = await api.getRoot(); - const { data: { results: [me] } } = await api.getMe(); + const [configRes, rootRes, meRes] = await Promise.all([ + api.getConfig(), + api.getRoot(), + api.getMe() + ]); this.setState({ value: { - ansible_version, - custom_virtualenvs, - version, - custom_logo, - custom_login_info, - me + ansible_version: configRes.data.ansible_version, + custom_virtualenvs: configRes.data.custom_virtualenvs, + version: configRes.data.version, + custom_logo: rootRes.data.custom_logo, + custom_login_info: rootRes.data.custom_login_info, + me: meRes.data.results } }); } catch (err) { diff --git a/src/pages/Login.jsx b/src/pages/Login.jsx index 849aaa6cc5..27ca4cfb5f 100644 --- a/src/pages/Login.jsx +++ b/src/pages/Login.jsx @@ -53,7 +53,7 @@ class AWXLogin extends Component { try { const { data } = await api.login(username, password); updateConfig(data); - fetchMe(); + await fetchMe(); this.setState({ isAuthenticated: true, isLoading: false }); } catch (error) { handleHttpError(error) || this.setState({ isInputValid: false, isLoading: false }); diff --git a/src/pages/Organizations/screens/Organization/Organization.jsx b/src/pages/Organizations/screens/Organization/Organization.jsx index e4d8a837ec..cb267f1391 100644 --- a/src/pages/Organizations/screens/Organization/Organization.jsx +++ b/src/pages/Organizations/screens/Organization/Organization.jsx @@ -240,9 +240,6 @@ class Organization extends Component { path="/organizations/:id/notifications" render={() => ( )} diff --git a/src/pages/Organizations/screens/OrganizationsList.jsx b/src/pages/Organizations/screens/OrganizationsList.jsx index f0d0054715..035665ed3b 100644 --- a/src/pages/Organizations/screens/OrganizationsList.jsx +++ b/src/pages/Organizations/screens/OrganizationsList.jsx @@ -73,7 +73,7 @@ class OrganizationsList extends Component { this.onSelectAll = this.onSelectAll.bind(this); this.onSelect = this.onSelect.bind(this); this.updateUrl = this.updateUrl.bind(this); - this.callOrganizations = this.callOrganizations.bind(this); + this.fetchOptionsOrganizations = this.fetchOptionsOrganizations.bind(this); this.fetchOrganizations = this.fetchOrganizations.bind(this); this.handleOrgDelete = this.handleOrgDelete.bind(this); this.handleOpenOrgDeleteModal = this.handleOpenOrgDeleteModal.bind(this); @@ -82,7 +82,7 @@ class OrganizationsList extends Component { componentDidMount () { const queryParams = this.getQueryParams(); - this.callOrganizations(); + this.fetchOptionsOrganizations(); this.fetchOrganizations(queryParams); } @@ -240,11 +240,11 @@ class OrganizationsList extends Component { } } - async callOrganizations () { + async fetchOptionsOrganizations () { const { api } = this.props; try { - const { data } = await api.callOrganizations(); + const { data } = await api.optionsOrganizations(); const { actions } = data; const stateToUpdate = { @@ -345,11 +345,14 @@ class OrganizationsList extends Component { You dont have permission to delete the following Organizations: - {selected.map(row => ( -
    - {row.name} -
    - ))} + {selected + .filter(row => !row.summary_fields.user_capabilities.delete) + .map(row => ( +
    + {row.name} +
    + )) + } ) : undefined }