mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
refactor: drop two returns that could only ever say one thing
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.
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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([]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user