address PR feedback

x-pack/plugins/security/common/model/role.ts

@@ -71,15 +71,6 @@ export function isRoleDeprecated(role: Partial<Role>) {
   return role.metadata?._deprecated ?? false;
 }
 
-/**
- * Returns the reason this role is deprecated.
- *
- * @param role the Role as returned by roles API
- */
-export function getRoleDeprecatedReason(role: Partial<Role>) {
-  return role.metadata?._deprecated_reason ?? '';
-}
-
 /**
  * Returns the extended deprecation notice for the provided role.
  *
@@ -124,3 +115,12 @@ export function prepareRoleClone(role: Role): Role {
 
   return clone;
 }
+
+/**
+ * Returns the reason this role is deprecated.
+ *
+ * @param role the Role as returned by roles API
+ */
+function getRoleDeprecatedReason(role: Partial<Role>) {
+  return role.metadata?._deprecated_reason ?? '';
+}

x-pack/plugins/security/public/management/role_combo_box/role_combo_box.test.tsx

@@ -0,0 +1,110 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import React from 'react';
+import { mountWithIntl, nextTick } from 'test_utils/enzyme_helpers';
+
+import { RoleComboBox } from '.';
+import { EuiComboBox } from '@elastic/eui';
+import { findTestSubject } from 'test_utils/find_test_subject';
+
+describe('RoleComboBox', () => {
+  it('renders the provided list of roles via EuiComboBox options', () => {
+    const availableRoles = [
+      {
+        name: 'role-1',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [],
+        metadata: {},
+      },
+      {
+        name: 'role-2',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [],
+        metadata: {},
+      },
+    ];
+    const wrapper = mountWithIntl(
+      <RoleComboBox availableRoles={availableRoles} selectedRoleNames={[]} onChange={jest.fn()} />
+    );
+
+    expect(wrapper.find(EuiComboBox).props().options).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "color": "default",
+          "data-test-subj": "roleOption-role-1",
+          "label": "role-1",
+          "value": Object {
+            "isDeprecated": false,
+          },
+        },
+        Object {
+          "color": "default",
+          "data-test-subj": "roleOption-role-2",
+          "label": "role-2",
+          "value": Object {
+            "isDeprecated": false,
+          },
+        },
+      ]
+    `);
+  });
+
+  it('renders deprecated roles as such', () => {
+    const availableRoles = [
+      {
+        name: 'role-1',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [],
+        metadata: { _deprecated: true },
+      },
+    ];
+    const wrapper = mountWithIntl(
+      <RoleComboBox availableRoles={availableRoles} selectedRoleNames={[]} onChange={jest.fn()} />
+    );
+
+    expect(wrapper.find(EuiComboBox).props().options).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "color": "warning",
+          "data-test-subj": "roleOption-role-1",
+          "label": "role-1",
+          "value": Object {
+            "isDeprecated": true,
+          },
+        },
+      ]
+    `);
+  });
+
+  it('renders the selected role names in the expanded list, coded according to deprecated status', () => {
+    const availableRoles = [
+      {
+        name: 'role-1',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [],
+        metadata: {},
+      },
+      {
+        name: 'role-2',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [],
+        metadata: {},
+      },
+    ];
+    const wrapper = mountWithIntl(
+      <div>
+        <RoleComboBox availableRoles={availableRoles} selectedRoleNames={[]} onChange={jest.fn()} />
+      </div>
+    );
+
+    findTestSubject(wrapper, 'comboBoxToggleListButton').simulate('click');
+
+    wrapper.find(EuiComboBox).setState({ isListOpen: true });
+
+    expect(findTestSubject(wrapper, 'rolesDropdown-renderOption')).toMatchInlineSnapshot(`null`);
+  });
+});

x-pack/plugins/security/public/management/role_combo_box/role_combo_box.tsx

@@ -6,8 +6,9 @@
 
 import React from 'react';
 import { i18n } from '@kbn/i18n';
-import { EuiComboBox, EuiText } from '@elastic/eui';
+import { EuiComboBox } from '@elastic/eui';
 import { Role, isRoleDeprecated } from '../../../common/model';
+import { RoleComboBoxOption } from './role_combo_box_option';
 
 interface Props {
   availableRoles: Role[];
@@ -25,7 +26,7 @@ export const RoleComboBox = (props: Props) => {
 
   const roleNameToOption = (roleName: string) => {
     const roleDefinition = props.availableRoles.find(role => role.name === roleName);
-    const isDeprecated = roleDefinition && isRoleDeprecated(roleDefinition);
+    const isDeprecated: boolean = (roleDefinition && isRoleDeprecated(roleDefinition)) ?? false;
     return {
       color: isDeprecated ? 'warning' : 'default',
       'data-test-subj': `roleOption-${roleName}`,
@@ -54,20 +55,7 @@ export const RoleComboBox = (props: Props) => {
       isDisabled={props.isDisabled}
       options={options}
       selectedOptions={selectedOptions}
-      renderOption={option => {
-        const isDeprecated = option.value!.isDeprecated;
-        const deprecatedLabel = i18n.translate(
-          'xpack.security.management.users.editUser.deprecatedRoleText',
-          {
-            defaultMessage: '(deprecated)',
-          }
-        );
-        return (
-          <EuiText color={option.color as any}>
-            {option.label} {isDeprecated ? deprecatedLabel : ''}
-          </EuiText>
-        );
-      }}
+      renderOption={option => <RoleComboBoxOption option={option} />}
     />
   );
 };

x-pack/plugins/security/public/management/role_combo_box/role_combo_box_option.test.tsx

@@ -0,0 +1,57 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import React from 'react';
+import { shallowWithIntl } from 'test_utils/enzyme_helpers';
+import { RoleComboBoxOption } from './role_combo_box_option';
+
+describe('RoleComboBoxOption', () => {
+  it('renders a regular role correctly', () => {
+    const wrapper = shallowWithIntl(
+      <RoleComboBoxOption
+        option={{
+          color: 'default',
+          label: 'role-1',
+        }}
+      />
+    );
+
+    expect(wrapper).toMatchInlineSnapshot(`
+      <EuiText
+        color="default"
+        data-test-subj="rolesDropdown-renderOption"
+      >
+        role-1
+         
+      </EuiText>
+    `);
+  });
+
+  it('renders a deprecated role correctly', () => {
+    const wrapper = shallowWithIntl(
+      <RoleComboBoxOption
+        option={{
+          color: 'warning',
+          label: 'role-1',
+          value: {
+            isDeprecated: true,
+          },
+        }}
+      />
+    );
+
+    expect(wrapper).toMatchInlineSnapshot(`
+      <EuiText
+        color="warning"
+        data-test-subj="rolesDropdown-renderOption"
+      >
+        role-1
+         
+        (deprecated)
+      </EuiText>
+    `);
+  });
+});

x-pack/plugins/security/public/management/role_combo_box/role_combo_box_option.tsx

@@ -0,0 +1,31 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import React from 'react';
+
+import { i18n } from '@kbn/i18n';
+
+import { EuiComboBoxOptionProps, EuiText } from '@elastic/eui';
+
+interface Props {
+  option: EuiComboBoxOptionProps<{ isDeprecated: boolean }>;
+}
+
+export const RoleComboBoxOption = ({ option }: Props) => {
+  const isDeprecated = option.value?.isDeprecated ?? false;
+  const deprecatedLabel = i18n.translate(
+    'xpack.security.management.users.editUser.deprecatedRoleText',
+    {
+      defaultMessage: '(deprecated)',
+    }
+  );
+
+  return (
+    <EuiText color={option.color as any} data-test-subj="rolesDropdown-renderOption">
+      {option.label} {isDeprecated ? deprecatedLabel : ''}
+    </EuiText>
+  );
+};

x-pack/plugins/security/public/management/roles/roles_grid/roles_grid_page.test.tsx

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { EuiIcon } from '@elastic/eui';
+import { EuiIcon, EuiBasicTable } from '@elastic/eui';
 import { ReactWrapper } from 'enzyme';
 import React from 'react';
 import { mountWithIntl } from 'test_utils/enzyme_helpers';
@@ -15,6 +15,7 @@ import { RolesGridPage } from './roles_grid_page';
 import { coreMock } from '../../../../../../../src/core/public/mocks';
 import { rolesAPIClientMock } from '../index.mock';
 import { ReservedBadge, DisabledBadge } from '../../badges';
+import { findTestSubject } from 'test_utils/find_test_subject';
 
 const mock403 = () => ({ body: { statusCode: 403 } });
 
@@ -140,4 +141,54 @@ describe('<RolesGridPage />', () => {
       wrapper.find('EuiButtonIcon[data-test-subj="clone-role-action-disabled-role"]')
     ).toHaveLength(1);
   });
+
+  it('hides reserved roles when instructed to', async () => {
+    const wrapper = mountWithIntl(
+      <RolesGridPage
+        rolesAPIClient={apiClientMock}
+        notifications={coreMock.createStart().notifications}
+      />
+    );
+    const initialIconCount = wrapper.find(EuiIcon).length;
+
+    await waitForRender(wrapper, updatedWrapper => {
+      return updatedWrapper.find(EuiIcon).length > initialIconCount;
+    });
+
+    expect(wrapper.find(EuiBasicTable).props().items).toEqual([
+      {
+        name: 'disabled-role',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [{ base: [], spaces: [], feature: {} }],
+        transient_metadata: { enabled: false },
+      },
+      {
+        name: 'reserved-role',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [{ base: [], spaces: [], feature: {} }],
+        metadata: { _reserved: true },
+      },
+      {
+        name: 'test-role-1',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [{ base: [], spaces: [], feature: {} }],
+      },
+    ]);
+
+    findTestSubject(wrapper, 'showReservedRolesSwitch').simulate('click');
+
+    expect(wrapper.find(EuiBasicTable).props().items).toEqual([
+      {
+        name: 'disabled-role',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [{ base: [], spaces: [], feature: {} }],
+        transient_metadata: { enabled: false },
+      },
+      {
+        name: 'test-role-1',
+        elasticsearch: { cluster: [], indices: [], run_as: [] },
+        kibana: [{ base: [], spaces: [], feature: {} }],
+      },
+    ]);
+  });
 });

x-pack/plugins/security/public/management/roles/roles_grid/roles_grid_page.tsx

@@ -377,6 +377,7 @@ export class RolesGridPage extends Component<Props, State> {
   private renderToolsRight() {
     return (
       <EuiSwitch
+        data-test-subj="showReservedRolesSwitch"
         label={
           <FormattedMessage
             id="xpack.security.management.roles.showReservedRolesLabel"