Pull request for issue #22#23
Conversation
This commit enhances the `odoo-data-flow` tool to allow exporting the external XML ID of the main record while using the tool's fast `read` mode. When a user requests both `.id` and `id` in the fields list, and the export is running in `read` mode, the tool will now perform a secondary enrichment step. After fetching the main data with numeric IDs, it makes another call to Odoo to retrieve the XML IDs (`module.name`) for those numeric IDs. This is implemented using a Polars left join to merge the XML IDs back into the main DataFrame. The `id` column is then overwritten with the XML ID, and the temporary `.id` column is dropped from the final output. This allows users to get external IDs even when `read` mode is forced (e.g., by binary or selection fields), which was the primary goal of this feature. A comprehensive unit test has been added to verify this new functionality, including the case where a record may not have an XML ID.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. |
This commit enhances the `odoo-data-flow` tool to allow exporting the external XML ID of the main record while using the tool's fast `read` mode. When a user requests both `.id` and `id` in the fields list, and the export is running in `read` mode, the tool will now perform a secondary enrichment step. After fetching the main data with numeric IDs, it makes another call to Odoo to retrieve the XML IDs (`module.name`) for those numeric IDs. This is implemented using a Polars left join to merge the XML IDs back into the main DataFrame. The `id` column is then populated with the XML ID, and per the user's final clarification, the `.id` column containing the numeric database ID is preserved in the final output. This allows users to have access to both the database ID and the external ID for transformation steps. A comprehensive unit test has been added to verify this new functionality, including the case where a record may not have an XML ID.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for enriching exported data with the main record's XML ID when both .id and id are requested. The implementation is solid, with a new dedicated function for enrichment and corresponding logic to trigger it. The accompanying tests are thorough and cover the new functionality well, including edge cases. I've found one issue in an edge case concerning schema generation for empty exports, which I've detailed in a specific comment. Once that is addressed, this PR should be good to go.
| if ".id" in polars_schema: | ||
| del polars_schema[".id"] | ||
| polars_schema["id"] = pl.String() |
There was a problem hiding this comment.
This logic incorrectly modifies the schema for an empty DataFrame when XML ID enrichment is active. It deletes the .id column from the schema, but the feature's purpose is to preserve the .id column (containing the database ID) alongside the enriched id column (containing the XML ID). Deleting it would lead to an incorrect schema for empty export files, which can cause issues for downstream consumers expecting a consistent structure.
The fix is to simply update the type of the id column to String without removing the .id column.
| if ".id" in polars_schema: | |
| del polars_schema[".id"] | |
| polars_schema["id"] = pl.String() | |
| # When enrichment is active, the 'id' column becomes a string (XML ID). | |
| # The '.id' column (database ID) is preserved. | |
| polars_schema["id"] = pl.String() |
This commit enhances the `odoo-data-flow` tool to allow exporting the external XML ID of the main record while using the tool's fast `read` mode. When a user requests both `.id` and `id` in the fields list, and the export is running in `read` mode, the tool will now perform a secondary enrichment step. After fetching the main data with numeric IDs, it makes another call to Odoo to retrieve the XML IDs (`module.name`) for those numeric IDs. This is implemented using a Polars left join to merge the XML IDs back into the main DataFrame. The `id` column is then populated with the XML ID, and per the user's final clarification, the `.id` column containing the numeric database ID is preserved in the final output. This allows users to have access to both the database ID and the external ID for transformation steps. A comprehensive unit test has been added to verify this new functionality, including the case where a record may not have an XML ID. fix(ci): Correct nox pre-commit session The `pre-commit` nox session was failing in CI because `uv` was ignoring the session's virtual environment. This commit fixes the issue by explicitly passing the session's python interpreter to the `uv sync` command, ensuring dependencies are installed in the correct location.
…ython
The `nox` sessions were failing in CI because `uv sync` was not installing dependencies into the correct session-specific virtual environments. This was caused by `uv` ignoring the `VIRTUAL_ENV` set by `nox` and defaulting to a different environment.
This commit resolves the issue by:
1. Adding `--python str(session.python)` to all `uv sync` calls in `noxfile.py`. This explicitly tells `uv` which python executable to use, forcing it to install dependencies into the correct isolated virtual environment for each session.
2. Removing all `external=True` flags from the `uv sync` calls, as they are no longer needed and were part of the problem.
3. Removing a redundant `session.install("pydoclint")` call, as the dependency is already handled by `uv sync`.
These changes ensure that `tests` and `mypy` sessions run reliably. The `pre-commit` session is now configured correctly, although it may still face issues in certain CI environments with unusual PATH configurations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature for enriching exported data by fetching and populating the main record's XML ID when both .id and id fields are requested. The implementation is well-contained within a new function and integrated into the existing export flow, with corresponding logic to enable it and handle edge cases like empty results. The changes are supported by a new, thorough unit test. Additionally, the PR includes valuable updates to the noxfile.py to improve dependency management with uv, making the development environment setup more robust. Overall, the changes are a great improvement. I've left a couple of suggestions in export_threaded.py to further refine the new functionality.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
remove redundant res_id Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Fixes #22
PR created automatically by Jules for task 1351672480625428708