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

[Workspace] Filter left nav menu items according to the current workspace #6234

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Mar 21, 2024

Description

In this PR, left nav bar will be filtered by the feature configuration of current workspace.

Issues Resolved

#6153

Screenshot

left.menu.filter.by.workspace.mov

Accessing app which is NOT configured by the workspace will display "Application Not Found" page
image

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

@ruanyl ruanyl changed the title Filter left nav menu items according to the current workspace [Workspace] Filter left nav menu items according to the current workspace Mar 21, 2024
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 32.65%. Comparing base (a0eaf84) to head (7f7cefe).
Report is 1 commits behind head on main.

❗ Current head 7f7cefe differs from pull request most recent head ae9f515. Consider uploading reports for the commit ae9f515 to get more accurate results

Files Patch % Lines
src/plugins/workspace/public/plugin.ts 53.84% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6234       +/-   ##
===========================================
- Coverage   67.56%   32.65%   -34.92%     
===========================================
  Files        3379     2249     -1130     
  Lines       65894    45512    -20382     
  Branches    10660     7133     -3527     
===========================================
- Hits        44522    14860    -29662     
- Misses      18774    29956    +11182     
+ Partials     2598      696     -1902     
Flag Coverage Δ
Linux_1 32.65% <73.91%> (+0.01%) ⬆️
Linux_2 ?
Linux_3 ?
Linux_4 ?
Windows_1 ?
Windows_2 ?
Windows_3 ?
Windows_4 ?

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.

@ruanyl ruanyl force-pushed the filter-left-menu-based-on-current-workspace branch 2 times, most recently from 4f70782 to 8629acb Compare March 22, 2024 06:27
* nav links, for example, filter the nav links or update a specific nav link's properties.
* {@link LinksUpdater}
*/
getLinkUpdaters$(): BehaviorSubject<LinksUpdater[]>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, I will update README.md, public.api.md is automatically generated.

SuZhou-Joe
SuZhou-Joe previously approved these changes Mar 22, 2024
}) => {
let matched = false;

for (const featureConfig of featureConfigs) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we do a quick break once matched or not matched? Or what's the expect behavior if the featureConfigs is ['@management', '!security']

Copy link
Member

Choose a reason for hiding this comment

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

Found the answer in other PR. But as the question is asked by many reviewer, maybe adding some comments could help to reduce such question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, updated.

@seraphjiang
Copy link
Member

@xluo-aws @ruanyl @BionIT @Flyingliuhub @bandinib-amzn @kgcreative @xeniatup @zengyan-amazon

Looks like we are going to have to global contexts: Workspace at left nav, DataSource at top navi if both are enabled

have we discussed or made the decision between Workspace and DataSource?

I assume DataSource always belongs to Workspace. if we all agree about it, from information hierarchy's perspective, should workspace selector being put on top nav bar. Once cx select workspace, then they could select datasource?

I feel we need to build both experience into future playground to get board feedback asap.

what do you think?

image

@ruanyl
Copy link
Member Author

ruanyl commented Mar 25, 2024

@seraphjiang

have we discussed or made the decision between Workspace and DataSource?
I feel we need to build both experience into future playground to get board feedback asap.

Agree on that, and yes, the current behavior of setting workspace and data source are more like setting two global contexts. However, base on my understanding so far, they may have correlations. For example, data source available in one workspace may not be available in another workspace.

I think we need to take this into consideration from UX design's perspective as early as possible. @kgcreative @lauralexis @xeniatup

@Hailong-am
Copy link
Collaborator

I have concern about mark those links as inaccessible for now, before we move index pattern/ saved objects page out, user have no ways to import data and create index pattern. That means user can't start create visualization and dashboards in a fresh workspace

@ruanyl
Copy link
Member Author

ruanyl commented Apr 9, 2024

I have concern about mark those links as inaccessible for now, before we move index pattern/ saved objects page out, user have no ways to import data and create index pattern. That means user can't start create visualization and dashboards in a fresh workspace

You'r right, maybe we should wait for index pattern to be moved out before merging this PR. For saved objects page, you mean saved object management page? Do we have upcoming PR/task about that?

@ruanyl
Copy link
Member Author

ruanyl commented Apr 9, 2024

@xluo-aws In this PR, it will forbid user to access the applications which are not configured in a workspace, even if user access directly via url. I believe this is the desired behavior, but this is different from what we implemented earlier which we just hide the nav links from ui in left nav menu. Do you have any concern with this?

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Apr 9, 2024

Perhaps we can hide it from nav in this PR(which only sets the navLinkStatus to hidden), and adjust the logic to make them inaccessible in the PR that moves index pattern out of data management.

For saved objects management page, I do not think that will be in 2.14.

@ruanyl
Copy link
Member Author

ruanyl commented Apr 9, 2024

Perhaps we can hide it from nav in this PR(which only sets the navLinkStatus to hidden), and adjust the logic to make these inaccessible in the PR that moves index pattern out of data management.

I think that's too much back and forth, the change in this PR represent an atomic feature, and the index pattern moving should be a standalone PR as well. I think we can wait for the other PR to be merged if that's really a concern :)

@xluo-aws
Copy link
Member

xluo-aws commented Apr 9, 2024

@xluo-aws In this PR, it will forbid user to access the applications which are not configured in a workspace, even if user access directly via url. I believe this is the desired behavior, but this is different from what we implemented earlier which we just hide the nav links from ui in left nav menu. Do you have any concern with this?

No concern, it's the right behavior.

@ruanyl
Copy link
Member Author

ruanyl commented Apr 10, 2024

I have concern about mark those links as inaccessible for now, before we move index pattern/ saved objects page out, user have no ways to import data and create index pattern. That means user can't start create visualization and dashboards in a fresh workspace

Since Index pattern will remain in "Dashboards Management" and "Dashboards Management" is available in workspace, so this is no longer a concern, is that correct?

@ruanyl ruanyl merged commit cb9b26c into opensearch-project:main Apr 10, 2024
66 checks passed
SuZhou-Joe added a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Apr 11, 2024
…pace (opensearch-project#6234)

* Filter left nav menu items according to the current workspace

An "Application Not Found" page will be displayed if accessing app which
is not configured by the workspace

---------

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
SuZhou-Joe added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 11, 2024
…pace (opensearch-project#6234) (#323)

* Filter left nav menu items according to the current workspace

An "Application Not Found" page will be displayed if accessing app which
is not configured by the workspace

---------

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
ruanyl added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 15, 2024
This is caused by opensearch-project#6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>
ruanyl added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 17, 2024
This is caused by opensearch-project#6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>
SuZhou-Joe pushed a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 17, 2024
…6234 (#331)

* remove unnecessary changes due to the refactor in 6234

Signed-off-by: Yulong Ruan <[email protected]>

* fix snapshot

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
ZilongX added a commit that referenced this pull request Apr 17, 2024
* fix(workspace): apps are missing when updating a workspace

This is caused by #6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>

* refactor(workspace): store workspace configurable apps in a global
variable

Signed-off-by: Yulong Ruan <[email protected]>

* fix linter

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: ZilongX <[email protected]>
ruanyl added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 18, 2024
This is caused by opensearch-project#6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>
ruanyl added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 19, 2024
* fix(workspace): apps are missing when updating a workspace

This is caused by opensearch-project#6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>

* refactor workspace list page to use workspaceConfigurableApps

Signed-off-by: Yulong Ruan <[email protected]>

* fix tests

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
raintygao pushed a commit to raintygao/OpenSearch-Dashboards that referenced this pull request Apr 19, 2024
…6234 (opensearch-project#331)

* remove unnecessary changes due to the refactor in 6234

Signed-off-by: Yulong Ruan <[email protected]>

* fix snapshot

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 19, 2024
…pace (#6234)

* Filter left nav menu items according to the current workspace

An "Application Not Found" page will be displayed if accessing app which
is not configured by the workspace

---------

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
(cherry picked from commit cb9b26c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
SuZhou-Joe pushed a commit that referenced this pull request Apr 19, 2024
…pace (#6234) (#6552)

* Filter left nav menu items according to the current workspace

An "Application Not Found" page will be displayed if accessing app which
is not configured by the workspace

---------

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
Co-authored-by: SuZhou-Joe <[email protected]>
(cherry picked from commit cb9b26c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 23, 2024
* fix(workspace): apps are missing when updating a workspace

This is caused by #6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.

Signed-off-by: Yulong Ruan <[email protected]>

* refactor(workspace): store workspace configurable apps in a global
variable

Signed-off-by: Yulong Ruan <[email protected]>

* fix linter

Signed-off-by: Yulong Ruan <[email protected]>

---------

Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: ZilongX <[email protected]>
(cherry picked from commit 4746f43)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Apr 23, 2024
…6606)

* fix(workspace): apps are missing when updating a workspace

This is caused by #6234 which marked apps as inaccessible when the apps
are not configured into the current workspace. However, inaccessible
apps can not be configured into a workspace.



* refactor(workspace): store workspace configurable apps in a global
variable



* fix linter



---------



(cherry picked from commit 4746f43)

Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: ZilongX <[email protected]>
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.

7 participants