Code Review Guidelines

Standards and best practices for reviewing code changes.

Overview

Code review ensures code quality, catches bugs early, shares knowledge, and maintains consistency across the codebase.

---

Review Process

Workflow

┌──────────┐     ┌──────────┐     ┌──────────┐     ┌──────────┐
│ Developer│     │   PR     │     │ Reviewer │     │  Merge   │
│  Submit  │────▶│ Created  │────▶│ Approved │────▶│ to Main  │
└──────────┘     └──────────┘     └──────────┘     └──────────┘
                      │                │
                      │   ┌────────┐   │
                      └──▶│ Changes│───┘
                          │Requested│
                          └────────┘

PR Checklist (Author)

Before requesting review:

• Code compiles/runs without errors
• Tests pass locally
• Self-reviewed the diff
• Removed debug code and console.logs
• Updated documentation if needed
• Bumped version in __manifest__.py
• Added entry to CHANGELOG.md
• PR description explains the changes
• Linked related issues

Review Checklist (Reviewer)

When reviewing:

• Understand the purpose of changes
• Check code correctness
• Verify code style consistency
• Look for security issues
• Check error handling
• Verify tests are adequate
• Check documentation updates
• Test locally if significant changes

---

What to Look For

1. Correctness

Check	Questions
Logic	Does the code do what it's supposed to?
Edge cases	What happens with null, empty, or extreme values?
Error handling	Are errors caught and handled properly?
Data integrity	Could this corrupt data?

Example Review Comment:

This loop doesn't handle empty lists. Consider adding:
if not records:
    return []

2. Security

Check	Questions
SQL Injection	Are queries parameterized?
XSS	Is user input sanitized?
Authentication	Is access properly restricted?
Secrets	Are credentials hardcoded?

Red Flags:

# BAD - SQL injection risk
self.env.cr.execute(f"SELECT * FROM users WHERE name = '{user_input}'")

# GOOD - Parameterized
self.env.cr.execute("SELECT * FROM users WHERE name = %s", [user_input])

3. Performance

Check	Questions
N+1 queries	Are records prefetched?
Unnecessary loops	Can this be done in SQL?
Large data	Is pagination used?
Caching	Should results be cached?

Red Flags:

# BAD - N+1 query
for order in orders:
    print(order.partner_id.name)  # Queries each time

# GOOD - Prefetch
orders = self.env['sale.order'].search([]).with_prefetch(['partner_id'])

4. Readability

Check	Questions
Naming	Are names descriptive?
Comments	Is complex logic explained?
Structure	Is code well-organized?
Length	Are functions too long?

Guidelines: - Functions should do one thing - Max 20-30 lines per function - Max 3-4 parameters per function - Avoid deep nesting (max 3 levels)

5. Maintainability

Check	Questions
DRY	Is code duplicated?
Coupling	Are components too dependent?
Hardcoding	Are magic numbers explained?
Extensibility	Is it easy to modify?

Red Flags:

# BAD - Magic number
if order.amount_total > 1000:

# GOOD - Named constant
LARGE_ORDER_THRESHOLD = 1000
if order.amount_total > LARGE_ORDER_THRESHOLD:

6. Testing

Check	Questions
Coverage	Are important paths tested?
Edge cases	Are boundaries tested?
Assertions	Are tests meaningful?
Independence	Can tests run in isolation?

---

Odoo-Specific Checks

Models

# Check for proper model definition
class MyModel(models.Model):
    _name = 'my.model'           # Required
    _description = 'My Model'    # Required for proper display
    _inherit = ['mail.thread']   # Consider if messaging needed

Fields

# Good field definition
name = fields.Char(
    string='Name',               # User-friendly label
    required=True,               # If mandatory
    index=True,                  # If frequently searched
    help='Description for users' # Tooltip
)

Security

<!-- Check security file exists -->
<!-- security/ir.model.access.csv -->
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
access_my_model,my.model,model_my_model,base.group_user,1,1,1,0

Views

<!-- Check view inheritance -->
<record id="view_form_inherited" model="ir.ui.view">
    <field name="inherit_id" ref="module.original_view"/>
    <field name="arch" type="xml">
        <!-- Use xpath for precise targeting -->
        <xpath expr="//field[@name='field_name']" position="after">
            <field name="new_field"/>
        </xpath>
    </field>
</record>

---

Python Code Review

Style

# Follow PEP 8
# - 4 spaces indentation
# - Max 79 characters per line (120 for Odoo)
# - Two blank lines between top-level definitions
# - One blank line between methods

Imports

# Good import order
import os                           # 1. Standard library
import sys

from dateutil import parser         # 2. Third-party

from odoo import api, fields, models # 3. Odoo
from odoo.exceptions import UserError

from .other_model import OtherClass  # 4. Local

Type Hints (Python 3.8+)

# Encouraged for complex functions
def calculate_total(
    orders: list,
    include_tax: bool = True
) -> float:
    """Calculate total amount for orders."""
    pass

---

JavaScript Code Review

PWA Code

// Check for proper error handling
async function fetchData() {
    try {
        const response = await fetch('/api/data');
        if (!response.ok) {
            throw new Error(`HTTP error: ${response.status}`);
        }
        return await response.json();
    } catch (error) {
        console.error('Fetch failed:', error);
        showErrorMessage('Failed to load data');
    }
}

DOM Manipulation

// Prefer getElementById for performance
const element = document.getElementById('myId');  // Fast

// Avoid
const element = document.querySelector('#myId');  // Slower

---

XML/View Code Review

XPath Expressions

<!-- Good - Specific xpath -->
<xpath expr="//field[@name='partner_id']" position="after">

<!-- Avoid - Fragile xpath -->
<xpath expr="//div/div/field" position="after">

Form Views

<!-- Check for proper groups -->
<group>
    <group string="Information">
        <field name="name"/>
        <field name="date"/>
    </group>
    <group string="Details">
        <field name="amount"/>
        <field name="state"/>
    </group>
</group>

---

Review Comments

Good Comments

# Constructive
"Consider using `search_read()` instead of `search()` + `read()`
for better performance."

# Specific
"This condition will fail when `partner_id` is False.
Add a check: `if self.partner_id and self.partner_id.email:`"

# Educational
"In Odoo, computed fields with `store=True` are recomputed
when dependencies change. Since this depends on `order_line`,
it will update automatically."

Bad Comments

# Too vague
"This is wrong."

# Not constructive
"Why would you do it this way?"

# Nitpicking
"I prefer single quotes over double quotes."

Comment Prefixes

Prefix	Meaning
[Required]	Must fix before merge
[Suggestion]	Nice to have, optional
[Question]	Need clarification
[Nitpick]	Minor style preference
[Praise]	Good job on this part

---

Review Etiquette

For Authors

Do	Don't
Respond to all comments	Ignore feedback
Explain your reasoning	Get defensive
Ask for clarification	Assume bad intent
Thank reviewers	Rush through fixes

For Reviewers

Do	Don't
Be respectful and kind	Be harsh or personal
Explain the "why"	Just say "change this"
Praise good code	Only point out negatives
Respond promptly	Let PRs sit for days
Focus on important issues	Nitpick every line

---

Approval Criteria

Approve When

• Code is correct and complete
• No security vulnerabilities
• Tests pass
• Documentation updated
• Follows coding standards
• No major concerns

Request Changes When

• Logic errors exist
• Security issues found
• Tests missing for critical code
• Major style violations
• Performance concerns

Comment Without Blocking When

• Minor style suggestions
• Alternative approaches to consider
• Questions about implementation
• Future improvement ideas

---

Review Metrics

Healthy Review Process

Metric	Target
Time to first review	< 24 hours
Review cycles	< 3 rounds
Comments per PR	5-15 (varies by size)
Approval rate	> 80% on second round

PR Size Guidelines

Size	Lines Changed	Review Time
Small	< 100	15-30 min
Medium	100-500	30-60 min
Large	500-1000	1-2 hours
Too Large	> 1000	Split the PR

---

Quick Reference

Must Check

1. Security - No SQL injection, XSS, hardcoded secrets
2. Correctness - Logic is right, edge cases handled
3. Tests - Important code is tested
4. Performance - No N+1 queries, efficient algorithms

Nice to Check

1. Style - Follows conventions
2. Readability - Clear naming, good structure
3. Documentation - Comments where needed
4. Maintainability - No code duplication

Approval Template

## Review Summary

**Status:** Approved / Changes Requested

### Checked
- [x] Correctness
- [x] Security
- [x] Performance
- [x] Tests
- [x] Documentation

### Comments
- Line 45: Consider adding error handling
- Line 78: Nice solution!

### Verdict
LGTM! Ready to merge after addressing minor comments.