commit 79abebff51abf1fe7c88797db1ff90807a0abfa2 Author: Eden Kirin Date: Fri Mar 6 14:50:30 2026 +0100 first commit diff --git a/REVIEW_GUIDELINES_assetor.md b/REVIEW_GUIDELINES_assetor.md new file mode 100644 index 0000000..21b15a8 --- /dev/null +++ b/REVIEW_GUIDELINES_assetor.md @@ -0,0 +1,175 @@ +# Assetor Review Guidelines + +Guidelines derived from 28 merged MRs of real code review history. Newer reviews take precedence. + +--- + +## Code Style & Naming + +- **AVOID** returning multi-value tuples from methods when a named dataclass would be clearer. + Tuple destructuring at the call site is hard to read and fragile. + + ```python + # AVOID + (asset_per_device_id, company_brand_ids, failed_responses) = await manager.bulk_create_preload(body, user_id) + + # DO instead + bulk_preload: BulkPreloadDto = await manager.bulk_create_preload(body, user_id) + ``` + + Use `@dataclass` (not Pydantic) for internal result objects to avoid forward-ref issues. + Pair with `from __future__ import annotations` and `TYPE_CHECKING` guards for + circular-import-safe type hints. (MR !3) + +- **DO** use typed dataclasses for structured error/response payloads rather than plain dicts. + + ```python + # AVOID + failed_responses.append({"device_id": device.id, "reason": "..."}) + + # DO instead + @dataclass + class FailedResponse: + device_id: int + reason: str + + # Consider static factory methods for repeated reason strings + FailedResponse.for_device_already_has_asset(device_id) + ``` + (MR !3) + +- **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. (MR !3) + +- **DO** name filter/query variables accurately. A misspelled variable like `asset_item_filer` + must be corrected before merge. (MR !21) + +- **DO** order method parameters hierarchically: `tenant_id` before `company_id`, more general + identifiers before more specific ones. Apply consistently across all call sites and signatures. + (MR !21) + +- **AVOID** `datetime.utcnow()` in new code. Use `datetime.now(UTC)` — + `utcnow()` is deprecated in Python 3.12+. (MR !25) + +- **AVOID** broad ruff/mypy ignore rules in `pyproject.toml`. Do not suppress entire error + categories without confirming they are genuinely needed — run `pre-commit run --all-files` + to verify. (MR !25) + +--- + +## API Design + +- **DO** return `404` (not `400` or `500`) when a referenced object is not found on POST and + PATCH endpoints. (MR !13) + +- **DO** keep `_update_fields_from_dto()` methods focused. Extract multi-step sub-operations + (e.g. warehouse update, master_system_id update) into their own dedicated `_update_()` + private methods rather than embedding logic inline. (MR !21) + +- **DO** use the correct auth client factory for each context: + - User token required: `AuthServiceAPIClientFactory.create_user_client(...)` → `AuthorizationServiceAPIClient` + - No authorization: `AuthServiceAPIClientFactory.create_public_client(...)` → `AuthorizationServicePublicAPIClient` + + Do not mix up the two client types. (MR !29) + +- **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 + (MR !29) + +--- + +## Error Handling & Exceptions + +- **DO** narrow `try/except` blocks to only the line(s) that can raise the caught exception. + Keep all subsequent logic outside the `try` block. + + ```python + # AVOID + try: + assignment = await self._get_asset_machine(asset) + if assignment.machine_id is None: + raise AssetItemNotAssigned(asset.id) + if machine.id != assignment.machine_id: + raise AssetItemAssignedToDifferentMachine(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) + if machine.id != assignment.machine_id: + raise AssetItemAssignedToDifferentMachine(asset.id) + ``` + (MR !5) + +- **DO** handle external service unavailability explicitly. Whenever calling an external service + (e.g. fiscal service), add error handling for the unreachable case. (MR !21) + +--- + +## Testing + +- **DO** put manager-level behaviour tests in `test__manager.py`, not + `test__endpoints.py`. Endpoint tests should only exercise code that lives in the + endpoint itself. Manager tests are faster and easier to isolate. (MR !21) + +- **DO** cover each status transition (e.g. `REPARATION → IN_USE`, `operation_status` changes) + with a dedicated test. (MRs !5, !19) + +- **DO** ensure the test DB name in default settings matches the actual local database name used + after a fresh clone. (MR !25) + +--- + +## Architecture & Domain Rules + +- **DO** resolve circular imports using `TYPE_CHECKING` guards and + `from __future__ import annotations`, not by loosening types to `Any`: + + ```python + from __future__ import annotations + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from televend_core.databases.televend_repositories.asset_item.model import AssetItem + from televend_core.databases.televend_repositories.device import Device + ``` + + Use `@dataclass` (stdlib) rather than Pydantic for internal DTOs when forward references + cause Pydantic config errors. (MR !3) + +- **DO** silently create dependent records (e.g. `AssetItemFiscalItalyDevice`) when missing + during an update that requires them. Document this in the method docstring. (MR !22) + +- **DO** validate that `is_fiscal=True` is never set on device types (e.g. banknote acceptors) + that cannot be fiscal. Enforce at the manager layer. (MRs !14, !15) + +- **DO** look up users by email (unique cross-tenant identifier) rather than by `cloud_user_id` + in token-based auth flows. (MR !9) + +--- + +## Git & Process + +- **DO** include the Jira ticket ID in both the branch name and every commit message: + `feature/CLOUD-XXXXX-description`, commit `CLOUD-XXXXX: description`. (all MRs) + +- **DO** add `/.venv` to `.gitignore`. (MR !25) + +- **DO** remove unused or transitively-provided dependencies from `pyproject.toml`. (MR !25) + +- **DO** bump the service version in `pyproject.toml` as part of any dependency-upgrade MR. + (MR !25) + +- **DO** update `README.md` quickstart section when performing a major runtime upgrade + (e.g. Python 3.12 migration). (MR !25) + +- **AVOID** `event_loop` fixture overrides in `conftest.py` — no longer required with modern + `pytest-asyncio`. Remove during any Python 3.12 upgrade. (MR !25) diff --git a/REVIEW_GUIDELINES_cashtrack.md b/REVIEW_GUIDELINES_cashtrack.md new file mode 100644 index 0000000..10312bb --- /dev/null +++ b/REVIEW_GUIDELINES_cashtrack.md @@ -0,0 +1,143 @@ +# Cashtrack Review Guidelines + +Guidelines derived from 61 merged MRs of real code review history. Newer reviews take precedence. + +--- + +## Code Style & Naming + +- **DO** include the token type in field names when multiple token types exist or may be added. + When a field holds only vend tokens, name it accordingly so adding `value_token_diff` later + is non-breaking and unambiguous. + + ```python + # AVOID + token_diff: Decimal + + # DO instead + vend_token_diff: Decimal + ``` + (MR !59) + +- **AVOID** generic `_update_field` helper methods that accept `Any` as a value type. A signature + like `_update_field(name: str, value: Any)` eliminates type-checking. Split into individual + named update methods with proper type signatures: + + ```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: ... + ``` + (MR !40) + +- **DO** keep constants (e.g. filter lists) in a dedicated `const.py` rather than inline in + query or view code. (MR !31) + +- **DO** add schema descriptions for non-obvious query parameters (e.g. `collected_after`). + (MR !31) + +--- + +## API Design + +- **DO** use generics throughout the filter pipeline: + ```python + filter_class: type[FilterBase[T]] + manager_method: FilterManagerMethod[T] + response_class: type[T] + ``` + (MR !16) + +- **DO** default time-bounded endpoints to a sensible range when date params are omitted. + `period_stats` defaults to the last 3 months ending today. Document this in the endpoint + description. (MR !60) + +- **DO** use request body (not query params) for mutation inputs such as a barcode. (MR !2) + +- **DO** enforce input length limits at the API layer to match DB column constraints: + - `note` on collections and conforms: max 500 chars (MR !41) + - `barcode` on cashbag conforms: max 128 chars (MR !42) + +- **DO** restrict collection list endpoints to a default 90-day window. (MR !31) + +- **DO** apply the `cash_room=True` flag when querying conforms inside a cash-room context. + (MR !40) + +--- + +## Error Handling & Exceptions + +- **DO** validate denomination payloads before persisting: + - Reject if all denominations and token values are zero. (MR !45) + - Reject if the same denomination appears more than once. (MR !51) + - Validate quantities against numeric DB constraints before hitting the DB. (MR !50) + +- **DO** enforce that exactly one of two mutually exclusive FKs is set (e.g. either `conform_id` + or `collection_id`, never both, never neither). Consider a DB-level `CHECK` constraint. (MR !52) + +- **DO** skip unknown `changed_field` values in the changelog endpoint rather than raising. + (MR !54) + +- **DO** treat `null` collection status as `open`. (MR !49) + +--- + +## Testing + +- **DO** write tests in the same MR as the feature, not in a follow-up. + +- **DO** cover filter behaviour at the endpoint level, including interactions with data-access + restrictions. (MRs !55, !48) + +- **DO** include a changelog entry in every MR that adds or changes user-facing behaviour. + (MR !59) + +--- + +## Architecture & Domain Rules + +- **DO** split query functions so each returns exactly one result shape. A function that computes + two unrelated shapes should be two functions. (MR !59) + +- **DO** always filter conform queries with `CashBagConform.alive = True`. This is the soft-delete + sentinel; omitting it causes deleted conforms to leak into results. Verify the `alive` condition + is present whenever you add a new join through the conform table. (MR !57) + +- **DO** use soft-delete (`alive` flag) for denominations. Cascade delete was removed (MR !39) + after soft-delete was introduced (MR !36) to preserve historical counting records. + +- **DO** reuse an existing conform on import rather than always inserting a new one. Look up by + natural key (barcode + cashroom) before creating. (MR !63) + +- **DO** recalculate `total_coins` / `total_bills` from denominations whenever denominations are + updated — never accept totals directly from the caller. (MR !31) + +- **DO** place manager classes in a `managers/` sub-folder, separate from endpoints and queries. + (MR !7) + +- **DO** attach `cashroom_id` to cashbag conform records for proper cash-room scoping. (MR !34) + +- **DO** wire data-access (company + user) restrictions into every new list or filter endpoint + before the MR is ready for review. (MRs !48, !55) + +--- + +## Git & Process + +- **DO** use branch names `feature/CLOUD-XXXXX-short-description` for all work targeting + `develop`. + +- **DO** title MRs as `CLOUD-XXXXX: description` (imperative). Avoid the "Resolve" prefix. + (MR !64) + +- **DO** keep MRs focused on a single ticket. Preliminary or follow-up changes belong in their + own MR. (MRs !59, !60 pattern) + +- **DO** resolve all open review threads before merging. (MRs !59, !16) + +- **DO** ensure ruff passes before requesting review. A cleanup-only "Ruff is happy" MR is a + sign that formatting discipline slipped. (MRs !61, !62) diff --git a/REVIEW_GUIDELINES_general.md b/REVIEW_GUIDELINES_general.md new file mode 100644 index 0000000..ba9897e --- /dev/null +++ b/REVIEW_GUIDELINES_general.md @@ -0,0 +1,439 @@ +# 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__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)* diff --git a/REVIEW_GUIDELINES_planogrammer.md b/REVIEW_GUIDELINES_planogrammer.md new file mode 100644 index 0000000..afdcded --- /dev/null +++ b/REVIEW_GUIDELINES_planogrammer.md @@ -0,0 +1,336 @@ +# 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)