From 94d3a8482a483a1d4faa44e9b81c3ad265752510 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Mon, 18 Nov 2024 14:43:02 +0100 Subject: [PATCH] front: drop PathStep.ch 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 --- .../useSetupItineraryForTrainUpdate.spec.ts | 36 +++++++++---------- .../hooks/useSetupItineraryForTrainUpdate.ts | 4 +-- .../StdcmForm/StdcmOperationalPoint.tsx | 12 ++++--- .../StdcmResults/SimulationReportSheet.tsx | 11 ++++-- .../StdcmResults/StdcmResultsTable.tsx | 5 ++- .../Itinerary/DisplayItinerary/Vias.tsx | 4 ++- .../components/Pathfinding/TypeAndPath.tsx | 1 - .../pathfinding/helpers/getStepLocation.ts | 4 +-- .../pathfinding/hooks/usePathfinding.ts | 2 +- front/src/modules/pathfinding/utils.ts | 4 +-- .../helpers/__tests__/formatSchedule.spec.ts | 4 +-- .../helpers/computeBasePathSteps.ts | 12 ++----- front/src/reducers/osrdconf/helpers.ts | 3 +- .../osrdConfCommon/__tests__/utils.ts | 1 - front/src/reducers/osrdconf/types.ts | 1 - 15 files changed, 53 insertions(+), 51 deletions(-) diff --git a/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts b/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts index 3a803d30487..785044bb175 100644 --- a/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts +++ b/front/src/applications/operationalStudies/hooks/__tests__/useSetupItineraryForTrainUpdate.spec.ts @@ -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, @@ -161,7 +161,7 @@ describe('updatePathStepsFrom', () => { id: 'id80', deleted: false, uic: 87747337, - ch: 'BV', + secondary_code: 'BV', name: '87747337', arrival: null, stopFor: '0', @@ -186,7 +186,7 @@ describe('updatePathStepsFrom', () => { id: 'id79', deleted: false, uic: 87747006, - ch: 'BV', + secondary_code: 'BV', name: 'Grenadille', kp: '130+538', positionOnPath: 0, @@ -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, @@ -208,7 +208,7 @@ describe('updatePathStepsFrom', () => { id: 'id80', deleted: false, uic: 87747337, - ch: 'BV', + secondary_code: 'BV', name: 'Voreppe', arrival: null, stopFor: '0', @@ -226,20 +226,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', }, ]; @@ -260,8 +260,8 @@ describe('updatePathStepsFrom', () => { const expected = [ { id: 'whatev-0', - ch: 'BV', trigram: 'GE', + secondary_code: 'BV', name: 'Grenadille', kp: '130+538', positionOnPath: 0, @@ -269,8 +269,8 @@ describe('updatePathStepsFrom', () => { }, { id: 'whatev-1', - ch: 'P2', trigram: 'GE', + secondary_code: 'P2', name: 'Grenadille', arrival: '15:00:00', kp: '129+952', @@ -279,8 +279,8 @@ describe('updatePathStepsFrom', () => { }, { id: 'who-0', - ch: 'BV', trigram: 'VPE', + secondary_code: 'BV', name: 'Voreppe', kp: '117+422', positionOnPath: 13116000, @@ -295,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', }, ]; @@ -329,7 +329,7 @@ describe('updatePathStepsFrom', () => { const expected = [ { id: 'whatev-0', - ch: 'BV', + secondary_code: 'BV', trigram: 'GE', name: '87747006', kp: undefined, @@ -338,7 +338,7 @@ describe('updatePathStepsFrom', () => { }, { id: 'whatev-1', - ch: 'P2', + secondary_code: 'P2', trigram: 'GE', name: '87747006', arrival: '15:00:00', @@ -348,7 +348,7 @@ describe('updatePathStepsFrom', () => { }, { id: 'who-0', - ch: 'BV', + secondary_code: 'BV', trigram: 'VPE', name: '87747337', kp: undefined, diff --git a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts index 0c2031eb754..20e20475da0 100644 --- a/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts +++ b/front/src/applications/operationalStudies/hooks/useSetupItineraryForTrainUpdate.ts @@ -46,11 +46,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], diff --git a/front/src/applications/stdcm/components/StdcmForm/StdcmOperationalPoint.tsx b/front/src/applications/stdcm/components/StdcmForm/StdcmOperationalPoint.tsx index 28217bbe623..754307dce29 100644 --- a/front/src/applications/stdcm/components/StdcmForm/StdcmOperationalPoint.tsx +++ b/front/src/applications/stdcm/components/StdcmForm/StdcmOperationalPoint.tsx @@ -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; @@ -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 })); }; @@ -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); diff --git a/front/src/applications/stdcm/components/StdcmResults/SimulationReportSheet.tsx b/front/src/applications/stdcm/components/StdcmResults/SimulationReportSheet.tsx index 0dc2c710117..349fdf4a49c 100644 --- a/front/src/applications/stdcm/components/StdcmResults/SimulationReportSheet.tsx +++ b/front/src/applications/stdcm/components/StdcmResults/SimulationReportSheet.tsx @@ -205,7 +205,9 @@ const SimulationReportSheet = ({ - {step.ch} + + {'secondary_code' in step ? step.secondary_code : undefined} + @@ -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; diff --git a/front/src/applications/stdcm/components/StdcmResults/StdcmResultsTable.tsx b/front/src/applications/stdcm/components/StdcmResults/StdcmResultsTable.tsx index 61509086f78..5670aa812a8 100644 --- a/front/src/applications/stdcm/components/StdcmResults/StdcmResultsTable.tsx +++ b/front/src/applications/stdcm/components/StdcmResults/StdcmResultsTable.tsx @@ -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 = diff --git a/front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx b/front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx index 0b013a5dda1..062994fb3dc 100644 --- a/front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx +++ b/front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx @@ -67,7 +67,9 @@ const Vias = ({ zoomToFeaturePoint, shouldManageStopDuration }: ViasProps) => { {`${via.name || (via.positionOnPath && `KM ${(Math.round(via.positionOnPath) / 1000000).toFixed(3)}`) || t('unavailableDistance')}`} - {via.ch && {via.ch}} + {'secondary_code' in via && via.secondary_code && ( + {via.secondary_code} + )} {'uic' in via && ( {formatUicToCi(via.uic)} diff --git a/front/src/modules/pathfinding/components/Pathfinding/TypeAndPath.tsx b/front/src/modules/pathfinding/components/Pathfinding/TypeAndPath.tsx index 39fb7a78d25..23d167ec608 100644 --- a/front/src/modules/pathfinding/components/Pathfinding/TypeAndPath.tsx +++ b/front/src/modules/pathfinding/components/Pathfinding/TypeAndPath.tsx @@ -167,7 +167,6 @@ const TypeAndPath = ({ setDisplayTypeAndPath }: TypeAndPathProps) => { .filter((op) => op.trigram !== '') .map(({ uic, ch }) => ({ uic, - ch, secondary_code: ch, id: nextId(), })); diff --git a/front/src/modules/pathfinding/helpers/getStepLocation.ts b/front/src/modules/pathfinding/helpers/getStepLocation.ts index 0bae3255ebf..8a653717ba8 100644 --- a/front/src/modules/pathfinding/helpers/getStepLocation.ts +++ b/front/src/modules/pathfinding/helpers/getStepLocation.ts @@ -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; diff --git a/front/src/modules/pathfinding/hooks/usePathfinding.ts b/front/src/modules/pathfinding/hooks/usePathfinding.ts index 27036c2331e..5f1b7200859 100644 --- a/front/src/modules/pathfinding/hooks/usePathfinding.ts +++ b/front/src/modules/pathfinding/hooks/usePathfinding.ts @@ -297,7 +297,7 @@ export const usePathfinding = ( ...(correspondingOp && { name: correspondingOp.name, uic: correspondingOp.uic, - ch: correspondingOp.ch, + secondary_code: correspondingOp.ch, kp: correspondingOp.kp, coordinates: correspondingOp.coordinates, }), diff --git a/front/src/modules/pathfinding/utils.ts b/front/src/modules/pathfinding/utils.ts index f484efc7889..db91fbed8e9 100644 --- a/front/src/modules/pathfinding/utils.ts +++ b/front/src/modules/pathfinding/utils.ts @@ -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; }; diff --git a/front/src/modules/trainschedule/components/ManageTrainSchedule/helpers/__tests__/formatSchedule.spec.ts b/front/src/modules/trainschedule/components/ManageTrainSchedule/helpers/__tests__/formatSchedule.spec.ts index de40cc5c0ce..404af3191de 100644 --- a/front/src/modules/trainschedule/components/ManageTrainSchedule/helpers/__tests__/formatSchedule.spec.ts +++ b/front/src/modules/trainschedule/components/ManageTrainSchedule/helpers/__tests__/formatSchedule.spec.ts @@ -12,7 +12,7 @@ describe('formatSchedule', () => { id: 'id331', deleted: false, uic: 8706, - ch: 'BV', + secondary_code: 'BV', kp: '130+538', name: 'G', positionOnPath: 0, @@ -27,7 +27,7 @@ describe('formatSchedule', () => { id: 'id332', deleted: false, uic: 8737, - ch: 'BV', + secondary_code: 'BV', kp: '117+422', name: 'V', positionOnPath: 13116000, diff --git a/front/src/modules/trainschedule/helpers/computeBasePathSteps.ts b/front/src/modules/trainschedule/helpers/computeBasePathSteps.ts index b50e452854a..03eaa539817 100644 --- a/front/src/modules/trainschedule/helpers/computeBasePathSteps.ts +++ b/front/src/modules/trainschedule/helpers/computeBasePathSteps.ts @@ -1,5 +1,3 @@ -import { omit } from 'lodash'; - import type { TrainScheduleResult } from 'common/api/osrdEditoastApi'; import type { PathStep } from 'reducers/osrdconf/types'; import { mmToM } from 'utils/physics'; @@ -34,12 +32,6 @@ const computeBasePathSteps = ( 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}` : ''); @@ -55,8 +47,8 @@ const computeBasePathSteps = ( } 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, diff --git a/front/src/reducers/osrdconf/helpers.ts b/front/src/reducers/osrdconf/helpers.ts index 58dab6aa211..b103ee80b4d 100644 --- a/front/src/reducers/osrdconf/helpers.ts +++ b/front/src/reducers/osrdconf/helpers.ts @@ -87,7 +87,6 @@ export function upsertPathStep(statePathSteps: (PathStep | null)[], op: Suggeste 'coordinates', 'positionOnPath', 'name', - 'ch', 'kp', 'stopFor', 'arrival', @@ -98,7 +97,7 @@ export function upsertPathStep(statePathSteps: (PathStep | null)[], op: Suggeste ]), id: nextId(), ...(op.uic - ? { uic: op.uic } + ? { uic: op.uic, secondary_code: op.ch } : { track: op.track, offset: op.offsetOnTrack, diff --git a/front/src/reducers/osrdconf/osrdConfCommon/__tests__/utils.ts b/front/src/reducers/osrdconf/osrdConfCommon/__tests__/utils.ts index d3d18475e71..48d974ddec0 100644 --- a/front/src/reducers/osrdconf/osrdConfCommon/__tests__/utils.ts +++ b/front/src/reducers/osrdconf/osrdConfCommon/__tests__/utils.ts @@ -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)); diff --git a/front/src/reducers/osrdconf/types.ts b/front/src/reducers/osrdconf/types.ts index 84383718a6f..47a4114e33c 100644 --- a/front/src/reducers/osrdconf/types.ts +++ b/front/src/reducers/osrdconf/types.ts @@ -81,7 +81,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;