Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page #42527

ashharrison90 · 2021-11-30T13:56:33Z

What this PR does / why we need it:

there is no way to currently delete an empty folder. this is because canDelete is currently based on canMove, and canMove is only true if a dashboard is selected.
change the logic so canDelete is enabled if anything is selected. this allows us to select empty folders, and also allows us to delete folders before they've been expanded.
add a new unit test to make sure we can delete empty folders

Which issue(s) this PR fixes:

Special notes for your reviewer:

we should probably hold off on merging this until the release is complete 😱

Clarity-89

Nice catch, works as expected 🎉 Could we also update the text copy for delete warning as it doesn't seem to account for singular folder case properly?

hugohaggmark

Looks good but I haven't had the chance to test it out (yet)

hugohaggmark · 2021-12-01T09:20:21Z

public/app/features/search/hooks/useManageDashboards.ts


  const canDelete = useMemo(() => {
+    const somethingChecked = results.some((result) => result.checked || result.items.some((item) => item.checked));


nit: I wonder if it would be worth to flatten everything and check for checked because using .some in another .some isn't super readable. I know I've been part of adding to this complexity in the past 🙈 I guess as long we have good tests for these cases its not super important. 🤔

@hugohaggmark yeah agreed it's not super readable, but also didn't wanna flatten as that means we definitely have to go through the entire array. when using list view that means looping an array containing every dashboard that exists in your instance 😬 whereas with the .some it shortcircuits that loop as soon as it finds one.

have moved the logic into a separate mini function instead. wdyt? 🤔

You're right! Looks better as a separate function.

ashharrison90 · 2021-12-01T13:12:11Z

Nice catch, works as expected 🎉 Could we also update the text copy for delete warning as it doesn't seem to account for singular folder case properly?

@Clarity-89 i don't think this is easily possible as there's currently no real way to distinguish between an empty folder (items: []), and a folder that just hasn't been expanded yet to fetch the inner dashboards (items: []). we could maybe make a call to the backend on deletion of a single folder to check if it's really empty or not, but my gut feeling is to err on the side of the more generic error message for simplicity. wdyt? 🤔

Clarity-89 · 2021-12-01T13:15:37Z

Nice catch, works as expected 🎉 Could we also update the text copy for delete warning as it doesn't seem to account for singular folder case properly?

@Clarity-89 i don't think this is easily possible as there's currently no real way to distinguish between an empty folder (items: []), and a folder that just hasn't been expanded yet to fetch the inner dashboards (items: []). we could maybe make a call to the backend on deletion of a single folder to check if it's really empty or not, but i'm erring on the side of the more generic error message for simplicity. wdyt? 🤔

Just to clarify, if only one folder is selected, the message should say Do you want to delete the selected folder and all its dashboards and alerts? instead of Do you want to delete the selected folder and all their dashboards and alerts?. The logic to detect if multiple dashboards are selected is already there, it just needs to be applied to this string.

ashharrison90 · 2021-12-01T13:19:20Z

@Clarity-89 ahhhhh gotcha, that we can probably do. 👍 i thought you were referring to the dashboards and alerts bit 🤦

* Manage dashboards: Can now delete empty folders * Split hasChecked out into a separate function * Update modal text (cherry picked from commit a365dcc)

* Manage dashboards: Can now delete empty folders * Split hasChecked out into a separate function * Update modal text (cherry picked from commit a365dcc) Co-authored-by: Ashley Harrison <ashley.harrison@grafana.com>

This reverts commit a365dcc.

Manage dashboards: Can now delete empty folders

5ee5619

ashharrison90 added type/bug area/dashboard/folders area/frontend add to changelog backport v8.3.x Mark PR for automatic backport to v8.3.x labels Nov 30, 2021

ashharrison90 added this to the 8.3.1 milestone Nov 30, 2021

ashharrison90 requested a review from a team as a code owner November 30, 2021 13:56

ashharrison90 self-assigned this Nov 30, 2021

ashharrison90 requested review from joshhunt and kaydelaney and removed request for a team November 30, 2021 13:56

Clarity-89 approved these changes Dec 1, 2021

View reviewed changes

hugohaggmark approved these changes Dec 1, 2021

View reviewed changes

Split hasChecked out into a separate function

fa8f096

Update modal text

f01e277

ashharrison90 merged commit a365dcc into main Dec 1, 2021

ashharrison90 deleted the ash/42486 branch December 1, 2021 14:53

grafanabot mentioned this pull request Dec 1, 2021

[v8.3.x] Manage dashboards: Can now delete empty folders #42591

Merged

grafanabot pushed a commit that referenced this pull request Dec 1, 2021

Manage dashboards: Can now delete empty folders (#42527)

ade9b24

* Manage dashboards: Can now delete empty folders * Split hasChecked out into a separate function * Update modal text (cherry picked from commit a365dcc)

marefr changed the title ~~Manage dashboards: Can now delete empty folders~~ Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page Dec 10, 2021

simPod added a commit to simPod/grafana that referenced this pull request Dec 13, 2021

Revert "Manage dashboards: Can now delete empty folders (grafana#42527)"

cbb81bf

This reverts commit a365dcc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page #42527

Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page #42527

ashharrison90 commented Nov 30, 2021 •

edited

Clarity-89 left a comment

hugohaggmark left a comment

hugohaggmark Dec 1, 2021

ashharrison90 Dec 1, 2021

hugohaggmark Dec 1, 2021

ashharrison90 commented Dec 1, 2021 •

edited

Clarity-89 commented Dec 1, 2021

ashharrison90 commented Dec 1, 2021 •

edited


		const canDelete = useMemo(() => {
		const somethingChecked = results.some((result) => result.checked \|\| result.items.some((item) => item.checked));

Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page #42527

Dashboards: Fix so that empty folders can be deleted from the manage dashboards/folders page #42527

Conversation

ashharrison90 commented Nov 30, 2021 • edited

Clarity-89 left a comment

Choose a reason for hiding this comment

hugohaggmark left a comment

Choose a reason for hiding this comment

hugohaggmark Dec 1, 2021

Choose a reason for hiding this comment

ashharrison90 Dec 1, 2021

Choose a reason for hiding this comment

hugohaggmark Dec 1, 2021

Choose a reason for hiding this comment

ashharrison90 commented Dec 1, 2021 • edited

Clarity-89 commented Dec 1, 2021

ashharrison90 commented Dec 1, 2021 • edited

ashharrison90 commented Nov 30, 2021 •

edited

ashharrison90 commented Dec 1, 2021 •

edited

ashharrison90 commented Dec 1, 2021 •

edited