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.

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

hide datasource and settings menu when workspace enabled and entering workspace #325

Merged
merged 11 commits into from
Apr 15, 2024

Conversation

raintygao
Copy link
Collaborator

@raintygao raintygao commented Apr 11, 2024

Description

hide datasource and settings menu in dashboard management section when workspace is enabled and user has entered workspace.

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

@raintygao raintygao marked this pull request as ready for review April 11, 2024 08:19
@raintygao raintygao requested a review from SuZhou-Joe as a code owner April 11, 2024 08:19
@@ -79,8 +82,19 @@ export const ManagementApp = ({ dependencies, history }: ManagementAppProps) =>
);

useEffect(() => {
// If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu
if (capabilities.workspaces.enabled && workspaces.currentWorkspace$.getValue()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it safely to access capabilities.workspaces.enabled? what if workspaces is undefined/null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As in type definition workspaces capabilities is required. Should we update?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a call out, I am just wondering if workspaces will still be a object if workspaces feature flag is turned off.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I checked that there are some other places using this way. https://github.com/ruanyl/OpenSearch-Dashboards/pull/322/files#diff-47d724c5a7bb6e8c1286f67e3ed1b117ac27e05387052760388bd477bd76337bL184
I am thinking that should we maintain to keep capabilities.workspaces.enabled existing and its value should be false if workspace is turned off?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, if we put this logic in workspace plugin, we don't need to judge if workspace is enabled.

@@ -79,8 +82,19 @@ export const ManagementApp = ({ dependencies, history }: ManagementAppProps) =>
);

useEffect(() => {
// If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu
Copy link
Collaborator

Choose a reason for hiding this comment

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

The change should happen inside workspace directory I suppose?

Copy link
Collaborator Author

@raintygao raintygao Apr 11, 2024

Choose a reason for hiding this comment

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

Im okay, but if we put it in workspace plugin, we may need to declare to require management plugin in workspace plugin to get management.sections.section.opensearchDashboards, not sure if it's okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's OK, workspaces has dependency on saved objects management page as well.
Hide data source and advanced settings seems to be a workspace specific feature and better to change inside workspace plugin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Then I will add a required declaration.

Copy link
Collaborator

@Hailong-am Hailong-am Apr 12, 2024

Choose a reason for hiding this comment

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

shouldn't be optional dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can use required. As this is an original required plugin, and mark it as required would not have any influences.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is an original required plugin

is it management a newly added dependency? let's say if management plugin is not installed somehow, that should not block workspace plugin setup and start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As management is a plugin in OSD, it will always be installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Hailong-am Although management will always be installed, marking it as optional looks more safety and normative, i will keep it the same as savedObjectsManagement and mark it as optional, thanks for deepping dive.

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.58%. Comparing base (0f34d69) to head (6ba49b9).
Report is 11 commits behind head on workspace-pr-integr.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           workspace-pr-integr     #325       +/-   ##
========================================================
+ Coverage                35.17%   53.58%   +18.41%     
========================================================
  Files                     1885     2236      +351     
  Lines                    36421    43530     +7109     
  Branches                  6672     8090     +1418     
========================================================
+ Hits                     12810    23325    +10515     
+ Misses                   22761    18561     -4200     
- Partials                   850     1644      +794     
Flag Coverage Δ
_3 43.62% <ø> (?)
_4 35.16% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -118,6 +123,13 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
currentAppIdSubscription.unsubscribe();
});
})();

// If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu and disable
Copy link
Collaborator

@SuZhou-Joe SuZhou-Joe Apr 11, 2024

Choose a reason for hiding this comment

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

Nit: better to extract to a private method and listen the currentWorkspaceId$ change. currentWorkspaceId$ should be the single source of truth for detecting whether inside a workspace.

Signed-off-by: tygao <[email protected]>
*/
private disableManagementApps(core: CoreSetup, management: ManagementSetup) {
const currentWorkspaceId$ = core.workspaces.currentWorkspaceId$;
this.managementCurrentWorkspaceIdSubscription?.unsubscribe();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move this to stop method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stop method will also call this.

@raintygao raintygao merged commit 2058998 into ruanyl:workspace-pr-integr Apr 15, 2024
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants