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

# 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:

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)

4.9 KiB Raw Permalink Blame History