[Nested Folder] Delete folder methode (#58444)

* transfer DeleteFolder changes from larger PR

* finish some thingies

* add the simplest delete logics

* some intermedia steps

* fix tests

* add test

* fix some comments

Co-authored-by: yangkb09 <yangkb09@gmail.com>

pkg/api/folder.go

@@ -2,7 +2,6 @@ package api
 
 import (
 	"errors"
-	"fmt"
 	"net/http"
 	"strconv"
 
@@ -11,9 +10,9 @@ import (
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/dashboards"
+	"github.com/grafana/grafana/pkg/services/folder"
 	"github.com/grafana/grafana/pkg/services/guardian"
 	"github.com/grafana/grafana/pkg/services/libraryelements"
-	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/web"
 )
 
@@ -175,16 +174,12 @@ func (hs *HTTPServer) DeleteFolder(c *models.ReqContext) response.Response { //
 	}
 
 	uid := web.Params(c.Req)[":uid"]
-	f, err := hs.folderService.DeleteFolder(c.Req.Context(), c.SignedInUser, c.OrgID, uid, c.QueryBool("forceDeleteRules"))
+	err = hs.folderService.DeleteFolder(c.Req.Context(), &folder.DeleteFolderCommand{UID: uid, OrgID: c.OrgID, ForceDeleteRules: c.QueryBool("forceDeleteRules")})
 	if err != nil {
 		return apierrors.ToFolderErrorResponse(err)
 	}
 
-	return response.JSON(http.StatusOK, util.DynMap{
+	return response.JSON(http.StatusOK, "")
-		"title":   f.Title,
-		"message": fmt.Sprintf("Folder %s deleted", f.Title),
-		"id":      f.Id,
-	})
 }
 
 func (hs *HTTPServer) toFolderDto(c *models.ReqContext, g guardian.DashboardGuardian, folder *models.Folder) dtos.Folder {

pkg/api/folder_test.go

@@ -308,7 +308,7 @@ type fakeFolderService struct {
 	CreateFolderError    error
 	UpdateFolderResult   *models.Folder
 	UpdateFolderError    error
-	DeleteFolderResult   *models.Folder
+	DeleteFolderResult   *folder.Folder
 	DeleteFolderError    error
 	DeletedFolderUids    []string
 }
@@ -334,7 +334,7 @@ func (s *fakeFolderService) UpdateFolder(ctx context.Context, user *user.SignedI
 	return s.UpdateFolderError
 }
 
-func (s *fakeFolderService) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) {
+func (s *fakeFolderService) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
-	s.DeletedFolderUids = append(s.DeletedFolderUids, uid)
+	s.DeletedFolderUids = append(s.DeletedFolderUids, cmd.UID)
-	return s.DeleteFolderResult, s.DeleteFolderError
+	return s.DeleteFolderError
 }

pkg/services/folder/folderimpl/folder.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/events"
+	"github.com/grafana/grafana/pkg/infra/appcontext"
 	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
@@ -232,7 +233,8 @@ func (s *Service) CreateFolder(ctx context.Context, user *user.SignedInUser, org
 			// We'll log the error and also roll back the previously-created
 			// (legacy) folder.
 			s.log.Error("error saving folder to nested folder store", err)
-			_, err = s.DeleteFolder(ctx, user, orgID, createdFolder.Uid, true)
+
+			err = s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.Uid, OrgID: orgID, ForceDeleteRules: true})
 			if err != nil {
 				s.log.Error("error deleting folder after failed save to nested folder store", err)
 			}
@@ -298,27 +300,37 @@ func (s *Service) UpdateFolder(ctx context.Context, user *user.SignedInUser, org
 	return nil
 }
 
-func (s *Service) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) {
+func (s *Service) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
-	dashFolder, err := s.dashboardStore.GetFolderByUID(ctx, orgID, uid)
+	if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
+		err := s.Delete(ctx, cmd)
+		if err != nil {
+			s.log.Error("the delete folder on folder table failed with err: ", err.Error())
+		}
+	}
+	user, err := appcontext.User(ctx)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	guard := guardian.New(ctx, dashFolder.Id, orgID, user)
+	dashFolder, err := s.dashboardStore.GetFolderByUID(ctx, cmd.OrgID, cmd.UID)
+	if err != nil {
+		return err
+	}
+
+	guard := guardian.New(ctx, dashFolder.Id, cmd.OrgID, user)
 	if canSave, err := guard.CanDelete(); err != nil || !canSave {
 		if err != nil {
-			return nil, toFolderError(err)
+			return toFolderError(err)
 		}
-		return nil, dashboards.ErrFolderAccessDenied
+		return dashboards.ErrFolderAccessDenied
 	}
 
-	deleteCmd := models.DeleteDashboardCommand{OrgId: orgID, Id: dashFolder.Id, ForceDeleteFolderRules: forceDeleteRules}
+	deleteCmd := models.DeleteDashboardCommand{OrgId: cmd.OrgID, Id: dashFolder.Id, ForceDeleteFolderRules: cmd.ForceDeleteRules}
 
 	if err := s.dashboardStore.DeleteDashboard(ctx, &deleteCmd); err != nil {
-		return nil, toFolderError(err)
+		return toFolderError(err)
 	}
-
+	return nil
-	return dashFolder, nil
 }
 
 func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) {
@@ -354,23 +366,30 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol
 	})
 }
 
-func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) (*folder.Folder, error) {
+func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
-	// check the flag, if old - do whatever did before
+	_, err := s.Get(ctx, &folder.GetFolderQuery{
-	//  for new only the store
-	// check if dashboard exists
-
-	foldr, err := s.Get(ctx, &folder.GetFolderQuery{
 		UID:   &cmd.UID,
 		OrgID: cmd.OrgID,
 	})
 	if err != nil {
-		return nil, err
+		return err
+	}
+
+	folders, err := s.store.GetChildren(ctx, folder.GetTreeQuery{UID: cmd.UID, OrgID: cmd.OrgID})
+	if err != nil {
+		return err
+	}
+	for _, f := range folders {
+		err := s.Delete(ctx, &folder.DeleteFolderCommand{UID: f.UID, OrgID: f.OrgID, ForceDeleteRules: cmd.ForceDeleteRules})
+		if err != nil {
+			return err
+		}
 	}
 	err = s.store.Delete(ctx, cmd.UID, cmd.OrgID)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	return foldr, nil
+	return nil
 }
 
 func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder.Folder, error) {

pkg/services/folder/folderimpl/folder_test.go

@@ -28,7 +28,7 @@ import (
 )
 
 var orgID = int64(1)
-var usr = &user.SignedInUser{UserID: 1}
+var usr = &user.SignedInUser{UserID: 1, OrgID: orgID}
 
 func TestIntegrationProvideFolderService(t *testing.T) {
 	if testing.Short() {
@@ -79,12 +79,12 @@ func TestIntegrationFolderService(t *testing.T) {
 			folderId := rand.Int63()
 			folderUID := util.GenerateShortUID()
 
-			folder := models.NewFolder("Folder")
+			newFolder := models.NewFolder("Folder")
-			folder.Id = folderId
+			newFolder.Id = folderId
-			folder.Uid = folderUID
+			newFolder.Uid = folderUID
 
-			dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(folder, nil)
+			dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(newFolder, nil)
-			dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(folder, nil)
+			dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(newFolder, nil)
 
 			t.Run("When get folder by id should return access denied error", func(t *testing.T) {
 				_, err := service.GetFolderByID(context.Background(), usr, folderId, orgID)
@@ -104,7 +104,7 @@ func TestIntegrationFolderService(t *testing.T) {
 
 			t.Run("When creating folder should return access denied error", func(t *testing.T) {
 				dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*models.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil).Times(2)
-				_, err := service.CreateFolder(context.Background(), usr, orgID, folder.Title, folderUID)
+				_, err := service.CreateFolder(context.Background(), usr, orgID, newFolder.Title, folderUID)
 				require.Equal(t, err, dashboards.ErrFolderAccessDenied)
 			})
 
@@ -122,7 +122,20 @@ func TestIntegrationFolderService(t *testing.T) {
 			})
 
 			t.Run("When deleting folder by uid should return access denied error", func(t *testing.T) {
-				_, err := service.DeleteFolder(context.Background(), usr, orgID, folderUID, false)
+				ctx := context.Background()
+				ctx = appcontext.WithUser(ctx, usr)
+
+				newFolder := models.NewFolder("Folder")
+				newFolder.Uid = folderUID
+
+				dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(newFolder, nil)
+				dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(newFolder, nil)
+
+				err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{
+					UID:              folderUID,
+					OrgID:            orgID,
+					ForceDeleteRules: false,
+				})
 				require.Error(t, err)
 				require.Equal(t, err, dashboards.ErrFolderAccessDenied)
 			})
@@ -190,7 +203,13 @@ func TestIntegrationFolderService(t *testing.T) {
 				}).Return(nil).Once()
 
 				expectedForceDeleteRules := rand.Int63()%2 == 0
-				_, err := service.DeleteFolder(context.Background(), usr, orgID, f.Uid, expectedForceDeleteRules)
+				ctx := context.Background()
+				ctx = appcontext.WithUser(ctx, usr)
+				err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{
+					UID:              f.Uid,
+					OrgID:            orgID,
+					ForceDeleteRules: expectedForceDeleteRules,
+				})
 				require.NoError(t, err)
 				require.NotNil(t, actualCmd)
 				require.Equal(t, f.Id, actualCmd.Id)
@@ -288,7 +307,7 @@ func TestFolderService(t *testing.T) {
 
 	t.Run("delete folder", func(t *testing.T) {
 		folderStore.ExpectedFolder = &folder.Folder{}
-		_, err := folderService.Delete(context.Background(), &folder.DeleteFolderCommand{})
+		err := folderService.Delete(context.Background(), &folder.DeleteFolderCommand{})
 		require.NoError(t, err)
 	})
 
@@ -334,7 +353,7 @@ func TestFolderService(t *testing.T) {
 	})
 }
 
-func TestCreate_NestedFolders(t *testing.T) {
+func TestNestedFolderService(t *testing.T) {
 	t.Run("with feature flag unset", func(t *testing.T) {
 		ctx := appcontext.WithUser(context.Background(), usr)
 		store := &FakeStore{}
@@ -354,17 +373,39 @@ func TestCreate_NestedFolders(t *testing.T) {
 			features:         features,
 		}
 
-		// dashboard store & service commands that should be called.
+		t.Run("When create folder, no create in folder table done", func(t *testing.T) {
-		dashboardsvc.On("BuildSaveDashboardCommand",
+			// dashboard store & service commands that should be called.
-			mock.Anything, mock.AnythingOfType("*dashboards.SaveDashboardDTO"),
+			dashboardsvc.On("BuildSaveDashboardCommand",
-			mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil)
+				mock.Anything, mock.AnythingOfType("*dashboards.SaveDashboardDTO"),
-		dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil)
+				mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil)
-		dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&models.Folder{}, nil)
+			dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil)
+			dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&models.Folder{}, nil)
 
-		_, err := foldersvc.CreateFolder(ctx, usr, orgID, "myFolder", "myFolder")
+			_, err := foldersvc.CreateFolder(ctx, usr, orgID, "myFolder", "myFolder")
-		require.NoError(t, err)
+			require.NoError(t, err)
-		// CreateFolder should not call the folder store create if the feature toggle is not enabled.
+			// CreateFolder should not call the folder store create if the feature toggle is not enabled.
-		require.False(t, store.CreateCalled)
+			require.False(t, store.CreateCalled)
+		})
+
+		t.Run("When delete folder, no delete in folder table done", func(t *testing.T) {
+			var actualCmd *models.DeleteDashboardCommand
+			dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+				actualCmd = args.Get(1).(*models.DeleteDashboardCommand)
+			}).Return(nil).Once()
+			dashStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&models.Folder{}, nil)
+
+			g := guardian.New
+			guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
+
+			err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID})
+			require.NoError(t, err)
+			require.NotNil(t, actualCmd)
+
+			t.Cleanup(func() {
+				guardian.New = g
+			})
+			require.False(t, store.DeleteCalled)
+		})
 	})
 
 	t.Run("with nested folder feature flag on", func(t *testing.T) {
@@ -426,5 +467,27 @@ func TestCreate_NestedFolders(t *testing.T) {
 				guardian.New = g
 			})
 		})
+
+		t.Run("delete with success", func(t *testing.T) {
+			var actualCmd *models.DeleteDashboardCommand
+			dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+				actualCmd = args.Get(1).(*models.DeleteDashboardCommand)
+			}).Return(nil).Once()
+			dashStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&models.Folder{}, nil)
+
+			g := guardian.New
+			guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
+
+			store.ExpectedError = nil
+
+			err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID})
+			require.NoError(t, err)
+			require.NotNil(t, actualCmd)
+
+			t.Cleanup(func() {
+				guardian.New = g
+			})
+			require.True(t, store.DeleteCalled)
+		})
 	})
 }

pkg/services/folder/folderimpl/sqlstore.go

@@ -79,15 +79,6 @@ func (ss *sqlStore) Delete(ctx context.Context, uid string, orgID int64) error {
 		if err != nil {
 			return folder.ErrDatabaseError.Errorf("failed to delete folder: %w", err)
 		}
-		/*
-			affected, err := res.RowsAffected()
-			if err != nil {
-				return folder.ErrDatabaseError.Errorf("failed to get affected rows: %w", err)
-			}
-				if affected == 0 {
-					return folder.ErrFolderNotFound.Errorf("folder not found uid:%s org_id:%d", uid, orgID)
-				}
-		*/
 		return nil
 	})
 }
@@ -161,7 +152,6 @@ func (ss *sqlStore) Get(ctx context.Context, q folder.GetFolderQuery) (*folder.F
 	err := ss.db.WithDbSession(ctx, func(sess *db.Session) error {
 		exists := false
 		var err error
-
 		switch {
 		case q.ID != nil:
 			exists, err = sess.SQL("SELECT * FROM folder WHERE id = ?", q.ID).Get(foldr)

pkg/services/folder/folderimpl/store_fake.go

@@ -12,6 +12,7 @@ type FakeStore struct {
 	ExpectedError   error
 
 	CreateCalled bool
+	DeleteCalled bool
 }
 
 func NewFakeStore() *FakeStore {
@@ -26,6 +27,7 @@ func (f *FakeStore) Create(ctx context.Context, cmd folder.CreateFolderCommand)
 }
 
 func (f *FakeStore) Delete(ctx context.Context, uid string, orgID int64) error {
+	f.DeleteCalled = true
 	return f.ExpectedError
 }
 

pkg/services/folder/foldertest/foldertest.go

@@ -35,8 +35,8 @@ func (s *FakeService) UpdateFolder(ctx context.Context, user *user.SignedInUser,
 	cmd.Result = s.ExpectedFolder
 	return s.ExpectedError
 }
-func (s *FakeService) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) {
+func (s *FakeService) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
-	return s.ExpectedFolder, s.ExpectedError
+	return s.ExpectedError
 }
 func (s *FakeService) MakeUserAdmin(ctx context.Context, orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error {
 	return s.ExpectedError

pkg/services/folder/model.go

@@ -78,8 +78,9 @@ type MoveFolderCommand struct {
 // DeleteFolderCommand captures the information required by the folder service
 // to delete a folder.
 type DeleteFolderCommand struct {
-	UID   string `json:"uid" xorm:"uid"`
+	UID              string `json:"uid" xorm:"uid"`
-	OrgID int64  `json:"orgId" xorm:"org_id"`
+	OrgID            int64  `json:"orgId" xorm:"org_id"`
+	ForceDeleteRules bool   `json:"forceDeleteRules"`
 }
 
 // GetFolderQuery is used for all folder Get requests. Only one of UID, ID, or

pkg/services/folder/service.go

@@ -14,7 +14,7 @@ type Service interface {
 	GetFolderByTitle(ctx context.Context, user *user.SignedInUser, orgID int64, title string) (*models.Folder, error)
 	CreateFolder(ctx context.Context, user *user.SignedInUser, orgID int64, title, uid string) (*models.Folder, error)
 	UpdateFolder(ctx context.Context, user *user.SignedInUser, orgID int64, existingUid string, cmd *models.UpdateFolderCommand) error
-	DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error)
+	DeleteFolder(ctx context.Context, cmd *DeleteFolderCommand) error
 	MakeUserAdmin(ctx context.Context, orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error
 }
 

pkg/tests/api/alerting/api_alertmanager_test.go

@@ -799,10 +799,9 @@ func TestDeleteFolderWithRules(t *testing.T) {
 			err := resp.Body.Close()
 			require.NoError(t, err)
 		})
-		b, err := io.ReadAll(resp.Body)
+		_, err = io.ReadAll(resp.Body)
 		require.NoError(t, err)
 		require.Equal(t, 200, resp.StatusCode)
-		require.JSONEq(t, `{"id":1,"message":"Folder default deleted","title":"default"}`, string(b))
 	}
 
 	// Finally, we ensure the rules were deleted.