Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

front: add, get and delete paced trains #10966

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

SharglutDev
Copy link
Contributor

@SharglutDev SharglutDev commented Feb 26, 2025

See commits (will be merged all together)

part of #10615

Note

PR is now testable !

As some logic from train schedule has been reused for the pace train mode, be sure to check that all previous behavior when paced train mode is off are still working (add/edit/get/delete, duplicate, import/export, nge, drag, selecting/projecting a train, stdcm)

E2E Tests :

@SharglutDev SharglutDev requested a review from a team as a code owner February 26, 2025 15:13
@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Feb 26, 2025
@SharglutDev SharglutDev marked this pull request as draft February 26, 2025 15:14
@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 96.69261% with 34 lines in your changes missing coverage. Please review.

Project coverage is 80.90%. Comparing base (ebb9c47) to head (362f2b0).

Files with missing lines Patch % Lines
front/src/common/api/osrdEditoastApi.ts 16.00% 21 Missing ⚠️
front/src/reducers/index.ts 12.50% 7 Missing ⚠️
...ents/Timetable/PacedTrain/hooks/useOccurrences.tsx 0.00% 4 Missing ⚠️
front/src/utils/trainId.ts 33.33% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10966      +/-   ##
==========================================
+ Coverage   80.86%   80.90%   +0.03%     
==========================================
  Files        1118     1119       +1     
  Lines      112736   113119     +383     
  Branches      759      759              
==========================================
+ Hits        91164    91517     +353     
- Misses      21517    21547      +30     
  Partials       55       55              
Flag Coverage Δ
editoast 72.61% <ø> (ø)
front 89.90% <96.69%> (+0.01%) ⬆️
gateway 2.49% <ø> (ø)
osrdyne 2.53% <ø> (ø)
railjson_generator 87.58% <ø> (ø)
tests 88.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SharglutDev SharglutDev self-assigned this Feb 26, 2025
@SharglutDev SharglutDev changed the title front: handle add paced trains add, get and delete paced trains Feb 26, 2025
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch 10 times, most recently from 5cf88dd to 77ebbd0 Compare March 7, 2025 09:21
@emersion emersion self-requested a review March 7, 2025 10:25
@emersion emersion changed the title add, get and delete paced trains front: add, get and delete paced trains Mar 7, 2025
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch from 77ebbd0 to 9d5a8e6 Compare March 7, 2025 10:37
@SharglutDev SharglutDev requested a review from RomainValls March 7, 2025 10:38
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the first two commits! Will continue the review later.

@emersion emersion self-requested a review March 7, 2025 16:05
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch 2 times, most recently from d37c225 to 9f154eb Compare March 10, 2025 12:43
@SharglutDev SharglutDev requested a review from Maymanaf March 10, 2025 12:45
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch 3 times, most recently from 46934e7 to b6811f7 Compare March 10, 2025 17:01
Comment on lines 155 to 166
const trainScheduleIds = useMemo(
() => (fetchedTrainSchedulesResults || []).map((t) => t.id),
[fetchedTrainSchedulesResults]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const trainScheduleIds = useMemo(
() => (fetchedTrainSchedulesResults || []).map((t) => t.id),
[fetchedTrainSchedulesResults]
);
const trainScheduleIds = useMemo(
() => (fetchedTrainSchedulesResults || []).map((trainSchedule) => trainSchedule.id),
[fetchedTrainSchedulesResults]
);

@SharglutDev SharglutDev requested a review from clarani March 11, 2025 08:33
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another round of reviews :P

@@ -11,6 +17,10 @@ export const isOccurrence = (id: string): id is OccurrenceId => {
export const isTrainSchedule = (id: string): id is TrainScheduleId =>
id.startsWith('trainschedule-');

export const isPacedTrainResultWithPacedTrainId = (
timetableITem: TimetableItemWithTimetableId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "timetableItem"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥷

@@ -11,6 +17,10 @@ export const isOccurrence = (id: string): id is OccurrenceId => {
export const isTrainSchedule = (id: string): id is TrainScheduleId =>
id.startsWith('trainschedule-');

export const isPacedTrainResultWithPacedTrainId = (
timetableITem: TimetableItemWithTimetableId
): timetableITem is PacedTrainResultWithPacedTrainId => 'paced' in timetableITem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd slightly prefer to check isPacedTrain(timetableItem.id) here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

duration: Duration.parse(formattedItem.paced.duration),
step: Duration.parse(formattedItem.paced.step),
},
} as PacedTrainWithDetails;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of casting, could we set the type of the variable?

const formattedPacedTrains: PacedTrainWithDetails = 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, done.

} as PacedTrainWithDetails;
acc[1].push(formattedPacedTrains);
} else {
acc[0].push(formattedItem as TrainScheduleWithDetails);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cast really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, you're right.

const result: PacedTrainResult[] = [];
while (!reachEnd) {
const promise = dispatch(
// TODO : update the endpoint with the proper one when back is ready
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this TODO can be dropped now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

...otherProps,
};

if (isPacedTrainResultWithPacedTrainId(formattedItem)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes a partially constructed train result to the function. In particular, formattedItem will not yet have a paced field, so the function will always return false here?

Seems safer to check the timetableItem.id instead of operating on an incomplete value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change in this comment, it should be fine I think ?

);

const [trainScheduleWithDetails, pacedTrainWithDetails] = relevantTimetableItems.reduce<
[TrainScheduleWithDetails[], PacedTrainWithDetails[]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reduce() is tricky to read for me. I would prefer a for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥲

Comment on lines 22 to 25
* This function formats the timetable items with their simulation summaries.
* Mapping the simulation results to the timetable items needs to be done in a second part
* because otherwise typescript doesn't understand that a PacedTrainWithDetails will necessarily
* be mapped with a PacedTrainId and not a TrainScheduleId (same for TrainScheduleWithDetails)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, curious. The following snippet works for me:

const formatTimetableItemSummaries = (
  timetableItemIds: TimetableItemId[],
  rawSummaries: Map<TimetableItemId, SimulationSummaryResult>,
  rawTimetableItems: Map<TimetableItemId, TimetableItemWithTimetableId>,
  rollingStocks: LightRollingStockWithLiveries[]
): Map<TimetableItemId, TimetableItemWithDetails> => {
  const relevantTimetableItems = compact(
    timetableItemIds.map((timetableItemId) => rawTimetableItems.get(timetableItemId))
  );

  const items = relevantTimetableItems.map((timetableItem): TimetableItemWithDetails | null => {
    const rollingStock = rollingStocks.find((rs) => rs.name === timetableItem.rolling_stock_name);

    const timetableItemSummary = rawSummaries.get(timetableItem.id);

    if (!timetableItemSummary) return null;

    let notHonoredReason: TimetableItemWithDetails['notHonoredReason'];
    if (timetableItemSummary.status === 'success') {
      if (isTooFast(timetableItem, timetableItemSummary)) notHonoredReason = 'trainTooFast';
      if (isScheduledPointsNotHonored(timetableItem, timetableItemSummary))
        notHonoredReason = 'scheduleNotHonored';
    }

    const startTime = new Date(timetableItem.start_time);

    const otherProps =
      timetableItemSummary.status === 'success'
        ? {
            isValid: true,
            arrivalTime: new Date(startTime.getTime() + timetableItemSummary.time),
            duration: new Duration({ milliseconds: timetableItemSummary.time }),
            pathLength: formatKmValue(timetableItemSummary.length, 'millimeters', 1),
            mechanicalEnergyConsumed: jouleToKwh(timetableItemSummary.energy_consumption, true),
            pathItemTimes: {
              base: timetableItemSummary.path_item_times_base,
              provisional: timetableItemSummary.path_item_times_provisional,
              final: timetableItemSummary.path_item_times_final,
            },
          }
        : {
            isValid: false,
            arrivalTime: null,
            duration: null,
            pathLength: '',
            mechanicalEnergyConsumed: 0,
            invalidReason:
              timetableItemSummary.status === 'pathfinding_not_found' ||
              timetableItemSummary.status === 'pathfinding_input_error'
                ? timetableItemSummary.error_type
                : timetableItemSummary.status,
          };

    const formattedItem = {
      ...timetableItem,
      name: timetableItem.train_name,
      startTime,
      stopsCount:
        (timetableItem.schedule?.filter(
          (step) => step.stop_for && Duration.parse(step.stop_for).ms > 0
        ).length ?? 0) + 1, // +1 to take the final stop (destination) into account
      speedLimitTag: timetableItem.speed_limit_tag ?? null,
      labels: timetableItem.labels ?? [],
      rollingStock,
      scheduledPointsNotHonored: notHonoredReason !== undefined,
      notHonoredReason,
      ...otherProps,
    };

    if (isPacedTrainResultWithPacedTrainId(formattedItem)) {
      return {
        ...formattedItem,
        paced: {
          duration: Duration.parse(formattedItem.paced.duration),
          step: Duration.parse(formattedItem.paced.step),
        },
      };
    }
    return formattedItem;
  });

  return mapBy(compact(items), 'id');
};

Does this map() approach not work on your machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes you're right. Not sure what I did before that made typescript complain.

[[], []]
);

const rawTrainScheduleSummaries = await postTrainScheduleSimulationSummary({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would be slightly nicer to fetch these in parallel, without waiting for the train schedule request to complete before sending the paced train request.

const trainSchedulesPromise = postTrainScheduleSimulationSummary();
const pacedTrainsPromise = postPacedTrainSimulationSummary();

const rawTrainScheduleSummaries = await trainSchedulesPromise;
const rawPacedTrainSummaries = await pacedTrainsPromise;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const rawTrainScheduleSummaries = await postTrainScheduleSimulationSummary({
body: {
infra_id: _infraId,
ids: editoastTrainScheduleIds,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list can be empty, would be nicer to skip sending an HTTP request when there is nothing to fetch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@emersion emersion self-requested a review March 11, 2025 15:28
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch from c1396d9 to d1851e9 Compare March 12, 2025 10:08
Comment on lines 199 to 206
setProjectedTrainsById((prev) => {
const newProjectedTrainsById = new Map(prev);
trainSchedulesToUpsert.forEach((trainSchedule) => {
timetableItemsToUpsert.forEach((trainSchedule) => {
newProjectedTrainsById.delete(trainSchedule.id);
});
return newProjectedTrainsById;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      setProjectedItemsById((prev) => {
        const newProjectedItemsById = new Map(prev);
        timetableItemsToUpsert.forEach((timetableItem) => {
          newProjectedTrainsById.delete(timetableItem.id);
        });
        return newProjectedItemsById;
      });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept trainSchedule inside for now to remember that we manipulate only train schedule and not paced train. I planned that we would change this when dealing with projection in the related issue. Is that ok for you ?

Copy link
Contributor

@Maymanaf Maymanaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job !
Left some comments

await this.verifyOccurrencesCount(totalOccurrences, index);

await expect(this.hideOccurrencesButton.nth(index)).not.toBeVisible();
await expect(pacedTrainItem.locator('.toggle-icon')).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use data-testid

Copy link
Contributor Author

@SharglutDev SharglutDev Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why but with the data-testid, playwright doesn't find the Locator 🤔

Comment on lines 434 to 418
const formattedDuration = dayjs.duration(pacedTrainDuration).asMinutes();
const formattedStep = dayjs.duration(step).asMinutes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to use the values directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 449 to 434
await expect(pacedTrainCadenceLocator).toHaveText(
`${String.fromCodePoint(0x2014)} ${formattedStep}min`
); // UI format: "- 20min"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can directly check for 20min

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm the displayed text is "- 20min" so I have to deal with the special character no ? Or you want me to test a .toEqual('20min') and pass this hardcoded value when calling the function ?

Copy link
Contributor

@Maymanaf Maymanaf Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toHaveText() check if 20min is in the text so the format is not needed
If you keep the format change it to toEqual()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like he wants the exact match :
Capture d’écran 2025-03-14 à 16 00 09

}

async clickOnEditTrainSchedule() {
await this.editTrainScheduleButton.click();
await this.closeToastNotification();
}

async verifyPacedTrainItemDetails(
pacedTrainData: Pick<PacedTrainBase, 'train_name' | 'start_time' | 'labels' | 'paced'>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can avoid using the project types and create a new type for the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} = pacedTrainData;

const pacedTrainItem = this.timetableTrains.nth(pacedTrainIndex);
const occurrenceItem = pacedTrainItem.getByTestId('occurrence-item').nth(occurrenceIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const occurrenceItem = pacedTrainItem.getByTestId('occurrence-item').nth(occurrenceIndex);
getOccurrenceItem(occurrenceIndex: number): Locator {
return this.page.getByTestId('occurrence-item').nth(occurrenceIndex);
}

Comment on lines 507 to 483
await occurrenceItem.hover();
await expect(occurrenceItem.getByTestId('project-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('duplicate-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('edit-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('delete-item')).not.toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await occurrenceItem.hover();
await expect(occurrenceItem.getByTestId('project-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('duplicate-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('edit-item')).not.toBeVisible();
await expect(occurrenceItem.getByTestId('delete-item')).not.toBeVisible();
getOccurrenceItem(occurrenceIndex: number): Locator {
return this.page.getByTestId('occurrence-item').nth(occurrenceIndex);
}
async getActionButtonsLocators(occurrenceIndex: number): Record<string, Locator> {
const occurrenceItem = this.getOccurrenceItem(occurrenceIndex);
await occurrenceItem.hover();
return {
projectItem: occurrenceItem.getByTestId('project-item'),
duplicateItem: occurrenceItem.getByTestId('duplicate-item'),
editItem: occurrenceItem.getByTestId('edit-item'),
deleteItem: occurrenceItem.getByTestId('delete-item'),
};
}
async verifyItemsNotVisible(occurrenceIndex: number): Promise<void> {
const actionButtonsLocators = this.getActionButtonsLocators(occurrenceIndex);
await Promise.all(
Object.values(actionButtonsLocators).map(locator => expect(locator).not.toBeVisible())
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getOccurrenceItem(occurrenceIndex: number): Locator {
return this.page.getByTestId('occurrence-item').nth(occurrenceIndex);
}

This won't work because even if they aren't displayed, occurence items still exist in the DOM, so doing page.get... will always return the first group of occurrence. We need to locate them relatively to the current paced train we are testing.

Also regarding this comment, It seems like you want me to fetch occurrenceItem only once but by calling it inside the getActionButtonsLocators method (and the one I'll be creating for name, startTime etc) means we will call it multiple times.

So instead of calling getOccurrenceItem in each method I could pass the current occurrence Locator we are looping on that we would fetch only once at the beginning of verifyOccurrenceDetails.

But if I do that (for example with the name)

  async verifyOccurrenceName(currentPacedTrainLocator: Locator, expectedName: string) {
    const occurrenceNameLocator = currentPacedTrainLocator.locator('.occurrence-item-name');
    await expect(occurrenceNameLocator).toHaveText(expectedName);
  }

I have this eslint warning for each method : Expected 'this' to be used by class async method 'verifyOccurrenceName'.

So best solution seems to be the actual one. Or do you think of something else ?

Comment on lines 25 to 26
duration: 'PT90M',
step: 'PT20M',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It better to hard code the values 90 and 20

Comment on lines 220 to 223
const formattedTimeRangeDuration = dayjs
.duration(DEFAULT_PACED_TRAIN_SETTINGS.paced.duration)
.asMinutes()
.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use hardcoded values

Comment on lines 227 to 230
const formattedCadence = dayjs
.duration(DEFAULT_PACED_TRAIN_SETTINGS.paced.step)
.asMinutes()
.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same use hardcoded values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because multiple locators are scoped within occurrenceItem, you can move all of them in a new page model paced-train-section.ts
and it's best to store them in a reusable function so that you only fetch occurrenceItem once and reuse it

@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch 2 times, most recently from 18590e6 to ce6ee25 Compare March 13, 2025 14:50
@@ -103,10 +103,12 @@ const ScenarioContent = ({
timeTableId: scenario.timetable_id,
netzgrafikDto,
addUpsertedTrainSchedules: (upsertedTrainSchedules: TrainScheduleResultWithTrainId[]) => {
upsertTrainSchedules(upsertedTrainSchedules);
// TODO Paced trains : update this in https://github.com/OpenRailAssociation/osrd/issues/10612
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the 2 comments, since there is nothing to change here (everything will by in the functions ;) )

Suggested change
// TODO Paced trains : update this in https://github.com/OpenRailAssociation/osrd/issues/10612

);

const [trainScheduleWithDetails, pacedTrainWithDetails] = relevantTimetableItems.reduce<
[TrainScheduleWithDetails[], PacedTrainWithDetails[]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥲

@@ -55,6 +59,7 @@ const SimulationResults = ({
infraId,
projectionData,
trainScheduleSummaries,
// timetableItemsWithDetails,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// timetableItemsWithDetails,

{ forceRefetch: true, subscribe: false }
)
);
// need to unsubscribe on get call to avoid cache issue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm have you unsubscribe here ? Or it is not necessary ?

if (validTrainConfig) {
const { timetableId, firstStartTime, trainCount, trainDelta, trainStep, baseTrainName } =
validTrainConfig;
const { timetableId, firstStartTime, trainCount, trainDelta, trainStep, baseTrainName } =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should setIsWorking(true) here

Comment on lines +73 to +74
if (showPacedTrains) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the try above the test showPacedTrains

Suggested change
if (showPacedTrains) {
try {
try {
if (showPacedTrains) {
...

This extra useEffect introduced a bug in end to end tests. The only way to reproduce it was to set the train name and start time in less that 500ms (the debounce used for these fields) which the e2e tests do.
At this speed, after filling the name, a useEffect is fired after 500ms after to update the store, but since the test is also filling the start time a few ms later, the useEffect was called because nameFromStore had been updated resulting in setting the start time back at its original value.
A workaround had been done for tests 011 and 012 to insert the selection of the rolling stock between the name and start time filling.

Signed-off-by: SharglutDev <[email protected]>
@SharglutDev SharglutDev force-pushed the pfn/front/add-get-delete-paced-trains branch from ce6ee25 to 362f2b0 Compare March 14, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants