feat: workspace level ui settings #328

ruanyl · 2024-04-12T08:57:22Z

When getting ui settings within a workspace, it will combine the workspace ui settings with the global ui settings and workspace ui settings have higher priority if the same setting was defined in both places

When updating ui settings within a workspace, it will update the workspace ui settings, the global ui settings will remain unchanged.

Description

Issues Resolved

Screenshot

Testing the changes

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

SuZhou-Joe

LGTM and I think maybe we should create integration test for new wrapper.

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

SuZhou-Joe · 2024-04-15T06:14:59Z

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

+    return {
+      ...wrapperOptions.client,
+      get: getUiSettingsWithWorkspace,
+      update: updateUiSettingsWithWorkspace,
+    };


It seems some methods of client are inherited through prototype and spreading the methods here may make this missing inside the methods. Better to declare those methods explicitly in each wrapper, though I think it should be an issue for client wrapper.

SuZhou-Joe · 2024-04-15T06:20:50Z

src/plugins/workspace/server/plugin.ts

+    );
+
+    core.savedObjects.addClientWrapper(
+      -3,


A meaningful variable to the magical priority number would help, I merged another backport PR and maybe you can try that pattern.

Should we declare the order sequence through a comment if there is?

@raintygao right, I'll put a comment

Hailong-am · 2024-04-15T06:35:44Z

do we ever have a comparison of workspace level setting as a normal setting object with workspace information vs a property of workspace itself? Thinking of user level setting, this solution may not have enough flexibility.

ruanyl · 2024-04-15T06:53:34Z

do we ever have a comparison of workspace level setting as a normal setting object with workspace information vs a property of workspace itself?

That's a very good point!
Yes, I've consider the option of making user level ui settings a standalone object associated with workspace. But I end up with the challenge to control the access of this object, because only workspace admin can update ui settings of the workspace. Having ui settings as a property of workspace object seems the most straight forward.

Thinking of user level setting, this solution may not have enough flexibility.

Actually I did think of this as well, I'd imagine user level ui settings is an object with ACL attached. Similar to how we handle workspace ui level in this PR, there could be another wrapper to handle user ui settings. What do you think?

Can you share more about your concerns? Don't know if I missed any corner cases.

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts

Hailong-am · 2024-04-15T07:10:57Z

only workspace admin can update ui settings of the workspace

I see, that's the point. thanks for explanation.

Similar to how we handle workspace ui level in this PR, there could be another wrapper to handle user ui settings. What do you think?

As we don't have a user object in kibana index, to save user level setting, we either reuse existing setting object type and associate it with user information. Base on this assumption, i am thinking workspace level setting could do same way. However, permission control is a issue.

Can you share more about your concerns?

one thing is that is there a case user need to export/import this kind of setting? For now, workspace itself is not exportable.

wanglam · 2024-04-15T09:28:19Z

LGTM

When getting ui settings within a workspace, it will combine the workspace ui settings with the global ui settings and workspace ui settings have higher priority if the same setting was defined in both places When updating ui settings within a workspace, it will update the workspace ui settings, the global ui settings will remain unchanged. Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

…gs_client_wrapper.ts Co-authored-by: SuZhou-Joe <suzhou@amazon.com>

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

When getting ui settings within a workspace, it will combine the workspace ui settings with the global ui settings and workspace ui settings have higher priority if the same setting was defined in both places When updating ui settings within a workspace, it will update the workspace ui settings, the global ui settings will remain unchanged. Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

ruanyl requested review from wanglam, raintygao and Hailong-am April 12, 2024 08:57

ruanyl requested a review from SuZhou-Joe as a code owner April 12, 2024 08:57

github-actions bot added the all-star-contributor label Apr 12, 2024

SuZhou-Joe reviewed Apr 15, 2024

View reviewed changes

wanglam reviewed Apr 15, 2024

View reviewed changes

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.ts Show resolved Hide resolved

Hailong-am approved these changes Apr 15, 2024

View reviewed changes

wanglam approved these changes Apr 15, 2024

View reviewed changes

ruanyl added 3 commits April 15, 2024 20:28

fix WorkspaceAttribute type

f0d2cb6

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

update based on PR comments

cf49d79

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

ruanyl force-pushed the feature/workspace-ui-settings branch from 615ee00 to cf49d79 Compare April 15, 2024 12:28

raintygao approved these changes Apr 16, 2024

View reviewed changes

ruanyl and others added 3 commits April 16, 2024 15:39

add integration tests for workspace ui settings wrapper

8ee9467

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

Update src/plugins/workspace/server/saved_objects/workspace_ui_settin…

1dfd7cc

…gs_client_wrapper.ts Co-authored-by: SuZhou-Joe <suzhou@amazon.com>

fix failed tests

0e49d7e

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

SuZhou-Joe approved these changes Apr 17, 2024

View reviewed changes

ruanyl merged commit 32fab61 into workspace-pr-integr Apr 17, 2024
76 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: workspace level ui settings #328

feat: workspace level ui settings #328

ruanyl commented Apr 12, 2024

SuZhou-Joe left a comment •

edited

Loading

SuZhou-Joe Apr 15, 2024 •

edited

Loading

SuZhou-Joe Apr 15, 2024 •

edited

Loading

raintygao Apr 15, 2024 •

edited

Loading

ruanyl Apr 15, 2024

Hailong-am commented Apr 15, 2024

ruanyl commented Apr 15, 2024 •

edited

Loading

Hailong-am commented Apr 15, 2024 •

edited

Loading

wanglam commented Apr 15, 2024

feat: workspace level ui settings #328

feat: workspace level ui settings #328

Conversation

ruanyl commented Apr 12, 2024

Description

Issues Resolved

Screenshot

Testing the changes

Check List

SuZhou-Joe left a comment • edited Loading

Choose a reason for hiding this comment

SuZhou-Joe Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

SuZhou-Joe Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

raintygao Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

ruanyl Apr 15, 2024

Choose a reason for hiding this comment

Hailong-am commented Apr 15, 2024

ruanyl commented Apr 15, 2024 • edited Loading

Hailong-am commented Apr 15, 2024 • edited Loading

wanglam commented Apr 15, 2024

SuZhou-Joe left a comment •

edited

Loading

SuZhou-Joe Apr 15, 2024 •

edited

Loading

SuZhou-Joe Apr 15, 2024 •

edited

Loading

raintygao Apr 15, 2024 •

edited

Loading

ruanyl commented Apr 15, 2024 •

edited

Loading

Hailong-am commented Apr 15, 2024 •

edited

Loading