Remove deprecated Base classes no longer used#1208
Draft
kaligrafy wants to merge 1 commit into
Draft
Conversation
WalkthroughThis change removes the entire baseObjects layer from evolution-common, deleting classes and types for: BaseAddress, BaseHousehold, BaseInterview, BaseJourney, BaseJunction, BaseOrganization, BasePerson, BasePlace, BaseSegment, BaseTrip, BaseTripChain, BaseVehicle, and BaseVisitedPlace. All related factory methods, validation logic, and unserialize helpers are removed. Corresponding unit tests for each deleted module are also removed. Additionally, baseObjectsAndCompositionDoc.md is updated to a platform-wide documentation describing object composition, named/custom attributes, validation steps, and standardized date/time naming. Possibly related PRs
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (27)
💤 Files with no reviewable changes (26)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/evolution-common/src/services/baseObjects/baseObjectsAndCompositionDoc.md (5)
1-1: Title tweak for clarity and scopeConsider reflecting the composition focus and post-deprecation context.
-# Documentation on Survey Objects (Household, Person, etc.) +# Survey Objects and Composition (Household, Person, Journey, etc.)
6-6: Terminology and casing: “JSON” and “static create method”Align with the factory pattern referenced below and fix casing; tighten wording.
-Classes can have composed objects. For example, a Person can have multiple WorkPlace objects and SchoolPlace objects as composed objects. A Person can also have multiple Journey objects, and each Journey can have VisitedPlace, Trip, and TripChain objects. The Place class can have an Address as a composed object. Composed objects are created automatically if included in the json params used to create the main parent object. The create function will validate params for the parent object and for the composed objects. Composed attributes are ignored in the first part of the parent class constructor, but are then sent to each composed class for creation/validation. +Classes can include composed objects. For example, a Person may include multiple WorkPlace and SchoolPlace objects. A Person can also have multiple Journey objects; each Journey may include VisitedPlace, Trip, and TripChain objects. Place may include Address. Composed objects are created automatically when included in the JSON params used to create the parent object. The static create method validates params for both the parent and its composed objects. During construction, composed-object params are not assigned directly; the factory forwards them to each composed class for creation/validation.
9-9: Minor wording: property name and persistenceSmall clarity tweak.
-Each class has named attributes. These are the attributes that are part of the core object structure, and composed objects have them too. They are stored in the attributes object and are saved into separate columns in the database. +Each class has named attributes. These are part of the core object structure (composed objects have them too). They are stored on the attributes property and persisted in dedicated database columns.
12-12: Capitalize JSON and clarify fieldConsistent casing and wording.
-Each class can have custom attributes. These are attributes that are not part of the named attributes, and are not part of the composed objects (but composed objects themselves can have custom attributes). They are stored in the customAttributes object and are saved into a json field in the database. +Each class can have custom attributes (composed objects can too). These are not part of the named attributes. They are stored on the customAttributes object and saved into a JSON field in the database.
15-15: Accessor wordingUse standard “getters/setters”.
-Each named attribute has accessors (get/set). Custom attributes do not have accessors. It is up to the specific project/survey using them to define accessors when needed. +Each named attribute has getters/setters. Custom attributes do not. Specific projects/surveys can define accessors if needed.packages/evolution-common/src/services/baseObjects/Place.ts (2)
343-345: Strengthen validation: enforce Device allowed values, not just string.Right now any string passes. Consider validating against the actual Device domain (enum/union).
Example change (assuming a DEVICE_VALUES export exists):
- errors.push(...ParamsValidatorUtils.isString('deviceUsed', dirtyParams.deviceUsed, displayName)); + // Ensure deviceUsed matches the Device domain + errors.push( + ...ParamsValidatorUtils.isOneOf('deviceUsed', dirtyParams.deviceUsed, DEVICE_VALUES, displayName) + );If DEVICE_VALUES doesn’t exist, we can add in attributeTypes/InterviewParadataAttributes.ts:
export type Device = 'desktop' | 'tablet' | 'mobile' | 'other'; export const DEVICE_VALUES = ['desktop','tablet','mobile','other'] as const;And import the constant here with a value import alongside the type-only import.
12-12: Use a type-only import for Device
Device is exported as a type in attributeTypes/InterviewParadataAttributes.ts (line 13), so switch to a type import to avoid any runtime import under TS5’s defaults.-import { Device } from './attributeTypes/InterviewParadataAttributes'; +import type { Device } from './attributeTypes/InterviewParadataAttributes';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/evolution-common/src/services/baseObjects/BaseAddress.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseHousehold.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseInterview.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseJourney.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseJunction.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseOrganization.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BasePerson.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BasePlace.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseSegment.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseTrip.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseTripChain.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseVehicle.ts(0 hunks)packages/evolution-common/src/services/baseObjects/BaseVisitedPlace.ts(0 hunks)packages/evolution-common/src/services/baseObjects/Place.ts(1 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseAddress.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseHousehold.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseInterview.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseJourney.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseJunction.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseOrganization.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BasePerson.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BasePlace.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseSegment.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseTrip.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseTripChain.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseVehicle.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/__tests__/BaseVisitedPlace.test.ts(0 hunks)packages/evolution-common/src/services/baseObjects/baseObjectsAndCompositionDoc.md(1 hunks)
💤 Files with no reviewable changes (26)
- packages/evolution-common/src/services/baseObjects/tests/BaseAddress.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BasePlace.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseJourney.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseVisitedPlace.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseOrganization.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseSegment.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseVehicle.test.ts
- packages/evolution-common/src/services/baseObjects/BaseJunction.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseTripChain.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseHousehold.test.ts
- packages/evolution-common/src/services/baseObjects/BasePerson.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseJunction.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseInterview.test.ts
- packages/evolution-common/src/services/baseObjects/tests/BasePerson.test.ts
- packages/evolution-common/src/services/baseObjects/BasePlace.ts
- packages/evolution-common/src/services/baseObjects/BaseTrip.ts
- packages/evolution-common/src/services/baseObjects/BaseSegment.ts
- packages/evolution-common/src/services/baseObjects/tests/BaseTrip.test.ts
- packages/evolution-common/src/services/baseObjects/BaseVisitedPlace.ts
- packages/evolution-common/src/services/baseObjects/BaseHousehold.ts
- packages/evolution-common/src/services/baseObjects/BaseVehicle.ts
- packages/evolution-common/src/services/baseObjects/BaseOrganization.ts
- packages/evolution-common/src/services/baseObjects/BaseTripChain.ts
- packages/evolution-common/src/services/baseObjects/BaseInterview.ts
- packages/evolution-common/src/services/baseObjects/BaseAddress.ts
- packages/evolution-common/src/services/baseObjects/BaseJourney.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: generate-and-build-generator (22.x)
- GitHub Check: test-sequential (22.x)
- GitHub Check: pr-build-check
- GitHub Check: build-and-test (22.x)
🔇 Additional comments (1)
packages/evolution-common/src/services/baseObjects/baseObjectsAndCompositionDoc.md (1)
3-3: LGTMClear purpose and scope.
|
|
||
| ## About naming dates and times | ||
| The first method of naming dates and times in the previous base objecst was to use departureDate, departureTime, arrivalDate and arrivalTime. The main issue with this is that for instance, for a visitedPlace the arrival would be at the begignning and the departure would be at the end, while for a trip it would be the opposite and this created ambiguities. The solution is now to use startDate, startTime, endDate, endTime, and thus the order is always the same for all timed objects like VisitedPlace, Trip, Junction, Segment, Journey, etc. | ||
| The current approach for naming dates and times uses a consistent pattern: startDate, startTime, endDate, endTime. This eliminates ambiguities that existed with previous naming conventions (departureDate/arrivalDate). The order is always the same for all timed objects like VisitedPlace, Trip, Junction, Segment, Journey, etc., where start represents the beginning of the activity/period and end represents the conclusion. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add time zone and format guidance; verify object names exist post-deprecation
Document UTC/ISO-8601 to avoid ambiguity. Also verify that all referenced classes (VisitedPlace, TripChain, Junction, Segment, WorkPlace, SchoolPlace) remain valid after removing Base* modules.
-The current approach for naming dates and times uses a consistent pattern: startDate, startTime, endDate, endTime. This eliminates ambiguities that existed with previous naming conventions (departureDate/arrivalDate). The order is always the same for all timed objects like VisitedPlace, Trip, Junction, Segment, Journey, etc., where start represents the beginning of the activity/period and end represents the conclusion.
+The current approach for naming dates and times uses a consistent pattern: startDate, startTime, endDate, endTime. This eliminates ambiguities that existed with previous naming conventions (departureDate/arrivalDate). The order is always the same for all timed objects like VisitedPlace, Trip, Junction, Segment, Journey, etc., where start represents the beginning of the activity/period and end represents the conclusion. All timestamps should use UTC and ISO‑8601 (e.g., 2025-09-08T18:52:27Z); date-only fields use YYYY‑MM‑DD.Run to verify symbol presence:
🏁 Script executed:
#!/bin/bash
# Verify referenced classes/types still exist after Base* removals
rg -nP -C2 --type=ts '\b(VisitedPlace|TripChain|Trip|Junction|Segment|Journey|WorkPlace|SchoolPlace|Place|Address)\b' \
| sed -n '1,300p'Length of output: 30977
Fix documentation references and add format guidance
- Remove stale references to TripChain, WorkPlace and SchoolPlace – these types aren’t present in the codebase (e.g. packages/evolution-common/src/services/baseObjects/baseObjectsAndCompositionDoc.md:21).
- Append: all timestamps must use UTC and ISO 8601 (e.g. 2025-09-08T18:52:27Z); date-only fields use YYYY-MM-DD.
🤖 Prompt for AI Agents
In
packages/evolution-common/src/services/baseObjects/baseObjectsAndCompositionDoc.md
around line 21, remove any stale references to TripChain, WorkPlace, and
SchoolPlace (they do not exist in the codebase) and update the wording to
reference only the actual timed object types (VisitedPlace, Trip, Junction,
Segment, Journey, etc.); also append a short format guidance sentence stating
that all timestamps must be in UTC using ISO 8601 with a trailing Z (e.g.
2025-09-08T18:52:27Z) and that date-only fields must use YYYY-MM-DD.
tahini
requested changes
Sep 8, 2025
Contributor
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Bien que je sois toujours heureuse d'enlever des fichiers, je me demande si c'est correct de faire ceci dans la version d'Evolution utilisée pour l'enquête nationale en cours ou si on est rendus à figer une version "prod". Je mets "Request changes" pour éviter que ce soit mergé avant qu'on en discute sérieusement. Mais il n'y a à priori aucun changement à faire.
These classes are not used anymore in the actual and future surveys. Also update documentation to reflect changes. Move BaseInterview old Device import to new InterviewParadataAttributes
kaligrafy
force-pushed
the
removeDeprecatedBasClasses
branch
from
c6203c7 to
dd317c0
Compare
Contributor
Je te dirais que c'est pas mal la dernière semaine que od_nationale devrait vivre avec prod. Pas mal le temps de brancher out pour juste faire des bug fix. A moins que vous vous entendiez à ne rien faire "out of scope" |
Contributor
Author
|
Moi, je vais travailler que dans l'admin et dans evolution, mias ça va prendre du temps... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These classes are not used anymore in the actual and future surveys.
Also update documentation to reflect changes.
Move BaseInterview old Device import to new InterviewParadataAttributes
Summary by CodeRabbit
Refactor
Tests
Documentation