337 lines
14 KiB
Markdown
337 lines
14 KiB
Markdown
# 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:
|
|
```python
|
|
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`:
|
|
```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
|
|
```
|
|
(MR 118)
|
|
|
|
- **DO** use the `X | None` syntax for optional type hints:
|
|
```python
|
|
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:
|
|
```python
|
|
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:
|
|
```python
|
|
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:
|
|
```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 ...
|
|
```
|
|
(MR 61)
|
|
|
|
- **DO** invert conditionals to enable early-return/continue patterns. Instead of a large
|
|
`if is_changed` block:
|
|
```python
|
|
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:
|
|
```python
|
|
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:
|
|
```python
|
|
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:
|
|
```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
|
|
```
|
|
(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:
|
|
```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. (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:
|
|
```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)
|
|
```
|
|
(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:
|
|
```python
|
|
@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`:
|
|
```python
|
|
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)
|