review-guidelines/REVIEW_GUIDELINES_planogrammer.md

Planogrammer Review Guidelines

These guidelines are derived from real code review comments on Planogrammer merge requests. Higher-numbered MRs (newer) take precedence where guidelines conflict.

---

Code Style & Naming

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

• DO name boolean variables so they read as a statement:

  is_changed = column.id in changed_column_ids
  

  Align the name with the underlying table/entity where it helps (machine_column_changes -> is_changed). (MR 134)

• DO simplify boolean assignments. Instead of an if/else block that sets True/False:

  # 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
  

  (MR 118)

• DO use the X | None syntax for optional type hints:

  column_filter: PushPlanogramColumnFilterDto | None = None
  

  (MR 134)

• 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. (MR 94)

• DO suffix all custom exception classes with Exception, e.g. UnableToDeleteColumnException, BulkLimitExceededException. (MR 65)

• DO use single _ prefix for protected helpers, double __ prefix for private module-level functions that implement name mangling. The convention is: _ = protected, __ = private. (MR 52)

• DO prefer named-constant arithmetic for time durations:

  SHORT_PRICE_REVERT_PERIOD = 24 * HOURS  # readable
  # not: 86400                            # magic number
  # not: 60 * 60 * 24                     # still unclear at a glance
  

  (MR 41)

• DO extract repeated small operations into named helper functions:

  def _clear_changed_prices(column): ...
  def _get_price_or_none(price): ...
  

  This removes duplication and makes the calling code read at a consistent level of abstraction. (MR 134)

• DO decompose complex boolean comparisons into named intermediate variables:

  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 ...
  

  (MR 61)

• DO invert conditionals to enable early-return/continue patterns. Instead of a large if is_changed block:

  if not is_changed:
      _clear_changed_prices(column)
      continue
  # handle the positive case at normal indentation
  

  (MR 134)

• AVOID using assert in production/runtime code. Assertions are stripped with -O and communicate the wrong intent. Raise an explicit exception instead. (MRs 61, 65)

• AVOID a static method when a module-level constant object suffices. Static methods add unnecessary call overhead and obscure intent. (MR 119)

• AVOID using api in endpoint paths -- all paths are API paths by definition. Similarly, avoid internals as a path segment if the endpoint belongs in an existing domain hierarchy. (MRs 53, 52)

• AVOID tabs; use spaces. Run ruff format before pushing. Install pre-commit hooks so formatting is enforced automatically on every commit. (MRs 52, 94)

---

API Design

• DO place GET endpoints for a new domain in their own folder, not in an existing domain's folder. For example, planogram_change_requests/ gets its own endpoints.py, manager.py, dto.py, and exceptions.py. Don't put PCR read endpoints inside push/. (MR 134)

• DO add an upper-bound validation on all bulk list inputs. If a caller sends more than the allowed maximum (e.g. 1000 IDs), raise an exception immediately rather than letting the query run. Name the limit constant with a MAX_ prefix:

  MAX_GET_PRODUCT_BULK_IDS = 1000
  if len(ids) > MAX_GET_PRODUCT_BULK_IDS:
      raise BulkLimitExceededException(...)
  

  (MRs 139, 94)

• DO make source a mandatory query parameter when it is always required for business logic. Optional parameters that are always supplied create false API flexibility. (MR 107)

• DO consider adding optional boolean flags for batch callers who pre-validate:

  check_machine_in_route: bool = Query(default=True)
  

  This lets callers with their own validation skip redundant checks without breaking the default behavior. (MR 97)

• DO use company_exists(company_id) when you only need to verify existence; don't fetch the full company ORM object just to check it's not None. Reserve get_company for when you need the object's fields. (MR 94)

• 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 that if an exception is raised partway through the handler, the transaction rolls back correctly. (MR 61)

• DO use the correct HTTP status code for business-logic conflicts: HTTP_409_CONFLICT (not HTTP_403_FORBIDDEN) when the conflict is unrelated to authorization. (MR 118)

• DO treat ALREADY_SEEN responses from dmsync as a success (2xx), not an error. Only raise when the response indicates a genuine failure. (MR 119)

• DO use RouteServiceAPIError from televend-core, not from cloud-adapters. Always prefer the televend-core version of shared error classes. (MR 130)

• DO write query parameter help text in imperative form: Set this parameter to "true" in order to load brands and models. Don't expose internal implementation details (e.g. which database is queried) in OpenAPI docs. (MRs 18, 19)

• DO keep success responses for void operations minimal. An endpoint that performs an action and has nothing meaningful to return should respond with a short "ok" or an empty 204, not a verbose success DTO. (MRs 61, 65)

• DO use mutually exclusive validation (XOR) on DTO fields when exactly one of several filters must be provided:

  @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
  

  (MR 134)

• AVOID kebab-case query parameter names when the rest of the service uses snake_case. Mixing casing conventions breaks uniformity and can create subtle bugs. (MR 22)

---

Error Handling & Exceptions

• DO place domain-specific exceptions in that domain's own exceptions.py. If an exception is only raised and caught within the push/ flow, it belongs in service/api/.../push/exceptions.py, not in a shared module. (MR 139)

• DO add a catch-all handler in every endpoint for the base exception class of your domain exceptions. This ensures that any exception you forgot to handle individually still returns a structured error response rather than a 500. (MRs 61, 65)

• DO log the caught exception (at least at DEBUG level) when swallowing or transforming it, so we can trace the original cause in logs without propagating it to the caller. (MRs 52, 61)

• DO import only the specific exception class you need, not the entire exceptions module, to keep imports explicit and avoid namespace pollution. (MR 65)

• AVOID raising a generic Exception for domain-specific error conditions. Define a named exception class that describes what went wrong. (MR 65)

---

Testing

• DO assert on IDs only when verifying list/collection responses, not on the full DTO content:

  assert {r["id"] for r in response_json["content"]} == {p1.id, p2.id}
  

  This prevents test breakage when DTO fields are added or renamed. (MR 94)

• DO write test cases for PCR-type filtering correctness. A PRICE_CHANGE PCR must not create MachineColumnChange records for columns that only have structural (column) changes, and vice versa. Cover all three types: PRICE_CHANGE, COLUMN_CHANGE, PLANOGRAM_CHANGE, including partial/filtered scenarios. (MR 134)

• DO add tests for prices with 4 decimal places. This edge case has caused production bugs and must be covered explicitly. (MR 65)

• DO write a serialization test for every response DTO that contains Decimal or condecimal fields:

  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)
  

  (MR 122)

• DO use standard pytest setup_method / teardown_method for synchronous setup/teardown. Only use async fixtures when you genuinely need to await inside them. (MR 52)

• AVOID hard-coding specific auto-increment IDs in test assertions. Use the IDs from the objects you created in the test setup, not literal integers. (MR 94)

---

Architecture & Domain Rules

• DO fetch company/entity once per request and pass the ORM object to helper functions. Do not call get_company multiple times in the same request path. (MR 139)

• DO move entity existence checks into manager/service methods, not into the endpoint layer. The endpoint should call manager.do_something(company, ids) and let the manager validate that all IDs exist. (MR 139)

• DO batch multiple individual queries into a single list query where possible, and wrap the entire batch in a single try/except block. (MR 139)

• DO decompose large methods that do multiple conceptually distinct things. A method that detects the PCR type and also builds the column dict should be split: one method returns the type, a separate method builds the dict. (MR 134)

• DO keep manager responsibilities focused on a single flow. The PushManager handles the push flow. Read/GET logic for PlanogramChangeRequest belongs in a separate PCRManager or equivalent. Don't put GET logic in the push manager. (MR 134)

• DO initialize collaborating managers in the constructor of the owning manager, not lazily inside methods. (MR 134)

• DO filter MachineColumnChange records by PCR type. A PRICE_CHANGE PCR should only create MCCs for columns with pending price changes; a COLUMN_CHANGE PCR only for structural column changes. Validate this in tests. (MR 134)

• DO use utcnow() from the project's utility module instead of datetime.utcnow() directly. This keeps timestamp generation consistent and easy to mock in tests. (MR 138)

• DO prefer a direct field-update query over a fetch-then-modify-then-save pattern when you are only updating a single field. This avoids a redundant round trip. (MR 138)

• DO use save_many(entities) for bulk saves instead of looping over individual save(entity) calls. (MRs 61, 65)

• DO pass already-loaded ORM objects to methods rather than re-fetching by ID. If you have the object in scope, use it. (MR 61)

• DO prefer explicit save() calls over relying on implicit ORM dirty-tracking. Explicit saves make the persistence boundary visible and prevent subtle bugs when objects are shared across transactions. (MR 118)

• AVOID nesting context managers from different sources (adapter context manager wrapping a core context manager). Use one transaction boundary per operation. (MR 134)

• AVOID the deprecated load_options parameter on repository calls. Define typed LoadOptions classes in televend-core and reference those instead. (MRs 61, 65)

• AVOID duplicate DB fetches for the same entity in the same request. Fetch once, reuse the result. (MRs 139, 94)

• AVOID generalizing too early. If two similar APIs share code only by coincidence and would diverge with the next requirement, keep them separate (Rule of Three). Document when you intentionally choose not to reuse an existing method. (MR 53)

• AVOID calling the scheduler's run_immediately flag unnecessarily. Running tasks immediately on every API call bypasses the scheduler's batching and can overload downstream services. (MR 50)

---

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:

  @classmethod
  def from_model(cls, m: MachinePCR) -> "MachinePCRDto":
      return cls(id=m.id, status=m.status, ...)
  

  (MR 134)

• DO use model_dump_json() (not model_dump()) when serializing DTOs that contain Decimal or datetime fields, to ensure correct JSON types (float, ISO string) rather than Python object types. (MR 95)

• DO use ConfigDict consistently in all Pydantic v2 models. Don't mix old class Config style with the new model_config = ConfigDict(...) style in the same codebase. (MR 95)

• DO add json_encoders in ConfigDict for any datetime fields that require custom serialization behavior across all relevant models. (MR 95)

• DO give optional Pydantic fields an explicit default=None:

  class MyDto(BaseModel):
      filter: str | None = None
  

  (MR 95)

• DO use [[tool.uv.index]] in pyproject.toml for configuring private package index sources, not under pytest options or other tool sections. (MR 95)

---

Git & Process

• DO name feature branches feature/CLOUD-NNNNN-short-description. The feature/ prefix is required for CI/CD pipeline triggers. Branch names without this prefix will not run the build pipeline. (MR 21)

• DO include the full endpoint path in CHANGELOG entries and make the description meaningful:

  • Bad: Added new endpoint
  • Good: POST /v1/tenants/{tenant_id}/companies/{company_id}/planogram-change-requests/push/bulk -- creates PCRs in bulk for given machine IDs

  Follow the CHANGELOG wiki format documented in Confluence. (MRs 65, 94, 52)

• DO rebase your feature branch on develop before requesting review to avoid merge conflicts that reviewers then have to mentally skip. (MR 134)

• DO install and run pre-commit with ruff so that formatting issues are caught before push, not during review. (MR 94)

• AVOID [ci-skip] commits for changelog-only changes unless the change truly has no code impact; prefer including the changelog update in the feature commit. (MR 52)