Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Spaces] Dynamically set the space disabled feature based on the space solution view #191927

Merged
merged 9 commits into from
Sep 17, 2024
Merged

[Spaces] Dynamically set the space disabled feature based on the space solution view #191927

merged 9 commits into from
Sep 17, 2024

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Sep 2, 2024 •
edited by kibanamachine
Loading

In this PR I've dynamically set the space disabledFeatures based on the space solution property.

When a space solution is defined (and its value is not "classic"), we want to automatically disable the features that belong to the other solutions. E.g. the space solution is set to security, we add to the disabledFeatures array all the features that belong to observability or enterpriseSearch.

@sebelga sebelga force-pushed the spaces/dynamic-feature-visibility-from-solution branch from a20d349 to 935d17c Compare September 3, 2024 10:45
@sebelga
Copy link
Contributor Author

sebelga commented Sep 3, 2024

/ci

@sebelga sebelga self-assigned this Sep 3, 2024
@sebelga sebelga added Feature:Security/Spaces Platform Security - Spaces feature Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Sep 3, 2024
@sebelga
Copy link
Contributor Author

sebelga commented Sep 3, 2024

/ci

@sebelga
Copy link
Contributor Author

sebelga commented Sep 3, 2024

/ci

@sebelga sebelga marked this pull request as ready for review September 3, 2024 14:10
@sebelga sebelga requested a review from a team as a code owner September 3, 2024 14:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Sep 3, 2024
legrego
legrego reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the way you encapsulated and reused this new logic. Thanks for taking this on!

x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.test.ts Show resolved Hide resolved
x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.test.ts Show resolved Hide resolved
@sebelga
Copy link
Contributor Author

sebelga commented Sep 16, 2024

Thanks for the review @legrego ! 👍 I addressed the changes, can you have another look?

Also, I was wondering if we should not also update the server client transformSavedObjectToSpace method so all consumer get the same dynamic values? And with that change I guess that the browser client would also then get updated array of disabled features. WDYT?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @sebelga

@sebelga sebelga requested a review from legrego September 17, 2024 13:18
legrego
legrego approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

Also, I was wondering if we should not also update the server client transformSavedObjectToSpace method so all consumer get the same dynamic values? And with that change I guess that the browser client would also then get updated array of disabled features. WDYT?

@sebelga I don't think we should do this, yet. One of the benefits of leaving it alone is that it allows a user to switch back to classic, and retain their original set of disabledFeatures. I'm sure there are ways to solve for this, but I'd rather not explore that unless there is a compelling reason to do so.

@sebelga
Copy link
Contributor Author

sebelga commented Sep 17, 2024

I don't think we should do this, yet. One of the benefits of leaving it alone is that it allows a user to switch back to classic, and retain their original set of disabledFeatures. I'm sure there are ways to solve for this, but I'd rather not explore that unless there is a compelling reason to do so.

Mmmm.. but they would. This is just dynamic values that we set at runtime. What is stored in the SO would not be affected and when the user switches back to "classic" the array they had previously stored will be returned.

With what I am suggesting we would be consistent accross all Kibana with thatdisabledFeatures array. Imagine a part of Kibana UI that checks if the space has X in their disabled feature and if they do the UI is different.
So to be clear, what I was suggesting is only at the read moment (get or getAll).

@sebelga sebelga merged commit 735380a into elastic:main Sep 17, 2024
19 checks passed
@sebelga sebelga deleted the spaces/dynamic-feature-visibility-from-solution branch September 17, 2024 16:52
@kibanamachine kibanamachine added v9.0.0 backport:skip This commit does not require backporting labels Sep 17, 2024
@sebelga sebelga added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Sep 18, 2024
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 191927

Questions ?

Please refer to the Backport tool documentation

@sebelga sebelga mentioned this pull request Sep 18, 2024
Merged
@sebelga
Copy link
Contributor Author

sebelga commented Sep 18, 2024

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

sebelga added a commit to sebelga/kibana that referenced this pull request Sep 18, 2024
@sebelga
[Spaces] Dynamically set the space disabled feature based on the spac…
89ed781 
…e solution view (elastic#191927)

(cherry picked from commit 735380a)

# Conflicts:
#	x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts
sebelga added a commit that referenced this pull request Sep 19, 2024
@sebelga
[8.x] [Spaces] Dynamically set the space disabled feature based on th…
dc13a02 
…e space solution view (#191927) (#193299)
@mgiota mgiota mentioned this pull request Sep 20, 2024
Closed
@sebelga sebelga mentioned this pull request Oct 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legrego legrego legrego approved these changes

Assignees

@sebelga sebelga

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Security/Spaces Platform Security - Spaces feature release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sebelga @elasticmachine @kibana-ci @kibanamachine @legrego