Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the Ctrack Crystal integration from the legacy app.datasource.ctrack module to a new app.datasource.ctrackcrystal package, while refactoring observation pulling to support multi-day catch-up and per-vehicle processed-trip state tracking.
Changes:
- Introduces the new
ctrackcrystalAPI client (models, exceptions, retrying HTTP client) and removes the legacyctrack.py. - Refactors observation pulling to support multi-day catch-up and skipping already-processed trips via stored per-vehicle
processed_trips. - Cleans up action configurations by removing the obsolete
TriggerFetchVehicleObservationsConfigand updates/extends unit tests accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/datasource/ctrackcrystal/client.py | New async httpx client with backoff/retry behavior and endpoint wrappers. |
| app/datasource/ctrackcrystal/models.py | New Pydantic response models for auth/vehicles/trips/trip summary. |
| app/datasource/ctrackcrystal/exceptions.py | New exception hierarchy for mapping HTTP errors and retryable failures. |
| app/datasource/ctrackcrystal/init.py | Exposes the public ctrackcrystal API surface and defines the default base URL. |
| app/datasource/ctrack.py | Removes the legacy monolithic client/models implementation. |
| app/actions/handlers.py | Refactors auth/token retrieval and observation pulling to use ctrackcrystal, adds multi-day catch-up + processed-trip pruning, and updates state/logging. |
| app/actions/configurations.py | Removes the obsolete trigger config and simplifies vehicle-trip config fields. |
| app/cli/ctrackcrystal.py | Updates CLI to use the new ctrackcrystal client and adds batch trips support. |
| app/actions/tests/test_handlers.py | Updates handler tests for new client + async-generator observation flow, adds tests for pruning and processed-trip skipping. |
| app/actions/tests/test_client.py | Migrates tests to ctrackcrystal client functions and updated signatures. |
| app/datasource/tests/test_ctrackcrystal.py | Adds unit tests for new client/models and retry helper behavior. |
| app/datasource/tests/init.py | Adds datasource test package marker. |
Comments suppressed due to low confidence (1)
app/cli/ctrackcrystal.py:31
_get_credentials()acceptsCTRACK_BASE_URLfrom env/CLI, but thectrackcrystalclient expects a base URL without/api(it appends/api/...internally). If a user provides the old style URL ending in/api, requests will become/api/api/.... Consider normalizing the input (e.g., strip a trailing/api) or validating and raising a clearUsageError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| ''' | ||
| Skip trips with an invalid trip ID. | ||
|
|
||
| Skip trips that are already in the processed_trips dictionary. | ||
| ''' | ||
| # Skip trips with an invalid trip ID. |
There was a problem hiding this comment.
The triple-quoted string inside the loop is being executed as a no-op expression on every iteration (it’s not a comment/docstring). Replace it with # comments to avoid unnecessary bytecode and to keep intent clear.
| ''' | |
| Skip trips with an invalid trip ID. | |
| Skip trips that are already in the processed_trips dictionary. | |
| ''' | |
| # Skip trips with an invalid trip ID. | |
| # Skip trips with an invalid trip ID. | |
| # Skip trips that are already in the processed_trips dictionary. |
| for tid, tstr in raw_processed.items(): | ||
| try: | ||
| t = datetime.fromisoformat(tstr).replace(tzinfo=timezone.utc) | ||
| processed_trips[tid] = t | ||
| except (TypeError, ValueError): | ||
| pass |
There was a problem hiding this comment.
When rehydrating processed_trips timestamps from state, datetime.fromisoformat(tstr).replace(tzinfo=timezone.utc) will incorrectly reinterpret any offset-aware timestamps that aren’t already UTC (it discards the offset). Prefer: if parsed datetime is naive, set tzinfo=UTC; if it’s aware, convert via .astimezone(timezone.utc).
| processed_trips: Dict[str, datetime] = {} | ||
| for tid, tstr in raw_processed.items(): | ||
| try: | ||
| t = datetime.fromisoformat(tstr).replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Same timestamp parsing issue here: datetime.fromisoformat(tstr).replace(tzinfo=timezone.utc) discards any existing offset info. Use a conditional naive->UTC replace vs aware->astimezone(timezone.utc) conversion to avoid shifting times if non-UTC values are ever stored.
| t = datetime.fromisoformat(tstr).replace(tzinfo=timezone.utc) | |
| t = datetime.fromisoformat(tstr) | |
| if t.tzinfo is None: | |
| t = t.replace(tzinfo=timezone.utc) | |
| else: | |
| t = t.astimezone(timezone.utc) |
| def transform(observation: client.LocationSummary, vehicle: PullVehicleTripsConfig) -> dict: | ||
| def transform(observation: ctrackcrystal.LocationSummary, vehicle: PullVehicleTripsConfig) -> dict: | ||
| additional_info = { | ||
| key: value for key, value in observation.dict().items() if value and key not in ["eventTime", "latitude", "longitude"] |
There was a problem hiding this comment.
transform() builds additional_info from observation.dict() but excludes the key eventTime. With the new ctrackcrystal.LocationSummary model, the dict key is event_time (snake_case) unless by_alias=True is used, so event_time will be unintentionally included in additional (duplicating recorded_at). Consider excluding event_time (and/or switching to observation.dict(by_alias=True) with a matching exclude list) to keep the payload consistent.
| key: value for key, value in observation.dict().items() if value and key not in ["eventTime", "latitude", "longitude"] | |
| key: value | |
| for key, value in observation.dict().items() | |
| if value and key not in ["eventTime", "event_time", "latitude", "longitude"] |
| vehicles_failed = 0 | ||
| total_observations = 0 | ||
| base_url = integration.base_url or CTC_BASE_URL | ||
| base_url = integration.base_url or ctrackcrystal.BASE_URL | ||
| auth_config = get_auth_config(integration) |
There was a problem hiding this comment.
base_url is now taken from integration.base_url or ctrackcrystal.BASE_URL, but the new ctrackcrystal client appends /api/... internally. If existing integrations/configs store a base URL that already ends with /api (as the previous CTC_BASE_URL did), requests will become /api/api/... and fail. It would be safer to normalize base_url (e.g., strip a trailing /api) or explicitly document/enforce the expected format.
| trips_response = await ctrackcrystal.get_trips( | ||
| base_url, | ||
| token.jwt, | ||
| auth_config.subscription_key.get_secret_value(), | ||
| base_url, | ||
| action_config.vehicle_id, | ||
| action_config.filter_day, | ||
| [action_config.vehicle_id], | ||
| action_config.filter_day.date(), | ||
| ) | ||
| if not trips_response: | ||
| return [], 0 | ||
| logger.warning( | ||
| f"No trips response returned for vehicle {action_config.vehicle_id} on {action_config.filter_day.date()}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
if not trips_response: will almost never be true here because get_trips() returns a Pydantic model instance (truthy) even when empty. This makes the warning/early-return effectively dead and could mask the intended "no trips" path. Consider checking trips_response is None (if possible) and/or if not trips_response.payload: instead.
This pull request refactors and improves the vehicle trip observation pulling logic, focusing on better state management, API integration, and support for multi-day catch-up. The changes remove the old
TriggerFetchVehicleObservationsConfig, switch to a newctrackcrystalAPI client, and introduce more robust handling for processed trips and vehicle states. The new approach allows for skipping already processed trips, supports multi-day fetching, and logs more granular activity.API integration and refactoring:
app.datasource.ctrackclient with the newctrackcrystalmodule, updating API calls, exception handling, and token retrieval logic for improved reliability and maintainability. [1] [2] [3] [4] [5] [6] [7] [8] [9]State management and processed trips:
_prune_processed_tripshelper and new logic for tracking and pruning processed trips per vehicle, ensuring that only trips within a configurable age window are considered for processing. [1] [2]_fetch_one_vehicle_trips_observationsto skip trips already processed and record new processed trips, improving efficiency and preventing duplicate processing.Multi-day catch-up and vehicle state:
Configuration cleanup:
TriggerFetchVehicleObservationsConfigand related fields fromapp/actions/configurations.py, simplifying configuration management.Logging and activity tracking: