Code Conclave Findings
Covers: ARCHITECT-001, ARCHITECT-002, ARCHITECT-003, NAVIGATOR-002
Problem
backend/app/services/customer_service.py has accumulated significant tech debt across multiple dimensions:
-
ARCHITECT-001 — God file / single-responsibility violation: The file handles CRUD, bulk import, CSV parsing, validation, preview generation, and sequence number generation all in one module. It should be decomposed into focused sub-modules (e.g., customer_import_service.py, customer_validation.py).
-
ARCHITECT-002 — Inconsistent error handling: Mix of raising exceptions, returning error dicts, and silently swallowing errors. Some functions return None on failure while others raise HTTPException. No consistent pattern for callers to rely on.
-
ARCHITECT-003 — Transaction management: Multiple db.commit() calls within single logical operations create non-atomic behavior. Bulk import is particularly affected — a failure partway through leaves partial data committed. Should use savepoints (db.begin_nested()) for rollback safety.
-
NAVIGATOR-002 — Import preview truncation: The bulk import preview silently truncates results without informing the user. If a CSV has 500 rows but preview shows 50, the user has no indication that 450 rows were not validated. Preview and actual import also use different validation scopes (e.g., email uniqueness checks differ).
Priority
Medium — Pre-existing tech debt, not a regression. No user-facing bugs, but increases risk of future bugs and makes the code harder to maintain.
Suggested Approach
- Extract CSV parsing/validation into
customer_import_service.py
- Standardize error handling to always raise typed exceptions (not HTTP exceptions from service layer)
- Wrap bulk operations in savepoints
- Add truncation indicator to preview response and align validation between preview and import
Code Conclave Findings
Covers: ARCHITECT-001, ARCHITECT-002, ARCHITECT-003, NAVIGATOR-002
Problem
backend/app/services/customer_service.pyhas accumulated significant tech debt across multiple dimensions:ARCHITECT-001 — God file / single-responsibility violation: The file handles CRUD, bulk import, CSV parsing, validation, preview generation, and sequence number generation all in one module. It should be decomposed into focused sub-modules (e.g.,
customer_import_service.py,customer_validation.py).ARCHITECT-002 — Inconsistent error handling: Mix of raising exceptions, returning error dicts, and silently swallowing errors. Some functions return
Noneon failure while others raiseHTTPException. No consistent pattern for callers to rely on.ARCHITECT-003 — Transaction management: Multiple
db.commit()calls within single logical operations create non-atomic behavior. Bulk import is particularly affected — a failure partway through leaves partial data committed. Should use savepoints (db.begin_nested()) for rollback safety.NAVIGATOR-002 — Import preview truncation: The bulk import preview silently truncates results without informing the user. If a CSV has 500 rows but preview shows 50, the user has no indication that 450 rows were not validated. Preview and actual import also use different validation scopes (e.g., email uniqueness checks differ).
Priority
Medium — Pre-existing tech debt, not a regression. No user-facing bugs, but increases risk of future bugs and makes the code harder to maintain.
Suggested Approach
customer_import_service.py