chore: refactor transfer functions to improve point ID handling and logging#518
chore: refactor transfer functions to improve point ID handling and logging#518
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b46439474
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not field_event_participants: | ||
| stats["rows_skipped_contacts"] += 1 | ||
| logger.warning( | ||
| "Skipping %s because no field event participants were found", | ||
| self._row_context(row), | ||
| ) | ||
| continue |
There was a problem hiding this comment.
This branch skips rows with no participants, but field_event has already been added to the session earlier in the same iteration, so the later group commit still persists a standalone FieldEvent without participants/activity/sample/observation. This is now reachable for rows with missing/invalid MeasuredBy or owner fallback with no thing contacts, and it silently creates inconsistent transfer data despite logging that the row was skipped.
Useful? React with 👍 / 👎.
| len(group), | ||
| ) | ||
|
|
||
| thing = session.query(Thing).where(Thing.name == pointid).one_or_none() |
There was a problem hiding this comment.
Using .one_or_none() here will raise MultipleResultsFound if more than one Thing shares the same name, and that exception is not caught in this loop, so one duplicate can abort the whole water-level transfer. The model does not enforce Thing.name uniqueness (see db/thing.py), so this is a real runtime regression from the previous .first() behavior unless duplicates are explicitly handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the water levels transfer functionality to improve error handling, logging, and point ID processing. The changes introduce per-group database commits instead of a single commit at the end, comprehensive statistics tracking throughout the transfer process, and better error context reporting.
Changes:
- Introduced detailed statistics tracking for water levels transfer operations (groups processed, rows created/skipped, entities created)
- Modified commit strategy to commit per PointID group rather than once at the end, with comprehensive exception handling for DatabaseError, SQLAlchemyError, and generic exceptions
- Added helper methods
_row_context()and_log_transfer_summary()to improve logging and debugging capabilities - Extracted
_get_test_pointids()function in transfer.py to reduce code duplication
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| transfers/waterlevels_transfer.py | Refactored to add per-group commits, comprehensive statistics tracking, improved error handling with better context logging, and helper methods for row context and summary logging |
| transfers/transfer.py | Extracted _get_test_pointids() function to reduce duplication and improve maintainability |
| logger.warning( | ||
| "Skipping %s because no field event participants were found", | ||
| self._row_context(row), | ||
| ) |
There was a problem hiding this comment.
When field_event_participants is empty, a FieldEvent is created and added to the session (lines 177-183) but then the code continues without creating any FieldEventParticipants (line 200). This leaves an orphaned FieldEvent record in the database with no participants. Consider moving the field_event_participants check before creating the FieldEvent, or using session.expunge(field_event) before continuing to remove it from the session.
| ) | |
| ) | |
| session.expunge(field_event) |
| continue | ||
|
|
||
| sampler = None | ||
| for i, participant in enumerate(field_event_participants): |
There was a problem hiding this comment.
The loop variable 'i' on line 203 shadows the outer loop variable 'i' from line 150. While this doesn't cause a bug in this case since the outer 'i' is not used after line 150, it's better practice to use a different variable name (e.g., 'participant_idx' or 'p_idx') to avoid confusion and potential bugs if the code is modified later.
| try: | ||
| session.commit() | ||
| session.expunge_all() | ||
| stats["groups_processed"] += 1 | ||
| except DatabaseError as e: | ||
| stats["groups_failed_commit"] += 1 | ||
| logger.exception( | ||
| "Failed committing WaterLevels group for PointID=%s: %s", | ||
| pointid, | ||
| e, | ||
| ) | ||
| session.rollback() | ||
| self._capture_database_error(pointid, e) | ||
| except SQLAlchemyError as e: | ||
| stats["groups_failed_commit"] += 1 | ||
| logger.exception( | ||
| "SQLAlchemy failure committing WaterLevels group for PointID=%s: %s", | ||
| pointid, | ||
| e, | ||
| ) | ||
| session.rollback() | ||
| self._capture_error(pointid, str(e), "UnknownField") | ||
| except Exception as e: | ||
| stats["groups_failed_commit"] += 1 | ||
| logger.exception( | ||
| "Unexpected failure committing WaterLevels group for PointID=%s: %s", | ||
| pointid, | ||
| e, | ||
| ) | ||
| session.rollback() | ||
| self._capture_error(pointid, str(e), "UnknownField") |
There was a problem hiding this comment.
The base Transferer.transfer() method calls session.commit() after _transfer_hook returns (line 65 of transferer.py). Since this implementation now commits per-group inside the loop, there will be a final commit after all groups are processed. While this is not necessarily wrong, it's redundant since all changes have already been committed. Consider whether this design is intentional or if the base class commit should be bypassed for this implementation.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?