Skip to content

Commit

Permalink
front: drop PathStep.ch
Browse files Browse the repository at this point in the history
This information is duplicated in PathItemLocation.secondary_code.
This has several downsides.

First, the field secondary_code is inherited in the PathStep type
event if it's never set (we omit() it when receiving the train
schedule path from the backend). As a result writing
"pathStep.secondary_code" in code is valid even if it would never
contain anything. Additionally, the field needs to be converted
back and forth from/to ch when sending HTTP requests.

Second, the field is always defined even for path steps for which
a ch doesn't make sense. For instance, if the path step is an
operational_point or is a TrackOffset, the ch property would still
be defined (and always be empty).

All of this makes it easy to trip over and use "ch" or
"secondary_code" in contexts where it doesn't make sense. While
this is easily discovered in testing when initially writing a
feature, any subsequent refactoring may subtly break the rest of
the code.

To fix this, drop the "ch" field and only use the auto-generated
"secondary_code" field. This forces any code making use of ch to
verify (with the "in" keyword) that accessing the field makes
sense.

Signed-off-by: Simon Ser <contact@emersion.fr>
  • Loading branch information
emersion committed Nov 21, 2024
1 parent d843b1a commit 131a5b3
Show file tree
Hide file tree
Showing 13 changed files with 46 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,14 @@ describe('updatePathStepsFrom', () => {
id: 'id79',
deleted: false,
uic: 87747006,
ch: 'BV',
secondary_code: 'BV',
name: '87747006',
},
{
id: 'id87',
deleted: false,
uic: 87747006,
ch: 'P2',
secondary_code: 'P2',
name: '87747006',
arrival: '15:00:00',
stopFor: null,
Expand All @@ -161,7 +161,7 @@ describe('updatePathStepsFrom', () => {
id: 'id80',
deleted: false,
uic: 87747337,
ch: 'BV',
secondary_code: 'BV',
name: '87747337',
arrival: null,
stopFor: '0',
Expand All @@ -186,7 +186,7 @@ describe('updatePathStepsFrom', () => {
id: 'id79',
deleted: false,
uic: 87747006,
ch: 'BV',
secondary_code: 'BV',
name: 'Grenadille',
kp: '130+538',
positionOnPath: 0,
Expand All @@ -196,7 +196,7 @@ describe('updatePathStepsFrom', () => {
id: 'id87',
deleted: false,
uic: 87747006,
ch: 'P2', // should not be BV here, it has the same uic but not the same ch
secondary_code: 'P2', // should not be BV here, it has the same uic but not the same ch
name: 'Grenadille',
arrival: '15:00:00',
stopFor: null,
Expand All @@ -208,7 +208,7 @@ describe('updatePathStepsFrom', () => {
id: 'id80',
deleted: false,
uic: 87747337,
ch: 'BV',
secondary_code: 'BV',
name: 'Voreppe',
arrival: null,
stopFor: '0',
Expand Down Expand Up @@ -260,7 +260,6 @@ describe('updatePathStepsFrom', () => {
const expected = [
{
id: 'whatev-0',
ch: 'BV',
trigram: 'GE',
secondary_code: 'BV',
name: 'Grenadille',
Expand All @@ -270,7 +269,6 @@ describe('updatePathStepsFrom', () => {
},
{
id: 'whatev-1',
ch: 'P2',
trigram: 'GE',
secondary_code: 'P2',
name: 'Grenadille',
Expand All @@ -281,7 +279,6 @@ describe('updatePathStepsFrom', () => {
},
{
id: 'who-0',
ch: 'BV',
secondary_code: 'BV',
trigram: 'VPE',
name: 'Voreppe',
Expand All @@ -298,20 +295,20 @@ describe('updatePathStepsFrom', () => {
{
id: 'whatev-0',
trigram: 'GE',
ch: 'BV',
secondary_code: 'BV',
name: '87747006',
},
{
id: 'whatev-1',
trigram: 'GE',
ch: 'P2',
secondary_code: 'P2',
name: '87747006',
arrival: '15:00:00',
},
{
id: 'who-0',
trigram: 'VPE',
ch: 'BV',
secondary_code: 'BV',
name: '87747337',
},
];
Expand All @@ -332,7 +329,7 @@ describe('updatePathStepsFrom', () => {
const expected = [
{
id: 'whatev-0',
ch: 'BV',
secondary_code: 'BV',
trigram: 'GE',
name: '87747006',
kp: undefined,
Expand All @@ -341,7 +338,7 @@ describe('updatePathStepsFrom', () => {
},
{
id: 'whatev-1',
ch: 'P2',
secondary_code: 'P2',
trigram: 'GE',
name: '87747006',
arrival: '15:00:00',
Expand All @@ -351,7 +348,7 @@ describe('updatePathStepsFrom', () => {
},
{
id: 'who-0',
ch: 'BV',
secondary_code: 'BV',
trigram: 'VPE',
name: '87747337',
kp: undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@ const computeBasePathSteps = (trainSchedule: TrainScheduleResult) =>
reception_signal: receptionSignal,
} = correspondingSchedule || {};

const stepWithoutSecondaryCode = omit(step, ['secondary_code']);

if ('track' in stepWithoutSecondaryCode) {
stepWithoutSecondaryCode.offset = mmToM(stepWithoutSecondaryCode.offset!);
}

let name;
if ('trigram' in step) {
name = step.trigram + (step.secondary_code ? `/${step.secondary_code}` : '');
Expand All @@ -70,8 +64,8 @@ const computeBasePathSteps = (trainSchedule: TrainScheduleResult) =>
}

return {
...stepWithoutSecondaryCode,
ch: 'secondary_code' in step ? step.secondary_code : undefined,
...step,
...('track' in step && { offset: mmToM(step.offset) }),
name,
arrival, // ISODurationString
stopFor: stopFor ? ISO8601Duration2sec(stopFor).toString() : stopFor,
Expand All @@ -91,11 +85,11 @@ export function updatePathStepsFromOperationalPoints(
matchPathStepAndOp(step, suggestedOp)
);

const { kp, name, ch } = correspondingOp || step;
const { kp, name } = correspondingOp || step;

return {
...step,
ch,
...(correspondingOp && { secondary_code: correspondingOp.ch }),
kp,
name,
positionOnPath: pathfindingResult.path_item_positions[i],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ function formatChCode(chCode: string) {
const StdcmOperationalPoint = ({ point, opPointId, disabled }: StdcmOperationalPointProps) => {
const { t } = useTranslation('stdcm');
const dispatch = useAppDispatch();
const pointCh =
'secondary_code' in point && point.secondary_code ? point.secondary_code : undefined;

const { searchTerm, chCodeFilter, sortedSearchResults, setSearchTerm, setChCodeFilter } =
useSearchOperationalPoint({ initialSearchTerm: point.name, initialChCodeFilter: point.ch });
useSearchOperationalPoint({ initialSearchTerm: point.name, initialChCodeFilter: pointCh });

const { updateStdcmPathStep } = useOsrdConfActions() as StdcmConfSliceActions;

Expand Down Expand Up @@ -75,15 +77,15 @@ const StdcmOperationalPoint = ({ point, opPointId, disabled }: StdcmOperationalP
);

const dispatchNewPoint = (p?: SearchResultItemOperationalPoint) => {
if (p && p.ch === point.ch && 'uic' in point && p.uic === point.uic) return;
if (p && 'uic' in point && p.ch === point.secondary_code && p.uic === point.uic) return;
const newPoint = p
? {
name: p.name,
ch: p.ch,
secondary_code: p.ch,
uic: p.uic,
coordinates: p.geographic.coordinates,
}
: { name: undefined, ch: undefined, uic: -1, coordinates: undefined };
: { name: undefined, secondary_code: undefined, uic: -1, coordinates: undefined };
dispatch(updateStdcmPathStep({ id: point.id, updates: newPoint }));
};

Expand Down Expand Up @@ -124,7 +126,7 @@ const StdcmOperationalPoint = ({ point, opPointId, disabled }: StdcmOperationalP
useEffect(() => {
if (point) {
setSearchTerm(point.name || '');
setChCodeFilter(point.ch || '');
setChCodeFilter(pointCh || '');
} else {
setSearchTerm('');
setChCodeFilter(undefined);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,9 @@ const SimulationReportSheet = ({
</TD>
</View>
<View style={styles.convoyAndRoute.stopTableChWidth}>
<TD style={styles.convoyAndRoute.stopTableChColumn}>{step.ch}</TD>
<TD style={styles.convoyAndRoute.stopTableChColumn}>
{'secondary_code' in step ? step.secondary_code : undefined}
</TD>
</View>
<View style={styles.convoyAndRoute.stopTableEndWidth}>
<TD style={styles.convoyAndRoute.stopTableItalicColumn}>
Expand Down Expand Up @@ -305,7 +307,12 @@ const SimulationReportSheet = ({
const prevStep = operationalPointsList[index - 1];
const isViaInSimulationPath = stdcmData.simulationPathSteps
.slice(1, -1)
.some((pathStep) => pathStep.name === step.name && pathStep.ch === step.ch);
.some(
(pathStep) =>
pathStep.name === step.name &&
'secondary_code' in pathStep &&
pathStep.secondary_code === step.ch
);
const isViaWithoutStop = isViaInSimulationPath && step.duration === 0;
const isNotExtremity = !isFirstStep && !isLastStep;
const isStepWithDuration = step.duration !== 0 && !isLastStep;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ const StcdmResultsTable = ({
const isLastStep = index === operationalPointsList.length - 1;
const prevStep = operationalPointsList[index - 1];
const isRequestedPathStep = stdcmData.simulationPathSteps.some(
(pathStep) => pathStep.name === step.name && pathStep.ch === step.ch
(pathStep) =>
pathStep.name === step.name &&
'secondary_code' in pathStep &&
pathStep.secondary_code === step.ch
);
const shouldRenderRow = isFirstStep || isRequestedPathStep || isLastStep;
const isPathStep =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ const Vias = ({ zoomToFeaturePoint, shouldManageStopDuration }: ViasProps) => {
<small data-testid="via-dropped-name" className="mr-1 text-nowrap">
{`${via.name || (via.positionOnPath && `KM ${(Math.round(via.positionOnPath) / 1000000).toFixed(3)}`) || t('unavailableDistance')}`}
</small>
{via.ch && <small data-testid="via-dropped-ch">{via.ch}</small>}
{'secondary_code' in via && via.secondary_code && (
<small data-testid="via-dropped-ch">{via.secondary_code}</small>
)}
{'uic' in via && (
<small data-testid="via-dropped-uic" className="text-muted ml-3">
{formatUicToCi(via.uic)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ const TypeAndPath = ({ setDisplayTypeAndPath }: TypeAndPathProps) => {
.filter((op) => op.trigram !== '')
.map(({ uic, ch }) => ({
uic,
ch,
secondary_code: ch,
id: nextId(),
}));
Expand Down
4 changes: 2 additions & 2 deletions front/src/modules/pathfinding/helpers/getStepLocation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ const getStepLocation = (step: PathStep | StdcmPathStep): PathItemLocation => {
return { operational_point: step.operational_point };
}
if ('trigram' in step) {
return { trigram: step.trigram, secondary_code: step.ch };
return { trigram: step.trigram, secondary_code: step.secondary_code };
}
if (step.uic === -1) {
throw new Error('Invalid UIC');
}
return { uic: step.uic, secondary_code: step.ch };
return { uic: step.uic, secondary_code: step.secondary_code };
};

export default getStepLocation;
2 changes: 1 addition & 1 deletion front/src/modules/pathfinding/hooks/usePathfinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ export const usePathfinding = (
...(correspondingOp && {
name: correspondingOp.name,
uic: correspondingOp.uic,
ch: correspondingOp.ch,
secondary_code: correspondingOp.ch,
kp: correspondingOp.kp,
coordinates: correspondingOp.coordinates,
}),
Expand Down
4 changes: 2 additions & 2 deletions front/src/modules/pathfinding/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ export const matchPathStepAndOp = (
return step.operational_point === op.opId;
}
if ('uic' in step) {
return step.uic === op.uic && step.ch === op.ch;
return step.uic === op.uic && step.secondary_code === op.ch;
}
if ('trigram' in step) {
return step.trigram === op.trigram && step.ch === op.ch;
return step.trigram === op.trigram && step.secondary_code === op.ch;
}
return step.track === op.track && step.offset === op.offsetOnTrack;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('formatSchedule', () => {
id: 'id331',
deleted: false,
uic: 8706,
ch: 'BV',
secondary_code: 'BV',
kp: '130+538',
name: 'G',
positionOnPath: 0,
Expand All @@ -27,7 +27,7 @@ describe('formatSchedule', () => {
id: 'id332',
deleted: false,
uic: 8737,
ch: 'BV',
secondary_code: 'BV',
kp: '117+422',
name: 'V',
positionOnPath: 13116000,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,6 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
locked: newVia.locked,
deleted: newVia.deleted,
name: newVia.name,
ch: newVia.ch,
};

store.dispatch(slice.actions.upsertViaFromSuggestedOP(newVia));
Expand Down
1 change: 0 additions & 1 deletion front/src/reducers/osrdconf/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ export type PathStep = PathItemLocation & {
coordinates?: Position;
// Metadatas given by the search endpoint in TypeAndPath (name)
name?: string;
ch?: string; // can be used to difference two steps from each other when they have same uic
// Metadatas given by ManageTrainScheduleMap click event to add origin/destination/via
metadata?: {
lineCode: number;
Expand Down

0 comments on commit 131a5b3

Please sign in to comment.