[Workspace] fix apps are missing when updating a workspace #6459

ruanyl · 2024-04-15T15:40:56Z

This is caused by #6234 which marked apps as inaccessible when the apps are not configured into the current workspace. However, inaccessible apps are filter out on workspace update page, this is unexpected as such apps can be configured into the current workspace again though they are not configured into the workspace at the moment.

Description

Issues Resolved

#6481

Changelog

skip

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

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

No much concern but maybe we should consider using capabilities service to filter out apps in workspace in the future.

wanglam · 2024-04-16T05:51:30Z

src/plugins/workspace/public/components/workspace_form/utils.ts

+     * Restrict apps are apps that can be configured into a workspace, but restrict to access
+     * because the current workspace didn't have the apps configured, such apps should NOT filter out
+     */
+    if (restrictedApps && restrictedApps.has(id)) {


In order to keep the same source in workspace create, update and list page, can we use restrictedApps for all customized features for workspaces. We can remove the check below.

return ( navLinkStatus !== AppNavLinkStatus.hidden && !chromeless && !DEFAULT_SELECTED_FEATURES_IDS.includes(id) && category?.id !== DEFAULT_APP_CATEGORIES.management.id );

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

codecov · 2024-04-16T10:04:04Z

Codecov Report

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

Project coverage is 32.95%. Comparing base (d2d410b) to head (e4b5b5a).
Report is 2 commits behind head on main.

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

Files	Patch %	Lines
src/plugins/workspace/public/plugin.ts	76.92%	3 Missing ⚠️
src/plugins/workspace/public/utils.ts	60.00%	2 Missing ⚠️
...components/workspace_updater/workspace_updater.tsx	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6459      +/-   ##
==========================================
+ Coverage   32.93%   32.95%   +0.01%     
==========================================
  Files        2260     2260              
  Lines       45769    45788      +19     
  Branches     7200     7203       +3     
==========================================
+ Hits        15075    15089      +14     
- Misses      29984    29988       +4     
- Partials      710      711       +1

Flag	Coverage Δ
Linux_1	`32.95% <77.77%> (+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.

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

SuZhou-Joe

LGTM

ZilongX · 2024-04-16T16:56:47Z

picked up latest main and re-running all checks now

…ge-missing-apps

github-actions · 2024-04-17T03:24:56Z

❌ Invalid Changelog Heading

The '## Changelog' heading in your PR description is either missing or malformed. Please make sure that your PR description includes a '## Changelog' heading with proper spelling, capitalization, spacing, and Markdown syntax.

github-actions · 2024-04-17T06:19:45Z

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

wanglam · 2024-04-17T09:06:25Z

src/plugins/workspace/public/components/workspace_form/utils.ts

@@ -65,23 +61,14 @@ export const convertApplicationsToFeaturesOrGroups = (
 ) => {


It seems the applications only need id, title and category. Can we remove navLinkStatus and chromeless?

wanglam · 2024-04-17T09:07:44Z

src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.tsx

  selectedFeatures: string[];
  onChange: (newFeatures: string[]) => void;
+  workspaceConfigurableApps?: PublicAppInfo[];


Seems we only need id, title and category for workspaceConfigurableApps here. Could we change to Pick<PublicAppInfo, 'id' | 'title' | 'category'>?

Actually I don't see a strong need to turn it into Pick<PublicAppInfo, 'id' | 'title' | 'category'> in this case. I understood that passing minimum set of data to child components is a best practice, but the application which type is PublicAppInfo is provided by the core module, the components expect PublicAppInfo type of data sounds reasonable to me as well.

I'm not holding a strong opinion, what else concerns do you have?

Not a big issue. We can keep current implementation.

wanglam · 2024-04-17T09:12:23Z

src/plugins/workspace/public/utils.ts

+
+export const filterWorkspaceConfigurableApps = (applications: PublicAppInfo[]) => {
+  const visibleApplications = applications.filter(({ navLinkStatus, chromeless, category, id }) => {
+    return (


Shall we add some unit tests here to cover this line?

Yes, there will be a followup PR from @yubonluo to cover this.

github-actions · 2024-04-17T09:51:58Z

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "Fixed applications not displayed in workspace update page ([#6481](https". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

ZilongX · 2024-04-17T16:30:05Z

pick up latest main and re-running all checks now

* 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>

…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]>

ruanyl requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, BionIT and xinruiba as code owners April 15, 2024 15:40

github-actions bot added the repeat-contributor label Apr 15, 2024

SuZhou-Joe previously approved these changes Apr 16, 2024

View reviewed changes

wanglam reviewed Apr 16, 2024

View reviewed changes

SuZhou-Joe added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Apr 16, 2024

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

79da6af

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

ruanyl dismissed SuZhou-Joe’s stale review via 79da6af April 16, 2024 09:49

fix linter

f3cbe9a

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

SuZhou-Joe previously approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'main' into fix-workspace-update-page-missing-apps

0cf8afe

Merge branch 'main' into fix-workspace-update-page-missing-apps

9334f20

ZilongX previously approved these changes Apr 17, 2024

View reviewed changes

Merge remote-tracking branch 'base/main' into fix-workspace-update-pa…

72baa1d

…ge-missing-apps

ruanyl dismissed stale reviews from ZilongX and SuZhou-Joe via 72baa1d April 17, 2024 03:03

ruanyl changed the title ~~fix(workspace): apps are missing when updating a workspace~~ [Workspace] fix apps are missing when updating a workspace Apr 17, 2024

ruanyl added the workspace label Apr 17, 2024

github-actions bot added failed changeset and removed Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Apr 17, 2024

SuZhou-Joe added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Apr 17, 2024

github-actions bot removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Apr 17, 2024

Merge branch 'main' into fix-workspace-update-page-missing-apps

e4b5b5a

wanglam reviewed Apr 17, 2024

View reviewed changes

github-actions bot added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Apr 17, 2024

SuZhou-Joe approved these changes Apr 17, 2024

View reviewed changes

ZilongX self-requested a review April 17, 2024 16:19

Merge branch 'main' into fix-workspace-update-page-missing-apps

e34e02c

Merge branch 'main' into fix-workspace-update-page-missing-apps

7d84f03

ZilongX approved these changes Apr 17, 2024

View reviewed changes

ZilongX merged commit 4746f43 into opensearch-project:main Apr 17, 2024
65 checks passed

SuZhou-Joe added backport 2.x v2.14.0 labels Apr 23, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 23, 2024

[Backport 2.x] [Workspace] fix apps are missing when updating a workspace #6606

Merged

SuZhou-Joe added the bug Something isn't working label Apr 23, 2024

AMoo-Miki added the in-next label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workspace] fix apps are missing when updating a workspace #6459

[Workspace] fix apps are missing when updating a workspace #6459

ruanyl commented Apr 15, 2024 •

edited

Loading

SuZhou-Joe left a comment

wanglam Apr 16, 2024

codecov bot commented Apr 16, 2024 •

edited

Loading

SuZhou-Joe left a comment

ZilongX commented Apr 16, 2024

github-actions bot commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

wanglam Apr 17, 2024

wanglam Apr 17, 2024

ruanyl Apr 17, 2024

wanglam Apr 17, 2024

wanglam Apr 17, 2024

ruanyl Apr 17, 2024

github-actions bot commented Apr 17, 2024

ZilongX commented Apr 17, 2024

		@@ -65,23 +61,14 @@ export const convertApplicationsToFeaturesOrGroups = (
		) => {

[Workspace] fix apps are missing when updating a workspace #6459

[Workspace] fix apps are missing when updating a workspace #6459

Conversation

ruanyl commented Apr 15, 2024 • edited Loading

Description

Issues Resolved

Changelog

Screenshot

Testing the changes

Check List

SuZhou-Joe left a comment

Choose a reason for hiding this comment

wanglam Apr 16, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2024 • edited Loading

Codecov Report

SuZhou-Joe left a comment

Choose a reason for hiding this comment

ZilongX commented Apr 16, 2024

github-actions bot commented Apr 17, 2024

❌ Invalid Changelog Heading

github-actions bot commented Apr 17, 2024

❌ Changelog Entry Missing Hyphen

wanglam Apr 17, 2024

Choose a reason for hiding this comment

wanglam Apr 17, 2024

Choose a reason for hiding this comment

ruanyl Apr 17, 2024

Choose a reason for hiding this comment

wanglam Apr 17, 2024

Choose a reason for hiding this comment

wanglam Apr 17, 2024

Choose a reason for hiding this comment

ruanyl Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2024

❌ Invalid Prefix For Manual Changeset Creation

ZilongX commented Apr 17, 2024

ruanyl commented Apr 15, 2024 •

edited

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading