refactor(person-factory): align Person.create call with other factories by kaligrafy · Pull Request #1745 · chairemobilite/evolution

kaligrafy · 2026-07-23T14:24:08Z

Summary

PersonFactory was using an old create pattern, we need to update it to be consistent with other base objects factories.

Omit nested journeys before calling Person.create, since journeys are built separately by JourneyFactory
Cast person attributes to ExtendedPersonAttributes, matching the pattern used in JourneyFactory and TripFactory
Add a unit test verifying journeys are not passed to Person.create

Test plan

yarn test --testPathPatterns='PersonFactory.test.ts' passes (7 tests)

Summary by CodeRabbit

Bug Fixes
- Improved survey object creation by populating each person’s journeys after successful creation.
- Ensured work and school locations are configured after journey population.
- Prevented follow-up setup when person creation fails, reducing incomplete survey data.
Tests
- Expanded coverage for person creation, journey population, and work/school location setup.
- Updated household creation tests to verify the revised processing flow.

greenscientist

Please explain why you need to omit nested journeys. (At least in the commit message, but it probably need a comment in the code also.

coderabbitai · 2026-07-29T15:38:40Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d6b42576-0a3e-4b85-93d0-557ed01c823e

📥 Commits

Reviewing files that changed from the base of the PR and between 4b421f4 and ec4d61f.

📒 Files selected for processing (5)

packages/evolution-backend/src/services/audits/__tests__/SurveyObjectParsers.test.ts
packages/evolution-backend/src/services/surveyObjects/PersonFactory.ts
packages/evolution-backend/src/services/surveyObjects/SurveyObjectsFactory.ts
packages/evolution-backend/src/services/surveyObjects/__tests__/PersonFactory.test.ts
packages/evolution-backend/src/services/surveyObjects/__tests__/SurveyObjectsFactory.test.ts

📜 Recent review details

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2026-03-31T18:20:32.322Z

Learnt from: kaligrafy
Repo: chairemobilite/evolution PR: 1462
File: packages/evolution-common/src/services/baseObjects/Completable.ts:1-6
Timestamp: 2026-03-31T18:20:32.322Z
Learning: In this repository (chairemobilite/evolution), do not flag or require a copyright year in file headers. For new files, the header format `Copyright Polytechnique Montreal and contributors` (without a year) is acceptable. Only verify the header presence/consistency, not the presence of the year.

Applied to files:

packages/evolution-backend/src/services/audits/__tests__/SurveyObjectParsers.test.ts
packages/evolution-backend/src/services/surveyObjects/SurveyObjectsFactory.ts
packages/evolution-backend/src/services/surveyObjects/PersonFactory.ts
packages/evolution-backend/src/services/surveyObjects/__tests__/PersonFactory.test.ts
packages/evolution-backend/src/services/surveyObjects/__tests__/SurveyObjectsFactory.test.ts

🔇 Additional comments (5)

packages/evolution-backend/src/services/surveyObjects/PersonFactory.ts (1)

8-36: LGTM!

Also applies to: 67-90

packages/evolution-backend/src/services/surveyObjects/__tests__/PersonFactory.test.ts (1)

10-10: LGTM!

Also applies to: 24-28, 86-86, 95-110, 146-263, 289-293, 303-311, 338-342

packages/evolution-backend/src/services/audits/__tests__/SurveyObjectParsers.test.ts (1)

189-189: LGTM!

packages/evolution-backend/src/services/surveyObjects/SurveyObjectsFactory.ts (1)

178-186: LGTM!

packages/evolution-backend/src/services/surveyObjects/__tests__/SurveyObjectsFactory.test.ts (1)

113-113: LGTM!

Also applies to: 178-178, 267-317

Walkthrough

populatePersonsForHousehold now accepts home context, creates persons without journeys, and populates journeys and work/school places after successful creation. SurveyObjectsFactory delegates this processing instead of iterating over household members directly. Tests cover the updated signature, delegation, side effects, and failure behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

chairemobilite/evolution#1744: Both changes modify PersonFactory.ts person-population ordering and journey generation.
chairemobilite/evolution#1762: Both changes use the updated populatePersonsForHousehold signature and journey-population refactor.

Suggested reviewers: greenscientist, tahini

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: updating the Person.create call to match the pattern used by other factories.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

kaligrafy · 2026-07-29T15:57:07Z

Please explain why you need to omit nested journeys. (At least in the commit message, but it probably need a comment in the code also.

Ajouté au commit message

tahini · 2026-07-29T15:59:58Z


-            // Verify person and journey factories were called
+            // Verify person factory was called (journeys/setupWorkAndSchoolPlaces are now
+            // triggered from within populatePersonsForHousehold, covered by PersonFactory.test.ts)


On n'a pas besoin de savoir ça, "are now triggered", now par rapport à quoi? Ceci est un commentaire justifiant le changement (genre dans le commit message), ça ne va pas dans le code

tahini · 2026-07-29T16:04:31Z

+        // Journey.create in JourneyFactory: it is a nested child collection, not a Person
+        // attribute. Once the person itself is successfully created, populateJourneysForPerson
+        // builds each journey individually so that per-journey errors are tracked separately
+        // instead of failing the whole Person.create call.


Svp, revoir les commentaires écrits par Claude, souvent, ils ne sont pas pertinents dans le code et ne font qu'expliquer pouquoi le changement a été fait. Il faut lire ce commentaire du point du vue de la personne qui va lire le code et vouloir le modifier dans 6 mois. Ici, simplement // Omit journeys as they will be populated separately in the next step est suffisant, pas besoin de comparer avec le visitedPlaces/trips car ça veut dire que le commentaire ici dépend de l'implémentation là-bas, il ne faut pas.

@kaligrafy

PersonFactory was using an old create pattern, we need to update it to be consistent with other base objects factories. Omit nested journeys before Person.create and cast to ExtendedPersonAttributes, matching the JourneyFactory pattern for child objects built separately. Why omit journeys: journeys are a nested child collection stored on the person's attributes, not a Person constructor input. Just like visitedPlaces/trips are omitted before Journey.create in JourneyFactory, journeys must be created individually (via populateJourneysForPerson) once the person itself exists, so that errors for each journey are tracked separately instead of failing the whole Person.create call. Also move the populateJourneysForPerson + setupWorkAndSchoolPlaces calls from SurveyObjectsFactory into populatePersonsForHousehold, right after a person is successfully created. This matches the pattern already used by JourneyFactory, which calls populateVisitedPlacesForJourney and populateTripsForJourney directly in its own isOk block instead of leaving that orchestration to the caller. Code written by Cursor Composer 2.5 and reviewed by @kaligrafy

github-actions · 2026-07-31T11:10:18Z

@coderabbitai review

coderabbitai · 2026-07-31T11:10:30Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

greenscientist

I guess I'll accept.
I feel that the omitting should be handled by the create function directly, maybe we some flags passed to create to tell it to omit certain part.
Or maybe we should pass new journeys directly to that create function.
Just some thoughts for your considerations.

kaligrafy · 2026-07-31T20:12:21Z

See #1774 1774

kaligrafy requested a review from greenscientist July 23, 2026 14:25

kaligrafy force-pushed the refactor/person-factory-create-pattern branch from 3b3d76a to 5c2f67a Compare July 23, 2026 14:26

chairemobilite deleted a comment from coderabbitai Bot Jul 23, 2026

greenscientist requested changes Jul 24, 2026

View reviewed changes

tahini requested changes Jul 29, 2026

View reviewed changes

Comment thread packages/evolution-backend/src/services/surveyObjects/PersonFactory.ts

kaligrafy force-pushed the refactor/person-factory-create-pattern branch from 5c2f67a to 8d8ac14 Compare July 29, 2026 15:38

kaligrafy requested review from greenscientist and tahini July 29, 2026 15:55

kaligrafy force-pushed the refactor/person-factory-create-pattern branch from 8d8ac14 to 40b5baa Compare July 29, 2026 15:57

tahini reviewed Jul 29, 2026

View reviewed changes

kaligrafy force-pushed the refactor/person-factory-create-pattern branch from 40b5baa to 4b421f4 Compare July 29, 2026 20:17

kaligrafy requested a review from tahini July 29, 2026 20:18

tahini approved these changes Jul 30, 2026

View reviewed changes

kaligrafy force-pushed the refactor/person-factory-create-pattern branch 2 times, most recently from ebf9609 to 799fd89 Compare July 30, 2026 19:36

kaligrafy force-pushed the refactor/person-factory-create-pattern branch from 799fd89 to ec4d61f Compare July 30, 2026 19:47

coderabbitai Bot mentioned this pull request Jul 30, 2026

Issue1719 #1762

Merged

greenscientist approved these changes Jul 31, 2026

View reviewed changes

kaligrafy merged commit 7e87fc6 into chairemobilite:main Jul 31, 2026
7 checks passed

kaligrafy deleted the refactor/person-factory-create-pattern branch July 31, 2026 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(person-factory): align Person.create call with other factories - #1745

refactor(person-factory): align Person.create call with other factories#1745
kaligrafy merged 1 commit into
chairemobilite:mainfrom
kaligrafy:refactor/person-factory-create-pattern

kaligrafy commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

greenscientist left a comment

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

kaligrafy commented Jul 29, 2026

Uh oh!

tahini Jul 29, 2026

Uh oh!

tahini Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

greenscientist left a comment

Uh oh!

kaligrafy commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kaligrafy commented Jul 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

greenscientist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

kaligrafy commented Jul 29, 2026

Uh oh!

tahini Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

tahini Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

coderabbitai Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenscientist left a comment

Choose a reason for hiding this comment

Uh oh!

kaligrafy commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaligrafy commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading