feat: Displaying details to Dataset/Database deletion modals #30016
Conversation
dosubot
bot
added
change:frontend
It'll scroll. I'm open to other ideas of course, which might include:
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. |
|
Some considerations if we actually want to list all the names:
|
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.
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.
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. |
| </p> | ||
| </React.Fragment> | ||
| `); | ||
| expect(wrapper.find(DeleteModal).props().description).toMatchSnapshot(); |
There was a problem hiding this comment.
InlineSnapshots are no longer supported by Jest. A few changed files in this PR allow us to use regular snapshots, which remain supported.
There was a problem hiding this comment.
Note that I also loosened constraints (prettier, license check) on those snapshot files, so they can be committed as-generated.
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 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.
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 have a database connection associated to them. When you delete a database, you'll delete the alerts too because of cascading.
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. |
@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 😄 |
rusackas
changed the title
mistercrunch
dismissed
their stale review
didn't realize there was another review...
|
Seems we can decouple the issues:
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 |
|
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: ... 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. |
| {DatabaseDeleteRelatedExtension && ( | ||
| <DatabaseDeleteRelatedExtension | ||
| database={databaseCurrentlyDeleting} | ||
| /> | ||
| )} | ||
| </> | ||
| /* |
There was a problem hiding this comment.
Did you forget to remove this commented code?
There was a problem hiding this comment.
@rusackas I think you missed this comment 👆🏼
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:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (Dataset deletion):
After (Dataset deletion):
Before (Database deletion)
After (Database deletion):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION