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

# 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_<field>() 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.

# 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_<domain>_manager.py, not test_<domain>_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:

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)

6.3 KiB Raw Permalink Blame History