[WIP][16.0] add partner_multi_relation_contact#2250
[WIP][16.0] add partner_multi_relation_contact#2250
Conversation
Instead of a SQL view that shows relatiosn from two sides, we will have a simple model, and a simple UI view, that however will adapt to from which partner relations are shown.
2dc8bb8 to
4fb406a
Compare
4fb406a to
04fa4e4
Compare
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause of Test Failure
The test failure occurs because the migration script attempts to drop SQL views (res_partner_relation_all and res_partner_relation_type_selection) that no longer exist in the updated module structure. These views were removed in favor of direct model-based relations, but the post-migration script still tries to drop them, causing a database error during upgrade.
2. Suggested Fix
In partner_multi_relation/migrations/16.0.2.0.0/post-migration.py, remove or conditionally execute the DROP VIEW statements for res_partner_relation_all and res_partner_relation_type_selection.
Specific change:
# Replace:
env.cr.execute("DROP VIEW IF EXISTS res_partner_relation_all;")
env.cr.execute("DROP VIEW IF EXISTS res_partner_relation_type_selection;")
# With:
# Only drop if the views actually exist
env.cr.execute("DROP VIEW IF EXISTS res_partner_relation_all;")
env.cr.execute("DROP VIEW IF EXISTS res_partner_relation_type_selection;")Or better yet, use openupgrade's helper to safely drop views:
openupgrade.drop_view_if_exists(env.cr, "res_partner_relation_all")
openupgrade.drop_view_if_exists(env.cr, "res_partner_relation_type_selection")This avoids errors in case the views are already gone or never existed.
3. Additional Code Issues
-
Missing
activefield inres_partner_relationmodel:
Theactivefield was added tores_partner_relation, but there is no logic to automatically set it toFalsewhendate_endis in the past.
✅ Suggestion: Add a cron job or a computed field that setsactive = Falsefor records wheredate_end < today. -
Inconsistent use of
self.env.contextin_get_current_partner:
The method usescontext.get("active_model") == "res.partner"to infer the current partner, but this may not be reliable in all contexts.
✅ Suggestion: Ensure thatcurrent_partner_idis always explicitly passed via context, or add a fallback toself.env.user.partner_idif no context partner is found. -
Potential performance issue in
_search_relation_date:
The method performs two separate searches onres.partner.relationwithdate_domain, which can be inefficient.
✅ Suggestion: Refactor to use a single query with a union or subquery if needed, or optimize the domain logic.
4. Test Improvements
Add tests for:
- Migration behavior:
UseTransactionCaseto simulate a database upgrade and assert thatres_partner_relation_allandres_partner_relation_type_selectionare safely dropped without errors.
Add tests for:
- Relation search methods (
_search_relation_*)
UseSavepointCaseto test edge cases such as:- Search with invalid operators (should raise
ValidationError) - Search with
date_startanddate_endconstraints - Search with
inandnot inoperators on relation types
- Search with invalid operators (should raise
Add tests for:
- Context-dependent fields (
_compute_*methods)
UseSavepointCaseto test:type_id_display,this_partner_id,other_partner_idwith and withoutcurrent_partner_idin context- Ensure correct display logic when partner is on the left or right side
Reference OCA Testing Patterns:
- Use
@tag("post_install", "at_install")for migration tests. - Use
TransactionCasefor full database-level behavior. - Use
SavepointCasefor unit tests with complex contexts and domain logic.
Summary
| Issue | File | Description |
|---|---|---|
| Migration failure | post-migration.py |
Drop views that no longer exist |
| Missing active logic | res_partner_relation.py |
No auto-archive logic for past-dated relations |
| Context handling | _get_current_partner |
Inconsistent inference of current partner |
| Search performance | _search_relation_date |
Redundant queries on res.partner.relation |
These changes will improve stability, correctness, and test coverage of the module.
⏰ This PR has been open for 69 days.
🔍 No human reviews yet after 69 days. PSC members: a quick review would help keep this contributor engaged with OCA.
💤 Last activity was 51 days ago.
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
- hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
- stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
- stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
- stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
- purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix
Reviewing each other's work helps the whole community move forward. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
Based on the simplified version of partner_multi_relation