Skip to content

Commit 85b4680

Browse files
iTzxMigzthomasspina
authored andcommitted
fix: Group services by travel time similarity in segment times modal
expandGroupedDataToServices was doing a full replace of each non-edited service with the representative's edits, which injected periods (e.g. EV) onto services that were merged in via mergeServicesWithSubsetPeriods and never had those periods originally. Propagate edits only for periods the target service already had, and add tests covering the leak case as well as confirming that serviceIds[0] is always the service with the most periods regardless of input order.
1 parent 4ee4d07 commit 85b4680

3 files changed

Lines changed: 146 additions & 15 deletions

File tree

packages/transition-common/src/services/path/PathServiceGrouping.ts

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -301,10 +301,14 @@ const buildServiceGroups = (
301301
allDayKeys.filter((day) => service.get(day)).forEach((d) => activeDaysSet.add(d));
302302
}
303303
const averageTimesByPeriod = getAverageSegmentTimesByPeriod(path, ids[0]);
304+
const hasHoliday = ids.some((id) => {
305+
const svc = servicesCollection?.getById(id);
306+
return svc && isHolidayService(svc);
307+
});
304308
return {
305309
serviceIds: ids,
306310
activeDays: [...activeDaysSet],
307-
hasHolidayService: false,
311+
hasHolidayService: hasHoliday,
308312
averageTimesByPeriod
309313
};
310314
});
@@ -397,8 +401,12 @@ export const groupServicesByTravelTimes = (
397401

398402
/**
399403
* Expand grouped segment time overrides to all services in each group.
400-
* When saving, the edits made on the representative service of a group
401-
* are copied to all other services in the same group.
404+
* When saving, edits made on the representative service of a group are
405+
* propagated to the other services in the same group — but only for
406+
* periods that those services already had in their original data.
407+
* Periods that exist only on the representative (e.g. when a smaller
408+
* service was merged into a superset via mergeServicesWithSubsetPeriods)
409+
* are not injected into services that never had them.
402410
*
403411
* @param localData - Edited segment times keyed by service ID then period shortname
404412
* @param serviceGroups - The service groups to expand across
@@ -411,14 +419,17 @@ export const expandGroupedDataToServices = (
411419
const expanded = { ...localData };
412420
for (const group of serviceGroups) {
413421
const editedServiceId = group.serviceIds.find((id) => expanded[id]);
414-
if (editedServiceId) {
415-
for (const otherId of group.serviceIds) {
416-
if (otherId !== editedServiceId) {
417-
expanded[otherId] = Object.fromEntries(
418-
Object.entries(expanded[editedServiceId]).map(([k, arr]) => [k, [...arr]])
419-
);
420-
}
422+
if (!editedServiceId) continue;
423+
const editedEntries = expanded[editedServiceId];
424+
for (const otherId of group.serviceIds) {
425+
if (otherId === editedServiceId) continue;
426+
const existing = expanded[otherId];
427+
if (!existing) continue;
428+
const merged: Record<string, number[]> = {};
429+
for (const period of Object.keys(existing)) {
430+
merged[period] = editedEntries[period] ? [...editedEntries[period]] : existing[period];
421431
}
432+
expanded[otherId] = merged;
422433
}
423434
}
424435
return expanded;

packages/transition-common/src/services/path/__tests__/PathServiceGrouping.test.ts

Lines changed: 118 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ describe('groupServicesByTravelTimes', () => {
208208
expect(groups.length).toEqual(1);
209209
expect(groups[0].serviceIds).toContain('regular');
210210
expect(groups[0].serviceIds).toContain('holiday');
211+
expect(groups[0].hasHolidayService).toBe(true);
211212
});
212213

213214
test('service with subset of periods merges into group with more periods', () => {
@@ -240,8 +241,55 @@ describe('groupServicesByTravelTimes', () => {
240241
expect(groups[0].activeDays.sort()).toEqual(['friday', 'monday', 'saturday', 'sunday', 'thursday', 'tuesday', 'wednesday']);
241242
});
242243

244+
test('representative (serviceIds[0]) is the service with the most periods even when passed last', () => {
245+
// subsetService has 3 periods, fullService has 4. They share the same times for
246+
// the 3 common periods, so subsetService is merged into fullService's bucket.
247+
// Passing subsetService first in serviceIds must not make it the representative.
248+
const path = makePath([
249+
{
250+
periodShortname: 'AM',
251+
services: [
252+
{ id: 'subsetService', travelTimeSeconds: [100, 100] },
253+
{ id: 'fullService', travelTimeSeconds: [100, 100] }
254+
]
255+
},
256+
{
257+
periodShortname: 'MD',
258+
services: [
259+
{ id: 'subsetService', travelTimeSeconds: [90, 90] },
260+
{ id: 'fullService', travelTimeSeconds: [90, 90] }
261+
]
262+
},
263+
{
264+
periodShortname: 'PM',
265+
services: [
266+
{ id: 'subsetService', travelTimeSeconds: [110, 110] },
267+
{ id: 'fullService', travelTimeSeconds: [110, 110] }
268+
]
269+
},
270+
// Only fullService has an EV period — this is what makes its key a superset.
271+
{ periodShortname: 'EV', services: [{ id: 'fullService', travelTimeSeconds: [80, 80] }] }
272+
]);
273+
274+
const services = makeServicesCollection({
275+
subsetService: makeService(
276+
'subsetService',
277+
{ monday: true, tuesday: true, wednesday: true, thursday: true, friday: true },
278+
'2026-01-01',
279+
'2026-12-31'
280+
),
281+
fullService: makeService('fullService', { saturday: true, sunday: true }, '2026-01-01', '2026-12-31')
282+
});
283+
284+
const groups = groupServicesByTravelTimes(path, ['subsetService', 'fullService'], 800, services);
285+
286+
expect(groups.length).toEqual(1);
287+
expect(groups[0].serviceIds).toHaveLength(2);
288+
expect(groups[0].serviceIds[0]).toEqual('fullService');
289+
expect(Object.keys(groups[0].averageTimesByPeriod).sort()).toEqual(['AM', 'EV', 'MD', 'PM']);
290+
});
291+
243292
test('service with no trips for the path produces empty fingerprint', () => {
244-
// TODO: Check if we keep this test
245293
const path = makePath([
246294
{
247295
periodShortname: 'AM',
@@ -389,7 +437,9 @@ describe('expandGroupedDataToServices', () => {
389437
}
390438
];
391439
const localData = {
392-
s1: { AM: [100, 200] }
440+
s1: { AM: [100, 200] },
441+
s2: { AM: [50, 60] },
442+
s3: { AM: [70, 80] }
393443
};
394444

395445
const expanded = expandGroupedDataToServices(localData, groups);
@@ -421,7 +471,9 @@ describe('expandGroupedDataToServices', () => {
421471
}
422472
];
423473
const localData = {
424-
s1: { AM: [100, 200], PM: [300, 400] }
474+
s1: { AM: [100, 200], PM: [300, 400] },
475+
s2: { AM: [10, 20], PM: [30, 40] },
476+
s3: { AM: [50, 60], PM: [70, 80] }
425477
};
426478

427479
const expanded = expandGroupedDataToServices(localData, groups);
@@ -438,4 +490,67 @@ describe('expandGroupedDataToServices', () => {
438490
expect(expanded.s1.PM[1]).toBe(400);
439491
expect(expanded.s3.PM[1]).toBe(400);
440492
});
493+
494+
test('does not propagate periods to services that did not originally have them', () => {
495+
// fullService is the representative (4 periods), subsetService was merged in
496+
// via mergeServicesWithSubsetPeriods and only has 3 periods. Editing EV on the
497+
// representative must not create an EV entry on subsetService.
498+
const groups = [
499+
{
500+
serviceIds: ['fullService', 'subsetService'],
501+
activeDays: ['monday'],
502+
hasHolidayService: false,
503+
averageTimesByPeriod: {}
504+
}
505+
];
506+
const localData = {
507+
fullService: {
508+
AM: [100, 200],
509+
MD: [90, 180],
510+
PM: [110, 220],
511+
EV: [80, 160]
512+
},
513+
subsetService: {
514+
AM: [100, 200],
515+
MD: [90, 180],
516+
PM: [110, 220]
517+
}
518+
};
519+
520+
const expanded = expandGroupedDataToServices(localData, groups);
521+
522+
expect(Object.keys(expanded.subsetService).sort()).toEqual(['AM', 'MD', 'PM']);
523+
expect(expanded.subsetService.EV).toBeUndefined();
524+
expect(expanded.fullService.EV).toEqual([80, 160]);
525+
});
526+
527+
test('propagates edited shared periods to the subset service without adding new ones', () => {
528+
// User edits AM and EV on the representative. AM is shared, EV is representative-only.
529+
// subsetService should receive the AM edits but nothing for EV.
530+
const groups = [
531+
{
532+
serviceIds: ['fullService', 'subsetService'],
533+
activeDays: ['monday'],
534+
hasHolidayService: false,
535+
averageTimesByPeriod: {}
536+
}
537+
];
538+
const localData = {
539+
fullService: {
540+
AM: [111, 222],
541+
PM: [110, 220],
542+
EV: [77, 155]
543+
},
544+
subsetService: {
545+
AM: [100, 200],
546+
PM: [110, 220]
547+
}
548+
};
549+
550+
const expanded = expandGroupedDataToServices(localData, groups);
551+
552+
expect(expanded.subsetService.AM).toEqual([111, 222]);
553+
expect(expanded.subsetService.PM).toEqual([110, 220]);
554+
expect(expanded.subsetService.EV).toBeUndefined();
555+
});
441556
});

packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,13 @@ import {
3131
const weekdays = ['monday', 'tuesday', 'wednesday', 'thursday', 'friday'] as const;
3232

3333
const dayKeyMap: Record<string, string> = {
34-
monday: 'Mon', tuesday: 'Tue', wednesday: 'Wed', thursday: 'Thu',
35-
friday: 'Fri', saturday: 'Sat', sunday: 'Sun'
34+
monday: 'Mon',
35+
tuesday: 'Tue',
36+
wednesday: 'Wed',
37+
thursday: 'Thu',
38+
friday: 'Fri',
39+
saturday: 'Sat',
40+
sunday: 'Sun'
3641
};
3742

3843
/** Build a human-readable label for a service group based on its active days. */

0 commit comments

Comments
 (0)