Skip to content

Commit

Permalink
[8.x] [Spaces] Open 'manage roles' link for spaces assign r…
Browse files Browse the repository at this point in the history
…ole flyout in new tab (#199506) (#200228)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Spaces] Open 'manage roles' link for spaces assign role
flyout in new tab
(#199506)](#199506)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Eyo O.
Eyo","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-14T17:28:50Z","message":"[Spaces]
Open 'manage roles' link for spaces assign role flyout in new tab
(#199506)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana-team/issues/1281\r\n\r\nModify the
\"manage role\" link in the assign roles to space tab, so it\r\nopens
the roles screen in a new tab, with an improvement so that
on\r\ntransitioning back to the assign roles space tab the user is
presented\r\nwith an updated list of roles created in the page that was
created\r\nopened, if any.\r\n\r\nCaveat:\r\n\r\nThis approach will
continually make calls to refresh the role list on\r\nevery page
visibility event that matches the conditions provided until\r\nthe
flyout gets closed.\r\n\r\n#####
Visuals\r\n\r\n\r\nhttps://github.com/user-attachments/assets/64cb296d-246d-4033-a655-7b10d0dafab1\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n<!--\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n<!--\r\n- [ ]
[Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for
breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n-->","sha":"190430b0b2011235963af069a0196cfafb7a5cd5","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Security","release_note:skip","v9.0.0","backport:prev-minor","ci:cloud-deploy"],"title":"[Spaces]
Open 'manage roles' link for spaces assign role flyout in new
tab","number":199506,"url":"https://github.com/elastic/kibana/pull/199506","mergeCommit":{"message":"[Spaces]
Open 'manage roles' link for spaces assign role flyout in new tab
(#199506)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana-team/issues/1281\r\n\r\nModify the
\"manage role\" link in the assign roles to space tab, so it\r\nopens
the roles screen in a new tab, with an improvement so that
on\r\ntransitioning back to the assign roles space tab the user is
presented\r\nwith an updated list of roles created in the page that was
created\r\nopened, if any.\r\n\r\nCaveat:\r\n\r\nThis approach will
continually make calls to refresh the role list on\r\nevery page
visibility event that matches the conditions provided until\r\nthe
flyout gets closed.\r\n\r\n#####
Visuals\r\n\r\n\r\nhttps://github.com/user-attachments/assets/64cb296d-246d-4033-a655-7b10d0dafab1\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n<!--\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n<!--\r\n- [ ]
[Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for
breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n-->","sha":"190430b0b2011235963af069a0196cfafb7a5cd5"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199506","number":199506,"mergeCommit":{"message":"[Spaces]
Open 'manage roles' link for spaces assign role flyout in new tab
(#199506)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana-team/issues/1281\r\n\r\nModify the
\"manage role\" link in the assign roles to space tab, so it\r\nopens
the roles screen in a new tab, with an improvement so that
on\r\ntransitioning back to the assign roles space tab the user is
presented\r\nwith an updated list of roles created in the page that was
created\r\nopened, if any.\r\n\r\nCaveat:\r\n\r\nThis approach will
continually make calls to refresh the role list on\r\nevery page
visibility event that matches the conditions provided until\r\nthe
flyout gets closed.\r\n\r\n#####
Visuals\r\n\r\n\r\nhttps://github.com/user-attachments/assets/64cb296d-246d-4033-a655-7b10d0dafab1\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n<!--\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n<!--\r\n- [ ]
[Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for
breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n-->","sha":"190430b0b2011235963af069a0196cfafb7a5cd5"}}]}]
BACKPORT-->

Co-authored-by: Eyo O. Eyo <[email protected]>
  • Loading branch information
kibanamachine and eokoneyo authored Nov 14, 2024
1 parent 070cbba commit a878dc8
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import { EuiThemeProvider } from '@elastic/eui';
import { render, screen, waitFor, within } from '@testing-library/react';
import { fireEvent, render, screen, waitFor, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import crypto from 'crypto';
import React from 'react';
Expand Down Expand Up @@ -142,6 +142,15 @@ describe('PrivilegesRolesForm', () => {
jest.clearAllMocks();
});

it("would open the 'manage roles' link in a new tab", () => {
getRolesSpy.mockResolvedValue([]);
getAllKibanaPrivilegeSpy.mockResolvedValue(createRawKibanaPrivileges(kibanaFeatures));

renderPrivilegeRolesForm();

expect(screen.getByText('Manage roles')).toHaveAttribute('target', '_blank');
});

it('does not display the privilege selection buttons or customization form when no role is selected', async () => {
getRolesSpy.mockResolvedValue([]);
getAllKibanaPrivilegeSpy.mockResolvedValue(createRawKibanaPrivileges(kibanaFeatures));
Expand Down Expand Up @@ -170,6 +179,23 @@ describe('PrivilegesRolesForm', () => {
expect(screen.getByTestId('space-assign-role-create-roles-privilege-button')).toBeDisabled();
});

it('makes a request to refetch available roles if page transitions back from a user interaction page visibility change', () => {
getRolesSpy.mockResolvedValue([]);
getAllKibanaPrivilegeSpy.mockResolvedValue(createRawKibanaPrivileges(kibanaFeatures));

renderPrivilegeRolesForm();

expect(getRolesSpy).toHaveBeenCalledTimes(1);

// trigger click on manage roles link, which is perquisite for page visibility handler to trigger role refetch
fireEvent.click(screen.getByText(/manage roles/i));

// trigger page visibility change
fireEvent(document, new Event('visibilitychange'));

expect(getRolesSpy).toHaveBeenCalledTimes(2);
});

it('renders with the assign roles button disabled when no base privileges or feature privileges are selected', async () => {
getRolesSpy.mockResolvedValue([]);
getAllKibanaPrivilegeSpy.mockResolvedValue(createRawKibanaPrivileges(kibanaFeatures));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
EuiSpacer,
EuiText,
EuiTitle,
useGeneratedHtmlId,
} from '@elastic/eui';
import type { EuiComboBoxOptionOption } from '@elastic/eui';
import type { FC } from 'react';
Expand Down Expand Up @@ -81,10 +82,13 @@ export const PrivilegesRolesForm: FC<PrivilegesRolesFormProps> = (props) => {
const [selectedRoles, setSelectedRoles] = useState<ReturnType<typeof createRolesComboBoxOptions>>(
createRolesComboBoxOptions(defaultSelected)
);
const manageRoleLinkId = useGeneratedHtmlId();
const isEditOperation = useRef(Boolean(defaultSelected.length));

useEffect(() => {
async function fetchRequiredData(spaceId: string) {
const userInvokedPageVisibilityChange = useRef<boolean | null>(null);

const fetchRequiredData = useCallback(
async (spaceId: string) => {
setFetchingDataDeps(true);

const [systemRoles, _kibanaPrivileges] = await invokeClient((clients) =>
Expand All @@ -109,10 +113,28 @@ export const PrivilegesRolesForm: FC<PrivilegesRolesFormProps> = (props) => {
);

setKibanaPrivileges(_kibanaPrivileges);
}
},
[invokeClient]
);

useEffect(() => {
fetchRequiredData(space.id!).finally(() => setFetchingDataDeps(false));
}, [invokeClient, space.id]);
}, [fetchRequiredData, invokeClient, space.id]);

useEffect(() => {
async function visibilityChangeHandler() {
// page just transitioned back to visible state from hidden state caused by user interaction
if (userInvokedPageVisibilityChange.current && !document.hidden) {
await fetchRequiredData(space.id!).finally(() => setFetchingDataDeps(false));
}
}

document.addEventListener('visibilitychange', visibilityChangeHandler);

return () => {
document.removeEventListener('visibilitychange', visibilityChangeHandler);
};
}, [fetchRequiredData, invokeClient, space.id]);

const selectedRolesCombinedPrivileges = useMemo(() => {
const combinedPrivilege = new Set(
Expand Down Expand Up @@ -345,12 +367,23 @@ export const PrivilegesRolesForm: FC<PrivilegesRolesFormProps> = (props) => {
<React.Fragment>
{!isEditOperation.current && (
<EuiFormRow
onClick={(event: React.MouseEvent<HTMLFieldSetElement>) => {
// leverage event propagation, check if manage role link element was clicked
if ((event.target as HTMLFieldSetElement).id === manageRoleLinkId) {
userInvokedPageVisibilityChange.current = true;
}
}}
label={i18n.translate(
'xpack.spaces.management.spaceDetails.roles.selectRolesFormRowLabel',
{ defaultMessage: 'Select roles' }
)}
labelAppend={
<EuiLink href={getUrlForApp('management', { deepLinkId: 'roles' })}>
<EuiLink
id={manageRoleLinkId}
href={getUrlForApp('management', { deepLinkId: 'roles' })}
external={false}
target="_blank"
>
{i18n.translate(
'xpack.spaces.management.spaceDetails.roles.selectRolesFormRowLabelAnchor',
{ defaultMessage: 'Manage roles' }
Expand Down

0 comments on commit a878dc8

Please sign in to comment.