feat: Displaying details to Dataset/Database deletion modals #30016

rusackas · 2024-08-26T18:53:57Z

SUMMARY

This PR lists all dashboards and charts affected by the (potential) deletion of a dataset/database.

The links to these assets all open in a single (named) new window.

It also does a little cleanup along the way:

modal is now always centered (so it scrolls sensibly)
the "delete" button is now the "Delete" button
Highlights the name of the dataset/database that's getting deleted... this seems important.
Updates the .pot file since some strings are getting tweaked in there.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before (Dataset deletion):

After (Dataset deletion):

Before (Database deletion)

After (Database deletion):

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

michael-s-molina

How is this going to work when there are thousands of dashboards and charts linked to a database?

rusackas · 2024-08-27T18:17:46Z

How is this going to work when there are thousands of dashboards and charts linked to a database?

It'll scroll.

I'm open to other ideas of course, which might include:

Limited height scroll areas for each asset type
Showing the first X instances and then saying "...and Y more"

This is why I pinged @kasiazjc for review. Even if it's annoying to see_ thousands of dashboards, it's far less dangerous than breaking those thousands of dashboards.

I think I prefer option 2, just showing 10 or so as a sampling with "and X others" but I'm flexible.

michael-s-molina · 2024-08-27T18:25:22Z

Some considerations if we actually want to list all the names:

We would probably need a search given that it will be hard to find any chart among thousands
Users should not scroll to fill the input or click on Cancel or Delete
What about other assets types such as Alerts, Saved Queries that will also be impacted?

rusackas · 2024-08-27T21:11:26Z

Some considerations if we actually want to list all the names:

We would probably need a search given that it will be hard to find any chart among thousands

I'm merely displaying the list that's already data the component has access to. I don't particularly want to go crazy with functionality here. If we need to add full-text fuzzy searching, or a new API for this, I'm open to discussing an additional PR, but I'd say it's out of scope for the moment.

Users should not scroll to fill the input or click on Cancel or Delete

This is possible for the button, as you can lock the modal's footer in place and scroll the contents. However, that footer would be disabled until the user scrolls down anyway to fill the input. We might be able to put the input IN the footer, but that's a weird new pattern to introduce.

I would also argue that if you don't even have time to scroll past the list of things you're about to break, you probably should not be allowed to click that button.

What about other assets types such as Alerts, Saved Queries that will also be impacted?

Alerts are not in the scope of the current data AFAIK. I could list the Saved Queries on the database modal if interested, but that seemed of less utility (including the links to them). I can add it if it seems valuable, but it's just more to scroll past, and might not affect data consumers.

Is the scrolling the real problem here? I'll just limit the results to fit on a sensible screen. I was just looking for a quick improvement over a modal where you a user has no idea what they're breaking.

Another possibility was to just use tooltips where the numbers are, but with a long list, that'd be even more painful.

Also open to abusing some other components (Popovers with scrolling lists? Tabs? ¯\_(ツ)_/¯ ) but again was just looking to make a simple (hah!) safety improvement.

rusackas · 2024-08-27T21:17:52Z

superset-frontend/src/pages/DatabaseList/DatabaseList.test.jsx

-        </p>
-      </React.Fragment>
-    `);
+    expect(wrapper.find(DeleteModal).props().description).toMatchSnapshot();


InlineSnapshots are no longer supported by Jest. A few changed files in this PR allow us to use regular snapshots, which remain supported.

Note that I also loosened constraints (prettier, license check) on those snapshot files, so they can be committed as-generated.

superset-frontend/src/components/DeleteModal/index.tsx

michael-s-molina · 2024-08-28T12:13:59Z

Also open to abusing some other components (Popovers with scrolling lists? Tabs? ¯_(ツ)_/¯ ) but again was just looking to make a simple (hah!) safety improvement.

I understand your objective but simple might mean different things depending on the environment. I'm just trying to reach a simple solution that works for organizations with a lot of assets.

I would also argue that if you don't even have time to scroll past the list of things you're about to break, you probably should not be allowed to click that button.
Is the scrolling the real problem here? I'll just limit the results to fit on a sensible screen. I was just looking for a quick improvement over a modal where you a user has no idea what they're breaking.

I don't think the problem is just the scroll. Think about a scenario where a database has 40k charts. The admin will not review every single chart. They might review a sample of charts, maybe the "popular" ones to understand the impact of the change. This might be one way to resolve this, show the top 10 assets of each category. We would need to determine how to calculate the top.

We also need to consider the payload size when returning the names for all results of each asset type.

I'm merely displaying the list that's already data the component has access to.

This is really concerning because it means that this query returns thousands of names when we currently only display a count. If we go for the top 10 solution, this query should also be modified to just return the necessary data.

Alerts are not in the scope of the current data AFAIK.

Alerts have a database connection associated to them. When you delete a database, you'll delete the alerts too because of cascading.

I could list the Saved Queries on the database modal if interested, but that seemed of less utility (including the links to them).

Saved queries are really important at Airbnb as they might be really complex and take a long time to build. Rebuilding a dashboard or chart might be way faster than rebuilding a saved query.

In summary, I think the top 10 approach might work for giving admins an idea of what's being affected by a database deletion. If you really want to show all values, we could introduce a configuration to limit the max number of assets displayed and show a message indicating the limit.

michael-s-molina · 2024-08-28T12:25:56Z

I'm merely displaying the list that's already data the component has access to.

@rusackas Just to give you an idea about the problems with this current query. We have a database that contains 78k charts and 4k dashboards. When we click on the delete icon, the query takes more than a minute to run, and because there's no loading indication, for the entire minute nothing happen on the screen giving the impression that the click didn't work. What generally happen in this case, is that the user will click again on the trash icon, firing another heavy query. Now, combine these problems with the additional step of rendering all these names on the modal 😄

mistercrunch

LGTM

didn't realize there was another review...

mistercrunch · 2024-09-06T19:09:56Z

Seems we can decouple the issues:

primary purpose: showing the user what's about to cascade-delete and the fact it's long (show more... ?)
no loading indicator: maybe can be addressed in this PR?
slow query - I'd love to decouple this issue

About slow query, I might be able to help improve the query, I'd just need to figure out the backend call behind it, review the query and make sure it uses an index

rusackas · 2024-09-07T04:35:53Z

As discussed with @michael-s-molina, I made one of several steps so that we might get this PR merged, so that we may follow up with more after this PR.

The results of the displayed at-risk items are now limited to 10. This gives more context than we had before, without causing massive scrolling problems.

Next steps may include:
• Displaying/counting more at-risk assets (saved queries, alerts/reports, etc)
• Making the number if displayed items configurable
• Adding "view more" link to load/paginate more items
• Adding search/filters for the list
• Adjusting the API to also limit to 10 items (or as configured) and thus reduce a pre-existing performance bottleneck (CC @mistercrunch who offered to chip in here)
• Add a spinner in case there is a bottleneck.

... but for now, users merely have a bit more context on what's at risk, which was my original (simplistic) hope, even if the rabbit hole goes a lot deeper from here.

michael-s-molina · 2024-09-09T12:56:50Z

superset-frontend/src/pages/DatabaseList/index.tsx

              {DatabaseDeleteRelatedExtension && (
                <DatabaseDeleteRelatedExtension
                  database={databaseCurrentlyDeleting}
                />
              )}
            </>
+            /*


Did you forget to remove this commented code?

@rusackas I think you missed this comment 👆🏼

rusackas added 3 commits August 26, 2024 09:59

dataset deletion modal

888b77d

modal always centered (for better scrolling)

f846e88

Details/links on Database deletion

4861a10

rusackas requested a review from kasiazjc August 26, 2024 18:53

pull-request-size bot added the size/L label Aug 26, 2024

dosubot bot added change:frontend Requires changing the frontend data Namespace | Anything related to data, including databases configurations, datasets, etc. labels Aug 26, 2024

github-actions bot added the preset-io label Aug 26, 2024

rusackas requested a review from kgabryje August 26, 2024 18:56

rusackas added 6 commits August 26, 2024 14:38

type fixes

d65d3dc

fixin' some tests

cd62fe1

whitespace tweak

b03641c

more tweaks

5320a2c

moving to snapshot from inlinesnapshot

7d711ac

Fresh snapshot

b52698c

michael-s-molina requested changes Aug 27, 2024

View reviewed changes

rusackas added 2 commits August 27, 2024 12:20

skip license check on snap files

482f045

Make prettier ignore test snapshots

71383e0

rusackas added 3 commits August 27, 2024 12:51

don't run the precommit hook on .snap files

5c65bac

fresh snap

2c7f19d

rat exclude adjustment

16e6f3e

rusackas commented Aug 27, 2024

View reviewed changes

superset-frontend/src/components/DeleteModal/index.tsx Show resolved Hide resolved

rusackas changed the title ~~feat: Adding details to Dataset/Database deletion modals~~ feat: Displaying details to Dataset/Database deletion modals Aug 28, 2024

mistercrunch previously approved these changes Sep 6, 2024

View reviewed changes

limiting results to 10 items *and more*

bf5ffb6

michael-s-molina reviewed Sep 9, 2024

View reviewed changes

rusackas requested a review from michael-s-molina September 10, 2024 16:34

deleting commented text.

f864e49

michael-s-molina approved these changes Sep 10, 2024

View reviewed changes

rusackas merged commit 7bb6a14 into master Sep 10, 2024
34 checks passed

rusackas deleted the delete-modal-improvements branch September 10, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Displaying details to Dataset/Database deletion modals #30016

feat: Displaying details to Dataset/Database deletion modals #30016

rusackas commented Aug 26, 2024 •

edited

Loading

michael-s-molina left a comment •

edited

Loading

rusackas commented Aug 27, 2024

michael-s-molina commented Aug 27, 2024

rusackas commented Aug 27, 2024 •

edited

Loading

rusackas Aug 27, 2024

rusackas Aug 27, 2024

michael-s-molina commented Aug 28, 2024

michael-s-molina commented Aug 28, 2024

mistercrunch left a comment

mistercrunch commented Sep 6, 2024

rusackas commented Sep 7, 2024 •

edited

Loading

michael-s-molina Sep 9, 2024

michael-s-molina Sep 10, 2024

rusackas Sep 10, 2024

feat: Displaying details to Dataset/Database deletion modals #30016

feat: Displaying details to Dataset/Database deletion modals #30016

Conversation

rusackas commented Aug 26, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina left a comment • edited Loading

Choose a reason for hiding this comment

rusackas commented Aug 27, 2024

michael-s-molina commented Aug 27, 2024

rusackas commented Aug 27, 2024 • edited Loading

rusackas Aug 27, 2024

Choose a reason for hiding this comment

rusackas Aug 27, 2024

Choose a reason for hiding this comment

michael-s-molina commented Aug 28, 2024

michael-s-molina commented Aug 28, 2024

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch commented Sep 6, 2024

rusackas commented Sep 7, 2024 • edited Loading

michael-s-molina Sep 9, 2024

Choose a reason for hiding this comment

michael-s-molina Sep 10, 2024

Choose a reason for hiding this comment

rusackas Sep 10, 2024

Choose a reason for hiding this comment

rusackas commented Aug 26, 2024 •

edited

Loading

michael-s-molina left a comment •

edited

Loading

rusackas commented Aug 27, 2024 •

edited

Loading

rusackas commented Sep 7, 2024 •

edited

Loading