Conversation
…mport overwrite toggle - spp_hazard_programs: remove unused ACL file, fix view XML - spp_import_match: migrate legacy JS/XML to OWL components, add overwrite match toggle with per-import control, add import result notifications with match counts - spp_oauth: fix RSA encode/decode and settings view for Odoo 19
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #85 +/- ##
==========================================
+ Coverage 69.15% 69.42% +0.26%
==========================================
Files 628 647 +19
Lines 35632 36055 +423
==========================================
+ Hits 24643 25030 +387
- Misses 10989 11025 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on refining and stabilizing three core OpenSPP modules for Odoo 19. It addresses critical compatibility issues, enhances user control over data import processes, and streamlines module configurations, moving them from an alpha to a beta development stage. The changes aim to improve the robustness and usability of the platform's hazard programs, data import functionalities, and OAuth authentication. Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 several fixes and improvements across spp_hazard_programs, spp_import_match, and spp_oauth modules, upgrading them to Beta status. The changes in spp_import_match are significant, migrating the import matching feature to the OWL framework, adding an overwrite toggle, and providing user feedback on import results. The use of threading.local is a good approach for passing data in Odoo's threaded environment, and the refactoring in spp_import_match/models/base.py to use self._fields is a great performance improvement. My review includes a suggestion to refactor some duplicated code for better maintainability and a fix for a documentation formatting issue.
| if dryrun: | ||
| _logger.info("Doing dry-run import") | ||
| if import_match_ids: | ||
| self = self.with_context(import_match_ids=import_match_ids) | ||
| return super().execute_import(fields, columns, options, dryrun=True) | ||
| self = self.with_context(import_match_ids=import_match_ids, overwrite_match=overwrite_match) | ||
| result = super().execute_import(fields, columns, options, dryrun=True) | ||
| counts = getattr(_import_match_local, "counts", None) | ||
| if counts: | ||
| result["import_match_counts"] = counts | ||
| _import_match_local.counts = None | ||
| return result | ||
|
|
||
| if len(input_file_data) <= 100: | ||
| _logger.info("Doing normal import") | ||
| if import_match_ids: | ||
| self = self.with_context(import_match_ids=import_match_ids) | ||
| return super().execute_import(fields, columns, options, dryrun=False) | ||
| self = self.with_context(import_match_ids=import_match_ids, overwrite_match=overwrite_match) | ||
| result = super().execute_import(fields, columns, options, dryrun=False) | ||
| counts = getattr(_import_match_local, "counts", None) | ||
| if counts: | ||
| result["import_match_counts"] = counts | ||
| _import_match_local.counts = None | ||
| return result |
There was a problem hiding this comment.
There's significant code duplication between the dryrun block and the synchronous import block (len(input_file_data) <= 100). Both blocks perform nearly identical logic for adding context, calling super().execute_import, and handling the results. This can be refactored into a single conditional block to improve maintainability and reduce redundancy.
| if dryrun: | |
| _logger.info("Doing dry-run import") | |
| if import_match_ids: | |
| self = self.with_context(import_match_ids=import_match_ids) | |
| return super().execute_import(fields, columns, options, dryrun=True) | |
| self = self.with_context(import_match_ids=import_match_ids, overwrite_match=overwrite_match) | |
| result = super().execute_import(fields, columns, options, dryrun=True) | |
| counts = getattr(_import_match_local, "counts", None) | |
| if counts: | |
| result["import_match_counts"] = counts | |
| _import_match_local.counts = None | |
| return result | |
| if len(input_file_data) <= 100: | |
| _logger.info("Doing normal import") | |
| if import_match_ids: | |
| self = self.with_context(import_match_ids=import_match_ids) | |
| return super().execute_import(fields, columns, options, dryrun=False) | |
| self = self.with_context(import_match_ids=import_match_ids, overwrite_match=overwrite_match) | |
| result = super().execute_import(fields, columns, options, dryrun=False) | |
| counts = getattr(_import_match_local, "counts", None) | |
| if counts: | |
| result["import_match_counts"] = counts | |
| _import_match_local.counts = None | |
| return result | |
| if dryrun or len(input_file_data) <= 100: | |
| _logger.info("Doing %s import", "dry-run" if dryrun else "synchronous") | |
| import_instance = self | |
| if import_match_ids: | |
| import_instance = self.with_context( | |
| import_match_ids=import_match_ids, overwrite_match=overwrite_match | |
| ) | |
| result = super(SPPBaseImport, import_instance).execute_import( | |
| fields, columns, options, dryrun=dryrun | |
| ) | |
| counts = getattr(_import_match_local, "counts", None) | |
| if counts: | |
| result["import_match_counts"] = counts | |
| _import_match_local.counts = None | |
| return result |
spp_oauth/README.rst
Outdated
| | ` | Decodes and verifies JWT token, | | ||
| | `verify_and_decode_signature()`` | returns payload | |
There was a problem hiding this comment.
There appears to be a formatting error in this reStructuredText table. The backticks for verify_and_decode_signature() are misplaced, which will likely cause rendering issues in the documentation. The function name should be on the same line and enclosed in double backticks. You may need to adjust the table's column widths to accommodate the full function name on one line.
| | ` | Decodes and verifies JWT token, | | |
| | `verify_and_decode_signature()`` | returns payload | | |
| | ``verify_and_decode_signature()`` | Decodes and verifies JWT token, returns payload | |
…et via UI When import_match_ids is not passed through the UI options (e.g. in tests or programmatic imports), fall back to reading overwrite_match from the matching config records instead of defaulting to False.
Reverts oca-gen-addon-readme output to match CI's version to avoid pre-commit failures from tool version differences.
Regenerated auto-generated files for spp_hazard_programs, spp_import_match, and spp_oauth to match CI oca-gen-addon-readme output with Beta status badges.
|
@emjay0921 Please increase the code coverage of spp_import_match/models/base_import.py. Fix the code base on the Gemini-code-assist recommendations. |
Add test_base_load.py (8 tests) covering Base.load() override paths: no usable rules, overwrite true/false, no match creates, mixed records, import_match_ids context filtering, id field append, match counts tracking. Add test_base_import_methods.py (8 tests) covering SPPBaseImport helpers: CSV attachment roundtrip, custom options, extract chunks, import_one_chunk success/error, ImportValidationError, execute_import with match_ids context. Add 3 tests to test_import_match_model.py: onchange_field_id skips relational fields, match_find sub_field matching, match_find multiple rule combinations.
…in execute_import Merge the nearly identical dryrun and synchronous (<= 100 rows) blocks into a single conditional, reducing code duplication while preserving identical behavior.
2 participants
Why is this change needed?
How was the change implemented?
ir.model.access.csv, fixedprogram_views.xmlimport_match_selector.js/.xml)threading.local()to pass counts betweenBase.load()andexecute_import()rsa_encode_decode.pyandres_config_view.xmlfor Odoo 19New unit tests
No new unit tests added. Existing tests for spp_oauth (10/10 pass) and spp_import_match verified.
Unit tests executed by the author
How to test manually
/tmp/test_spp_oauth.pyfor JWT signing/verificationRelated links