[FIX] Dashboard list integrated delete #3796

pjfitzgibbons · 2023-04-06T15:01:05Z

Description

Fix delete on Dashboard items with Integrated Providers feature

Issues Resolved

resolves #3795

Check List

codecov-commenter · 2023-04-07T22:44:37Z

Codecov Report

Merging #3796 (815b4a9) into main (8b0587e) will increase coverage by 0.00%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #3796   +/-   ##
=======================================
  Coverage   66.46%   66.46%           
=======================================
  Files        3226     3226           
  Lines       62012    62012           
  Branches     9588     9588           
=======================================
+ Hits        41215    41219    +4     
+ Misses      18497    18494    -3     
+ Partials     2300     2299    -1

Flag	Coverage Δ
Linux	`66.41% <ø> (+<0.01%)`	⬆️
Windows	`66.40% <ø> (ø)`

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

Impacted Files	Coverage Δ
src/plugins/dashboard/public/plugin.tsx	`0.00% <ø> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

joshuarrrr · 2023-04-11T00:28:46Z

@pjfitzgibbons One of the commits is missing the sign-off for DCO: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3796/checks?check_run_id=12607666399

joshuarrrr

@pjfitzgibbons For a bug fix like this, I'd expect a regression test case. Is the deletion case covered by any of the functional or unit tests? If you have a screencast that shows bulk and individual delete operations working, that's always helpful too.

Otherwise just some minor questions about the implementation choices and nature of the bug.

src/plugins/dashboard/public/plugin.tsx

src/plugins/dashboard/public/application/legacy_app.js

joshuarrrr · 2023-04-13T20:21:08Z

@pjfitzgibbons Just checking in on this - code freeze is Monday, so we'd like to be able to merge before then.

pjfitzgibbons · 2023-04-13T22:16:18Z

@pjfitzgibbons For a bug fix like this, I'd expect a regression test case. Is the deletion case covered by any of the functional or unit tests? If you have a screencast that shows bulk and individual delete operations working, that's always helpful too.

Otherwise just some minor questions about the implementation choices and nature of the bug.

Existing test cases should cover. There are delete cases, no-error means refactor has not changed behaviour.

Do we have test cases that specifically cover "optional" plugin-available APIs ?

Signed-off-by: Peter Fitzgibbons <[email protected]>

pjfitzgibbons · 2023-04-13T22:50:43Z

@pjfitzgibbons One of the commits is missing the sign-off for DCO: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3796/checks?check_run_id=12607666399

Fixed. (squashed the branch)

abbyhu2000 · 2023-04-14T16:08:36Z

Ran the code and functionality looks good with deleting the dashboards. Have not tried deleting with the new dashboard list functionality with other plugins.

joshuarrrr

@pjfitzgibbons This looks good - if you can add the suggested error toast, we should be ready to go.

src/plugins/dashboard/public/application/legacy_app.js

joshuarrrr · 2023-04-14T17:54:29Z

Existing test cases should cover. There are delete cases, no-error means refactor has not changed behaviour.

Do we have test cases that specifically cover "optional" plugin-available APIs ?

So this bug was only for deleting objects that used the new interface (deleting legacy dashboards worked fine)? If that's the case, then yeah, we'd want to update our test cases to include mock non-dashboard objects that use the new interface that validates those functionalities.

joshuarrrr · 2023-04-14T20:28:36Z

(current test failures can be ignored)

CHANGELOG.md

abbyhu2000 · 2023-04-17T17:01:44Z

Also need to be rebased to fix the link and cypress tests and the merge conflicts. @pjfitzgibbons

joshuarrrr · 2023-04-17T17:31:10Z

@abbyhu2000 See #3796 (comment) - those failures are unrelated. But given the changelog conflict, I'll resolve and restart the automation.

kavilla · 2023-04-17T20:29:36Z

@pjfitzgibbons, attempting to recreate the error on main with and without an extra plugin registering but not able to recreate it.

Are you still seeing this issue? What was the error from the toast?

joshuarrrr · 2023-04-17T22:15:33Z

@pjfitzgibbons, attempting to recreate the error on main with and without an extra plugin registering but not able to recreate it.

Are you still seeing this issue? What was the error from the toast?

I believe deleting existing dashboard items worked fine - so you'd need to have a non-dashboard item registered to be able to delete and see the error, which is what I suggested in terms of test expansion.

joshuarrrr · 2023-04-18T00:00:20Z

All checks passed.

Signed-off-by: Josh Romero <[email protected]>

pjfitzgibbons · 2023-04-18T00:19:46Z

I'll look at testing tomorrow.

Signed-off-by: Josh Romero <[email protected]>

AMoo-Miki · 2023-04-18T02:23:18Z

src/plugins/dashboard/public/application/legacy_app.js

+            return Promise.all(
+              ids.map(({ id, appId }) => {
+                return deps.savedObjectsClient.delete(appId, id);
+              })
+            ).catch((error) => {
+              deps.toastNotifications.addError(error, {
+                title: i18n.translate('dashboard.dashboardListingDeleteErrorTitle', {
+                  defaultMessage: 'Error deleting dashboard',
+                }),
+              });
+            });


If I understand this correctly, we fire off a bunch of delete requests in parallel and if one fails we show an error that there was a problem deleting the "dashboard" but in reality, we have deleted some stuff. This doesn't sound like good UX.

I think we want to go with consistent UX (so that the same component on dashboard and visualize behaves the same) first, and then we can improve the existing UX in a follow-up.

AMoo-Miki · 2023-04-18T02:25:13Z

src/plugins/dashboard/public/application/legacy_app.js

@@ -171,7 +170,18 @@ export function initDashboardApp(app, deps) {
            history.push(deps.addBasePath(viewUrl));
          };
          $scope.delete = (dashboards) => {
-            return service.delete(dashboards.map((d) => d.id));
+            const ids = dashboards.map((d) => ({ id: d.id, appId: d.appId }));


Any reason why this map is not combined with the map inside the all?

* [FIx] Dashboard-List Integrated Delete Signed-off-by: Peter Fitzgibbons <[email protected]> * Add toast notification on deletion error Signed-off-by: Josh Romero <[email protected]> * add toast notification dependency Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Peter Fitzgibbons <[email protected]> Signed-off-by: Josh Romero <[email protected]> Co-authored-by: Peter Fitzgibbons <[email protected]> Co-authored-by: Josh Romero <[email protected]> (cherry picked from commit 9d27023) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* [FIx] Dashboard-List Integrated Delete * Add toast notification on deletion error * add toast notification dependency --------- (cherry picked from commit 9d27023) Signed-off-by: Peter Fitzgibbons <[email protected]> Signed-off-by: Josh Romero <[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: Peter Fitzgibbons <[email protected]> Co-authored-by: Josh Romero <[email protected]>

pjfitzgibbons requested a review from a team as a code owner April 6, 2023 15:01

github-actions bot added the first-time-contributor label Apr 6, 2023

ashwin-pc added the v2.7.0 label Apr 6, 2023

joshuarrrr assigned abbyhu2000 Apr 11, 2023

joshuarrrr reviewed Apr 12, 2023

View reviewed changes

src/plugins/dashboard/public/plugin.tsx Show resolved Hide resolved

src/plugins/dashboard/public/application/legacy_app.js Outdated Show resolved Hide resolved

joshuarrrr mentioned this pull request Apr 12, 2023

2.7.0 Platform Upgrades for Plugins (O11y, MDS, Geospatial,+) #3674

Closed

23 tasks

joshuarrrr self-assigned this Apr 13, 2023

pjfitzgibbons requested review from ananzh, kavilla, seanneumann, AMoo-Miki, ashwin-pc, zengyan-amazon, kristenTian, zhongnansu and manasvinibs as code owners April 13, 2023 22:16

[FIx] Dashboard-List Integrated Delete

164f022

Signed-off-by: Peter Fitzgibbons <[email protected]>

pjfitzgibbons force-pushed the dashboard-list-integrated-delete branch from ae6d708 to 164f022 Compare April 13, 2023 22:49

joshuarrrr reviewed Apr 14, 2023

View reviewed changes

src/plugins/dashboard/public/application/legacy_app.js Outdated Show resolved Hide resolved

src/plugins/dashboard/public/application/legacy_app.js Outdated Show resolved Hide resolved

AMoo-Miki reviewed Apr 17, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Merge branch 'main' into dashboard-list-integrated-delete

942ffa4

joshuarrrr added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry backport 2.x labels Apr 17, 2023

joshuarrrr added 2 commits April 17, 2023 17:01

Add toast notification on deletion error

47c066b

Signed-off-by: Josh Romero <[email protected]>

Merge branch 'main' into dashboard-list-integrated-delete

8c5982c

add toast notification dependency

815b4a9

Signed-off-by: Josh Romero <[email protected]>

joshuarrrr approved these changes Apr 18, 2023

View reviewed changes

AMoo-Miki reviewed Apr 18, 2023

View reviewed changes

ashwin-pc approved these changes Apr 18, 2023

View reviewed changes

ashwin-pc merged commit 9d27023 into opensearch-project:main Apr 18, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 18, 2023

[Backport 2.x] [FIX] Dashboard list integrated delete #3876

Merged

joshuarrrr mentioned this pull request Apr 18, 2023

Improve toast messaging on failed deletions from dashboard listing or visualize listing pages #3877

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Dashboard list integrated delete #3796

[FIX] Dashboard list integrated delete #3796

pjfitzgibbons commented Apr 6, 2023 •

edited

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading

joshuarrrr commented Apr 11, 2023

joshuarrrr left a comment

joshuarrrr commented Apr 13, 2023

pjfitzgibbons commented Apr 13, 2023

pjfitzgibbons commented Apr 13, 2023

abbyhu2000 commented Apr 14, 2023 •

edited

Loading

joshuarrrr left a comment

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

abbyhu2000 commented Apr 17, 2023

joshuarrrr commented Apr 17, 2023

kavilla commented Apr 17, 2023

joshuarrrr commented Apr 17, 2023

joshuarrrr commented Apr 18, 2023

pjfitzgibbons commented Apr 18, 2023

AMoo-Miki Apr 18, 2023

joshuarrrr Apr 18, 2023

joshuarrrr Apr 18, 2023

AMoo-Miki Apr 18, 2023

[FIX] Dashboard list integrated delete #3796

[FIX] Dashboard list integrated delete #3796

Conversation

pjfitzgibbons commented Apr 6, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Apr 7, 2023 • edited Loading

Codecov Report

joshuarrrr commented Apr 11, 2023

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr commented Apr 13, 2023

pjfitzgibbons commented Apr 13, 2023

pjfitzgibbons commented Apr 13, 2023

abbyhu2000 commented Apr 14, 2023 • edited Loading

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr commented Apr 14, 2023

joshuarrrr commented Apr 14, 2023

abbyhu2000 commented Apr 17, 2023

joshuarrrr commented Apr 17, 2023

kavilla commented Apr 17, 2023

joshuarrrr commented Apr 17, 2023

joshuarrrr commented Apr 18, 2023

pjfitzgibbons commented Apr 18, 2023

AMoo-Miki Apr 18, 2023

Choose a reason for hiding this comment

joshuarrrr Apr 18, 2023

Choose a reason for hiding this comment

joshuarrrr Apr 18, 2023

Choose a reason for hiding this comment

AMoo-Miki Apr 18, 2023

Choose a reason for hiding this comment

pjfitzgibbons commented Apr 6, 2023 •

edited

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading

abbyhu2000 commented Apr 14, 2023 •

edited

Loading