chore: pr feedback

migrations/V193__Add_MTO_Category_Delete_Trigger.sql → migrations/V200__Add_MTO_Category_Delete_Trigger.sql

@@ -10,30 +10,22 @@ BEGIN
     RETURN OLD;
   END IF;
 
-  -- Recursively gather all descendant categories, including OLD.id
-  WITH RECURSIVE cat_tree AS (
-    SELECT id, parent_id
-    FROM mto_category
-    WHERE id = OLD.id
-    UNION ALL
-    SELECT c.id, c.parent_id
-    FROM mto_category c
-           JOIN cat_tree t ON t.id = c.parent_id
-  )
-  SELECT array_agg(id) INTO cat_ids FROM cat_tree;
+  -- Since we only have one level of depth, we can simply select the current category and its direct children
+  SELECT array_agg(id) INTO cat_ids
+  FROM mto_category
+  WHERE parent_id = OLD.id OR id = OLD.id;
 
-  -- Always assign milestones to OLD.parent_id
-  -- If OLD.parent_id is NULL, they become uncategorized.
+  -- Reassign all milestones that referenced these categories to OLD.parent_id
   UPDATE mto_milestone
   SET mto_category_id = OLD.parent_id
   WHERE mto_category_id = ANY(cat_ids);
 
-  -- Delete all descendant categories except the current one
+  -- Delete all subcategories (direct children), excluding the category currently being deleted
   DELETE FROM mto_category
   WHERE id != OLD.id
     AND id = ANY(cat_ids);
 
-  RETURN OLD;
+  RETURN OLD; -- Allow the original DELETE to proceed, removing the OLD.id category
 END;
 $$ LANGUAGE plpgsql;
 

pkg/graph/resolvers/mto_category.go

@@ -37,26 +37,26 @@ func MTOCategoryCreate(ctx context.Context, logger *zap.Logger, principal authen
 }
 
 // MTOCategoryDelete removes an MTOCategory or SubCategory
-func MTOCategoryDelete(ctx context.Context, logger *zap.Logger, principal authentication.Principal, store *storage.Store,
-	id uuid.UUID,
-) error {
+func MTOCategoryDelete(logger *zap.Logger, principal authentication.Principal, store *storage.Store, id uuid.UUID) error {
 	principalAccount := principal.Account()
 	if principalAccount == nil {
 		return fmt.Errorf("principal doesn't have an account, username %s", principal.String())
 	}
 
-	existing, err := storage.MTOCategoryGetByID(store, logger, id)
-	if err != nil {
-		return fmt.Errorf("unable to delete MTO category. Err %w", err)
-	}
+	return sqlutils.WithTransactionNoReturn(store, func(tx *sqlx.Tx) error {
+		existing, err := storage.MTOCategoryGetByID(store, logger, id)
+		if err != nil {
+			return fmt.Errorf("unable to delete MTO category. Err %w", err)
+		}
 
-	// Just check access, don't apply changes here
-	err = BaseStructPreDelete(logger, existing, principal, store, true)
-	if err != nil {
-		return fmt.Errorf("unable to delete MTO category. Err %w", err)
-	}
+		// Just check access, don't apply changes here
+		err = BaseStructPreDelete(logger, existing, principal, store, true)
+		if err != nil {
+			return fmt.Errorf("unable to delete MTO category. Err %w", err)
+		}
 
-	return storage.MTOCategoryDelete(store, logger, id)
+		return storage.MTOCategoryDelete(tx, principalAccount.ID, id)
+	})
 }
 
 // MTOCategoryRename updates the name of MTOCategory or SubCategory

pkg/graph/resolvers/mto_category_test.go

@@ -678,7 +678,7 @@ func (suite *ResolverSuite) TestMTOCategoryDelete_NullID() {
 	// Expectation: This should return an error indicating the category does not exist or invalid input.
 
 	invalidID := uuid.Nil
-	err := MTOCategoryDelete(suite.testConfigs.Context, suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, invalidID)
+	err := MTOCategoryDelete(suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, invalidID)
 	suite.Error(err, "Deleting a category with a null (uuid.Nil) ID should result in an error")
 }
 
@@ -696,7 +696,7 @@ func (suite *ResolverSuite) TestMTOCategoryDelete_NoMilestones() {
 	suite.Equal(category.ID, catBeforeDelete.ID, "Category should exist before deletion")
 
 	// Delete the category
-	err = MTOCategoryDelete(suite.testConfigs.Context, suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, category.ID)
+	err = MTOCategoryDelete(suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, category.ID)
 	suite.NoError(err, "Deleting a category with no milestones should succeed")
 
 	// Confirm category no longer exists
@@ -731,7 +731,7 @@ func (suite *ResolverSuite) TestMTOCategoryDelete_TopLevelCategory() {
 	suite.NoError(err)
 
 	// Delete the top-level category
-	err = MTOCategoryDelete(suite.testConfigs.Context, suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, topCategory.ID)
+	err = MTOCategoryDelete(suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, topCategory.ID)
 	suite.NoError(err, "Deleting a top-level category should succeed")
 
 	// Verify top-level category and its subcategories no longer exist
@@ -779,13 +779,7 @@ func (suite *ResolverSuite) TestMTOCategoryDelete_SubCategory() {
 	suite.NoError(err, "Creating a milestone in the subcategory should succeed")
 
 	// Delete the subcategory
-	err = MTOCategoryDelete(
-		suite.testConfigs.Context,
-		suite.testConfigs.Logger,
-		suite.testConfigs.Principal,
-		suite.testConfigs.Store,
-		subCategory.ID,
-	)
+	err = MTOCategoryDelete(suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, subCategory.ID)
 	suite.NoError(err, "Deleting a subcategory should succeed")
 
 	// Verify subcategory no longer exists
@@ -829,7 +823,7 @@ func (suite *ResolverSuite) TestMTOCategoryDelete_RecursiveSubCategory() {
 
 	// Delete the top-level category, which should recursively delete subcategories
 	// and reassign or uncategorize all milestones.
-	err = MTOCategoryDelete(suite.testConfigs.Context, suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, topCategory.ID)
+	err = MTOCategoryDelete(suite.testConfigs.Logger, suite.testConfigs.Principal, suite.testConfigs.Store, topCategory.ID)
 	suite.NoError(err, "Deleting a top-level category with nested subcategories should succeed")
 
 	// Verify top-level category no longer exists

pkg/storage/mto_category.go

@@ -3,6 +3,8 @@ package storage
 import (
 	"fmt"
 
+	"github.com/jmoiron/sqlx"
+
 	"github.com/google/uuid"
 	"github.com/lib/pq"
 	"go.uber.org/zap"
@@ -83,10 +85,19 @@ func MTOCategoryCreate(np sqlutils.NamedPreparer, _ *zap.Logger, MTOCategory *mo
 }
 
 // MTOCategoryDelete deletes an existing MTOCategory from the database
-func MTOCategoryDelete(np sqlutils.NamedPreparer, _ *zap.Logger, id uuid.UUID) error {
+func MTOCategoryDelete(tx *sqlx.Tx, actorUserID uuid.UUID, id uuid.UUID) error {
+	// We need to set the session user variable so that the audit trigger knows who made the delete operation
+	err := setCurrentSessionUserVariable(tx, actorUserID)
+	if err != nil {
+		return err
+	}
+
+	// Delete the MTOCategory
+	// Note: Child subcategories are deleted by the database. If this category is assigned, the reference will be set
+	// to the parent category or NULL if this is a top-level category
 	arg := map[string]interface{}{"id": id}
 
-	_, procErr := sqlutils.GetProcedure[models.MTOCategory](np, sqlqueries.MTOCategory.Delete, arg)
+	_, procErr := sqlutils.GetProcedure[models.MTOCategory](tx, sqlqueries.MTOCategory.Delete, arg)
 	if procErr != nil {
 		return fmt.Errorf("issue deleting MTOCategory object: %w", procErr)
 	}