odoo-code-review

•

184393 months ago

Review Odoo code for correctness, security, performance, and Odoo 18 standards. Use when reviewing Odoo modules, diffs, or pull requests; produce a scored report with weighted criteria.

Quick Install

npxskills add majiayu000/claude-skill-registry--skill odoo-code-review

Instructions

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

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

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)
❌ use in Odoo 18 (use )

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

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:

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
Are there N+1 queries?
- Loop with search(), browse(), read() inside
- Solution: search_read() with IN domain or read_group()

Loading…

Related Skills

skill-creator

Create or update AgentSkills. Use when designing, structuring, or packaging skills with scripts, references, and assets.

367,360