From 40957e909b68ec4b59f25740ce8052ac6cbb91fc Mon Sep 17 00:00:00 2001
From: Maurice <mauriceboe@icloud.com>
Date: Tue, 25 Aug 2026 10:45:14 +0200
Subject: [PATCH] refactor: drop two returns that could only ever say one thing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both functions had two exits handing back the identical value, which reads as a
decision and is none — Sonar's S3516, and the last two Blockers on the backend.

colorizeImportedTracks returned the very object it was given, coloured in place.
Saying `void` and letting the two importers return their own result is what the
function was doing all along. PLACES-SVC-005 checked the pass-through and now
checks the thing worth checking: an empty import writes no colours.

acceptInvite bailed out early on a refusal only to return the same result the
line below returns. The notification is what the early exit was really skipping,
so it is the notification that gets the condition.
---
 server/src/nest/places/places.service.ts      | 18 ++++++++++++------
 server/src/nest/vacay/vacay.service.ts        |  5 +++--
 server/tests/unit/nest/places.service.test.ts |  9 ++++++---
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts
index fea5d1e94..abd50cd45 100644
--- a/server/src/nest/places/places.service.ts
+++ b/server/src/nest/places/places.service.ts
@@ -562,7 +562,9 @@ export class PlacesService {
   // -------------------------------------------------------------------------
 
   importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOptions = {}): GpxImportResult | null {
-    return this.colorizeImportedTracks(tripId, this.importGpxRows(tripId, fileBuffer, opts));
+    const result = this.importGpxRows(tripId, fileBuffer, opts);
+    this.colorizeImportedTracks(tripId, result);
+    return result;
   }
 
   /**
@@ -713,7 +715,9 @@ export class PlacesService {
   // -------------------------------------------------------------------------
 
   async importMapFile(tripId: string, fileBuffer: Buffer, filename: string, opts: KmlImportOptions = {}): Promise<PlaceImportResult> {
-    return this.colorizeImportedTracks(tripId, await this.importMapFileRows(tripId, fileBuffer, filename, opts));
+    const result = await this.importMapFileRows(tripId, fileBuffer, filename, opts);
+    this.colorizeImportedTracks(tripId, result);
+    return result;
   }
 
   private async importMapFileRows(tripId: string, fileBuffer: Buffer, filename: string, opts: KmlImportOptions = {}): Promise<PlaceImportResult> {
@@ -844,10 +848,14 @@ export class PlacesService {
    *
    * Only rows that carry geometry are touched, and only ones that have no
    * colour yet; plain waypoints and existing places stay untouched.
+   *
+   * Writes the colour into the rows AND onto the passed-in result, in place —
+   * it used to hand the same object back, which read like a transformation and
+   * was none.
    */
-  private colorizeImportedTracks<T extends { places: ImportedPlace[] } | null>(tripId: string, result: T): T {
+  private colorizeImportedTracks(tripId: string, result: { places: ImportedPlace[] } | null): void {
     const tracks = result?.places?.filter((p) => p.route_geometry && !p.route_color) ?? [];
-    if (tracks.length === 0) return result;
+    if (tracks.length === 0) return;
 
     // Read and write in one transaction so two concurrent imports cannot both
     // read the same set of free colours.
@@ -867,8 +875,6 @@ export class PlacesService {
         track.route_color = color;
       });
     });
-
-    return result;
   }
 
   // -------------------------------------------------------------------------
diff --git a/server/src/nest/vacay/vacay.service.ts b/server/src/nest/vacay/vacay.service.ts
index 474ef7a7a..cff2de8d3 100644
--- a/server/src/nest/vacay/vacay.service.ts
+++ b/server/src/nest/vacay/vacay.service.ts
@@ -716,8 +716,9 @@ export class VacayService {
       return {};
     });
 
-    if (result.error) return result;
-    this.notifyPlanUsers(planId, socketId, 'vacay:accepted');
+    // Only announce a fusion that actually happened — the transaction returns the
+    // refusal for an invite that was already gone.
+    if (!result.error) this.notifyPlanUsers(planId, socketId, 'vacay:accepted');
     return result;
   }
 
diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts
index 5de63f564..6663f6d1c 100644
--- a/server/tests/unit/nest/places.service.test.ts
+++ b/server/tests/unit/nest/places.service.test.ts
@@ -1157,11 +1157,14 @@ describe('PlacesService — automatic track colours (#776)', () => {
     expect(new Set(colors).size).toBe(colors.length);
   });
 
-  it('PLACES-SVC-005 — an import that yields nothing is passed straight through', () => {
+  it('PLACES-SVC-005 — an import that yields nothing colours nothing', () => {
     const { user } = createUser(testDb);
     const trip = createTrip(testDb, user.id);
-    expect((svc as any).colorizeImportedTracks(String(trip.id), null)).toBeNull();
-    expect((svc as any).colorizeImportedTracks(String(trip.id), { places: [] })).toEqual({ places: [] });
+
+    expect(() => (svc as any).colorizeImportedTracks(String(trip.id), null)).not.toThrow();
+    expect(() => (svc as any).colorizeImportedTracks(String(trip.id), { places: [] })).not.toThrow();
+
+    expect(tracksOf(trip.id)).toEqual([]);
   });
 });