review-guidelines/REVIEW_GUIDELINES_general.md

# General Review Guidelines

Derived from real code review history across three services:
- **planogrammer** — 128 MRs, 954 comments
- **cashtrack** — 61 MRs, 238 comments
- **assetor** — 28 MRs, 195 comments

Rules confirmed across multiple services are marked with the services that evidence them.
Newer reviews take precedence over older ones within each service.

---

## Code Style & Naming

- **DO** name variables to reflect their precise meaning. The name should answer "what is this
  a collection of?" Prefer `non_existing_ids` over `missing`, `changed_column_ids` over
  `changes`. *(planogrammer)*

- **DO** name boolean variables so they read as a statement:
  ```python
  is_changed = column.id in changed_column_ids
  ```
  *(planogrammer)*

- **DO** simplify boolean assignments. Instead of an `if/else` block that sets `True`/`False`:
  ```python
  # Before
  if source == PlanogramChangeRequestSource.AI:
      machine.planogram_optimized_by_ai = True
  else:
      machine.planogram_optimized_by_ai = False

  # After
  machine.planogram_optimized_by_ai = source == PlanogramChangeRequestSource.AI
  ```
  *(planogrammer)*

- **DO** decompose complex boolean comparisons into named intermediate variables:
  ```python
  are_products_equal = layout_column.product is None or layout_column.product == machine_column.product
  are_recipes_equal  = layout_column.recipe is None  or layout_column.recipe  == machine_column.recipe
  return are_products_equal and are_recipes_equal and ...
  ```
  *(planogrammer)*

- **DO** use the `X | None` syntax for optional type hints:
  ```python
  column_filter: PushPlanogramColumnFilterDto | None = None
  ```
  *(planogrammer)*

- **DO** use `Iterable[T]` for input parameters that only need to be iterated, not `List[T]`.
  Accept the most general type; return the most specific type. *(assetor)*

- **DO** use typed dataclasses for internal structured return values and error payloads rather
  than plain dicts or multi-value tuples:
  ```python
  # AVOID
  (asset_per_device_id, company_brand_ids, failed_responses) = await manager.bulk_create_preload(...)
  failed_responses.append({"device_id": device.id, "reason": "..."})

  # DO instead
  @dataclass
  class BulkPreloadResult:
      asset_per_device_id: dict
      failed_responses: list[FailedResponse]

  @dataclass
  class FailedResponse:
      device_id: int
      reason: str
  ```
  Consider static factory methods for repeated reason strings:
  `FailedResponse.for_device_already_has_asset(device_id)`. *(assetor)*

- **DO** prefix limit/capacity constants with `MAX_`, e.g. `MAX_GET_PRODUCT_BULK_IDS = 1000`.
  This signals that the constant is an upper bound, not a config value. *(planogrammer)*

- **DO** suffix all custom exception classes with `Exception`, e.g. `UnableToDeleteColumnException`,
  `BulkLimitExceededException`. *(planogrammer)*

- **DO** use single `_` prefix for protected helpers, double `__` prefix for private
  module-level functions (name mangling). The convention is: `_` = protected, `__` = private.
  *(planogrammer)*

- **DO** prefer named-constant arithmetic for time durations:
  ```python
  SHORT_PRICE_REVERT_PERIOD = 24 * HOURS  # readable
  # not: 86400                            # magic number
  # not: 60 * 60 * 24                     # still unclear at a glance
  ```
  *(planogrammer)*

- **DO** extract repeated small operations into named helper functions. This removes duplication
  and makes the calling code read at a consistent level of abstraction:
  ```python
  def _clear_changed_prices(column): ...
  def _get_price_or_none(price): ...
  ```
  *(planogrammer)*

- **DO** invert conditionals to enable early-return/continue patterns:
  ```python
  if not is_changed:
      _clear_changed_prices(column)
      continue
  # handle the positive case at normal indentation
  ```
  *(planogrammer)*

- **AVOID** generic update helpers that accept `Any`. A signature like
  `_update_field(name: str, value: Any)` eliminates type-checking. Use individual named methods:
  ```python
  # AVOID
  def _update_field(self, name: str, value: Any) -> None:
      setattr(self._conform, name, value)

  # DO instead
  def update_status(self, status: ConformStatus) -> None: ...
  def update_note(self, note: str) -> None: ...
  ```
  *(cashtrack)*

- **DO** include the type in field names when multiple variants exist or may be added:
  ```python
  # AVOID — breaks when value_token_diff is added later
  token_diff: Decimal

  # DO instead
  vend_token_diff: Decimal
  ```
  *(cashtrack)*

- **DO** keep constants (filter lists, limits, defaults) in a dedicated `const.py` rather than
  inline in query or view code. *(cashtrack)*

- **DO** add schema descriptions for non-obvious query parameters. *(cashtrack)*

- **DO** order method parameters hierarchically: `tenant_id` before `company_id`, more general
  before more specific. Apply consistently across all call sites. *(assetor)*

- **AVOID** `datetime.utcnow()` in new code. Use `datetime.now(UTC)` — `utcnow()` is deprecated
  in Python 3.12+. *(assetor, planogrammer: use project utility module)*

- **AVOID** using `assert` in production/runtime code. Assertions are stripped with `-O`.
  Raise an explicit exception instead. *(planogrammer)*

- **AVOID** tabs; use spaces. Run `ruff format` before pushing. Install pre-commit hooks so
  formatting is enforced automatically on every commit. *(planogrammer, cashtrack, assetor)*

- **AVOID** broad ruff/mypy ignore rules in `pyproject.toml`. Verify each suppression is
  genuinely needed by running `pre-commit run --all-files`. *(assetor)*

---

## API Design

- **DO** place each new domain in its own folder with its own `endpoints.py`, `manager.py`,
  `dto.py`, and `exceptions.py`. Don't embed new domain read logic in an existing domain's
  folder. *(planogrammer)*

- **DO** add an upper-bound validation on all bulk list inputs:
  ```python
  MAX_GET_PRODUCT_BULK_IDS = 1000
  if len(ids) > MAX_GET_PRODUCT_BULK_IDS:
      raise BulkLimitExceededException(...)
  ```
  *(planogrammer, cashtrack)*

- **DO** enforce input length limits at the API layer to match DB column constraints:
  - `note` fields: max 500 chars
  - `barcode` fields: max 128 chars
  *(cashtrack)*

- **DO** use request body (not query params) for mutation inputs. *(cashtrack)*

- **DO** default time-bounded list endpoints to a sensible window when date params are omitted
  (e.g. last 90 days). Document the default in the endpoint description. *(cashtrack)*

- **DO** make parameters mandatory when they are always required for business logic. Optional
  parameters that are always supplied create false API flexibility. *(planogrammer)*

- **DO** use mutually exclusive validation when exactly one of several filters must be provided:
  ```python
  @model_validator(mode="after")
  def validate_exclusive_filter(self) -> "PushFilterDto":
      provided = sum([bool(self.ids), bool(self.columns), bool(self.view_columns)])
      if provided != 1:
          raise ValueError("Exactly one filter must be provided")
      return self
  ```
  *(planogrammer, cashtrack)*

- **DO** use the correct HTTP status codes:
  - `404` when a referenced resource is not found (not `400` or `500`) *(assetor)*
  - `409 Conflict` for business-rule violations unrelated to authorization (not `403`) *(planogrammer)*

- **DO** for POST/PUT/PATCH/DELETE endpoints, instantiate the repository directly in the
  endpoint function rather than injecting it as a FastAPI dependency. This ensures transactions
  roll back correctly on exception. *(planogrammer)*

- **DO** use `company_exists(company_id)` when you only need to verify existence; reserve
  `get_company` for when you need the object's fields. *(planogrammer)*

- **DO** write query parameter help text in imperative form. Don't expose internal
  implementation details (e.g. which database is queried) in OpenAPI docs. *(planogrammer)*

- **DO** keep success responses for void operations minimal — a short `"ok"` or empty 204,
  not a verbose success DTO. *(planogrammer)*

- **DO** use SQLAlchemy 2.x style in all new queries:
  - `select(Model)` not `select([Model])`
  - `(condition, value)` not `[(condition, value)]` in CASE expressions
  - Add `.mappings()` to raw SQL results for dictionary-like access
  *(assetor)*

- **AVOID** kebab-case query parameter names when the rest of the service uses snake_case.
  *(planogrammer)*

---

## Error Handling & Exceptions

- **DO** place domain-specific exceptions in that domain's own `exceptions.py`. If an exception
  is only raised within one flow, it belongs in that flow's module. *(planogrammer)*

- **DO** add a catch-all handler in every endpoint for the base exception class of your domain
  exceptions. This ensures unhandled domain errors return a structured response rather than 500.
  *(planogrammer)*

- **DO** narrow `try/except` blocks to only the lines that can raise the caught exception:
  ```python
  # AVOID — business logic inside the try block catches unintended exceptions
  try:
      assignment = await self._get_asset_machine(asset)
      if assignment.machine_id is None:
          raise AssetItemNotAssigned(asset.id)
  except ObjectNotFound:
      raise AssetItemNotAssigned(asset.id)

  # DO instead
  try:
      assignment = await self._get_asset_machine(asset)
  except ObjectNotFound:
      raise AssetItemNotAssigned(asset.id)

  if assignment.machine_id is None:
      raise AssetItemNotAssigned(asset.id)
  ```
  *(assetor)*

- **DO** log the caught exception (at least at `DEBUG` level) when swallowing or transforming
  it, so the original cause remains traceable in logs. *(planogrammer)*

- **DO** validate payloads thoroughly before persisting:
  - Reject if all denomination/token values are zero
  - Reject duplicate entries in the same payload
  - Validate numeric values against DB column constraints before hitting the DB
  *(cashtrack)*

- **DO** handle external service unavailability explicitly. Always add error handling for the
  unreachable case when calling external services. *(assetor)*

- **DO** skip unknown enum/field values in changelog and audit endpoints rather than raising.
  *(cashtrack)*

- **AVOID** raising a generic `Exception` for domain-specific error conditions. Define a named
  exception class. *(planogrammer)*

- **AVOID** importing the entire `exceptions` module — import only the specific class you need.
  *(planogrammer)*

---

## Testing

- **DO** write tests in the same MR as the feature. *(cashtrack, planogrammer)*

- **DO** assert on IDs only when verifying list/collection responses, not on full DTO content:
  ```python
  assert {r["id"] for r in response_json["content"]} == {p1.id, p2.id}
  ```
  This prevents test breakage when DTO fields are added or renamed. *(planogrammer)*

- **DO** put manager-level behaviour tests in `test_<domain>_manager.py`, not in endpoint
  tests. Endpoint tests should only exercise endpoint-layer code. *(assetor)*

- **DO** cover every status transition with a dedicated test. *(assetor, cashtrack)*

- **DO** cover filter behaviour at the endpoint level, including interactions with data-access
  restrictions. *(cashtrack)*

- **DO** add tests for prices with 4 decimal places — this edge case has caused production bugs.
  *(planogrammer)*

- **DO** write a serialization test for every response DTO that contains `Decimal` or `datetime`
  fields:
  ```python
  def test_when_model_dump_with_json_mode_then_price_is_float():
      dto = MyResponseDto(price=Decimal("1.2345"))
      result = json.loads(dto.model_dump_json())
      assert isinstance(result["price"], float)
  ```
  *(planogrammer)*

- **DO** use standard `setup_method` / `teardown_method` for synchronous test setup/teardown.
  Only use `async` fixtures when you genuinely need to `await` inside them. *(planogrammer)*

- **AVOID** hard-coding specific auto-increment IDs in test assertions. Use the IDs from the
  objects created in test setup. *(planogrammer)*

---

## Architecture & Domain Rules

- **DO** fetch company/entity once per request and pass the object downstream. Never call
  `get_company` (or equivalent) more than once in the same request path. *(planogrammer)*

- **DO** move entity existence checks into manager/service methods, not the endpoint layer.
  *(planogrammer)*

- **DO** batch multiple individual DB queries into a single `list` query where possible.
  *(planogrammer)*

- **DO** decompose large methods that do multiple conceptually distinct things. One method should
  do one thing. *(planogrammer, cashtrack)*

- **DO** keep manager responsibilities focused on a single flow. Read/GET logic and write logic
  belong in separate managers. *(planogrammer)*

- **DO** initialize collaborating managers in the constructor, not lazily inside methods.
  *(planogrammer)*

- **DO** use `save_many(entities)` for bulk saves instead of looping `save(entity)`. *(planogrammer)*

- **DO** pass already-loaded ORM objects to methods rather than re-fetching by ID. *(planogrammer)*

- **DO** prefer explicit `save()` calls over relying on implicit ORM dirty-tracking. Explicit
  saves make the persistence boundary visible. *(planogrammer)*

- **DO** prefer a direct field-update query over a fetch-then-modify-then-save pattern when
  updating a single field. *(planogrammer)*

- **DO** use soft-delete (`alive` flag or equivalent) to preserve historical records. Always
  filter queries with the soft-delete sentinel — omitting it causes deleted records to leak into
  results. *(cashtrack)*

- **DO** look up by natural key before creating on import operations. Reuse an existing record
  if it already exists rather than always inserting. *(cashtrack)*

- **DO** recalculate derived totals (e.g. `total_coins`, `total_bills`) from source data
  whenever source data changes — never accept pre-calculated totals from the caller. *(cashtrack)*

- **DO** wire data-access (company + user) restrictions into every new list or filter endpoint
  before the MR is ready for review. *(cashtrack)*

- **DO** resolve circular imports using `TYPE_CHECKING` guards and `from __future__ import annotations`:
  ```python
  from __future__ import annotations
  from typing import TYPE_CHECKING

  if TYPE_CHECKING:
      from televend_core.databases.televend_repositories.asset_item.model import AssetItem
  ```
  Use `@dataclass` (stdlib) rather than Pydantic for internal DTOs when forward references
  cause Pydantic config errors. *(assetor)*

- **AVOID** nesting context managers from different sources. Use one transaction boundary per
  operation. *(planogrammer)*

- **AVOID** the deprecated `load_options` parameter on repository calls. Use typed `LoadOptions`
  classes from `televend-core`. *(planogrammer)*

- **AVOID** generalizing too early. If two similar pieces of code would diverge with the next
  requirement, keep them separate (Rule of Three). *(planogrammer)*

---

## DTO & Serialization

- **DO** implement `from_model(cls, orm_model)` as a classmethod on every response DTO. Keep
  endpoint handlers clean — don't inline model-to-DTO mapping there:
  ```python
  @classmethod
  def from_model(cls, m: MachinePCR) -> "MachinePCRDto":
      return cls(id=m.id, status=m.status, ...)
  ```
  *(planogrammer)*

- **DO** use `model_dump_json()` (not `model_dump()`) when serializing DTOs that contain
  `Decimal` or `datetime` fields, to ensure correct JSON types. *(planogrammer)*

- **DO** use `ConfigDict` consistently in all Pydantic v2 models. Don't mix old `class Config`
  style with `model_config = ConfigDict(...)` in the same codebase. *(planogrammer)*

- **DO** give optional Pydantic fields an explicit `default=None`:
  ```python
  class MyDto(BaseModel):
      filter: str | None = None
  ```
  *(planogrammer)*

---

## Git & Process

- **DO** name feature branches `feature/CLOUD-NNNNN-short-description`. The `feature/` prefix
  is required for CI/CD pipeline triggers. *(planogrammer, cashtrack, assetor)*

- **DO** title MRs as `CLOUD-NNNNN: description` (imperative, present tense). Avoid the
  "Resolve" prefix auto-generated by GitLab. *(cashtrack)*

- **DO** include the full endpoint path in CHANGELOG entries:
  - Bad: `Added new endpoint`
  - Good: `POST /v1/tenants/{tenant_id}/companies/{company_id}/planogram-change-requests/push/bulk`
  *(planogrammer)*

- **DO** keep MRs focused on a single ticket. Preliminary or follow-up changes belong in
  their own MR. *(cashtrack)*

- **DO** rebase your feature branch on `develop` before requesting review. *(planogrammer)*

- **DO** resolve all open review threads before merging. *(cashtrack, planogrammer)*

- **DO** ensure ruff passes before requesting review — a cleanup-only "Ruff is happy" MR is
  a sign that formatting discipline slipped. *(cashtrack, assetor, planogrammer)*

- **DO** add `/.venv` to `.gitignore`. *(assetor)*

- **DO** bump the service version in `pyproject.toml` as part of any dependency-upgrade MR.
  *(assetor)*

- **DO** update `README.md` when performing a major runtime upgrade. *(assetor)*

- **AVOID** `event_loop` fixture overrides in `conftest.py` — no longer required with modern
  `pytest-asyncio`. *(assetor)*

- **AVOID** `[ci-skip]` commits for changelog-only changes unless the change truly has no code
  impact. *(planogrammer)*