odoo-code-review

Odoo Code Review

Objective

Review Odoo code changes against clear criteria, identify risks, and score using a weighted scale from an Odoo 18 expert perspective.

Pre-review Requirements

• Read skills/odoo/18.0/SKILL.md as the master index for all Odoo 18 guides.
• Read relevant guides from skills/odoo/18.0/dev/ based on change scope:
  • Models/ORM: odoo-18-model-guide.md
  • Fields: odoo-18-field-guide.md
  • Decorators: odoo-18-decorator-guide.md
  • Performance: odoo-18-performance-guide.md
  • Views/XML: odoo-18-view-guide.md
  • Security: odoo-18-security-guide.md
  • Controllers: odoo-18-controller-guide.md
  • Transactions: odoo-18-transaction-guide.md
  • Mixins: odoo-18-mixins-guide.md (mail.thread, activities)
  • Testing: odoo-18-testing-guide.md
  • Migration: odoo-18-migration-guide.md
  • Actions: odoo-18-actions-guide.md
  • Data Files: odoo-18-data-guide.md
  • Manifest: odoo-18-manifest-guide.md
• Identify scope: module, file, and change context.
• Master Odoo 18 API changes: <list> instead of <tree>, @api.ondelete, etc.

Expert Review Process

1. Scope: Identify change scope, objectives, and key risks
2. ORM & Model Methods: Search patterns, CRUD operations, recordset operations
3. Field Definitions: Field types, computed fields, relational field parameters
4. API Decorators: @api.depends, @api.constrains, @api.ondelete (Odoo 18!)
5. Performance: N+1 detection, batch operations, field selection
6. Transaction Management: Savepoints, UniqueViolation, serialization
7. Views & XML: Odoo 18 tags (<list>), inheritance, structure
8. Security: ACL, record rules, exceptions, sudo usage
9. Controllers: Auth types, CSRF protection, routing
10. Mixins: mail.thread, mail.activity.mixin, mail.alias.mixin usage
11. Testing: Test coverage, proper test cases, @tagged decorators
12. Migration: Migration scripts, data migration patterns
13. Actions: Window actions, server actions, cron jobs
14. Data Files: XML/CSV data structure, noupdate, shortcuts
15. Manifest: Dependencies, external deps, hooks, assets

Odoo 18 Complete Checklist

ORM & Model Methods (30%)

• ❌ DO NOT use search() inside loop (N+1 anti-pattern)
• ✅ Use search_read() when dict output needed
• ✅ Use read_group() for aggregate queries
• ✅ Use IN domain instead of search in loop: [('order_id', 'in', orders.ids)]
• ✅ Batch create([{...}, {...}]) for multiple records
• ✅ Use recordset.write() instead of loop
• ✅ Use recordset.unlink() instead of loop
• ✅ Use mapped() instead of list comprehension
• ✅ Use filtered() before operations
• ✅ Use exists() to filter non-existing records

Field Definitions (15%)

• ✅ Many2one has ondelete parameter (cascade, restrict, set null)
• ✅ Monetary has currency_field parameter
• ✅ One2many has inverse_name parameter
• ❌ DO NOT use Float for currency (use Monetary)
• ❌ DO NOT use <tree> in Odoo 18 (use <list>)
• ✅ Computed fields have store=True if searchable/groupable needed
• ✅ @api.depends includes ALL dependencies with dotted paths

API Decorators (15%)

• ✅ @api.depends uses dotted paths for related fields: @api.depends('partner_id.email')
• ❌ DO NOT use dotted paths in @api.constrains (only simple field names)
• ✅ @api.ondelete(at_uninstall=False) instead of overriding unlink() for validation (Odoo 18!)
• ✅ @api.constrains raises ValidationError
• ✅ @api.model_create_multi for batch create (Odoo 18)

Performance (20%)

• ❌ DO NOT search() in loop
• ❌ DO NOT browse() in loop
• ❌ DO NOT create() in loop
• ❌ DO NOT write() in loop
• ❌ DO NOT unlink() in loop
• ✅ Use prefetch (automatic) for related field access
• ✅ Use search_read() to fetch specific fields
• ✅ Use bin_size=True for binary fields
• ✅ Use advisory locks for concurrent operations

Transaction Management (10%)

• ✅ Use with self.env.cr.savepoint(): for error isolation
• ❌ DO NOT continue after UniqueViolation without savepoint
• ✅ Use advisory locks to prevent serialization errors
• ✅ Group identical updates to minimize conflicts

Views & XML (5%)

• ✅ Use <list> instead of <tree> (Odoo 18!)
• ✅ Use decoration-* for row styling
• ✅ Use xpath or shorthand with position for inheritance
• ✅ Proper inherit_id reference

Security (5%)

• ✅ Has ir.model.access.csv file with proper permissions
• ✅ Use UserError for business logic errors
• ✅ Use ValidationError for constraint violations
• ✅ Use AccessError for permission issues
• ❌ DO NOT raise generic Exception
• ✅ Record rules defined with proper domain_force

Controllers (5%)

• ✅ Use correct auth type (user, public, none)
• ✅ Use auth='none' for truly public endpoints (webhooks)
• ✅ CSRF enabled for POST (default)
• ✅ csrf=False only for external webhooks

Mixins (Additional Check)

• ✅ mail.thread properly configured with tracking=True on tracked fields
• ✅ mail.activity.mixin used for activity-enabled models
• ✅ mail.alias.mixin properly configured with alias fields
• ✅ utm.mixin for campaign tracking when applicable
• ✅ Proper message_post usage, not direct chatter manipulation

Testing (Additional Check)

• ✅ Tests cover new functionality
• ✅ Proper use of @tagged decorators (standard, post_install, etc.)
• ✅ TransactionCase for model tests, HttpCase for web tests
• ✅ Test data properly isolated
• ✅ Query count assertions for performance-critical code

Migration (Additional Check)

• ✅ Migration scripts in migrations/{version}/ directory
• ✅ Pre-migration scripts for data cleanup
• ✅ Post-migration scripts for data migration
• ✅ Uses hooks (pre_init, post_init, uninstall) appropriately
• ✅ Idempotent migration scripts

Anti-Patterns to Detect

Anti-Pattern	Consequence	Fix
search() in loop	N+1 queries	Use search_read() with IN domain
create() in loop	N INSERT statements	Batch: create([{...}, {...}])
write() in loop	N UPDATE statements	records.write({...})
unlink() in loop	N DELETE statements	records.unlink()
Override unlink() for validation	Breaks module uninstall	Use @api.ondelete(at_uninstall=False)
@api.depends('a') then access a.b	N queries	Add @api.depends('a.b')
@api.constrains('a.b')	Not supported	Use only @api.constrains('a')
<tree> in Odoo 18	Deprecated	Use <list>
Float for currency	Precision issues	Use Monetary
Missing ondelete on Many2one	Orphan records	Add ondelete='cascade/restrict'
Generic Exception	Poor UX	Use UserError, ValidationError
Continue after UniqueViolation without savepoint	Transaction aborted	Use with self.env.cr.savepoint():
Direct chatter manipulation instead of message_post	Breaks mail.thread features	Use message_post() with proper subtype
Missing tracking=True on tracked fields	No field tracking in chatter	Add tracking=True to field definition
Tests without @tagged decorators	Wrong test environment	Add @tagged('standard'), @tagged('post_install')
Non-idempotent migration script	Fails on re-run	Use if not field_exists: checks
Missing noupdate="1" on reference data	Data overwritten on update	Add noupdate="1" to reference records
Cron without interval_number and interval_type	Never runs	Add proper interval configuration

Scoring Scale (Weighted)

Criteria (score 1-10):

• ORM & Model Methods (28%)
• Field Definitions (14%)
• API Decorators (14%)
• Performance (18%)
• Transaction Management (10%)
• Views & XML (4%)
• Security (6%)
• Controllers (6%)

Total calculation:

total = 0.28*orm + 0.14*fields + 0.14*decorators + 0.18*performance + 0.10*transaction + 0.04*views + 0.06*security + 0.06*controllers

Score anchors:

• 9-10: Excellent, no significant risks, follows all best practices
• 7-8: Good, minor issues or improvements possible
• 5-6: Average, clear risks to address, has anti-patterns
• 3-4: Poor, serious errors or regression-prone
• 1-2: Very poor, cannot merge, violates critical patterns

Report Format (Required)

## Quick Summary
- [1-2 sentences summarizing key points]

## Overall Score
- Total: X.X/10
- Formula: 0.28*ORM + 0.14*Fields + 0.14*Decorators + 0.18*Perf + 0.10*Trans + 0.04*Views + 0.06*Sec + 0.06*Controllers

## Score by Criteria
- ORM & Model Methods: X/10 — [brief reason, any anti-patterns?]
- Field Definitions: X/10 — [brief reason]
- API Decorators: X/10 — [brief reason, check @api.ondelete, dotted paths]
- Performance: X/10 — [brief reason, any N+1?]
- Transaction Management: X/10 — [brief reason, savepoints correct?]
- Views & XML: X/10 — [brief reason, using <list>?]
- Security: X/10 — [brief reason]
- Controllers: X/10 — [brief reason]

## Key Findings (high → low priority)

### 🔴 Critical (Must Fix)
- [Severity] Brief description + consequence + fix suggestion
- Code reference: `path/file.py:XX`

### 🟡 Major (Should Fix)
- [Severity] Brief description + consequence + fix suggestion
- Code reference: `path/file.py:XX`

### 🔵 Minor (Nice to Have)
- [Severity] Brief description + improvement suggestion

## Positive Patterns Found
- ✅ [Good pattern found] - Line XX

## Recommendations
- [Specific, clear improvements, in priority order]

## Testing
- Ran: [if any, state commands]
- Missing: [tests missing or not run, N+1 scenarios]

Response Rules

• Prioritize error and risk detection first, then suggestions
• If no significant issues, clearly state "No findings"
• Cite correct file and code when needed: path/to/file.py:XX
• State assumptions when information is missing (don't guess)
• Focus on Odoo-specific patterns, not generic Python advice
• Provide code examples for complex issues
• Reference Odoo documentation when applicable

Deep Dive Checks

When reviewing, thoroughly check:

1. Does @api.depends have complete dependencies?

   • Check dotted paths: partner_id.email instead of just partner_id
   • Missing dependencies cause N queries
   • Reference: dev/odoo-18-decorator-guide.md

2. Are there N+1 queries?

   • Loop with search(), browse(), read() inside
   • Solution: search_read() with IN domain or read_group()
   • Reference: dev/odoo-18-performance-guide.md

3. Are there batch operations?

   • create(), write(), unlink() in loop
   • Solution: Batch operations on recordset
   • Reference: dev/odoo-18-performance-guide.md

4. Is transaction safe?

   • UniqueViolation handling without savepoint
   • Concurrent updates without advisory lock
   • Reference: dev/odoo-18-transaction-guide.md

5. Are Odoo 18 patterns correct?

   • Use <list> instead of <tree>
   • Use @api.ondelete() instead of overriding unlink()
   • Use @api.model_create_multi for batch create
   • Reference: dev/odoo-18-view-guide.md

6. Are field definitions correct?

   • Monetary with currency_field
   • Many2one with ondelete
   • Computed field with store=True if needed
   • Reference: dev/odoo-18-field-guide.md

7. Is exception handling correct?

   • UserError, ValidationError, AccessError
   • No generic Exception
   • Reference: dev/odoo-18-security-guide.md

8. Are mixins properly configured?

   • mail.thread with proper tracking fields
   • mail.activity.mixin for activities
   • mail.alias.mixin with alias fields
   • Reference: dev/odoo-18-mixins-guide.md

9. Is testing adequate?

   • Tests for new functionality
   • Proper use of @tagged decorators
   • Query count assertions for performance
   • Reference: dev/odoo-18-testing-guide.md

10. Are migrations handled correctly?

    • Proper migration script location
    • Pre/post migration scripts
    • Idempotent operations
    • Reference: dev/odoo-18-migration-guide.md

11. Are actions properly defined?

    • Window actions with correct context
    • Server actions for automation
    • Cron jobs with proper intervals
    • Reference: dev/odoo-18-actions-guide.md

12. Are data files correct?

    • Proper XML record structure
    • noupdate="1" for reference data
    • CSV data properly formatted
    • Reference: dev/odoo-18-data-guide.md

13. Is manifest correct?

    • All dependencies declared
    • External dependencies listed
    • Hooks properly configured
    • Reference: dev/odoo-18-manifest-guide.md