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`