fix(spp_vocabulary): support extra domain for vocabulary_code search of the same display name#82
fix(spp_vocabulary): support extra domain for vocabulary_code search of the same display name#82
Conversation
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 addresses a critical issue in Odoo's CSV import process where Highlights
Changelog
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a clever solution to disambiguate vocabulary codes with the same display name during CSV imports by leveraging the field's domain. The override of db_id_for in a new ir.fields.converter model and the corresponding adjustment in name_search are well-implemented. The inclusion of comprehensive unit tests is also a great addition.
My main feedback is to improve the robustness of the error handling and logging within db_id_for to prevent potential crashes within the exception handler itself, especially when model is None as seen in the tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #82 +/- ##
==========================================
+ Coverage 68.28% 68.43% +0.15%
==========================================
Files 557 562 +5
Lines 31666 31852 +186
==========================================
+ Hits 21622 21797 +175
- Misses 10044 10055 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3c0f120 to
9330982
Compare
…of the same display name Signed-off-by: Aldrin Navarro <aldrin@newlogic.com>
9330982 to
3426b62
Compare
Why is this change needed?
When importing CSV data with Many2one fields pointing to
spp.vocabulary.code, Odoo's built-indb_id_forresolves records by display name.Because multiple vocabulary codes across different vocabularies can share the same display name (e.g. "Active"), the importer produces a multiple-match warning and resolves arbitrarily instead of picking the correct vocabulary.
Expected
No validation error
Actual
There were two vocabularies with the same display name but the resolver fails to differentiate them since the default lookup finds they share the same table.
How was the change implemented?
Two models extended in
spp_vocabulary:ir_fields_converter(new file) — Overridesdb_id_forscoped exclusively tospp.vocabulary.codefields. When the field carries a domain, it is evaluated (supporting both list and string forms) and passed as_import_name_search_domainin the search context. Malformed string domains are caught and logged as a warning rather than silently dropped.vocabulary_code.VocabularyCode— Overridesname_searchto merge any_import_name_search_domainfrom context into the search domain before callingsuper().limitdefaults toNoneto let Odoo apply its own default.New unit tests
Six new tests in
spp_vocabulary/tests/test_vocabulary_code.py:test_name_search_without_context_returns_all_matchestest_name_search_with_context_domain_scopes_to_vocabularytest_name_search_context_domain_does_not_affect_other_vocabularytest_db_id_for_list_domain_scopes_name_searchtest_db_id_for_string_domain_scopes_name_searchtest_db_id_for_no_domain_returns_multiple_match_warningUnit tests executed by the author
All 6 new tests plus the full
spp_vocabularytest suite pass.How to test manually
spp.vocabulary.codewith adomainrestricting to one vocabulary.