[Workspace] Jump to non-workspace url when clicking home icon #316

SuZhou-Joe · 2024-04-07T01:41:45Z

Description

Home page, which is supposed to be only visited without workspace, now is enabled to be accessed by clicking the home icon or enter the url manually in the browser.

This PR introduces a new field workspaceAccessibility inside App, which indicates the accessibility of the application based on workspace. This field can bring two benefits:

When navigate between apps, plugins and core application are using the navigateToApp method. By using the workspaceAccessibility field, workspace is able to do the hard navigation in a central place.
The workspaceAccessibility field can be used in workspace create page to filter applications that should not be visible inside workspace. For now we filter out home overview page and all the pages under management section by hard code, which is not elegant and not extensible.

Issues Resolved

opensearch-project#6362

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

codecov-commenter · 2024-04-07T02:19:00Z

Codecov Report

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

Project coverage is 44.41%. Comparing base (0f34d69) to head (00682b2).
Report is 9 commits behind head on workspace-pr-integr.

Files	Patch %	Lines
...rc/core/public/application/application_service.tsx	0.00%	7 Missing ⚠️
src/core/public/chrome/nav_links/to_nav_link.ts	0.00%	3 Missing ⚠️
src/core/public/core_system.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           workspace-pr-integr     #316      +/-   ##
=======================================================
+ Coverage                35.17%   44.41%   +9.24%     
=======================================================
  Files                     1885     2741     +856     
  Lines                    36421    54278   +17857     
  Branches                  6672     8543    +1871     
=======================================================
+ Hits                     12810    24109   +11299     
- Misses                   22761    28796    +6035     
- Partials                   850     1373     +523

Flag	Coverage Δ
_1	`32.80% <0.00%> (?)`
_4	`35.15% <0.00%> (-0.02%)`	⬇️

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.

src/core/public/application/application_service.tsx

ruanyl · 2024-04-08T01:55:07Z

@SuZhou-Joe I'm getting "Not Found" page when server base path is enabled

ruanyl · 2024-04-08T02:08:08Z

I can see the anchor of home logo is still /mzt/w/management/app/home

I think toNavLink function should also be updated

SuZhou-Joe · 2024-04-08T02:29:29Z

@SuZhou-Joe I'm getting "Not Found" page when server base path is enabled

Sorry for the mistake. Fix for that.

SuZhou-Joe · 2024-04-08T02:42:33Z

How do you like the idea of adding a new field visibility on App? If it makes sense, I will add more unit test for this PR to make it prepared for merge.

@wanglam @ruanyl @gaobinlong @Hailong-am @raintygao @xluo-aws

SuZhou-Joe · 2024-04-08T02:44:13Z

I can see the anchor of home logo is still /mzt/w/management/app/home

I think toNavLink function should also be updated

Sure, done for that. But actually the home icon's link is coming from another place and fix for that as well.

wanglam · 2024-04-08T02:51:10Z

src/core/public/application/types.ts

+   * Defaulting to `both`
+   * See {@link AppVisibility}
+   */
+  visibility?: AppVisibility;


Since this property only be used for workspace. Shall we change to a boolean property, and renaming to something like workspaceVisible?

There should be 3 states: both, homeOnly and workspaceOnly. I have some concern that regarding undefined as both would introduce some confusion.

Though actually a link should be either homeOnly or workspaceOnly, I am afraid we can not force plugins to change the registration by 2.14 .

Just a little concerned about there are too many visibility related property (status, navLinkStatus, chromeless) in the interface. Or we can just renaming to workspaceLess, then we just mark home plugin to true.

Yes, agree on that there are too many similar property. So I think maybe we could extend the status field by adding inaccessible-in-workspace and inaccessible-in-home, what do you think?

an app can only be accessed within a workspace

workspace update/overview page are two examples

I mean only two state is not enough, if using workspaceless which is boolean, it can only represent two state above.

Is it necessary to mark the application as "an app can only be accessed within a workspace"? It will a little bit confused when workspace not enabled. For these pages, I think we still need to be displayed when workspace is turn off.

I mean only two state is not enough

I see. do you think undefined/null, true, false will be three states and enough for our case? undefined/null same as current.

If we need to represent 3 states in a single field, an enum would be better instead of regarding undefined/null as a different state to false. This is a draft PR and we can adjust that.
What about:

workspaceAccessibility.withinWorkspace -> only accessible in workspace, should be filter out from home in the future when workspace enabled, which means user won't be enable to create visualizations and dashboards out of workspace.

workspaceAccessibility.outOfWorkspace -> only accessible out of workspace, can be used for pages like home/workspace list/workspace create page. Should be used in workspace-create/workspace-update pages to filter out inaccessible features.

undefined -> as current to keep backward capability.

Is it necessary to mark the application as "an app can only be accessed within a workspace"? It will a little bit confused when workspace not enabled. For these pages, I think we still need to be displayed when workspace is turn off.

That's a good point. One way that I can think of is to check the workspace flag when plugin register the app, for example, via capabilities service. If workspace is turned off, plugin should not use this flag

wanglam · 2024-04-08T02:52:30Z

How do you like the idea of adding a new field visibility on App? If it makes sense, I will add more unit test for this PR to make it prepared for merge.

@wanglam @ruanyl @gaobinlong @Hailong-am @raintygao @xluo-aws

Shall we filter out features in workspace create and update page by the new field in this PR?

SuZhou-Joe · 2024-04-08T02:54:18Z

How do you like the idea of adding a new field visibility on App? If it makes sense, I will add more unit test for this PR to make it prepared for merge.
@wanglam @ruanyl @gaobinlong @Hailong-am @raintygao @xluo-aws

Shall we filter out features in workspace create and update page by the new field in this PR?

Will be raised in a following PR.

src/core/public/application/application_service.tsx

src/core/public/application/types.ts

Hailong-am · 2024-04-08T08:09:20Z

do we need to add workspaceless flag to workspace create/list application?

src/core/public/application/types.ts

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

…rch-project#6427) * [Workspace] Jump to non-workspace url when clicking home icon (#316) * temp: save Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: complete the feature Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: bootstrap error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: page not found error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: anchor href Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update toNavLink to comply with workspace Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: change to workspaceless Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: change to workspaceless Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: change to workspaceless Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: register list and create page as workspaceless Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update to WorkspaceVisibility Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize the jump logic Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: make app inaccessible if workspaceAccessibility is No Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * refactor: using WorkspaceAvailability Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: change test name Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update wording Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove CHANGELOG change Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Changeset file for PR opensearch-project#6427 created/updated * Apply suggestions from code review Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/workspace/public/utils.test.ts Co-authored-by: Miki <amoo_miki@yahoo.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Lint src/plugins/workspace/public/utils.test.ts Signed-off-by: Miki <amoo_miki@yahoo.com> * Lint src/plugins/workspace/public/utils.test.ts Signed-off-by: Miki <amoo_miki@yahoo.com> * Lint src/plugins/workspace/public/utils.test.ts Signed-off-by: Miki <amoo_miki@yahoo.com> * fix: lint error Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Miki <amoo_miki@yahoo.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> Co-authored-by: Miki <amoo_miki@yahoo.com>

…rch-project#6427) (opensearch-project#6628) * [Workspace] Jump to non-workspace url when clicking home icon (#316) * temp: save * feat: complete the feature * feat: remove useless code * fix: bootstrap error * fix: bootstrap error * fix: page not found error * fix: anchor href * feat: update toNavLink to comply with workspace * feat: change to workspaceless * feat: change to workspaceless * feat: change to workspaceless * feat: register list and create page as workspaceless * feat: optimize code * feat: update to WorkspaceVisibility * feat: add unit test * feat: optimize the jump logic * fix: unit test * feat: make app inaccessible if workspaceAccessibility is No --------- * refactor: using WorkspaceAvailability * feat: change test name * feat: update wording * feat: update test * feat: add unit test * feat: remove CHANGELOG change * Changeset file for PR opensearch-project#6427 created/updated * Apply suggestions from code review * Update src/plugins/workspace/public/utils.test.ts * Lint src/plugins/workspace/public/utils.test.ts * Lint src/plugins/workspace/public/utils.test.ts * Lint src/plugins/workspace/public/utils.test.ts * fix: lint error * fix: unit test --------- (cherry picked from commit 104ee42) Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Miki <amoo_miki@yahoo.com> 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: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> Co-authored-by: Miki <amoo_miki@yahoo.com>

github-actions bot added the distinguished-contributor label Apr 7, 2024

SuZhou-Joe force-pushed the feature/home-icon-click branch 2 times, most recently from 0026219 to 5602f14 Compare April 7, 2024 07:33

ruanyl reviewed Apr 8, 2024

View reviewed changes

src/core/public/application/application_service.tsx Outdated Show resolved Hide resolved

wanglam reviewed Apr 8, 2024

View reviewed changes

Hailong-am reviewed Apr 8, 2024

View reviewed changes

src/core/public/application/application_service.tsx Show resolved Hide resolved

raintygao reviewed Apr 8, 2024

View reviewed changes

src/core/public/application/types.ts Outdated Show resolved Hide resolved

yubonluo reviewed Apr 9, 2024

View reviewed changes

src/core/public/application/types.ts Outdated Show resolved Hide resolved

Hailong-am approved these changes Apr 11, 2024

View reviewed changes

SuZhou-Joe force-pushed the feature/home-icon-click branch from c20b2d8 to 764d168 Compare April 11, 2024 08:15

SuZhou-Joe added 11 commits April 12, 2024 14:02

temp: save

b638ebb

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: complete the feature

401f9c7

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: remove useless code

63b8e91

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

fix: bootstrap error

c0a8eb5

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

fix: bootstrap error

3155917

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

fix: page not found error

01bd26a

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

fix: anchor href

2af3766

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: update toNavLink to comply with workspace

0dc2628

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: change to workspaceless

b7b3841

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: change to workspaceless

533d194

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: change to workspaceless

13d251e

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

SuZhou-Joe added 6 commits April 12, 2024 14:02

feat: register list and create page as workspaceless

de65301

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: optimize code

2b88233

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: update to WorkspaceVisibility

9c08db7

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: add unit test

29c46a5

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

feat: optimize the jump logic

f6ad012

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

fix: unit test

dec7c4f

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

SuZhou-Joe force-pushed the feature/home-icon-click branch from ef97b45 to dec7c4f Compare April 12, 2024 06:03

feat: make app inaccessible if workspaceAccessibility is No

ff74dda

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

SuZhou-Joe merged commit 2c0827c into ruanyl:workspace-pr-integr Apr 12, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workspace] Jump to non-workspace url when clicking home icon #316

[Workspace] Jump to non-workspace url when clicking home icon #316

SuZhou-Joe commented Apr 7, 2024 •

edited

Loading

codecov-commenter commented Apr 7, 2024 •

edited

Loading

ruanyl commented Apr 8, 2024

ruanyl commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

wanglam Apr 8, 2024

SuZhou-Joe Apr 8, 2024

SuZhou-Joe Apr 8, 2024

wanglam Apr 8, 2024

SuZhou-Joe Apr 8, 2024

ruanyl Apr 9, 2024

wanglam Apr 9, 2024 •

edited

Loading

Hailong-am Apr 9, 2024

SuZhou-Joe Apr 9, 2024 •

edited

Loading

ruanyl Apr 9, 2024

wanglam commented Apr 8, 2024

SuZhou-Joe commented Apr 8, 2024

Hailong-am commented Apr 8, 2024

[Workspace] Jump to non-workspace url when clicking home icon #316

[Workspace] Jump to non-workspace url when clicking home icon #316

Conversation

SuZhou-Joe commented Apr 7, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Apr 7, 2024 • edited Loading

Codecov Report

ruanyl commented Apr 8, 2024

ruanyl commented Apr 8, 2024 • edited Loading

SuZhou-Joe commented Apr 8, 2024 • edited Loading

SuZhou-Joe commented Apr 8, 2024 • edited Loading

SuZhou-Joe commented Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam commented Apr 8, 2024

SuZhou-Joe commented Apr 8, 2024

Hailong-am commented Apr 8, 2024

SuZhou-Joe commented Apr 7, 2024 •

edited

Loading

codecov-commenter commented Apr 7, 2024 •

edited

Loading

ruanyl commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

SuZhou-Joe commented Apr 8, 2024 •

edited

Loading

wanglam Apr 9, 2024 •

edited

Loading

SuZhou-Joe Apr 9, 2024 •

edited

Loading