fix(organizations): re-point explored caves on duplicate merge

[dawoldo]

🤔 What

Fix the permanent deletion of an organization so that its explored caves are preserved when the deletion is a merge into another organization.

• DELETE /api/v1/organizations/:id?isPermanent=true&entityId=<survivor> now re-points the deleted org's j_grotto_cave_explorer rows to the surviving org.
• Without an entityId (plain permanent delete), behaviour is unchanged: the relationships are dropped.
• Added integration test coverage for the merge case, including the duplicate-cave edge case.

🤷‍♂️ Why

When two organizations are duplicates, moderators permanently delete one while passing entityId to merge it into the survivor. Documents, editor/library references and redirectTo pointers were already re-pointed to the survivor, but explored caves were not: JGrottoCaveExplorer.destroy({ grotto: organizationId }) ran unconditionally.

The result was silent data loss — merging two duplicate organizations destroyed the exploration history of the one being deleted, even though it belonged to the same real-world organization.

🔍 How

In api/controllers/v1/organization/delete.js, the unconditional destroy is now branched on shouldMergeInto:

• No merge target → unchanged JGrottoCaveExplorer.destroy({ grotto: organizationId }).
• Merge target → two native queries:
  1. DELETE the deleted org's rows for caves the survivor already explores.
  2. UPDATE ... SET id_grotto = <survivor> for the remaining rows.

Step 1 is required because j_grotto_cave_explorer has a composite primary key (id_cave, id_grotto): a blind UPDATE would raise a unique violation as soon as both organizations explore the same cave — precisely the situation for duplicates. Deduplicating first, then re-pointing, keeps exactly one row per cave for the survivor.

Raw SQL (sails.sendNativeQuery) is used rather than the Waterline model because this is a set-based update on a join table; doing it through the ORM would mean fetching and re-creating rows one by one.

🧪 Testing

The integration test in test/integration/4_routes/Organization/delete.test.js covers the merge path.

Manual check:

1. Create two organizations A and B.
2. Give A two explored caves, one of which B also explores.
3. DELETE /api/v1/organizations/<A>?isPermanent=true&entityId=<B> as a moderator.
4. B should now explore both caves, each exactly once, and no row should reference A.

Note: permanent deletion still returns 501 if the organization has partner networks, partner entrances or cavers — use an org without those to reach this code path.

📸 Previews

N/A — API-only change, no UI surface.

[ClemRz]

The fix is correct, well-reasoned, and the test covers the meaningful edge cases. A few notes below.

Issues (Must Fix)

• [api/controllers/v1/organization/delete.js:83-97] The DELETE and the UPDATE run as two separate auto-committed statements. If the UPDATE fails after the DELETE succeeds, the surviving org loses the caves that were shared with the deleted org — they get deleted but never re-pointed. The rest of the permanent-delete block has the same pattern (no transaction wrapping), but this is the only place where two raw statements form an interdependent pair where partial success produces silent data loss.

  Consider wrapping these two in an explicit transaction:

  await sails.sendNativeQuery('BEGIN');
  try {
    await sails.sendNativeQuery(
      `DELETE FROM j_grotto_cave_explorer d
         WHERE d.id_grotto = $1
           AND EXISTS (
             SELECT 1 FROM j_grotto_cave_explorer k
             WHERE k.id_grotto = $2 AND k.id_cave = d.id_cave
           )`,
      [organizationId, mergeIntoId]
    );
    await sails.sendNativeQuery(
      `UPDATE j_grotto_cave_explorer
         SET id_grotto = $2
         WHERE id_grotto = $1`,
      [organizationId, mergeIntoId]
    );
    await sails.sendNativeQuery('COMMIT');
  } catch (err) {
    await sails.sendNativeQuery('ROLLBACK');
    throw err;
  }

  Alternatively, the two statements can be collapsed into one atomic query using a CTE:

  WITH duplicates AS (
    DELETE FROM j_grotto_cave_explorer
    WHERE id_grotto = $1
      AND EXISTS (
        SELECT 1 FROM j_grotto_cave_explorer k
        WHERE k.id_grotto = $2 AND k.id_cave = j_grotto_cave_explorer.id_cave
      )
  )
  UPDATE j_grotto_cave_explorer
    SET id_grotto = $2
    WHERE id_grotto = $1

  The CTE form is simpler and avoids the need for manual transaction management.

Suggestions (Should Consider)

• [api/controllers/v1/organization/delete.js:19] organizationId is the raw string from req.param('id') (the validateId policy is not applied to this route — only tokenAuth is, per config/policies.js:278). It ends up as $1 in both native queries. PostgreSQL will implicitly cast a numeric string to integer for a FK column, so this works in practice, but it's a latent footgun: a non-numeric value (bypassing route-level validation) would produce a database-level error rather than a clean notFound. Pre-existing issue, not introduced here, but worth noting while the surrounding code is being touched.

• [test/integration/4_routes/Organization/delete.test.js:109] The test creates targetOrg without isDeleted: true, which is correct for a merge target. It would also be useful to assert that the sharedCave is present in survivorCaves (not just that the count is 2 and both IDs appear). The current deepEqual on the sorted ID array already covers this implicitly, so this is just an observation — the test is solid.

• [assets/swaggerV1.yaml:5818] The DELETE /organizations/{id} spec doesn't document the isPermanent or entityId query parameters, or the 501 response. This PR doesn't change that surface, but since the explored-caves merge path is now correctly implemented, it's a good moment to fill in the missing parameter documentation.

Nitpicks (Optional)

• [api/controllers/v1/organization/delete.js:74-100] The inline comment block is thorough and helpful. Minor: the phrase "deleted duplicate's relationships" in the comment could be "the deleted org's relationships" to stay consistent with the surrounding code's vocabulary (shouldMergeInto, mergeIntoId). No action needed.

[ClemRz]

The transaction concern from the previous review has been addressed — the two-statement sequence is now correctly wrapped in sails.getDatastore().transaction(async (db) => { ... }) with .usingConnection(db), matching the established pattern elsewhere in the codebase (e.g. EnrichmentQueueService, GeoAssociationService). The logic itself is sound and the test covers the important PK-collision edge case. One correctness issue to fix before merging.

Issues (Must Fix)

• [assets/swaggerV1.yaml:5857] The responses block documents '204' for a successful operation, but the controller calls ControllerService.treatAndConvert which resolves to res.ok() (HTTP 200). The existing tests also assert .expect(200). This PR touches the responses block, so it's the right moment to fix it.

        '200':
          description: Successful operation
  

Suggestions (Should Consider)

• [api/controllers/v1/organization/delete.js:87-107] sails.getDatastore().sendNativeQuery(...).usingConnection(db) is the correct pattern, but CommonService.query(sql, values, db) already wraps it and is used throughout the codebase for raw queries inside transactions (the connection third argument threads the connection through). Consider using it for consistency:

  const CommonService = require('../../../services/CommonService');
  // ...
  await sails.getDatastore().transaction(async (db) => {
    await CommonService.query(
      `DELETE FROM j_grotto_cave_explorer d
         WHERE d.id_grotto = $1
           AND EXISTS (
             SELECT 1 FROM j_grotto_cave_explorer k
             WHERE k.id_grotto = $2 AND k.id_cave = d.id_cave
           )`,
      [organizationId, mergeIntoId],
      db
    );
    await CommonService.query(
      `UPDATE j_grotto_cave_explorer
         SET id_grotto = $2
         WHERE id_grotto = $1`,
      [organizationId, mergeIntoId],
      db
    );
  });

• [config/policies.js:278] The validateId policy is now correctly applied to the delete route. Note that entityId (a query param, not a path param) is not covered by validateId, which only inspects path params ending in Id plus the id route param. The controller already handles non-numeric entityId safely via parseInt + Number.isNaN, so this is not a bug — just worth being aware of.

Nitpicks (Optional)

• [assets/swaggerV1.yaml:5831] The id path parameter is typed as type: string while entityId is type: integer. The actual id is an integer FK — this is a pre-existing inconsistency, no action needed here.