From 413bf67f152d1a16317b2a5030ecae3f76c0e484 Mon Sep 17 00:00:00 2001
From: andrea rota <andrea.rota@vizzuality.com>
Date: Thu, 8 Feb 2024 14:57:53 +0000
Subject: [PATCH] delete dangling feature_amounts_per_planning_unit data
 straight away when deleting a feature

This is a fast operation, so it can be done straight away. Other more
expensive db deletions are still left to the garbage collector.
---
 .../geo-features/geo-features.service.ts      | 71 +++++++++++++++++--
 .../cleanup-tasks/cleanup-tasks.service.ts    |  7 ++
 2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/api/apps/api/src/modules/geo-features/geo-features.service.ts b/api/apps/api/src/modules/geo-features/geo-features.service.ts
index 28751ec918..6a103ce238 100644
--- a/api/apps/api/src/modules/geo-features/geo-features.service.ts
+++ b/api/apps/api/src/modules/geo-features/geo-features.service.ts
@@ -384,6 +384,12 @@ export class GeoFeaturesService extends AppBaseService<
     data: UploadShapefileDTO,
     features: Record<string, any>[],
   ): Promise<Either<Error, GeoFeature>> {
+    /**
+     * @debt Avoid duplicating transaction scaffolding in multiple sites
+     * within this class: this should be wrapped in a utility method, and the
+     * code to be executed within transactions, as well as error handling
+     * (`catch`) and cleanup (`finally`) should be passed to the utility method.
+     */
     const apiQueryRunner = this.apiDataSource.createQueryRunner();
     const geoQueryRunner = this.geoDataSource.createQueryRunner();
 
@@ -478,6 +484,16 @@ export class GeoFeaturesService extends AppBaseService<
     );
   }
 
+  private async deleteFeatureAmountsPerPlanningUnit(
+    geoEntityManager: EntityManager,
+    featureId: string,
+  ): Promise<void> {
+    const repo = geoEntityManager.getRepository(
+      FeatureAmountsPerPlanningUnitEntity,
+    );
+    await repo.delete({ featureId });
+  }
+
   public async updateFeatureForProject(
     userId: string,
     featureId: string,
@@ -573,11 +589,56 @@ export class GeoFeaturesService extends AppBaseService<
       return left(featureNotDeletable);
     }
 
-    // NOTE
-    // This deletes the feature on the API DB but not on the GEO DB. That task is left up to the CleanupTasks
-    // cronjob; the cronjob is not activated manually because it may take a non trivial amount of time because
-    // of other data to be garbage collected
-    await this.repository.delete(featureId);
+    /**
+     * @debt Avoid duplicating transaction scaffolding in multiple sites within
+     * this class: this should be wrapped in a utility method, and the code to
+     * be executed within transactions, as well as error handling (`catch`) and
+     * cleanup (`finally`) should be passed to the utility method.
+     */
+    const apiQueryRunner = this.apiDataSource.createQueryRunner();
+    const geoQueryRunner = this.geoDataSource.createQueryRunner();
+
+    await apiQueryRunner.connect();
+    await geoQueryRunner.connect();
+
+    await apiQueryRunner.startTransaction();
+    await geoQueryRunner.startTransaction();
+
+    try {
+      /**
+       * Delete the feature, as well as its associated amount per planning unit
+       * data.
+       *
+       * This is fast, and leaving amount per planning unit data behind until it
+       * is eventually garbage-collected by a scheduled cleanup task may cause
+       * issues with piece exporters/importers, so it is ok to delete this
+       * associated data straight away.
+       *
+       * Other feature data (such as spatial data) can be left to the cleanup
+       * tasks to delete when suitable, as doing so synchronously at this stage
+       * would be a potentially expensive operation.
+       */
+      await apiQueryRunner.manager.delete(GeoFeature, { id: featureId });
+      await this.deleteFeatureAmountsPerPlanningUnit(
+        geoQueryRunner.manager,
+        featureId,
+      );
+      await apiQueryRunner.commitTransaction();
+      await geoQueryRunner.commitTransaction();
+    } catch (err) {
+      await apiQueryRunner.rollbackTransaction();
+      await geoQueryRunner.rollbackTransaction();
+
+      this.logger.error(
+        `An error occurred while deleting feature with id ${featureId} or any of its related data (changes have been rolled back)`,
+        String(err),
+      );
+      throw err;
+    } finally {
+      // you need to release a queryRunner which was manually instantiated
+      await apiQueryRunner.release();
+      await geoQueryRunner.release();
+    }
 
     return right(true);
   }
diff --git a/api/apps/geoprocessing/src/modules/cleanup-tasks/cleanup-tasks.service.ts b/api/apps/geoprocessing/src/modules/cleanup-tasks/cleanup-tasks.service.ts
index 171b0f065f..dc74a51b47 100644
--- a/api/apps/geoprocessing/src/modules/cleanup-tasks/cleanup-tasks.service.ts
+++ b/api/apps/geoprocessing/src/modules/cleanup-tasks/cleanup-tasks.service.ts
@@ -473,6 +473,13 @@ export class CleanupTasksService implements CleanupTasks {
         SELECT df.feature_id FROM dangling_features df
       );`,
     );
+
+    await this.geoEntityManager.query(
+      `DELETE FROM feature_amounts_per_planning_unit fappu
+      WHERE fappu.feature_id IN (
+        SELECT df.feature_id FROM dangling_features df
+      );`,
+    );
   }
 
   async deleteDanglingCostSurfacesIdsInGeoDb() {