fix(spp_hazard_programs,spp_import_match,spp_oauth): beta fixes and import overwrite toggle#85
fix(spp_hazard_programs,spp_import_match,spp_oauth): beta fixes and import overwrite toggle#85
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #85 +/- ##
==========================================
+ Coverage 69.15% 69.28% +0.12%
==========================================
Files 628 647 +19
Lines 35632 36061 +429
==========================================
+ Hits 24643 24986 +343
- Misses 10989 11075 +86
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 |
| | ` | 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 | |
1 participant
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