diff --git a/alembic/versions/m1_a_property_org_not_null_backfill.py b/alembic/versions/m1_a_property_org_not_null_backfill.py new file mode 100644 index 0000000..0641b8e --- /dev/null +++ b/alembic/versions/m1_a_property_org_not_null_backfill.py @@ -0,0 +1,227 @@ +"""PASAY-MILESTONE-001: Property.organization_id backfill + NOT NULL harden (P1). + +Revision ID: m1a000000001 +Revises: b5c6d7e8f9a0 +Create Date: 2026-08-21 + +Owner contract: + 1. Legacy NULL Property.organization_id rows are auto-backfilled ONLY when the + ownership is UNIQUE AND UNAMBIGUOUS. + 2. If multiple Organizations exist and ownership cannot be uniquely determined, + FAIL CLOSED with specific Property IDs and candidate Organizations. + 3. NEVER take the first organization (oldest/newest/smallest id) guesswork IS forbidden. + +Backfill rules (deterministic, ordered by priority): + A. If organizations table has exactly 1 row -> backfill all NULL rows to that id. + B. If 0 rows -> FAIL CLOSED (no org to backfill to). + C. If >1 rows: for every NULL property_id we try Property.created_by -> + Membership ACTIVE -> organization_id. If that resolves to exactly 1 org id + across all created_by memberships of the property, backfill it. Otherwise list + the property_id together with all candidate orgs and FAIL CLOSED. + +Downgrade safety (Alembic downgrade MUST preserve column semantics): + - Before dropping NOT NULL we verify organization_id is still BIGINT + no type drift + via `sa.inspect` — otherwise refuse downgrade (FAIL CLOSED). + - The column remains BIGINT REFERENCES organizations(id) (nullable after downgrade). +""" +from __future__ import annotations + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.engine import Connection + +revision: str = "m1a000000001" +down_revision: str | None = "b5c6d7e8f9a0" +branch_labels = None +depends_on = None + + +class _AmbiguousBackfill(RuntimeError): + """Raised when legacy data cannot be uniquely backfilled (Owner rule #2).""" + + +# --------------------------------------------------------------------------- +# UPGRADE helpers +# --------------------------------------------------------------------------- + + +def _gather_null_property_ids_without_org(conn: Connection) -> list[int]: + rows = conn.execute( + sa.text( + "SELECT p.id FROM properties p " + "WHERE p.organization_id IS NULL " + "ORDER BY p.id" + ) + ).all() + return [r[0] for r in rows] + + +def _count_organizations(conn: Connection) -> int: + return conn.execute(sa.text("SELECT COUNT(*) FROM organizations")).scalar() or 0 + + +def _only_org_id(conn: Connection) -> int: + row = conn.execute(sa.text("SELECT id FROM organizations ORDER BY id LIMIT 1")).one() + return row[0] + + +def _backfill_with_org_id(conn: Connection, org_id: int, target_ids: list[int]) -> None: + if not target_ids: + return + chunk_size = 500 + for i in range(0, len(target_ids), chunk_size): + chunk = target_ids[i : i + chunk_size] + conn.execute( + sa.text( + "UPDATE properties SET organization_id = :org WHERE id = ANY(:ids)" + ), + {"org": org_id, "ids": chunk}, + ) + + +def _resolve_created_by_org(conn: Connection, property_ids: list[int]) -> dict[int, list[int]]: + """For each NULL property, collect DISTINCT org_ids reachable via + property.created_by -> membership ACTIVE. Returns + {property_id: [sorted candidate org_ids]}. + """ + result: dict[int, list[int]] = {} + for pid in property_ids: + rows = conn.execute( + sa.text( + "SELECT DISTINCT m.organization_id " + "FROM properties p " + "JOIN memberships m ON m.user_id = p.created_by " + "WHERE p.id = :pid AND m.state = 'ACTIVE' " + "AND m.removed_at IS NULL " + "ORDER BY m.organization_id" + ), + {"pid": pid}, + ).all() + result[pid] = sorted(r[0] for r in rows) + return result + + +def _organizations_lookup(conn: Connection) -> dict[int, str]: + rows = conn.execute(sa.text("SELECT id, name FROM organizations ORDER BY id")).all() + return {r[0]: r[1] for r in rows} + + +def _fail_closed_ambiguous(conn: Connection, ambiguous: dict[int, list[int]]) -> None: + """Abort the upgrade deterministically with a deterministic error report for Owner review.""" + org_names = _organizations_lookup(conn) + lines = [ + "PASAY-MILESTONE-001 FAIL CLOSED: Property.org_id backfill is AMBIGUOUS for the", + "following legacy properties. Owner must decide their org manually.", + ] + for pid, candidates in sorted(ambiguous.items()): + cand_repr = ", ".join( + f"org_id={c} ({org_names.get(c, 'UNKNOWN')})" for c in candidates + ) or "NO candidates (0 ACTIVE membership for created_by)" + lines.append(f" - property_id={pid} candidates=[{cand_repr}]") + lines.append("Candidates Organizations (known):") + for oid, oname in sorted(org_names.items()): + lines.append(f" - org_id={oid} name={oname!r}") + raise _AmbiguousBackfill("\n".join(lines)) + + +def upgrade() -> None: + conn = op.get_bind() + null_ids = _gather_null_property_ids_without_org(conn) + if not null_ids: + # Nothing to backfill — directly tighten NOT NULL. + pass + else: + org_count = _count_organizations(conn) + if org_count == 0: + # No orgs exist at all: this is either a fresh empty test DB with + # zero legacy data (Base.metadata.create_all path). In that case the + # properties rows wouldn't exist either → this case is actually empty set, + # should not reach here because null_ids=[]. If it does reach here fail + # deterministically: + _fail_closed_ambiguous(conn, {pid: [] for pid in null_ids}) + if org_count == 1: + # Case A: exactly one organization → auto-backfill deterministically. + _backfill_with_org_id(conn, _only_org_id(conn), null_ids) + else: + # Case C: >1 org → created_by membership resolution, per-property. + resolved = _resolve_created_by_org(conn, null_ids) + ambiguous: dict[int, list[int]] = {} + can_backfill: dict[int, int] = {} + for pid, orgs in resolved.items(): + if len(orgs) == 1: + can_backfill[pid] = orgs[0] + else: + ambiguous[pid] = orgs + if ambiguous: + _fail_closed_ambiguous(conn, ambiguous) + for org_id in sorted(set(can_backfill.values())): + ids_for_org = sorted(p for p, o in can_backfill.items() if o == org_id) + _backfill_with_org_id(conn, org_id, ids_for_org) + # -------------------- tighten NOT NULL constraint -------------------- + op.alter_column( + "properties", + "organization_id", + existing_type=sa.BigInteger(), + existing_nullable=True, + nullable=False, + ) + + +# --------------------------------------------------------------------------- +# DOWNGRADE (safe with sa.inspect audit gate) +# --------------------------------------------------------------------------- + + +def downgrade() -> None: + conn = op.get_bind() + insp = sa.inspect(conn) + cols = {c["name"]: c for c in insp.get_columns("properties")} + col = cols.get("organization_id") + if col is None: + raise RuntimeError( + "PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + "column properties.organization_id not present " + "(schema drift; refusing downgrade)." + ) + col_type = col.get("type") + # Must be BIGINT-compatible; NOT a different scalar type. + type_name = type(col_type).__name__ + if "BIGINT" not in type_name.upper() and "BigInteger" not in type_name: + try: + compiled = str(col_type.compile(dialect=conn.dialect)) + except Exception: # noqa: BLE001 + import logging + + logging.getLogger("alembic.runtime.migration").warning( + "cannot compile organization_id column type in downgrade audit", exc_info=True + ) + compiled = "" + if "BIGINT" not in compiled.upper(): + raise RuntimeError( + f"PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + f"properties.organization_id type drifted to {type_name!r} " + f"(compiled={compiled!r}); expected BIGINT. " + f"Refusing downgrade to prevent numeric semantic loss." + ) + # Foreign key sanity: the column must still reference organizations(id). + fks = insp.get_foreign_keys("properties") + fk_match = any( + set(fk.get("constrained_columns") or []) == {"organization_id"} + and (fk.get("referred_table") == "organizations") + and set(fk.get("referred_columns") or []) == {"id"} + for fk in fks + ) + if not fk_match: + raise RuntimeError( + "PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + "FK properties.organization_id -> organizations(id) " + "drifted or missing; refusing downgrade." + ) + # Semantics audit passed; relax NOT NULL. + op.alter_column( + "properties", + "organization_id", + existing_type=sa.BigInteger(), + existing_nullable=False, + nullable=True, + ) diff --git a/alembic/versions/m1_b_tenant_add_organization_id.py b/alembic/versions/m1_b_tenant_add_organization_id.py new file mode 100644 index 0000000..7f6bfcb --- /dev/null +++ b/alembic/versions/m1_b_tenant_add_organization_id.py @@ -0,0 +1,206 @@ +"""PASAY-MILESTONE-001: Tenant.organization_id NOT NULL + backfill (P2). + +Revision ID: m1b000000001 +Revises: m1a000000001 +Create Date: 2026-08-21 + +Owner contract: + - Tenant.organization_id NOT NULL. + - Legacy Tenant inferred via Lease -> Unit -> Property -> Organization chain. + - UNIQUE inference or FAIL CLOSED. Guessing forbidden. + +Downgrade safety: + - sa.inspect must confirm column: BIGINT REFERENCES organizations(id), + type drift/missing FK -> refuse downgrade. + - Index removal verified by sa.inspect existence too. +""" +from __future__ import annotations + +import logging + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.engine import Connection + +logger = logging.getLogger("alembic.runtime.migration") + +revision: str = "m1b000000001" +down_revision: str | None = "m1a000000001" +branch_labels = None +depends_on = None + + +class _AmbiguousTenantBackfill(RuntimeError): + pass + + +def _list_all_tenant_ids(conn: Connection) -> list[int]: + rows = conn.execute( + sa.text("SELECT t.id FROM tenants t ORDER BY t.id") + ).all() + return [r[0] for r in rows] + + +def _infer_tenant_orgs(conn: Connection, tenant_ids: list[int]) -> dict[int, list[int]]: + """For every tenant_id, collect all DISTINCT org_ids via + tenant -> Lease -> Unit -> Property.org. If tenant has 0 leases -> empty. + + All rows (including soft-deleted Tenants / Leases / Units / Properties) + enter resolution because the final column is NOT NULL for the entire + table; ambiguous legacy soft-deleted facts still fail closed. + """ + out: dict[int, list[int]] = {tid: [] for tid in tenant_ids} + rows = conn.execute( + sa.text( + "SELECT DISTINCT t.id AS tenant_id, pr.organization_id " + "FROM tenants t " + "JOIN leases l ON l.tenant_id = t.id " + "JOIN units u ON u.id = l.unit_id " + "JOIN properties pr ON pr.id = u.property_id " + "WHERE pr.organization_id IS NOT NULL " + "ORDER BY t.id, pr.organization_id" + ) + ).all() + for tid, oid in rows: + if tid in out and oid not in out[tid]: + out[tid].append(oid) + return out + + +def _backfill_tenant_orgs(conn: Connection, backfill_map: dict[int, int]) -> None: + if not backfill_map: + return + chunk_size = 500 + by_org: dict[int, list[int]] = {} + for tid, oid in sorted(backfill_map.items()): + by_org.setdefault(oid, []).append(tid) + stmt = sa.text( + "UPDATE tenants SET organization_id = :oid WHERE id IN :ids" + ).bindparams(sa.bindparam("ids", expanding=True)) + for oid, ids in sorted(by_org.items()): + for i in range(0, len(ids), chunk_size): + conn.execute(stmt, {"oid": oid, "ids": ids[i : i + chunk_size]}) + + +def _organizations_lookup(conn: Connection) -> dict[int, str]: + rows = conn.execute(sa.text("SELECT id, name FROM organizations ORDER BY id")).all() + return {r[0]: r[1] for r in rows} + + +def _fail_closed(conn: Connection, bad: dict[int, list[int]]) -> None: + names = _organizations_lookup(conn) + lines = [ + "PASAY-MILESTONE-001 FAIL CLOSED: Tenant.organization_id backfill is " + "AMBIGUOUS for the following legacy tenants. Owner must decide manually." + ] + for tid, orgs in sorted(bad.items()): + cand = ", ".join(f"org_id={o} ({names.get(o,'UNKNOWN')})" for o in orgs) or ( + "NO candidates (tenant has 0 leases or all leases/org NULL)." + ) + lines.append(f" - tenant_id={tid} candidates=[{cand}]") + lines.append("All candidate Organizations:") + for oid, oname in sorted(names.items()): + lines.append(f" - org_id={oid} name={oname!r}") + raise _AmbiguousTenantBackfill("\n".join(lines)) + + +def upgrade() -> None: + conn = op.get_bind() + # (1) Add column BIGINT NULL + FK + op.add_column( + "tenants", + sa.Column( + "organization_id", + sa.BigInteger(), + sa.ForeignKey("organizations.id"), + nullable=True, + ), + ) + # (2) Create index explicitly (op.add_column ignores Column(index=True)). + op.create_index( + "ix_tenants_organization_id", "tenants", ["organization_id"] + ) + # (3) Backfill via Lease->Unit->Property + tids = _list_all_tenant_ids(conn) + if tids: + inferred = _infer_tenant_orgs(conn, tids) + ambiguous: dict[int, list[int]] = {} + backfill: dict[int, int] = {} + for tid, orgs in inferred.items(): + if len(orgs) == 1: + backfill[tid] = orgs[0] + else: + ambiguous[tid] = orgs + if ambiguous: + _fail_closed(conn, ambiguous) + _backfill_tenant_orgs(conn, backfill) + # (4) Enforce NOT NULL + op.alter_column( + "tenants", + "organization_id", + existing_type=sa.BigInteger(), + existing_nullable=True, + nullable=False, + ) + + +def downgrade() -> None: + conn = op.get_bind() + insp = sa.inspect(conn) + cols = {c["name"]: c for c in insp.get_columns("tenants")} + col = cols.get("organization_id") + if col is None: + raise RuntimeError( + "PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + "tenants.organization_id already dropped (schema drift)." + ) + tname = type(col["type"]).__name__ + compiled = "" + try: + compiled = str(col["type"].compile(dialect=conn.dialect)) + except Exception: # noqa: BLE001 + logger.warning("cannot compile tenants.organization_id type", exc_info=True) + if "BIGINT" not in tname.upper() and "BigInteger" not in tname and "BIGINT" not in compiled.upper(): + raise RuntimeError( + f"PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + f"tenants.organization_id type drifted to {tname!r} (compiled={compiled!r})." + ) + fks = insp.get_foreign_keys("tenants") + matches = any( + set(f.get("constrained_columns") or []) == {"organization_id"} + and f.get("referred_table") == "organizations" + and set(f.get("referred_columns") or []) == {"id"} + for f in fks + ) + if not matches: + # In some DB introspections the FK name may have auto-named; also + # accept if ANY FK carries organization_id -> organizations.id + any_match = False + for f in fks: + if "organization_id" in (f.get("constrained_columns") or []): + if f.get("referred_table") == "organizations": + any_match = True + break + if not any_match: + raise RuntimeError( + "PASAY-MILESTONE-001 downgrade FAIL CLOSED: " + "FK tenants.organization_id -> organizations(id) missing." + ) + # Drop index FIRST (before dropping column; column drop cascades index drop). + # IF EXISTS already covers missing-index case. Any REAL DDL failure (e.g. lock + # timeout, permission denied, broken storage, unknown catalog error) MUST + # propagate and stop downgrade (FAIL CLOSED — never catch-and-continue). + conn = op.get_bind() + conn.execute( + sa.text("DROP INDEX IF EXISTS ix_tenants_organization_id") + ) + # Relax NOT NULL -> NULL + op.alter_column( + "tenants", + "organization_id", + existing_type=sa.BigInteger(), + existing_nullable=False, + nullable=True, + ) + # Drop column + op.drop_column("tenants", "organization_id") diff --git a/alembic/versions/m1_c_expense_add_property_id.py b/alembic/versions/m1_c_expense_add_property_id.py new file mode 100644 index 0000000..b4df251 --- /dev/null +++ b/alembic/versions/m1_c_expense_add_property_id.py @@ -0,0 +1,345 @@ +"""PASAY-MILESTONE-001 FIX1: Expense.property_id NOT NULL + fail-closed backfill (m1c). + +Owner Blocker #4: 业务能力回退 — Expense 原本允许 unit_id nullable (building / property +level expenses). M1 通过 inner join Unit 强制 scoped，导致全部历史 unitless Expense +从 scoped list/get 消失，且新 create 拒绝“Expense must be associated with a unit”。 + +本迁移策略（严格遵循 Owner §3 + §4：不冗余 organization_id，只引入 property_id 作为 +building-level canonical ownership 锚点）： + + 1. ALTER TABLE expenses ADD COLUMN property_id BIGINT NULL REFERENCES properties(id). + 2. FAIL-CLOSED backfill 每一条 expenses： + A. 若 unit_id NOT NULL → 直接解析 unit.property_id 填充（unit 仍存在则必定 + 唯一；unit 已被删除 / orphan → 进入 ambiguous）。 + B. 若 unit_id NULL → 按 created_by → ACTIVE memberships 解析，若跨多 + Organization 或 0 候选 → FAIL CLOSED + 列出具体 expense_id。 + 3. 回填完成后 ALTER COLUMN property_id SET NOT NULL + CREATE INDEX。 + +Downgrade safety: + - sa.inspect 审计 property_id 类型为 BIGINT + FK(properties.id)；漂移则拒绝。 + - 先 DROP INDEX → relax NOT NULL → DROP COLUMN。 + +Revision ID: m1c000000001 +Revises: m1b000000001 +Create Date: 2026-08-21 +""" +from __future__ import annotations + +import logging + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.engine import Connection + +logger = logging.getLogger("alembic.runtime.migration") + +revision: str = "m1c000000001" +down_revision: str | None = "m1b000000001" +branch_labels = None +depends_on = None + + +class _AmbiguousExpenseBackfill(RuntimeError): + pass + + +# --------------------------------------------------------------------------- +# UPGRADE helpers +# --------------------------------------------------------------------------- + + +def _list_all_expense_ids(conn: Connection) -> list[int]: + rows = conn.execute( + sa.text("SELECT e.id FROM expenses e ORDER BY e.id") + ).all() + return [r[0] for r in rows] + + +def _resolve_unit_property(conn: Connection, expense_ids: list[int]) -> dict[int, int | None]: + """For each expense, if unit_id is set AND unit still exists, resolve unit.property_id. + Returns {expense_id: pid | None} where None means unit_id NULL or orphan unit. + """ + result: dict[int, int | None] = {eid: None for eid in expense_ids} + rows = conn.execute( + sa.text( + "SELECT e.id, u.property_id " + "FROM expenses e " + "JOIN units u ON u.id = e.unit_id " + "WHERE e.id = ANY(:ids)" + ), + {"ids": expense_ids}, + ).all() + for eid, pid in rows: + result[eid] = pid + return result + + +def _resolve_created_by_orgs(conn: Connection, expense_ids: list[int]) -> dict[int, list[int]]: + """Fallback step 1 for expenses with no resolvable unit → created_by user's ACTIVE memberships. + Returns {expense_id: [sorted candidate org_ids]}. + """ + result: dict[int, list[int]] = {eid: [] for eid in expense_ids} + for eid in expense_ids: + rows = conn.execute( + sa.text( + "SELECT DISTINCT m.organization_id " + "FROM expenses e " + "JOIN memberships m ON m.user_id = e.created_by " + "WHERE e.id = :eid AND m.state = 'ACTIVE' AND m.removed_at IS NULL " + "ORDER BY m.organization_id" + ), + {"eid": eid}, + ).all() + result[eid] = sorted(r[0] for r in rows) + return result + + +def _resolve_org_unique_property( + conn: Connection, org_ids_by_expense: dict[int, list[int]] +) -> dict[int, list[int]]: + """Fallback step 2: given candidate org_ids per expense, resolve each org's active + (non-deleted) Properties. For an expense to be unitless-backfillable, it must: + (a) have exactly 1 candidate org (already filtered upstream), AND + (b) that org has exactly 1 active (deleted_at IS NULL) Property. + The Property IDs are in a DIFFERENT namespace from org IDs; mixing is forbidden. + Returns {expense_id: [sorted candidate property_ids]}. If the expense has 0 or >1 + active properties → ambiguous → stay empty in FAIL CLOSED. + """ + result: dict[int, list[int]] = {} + all_orgs: set[int] = set() + for ids in org_ids_by_expense.values(): + all_orgs.update(ids) + org_to_props: dict[int, list[int]] = {} + if all_orgs: + rows = conn.execute( + sa.text( + "SELECT p.organization_id, p.id " + "FROM properties p " + "WHERE p.organization_id = ANY(:orgs) AND p.deleted_at IS NULL " + "ORDER BY p.organization_id, p.id" + ), + {"orgs": sorted(all_orgs)}, + ).all() + for oid, pid in rows: + org_to_props.setdefault(oid, []).append(pid) + for eid, org_ids in org_ids_by_expense.items(): + if len(org_ids) != 1: + result[eid] = [] + continue + only_org = org_ids[0] + props = sorted(org_to_props.get(only_org, [])) + result[eid] = props + return result + + +def _backfill_expenses(conn: Connection, backfill_map: dict[int, int]) -> None: + if not backfill_map: + return + chunk_size = 500 + by_property: dict[int, list[int]] = {} + for eid, pid in sorted(backfill_map.items()): + by_property.setdefault(pid, []).append(eid) + stmt = sa.text( + "UPDATE expenses SET property_id = :pid WHERE id IN :ids" + ).bindparams(sa.bindparam("ids", expanding=True)) + for pid, ids in sorted(by_property.items()): + for i in range(0, len(ids), chunk_size): + conn.execute(stmt, {"pid": pid, "ids": ids[i : i + chunk_size]}) + + +def _organizations_lookup(conn: Connection) -> dict[int, str]: + rows = conn.execute(sa.text("SELECT id, name FROM organizations ORDER BY id")).all() + return {r[0]: r[1] for r in rows} + + +def _properties_lookup(conn: Connection) -> dict[int, tuple[int, str]]: + rows = conn.execute( + sa.text( + "SELECT p.id, p.organization_id, p.name FROM properties p ORDER BY p.id" + ) + ).all() + return {r[0]: (r[1], r[2]) for r in rows} + + +def _fail_closed( + conn: Connection, + bad: dict[int, tuple[list[int], list[int]]], +) -> None: + """bad map: {expense_id: (candidate_org_ids, candidate_property_ids)}. + candidate_org_ids is from step-1 membership resolution; candidate_property_ids + is from step-2 org→active-properties resolution. Both are always printed so + Owner has full forensic context of WHY backfill is ambiguous. + """ + org_names = _organizations_lookup(conn) + prop_info = _properties_lookup(conn) + lines = [ + "PASAY-MILESTONE-001 FIX2 FAIL CLOSED: Expense.property_id backfill is AMBIGUOUS " + "for the following unitless/orphan-unit legacy expenses. Owner must decide manually. " + "CRITICAL: org_id and property_id are SEPARATE namespaces; we NEVER write org_id " + "into the property_id column, even if numeric values accidentally overlap." + ] + for eid, (orgs, props) in sorted(bad.items()): + org_cand = ", ".join( + f"org_id={o} ({org_names.get(o, 'UNKNOWN')})" for o in orgs + ) or ( + "NO org candidates (expense has unit_id NULL and created_by has 0 ACTIVE memberships)." + ) + prop_cand_fragments = [] + for pid in props: + p_tuple = prop_info.get(pid) + if p_tuple is None: + prop_cand_fragments.append(f"property_id={pid} (NOT IN properties table)") + else: + p_org, p_name = p_tuple + prop_cand_fragments.append( + f"property_id={pid} (org_id={p_org} name={p_name!r})" + ) + prop_cand = ", ".join(prop_cand_fragments) or ( + "NO property candidates (resolved org has 0 active properties, OR 0 orgs resolved)." + ) + lines.append(f" - expense_id={eid}") + lines.append(f" step-1 org candidates: [{org_cand}]") + lines.append(f" step-2 prop candidates: [{prop_cand}]") + lines.append("All candidate Organizations:") + for oid, oname in sorted(org_names.items()): + lines.append(f" - org_id={oid} name={oname!r}") + lines.append("All candidate Properties (for forensic cross-reference):") + for pid, (p_org, p_name) in sorted(prop_info.items()): + lines.append( + f" - property_id={pid} org_id={p_org} (org={org_names.get(p_org, 'UNKNOWN')!r}) name={p_name!r}" + ) + raise _AmbiguousExpenseBackfill("\n".join(lines)) + + +# --------------------------------------------------------------------------- +# UPGRADE +# --------------------------------------------------------------------------- + + +def upgrade() -> None: + conn = op.get_bind() + # (1) Add nullable column + FK + op.add_column( + "expenses", + sa.Column( + "property_id", + sa.BigInteger(), + sa.ForeignKey("properties.id"), + nullable=True, + ), + ) + # (2) Backfill — STRICT namespace separation. + # Stage A: expenses WITH a live unit → use unit.property_id (DIFFERENT namespace!) + # Stage B: unitless/orphan-unit expenses → step-1 membership→org candidates; + # step-2 org→active properties. Only accept EXACTLY 1 property. + # Org ID is NEVER written into property_id column. + eids = _list_all_expense_ids(conn) + if eids: + unit_pids = _resolve_unit_property(conn, eids) + needs_created_by_fallback: list[int] = [ + eid for eid, pid in unit_pids.items() if pid is None + ] + created_by_orgs: dict[int, list[int]] = ( + _resolve_created_by_orgs(conn, needs_created_by_fallback) + if needs_created_by_fallback + else {} + ) + # Step-2: for each expense's candidate-orgs list, resolve to active property IDs. + candidate_props_by_expense: dict[int, list[int]] = ( + _resolve_org_unique_property(conn, created_by_orgs) + if created_by_orgs + else {} + ) + backfill: dict[int, int] = {} + # Ambiguous map now carries BOTH org candidates AND property candidates for + # forensic context (owner can see WHY it failed). + ambiguous: dict[int, tuple[list[int], list[int]]] = {} + for eid in eids: + pid = unit_pids.get(eid) + if pid is not None: + # Confirmed valid property ID from unit→property chain. + # This PID is already the CORRECT namespace, NEVER an org ID. + backfill[eid] = pid + continue + orgs = created_by_orgs.get(eid, []) + props = candidate_props_by_expense.get(eid, []) + if len(props) == 1: + # EXACTLY one active property under the (exactly-1) resolved org. + # props[0] is a bona fide property ID from the properties table, + # NEVER an org ID. Fail closed on 0 or >=2. + backfill[eid] = props[0] + else: + ambiguous[eid] = (orgs, props) + if ambiguous: + _fail_closed(conn, ambiguous) + _backfill_expenses(conn, backfill) + # (3) Enforce NOT NULL + Index + op.alter_column( + "expenses", + "property_id", + existing_type=sa.BigInteger(), + existing_nullable=True, + nullable=False, + ) + op.create_index("ix_expenses_property_id", "expenses", ["property_id"]) + + +# --------------------------------------------------------------------------- +# DOWNGRADE +# --------------------------------------------------------------------------- + + +def downgrade() -> None: + conn = op.get_bind() + insp = sa.inspect(conn) + cols = {c["name"]: c for c in insp.get_columns("expenses")} + col = cols.get("property_id") + if col is None: + raise RuntimeError( + "PASAY-MILESTONE-001 FIX1 downgrade FAIL CLOSED: " + "expenses.property_id not present (schema drift)." + ) + tname = type(col["type"]).__name__ + compiled = "" + try: + compiled = str(col["type"].compile(dialect=conn.dialect)) + except Exception: # noqa: BLE001 + logger.warning("cannot compile expenses.property_id type", exc_info=True) + if "BIGINT" not in tname.upper() and "BigInteger" not in tname and "BIGINT" not in compiled.upper(): + raise RuntimeError( + f"PASAY-MILESTONE-001 FIX1 downgrade FAIL CLOSED: " + f"expenses.property_id type drifted to {tname!r} (compiled={compiled!r})." + ) + fks = insp.get_foreign_keys("expenses") + matches = any( + set(f.get("constrained_columns") or []) == {"property_id"} + and f.get("referred_table") == "properties" + and set(f.get("referred_columns") or []) == {"id"} + for f in fks + ) + any_match = False + if not matches: + for f in fks: + if "property_id" in (f.get("constrained_columns") or []): + if f.get("referred_table") == "properties": + any_match = True + break + if not matches and not any_match: + raise RuntimeError( + "PASAY-MILESTONE-001 FIX1 downgrade FAIL CLOSED: " + "FK expenses.property_id -> properties(id) missing." + ) + # Drop index FIRST (IF EXISTS via raw SQL, avoid cascade surprises). + # IF EXISTS already covers the missing-index case. Real DDL failures must + # propagate and halt downgrade (FAIL CLOSED — never catch-and-continue). + conn.execute(sa.text("DROP INDEX IF EXISTS ix_expenses_property_id")) + # Relax NOT NULL -> NULL + op.alter_column( + "expenses", + "property_id", + existing_type=sa.BigInteger(), + existing_nullable=False, + nullable=True, + ) + # Drop column + op.drop_column("expenses", "property_id") diff --git a/app/api/routers/expense.py b/app/api/routers/expense.py index 2f35b99..6092776 100644 --- a/app/api/routers/expense.py +++ b/app/api/routers/expense.py @@ -1,20 +1,19 @@ from datetime import datetime, timezone -from decimal import Decimal from fastapi import APIRouter, Depends, HTTPException, status from sqlalchemy import func, update from sqlalchemy.orm import Session -from app.api.deps import admin_only, get_current_user, manager_or_admin +from app.api.deps import get_current_user from app.database import get_db -from app.models.expense_claim import ClaimStatus, ExpensePaymentClaim +from app.models.expense_claim import ClaimStatus from app.models.financial import Expense, ExpenseStatus from app.models.operations import ( OperationalTask, OperationalTaskStatus, OperationalTaskType, ) -from app.models.property import Unit +from app.models.property import Property, Unit from app.models.user import User from app.schemas.financial import ( ExpenseCreate, @@ -37,31 +36,34 @@ expense_finance_payload, payment_truth, clear_approval, - sync_expense_status, ) from app.services.expense_timeline import build_expense_timeline from app.services.operations import projection as task_projection +from app.services.organization_scope import ( + CrossOrgReference, + OrganizationRole, + OwnerRequired, + ScopeBlocked, + assert_co_org, + property_org_id, + resolve_org_membership, + scope_exception_to_http, + scoped_get_expense, + scoped_list_expenses, + unit_org_id, +) router = APIRouter(prefix="/expenses", tags=["expenses"]) _CREATABLE = {ExpenseStatus.pending, ExpenseStatus.approved} -# Critical financial fields (PASAY-EXPENSE-OPERATION-003B §9): changing any of -# these after approval invalidates the old approval and requires re-approval. -_CRITICAL_FIELDS = ("amount", "payee", "category", "description", "unit_id", "payer_user_id") +_CRITICAL_FIELDS = ("amount", "payee", "category", "description", "unit_id", "property_id", "payer_user_id") class _ResubmitPayload(ExpenseUpdate): idempotency_key: str | None = None -def _get_or_404(db: Session, expense_id: int) -> Expense: - obj = db.query(Expense).filter(Expense.id == expense_id).first() - if obj is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Expense not found") - return obj - - def _check_unit(db: Session, unit_id: int | None) -> None: if unit_id is None: return @@ -74,9 +76,57 @@ def _check_unit(db: Session, unit_id: int | None) -> None: raise HTTPException(status.HTTP_404_NOT_FOUND, "Unit not found") +def _check_property(db: Session, property_id: int | None) -> None: + if property_id is None: + return + prop = ( + db.query(Property) + .filter(Property.id == property_id, Property.deleted_at.is_(None)) + .first() + ) + if prop is None: + raise HTTPException(status.HTTP_404_NOT_FOUND, "Property not found") + + +def _resolve_expense_property_id(db: Session, unit_id: int | None, property_id: int | None) -> int: + """Resolve the final expense.property_id with canonical ownership rules. + + * At least one of unit_id or property_id MUST be present. + * If both present: unit.property_id MUST equal property_id, else CrossOrgReference (409). + * Returns the property_id to store on Expense (NOT NULL). + """ + resolved_from_unit: int | None = None + if unit_id is not None: + unit_row = db.query(Unit.property_id).filter(Unit.id == unit_id, Unit.deleted_at.is_(None)).one_or_none() + if unit_row is None: + raise LookupError("Unit not found") + resolved_from_unit = unit_row[0] + if property_id is not None and resolved_from_unit is not None: + if property_id != resolved_from_unit: + raise CrossOrgReference( + "Expense unit_id and property_id must belong to the same property" + ) + final_property_id = resolved_from_unit if resolved_from_unit is not None else property_id + if final_property_id is None: + raise HTTPException( + status.HTTP_422_UNPROCESSABLE_ENTITY, + "Expense requires at least unit_id or property_id (canonical ownership anchor)", + ) + return final_property_id + + +def _resolve_expense_org_id(db: Session, unit_id: int | None, property_id: int | None) -> int: + """Resolve canonical organization_id for an Expense via both channels (fail closed).""" + final_property_id = _resolve_expense_property_id(db, unit_id, property_id) + oid = property_org_id(db, final_property_id) + if oid is None: + raise CrossOrgReference( + f"Expense property id={final_property_id} not found or has no organization" + ) + return oid + + def _check_payer(db: Session, payer_user_id: int | None) -> None: - """AI-OPS-FOUNDATION-001 §4/§8: a recorded payer must be a real active - user so the PAYMENT_PENDING task can actually reach them.""" if payer_user_id is None: return from app.models.user import User @@ -87,11 +137,6 @@ def _check_payer(db: Session, payer_user_id: int | None) -> None: def _guard_edit(db: Session, obj: Expense, updates: dict, user: User) -> None: - """003B §9: a critical financial-field change on an APPROVED/PAID/PARTIAL - expense must demote it back to PENDING (re-approval required) instead of - silently keeping the stale approval. Already-reversed/pending edits are - allowed; rejected records can be edited to resubmit through the resubmit - endpoint.""" if obj.status == ExpenseStatus.reversed: raise HTTPException(status.HTTP_409_CONFLICT, "Cannot edit a reversed expense") if obj.status == ExpenseStatus.rejected: @@ -102,8 +147,6 @@ def _guard_edit(db: Session, obj: Expense, updates: dict, user: User) -> None: critical_changed = {f for f in _CRITICAL_FIELDS if f in updates and updates[f] != getattr(obj, f)} if obj.status in (ExpenseStatus.approved, ExpenseStatus.paid, ExpenseStatus.partially_paid, ExpenseStatus.payment_claimed): if critical_changed: - # Invalidate the old approval; the expense returns to PENDING and - # must go through Owner review again before payment can continue. clear_approval(db, obj, actor_id=user.id, reason="Critical financial field changed after approval " f"({', '.join(sorted(critical_changed))})") @@ -116,18 +159,6 @@ def _complete_linked_approval_task( actor_id: int, reason: str, ) -> None: - """Closing an expense approval also closes the linked APPROVAL_PENDING - operational task atomically in the same transaction (single source of - truth; the bot never does this itself). - - Convergence boundary (PASAY-VNEXT-FOUNDATION-LEGACY-001): the - ``OperationalTask`` close + redelivery-suppression routes through - ``app/services.operations.projection``. The audit ACTION NAME remains - the existing ``task_completed_via_approval`` (or ``..._rejection``) so - production contracts and tests that match the exact string stay - green; ``source_domain`` / ``reason`` ride in ``changed_fields`` for - provenance (no new enum slot, no migration). - """ task = ( db.query(OperationalTask) .filter( @@ -163,12 +194,6 @@ def _complete_linked_payment_tasks( *, actor_id: int, ) -> None: - """When an expense is FULLY paid (remaining == 0), close every still-active - expense-linked payment task so the to-do list never keeps showing 'waiting - for payment' for a fully-paid expense. NEVER closes related Repair tasks. - - Audit ACTION NAME preserved as the existing ``task_completed_via_payment``. - """ tasks = ( db.query(OperationalTask) .filter( @@ -234,7 +259,6 @@ def _build_detail(db: Session, expense: Expense) -> ExpenseDetailOut: def _evidence_for_expense(db: Session, expense: Expense, claims) -> dict: - """Group evidence by claim for the Mini App detail (003B §10).""" from app.models.evidence import Evidence out = {} @@ -254,7 +278,6 @@ def _evidence_for_expense(db: Session, expense: Expense, claims) -> dict: } for e in rows ] - # root-level (legacy) receipt if expense.receipt_attachment_id: out["receipt_attachment_id"] = expense.receipt_attachment_id return out @@ -265,26 +288,34 @@ def _evidence_for_expense(db: Session, expense: Expense, claims) -> dict: # --------------------------------------------------------------------------- @router.get("", response_model=list[ExpenseRead]) -def list_expenses(db: Session = Depends(get_db), _: User = Depends(get_current_user)): - return db.query(Expense).order_by(Expense.id).all() +def list_expenses(db: Session = Depends(get_db), user: User = Depends(get_current_user)): + try: + return scoped_list_expenses(db, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc @router.get("/{expense_id}", response_model=ExpenseRead) def get_expense( - expense_id: int, db: Session = Depends(get_db), _: User = Depends(get_current_user) + expense_id: int, db: Session = Depends(get_db), user: User = Depends(get_current_user) ): - return _get_or_404(db, expense_id) + try: + obj, _membership = scoped_get_expense(db, expense_id, for_user_id=user.id) + return obj + except Exception as exc: + raise scope_exception_to_http(exc) from exc @router.get("/{expense_id}/detail", response_model=ExpenseDetailOut) def expense_detail( expense_id: int, db: Session = Depends(get_db), - _: User = Depends(get_current_user), + user: User = Depends(get_current_user), ): - """Mini App full detail: Expense / Approval / Payment / Claims / Evidence / - Verification / Actions / Timeline (003B §15).""" - obj = _get_or_404(db, expense_id) + try: + obj, _membership = scoped_get_expense(db, expense_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc return _build_detail(db, obj) @@ -296,20 +327,34 @@ def expense_detail( def create_expense( payload: ExpenseCreate, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): if payload.status not in _CREATABLE: raise HTTPException( status.HTTP_422_UNPROCESSABLE_ENTITY, "Expense can only be created as pending or approved", ) - if payload.status == ExpenseStatus.approved and user.role != "admin": + + membership = None + try: + final_property_id = _resolve_expense_property_id(db, payload.unit_id, payload.property_id) + e_org_id = _resolve_expense_org_id(db, payload.unit_id, payload.property_id) + membership = resolve_org_membership( + db, user.id, e_org_id, role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY] + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + + if payload.status == ExpenseStatus.approved and membership.role != OrganizationRole.OWNER: raise HTTPException( - status.HTTP_403_FORBIDDEN, "Only admin can create an approved expense" + status.HTTP_403_FORBIDDEN, "Only OWNER can create an approved expense" ) _check_unit(db, payload.unit_id) + _check_property(db, payload.property_id) _check_payer(db, payload.payer_user_id) - obj = Expense(**payload.model_dump(), version=1) + data = payload.model_dump() + data["property_id"] = final_property_id + obj = Expense(**data, version=1) obj.created_by = user.id obj.updated_by = user.id if obj.status == ExpenseStatus.approved: @@ -335,14 +380,38 @@ def update_expense( expense_id: int, payload: ExpenseUpdate, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, expense_id) + try: + obj, membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + updates = payload.model_dump(exclude_unset=True) if not updates: return obj _guard_edit(db, obj, updates, user) + + effective_unit_id = updates.get("unit_id", obj.unit_id) + effective_property_id = updates.get("property_id", obj.property_id) + try: + final_property_id = _resolve_expense_property_id(db, effective_unit_id, effective_property_id) + new_org_id = _resolve_expense_org_id(db, effective_unit_id, effective_property_id) + assert_co_org( + db, + user_org_id=membership.organization_id, + object_org_id=new_org_id, + object_kind="Expense (after update)", + object_id=expense_id, + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + _check_unit(db, updates.get("unit_id")) + _check_property(db, updates.get("property_id")) if "payer_user_id" in updates: _check_payer(db, updates.get("payer_user_id")) old = serialize_row(obj) @@ -350,6 +419,7 @@ def update_expense( for field, value in updates.items(): if field != "status": setattr(obj, field, value) + obj.property_id = final_property_id obj.updated_by = user.id record_audit( db, @@ -374,10 +444,16 @@ def update_expense( def approve_expense( expense_id: int, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, expense_id) - if user.role == "manager" and obj.created_by == user.id: + try: + obj, membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + + if obj.created_by == user.id and membership.role != OrganizationRole.OWNER: raise HTTPException( status.HTTP_403_FORBIDDEN, "Cannot approve an expense you created" ) @@ -410,7 +486,7 @@ def approve_expense( db.refresh(obj) return obj db.rollback() - current = _get_or_404(db, expense_id) + current, _m = scoped_get_expense(db, expense_id, for_user_id=user.id) if current.status == ExpenseStatus.approved: return current raise HTTPException(status.HTTP_409_CONFLICT, "Only pending expenses can be approved") @@ -421,14 +497,16 @@ def reject_expense( expense_id: int, payload: PaymentClaimIn | None = None, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - """Reject the CURRENT proposal/version. The rejected V1 is preserved (row - kept with status=rejected + rejection_reason); the Secretary resubmits a - corrected version through ``POST /resubmit`` which creates a NEW V2 row - (003B §8 / E8).""" - obj = _get_or_404(db, expense_id) - if user.role == "manager" and obj.created_by == user.id: + try: + obj, membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + + if obj.created_by == user.id and membership.role != OrganizationRole.OWNER: raise HTTPException( status.HTTP_403_FORBIDDEN, "Cannot reject an expense you created" ) @@ -451,7 +529,7 @@ def reject_expense( db, table_name="expenses", record_id=obj.id, - action="reject", # legacy audit action (parity + existing contract) + action="reject", actor_id=user.id, changed_fields={ "status": ["pending", "rejected"], @@ -464,7 +542,7 @@ def reject_expense( db.refresh(obj) return obj db.rollback() - current = _get_or_404(db, expense_id) + current, _m = scoped_get_expense(db, expense_id, for_user_id=user.id) if current.status == ExpenseStatus.rejected: return current raise HTTPException(status.HTTP_409_CONFLICT, "Only pending expenses can be rejected") @@ -475,21 +553,36 @@ def resubmit_expense( expense_id: int, payload: _ResubmitPayload, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - """Create the NEXT version of a rejected (or reapproval-demoted) expense. - The old V1 REJECTED row is preserved; a new V2 PENDING row is created that - links back via ``parent_expense_id`` (003B §8 / E8).""" - prior = _get_or_404(db, expense_id) + try: + prior, membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + if prior.status != ExpenseStatus.rejected: raise HTTPException( status.HTTP_409_CONFLICT, "Only a rejected expense can be resubmitted as the next version", ) updates = payload.model_dump(exclude_unset=True) - _check_unit(db, updates.get("unit_id") or prior.unit_id) - if updates.get("payer_user_id") or prior.payer_user_id: - _check_payer(db, updates.get("payer_user_id") or prior.payer_user_id) + effective_unit_id = updates.get("unit_id", prior.unit_id) + effective_property_id = updates.get("property_id", prior.property_id) + try: + final_property_id = _resolve_expense_property_id(db, effective_unit_id, effective_property_id) + new_org_id = _resolve_expense_org_id(db, effective_unit_id, effective_property_id) + assert_co_org(db, user_org_id=membership.organization_id, object_org_id=new_org_id, object_kind="Expense (resubmit)", object_id=expense_id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + + _check_unit(db, effective_unit_id) + _check_property(db, effective_property_id) + effective_payer = updates.get("payer_user_id", prior.payer_user_id) + if effective_payer: + _check_payer(db, effective_payer) data = { "expense_date": updates.get("expense_date", prior.expense_date), "due_date": updates.get("due_date", prior.due_date), @@ -497,8 +590,9 @@ def resubmit_expense( "amount": updates.get("amount", prior.amount), "payee": updates.get("payee", prior.payee), "description": updates.get("description", prior.description), - "unit_id": updates.get("unit_id", prior.unit_id), - "payer_user_id": updates.get("payer_user_id", prior.payer_user_id), + "unit_id": effective_unit_id, + "property_id": final_property_id, + "payer_user_id": effective_payer, "status": ExpenseStatus.pending, "version": (prior.version or 1) + 1, "parent_expense_id": prior.id, @@ -532,12 +626,16 @@ def create_payment_claim( expense_id: int, payload: PaymentClaimIn, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - """Secretary reports a payment -> create a PENDING claim (payment reported, - awaiting verification). Idempotent on ``idempotency_key``/deterministic key - (003B §3/§6 / E2 / E5).""" - expense = _get_or_404(db, expense_id) + try: + expense, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + if payload.claimed_amount is None: raise HTTPException(status.HTTP_422_UNPROCESSABLE_ENTITY, "claimed_amount is required") try: @@ -562,9 +660,12 @@ def create_payment_claim( def list_payment_claims( expense_id: int, db: Session = Depends(get_db), - _: User = Depends(get_current_user), + user: User = Depends(get_current_user), ): - _get_or_404(db, expense_id) + try: + _expense, _membership = scoped_get_expense(db, expense_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc return [PaymentClaimOut(**__claim_out(c)) for c in list_claims(db, expense_id)] @@ -593,13 +694,15 @@ def verify_payment_claim( claim_id: int, payload: PaymentClaimIn, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - """Owner/verifier confirms the payment is real. On success the claimed - amount enters the verified aggregate and the expense reconciles to - partial/full paid (003B §3/§4 / E3 / E4). An over-claim is surfaced as a - mismatch and never auto-PAIDs (E6).""" - expense = _get_or_404(db, expense_id) + try: + expense, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + try: claim = verify_claim( db, @@ -624,9 +727,15 @@ def fail_payment_claim( claim_id: int, payload: PaymentClaimIn, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - expense = _get_or_404(db, expense_id) + try: + expense, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + try: claim = fail_claim( db, expense, claim_id, @@ -646,11 +755,15 @@ def reverse_payment_claim( claim_id: int, payload: PaymentClaimIn, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - """Reverse a VERIFIED claim — verified aggregate recomputes, remaining - returns, a fully-paid expense re-enters a payable state (003B §18 / E13).""" - expense = _get_or_404(db, expense_id) + try: + expense, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + try: claim = reverse_claim( db, expense, claim_id, @@ -669,9 +782,6 @@ def reverse_payment_claim( # --------------------------------------------------------------------------- def _finalize_paid(expense: Expense, db: Session, actor_id: int | None = None) -> None: - """After a payment mutation, if the expense is now FULLY paid (derived from - the verified aggregate) close its payment tasks. Expense status/audit was - already updated by sync_expense_status — this only removes stale to-dos.""" truth = payment_truth(db, expense) if truth.fully_paid: _complete_linked_payment_tasks(db, expense, actor_id=actor_id or expense.updated_by) @@ -682,19 +792,15 @@ def pay_expense( expense_id: int, payload: PaymentClaimIn | None = None, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - """Owner is the final verifier. Tapping 'confirm paid' now records a REAL - VERIFIED payment claim for the remaining amount and derives paid status - from the verified aggregate — never a bare status flip (003B §3/§7). Replays - are idempotent: an already-paid expense returns the current record. - - Concurrent replays are serialized on the expense row (SELECT ... FOR - UPDATE) so only ONE caller performs the payment mutation; the others re-read - the fully-paid state and return the current record (legacy concurrent-pay - contract, E5).""" - # Lock the expense row to serialize concurrent Owner-pay replays. - _get_or_404(db, expense_id) # ensure existence/404 + try: + obj, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + obj = ( db.query(Expense) .filter(Expense.id == expense_id) @@ -715,9 +821,6 @@ def pay_expense( db.refresh(obj) return obj now = datetime.now(timezone.utc) - # Owner-complete-remaining: a STABLE deterministic idempotency tag so - # concurrent/duplicate taps of the same "confirm paid" converge to ONE - # verified claim (section 6 / E5 — the old concurrent-pay replay contract). from app.services.expense_claims import claim_idempotency_key key = payload.idempotency_key if payload and payload.idempotency_key else claim_idempotency_key( @@ -734,16 +837,11 @@ def pay_expense( idempotency_key=key, now=now, ) - # Owner-verified: the claim is admitted immediately into truth. verify_claim( db, obj, claim.id, verified_by=user.id, verified_amount=None, result=(payload.result if payload else None), now=now, ) except ClaimError as exc: - # A concurrent caller may have completed the payment while this retry - # was in flight. Roll back the failed statement, then re-read the true - # expense state: if it is now fully paid it is an idempotent success - # (return current), not a failure. db.rollback() db.expire_all() db.refresh(obj) @@ -754,9 +852,6 @@ def pay_expense( db.refresh(obj) return obj raise HTTPException(status.HTTP_409_CONFLICT, str(exc)) from exc - # Owner's explicit payment confirmation — record the `pay` audit action. On - # an idempotent replay (claim not newly created) skip it so concurrent pay - # replays converge to exactly one `pay` (legacy concurrent contract). if was_created: record_audit( db, @@ -776,15 +871,15 @@ def pay_expense( def reverse_expense( expense_id: int, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): - """Reverse a fully/partially paid expense by reversing every VERIFIED - claim (history preserved, aggregate recomputed, remaining returns) — the - old row flips to ``reversed`` (003B §18 / E13). Concurrent reversals are - serialized on the expense row; an already-reversed expense returns the - current record (idempotent, legacy contract).""" - # Lock the row to serialize concurrent reversals. - _get_or_404(db, expense_id) + try: + obj, _membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + obj = ( db.query(Expense) .filter(Expense.id == expense_id) @@ -799,6 +894,11 @@ def reverse_expense( return obj claims = [c for c in list_claims(db, obj.id) if c.status == ClaimStatus.VERIFIED] if not claims: + db.refresh(obj) + if obj.status == ExpenseStatus.reversed: + db.commit() + db.refresh(obj) + return obj raise HTTPException(status.HTTP_409_CONFLICT, "No verified payments to reverse") now = datetime.now(timezone.utc) for c in claims: diff --git a/app/api/routers/income.py b/app/api/routers/income.py index 4d4a195..493fb7f 100644 --- a/app/api/routers/income.py +++ b/app/api/routers/income.py @@ -12,28 +12,60 @@ from app.models.user import User from app.schemas.financial import IncomeCreate, IncomeRead, IncomeUpdate from app.services.audit import field_changes, record_audit, serialize_row +from app.services.organization_scope import ( + CrossOrgReference, + OwnerRequired, + ScopeBlocked, + income_org_id, + lease_org_id, + resolve_org_membership, + scope_exception_to_http, + scoped_get_income, + scoped_list_incomes, + list_active_org_ids_for_user, +) router = APIRouter(prefix="/incomes", tags=["incomes"]) -def _get_or_404(db: Session, income_id: int) -> Income: - obj = db.query(Income).filter(Income.id == income_id).first() - if obj is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Income not found") - return obj - - def _check_lease(db: Session, lease_id: int | None) -> None: if lease_id is None: - return + raise CrossOrgReference("Income requires canonical lease ownership (lease_id is required)") lease = db.query(Lease).filter(Lease.id == lease_id, Lease.deleted_at.is_(None)).first() if lease is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "Lease not found") +def _assert_lease_co_org(db: Session, user: User, lease_id: int | None) -> None: + if lease_id is None: + raise CrossOrgReference("Income requires canonical lease ownership (lease_id is required)") + object_org_id = lease_org_id(db, lease_id) + if object_org_id is None: + raise CrossOrgReference( + f"Lease id={lease_id} not found or has no organization" + ) + try: + resolve_org_membership(db, user.id, object_org_id) + except ScopeBlocked: + raise CrossOrgReference( + f"Lease id={lease_id} does not belong to the caller's organization" + ) from None + + +def _assert_income_co_org(db: Session, user: User, income_id: int) -> None: + object_org_id = income_org_id(db, income_id) + if object_org_id is None: + raise LookupError(f"income {income_id} not found or has no organization") + if object_org_id not in list_active_org_ids_for_user(db, user.id): + raise LookupError(f"income {income_id} not found") + + @router.get("", response_model=list[IncomeRead]) -def list_incomes(db: Session = Depends(get_db), _: User = Depends(get_current_user)): - return db.query(Income).order_by(Income.id).all() +def list_incomes(db: Session = Depends(get_db), user: User = Depends(get_current_user)): + try: + return scoped_list_incomes(db, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc @router.post("", response_model=IncomeRead, status_code=status.HTTP_201_CREATED) @@ -48,7 +80,11 @@ def create_income( status.HTTP_422_UNPROCESSABLE_ENTITY, "Income can only be created as pending or confirmed", ) - _check_lease(db, payload.lease_id) + try: + _check_lease(db, payload.lease_id) + _assert_lease_co_org(db, user, payload.lease_id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc if payload.idempotency_key: existing = ( db.query(Income) @@ -56,7 +92,10 @@ def create_income( .first() ) if existing is not None: - # Idempotent replay: the create already landed -> return it. + try: + _assert_income_co_org(db, user, existing.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc response.status_code = status.HTTP_200_OK return existing obj = Income(**payload.model_dump()) @@ -78,8 +117,6 @@ def create_income( ) db.commit() except IntegrityError: - # UNIQUE(idempotency_key) is the atomic backstop: a concurrent - # create with the same key won the race. Re-read and return it. db.rollback() if payload.idempotency_key: existing = ( @@ -88,6 +125,10 @@ def create_income( .first() ) if existing is not None: + try: + _assert_income_co_org(db, user, existing.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc response.status_code = status.HTTP_200_OK return existing raise @@ -97,9 +138,13 @@ def create_income( @router.get("/{income_id}", response_model=IncomeRead) def get_income( - income_id: int, db: Session = Depends(get_db), _: User = Depends(get_current_user) + income_id: int, db: Session = Depends(get_db), user: User = Depends(get_current_user) ): - return _get_or_404(db, income_id) + try: + obj, _membership = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + return obj @router.patch("/{income_id}", response_model=IncomeRead) @@ -109,7 +154,10 @@ def update_income( db: Session = Depends(get_db), user: User = Depends(manager_or_admin), ): - obj = _get_or_404(db, income_id) + try: + obj, _membership = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc updates = payload.model_dump(exclude_unset=True) if not updates: return obj @@ -120,7 +168,11 @@ def update_income( status.HTTP_409_CONFLICT, "Cannot change the amount of a confirmed income" ) if "lease_id" in updates: - _check_lease(db, updates["lease_id"]) + try: + _check_lease(db, updates["lease_id"]) + _assert_lease_co_org(db, user, updates["lease_id"]) + except Exception as exc: + raise scope_exception_to_http(exc) from exc old = serialize_row(obj) changed = field_changes(obj, updates) for field, value in updates.items(): @@ -147,7 +199,10 @@ def confirm_income( db: Session = Depends(get_db), user: User = Depends(owner_subject_only), ): - obj = _get_or_404(db, income_id) + try: + obj, _membership = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc old = serialize_row(obj) result = db.execute( update(Income) @@ -175,10 +230,11 @@ def confirm_income( db.commit() db.refresh(obj) return obj - # rowcount == 0 -> no transition happened. Replay (already confirmed) - # returns the current state; any other state is a genuine conflict. db.rollback() - current = _get_or_404(db, income_id) + try: + current, _m = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc if current.status == IncomeStatus.confirmed: return current raise HTTPException(status.HTTP_409_CONFLICT, "Only pending income can be confirmed") @@ -190,7 +246,10 @@ def reverse_income( db: Session = Depends(get_db), user: User = Depends(owner_subject_only), ): - obj = _get_or_404(db, income_id) + try: + obj, _membership = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc old = serialize_row(obj) result = db.execute( update(Income) @@ -217,7 +276,10 @@ def reverse_income( db.refresh(obj) return obj db.rollback() - current = _get_or_404(db, income_id) + try: + current, _m = scoped_get_income(db, income_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc if current.status == IncomeStatus.reversed: return current raise HTTPException(status.HTTP_409_CONFLICT, "Only confirmed income can be reversed") diff --git a/app/api/routers/leases.py b/app/api/routers/leases.py index 43c3ec4..17253e6 100644 --- a/app/api/routers/leases.py +++ b/app/api/routers/leases.py @@ -1,9 +1,8 @@ from fastapi import APIRouter, Depends, HTTPException, status from sqlalchemy.orm import Session -from app.api.deps import get_current_user, manager_or_admin +from app.api.deps import get_current_user from app.database import get_db -from app.models.commission import CommissionSettlement from app.models.lease import Lease, LeaseStatus from app.models.property import Unit, UnitStatus from app.models.tenant import Tenant @@ -11,31 +10,24 @@ from app.schemas.common import MessageResponse from app.schemas.lease import LeaseCreate, LeaseRead, LeaseUpdate from app.services.audit import field_changes, record_audit, serialize_row +from app.services.organization_scope import ( + CrossOrgReference, + OrganizationRole, + OwnerRequired, + ScopeBlocked, + assert_co_org, + resolve_org_membership, + scope_exception_to_http, + scoped_get_lease, + scoped_list_leases, + tenant_org_id, + unit_org_id, +) router = APIRouter(prefix="/leases", tags=["leases"]) -def _get_or_404(db: Session, lease_id: int) -> Lease: - obj = db.query(Lease).filter(Lease.id == lease_id, Lease.deleted_at.is_(None)).first() - if obj is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Lease not found") - return obj - - -def _visible_lease_ids(db: Session, user: User) -> set[int] | None: - """For agents: leases related to their commission settlements. None = no filter.""" - if user.role != "agent": - return None - rows = ( - db.query(CommissionSettlement.lease_id) - .filter(CommissionSettlement.agent_id == user.id) - .all() - ) - return {r[0] for r in rows} - - def _sync_unit_status(db: Session, unit: Unit) -> None: - """Recompute unit occupancy from its active leases.""" active = ( db.query(Lease) .filter(Lease.unit_id == unit.id, Lease.status == LeaseStatus.active, Lease.deleted_at.is_(None)) @@ -51,19 +43,32 @@ def _sync_unit_status(db: Session, unit: Unit) -> None: def list_leases( db: Session = Depends(get_db), user: User = Depends(get_current_user) ): - query = db.query(Lease).filter(Lease.deleted_at.is_(None)) - visible = _visible_lease_ids(db, user) - if visible is not None: - query = query.filter(Lease.id.in_(visible)) - return query.order_by(Lease.id).all() + try: + return scoped_list_leases(db, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc @router.post("", response_model=LeaseRead, status_code=status.HTTP_201_CREATED) def create_lease( payload: LeaseCreate, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): + try: + u_org_id = unit_org_id(db, payload.unit_id) + if u_org_id is None: + raise LookupError("Unit not found") + membership = resolve_org_membership( + db, user.id, u_org_id, role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY] + ) + t_org_id = tenant_org_id(db, payload.tenant_id) + if t_org_id is None: + raise LookupError("Tenant not found") + assert_co_org(db, user_org_id=membership.organization_id, object_org_id=t_org_id, object_kind="Tenant", object_id=payload.tenant_id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + unit = db.query(Unit).filter(Unit.id == payload.unit_id, Unit.deleted_at.is_(None)).first() if unit is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "Unit not found") @@ -100,11 +105,11 @@ def get_lease( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - obj = _get_or_404(db, lease_id) - visible = _visible_lease_ids(db, user) - if visible is not None and obj.id not in visible: - raise HTTPException(status.HTTP_403_FORBIDDEN, "Insufficient permissions") - return obj + try: + obj, _membership = scoped_get_lease(db, lease_id, for_user_id=user.id) + return obj + except Exception as exc: + raise scope_exception_to_http(exc) from exc @router.patch("/{lease_id}", response_model=LeaseRead) @@ -112,9 +117,16 @@ def update_lease( lease_id: int, payload: LeaseUpdate, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, lease_id) + try: + obj, membership = scoped_get_lease( + db, lease_id, for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + updates = payload.model_dump(exclude_unset=True) if not updates: return obj @@ -134,6 +146,20 @@ def update_lease( "accounting_start_date must be within [start_date, end_date]", ) + try: + if updates.get("unit_id") is not None and updates["unit_id"] != obj.unit_id: + new_unit_org = unit_org_id(db, updates["unit_id"]) + if new_unit_org is None: + raise LookupError("Unit not found") + assert_co_org(db, user_org_id=membership.organization_id, object_org_id=new_unit_org, object_kind="Unit", object_id=updates["unit_id"]) + if updates.get("tenant_id") is not None and updates["tenant_id"] != obj.tenant_id: + new_tenant_org = tenant_org_id(db, updates["tenant_id"]) + if new_tenant_org is None: + raise LookupError("Tenant not found") + assert_co_org(db, user_org_id=membership.organization_id, object_org_id=new_tenant_org, object_kind="Tenant", object_id=updates["tenant_id"]) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + if updates.get("unit_id") is not None and updates["unit_id"] != obj.unit_id: unit = db.query(Unit).filter(Unit.id == updates["unit_id"], Unit.deleted_at.is_(None)).first() if unit is None: @@ -164,7 +190,7 @@ def update_lease( for field, value in updates.items(): setattr(obj, field, value) obj.updated_by = user.id - db.flush() # make the status change visible to the occupancy re-check + db.flush() if unit is not None: if new_status == LeaseStatus.active: unit.status = UnitStatus.occupied @@ -190,9 +216,16 @@ def update_lease( def delete_lease( lease_id: int, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, lease_id) + try: + obj, _membership = scoped_get_lease( + db, lease_id, for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + if obj.status == LeaseStatus.active: raise HTTPException( status.HTTP_409_CONFLICT, "Terminate the lease before deleting it" diff --git a/app/api/routers/properties.py b/app/api/routers/properties.py index c2ad4e2..29ad100 100644 --- a/app/api/routers/properties.py +++ b/app/api/routers/properties.py @@ -9,6 +9,7 @@ from app.schemas.common import MessageResponse from app.schemas.property import PropertyCreate, PropertyRead, PropertyUpdate from app.services.audit import field_changes, record_audit, serialize_row +from app.services.organization_scope import scope_exception_to_http from app.services.property_channel import ( OwnerRequired, ScopeBlocked, @@ -21,14 +22,6 @@ router = APIRouter(prefix="/properties", tags=["properties"]) -def _scope_exception_to_http(exc: Exception) -> HTTPException: - if isinstance(exc, LookupError): - return HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) - if isinstance(exc, (ScopeBlocked, OwnerRequired)): - return HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) - return HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR, type(exc).__name__) - - @router.get("", response_model=list[PropertyRead]) def list_properties( db: Session = Depends(get_db), user: User = Depends(get_current_user) @@ -36,7 +29,7 @@ def list_properties( try: return scoped_list_properties(db, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc + raise scope_exception_to_http(exc) from exc @router.post("", response_model=PropertyRead, status_code=status.HTTP_201_CREATED) @@ -47,7 +40,10 @@ def create_property( ): try: resolve_org_membership( - db, user.id, payload.organization_id, role=OrganizationRole.OWNER, + db, + user.id, + payload.organization_id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], ) except ScopeBlocked as exc: raise HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) from exc @@ -79,7 +75,7 @@ def get_property( try: prop, _membership = scoped_get_property(db, property_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc + raise scope_exception_to_http(exc) from exc return prop @@ -93,14 +89,14 @@ def update_property( try: obj, membership = scoped_get_property(db, property_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc + raise scope_exception_to_http(exc) from exc updates = payload.model_dump(exclude_unset=True) if not updates: return obj try: - if membership.role != OrganizationRole.OWNER.value: + if membership.role != OrganizationRole.OWNER: filter_secretary_property_updates(set(updates.keys())) except OwnerRequired as exc: raise HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) from exc @@ -134,8 +130,8 @@ def delete_property( try: obj, membership = scoped_get_property(db, property_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc - if membership.role != OrganizationRole.OWNER.value: + raise scope_exception_to_http(exc) from exc + if membership.role != OrganizationRole.OWNER: raise HTTPException( status.HTTP_403_FORBIDDEN, "Only ACTIVE OWNER may soft-delete a Property", diff --git a/app/api/routers/property_channel.py b/app/api/routers/property_channel.py index 388ec49..611dce9 100644 --- a/app/api/routers/property_channel.py +++ b/app/api/routers/property_channel.py @@ -7,6 +7,7 @@ from app.models.property_channel import BindingStatus from app.models.user import User from app.schemas.property import UnitChannelBindingCreate, UnitChannelBindingRead +from app.services.organization_scope import scope_exception_to_http from app.services.property_channel import ( BindingConflict, OwnerRequired, @@ -23,16 +24,14 @@ router = APIRouter(prefix="/property-channel", tags=["property_channel"]) -def _scope_exception_to_http(exc: Exception) -> HTTPException: - if isinstance(exc, LookupError): - return HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) - if isinstance(exc, (ScopeBlocked, OwnerRequired)): - return HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) +def _route_exception_to_http(exc: Exception) -> HTTPException: + """Router-specific mapping (ValueError→400, BindingConflict→409); + all org-scope and unknown exceptions fall through to canonical shared helper.""" if isinstance(exc, ValueError): return HTTPException(status.HTTP_400_BAD_REQUEST, str(exc)) if isinstance(exc, BindingConflict): return HTTPException(status.HTTP_409_CONFLICT, str(exc)) - return HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR, type(exc).__name__) + return scope_exception_to_http(exc) @router.get("/units/lookup", response_model=UnitChannelBindingRead | None) @@ -47,7 +46,7 @@ def lookup_unit( try: _validate_purpose(purpose) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc try: _unit, _membership = scoped_lookup_unit( db, @@ -57,7 +56,7 @@ def lookup_unit( for_user_id=user.id, ) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc active = get_active_binding(db, _unit.id, purpose) return active @@ -72,7 +71,7 @@ def list_unit_bindings( try: _unit, _membership = scoped_get_unit(db, unit_id, for_user_id=user.id) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc return list_bindings_for_unit(db, unit_id, status=status_filter) @@ -86,11 +85,11 @@ def get_unit_active_binding( try: _validate_purpose(purpose) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc try: _unit, _membership = scoped_get_unit(db, unit_id, for_user_id=user.id) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc return get_active_binding(db, unit_id, purpose) @@ -111,7 +110,7 @@ def create_binding( notes=payload.notes, ) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc try: db.commit() except IntegrityError as exc: @@ -138,7 +137,7 @@ def revoke_binding( actor_user_id=user.id, ) except Exception as exc: - raise _scope_exception_to_http(exc) from exc + raise _route_exception_to_http(exc) from exc db.commit() db.refresh(binding) return binding diff --git a/app/api/routers/repairs.py b/app/api/routers/repairs.py index fb1c4d3..912843b 100644 --- a/app/api/routers/repairs.py +++ b/app/api/routers/repairs.py @@ -1,21 +1,3 @@ -"""REPAIR-AI-EMPLOYEE-WORKFLOW-008A — Repair API router (/api/v1/repairs). - -Owns the Repair Operation lifecycle and its decoupled Proposals / Actions. - -RBAC (consistent with the rest of the app): -- admin: everything. -- manager: view + submit proposals + record results. -- agent: only view/act repairs assigned to them. - -The Router enforces the 008A invariants by delegating to the service layer: -- reject_proposal keeps the repair OPEN/WAITING_HUMAN (never closes/cancels); -- approve_proposal -> WAITING_PAYMENT (never closes); -- pay links Expense-paid -> VERIFYING at most (never closes); -- ONLY verify closes the repair (verification gate). - -Timeline is surfaced via the existing audit_log (every mutation records an -audit event with action + old/new row). -""" from __future__ import annotations from datetime import datetime, timezone @@ -26,6 +8,7 @@ from app.api.deps import admin_only, get_current_user, manager_or_admin from app.database import get_db from app.models.financial import Expense, ExpenseStatus +from app.models.property import Unit from app.models.repair import ( RepairOperation, RepairProposal, @@ -42,7 +25,22 @@ RepairVerifyIn, ) from app.services.audit import record_audit, serialize_row -from app.services.repairs import continuation, operations as op_svc +from app.services.organization_scope import ( + CrossOrgReference, + OrganizationRole, + OwnerRequired, + ScopeBlocked, + expense_org_id, + property_org_id, + resolve_org_membership, + scope_exception_to_http, + scoped_get_expense, + scoped_get_repair, + scoped_list_repairs, + unit_org_id, +) +from app.services.repairs import continuation +from app.services.repairs import operations as op_svc from app.services.repairs import payment as payment_svc from app.services.repairs import proposals as prop_svc from app.services.repairs import verification as verify_svc @@ -51,11 +49,45 @@ router = APIRouter(prefix="/repairs", tags=["repairs"]) -def _get_repair_or_404(db: Session, repair_id: int) -> RepairOperation: - repair = db.get(RepairOperation, repair_id) - if repair is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Repair not found") - return repair +def _scope_guard(repair: RepairOperation, user: User) -> None: + if user.role == UserRole.agent and repair.assignee_user_id not in (None, user.id): + raise HTTPException( + status.HTTP_403_FORBIDDEN, "Cannot access a repair assigned to another user" + ) + + +def _resolve_repair_property_id(db: Session, repair: RepairOperation) -> int | None: + """Return canonical property_id for repair (unit.property_id preferred, else direct).""" + if repair.unit_id is not None: + row = db.query(Unit.property_id).filter(Unit.id == repair.unit_id, Unit.deleted_at.is_(None)).one_or_none() + if row is not None: + return row[0] + return repair.property_id + + +def _resolve_target_org_id_for_create( + db: Session, payload: RepairCreateIn +) -> tuple[int | None, int | None]: + """Return (target_org_id, final_property_id). If both unit_id and property_id are present, + assert they resolve to the SAME property_id (fail closed otherwise).""" + resolved_from_unit_prop: int | None = None + if payload.unit_id is not None: + unit_row = db.query(Unit.property_id).filter(Unit.id == payload.unit_id, Unit.deleted_at.is_(None)).one_or_none() + if unit_row is None: + raise LookupError("Unit not found") + resolved_from_unit_prop = unit_row[0] + if payload.property_id is not None and resolved_from_unit_prop != payload.property_id: + raise CrossOrgReference( + "Repair unit_id and property_id must belong to the same property" + ) + final_property_id = resolved_from_unit_prop if resolved_from_unit_prop is not None else payload.property_id + if payload.unit_id is not None: + oid = unit_org_id(db, payload.unit_id) + if oid is not None: + return oid, final_property_id + if payload.property_id is not None: + return property_org_id(db, payload.property_id), final_property_id + return None, None def _get_proposal_or_404(db: Session, proposal_id: int) -> RepairProposal: @@ -65,13 +97,6 @@ def _get_proposal_or_404(db: Session, proposal_id: int) -> RepairProposal: return p -def _scope_guard(repair: RepairOperation, user: User) -> None: - if user.role == UserRole.agent and repair.assignee_user_id not in (None, user.id): - raise HTTPException( - status.HTTP_403_FORBIDDEN, "Cannot access a repair assigned to another user" - ) - - def _resolve_proposal(db, repair, payload: RepairDecisionIn) -> RepairProposal: """Resolve a proposal by id or by version.""" if payload.proposal_id is not None and payload.version is not None: @@ -95,7 +120,6 @@ def _resolve_proposal(db, repair, payload: RepairDecisionIn) -> RepairProposal: status.HTTP_404_NOT_FOUND, f"Proposal V{payload.version} not found" ) return proposal - # default: the latest proposal proposal = prop_svc.latest_proposal(db, repair.id) if proposal is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "Repair has no proposals") @@ -111,10 +135,12 @@ def list_repairs( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - query = db.query(RepairOperation) + try: + items = scoped_list_repairs(db, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc if user.role == UserRole.agent: - query = query.filter(RepairOperation.assignee_user_id == user.id) - items = query.order_by(RepairOperation.id.desc()).all() + items = [r for r in items if r.assignee_user_id == user.id] return RepairListOut(items=items, total=len(items)) @@ -124,7 +150,10 @@ def get_repair_detail( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - repair = _get_repair_or_404(db, repair_id) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc _scope_guard(repair, user) proposals = prop_svc.list_proposals(db, repair.id) actions = continuation.resolve_actions(db, repair.id) @@ -149,7 +178,11 @@ def list_repair_actions( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - _scope_guard(_get_repair_or_404(db, repair_id), user) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + _scope_guard(repair, user) return continuation.resolve_actions(db, repair_id) @@ -161,8 +194,25 @@ def list_repair_actions( def create_repair( payload: RepairCreateIn, db: Session = Depends(get_db), - user: User = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): + try: + target_org_id, _final_property_id = _resolve_target_org_id_for_create(db, payload) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + if target_org_id is None: + raise HTTPException( + status.HTTP_409_CONFLICT, + "Cannot determine organization for repair (unit_id/property_id missing or invalid)", + ) + try: + resolve_org_membership( + db, user.id, target_org_id, + role=[OrganizationRole.SECRETARY, OrganizationRole.OWNER], + ) + except ScopeBlocked as exc: + raise scope_exception_to_http(exc) from exc + repair = op_svc.create_repair( db, issue=payload.issue, @@ -195,7 +245,10 @@ def submit_proposal( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - repair = _get_repair_or_404(db, repair_id) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc _scope_guard(repair, user) try: proposal, version = prop_svc.submit_proposal( @@ -236,10 +289,13 @@ def decide_proposal( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - repair = _get_repair_or_404(db, repair_id) + try: + repair, membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc proposal = _resolve_proposal(db, repair, payload) - if user.role == UserRole.manager and proposal.submitted_by == user.id: + if membership.role == OrganizationRole.SECRETARY and proposal.submitted_by == user.id: raise HTTPException( status.HTTP_403_FORBIDDEN, "Cannot decide on a proposal you submitted" ) @@ -268,13 +324,28 @@ def decide_proposal( changed_fields={"status": [proposal.status.value, "APPROVED"]}, new_value=serialize_row(proposal), ) - # If the decision carries a pre-created expense, link it so payment - # is tracked separately from the repair. if payload.expense_id is not None: - expense = db.get(Expense, payload.expense_id) - if expense is None: + try: + expense, _exp_membership = scoped_get_expense( + db, payload.expense_id, for_user_id=user.id, + role=[OrganizationRole.SECRETARY, OrganizationRole.OWNER], + ) + except Exception as exc: db.rollback() - raise HTTPException(status.HTTP_404_NOT_FOUND, "Expense not found") + raise scope_exception_to_http(exc) from exc + repair_canonical_property_id = _resolve_repair_property_id(db, repair) + expense_canonical_property_id = expense.property_id + if ( + repair_canonical_property_id is not None + and expense_canonical_property_id is not None + and repair_canonical_property_id != expense_canonical_property_id + ): + db.rollback() + raise scope_exception_to_http( + CrossOrgReference( + "Linked expense must belong to the same property as the repair" + ) + ) payment_svc.link_expense_to_proposal(db, proposal, expense) else: prop_svc.reject_proposal( @@ -293,8 +364,6 @@ def decide_proposal( changed_fields={"status": ["PENDING", "REJECTED"]}, new_value=serialize_row(proposal), ) - # 008A AI-continuation: reject -> create ONE dedup'd requote action - # (Case C proves repeated calls create only one active action). continuation.ensure_requote_action(db, repair, proposal, actor_id=user.id) except prop_svc.ProposalError as exc: db.rollback() @@ -309,17 +378,35 @@ def pay_linked_expense( repair_id: int, expense_id: int, db: Session = Depends(get_db), - user: User = Depends(admin_only), + user: User = Depends(get_current_user), ): """Mark the linked expense paid via a VERIFIED payment claim (003B) — the repair goes at most to VERIFYING, NEVER CLOSED.""" - repair = _get_repair_or_404(db, repair_id) - expense = db.get(Expense, expense_id) - if expense is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Expense not found") + try: + repair, _membership = scoped_get_repair( + db, repair_id, for_user_id=user.id, role=OrganizationRole.OWNER, + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + try: + expense, _exp_membership = scoped_get_expense( + db, expense_id, for_user_id=user.id, role=OrganizationRole.OWNER, + ) + except Exception as exc: + raise scope_exception_to_http(exc) from exc + repair_canonical_property_id = _resolve_repair_property_id(db, repair) + if ( + repair_canonical_property_id is not None + and expense.property_id is not None + and repair_canonical_property_id != expense.property_id + ): + raise scope_exception_to_http( + CrossOrgReference( + "Paid expense must belong to the same property as the repair" + ) + ) if expense.status != ExpenseStatus.approved: raise HTTPException(status.HTTP_409_CONFLICT, "Only approved expenses can be paid") - # 003B: paid status is reached ONLY through verified-claims aggregation. try: from app.services.expense_claims import create_claim, verify_claim @@ -328,7 +415,7 @@ def pay_linked_expense( idempotency_key=f"repair:{repair_id}:expense:{expense.id}:owner-pay", ) verify_claim(db, expense, claim.id, verified_by=user.id) - except Exception as exc: # noqa: BLE001 - preserve the 409 financial guard + except Exception as exc: db.rollback() raise HTTPException(status.HTTP_409_CONFLICT, f"Payment verification failed: {exc}") from exc record_audit( @@ -352,7 +439,10 @@ def record_result( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - repair = _get_repair_or_404(db, repair_id) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc _scope_guard(repair, user) try: verify_svc.mark_repair_completed( @@ -388,7 +478,10 @@ def verify_repair( db: Session = Depends(get_db), user: User = Depends(get_current_user), ): - repair = _get_repair_or_404(db, repair_id) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc _scope_guard(repair, user) try: verify_svc.verify_and_close( @@ -399,7 +492,7 @@ def verify_repair( closure_signal=payload.closure_signal, source=payload.source, ) - except Exception as exc: # noqa: BLE001 + except Exception as exc: db.rollback() raise HTTPException(status.HTTP_409_CONFLICT, f"Verification failed: {exc}") from exc record_audit( @@ -431,7 +524,10 @@ def cancel_repair( the guard. Terminal states are rejected here so the message is consistent for the bot / tests. """ - repair = _get_repair_or_404(db, repair_id) + try: + repair, _membership = scoped_get_repair(db, repair_id, for_user_id=user.id) + except Exception as exc: + raise scope_exception_to_http(exc) from exc if repair.status in ("CLOSED", "CANCELLED"): raise HTTPException(status.HTTP_409_CONFLICT, f"Repair is already {repair.status}") try: @@ -460,6 +556,7 @@ def _create_linked_expense(db, repair, proposal, version, user: User) -> Expense """Create an Expense for an approved-style quote. expense_date = today.""" import datetime as _dt + repair_property_id = _resolve_repair_property_id(db, repair) expense = Expense( expense_date=_dt.date.today(), category="维修", @@ -467,6 +564,7 @@ def _create_linked_expense(db, repair, proposal, version, user: User) -> Expense payee=proposal.vendor or "Vendor", description=(proposal.description or repair.issue)[:500], unit_id=repair.unit_id, + property_id=repair_property_id, status=ExpenseStatus.pending, created_by=user.id, updated_by=user.id, diff --git a/app/api/routers/tenants.py b/app/api/routers/tenants.py index 315b509..c49026e 100644 --- a/app/api/routers/tenants.py +++ b/app/api/routers/tenants.py @@ -1,31 +1,28 @@ from fastapi import APIRouter, Depends, HTTPException, status from sqlalchemy.orm import Session -from app.api.deps import get_current_user, manager_or_admin +from app.api.deps import get_current_user from app.database import get_db +from app.models.membership import OrganizationRole from app.models.tenant import Tenant +from app.models.user import User from app.schemas.common import MessageResponse from app.schemas.tenant import TenantCreate, TenantPublic, TenantUpdate from app.services.audit import field_changes, record_audit, serialize_row +from app.services.organization_scope import ( + CrossOrgReference, + OwnerRequired, + ScopeBlocked, + resolve_org_membership, + scope_exception_to_http, + scoped_get_tenant, + scoped_list_tenants, +) router = APIRouter(prefix="/tenants", tags=["tenants"]) -def _get_or_404(db: Session, tenant_id: int) -> Tenant: - obj = ( - db.query(Tenant) - .filter(Tenant.id == tenant_id, Tenant.deleted_at.is_(None)) - .first() - ) - if obj is None: - raise HTTPException(status.HTTP_404_NOT_FOUND, "Tenant not found") - return obj - - def _to_public(obj: Tenant) -> TenantPublic: - """Safe tenant read shape: the raw id_number / id file ids are NEVER - returned (group / Daily Digest / archive bodies can only show the - ``id_registered`` boolean = ``ID：已登记``).""" return TenantPublic( id=obj.id, full_name=obj.full_name, @@ -50,10 +47,13 @@ def _to_public(obj: Tenant) -> TenantPublic: @router.get("", response_model=list[TenantPublic]) -def list_tenants(db: Session = Depends(get_db), _: Tenant = Depends(get_current_user)): +def list_tenants( + db: Session = Depends(get_db), + user: User = Depends(get_current_user), +): return [ _to_public(obj) - for obj in db.query(Tenant).filter(Tenant.deleted_at.is_(None)).order_by(Tenant.id).all() + for obj in scoped_list_tenants(db, for_user_id=user.id) ] @@ -61,8 +61,17 @@ def list_tenants(db: Session = Depends(get_db), _: Tenant = Depends(get_current_ def create_tenant( payload: TenantCreate, db: Session = Depends(get_db), - user: Tenant = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): + try: + resolve_org_membership( + db, + user.id, + payload.organization_id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except (ScopeBlocked, LookupError) as exc: + raise scope_exception_to_http(exc) from exc obj = Tenant(**payload.model_dump()) obj.created_by = user.id obj.updated_by = user.id @@ -83,9 +92,20 @@ def create_tenant( @router.get("/{tenant_id}", response_model=TenantPublic) def get_tenant( - tenant_id: int, db: Session = Depends(get_db), _: Tenant = Depends(get_current_user) + tenant_id: int, + db: Session = Depends(get_db), + user: User = Depends(get_current_user), ): - return _to_public(_get_or_404(db, tenant_id)) + try: + obj, _ = scoped_get_tenant( + db, + tenant_id, + for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + except (LookupError, ScopeBlocked) as exc: + raise scope_exception_to_http(exc) from exc + return _to_public(obj) @router.patch("/{tenant_id}", response_model=TenantPublic) @@ -93,10 +113,22 @@ def update_tenant( tenant_id: int, payload: TenantUpdate, db: Session = Depends(get_db), - user: Tenant = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, tenant_id) - updates = payload.model_dump(exclude_unset=True) + try: + obj, _ = scoped_get_tenant( + db, + tenant_id, + for_user_id=user.id, + role=[OrganizationRole.OWNER, OrganizationRole.SECRETARY], + ) + updates = payload.model_dump(exclude_unset=True) + if "organization_id" in updates: + raise CrossOrgReference( + "Tenant organization_id is immutable in this milestone; use a dedicated transfer workflow" + ) + except (LookupError, ScopeBlocked, CrossOrgReference) as exc: + raise scope_exception_to_http(exc) from exc if not updates: return _to_public(obj) old = serialize_row(obj) @@ -123,9 +155,17 @@ def update_tenant( def delete_tenant( tenant_id: int, db: Session = Depends(get_db), - user: Tenant = Depends(manager_or_admin), + user: User = Depends(get_current_user), ): - obj = _get_or_404(db, tenant_id) + try: + obj, _ = scoped_get_tenant( + db, + tenant_id, + for_user_id=user.id, + role=OrganizationRole.OWNER, + ) + except (LookupError, ScopeBlocked, OwnerRequired) as exc: + raise scope_exception_to_http(exc) from exc from datetime import datetime, timezone obj.deleted_at = datetime.now(timezone.utc) diff --git a/app/api/routers/units.py b/app/api/routers/units.py index 6dcec46..bdd268e 100644 --- a/app/api/routers/units.py +++ b/app/api/routers/units.py @@ -11,6 +11,7 @@ from app.schemas.common import MessageResponse from app.schemas.property import UnitCreate, UnitRead, UnitUpdate from app.services.audit import field_changes, record_audit, serialize_row +from app.services.organization_scope import scope_exception_to_http from app.services.property_channel import ( OwnerRequired, ScopeBlocked, @@ -23,14 +24,6 @@ router = APIRouter(prefix="/units", tags=["units"]) -def _scope_exception_to_http(exc: Exception) -> HTTPException: - if isinstance(exc, LookupError): - return HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) - if isinstance(exc, (ScopeBlocked, OwnerRequired)): - return HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) - return HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR, type(exc).__name__) - - @router.get("", response_model=list[UnitRead]) def list_units( db: Session = Depends(get_db), user: User = Depends(get_current_user) @@ -94,7 +87,7 @@ def get_unit( try: unit, _membership = scoped_get_unit(db, unit_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc + raise scope_exception_to_http(exc) from exc return unit @@ -108,14 +101,14 @@ def update_unit( try: obj, membership = scoped_get_unit(db, unit_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc + raise scope_exception_to_http(exc) from exc updates = payload.model_dump(exclude_unset=True) if not updates: return obj try: - if membership.role != OrganizationRole.OWNER.value: + if membership.role != OrganizationRole.OWNER: filter_secretary_unit_updates(set(updates.keys())) except OwnerRequired as exc: raise HTTPException(status.HTTP_403_FORBIDDEN, str(exc)) from exc @@ -171,8 +164,8 @@ def delete_unit( try: obj, membership = scoped_get_unit(db, unit_id, for_user_id=user.id) except Exception as exc: # noqa: BLE001 - raise _scope_exception_to_http(exc) from exc - if membership.role != OrganizationRole.OWNER.value: + raise scope_exception_to_http(exc) from exc + if membership.role != OrganizationRole.OWNER: raise HTTPException( status.HTTP_403_FORBIDDEN, "Only ACTIVE OWNER may soft-delete a Unit", diff --git a/app/models/financial.py b/app/models/financial.py index 19bba59..fed5f2a 100644 --- a/app/models/financial.py +++ b/app/models/financial.py @@ -1,100 +1,103 @@ -from datetime import date, datetime -from enum import Enum -from decimal import Decimal - -from sqlalchemy import BigInteger, Date, DateTime, ForeignKey, Index, Numeric, String, Text, func, text -from sqlalchemy.orm import Mapped, mapped_column - -from app.models.base import AuditMixin, Base, pg_enum - - -class IncomeStatus(str, Enum): - pending = "pending" - confirmed = "confirmed" - reversed = "reversed" - - -class Income(AuditMixin, Base): - __tablename__ = "incomes" - __table_args__ = ( - Index( - "uq_incomes_idempotency_key", - "idempotency_key", - unique=True, - postgresql_where=text("idempotency_key IS NOT NULL"), - ), - ) - - lease_id: Mapped[int | None] = mapped_column(ForeignKey("leases.id"), nullable=True, index=True) - amount: Mapped[Decimal] = mapped_column(Numeric(14, 2), nullable=False) - received_date: Mapped[date] = mapped_column(Date, nullable=False) - payment_method: Mapped[str | None] = mapped_column(String(50), nullable=True) - idempotency_key: Mapped[str | None] = mapped_column(String(128), nullable=True) - status: Mapped[IncomeStatus] = mapped_column( - pg_enum(IncomeStatus, "income_status"), nullable=False, default=IncomeStatus.pending - ) - description: Mapped[str | None] = mapped_column(Text, nullable=True) - confirmed_by: Mapped[int | None] = mapped_column(BigInteger, nullable=True) - confirmed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) - - -class ExpenseStatus(str, Enum): - pending = "pending" - approved = "approved" - rejected = "rejected" - paid = "paid" - reversed = "reversed" - # PASAY-EXPENSE-OPERATION-003B: distinct truth states between APPROVED and - # PAID. A payment claim has been reported but is NOT yet verified - # (payment_claimed = "payment reported / awaiting verification"); a partial - # verified amount makes the expense PARTIALLY_PAID until the full amount is - # VERIFIED. `paid` is reached ONLY via verified-claims aggregation. - payment_claimed = "payment_claimed" - partially_paid = "partially_paid" - - -class Expense(AuditMixin, Base): - __tablename__ = "expenses" - - expense_date: Mapped[date] = mapped_column(Date, nullable=False) - due_date: Mapped[date | None] = mapped_column(Date, nullable=True) - category: Mapped[str] = mapped_column(String(100), nullable=False) - amount: Mapped[Decimal] = mapped_column(Numeric(14, 2), nullable=False) - payee: Mapped[str] = mapped_column(String(200), nullable=False) - description: Mapped[str | None] = mapped_column(Text, nullable=True) - unit_id: Mapped[int | None] = mapped_column( - ForeignKey("units.id"), nullable=True, index=True - ) - status: Mapped[ExpenseStatus] = mapped_column( - pg_enum(ExpenseStatus, "expense_status"), nullable=False, default=ExpenseStatus.pending - ) - approved_by: Mapped[int | None] = mapped_column(BigInteger, nullable=True) - approved_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) - receipt_attachment_id: Mapped[int | None] = mapped_column( - ForeignKey("attachments.id"), nullable=True - ) - # AI-OPS-FOUNDATION-001 §4/§8: the ACTUAL human responsible for payment. - # After approval the PAYMENT_PENDING task routes to this payer, not always - # the Owner; None falls back to the Owner at generation time. - payer_user_id: Mapped[int | None] = mapped_column( - ForeignKey("users.id"), nullable=True, index=True - ) - # PASAY-EXPENSE-OPERATION-003B: approve-vs-pay, reject->resubmit, and - # critical-field reapproval continuity. - # - # rejection_reason: Owner's reason when the CURRENT version was rejected - # (section 8) — preserved so the next version + Mini App/timeline can show - # why V1 was rejected. - rejection_reason: Mapped[str | None] = mapped_column(Text, nullable=True) - # reapproval_reason: why a previously-approved expense was demoted back to - # PENDING because a critical financial field changed (section 9). - reapproval_reason: Mapped[str | None] = mapped_column(Text, nullable=True) - # version: an increasing version counter for the SAME logical expense need. - # Reject->edit->resubmit and critical-field reapproval bump this; the OLD - # rejected state is never overwritten in place (section 8/9). - version: Mapped[int | None] = mapped_column(BigInteger, nullable=True, default=1) - # parent_expense_id: points to the immediately-previous version row, so the - # family (V1 REJECTED -> V2 PENDING) is preserved without a separate table. - parent_expense_id: Mapped[int | None] = mapped_column( - BigInteger, nullable=True, index=True - ) +from datetime import date, datetime +from enum import Enum +from decimal import Decimal + +from sqlalchemy import BigInteger, Date, DateTime, ForeignKey, Index, Numeric, String, Text, func, text +from sqlalchemy.orm import Mapped, mapped_column + +from app.models.base import AuditMixin, Base, pg_enum + + +class IncomeStatus(str, Enum): + pending = "pending" + confirmed = "confirmed" + reversed = "reversed" + + +class Income(AuditMixin, Base): + __tablename__ = "incomes" + __table_args__ = ( + Index( + "uq_incomes_idempotency_key", + "idempotency_key", + unique=True, + postgresql_where=text("idempotency_key IS NOT NULL"), + ), + ) + + lease_id: Mapped[int | None] = mapped_column(ForeignKey("leases.id"), nullable=True, index=True) + amount: Mapped[Decimal] = mapped_column(Numeric(14, 2), nullable=False) + received_date: Mapped[date] = mapped_column(Date, nullable=False) + payment_method: Mapped[str | None] = mapped_column(String(50), nullable=True) + idempotency_key: Mapped[str | None] = mapped_column(String(128), nullable=True) + status: Mapped[IncomeStatus] = mapped_column( + pg_enum(IncomeStatus, "income_status"), nullable=False, default=IncomeStatus.pending + ) + description: Mapped[str | None] = mapped_column(Text, nullable=True) + confirmed_by: Mapped[int | None] = mapped_column(BigInteger, nullable=True) + confirmed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + + +class ExpenseStatus(str, Enum): + pending = "pending" + approved = "approved" + rejected = "rejected" + paid = "paid" + reversed = "reversed" + # PASAY-EXPENSE-OPERATION-003B: distinct truth states between APPROVED and + # PAID. A payment claim has been reported but is NOT yet verified + # (payment_claimed = "payment reported / awaiting verification"); a partial + # verified amount makes the expense PARTIALLY_PAID until the full amount is + # VERIFIED. `paid` is reached ONLY via verified-claims aggregation. + payment_claimed = "payment_claimed" + partially_paid = "partially_paid" + + +class Expense(AuditMixin, Base): + __tablename__ = "expenses" + + expense_date: Mapped[date] = mapped_column(Date, nullable=False) + due_date: Mapped[date | None] = mapped_column(Date, nullable=True) + category: Mapped[str] = mapped_column(String(100), nullable=False) + amount: Mapped[Decimal] = mapped_column(Numeric(14, 2), nullable=False) + payee: Mapped[str] = mapped_column(String(200), nullable=False) + description: Mapped[str | None] = mapped_column(Text, nullable=True) + unit_id: Mapped[int | None] = mapped_column( + ForeignKey("units.id"), nullable=True, index=True + ) + property_id: Mapped[int] = mapped_column( + ForeignKey("properties.id"), nullable=False, index=True + ) + status: Mapped[ExpenseStatus] = mapped_column( + pg_enum(ExpenseStatus, "expense_status"), nullable=False, default=ExpenseStatus.pending + ) + approved_by: Mapped[int | None] = mapped_column(BigInteger, nullable=True) + approved_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + receipt_attachment_id: Mapped[int | None] = mapped_column( + ForeignKey("attachments.id"), nullable=True + ) + # AI-OPS-FOUNDATION-001 §4/§8: the ACTUAL human responsible for payment. + # After approval the PAYMENT_PENDING task routes to this payer, not always + # the Owner; None falls back to the Owner at generation time. + payer_user_id: Mapped[int | None] = mapped_column( + ForeignKey("users.id"), nullable=True, index=True + ) + # PASAY-EXPENSE-OPERATION-003B: approve-vs-pay, reject->resubmit, and + # critical-field reapproval continuity. + # + # rejection_reason: Owner's reason when the CURRENT version was rejected + # (section 8) — preserved so the next version + Mini App/timeline can show + # why V1 was rejected. + rejection_reason: Mapped[str | None] = mapped_column(Text, nullable=True) + # reapproval_reason: why a previously-approved expense was demoted back to + # PENDING because a critical financial field changed (section 9). + reapproval_reason: Mapped[str | None] = mapped_column(Text, nullable=True) + # version: an increasing version counter for the SAME logical expense need. + # Reject->edit->resubmit and critical-field reapproval bump this; the OLD + # rejected state is never overwritten in place (section 8/9). + version: Mapped[int | None] = mapped_column(BigInteger, nullable=True, default=1) + # parent_expense_id: points to the immediately-previous version row, so the + # family (V1 REJECTED -> V2 PENDING) is preserved without a separate table. + parent_expense_id: Mapped[int | None] = mapped_column( + BigInteger, nullable=True, index=True + ) diff --git a/app/models/property.py b/app/models/property.py index dd2cf60..af0f8a0 100644 --- a/app/models/property.py +++ b/app/models/property.py @@ -18,8 +18,8 @@ class Property(AuditMixin, SoftDeleteMixin, Base): ), ) - organization_id: Mapped[int | None] = mapped_column( - BigInteger, ForeignKey("organizations.id"), nullable=True, index=True + organization_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("organizations.id"), nullable=False, index=True ) name: Mapped[str] = mapped_column(String(200), nullable=False) address: Mapped[str] = mapped_column(String(500), nullable=False) diff --git a/app/models/tenant.py b/app/models/tenant.py index c61b93b..79b2fdd 100644 --- a/app/models/tenant.py +++ b/app/models/tenant.py @@ -1,54 +1,57 @@ -from datetime import datetime -from enum import Enum - -from sqlalchemy import Boolean, DateTime, String -from sqlalchemy.orm import Mapped, mapped_column - -from app.models.base import AuditMixin, Base, SoftDeleteMixin, pg_enum - - -class TenantContactStatus(str, Enum): - """PASAY-AI-EMPLOYEE-FOUNDATION-007 §3: tenant contact validation state.""" - - UNKNOWN = "UNKNOWN" - UNVERIFIED = "UNVERIFIED" - VERIFIED = "VERIFIED" - WRONG_NUMBER = "WRONG_NUMBER" - UNREACHABLE = "UNREACHABLE" - CHANGED = "CHANGED" - - -class Tenant(AuditMixin, SoftDeleteMixin, Base): - __tablename__ = "tenants" - - full_name: Mapped[str] = mapped_column(String(200), nullable=False) - phone: Mapped[str | None] = mapped_column(String(50), nullable=True) - # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3: structured contact fields. The - # legacy ``phone`` column doubles as the primary phone for back-compat. - secondary_phone: Mapped[str | None] = mapped_column(String(50), nullable=True) - telegram: Mapped[str | None] = mapped_column(String(100), nullable=True) - whatsapp: Mapped[str | None] = mapped_column(String(50), nullable=True) - email: Mapped[str | None] = mapped_column(String(200), nullable=True) - contact_status: Mapped[TenantContactStatus | None] = mapped_column( - pg_enum(TenantContactStatus, "tenant_contact_status"), nullable=True - ) - last_confirmed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) - last_confirmed_by: Mapped[str | None] = mapped_column(String(200), nullable=True) - notes: Mapped[str | None] = mapped_column(String(1000), nullable=True) - nationality: Mapped[str | None] = mapped_column(String(100), nullable=True) - # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3.1: tenant identity is SENSITIVE. The - # structured id number is stored here but the DEFAULT read path redacts it - # (``id_number`` returns None and ``id_registered`` carries a boolean), so - # group / Daily Digest / archive bodies can only ever show ``ID：已登记``. - id_number: Mapped[str | None] = mapped_column(String(100), nullable=True) - id_front_file_id: Mapped[str | None] = mapped_column(String(300), nullable=True) - id_back_file_id: Mapped[str | None] = mapped_column(String(300), nullable=True) - # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3.2: structured emergency contact. - emergency_name: Mapped[str | None] = mapped_column(String(200), nullable=True) - emergency_relationship: Mapped[str | None] = mapped_column(String(100), nullable=True) - emergency_phone: Mapped[str | None] = mapped_column(String(50), nullable=True) - is_active: Mapped[bool] = mapped_column(Boolean, nullable=False, default=True) - - @property - def primary_phone(self) -> str | None: - return self.phone +from datetime import datetime +from enum import Enum + +from sqlalchemy import BigInteger, Boolean, DateTime, ForeignKey, String +from sqlalchemy.orm import Mapped, mapped_column + +from app.models.base import AuditMixin, Base, SoftDeleteMixin, pg_enum + + +class TenantContactStatus(str, Enum): + """PASAY-AI-EMPLOYEE-FOUNDATION-007 §3: tenant contact validation state.""" + + UNKNOWN = "UNKNOWN" + UNVERIFIED = "UNVERIFIED" + VERIFIED = "VERIFIED" + WRONG_NUMBER = "WRONG_NUMBER" + UNREACHABLE = "UNREACHABLE" + CHANGED = "CHANGED" + + +class Tenant(AuditMixin, SoftDeleteMixin, Base): + __tablename__ = "tenants" + + organization_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("organizations.id"), nullable=False, index=True + ) + full_name: Mapped[str] = mapped_column(String(200), nullable=False) + phone: Mapped[str | None] = mapped_column(String(50), nullable=True) + # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3: structured contact fields. The + # legacy ``phone`` column doubles as the primary phone for back-compat. + secondary_phone: Mapped[str | None] = mapped_column(String(50), nullable=True) + telegram: Mapped[str | None] = mapped_column(String(100), nullable=True) + whatsapp: Mapped[str | None] = mapped_column(String(50), nullable=True) + email: Mapped[str | None] = mapped_column(String(200), nullable=True) + contact_status: Mapped[TenantContactStatus | None] = mapped_column( + pg_enum(TenantContactStatus, "tenant_contact_status"), nullable=True + ) + last_confirmed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + last_confirmed_by: Mapped[str | None] = mapped_column(String(200), nullable=True) + notes: Mapped[str | None] = mapped_column(String(1000), nullable=True) + nationality: Mapped[str | None] = mapped_column(String(100), nullable=True) + # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3.1: tenant identity is SENSITIVE. The + # structured id number is stored here but the DEFAULT read path redacts it + # (``id_number`` returns None and ``id_registered`` carries a boolean), so + # group / Daily Digest / archive bodies can only ever show ``ID：已登记``. + id_number: Mapped[str | None] = mapped_column(String(100), nullable=True) + id_front_file_id: Mapped[str | None] = mapped_column(String(300), nullable=True) + id_back_file_id: Mapped[str | None] = mapped_column(String(300), nullable=True) + # PASAY-AI-EMPLOYEE-FOUNDATION-007 §3.2: structured emergency contact. + emergency_name: Mapped[str | None] = mapped_column(String(200), nullable=True) + emergency_relationship: Mapped[str | None] = mapped_column(String(100), nullable=True) + emergency_phone: Mapped[str | None] = mapped_column(String(50), nullable=True) + is_active: Mapped[bool] = mapped_column(Boolean, nullable=False, default=True) + + @property + def primary_phone(self) -> str | None: + return self.phone diff --git a/app/schemas/financial.py b/app/schemas/financial.py index abf71f3..72542e5 100644 --- a/app/schemas/financial.py +++ b/app/schemas/financial.py @@ -1,179 +1,179 @@ -from datetime import date, datetime -from decimal import Decimal - -from pydantic import BaseModel, Field, field_validator - -from app.models import ExpenseStatus, IncomeStatus -from app.schemas.common import AuditFields, money_field - -# P1-PASAY-NIGHTLY-PRODUCT-HARDENING-008 A2: placeholder/empty text is never a -# meaningful expense identity. These sentinels mirror the read-path cleaners -# (app.services.operations.quick._clean_text and the bot's -# _clean_free_text) so the write path rejects at the boundary what the read -# path would otherwise have to hide. Payee keeps the established `-` -# "unknown vendor" sentinel (the bot's DB NOT NULL contract); it is never a -# displayed purpose (renderers drop it). -_PLACEHOLDER_SENTINELS = {"??", "?", "--", "none", "null", "n/a", "na", "unknown"} - - -def _meaningful_label(value: str, *, field_name: str, allow_dash: bool = False) -> str: - """Normalize a human label and reject empty/whitespace/placeholder text. - - Raises ValueError (pydantic 422) when no meaningful label remains, so a - newly created Expense can never carry `??`, whitespace-only or equivalent - placeholder text as its identity (A2).""" - text = " ".join(str(value).split()) - if not text: - raise ValueError(f"{field_name} must not be empty or whitespace") - lowered = text.lower() - if lowered in _PLACEHOLDER_SENTINELS or (not allow_dash and lowered == "-"): - raise ValueError( - f"{field_name} must be a meaningful human-readable label, not a placeholder" - ) - return text - - -class IncomeBase(BaseModel): - lease_id: int | None = None - amount: Decimal = money_field(gt=0) - received_date: date - payment_method: str | None = Field(default=None, max_length=50) - idempotency_key: str | None = Field(default=None, max_length=128) - status: IncomeStatus - description: str | None = None - - -class IncomeCreate(IncomeBase): - pass - - -class IncomeUpdate(BaseModel): - lease_id: int | None = None - amount: Decimal | None = money_field(gt=0) - received_date: date | None = None - payment_method: str | None = Field(default=None, max_length=50) - description: str | None = None - - -class IncomeRead(IncomeBase, AuditFields): - id: int - confirmed_by: int | None = None - confirmed_at: datetime | None = None - - -class ExpenseBase(BaseModel): - expense_date: date - due_date: date | None = None - category: str = Field(min_length=1, max_length=100) - amount: Decimal = money_field(gt=0) - payee: str = Field(min_length=1, max_length=200) - description: str | None = None - unit_id: int | None = None - status: ExpenseStatus - receipt_attachment_id: int | None = None - # AI-OPS-FOUNDATION-001 §4/§8: the actual payer; payment responsibility - # routes to this user after approval instead of always the Owner. - payer_user_id: int | None = None - - -class ExpenseCreate(ExpenseBase): - @field_validator("category") - @classmethod - def _category_meaningful(cls, value: str) -> str: - return _meaningful_label(value, field_name="category") - - @field_validator("payee") - @classmethod - def _payee_meaningful(cls, value: str) -> str: - # `-` stays allowed: it is the bot's established DB-NOT-NULL "unknown - # vendor" sentinel and is never rendered as a purpose. - return _meaningful_label(value, field_name="payee", allow_dash=True) - - -class ExpenseUpdate(BaseModel): - expense_date: date | None = None - due_date: date | None = None - category: str | None = Field(default=None, min_length=1, max_length=100) - amount: Decimal | None = money_field(gt=0, default=None) - payee: str | None = Field(default=None, min_length=1, max_length=200) - description: str | None = None - unit_id: int | None = None - receipt_attachment_id: int | None = None - payer_user_id: int | None = None - - @field_validator("category") - @classmethod - def _category_meaningful(cls, value: str | None) -> str | None: - if value is None: - return None - return _meaningful_label(value, field_name="category") - - @field_validator("payee") - @classmethod - def _payee_meaningful(cls, value: str | None) -> str | None: - if value is None: - return None - return _meaningful_label(value, field_name="payee", allow_dash=True) - - -class ExpenseRead(ExpenseBase, AuditFields): - id: int - approved_by: int | None = None - approved_at: datetime | None = None - # PASAY-EXPENSE-OPERATION-003B: additional facts surfaced on reads. - rejection_reason: str | None = None - reapproval_reason: str | None = None - version: int | None = None - parent_expense_id: int | None = None - - -class PaymentClaimIn(BaseModel): - """Create/verify/fail/reverse a payment claim.""" - claimed_amount: Decimal | None = Field(default=None, gt=0) - verification_note: str | None = None - evidence_ids: list[int] | None = None - idempotency_key: str | None = Field(default=None, max_length=255) - # verify-specific - verified_amount: Decimal | None = Field(default=None, gt=0) - result: str | None = None - reason: str | None = Field(default=None, max_length=500) - - -class PaymentClaimOut(BaseModel): - id: int - expense_id: int - claimed_amount: str - claimed_by: int | None = None - claimed_at: datetime | None = None - status: str - evidence_ids: list = [] - verification_note: str | None = None - verified_amount: str | None = None - verified_by: int | None = None - verified_at: datetime | None = None - mismatch: bool = False - mismatch_reason: str | None = None - failure_reason: str | None = None - - -class ExpensePaymentInfo(BaseModel): - required_amount: str - verified_paid: str - remaining: str - fully_paid: bool - pending_claims: int - has_mismatch: bool - verified_claim_count: int = 0 - pending_claim_count: int = 0 - claims: list[dict] = [] - - -class ExpenseDetailOut(ExpenseRead): - """Mini App full detail view (PASAY-EXPENSE-OPERATION-003B section 15): - Expense / Approval / Payment / Claims / Evidence / Verification / - Actions / Timeline — all derived from the real rows.""" - payment: ExpensePaymentInfo - claims: list[PaymentClaimOut] = [] - evidence: dict | None = None - timeline: list[dict] = [] - reviewed: dict | None = None +from datetime import date, datetime +from decimal import Decimal + +from pydantic import BaseModel, Field, field_validator + +from app.models import ExpenseStatus, IncomeStatus +from app.schemas.common import AuditFields, money_field + +# P1-PASAY-NIGHTLY-PRODUCT-HARDENING-008 A2: placeholder/empty text is never a +# meaningful expense identity. These sentinels mirror the read-path cleaners +# (app.services.operations.quick._clean_text and the bot's +# _clean_free_text) so the write path rejects at the boundary what the read +# path would otherwise have to hide. Payee keeps the established `-` +# "unknown vendor" sentinel (the bot's DB NOT NULL contract); it is never a +# displayed purpose (renderers drop it). +_PLACEHOLDER_SENTINELS = {"??", "?", "--", "none", "null", "n/a", "na", "unknown"} + + +def _meaningful_label(value: str, *, field_name: str, allow_dash: bool = False) -> str: + """Normalize a human label and reject empty/whitespace/placeholder text. + + Raises ValueError (pydantic 422) when no meaningful label remains, so a + newly created Expense can never carry `??`, whitespace-only or equivalent + placeholder text as its identity (A2).""" + text = " ".join(str(value).split()) + if not text: + raise ValueError(f"{field_name} must not be empty or whitespace") + lowered = text.lower() + if lowered in _PLACEHOLDER_SENTINELS or (not allow_dash and lowered == "-"): + raise ValueError( + f"{field_name} must be a meaningful human-readable label, not a placeholder" + ) + return text + + +class IncomeBase(BaseModel): + lease_id: int + amount: Decimal = money_field(gt=0) + received_date: date + payment_method: str | None = Field(default=None, max_length=50) + idempotency_key: str | None = Field(default=None, max_length=128) + status: IncomeStatus + description: str | None = None + + +class IncomeCreate(IncomeBase): + pass + + +class IncomeUpdate(BaseModel): + lease_id: int | None = None + amount: Decimal | None = money_field(gt=0) + received_date: date | None = None + payment_method: str | None = Field(default=None, max_length=50) + description: str | None = None + + +class IncomeRead(IncomeBase, AuditFields): + id: int + confirmed_by: int | None = None + confirmed_at: datetime | None = None + + +class ExpenseBase(BaseModel): + expense_date: date + due_date: date | None = None + category: str = Field(min_length=1, max_length=100) + amount: Decimal = money_field(gt=0) + payee: str = Field(min_length=1, max_length=200) + description: str | None = None + unit_id: int | None = None + property_id: int | None = None + status: ExpenseStatus + receipt_attachment_id: int | None = None + payer_user_id: int | None = None + + +class ExpenseCreate(ExpenseBase): + @field_validator("category") + @classmethod + def _category_meaningful(cls, value: str) -> str: + return _meaningful_label(value, field_name="category") + + @field_validator("payee") + @classmethod + def _payee_meaningful(cls, value: str) -> str: + # `-` stays allowed: it is the bot's established DB-NOT-NULL "unknown + # vendor" sentinel and is never rendered as a purpose. + return _meaningful_label(value, field_name="payee", allow_dash=True) + + +class ExpenseUpdate(BaseModel): + expense_date: date | None = None + due_date: date | None = None + category: str | None = Field(default=None, min_length=1, max_length=100) + amount: Decimal | None = money_field(gt=0, default=None) + payee: str | None = Field(default=None, min_length=1, max_length=200) + description: str | None = None + unit_id: int | None = None + property_id: int | None = None + receipt_attachment_id: int | None = None + payer_user_id: int | None = None + + @field_validator("category") + @classmethod + def _category_meaningful(cls, value: str | None) -> str | None: + if value is None: + return None + return _meaningful_label(value, field_name="category") + + @field_validator("payee") + @classmethod + def _payee_meaningful(cls, value: str | None) -> str | None: + if value is None: + return None + return _meaningful_label(value, field_name="payee", allow_dash=True) + + +class ExpenseRead(ExpenseBase, AuditFields): + id: int + approved_by: int | None = None + approved_at: datetime | None = None + # PASAY-EXPENSE-OPERATION-003B: additional facts surfaced on reads. + rejection_reason: str | None = None + reapproval_reason: str | None = None + version: int | None = None + parent_expense_id: int | None = None + + +class PaymentClaimIn(BaseModel): + """Create/verify/fail/reverse a payment claim.""" + claimed_amount: Decimal | None = Field(default=None, gt=0) + verification_note: str | None = None + evidence_ids: list[int] | None = None + idempotency_key: str | None = Field(default=None, max_length=255) + # verify-specific + verified_amount: Decimal | None = Field(default=None, gt=0) + result: str | None = None + reason: str | None = Field(default=None, max_length=500) + + +class PaymentClaimOut(BaseModel): + id: int + expense_id: int + claimed_amount: str + claimed_by: int | None = None + claimed_at: datetime | None = None + status: str + evidence_ids: list = [] + verification_note: str | None = None + verified_amount: str | None = None + verified_by: int | None = None + verified_at: datetime | None = None + mismatch: bool = False + mismatch_reason: str | None = None + failure_reason: str | None = None + + +class ExpensePaymentInfo(BaseModel): + required_amount: str + verified_paid: str + remaining: str + fully_paid: bool + pending_claims: int + has_mismatch: bool + verified_claim_count: int = 0 + pending_claim_count: int = 0 + claims: list[dict] = [] + + +class ExpenseDetailOut(ExpenseRead): + """Mini App full detail view (PASAY-EXPENSE-OPERATION-003B section 15): + Expense / Approval / Payment / Claims / Evidence / Verification / + Actions / Timeline — all derived from the real rows.""" + payment: ExpensePaymentInfo + claims: list[PaymentClaimOut] = [] + evidence: dict | None = None + timeline: list[dict] = [] + reviewed: dict | None = None diff --git a/app/schemas/tenant.py b/app/schemas/tenant.py index 12b37d7..835e770 100644 --- a/app/schemas/tenant.py +++ b/app/schemas/tenant.py @@ -26,10 +26,11 @@ class TenantBase(BaseModel): class TenantCreate(TenantBase): - pass + organization_id: int class TenantUpdate(BaseModel): + organization_id: int | None = None full_name: str | None = Field(default=None, min_length=1, max_length=200) phone: str | None = Field(default=None, max_length=50) secondary_phone: str | None = Field(default=None, max_length=50) diff --git a/app/services/organization_scope.py b/app/services/organization_scope.py new file mode 100644 index 0000000..791b014 --- /dev/null +++ b/app/services/organization_scope.py @@ -0,0 +1,615 @@ +"""Canonical Organization Scope Foundation (PASAY-MILESTONE-001). + +Single home for every domain's organization-scoped read and write guards. + +Ownership chains (M1 canonical): + Property -> organization_id (the root boundary; NOT NULL after M1) + Unit -> property_id -> Property.organization_id + Lease -> unit_id -> Unit.property_id -> Property.organization_id + Income -> lease_id -> Lease.unit_id -> ... -> Property.organization_id + Expense -> unit_id -> Unit.property_id -> Property.organization_id + Repair -> unit_id/property_id -> ... -> Property.organization_id + Tenant -> organization_id (NOT NULL added in M1) + +Permission semantics (§5 in M1 contract): + * list: only rows whose ownership chain resolves to an Organization + where the caller has an ACTIVE Membership. + * lookup: cross-org object -> 404 fail-closed (not 403). + * action: same Org but Membership.role insufficient -> 403. + * writes: any referenced foreign object (Unit/Tenant/Lease/Expense/...) + MUST be co-org with the caller's ACTIVE Membership, else 409. + +Philosophy: + * New human-authority code paths in this module exclusively use + Membership (ACTIVE + OWNER/SECRETARY). + * Legacy users.role is NEVER consulted here. + * NULL ownership chains -> LookupError (mapped to 404 at the router). +""" +from __future__ import annotations + +from collections.abc import Iterable + +from fastapi import HTTPException, status +from sqlalchemy import and_, select +from sqlalchemy.orm import Session + +from app.models.financial import Expense, Income +from app.models.lease import Lease +from app.models.membership import ( + Membership, + MembershipState, + OrganizationRole, +) +from app.models.property import Property, Unit +from app.models.repair import RepairOperation +from app.models.tenant import Tenant +from app.services.membership import has_active_membership + +# --------------------------------------------------------------------------- +# Exceptions +# --------------------------------------------------------------------------- + + +class ScopeBlocked(PermissionError): + """Caller has no ACTIVE Membership in the target Organization.""" + + +class OwnerRequired(PermissionError): + """Caller is ACTIVE SECRETARY; the action needs ACTIVE OWNER.""" + + +class CrossOrgReference(ValueError): + """A create/update payload references an object belonging to a + different Organization than the caller's ACTIVE Membership.""" + + +# --------------------------------------------------------------------------- +# Shared HTTP translator (Owner contract §7 / FIX1 contract #7): +# * LookupError (cross-org object lookup or missing object) -> HTTP 404 +# (fail-closed, does not leak existence). +# * ScopeBlocked / OwnerRequired (same org, Membership.role insufficient +# or no ACTIVE membership) -> HTTP 403. +# * CrossOrgReference (create/update FK referencing other org) -> HTTP 409. +# * Any other exception -> propagate unchanged (do not swallow unknowns +# or leak type(exc).__name__ in a 500 body). +# --------------------------------------------------------------------------- + + +def scope_exception_to_http(exc: Exception) -> HTTPException: + if isinstance(exc, LookupError): + return HTTPException(status.HTTP_404_NOT_FOUND, str(exc) or "Not found") + if isinstance(exc, (ScopeBlocked, OwnerRequired)): + return HTTPException(status.HTTP_403_FORBIDDEN, str(exc) or "Forbidden") + if isinstance(exc, CrossOrgReference): + return HTTPException(status.HTTP_409_CONFLICT, str(exc) or "Conflict") + raise exc + + +# --------------------------------------------------------------------------- +# Membership resolution +# --------------------------------------------------------------------------- + + +def resolve_org_membership( + db: Session, + user_id: int, + organization_id: int, + *, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> Membership: + """Return the ACTIVE Membership for (user_id, organization_id) or raise. + + * No ACTIVE membership at all -> :class:`ScopeBlocked` (maps to HTTP 403). + * ACTIVE membership exists but role does not match the requested role -> + :class:`OwnerRequired` (maps to HTTP 403). + """ + m = has_active_membership(db, user_id, organization_id, role=None) + if m is None: + hint = "ACTIVE" + raise ScopeBlocked( + f"user_id={user_id!r} has no {hint} membership in org={organization_id!r}" + ) + if role is not None: + roles = list(role) if not isinstance(role, OrganizationRole) else [role] + if m.role not in roles: + hint = "ACTIVE " + "/".join(r.value for r in roles) + raise OwnerRequired( + f"user_id={user_id!r} has ACTIVE {m.role.value} membership in " + f"org={organization_id!r}; action requires {hint}" + ) + return m + + +def list_active_org_ids_for_user(db: Session, user_id: int) -> list[int]: + """Return every organization_id where the user holds an ACTIVE Membership. + + Used by all scoped_list_* helpers to JOIN-filter lists. Empty list = the + caller sees nothing. + """ + rows = ( + db.query(Membership.organization_id) + .filter( + Membership.user_id == user_id, + Membership.state == MembershipState.ACTIVE, + Membership.removed_at.is_(None), + ) + .all() + ) + return [r[0] for r in rows] + + +# --------------------------------------------------------------------------- +# Ownership-chain org_id resolvers +# --------------------------------------------------------------------------- + + +def property_org_id(db: Session, property_id: int) -> int | None: + row = ( + db.query(Property.organization_id) + .filter(Property.id == property_id, Property.deleted_at.is_(None)) + .one_or_none() + ) + return row[0] if row else None + + +def unit_org_id(db: Session, unit_id: int) -> int | None: + row = ( + db.query(Property.organization_id) + .select_from(Unit) + .join(Property, Property.id == Unit.property_id) + .filter(Unit.id == unit_id, Unit.deleted_at.is_(None)) + .one_or_none() + ) + return row[0] if row else None + + +def lease_org_id(db: Session, lease_id: int) -> int | None: + row = ( + db.query(Property.organization_id) + .select_from(Lease) + .join(Unit, Unit.id == Lease.unit_id) + .join(Property, Property.id == Unit.property_id) + .filter(Lease.id == lease_id, Lease.deleted_at.is_(None)) + .one_or_none() + ) + return row[0] if row else None + + +def income_org_id(db: Session, income_id: int) -> int | None: + row = ( + db.query(Property.organization_id) + .select_from(Income) + .join(Lease, Lease.id == Income.lease_id) + .join(Unit, Unit.id == Lease.unit_id) + .join(Property, Property.id == Unit.property_id) + .filter(Income.id == income_id) + .one_or_none() + ) + return row[0] if row else None + + +def expense_org_id(db: Session, expense_id: int) -> int | None: + """Expense ownership chain (Owner FIX1 #4: canonical via unit→property OR + directly through expense.property_id). + + Returns Property.organization_id for whichever chain resolves first. + Both chains MUST ultimately resolve to the SAME organization; if the + unit chain and the direct property_id chain diverge, fail closed (None). + """ + row = ( + db.query(Expense.unit_id, Expense.property_id) + .filter(Expense.id == expense_id) + .one_or_none() + ) + if row is None: + return None + unit_id, property_id = row + oid_from_unit: int | None = None + if unit_id is not None: + oid_from_unit = unit_org_id(db, unit_id) + oid_from_property: int | None = None + if property_id is not None: + oid_from_property = property_org_id(db, property_id) + # If only one chain resolves, use it. If both resolve, they must match. + if oid_from_unit is None: + return oid_from_property + if oid_from_property is None: + return oid_from_unit + if oid_from_unit != oid_from_property: + return None + return oid_from_unit + + +def repair_org_id(db: Session, repair_id: int) -> int | None: + """Resolve RepairOperation owner org through unit_id (preferred) then + property_id (fallback).""" + row = ( + db.query(RepairOperation.unit_id, RepairOperation.property_id) + .filter(RepairOperation.id == repair_id) + .one_or_none() + ) + if row is None: + return None + unit_id, property_id = row + if unit_id is not None: + oid = unit_org_id(db, unit_id) + if oid is not None: + return oid + if property_id is not None: + return property_org_id(db, property_id) + return None + + +def tenant_org_id(db: Session, tenant_id: int) -> int | None: + row = ( + db.query(Tenant.organization_id) + .filter(Tenant.id == tenant_id, Tenant.deleted_at.is_(None)) + .one_or_none() + ) + return row[0] if row else None + + +# --------------------------------------------------------------------------- +# Cross-org membership enforcement: +# - caller not ACTIVE member of object org -> LookupError (404 fail-closed) +# - caller ACTIVE member of object org but role insufficient -> OwnerRequired/ScopeBlocked (403) +# --------------------------------------------------------------------------- + + +def _resolve_scoped_membership( + db: Session, + *, + for_user_id: int, + object_org_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, + object_kind: str = "object", +) -> Membership: + """Resolve membership for a scoped object lookup. + + * Caller outside the object's organization -> :class:`LookupError` + (HTTP 404 fail-closed — no existence leakage). + * Caller inside object's org but no ACTIVE membership -> + :class:`ScopeBlocked` (HTTP 403). + * Caller inside object's org but role insufficient -> + :class:`OwnerRequired` (HTTP 403). + """ + if object_org_id not in list_active_org_ids_for_user(db, for_user_id): + raise LookupError(f"{object_kind} not found") + return resolve_org_membership(db, for_user_id, object_org_id, role=role) + + +# --------------------------------------------------------------------------- +# Cross-org reference guards (create/update payloads) +# --------------------------------------------------------------------------- + + +def assert_co_org( + db: Session, + *, + user_org_id: int, + object_org_id: int | None, + object_kind: str, + object_id: int | None = None, +) -> None: + """Raise CrossOrgReference if object_org_id != user_org_id (or is None).""" + if object_org_id is None or object_org_id != user_org_id: + label = f"{object_kind}" + (f" id={object_id}" if object_id else "") + raise CrossOrgReference( + f"{label} does not belong to the caller's organization" + ) + + +# --------------------------------------------------------------------------- +# scoped_list_* — only rows in orgs where caller has ACTIVE Membership +# --------------------------------------------------------------------------- + + +def scoped_list_properties(db: Session, *, for_user_id: int) -> list[Property]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + return ( + db.query(Property) + .filter( + Property.organization_id.in_(orgs), + Property.deleted_at.is_(None), + ) + .order_by(Property.id) + .all() + ) + + +def scoped_list_units(db: Session, *, for_user_id: int) -> list[Unit]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + return ( + db.query(Unit) + .join(Property, Property.id == Unit.property_id) + .filter( + Property.organization_id.in_(orgs), + Unit.deleted_at.is_(None), + ) + .order_by(Unit.id) + .all() + ) + + +def scoped_list_tenants(db: Session, *, for_user_id: int) -> list[Tenant]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + return ( + db.query(Tenant) + .filter( + Tenant.organization_id.in_(orgs), + Tenant.deleted_at.is_(None), + ) + .order_by(Tenant.id) + .all() + ) + + +def scoped_list_leases(db: Session, *, for_user_id: int) -> list[Lease]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + return ( + db.query(Lease) + .join(Unit, Unit.id == Lease.unit_id) + .join(Property, Property.id == Unit.property_id) + .filter( + Property.organization_id.in_(orgs), + Lease.deleted_at.is_(None), + ) + .order_by(Lease.id) + .all() + ) + + +def scoped_list_incomes(db: Session, *, for_user_id: int) -> list[Income]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + return ( + db.query(Income) + .join(Lease, Lease.id == Income.lease_id) + .join(Unit, Unit.id == Lease.unit_id) + .join(Property, Property.id == Unit.property_id) + .filter(Property.organization_id.in_(orgs)) + .order_by(Income.id) + .all() + ) + + +def scoped_list_expenses(db: Session, *, for_user_id: int) -> list[Expense]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + # Expense canonical ownership (Owner FIX1 #4): + # Expense.property_id -> Property.organization_id (direct building-level path). + # Expense.unit_id remains nullable (unit-level path) automatically resolves + # to the same organization because Expense.property_id via migration m1c backfill; + # hence filtering on Expense.property_id covers both cases. + return ( + db.query(Expense) + .join(Property, Property.id == Expense.property_id) + .filter(Property.organization_id.in_(orgs)) + .order_by(Expense.id) + .all() + ) + + +def scoped_list_repairs(db: Session, *, for_user_id: int) -> list[RepairOperation]: + orgs = list_active_org_ids_for_user(db, for_user_id) + if not orgs: + return [] + org_property_ids = ( + select(Property.id) + .where( + Property.organization_id.in_(orgs), + Property.deleted_at.is_(None), + ) + .scalar_subquery() + ) + org_unit_ids = ( + select(Unit.id) + .where( + Unit.property_id.in_(org_property_ids), + Unit.deleted_at.is_(None), + ) + .scalar_subquery() + ) + return ( + db.query(RepairOperation) + .filter( + (RepairOperation.unit_id.in_(org_unit_ids)) + | (RepairOperation.property_id.in_(org_property_ids)) + ) + .order_by(RepairOperation.id.desc()) + .all() + ) + + +# --------------------------------------------------------------------------- +# scoped_get_* — cross-org lookup -> LookupError (404); role gate if given +# --------------------------------------------------------------------------- + + +def scoped_get_property( + db: Session, + property_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Property, Membership]: + org_id = property_org_id(db, property_id) + if org_id is None: + raise LookupError( + f"property {property_id} not found or has no organization" + ) + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="property", + ) + prop = ( + db.query(Property) + .filter(Property.id == property_id, Property.deleted_at.is_(None)) + .first() + ) + if prop is None: + raise LookupError(f"property {property_id} not found") + return prop, membership + + +def scoped_get_unit( + db: Session, + unit_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Unit, Membership]: + org_id = unit_org_id(db, unit_id) + if org_id is None: + raise LookupError(f"unit {unit_id} not found or property has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="unit", + ) + unit = ( + db.query(Unit) + .filter(Unit.id == unit_id, Unit.deleted_at.is_(None)) + .first() + ) + if unit is None: + raise LookupError(f"unit {unit_id} not found") + return unit, membership + + +def scoped_get_tenant( + db: Session, + tenant_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Tenant, Membership]: + org_id = tenant_org_id(db, tenant_id) + if org_id is None: + raise LookupError(f"tenant {tenant_id} not found or has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="tenant", + ) + tenant = ( + db.query(Tenant) + .filter(Tenant.id == tenant_id, Tenant.deleted_at.is_(None)) + .first() + ) + if tenant is None: + raise LookupError(f"tenant {tenant_id} not found") + return tenant, membership + + +def scoped_get_lease( + db: Session, + lease_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Lease, Membership]: + org_id = lease_org_id(db, lease_id) + if org_id is None: + raise LookupError(f"lease {lease_id} not found or has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="lease", + ) + lease = ( + db.query(Lease) + .filter(Lease.id == lease_id, Lease.deleted_at.is_(None)) + .first() + ) + if lease is None: + raise LookupError(f"lease {lease_id} not found") + return lease, membership + + +def scoped_get_income( + db: Session, + income_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Income, Membership]: + org_id = income_org_id(db, income_id) + if org_id is None: + raise LookupError(f"income {income_id} not found or has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="income", + ) + obj = db.query(Income).filter(Income.id == income_id).first() + if obj is None: + raise LookupError(f"income {income_id} not found") + return obj, membership + + +def scoped_get_expense( + db: Session, + expense_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[Expense, Membership]: + org_id = expense_org_id(db, expense_id) + if org_id is None: + raise LookupError(f"expense {expense_id} not found or has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="expense", + ) + obj = db.query(Expense).filter(Expense.id == expense_id).first() + if obj is None: + raise LookupError(f"expense {expense_id} not found") + return obj, membership + + +def scoped_get_repair( + db: Session, + repair_id: int, + *, + for_user_id: int, + role: OrganizationRole | Iterable[OrganizationRole] | None = None, +) -> tuple[RepairOperation, Membership]: + org_id = repair_org_id(db, repair_id) + if org_id is None: + raise LookupError(f"repair {repair_id} not found or has no organization") + membership = _resolve_scoped_membership( + db, + for_user_id=for_user_id, + object_org_id=org_id, + role=role, + object_kind="repair", + ) + obj = db.query(RepairOperation).filter(RepairOperation.id == repair_id).first() + if obj is None: + raise LookupError(f"repair {repair_id} not found") + return obj, membership diff --git a/app/services/property_channel.py b/app/services/property_channel.py index 3640a80..667e3eb 100644 --- a/app/services/property_channel.py +++ b/app/services/property_channel.py @@ -30,7 +30,6 @@ """ from __future__ import annotations -from collections.abc import Iterable from datetime import datetime, timezone from sqlalchemy import select @@ -38,11 +37,7 @@ from sqlalchemy.orm import Session from app.models.audit_log import AuditAction -from app.models.membership import ( - Membership, - MembershipState, - OrganizationRole, -) +from app.models.membership import Membership, OrganizationRole from app.models.property import Property, Unit from app.models.property_channel import ( BindingStatus, @@ -50,18 +45,37 @@ UnitChannelBinding, ) from app.services.audit import record_audit, serialize_row -from app.services.membership import has_active_membership - -# --------------------------------------------------------------------------- -# Organization-scope resolution helpers -# --------------------------------------------------------------------------- - -class ScopeBlocked(PermissionError): - """Caller has no ACTIVE Membership in the target organization/property.""" - - -class OwnerRequired(PermissionError): - """Caller is an ACTIVE SECRETARY but this action needs ACTIVE OWNER.""" +# TODO(M2): Deprecate this re-export block. Canonical home for all org-scope +# helpers is app/services/organization_scope.py. This block is kept for +# back-compat only; new code MUST import directly from organization_scope. +from app.services.organization_scope import ( # noqa: F401 (re-exported for back-compat) + CrossOrgReference, + OwnerRequired, + ScopeBlocked, + expense_org_id, + income_org_id, + lease_org_id, + list_active_org_ids_for_user, + property_org_id, + repair_org_id, + resolve_org_membership, + scoped_get_expense, + scoped_get_income, + scoped_get_lease, + scoped_get_property, + scoped_get_repair, + scoped_get_tenant, + scoped_get_unit, + scoped_list_expenses, + scoped_list_incomes, + scoped_list_leases, + scoped_list_properties, + scoped_list_repairs, + scoped_list_tenants, + scoped_list_units, + tenant_org_id, + unit_org_id, +) class BindingConflict(Exception): @@ -69,111 +83,10 @@ class BindingConflict(Exception): partial unique index. Mapped to HTTP 409 at the API layer.""" -def resolve_org_membership( - db: Session, user_id: int, organization_id: int, - *, role: OrganizationRole | Iterable[OrganizationRole] | None = None, -) -> Membership: - """Return the ACTIVE Membership for (user_id, organization_id) or raise ScopeBlocked.""" - m = has_active_membership(db, user_id, organization_id, role=role) - if m is None: - if role is None: - hint = "ACTIVE" - elif isinstance(role, OrganizationRole): - hint = f"ACTIVE {role.value}" - else: - hint = "ACTIVE " + "/".join(r.value for r in role) - raise ScopeBlocked( - f"user_id={user_id!r} has no {hint} membership in org={organization_id!r}" - ) - return m - - -def property_org_id(db: Session, property_id: int) -> int | None: - """Return the Property.organization_id, or None. - - Legacy compatibility (Issue #25 "不得粗暴删除旧数据"): - - Pre-migration rows keep ``organization_id = NULL`` and survive the - ALTER ADD COLUMN because the column is nullable. - - A NULL org_id is treated as "unscoped / not yet on-boarded" and the - scoped_* helpers below raise LookupError (→ HTTP 404) instead of - leaking the row to the nearest org. The Property row itself is NEVER - deleted or modified by this service layer. - - New writes via the Property API enforce ``organization_id NOT NULL`` - on the request schema, so the NULL set is monotonically non-growing. - """ - row = db.query(Property.organization_id).filter( - Property.id == property_id, Property.deleted_at.is_(None) - ).one_or_none() - return row[0] if row else None - - -def unit_org_id(db: Session, unit_id: int) -> int | None: - row = db.query(Property.organization_id).select_from(Unit).join( - Property, Property.id == Unit.property_id - ).filter( - Unit.id == unit_id, Unit.deleted_at.is_(None) - ).one_or_none() - return row[0] if row else None - - # --------------------------------------------------------------------------- -# Organization-scoped Property / Unit access (cross-org isolation enforced) +# Organization-scoped stable lookup (Property + Unit Number → Unit) # --------------------------------------------------------------------------- -def scoped_get_property( - db: Session, property_id: int, *, for_user_id: int, -) -> tuple[Property, Membership]: - org_id = property_org_id(db, property_id) - if org_id is None: - # Legacy (organization_id = NULL) rows: fail closed with LookupError - # (→ 404) instead of 500 or accidental cross-org exposure. - raise LookupError( - f"property {property_id} not found or has no organization" - " (legacy pre-migration row, not yet org-scoped)" - ) - membership = resolve_org_membership(db, for_user_id, org_id) - prop = db.query(Property).filter( - Property.id == property_id, Property.deleted_at.is_(None) - ).first() - if prop is None: - raise LookupError(f"property {property_id} not found") - return prop, membership - - -def scoped_list_properties( - db: Session, *, for_user_id: int, -) -> list[Property]: - rows = ( - db.query(Property, Membership) - .join(Membership, Membership.organization_id == Property.organization_id) - .filter( - Membership.user_id == for_user_id, - Membership.state == MembershipState.ACTIVE, - Property.deleted_at.is_(None), - # Legacy rows: organization_id IS NULL would fail the JOIN - # condition above, so they are naturally excluded here without - # any extra filter. No legacy data is deleted. - ) - .order_by(Property.id) - .all() - ) - return [p for p, _m in rows] - - -def scoped_get_unit( - db: Session, unit_id: int, *, for_user_id: int, -) -> tuple[Unit, Membership]: - org_id = unit_org_id(db, unit_id) - if org_id is None: - raise LookupError(f"unit {unit_id} not found or property has no organization") - membership = resolve_org_membership(db, for_user_id, org_id) - unit = db.query(Unit).filter( - Unit.id == unit_id, Unit.deleted_at.is_(None) - ).first() - if unit is None: - raise LookupError(f"unit {unit_id} not found") - return unit, membership - def scoped_lookup_unit( db: Session, @@ -185,10 +98,8 @@ def scoped_lookup_unit( ) -> tuple[Unit, Membership]: """Stable ``organization + property + unit_number → Unit`` lookup. - This is the Issue #25 §3 canonical unit-positioner so the rest of the - system never confuses Org X / Building Bayshore / 1608 with Org Y / - Building Whatever / 1608 — an ACTIVE Membership in organization_id is - mandatory before we even look at the unit row. + Mirrors the canonical unit-positioner: an ACTIVE Membership in + organization_id is mandatory before we even look at the unit row. """ membership = resolve_org_membership(db, for_user_id, organization_id) unit = ( diff --git a/tests/conftest.py b/tests/conftest.py index ee83ae5..ac70603 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,7 @@ from app.main import app from app.models.user import User, UserRole from app.models.identity import ApiCredential, CredentialState, Principal, PrincipalType +from app.models.membership import Organization, Membership, OrganizationRole, MembershipState from app.services.audit import audit_context TEST_DB_NAME = os.getenv("PASAY_TEST_DB_NAME", "pasay_pm_test") @@ -185,21 +186,117 @@ def agent_headers(agent): return _headers(agent[1]) -# --- shared data fixtures (created through the API as admin) --- +# --- organization fixtures --- @pytest.fixture() -def property_id(client, admin_headers): +def org_a(db_session): + org = Organization(name="Org-A", display_name="Pasay Org A") + db_session.add(org) + db_session.flush() + admin_user = db_session.query(User).filter(User.username == "admin").first() + if admin_user is not None: + db_session.add(Membership( + organization_id=org.id, + user_id=admin_user.id, + role=OrganizationRole.OWNER, + state=MembershipState.ACTIVE, + )) + manager_user = db_session.query(User).filter(User.username == "manager").first() + if manager_user is not None: + db_session.add(Membership( + organization_id=org.id, + user_id=manager_user.id, + role=OrganizationRole.SECRETARY, + state=MembershipState.ACTIVE, + )) + agent_user = db_session.query(User).filter(User.username == "agent").first() + if agent_user is not None: + db_session.add(Membership( + organization_id=org.id, + user_id=agent_user.id, + role=OrganizationRole.SECRETARY, + state=MembershipState.ACTIVE, + )) + db_session.commit() + db_session.refresh(org) + return org + + +@pytest.fixture() +def org_b(db_session): + org = Organization(name="Org-B", display_name="Pasay Org B") + db_session.add(org) + db_session.commit() + db_session.refresh(org) + return org + + +@pytest.fixture() +def owner_a(db_session, org_a): + user, api_key = make_user(db_session, "owner_a", UserRole.admin) + membership = Membership( + organization_id=org_a.id, + user_id=user.id, + role=OrganizationRole.OWNER, + state=MembershipState.ACTIVE, + ) + db_session.add(membership) + db_session.commit() + db_session.refresh(membership) + return user, api_key, membership + + +@pytest.fixture() +def secretary_a(db_session, org_a): + user, api_key = make_user(db_session, "secretary_a", UserRole.manager) + membership = Membership( + organization_id=org_a.id, + user_id=user.id, + role=OrganizationRole.SECRETARY, + state=MembershipState.ACTIVE, + ) + db_session.add(membership) + db_session.commit() + db_session.refresh(membership) + return user, api_key, membership + + +@pytest.fixture() +def owner_b(db_session, org_b): + user, api_key = make_user(db_session, "owner_b", UserRole.admin) + membership = Membership( + organization_id=org_b.id, + user_id=user.id, + role=OrganizationRole.OWNER, + state=MembershipState.ACTIVE, + ) + db_session.add(membership) + db_session.commit() + db_session.refresh(membership) + return user, api_key, membership + + +# --- shared data fixtures (created through the API as Org Owner) --- + +@pytest.fixture() +def property_id(client, owner_a, org_a): resp = client.post( "/api/v1/properties", - json={"name": "Sunset Tower", "address": "1 Roxas Blvd", "city": "Pasay", "total_units": 4}, - headers=admin_headers, + json={ + "name": "Sunset Tower", + "address": "1 Roxas Blvd", + "city": "Pasay", + "total_units": 4, + "organization_id": org_a.id, + }, + headers=_headers(owner_a[1]), ) assert resp.status_code == 201, resp.text return resp.json()["id"] @pytest.fixture() -def unit_id(client, admin_headers, property_id): +def unit_id(client, owner_a, property_id): resp = client.post( "/api/v1/units", json={ @@ -210,25 +307,30 @@ def unit_id(client, admin_headers, property_id): "monthly_rent": "12000.00", "status": "vacant", }, - headers=admin_headers, + headers=_headers(owner_a[1]), ) assert resp.status_code == 201, resp.text return resp.json()["id"] @pytest.fixture() -def tenant_id(client, admin_headers): +def tenant_id(client, owner_a, org_a): resp = client.post( "/api/v1/tenants", - json={"full_name": "Juan Dela Cruz", "phone": "+639170000000", "email": "juan@example.com"}, - headers=admin_headers, + json={ + "full_name": "Juan Dela Cruz", + "phone": "+639170000000", + "email": "juan@example.com", + "organization_id": org_a.id, + }, + headers=_headers(owner_a[1]), ) assert resp.status_code == 201, resp.text return resp.json()["id"] @pytest.fixture() -def lease_id(client, admin_headers, unit_id, tenant_id): +def lease_id(client, owner_a, unit_id, tenant_id): resp = client.post( "/api/v1/leases", json={ @@ -240,7 +342,7 @@ def lease_id(client, admin_headers, unit_id, tenant_id): "deposit": "24000.00", "status": "active", }, - headers=admin_headers, + headers=_headers(owner_a[1]), ) assert resp.status_code == 201, resp.text return resp.json()["id"] diff --git a/tests/test_financial.py b/tests/test_financial.py index 9f49962..fb7280c 100644 --- a/tests/test_financial.py +++ b/tests/test_financial.py @@ -1,237 +1,243 @@ -from decimal import Decimal - -API = "/api/v1" - - -def _income(lease_id, amount="12000.00", status="pending"): - return { - "lease_id": lease_id, - "amount": amount, - "received_date": "2026-02-01", - "payment_method": "cash", - "status": status, - "description": "rent Feb", - } - - -def _expense(status="pending"): - return { - "expense_date": "2026-02-05", - "category": "repair", - "amount": "5000.00", - "payee": "Fix-It Co", - "description": "AC repair", - "status": status, - } - - -def test_income_requires_status(client, admin_headers, lease_id): - payload = _income(lease_id) - payload.pop("status") - resp = client.post(f"{API}/incomes", json=payload, headers=admin_headers) - assert resp.status_code == 422 - - -def test_income_confirm_and_double_confirm_idempotent(client, admin_headers, lease_id): - resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) - income_id = resp.json()["id"] - assert resp.json()["status"] == "pending" - - resp = client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "confirmed" - assert resp.json()["confirmed_by"] is not None - - resp = client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) - # Financial-safety V1.1: an idempotent replay of confirm returns the - # current confirmed state instead of a conflict. - assert resp.status_code == 200 - assert resp.json()["status"] == "confirmed" - - -def test_income_reverse_flow(client, admin_headers, lease_id): - resp = client.post(f"{API}/incomes", json=_income(lease_id, status="confirmed"), headers=admin_headers) - income_id = resp.json()["id"] - assert resp.json()["status"] == "confirmed" - - resp = client.post(f"{API}/incomes/{income_id}/reverse", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "reversed" - - resp = client.post(f"{API}/incomes/{income_id}/reverse", headers=admin_headers) - # Financial-safety V1.1: replay of reverse returns the current reversed - # state instead of a conflict. - assert resp.status_code == 200 - assert resp.json()["status"] == "reversed" - - -def test_income_no_delete_endpoint(client, admin_headers, lease_id): - resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) - income_id = resp.json()["id"] - resp = client.delete(f"{API}/incomes/{income_id}", headers=admin_headers) - assert resp.status_code == 405 - - -def test_income_amount_locked_after_confirm(client, admin_headers, lease_id): - resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) - income_id = resp.json()["id"] - client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) - - resp = client.patch( - f"{API}/incomes/{income_id}", json={"amount": "9999.00"}, headers=admin_headers - ) - assert resp.status_code == 409 - - -def test_expense_requires_status(client, admin_headers): - payload = _expense() - payload.pop("status") - resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) - assert resp.status_code == 422 - - -def test_expense_approve_pay_reverse_flow(client, admin_headers, manager_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - assert resp.status_code == 201 - expense_id = resp.json()["id"] - - resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "approved" - assert resp.json()["approved_by"] is not None - - resp = client.post(f"{API}/expenses/{expense_id}/pay", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "paid" - - resp = client.post(f"{API}/expenses/{expense_id}/reverse", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "reversed" - - -def test_expense_admin_can_approve_own_created(client, admin_headers, manager_headers): - # admin creates an expense, then approves their own -> allowed - resp = client.post(f"{API}/expenses", json=_expense(), headers=admin_headers) - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "approved" - assert resp.json()["approved_by"] is not None - - -def test_expense_manager_cannot_approve_own_created(client, admin_headers, manager_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=manager_headers) - assert resp.status_code == 403 - - -def test_expense_admin_can_reject_own_created(client, admin_headers): - # Owner records an expense themselves, then rejects it -> allowed (the - # Owner is the final authority; this is the V1 Owner-records flow). - resp = client.post(f"{API}/expenses", json=_expense(), headers=admin_headers) - assert resp.status_code == 201 - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "rejected" - - -def test_expense_manager_cannot_reject_own_created(client, admin_headers, manager_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=manager_headers) - assert resp.status_code == 403 - - -def test_expense_reject_flow(client, admin_headers, manager_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["status"] == "rejected" - - -def test_expense_amount_locked_after_approve(client, admin_headers, manager_headers): - """003B §9: a critical financial field (amount) changed after approval must - INVALIDATE the old approval and return the expense to PENDING for Owner - re-review — it must NOT keep showing a stale APPROVED with the new amount.""" - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - expense_id = resp.json()["id"] - client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) - - resp = client.patch( - f"{API}/expenses/{expense_id}", json={"amount": "35000.00"}, headers=admin_headers - ) - assert resp.status_code == 200 - body = resp.json() - assert body["status"] == "pending" # re-approval required, never stale APPROVED - assert body["amount"] == "35000.00" - assert body["approved_by"] is None - assert body["reapproval_reason"] is not None - - -def test_expense_pay_requires_approved(client, admin_headers, manager_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=manager_headers) - expense_id = resp.json()["id"] - resp = client.post(f"{API}/expenses/{expense_id}/pay", headers=admin_headers) - assert resp.status_code == 409 - - -def test_expense_no_delete_endpoint(client, admin_headers): - resp = client.post(f"{API}/expenses", json=_expense(), headers=admin_headers) - expense_id = resp.json()["id"] - resp = client.delete(f"{API}/expenses/{expense_id}", headers=admin_headers) - assert resp.status_code == 405 - - -def test_attachment_upload_download(client, admin_headers): - files = {"file": ("receipt.pdf", b"%PDF-1.4 fake", "application/pdf")} - resp = client.post(f"{API}/attachments", files=files, headers=admin_headers) - assert resp.status_code == 201 - attachment_id = resp.json()["id"] - assert resp.json()["original_filename"] == "receipt.pdf" - - resp = client.get(f"{API}/attachments/{attachment_id}/download", headers=admin_headers) - assert resp.status_code == 200 - assert resp.content.startswith(b"%PDF-1.4") - - -def test_money_precision_never_float(client, admin_headers, lease_id): - # Decimal serialized as string, never as a float - resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) - assert resp.status_code == 201 - assert resp.json()["amount"] == "12000.00" - - # amounts are limited to Numeric(14,2): too many digits -> 422 - resp = client.post( - f"{API}/incomes", - json=_income(lease_id, amount="99999999999999.99"), - headers=admin_headers, - ) - assert resp.status_code == 422 - - -def test_money_rejects_float_input(client, admin_headers, lease_id): - resp = client.post( - f"{API}/incomes", - json=_income(lease_id, amount=12000.556), # float with >2 decimals - headers=admin_headers, - ) - assert resp.status_code == 422 - - -def test_expense_due_date_and_unit(client, admin_headers, unit_id): - payload = _expense() - payload.update({"due_date": "2026-08-18", "unit_id": unit_id}) - resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) - assert resp.status_code == 201 - assert resp.json()["due_date"] == "2026-08-18" - assert resp.json()["unit_id"] == unit_id - - -def test_expense_unknown_unit_404(client, admin_headers): - payload = _expense() - payload["unit_id"] = 999999 - resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) - assert resp.status_code == 404 +from decimal import Decimal + +API = "/api/v1" + + +def _income(lease_id, amount="12000.00", status="pending"): + return { + "lease_id": lease_id, + "amount": amount, + "received_date": "2026-02-01", + "payment_method": "cash", + "status": status, + "description": "rent Feb", + } + + +def _expense(*, status="pending", unit_id=None, property_id=None): + """FIX1: Expense requires canonical ownership anchor (unit_id or property_id).""" + d = { + "expense_date": "2026-02-05", + "category": "repair", + "amount": "5000.00", + "payee": "Fix-It Co", + "description": "AC repair", + "status": status, + } + if unit_id is not None: + d["unit_id"] = unit_id + if property_id is not None: + d["property_id"] = property_id + return d + + +def test_income_requires_status(client, admin_headers, lease_id): + payload = _income(lease_id) + payload.pop("status") + resp = client.post(f"{API}/incomes", json=payload, headers=admin_headers) + assert resp.status_code == 422 + + +def test_income_confirm_and_double_confirm_idempotent(client, admin_headers, lease_id): + resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) + income_id = resp.json()["id"] + assert resp.json()["status"] == "pending" + + resp = client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "confirmed" + assert resp.json()["confirmed_by"] is not None + + resp = client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) + # Financial-safety V1.1: an idempotent replay of confirm returns the + # current confirmed state instead of a conflict. + assert resp.status_code == 200 + assert resp.json()["status"] == "confirmed" + + +def test_income_reverse_flow(client, admin_headers, lease_id): + resp = client.post(f"{API}/incomes", json=_income(lease_id, status="confirmed"), headers=admin_headers) + income_id = resp.json()["id"] + assert resp.json()["status"] == "confirmed" + + resp = client.post(f"{API}/incomes/{income_id}/reverse", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "reversed" + + resp = client.post(f"{API}/incomes/{income_id}/reverse", headers=admin_headers) + # Financial-safety V1.1: replay of reverse returns the current reversed + # state instead of a conflict. + assert resp.status_code == 200 + assert resp.json()["status"] == "reversed" + + +def test_income_no_delete_endpoint(client, admin_headers, lease_id): + resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) + income_id = resp.json()["id"] + resp = client.delete(f"{API}/incomes/{income_id}", headers=admin_headers) + assert resp.status_code == 405 + + +def test_income_amount_locked_after_confirm(client, admin_headers, lease_id): + resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) + income_id = resp.json()["id"] + client.post(f"{API}/incomes/{income_id}/confirm", headers=admin_headers) + + resp = client.patch( + f"{API}/incomes/{income_id}", json={"amount": "9999.00"}, headers=admin_headers + ) + assert resp.status_code == 409 + + +def test_expense_requires_status(client, admin_headers): + payload = _expense() + payload.pop("status") + resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) + assert resp.status_code == 422 + + +def test_expense_approve_pay_reverse_flow(client, admin_headers, manager_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + assert resp.status_code == 201 + expense_id = resp.json()["id"] + + resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "approved" + assert resp.json()["approved_by"] is not None + + resp = client.post(f"{API}/expenses/{expense_id}/pay", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "paid" + + resp = client.post(f"{API}/expenses/{expense_id}/reverse", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "reversed" + + +def test_expense_admin_can_approve_own_created(client, admin_headers, manager_headers, unit_id): + # admin creates an expense, then approves their own -> allowed + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=admin_headers) + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "approved" + assert resp.json()["approved_by"] is not None + + +def test_expense_manager_cannot_approve_own_created(client, admin_headers, manager_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/approve", headers=manager_headers) + assert resp.status_code == 403 + + +def test_expense_admin_can_reject_own_created(client, admin_headers, unit_id): + # Owner records an expense themselves, then rejects it -> allowed (the + # Owner is the final authority; this is the V1 Owner-records flow). + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=admin_headers) + assert resp.status_code == 201 + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "rejected" + + +def test_expense_manager_cannot_reject_own_created(client, admin_headers, manager_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=manager_headers) + assert resp.status_code == 403 + + +def test_expense_reject_flow(client, admin_headers, manager_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/reject", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["status"] == "rejected" + + +def test_expense_amount_locked_after_approve(client, admin_headers, manager_headers, unit_id): + """003B §9: a critical financial field (amount) changed after approval must + INVALIDATE the old approval and return the expense to PENDING for Owner + re-review — it must NOT keep showing a stale APPROVED with the new amount.""" + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + expense_id = resp.json()["id"] + client.post(f"{API}/expenses/{expense_id}/approve", headers=admin_headers) + + resp = client.patch( + f"{API}/expenses/{expense_id}", json={"amount": "35000.00"}, headers=admin_headers + ) + assert resp.status_code == 200 + body = resp.json() + assert body["status"] == "pending" # re-approval required, never stale APPROVED + assert body["amount"] == "35000.00" + assert body["approved_by"] is None + assert body["reapproval_reason"] is not None + + +def test_expense_pay_requires_approved(client, admin_headers, manager_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=manager_headers) + expense_id = resp.json()["id"] + resp = client.post(f"{API}/expenses/{expense_id}/pay", headers=admin_headers) + assert resp.status_code == 409 + + +def test_expense_no_delete_endpoint(client, admin_headers, unit_id): + resp = client.post(f"{API}/expenses", json=_expense(unit_id=unit_id), headers=admin_headers) + expense_id = resp.json()["id"] + resp = client.delete(f"{API}/expenses/{expense_id}", headers=admin_headers) + assert resp.status_code == 405 + + +def test_attachment_upload_download(client, admin_headers): + files = {"file": ("receipt.pdf", b"%PDF-1.4 fake", "application/pdf")} + resp = client.post(f"{API}/attachments", files=files, headers=admin_headers) + assert resp.status_code == 201 + attachment_id = resp.json()["id"] + assert resp.json()["original_filename"] == "receipt.pdf" + + resp = client.get(f"{API}/attachments/{attachment_id}/download", headers=admin_headers) + assert resp.status_code == 200 + assert resp.content.startswith(b"%PDF-1.4") + + +def test_money_precision_never_float(client, admin_headers, lease_id): + # Decimal serialized as string, never as a float + resp = client.post(f"{API}/incomes", json=_income(lease_id), headers=admin_headers) + assert resp.status_code == 201 + assert resp.json()["amount"] == "12000.00" + + # amounts are limited to Numeric(14,2): too many digits -> 422 + resp = client.post( + f"{API}/incomes", + json=_income(lease_id, amount="99999999999999.99"), + headers=admin_headers, + ) + assert resp.status_code == 422 + + +def test_money_rejects_float_input(client, admin_headers, lease_id): + resp = client.post( + f"{API}/incomes", + json=_income(lease_id, amount=12000.556), # float with >2 decimals + headers=admin_headers, + ) + assert resp.status_code == 422 + + +def test_expense_due_date_and_unit(client, admin_headers, unit_id): + payload = _expense() + payload.update({"due_date": "2026-08-18", "unit_id": unit_id}) + resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) + assert resp.status_code == 201 + assert resp.json()["due_date"] == "2026-08-18" + assert resp.json()["unit_id"] == unit_id + + +def test_expense_unknown_unit_404(client, admin_headers): + payload = _expense() + payload["unit_id"] = 999999 + resp = client.post(f"{API}/expenses", json=payload, headers=admin_headers) + assert resp.status_code == 404 diff --git a/tests/test_financial_idempotency.py b/tests/test_financial_idempotency.py index c2a9e0a..5751740 100644 --- a/tests/test_financial_idempotency.py +++ b/tests/test_financial_idempotency.py @@ -1,550 +1,584 @@ -"""PASay V1.1 financial-safety idempotency tests. - -Runs against the REAL PostgreSQL test database (``pasay_pm_test``), no mocks: -- sequential x10 and concurrent x10 replays of every financial command -- timeout-after-commit replay (DB committed, HTTP response lost) -- stale-callback replay -- the system invariant: N identical commands (sequential or concurrent) - produce the same final DB state as executing the command once. - -Concurrency is executed through a real uvicorn ASGI server bound to the test -DB with per-request sessions, driven by a ThreadPoolExecutor. -""" -from __future__ import annotations - -import concurrent.futures -import threading -import time - -import httpx -import pytest -import uvicorn -from sqlalchemy import create_engine, text -from sqlalchemy.orm import sessionmaker - -from app.database import get_db -from app.main import app -from app.models.lease import Lease, LeaseStatus -from app.models.property import Property, Unit, UnitStatus -from app.models.tenant import Tenant - -API = "/api/v1" - -# -------------------------------------------------------------------------- -# helpers -# -------------------------------------------------------------------------- - - -def _income_payload(lease_id, key, amount="12000.00", status="pending"): - return { - "lease_id": lease_id, - "amount": amount, - "received_date": "2026-02-01", - "payment_method": "cash", - "status": status, - "description": "rent Feb", - "idempotency_key": key, - } - - -def _expense_payload(status="pending"): - return { - "expense_date": "2026-02-05", - "category": "repair", - "amount": "5000.00", - "payee": "Fix-It Co", - "description": "AC repair", - "status": status, - } - - -def _audit_count(db, table_name, action, record_id=None): - sql = ( - "SELECT count(*) FROM audit_logs WHERE table_name=:t AND action=:a" - + (" AND record_id=:rid" if record_id is not None else "") - ) - params = {"t": table_name, "a": action} - if record_id is not None: - params["rid"] = record_id - return db.execute(text(sql), params).scalar() - - -@pytest.fixture() -def http_server(db_session, test_engine): - """Start a real uvicorn server on the test DB with per-request sessions.""" - Session = sessionmaker(bind=test_engine, autoflush=False, expire_on_commit=False) - - def override_get_db(): - db = Session() - try: - yield db - finally: - db.close() - - app.dependency_overrides[get_db] = override_get_db - config = uvicorn.Config(app, host="127.0.0.1", port=0, log_level="warning", lifespan="off") - server = uvicorn.Server(config) - thread = threading.Thread(target=server.run, daemon=True) - thread.start() - try: - deadline = time.time() + 20 - while not server.started: - if time.time() > deadline: - raise RuntimeError("uvicorn did not start") - time.sleep(0.02) - port = server.servers[0].sockets[0].getsockname()[1] - yield f"http://127.0.0.1:{port}" - finally: - server.should_exit = True - thread.join(timeout=10) - app.dependency_overrides.pop(get_db, None) - - -@pytest.fixture() -def lease_id(db_session): - """Lease data created directly in the test DB (never through the API), so - the conftest ``client`` fixture never clobbers the per-request dependency - override that ``http_server`` installs.""" - prop = Property(name="Sunset Tower", address="1 Roxas Blvd", city="Pasay", total_units=4) - db_session.add(prop) - db_session.flush() - unit = Unit( - property_id=prop.id, - unit_number="101", - floor="1", - size_sqm="32.50", - monthly_rent="12000.00", - status=UnitStatus.vacant, - ) - tenant = Tenant(full_name="Juan Dela Cruz", phone="+639170000000") - db_session.add_all([unit, tenant]) - db_session.flush() - lease = Lease( - unit_id=unit.id, - tenant_id=tenant.id, - start_date="2026-01-01", - end_date="2026-12-31", - monthly_rent="12000.00", - deposit="24000.00", - status=LeaseStatus.active, - ) - db_session.add(lease) - db_session.commit() - db_session.refresh(lease) - return lease.id - - -def _concurrent(worker, n=10): - """Run ``worker(i)`` on n threads and return results in order.""" - with concurrent.futures.ThreadPoolExecutor(max_workers=n) as ex: - return [f.result() for f in [ex.submit(worker, i) for i in range(n)]] - - -def _new_client(base_url, api_key): - return httpx.Client(base_url=base_url, headers={"Authorization": f"Bearer {api_key}"}, timeout=30.0) - - -def _final_state(test_engine, key, income_id): - """Business-state fingerprint for one idempotent create+confirm event.""" - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - rows = db.execute( - text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() - status = db.execute( - text("SELECT status FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() - create_audits = _audit_count(db, "incomes", "create", income_id) - confirm_audits = _audit_count(db, "incomes", "confirm", income_id) - return (rows, status, create_audits, confirm_audits) - finally: - db.close() - - -# -------------------------------------------------------------------------- -# income create idempotency -# -------------------------------------------------------------------------- - - -def test_create_income_same_key_sequential_x10( - http_server, admin_headers, lease_id, test_engine -): - base = http_server - key = f"seq-{time.time_ns()}" - statuses, ids = [], [] - with _new_client(base, admin_headers["Authorization"].split()[-1]) as c: - for _ in range(10): - resp = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - statuses.append(resp.status_code) - assert resp.status_code in (200, 201), resp.text - ids.append(resp.json()["id"]) - assert statuses[0] == 201 - assert all(s == 200 for s in statuses[1:]) - assert len(set(ids)) == 1 - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - rows = db.execute( - text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() - create_audits = _audit_count(db, "incomes", "create", ids[0]) - finally: - db.close() - assert rows == 1 - assert create_audits == 1 - - -def test_create_income_same_key_concurrent_x10( - http_server, admin_headers, lease_id, test_engine -): - key = f"con-{time.time_ns()}" - statuses, ids = [], [] - - def worker(i): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - resp = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - return resp.status_code, resp.json().get("id") - - results = _concurrent(worker) - for code, _id in results: - assert code in (200, 201) - ids.append(_id) - assert len(set(ids)) == 1 - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - rows = db.execute( - text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() - create_audits = _audit_count(db, "incomes", "create", ids[0]) - finally: - db.close() - assert rows == 1 # UNIQUE backstop: exactly one row landed - assert create_audits == 1 - - -# -------------------------------------------------------------------------- -# income confirm / reverse -# -------------------------------------------------------------------------- - - -def test_income_confirm_sequential_and_concurrent_x10( - http_server, admin_headers, lease_id, test_engine -): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - # sequential x10 on one income - inc1 = c.post(f"{API}/incomes", json=_income_payload(lease_id, f"cf-s-{time.time_ns()}")).json() - for _ in range(10): - resp = c.post(f"{API}/incomes/{inc1['id']}/confirm") - assert resp.status_code == 200, resp.text - assert resp.json()["status"] == "confirmed" - # concurrent x10 on a second income - inc2 = c.post(f"{API}/incomes", json=_income_payload(lease_id, f"cf-c-{time.time_ns()}")).json() - - def worker(i): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: - return cc.post(f"{API}/incomes/{inc2['id']}/confirm") - - resps = _concurrent(worker) - for r in resps: - assert r.status_code == 200, r.text - assert r.json()["status"] == "confirmed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - for inc_id in (inc1["id"], inc2["id"]): - status = db.execute( - text("SELECT status FROM incomes WHERE id=:i"), {"i": inc_id} - ).scalar() - assert status == "confirmed" - assert _audit_count(db, "incomes", "confirm", inc_id) == 1 - finally: - db.close() - - -def test_income_reverse_sequential_and_concurrent_x10( - http_server, admin_headers, lease_id, test_engine -): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - # create as confirmed (admin), then reverse sequential x10 - inc1 = c.post( - f"{API}/incomes", json=_income_payload(lease_id, f"rv-s-{time.time_ns()}", status="confirmed") - ).json() - for _ in range(10): - resp = c.post(f"{API}/incomes/{inc1['id']}/reverse") - assert resp.status_code == 200, resp.text - assert resp.json()["status"] == "reversed" - # concurrent x10 on a second income - inc2 = c.post( - f"{API}/incomes", json=_income_payload(lease_id, f"rv-c-{time.time_ns()}", status="confirmed") - ).json() - - def worker(i): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: - return cc.post(f"{API}/incomes/{inc2['id']}/reverse") - - resps = _concurrent(worker) - for r in resps: - assert r.status_code == 200, r.text - assert r.json()["status"] == "reversed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - for inc_id in (inc1["id"], inc2["id"]): - status = db.execute( - text("SELECT status FROM incomes WHERE id=:i"), {"i": inc_id} - ).scalar() - assert status == "reversed" - assert _audit_count(db, "incomes", "reverse", inc_id) == 1 - finally: - db.close() - - -# -------------------------------------------------------------------------- -# expense approve / reject / pay / reverse -# -------------------------------------------------------------------------- - - -def test_expense_approve_reject_pay_reverse_sequential_and_concurrent_x10( - http_server, admin_headers, manager_headers, test_engine -): - admin_key = admin_headers["Authorization"].split()[-1] - with _new_client(http_server, admin_key) as c: - def _mk_expense(): - resp = c.post(f"{API}/expenses", json=_expense_payload(), headers=manager_headers) - assert resp.status_code == 201, resp.text - return resp.json()["id"] - - # --- approve: sequential x10 then concurrent x10 --- - e1 = _mk_expense() - for _ in range(10): - r = c.post(f"{API}/expenses/{e1}/approve") - assert r.status_code == 200 and r.json()["status"] == "approved" - e2 = _mk_expense() - - def approve(i): - with _new_client(http_server, admin_key) as cc: - return cc.post(f"{API}/expenses/{e2}/approve") - - for r in _concurrent(approve): - assert r.status_code == 200 and r.json()["status"] == "approved" - - # --- reject: concurrent x10 --- - e3 = _mk_expense() - - def reject(i): - with _new_client(http_server, admin_key) as cc: - return cc.post(f"{API}/expenses/{e3}/reject") - - for r in _concurrent(reject): - assert r.status_code == 200 and r.json()["status"] == "rejected" - - # --- pay: concurrent x10 (approve first) --- - e4 = _mk_expense() - assert c.post(f"{API}/expenses/{e4}/approve").json()["status"] == "approved" - - def pay(i): - with _new_client(http_server, admin_key) as cc: - return cc.post(f"{API}/expenses/{e4}/pay") - - for r in _concurrent(pay): - assert r.status_code == 200 and r.json()["status"] == "paid" - - # --- reverse: concurrent x10 (approve+pay first) --- - e5 = _mk_expense() - c.post(f"{API}/expenses/{e5}/approve") - c.post(f"{API}/expenses/{e5}/pay") - - def reverse_exp(i): - with _new_client(http_server, admin_key) as cc: - return cc.post(f"{API}/expenses/{e5}/reverse") - - for r in _concurrent(reverse_exp): - assert r.status_code == 200 and r.json()["status"] == "reversed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e1}).scalar() == "approved" - assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e2}).scalar() == "approved" - assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e3}).scalar() == "rejected" - assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e4}).scalar() == "paid" - assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e5}).scalar() == "reversed" - # each transition audited exactly once per expense - assert _audit_count(db, "expenses", "approve", e1) == 1 - assert _audit_count(db, "expenses", "approve", e2) == 1 - assert _audit_count(db, "expenses", "reject", e3) == 1 - assert _audit_count(db, "expenses", "pay", e4) == 1 - assert _audit_count(db, "expenses", "reverse", e5) == 1 - finally: - db.close() - - -# -------------------------------------------------------------------------- -# commission settlement confirm -# -------------------------------------------------------------------------- - - -def _rule(client, headers): - resp = client.post( - f"{API}/commission/rules", - json={"name": "Rent commission", "rule_type": "percentage", "value": "10", "agent_role": "出租"}, - headers=headers, - ) - assert resp.status_code == 201, resp.text - return resp.json()["id"] - - -def test_settlement_confirm_sequential_and_concurrent_x10( - http_server, admin_headers, agent, lease_id, test_engine -): - admin_key = admin_headers["Authorization"].split()[-1] - with _new_client(http_server, admin_key) as c: - rule_id = _rule(c, admin_headers) - s1 = c.post( - f"{API}/commission/settlements", - json={"agent_id": agent[0].id, "lease_id": lease_id, "rule_id": rule_id}, - headers=admin_headers, - ).json()["id"] - for _ in range(10): - r = c.post(f"{API}/commission/settlements/{s1}/confirm") - assert r.status_code == 200 and r.json()["status"] == "confirmed" - s2 = c.post( - f"{API}/commission/settlements", - json={"agent_id": agent[0].id, "lease_id": lease_id, "rule_id": rule_id}, - headers=admin_headers, - ).json()["id"] - - def confirm(i): - with _new_client(http_server, admin_key) as cc: - return cc.post(f"{API}/commission/settlements/{s2}/confirm") - - for r in _concurrent(confirm): - assert r.status_code == 200 and r.json()["status"] == "confirmed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - for sid in (s1, s2): - status = db.execute( - text("SELECT status FROM commission_settlements WHERE id=:i"), {"i": sid} - ).scalar() - assert status == "confirmed" - assert _audit_count(db, "commission_settlements", "confirm", sid) == 1 - finally: - db.close() - - -# -------------------------------------------------------------------------- -# timeout-after-commit + stale callback replay -# -------------------------------------------------------------------------- - - -def test_timeout_after_commit_replay(http_server, admin_headers, lease_id, test_engine): - """DB committed but HTTP response lost: retry with the same key must - return the existing row, never a second one.""" - key = f"timeout-{time.time_ns()}" - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - first = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - assert first.status_code == 201 - income_id = first.json()["id"] - # simulate the client never receiving the response: retry same key - retry = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - assert retry.status_code == 200 - assert retry.json()["id"] == income_id - - # timeout-after-commit on confirm as well - conf1 = c.post(f"{API}/incomes/{income_id}/confirm") - assert conf1.status_code == 200 - conf2 = c.post(f"{API}/incomes/{income_id}/confirm") - assert conf2.status_code == 200 and conf2.json()["status"] == "confirmed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - assert db.execute( - text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() == 1 - assert _audit_count(db, "incomes", "create", income_id) == 1 - assert _audit_count(db, "incomes", "confirm", income_id) == 1 - finally: - db.close() - - -def test_stale_callback_replay(http_server, admin_headers, lease_id, test_engine): - """A stale card replayed after the event fully landed must not re-create - anything and must return the current state.""" - key = f"stale-{time.time_ns()}" - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - created = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - assert created.status_code == 201 - income_id = created.json()["id"] - assert c.post(f"{API}/incomes/{income_id}/confirm").json()["status"] == "confirmed" - - # stale replay of the whole flow: create (same key) + confirm - replay = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) - assert replay.status_code == 200 - assert replay.json()["id"] == income_id - assert replay.json()["status"] == "confirmed" - replay_confirm = c.post(f"{API}/incomes/{income_id}/confirm") - assert replay_confirm.status_code == 200 - assert replay_confirm.json()["status"] == "confirmed" - - Session = sessionmaker(bind=test_engine, expire_on_commit=False) - db = Session() - try: - assert db.execute( - text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} - ).scalar() == 1 - assert _audit_count(db, "incomes", "create", income_id) == 1 - assert _audit_count(db, "incomes", "confirm", income_id) == 1 - finally: - db.close() - - -# -------------------------------------------------------------------------- -# system invariant: N identical commands == executing once -# -------------------------------------------------------------------------- - - -def test_invariant_n_identical_commands_equal_one( - http_server, admin_headers, lease_id, test_engine -): - """N identical commands (sequential or concurrent) must leave the same - final business state as executing the command exactly once.""" - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: - # once - key1 = f"once-{time.time_ns()}" - inc_once = c.post(f"{API}/incomes", json=_income_payload(lease_id, key1)).json() - c.post(f"{API}/incomes/{inc_once['id']}/confirm") - state_once = _final_state(test_engine, key1, inc_once["id"]) - - # sequential x10 - key2 = f"seq-{time.time_ns()}" - inc_seq = c.post(f"{API}/incomes", json=_income_payload(lease_id, key2)).json() - for _ in range(10): - c.post(f"{API}/incomes", json=_income_payload(lease_id, key2)) - c.post(f"{API}/incomes/{inc_seq['id']}/confirm") - state_seq = _final_state(test_engine, key2, inc_seq["id"]) - - # concurrent x10 - key3 = f"conc-{time.time_ns()}" - - def full_flow(i): - with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: - created = cc.post(f"{API}/incomes", json=_income_payload(lease_id, key3)).json() - cc.post(f"{API}/incomes/{created['id']}/confirm") - return created["id"] - - ids = _concurrent(full_flow) - state_concurrent = _final_state(test_engine, key3, ids[0]) - - assert len(set(ids)) == 1 - assert state_once == (1, "confirmed", 1, 1) - assert state_seq == state_once - assert state_concurrent == state_once +"""PASay V1.1 financial-safety idempotency tests. + +Runs against the REAL PostgreSQL test database (``pasay_pm_test``), no mocks: +- sequential x10 and concurrent x10 replays of every financial command +- timeout-after-commit replay (DB committed, HTTP response lost) +- stale-callback replay +- the system invariant: N identical commands (sequential or concurrent) + produce the same final DB state as executing the command once. + +Concurrency is executed through a real uvicorn ASGI server bound to the test +DB with per-request sessions, driven by a ThreadPoolExecutor. +""" +from __future__ import annotations + +import concurrent.futures +import threading +import time + +import httpx +import pytest +import uvicorn +from sqlalchemy import create_engine, text +from sqlalchemy.orm import sessionmaker + +from app.database import get_db +from app.main import app +from app.models.lease import Lease, LeaseStatus +from app.models.property import Property, Unit, UnitStatus +from app.models.tenant import Tenant + +API = "/api/v1" + +# -------------------------------------------------------------------------- +# helpers +# -------------------------------------------------------------------------- + + +def _income_payload(lease_id, key, amount="12000.00", status="pending"): + return { + "lease_id": lease_id, + "amount": amount, + "received_date": "2026-02-01", + "payment_method": "cash", + "status": status, + "description": "rent Feb", + "idempotency_key": key, + } + + +def _expense_payload(*, status="pending", unit_id=None, property_id=None): + """FIX1: Expense requires canonical ownership anchor (unit_id or property_id).""" + d = { + "expense_date": "2026-02-05", + "category": "repair", + "amount": "5000.00", + "payee": "Fix-It Co", + "description": "AC repair", + "status": status, + } + if unit_id is not None: + d["unit_id"] = unit_id + if property_id is not None: + d["property_id"] = property_id + return d + + +def _audit_count(db, table_name, action, record_id=None): + sql = ( + "SELECT count(*) FROM audit_logs WHERE table_name=:t AND action=:a" + + (" AND record_id=:rid" if record_id is not None else "") + ) + params = {"t": table_name, "a": action} + if record_id is not None: + params["rid"] = record_id + return db.execute(text(sql), params).scalar() + + +@pytest.fixture() +def http_server(db_session, test_engine): + """Start a real uvicorn server on the test DB with per-request sessions.""" + Session = sessionmaker(bind=test_engine, autoflush=False, expire_on_commit=False) + + def override_get_db(): + db = Session() + try: + yield db + finally: + db.close() + + app.dependency_overrides[get_db] = override_get_db + config = uvicorn.Config(app, host="127.0.0.1", port=0, log_level="warning", lifespan="off") + server = uvicorn.Server(config) + thread = threading.Thread(target=server.run, daemon=True) + thread.start() + try: + deadline = time.time() + 20 + while not server.started: + if time.time() > deadline: + raise RuntimeError("uvicorn did not start") + time.sleep(0.02) + port = server.servers[0].sockets[0].getsockname()[1] + yield f"http://127.0.0.1:{port}" + finally: + server.should_exit = True + thread.join(timeout=10) + app.dependency_overrides.pop(get_db, None) + + +@pytest.fixture() +def lease_id(db_session, org_a, admin): + """Lease data created directly in the test DB (never through the API), so + the conftest ``client`` fixture never clobbers the per-request dependency + override that ``http_server`` installs. + + FIX1 (NOT NULL constraint for org_id): reuses conftest ``org_a`` (where + ``admin`` user is already OWNER via conftest ``org_a`` fixture membership + setup) + ``admin`` user for canonical ownership chain consistency with + ``admin_headers`` calls in tests. Property/Tenant/Lease are scoped to + ``org_a`` so any admin user HTTP call resolves to the same org. + """ + del admin # Consumed transitively by org_a fixture to bind OWNER membership; + # admin_headers already carries the same user's API key. + prop = Property( + organization_id=org_a.id, + name="Sunset Tower", + address="1 Roxas Blvd", + city="Pasay", + total_units=4, + ) + db_session.add(prop) + db_session.flush() + unit = Unit( + property_id=prop.id, + unit_number="101", + floor="1", + size_sqm="32.50", + monthly_rent="12000.00", + status=UnitStatus.vacant, + ) + tenant = Tenant( + organization_id=org_a.id, + full_name="Juan Dela Cruz", + phone="+639170000000", + ) + db_session.add_all([unit, tenant]) + db_session.flush() + lease = Lease( + unit_id=unit.id, + tenant_id=tenant.id, + start_date="2026-01-01", + end_date="2026-12-31", + monthly_rent="12000.00", + deposit="24000.00", + status=LeaseStatus.active, + ) + db_session.add(lease) + db_session.commit() + db_session.refresh(lease) + return lease.id + + +def _concurrent(worker, n=10): + """Run ``worker(i)`` on n threads and return results in order.""" + with concurrent.futures.ThreadPoolExecutor(max_workers=n) as ex: + return [f.result() for f in [ex.submit(worker, i) for i in range(n)]] + + +def _new_client(base_url, api_key): + return httpx.Client(base_url=base_url, headers={"Authorization": f"Bearer {api_key}"}, timeout=30.0) + + +def _final_state(test_engine, key, income_id): + """Business-state fingerprint for one idempotent create+confirm event.""" + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + rows = db.execute( + text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() + status = db.execute( + text("SELECT status FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() + create_audits = _audit_count(db, "incomes", "create", income_id) + confirm_audits = _audit_count(db, "incomes", "confirm", income_id) + return (rows, status, create_audits, confirm_audits) + finally: + db.close() + + +# -------------------------------------------------------------------------- +# income create idempotency +# -------------------------------------------------------------------------- + + +def test_create_income_same_key_sequential_x10( + http_server, admin_headers, lease_id, test_engine +): + base = http_server + key = f"seq-{time.time_ns()}" + statuses, ids = [], [] + with _new_client(base, admin_headers["Authorization"].split()[-1]) as c: + for _ in range(10): + resp = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + statuses.append(resp.status_code) + assert resp.status_code in (200, 201), resp.text + ids.append(resp.json()["id"]) + assert statuses[0] == 201 + assert all(s == 200 for s in statuses[1:]) + assert len(set(ids)) == 1 + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + rows = db.execute( + text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() + create_audits = _audit_count(db, "incomes", "create", ids[0]) + finally: + db.close() + assert rows == 1 + assert create_audits == 1 + + +def test_create_income_same_key_concurrent_x10( + http_server, admin_headers, lease_id, test_engine +): + key = f"con-{time.time_ns()}" + statuses, ids = [], [] + + def worker(i): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + resp = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + return resp.status_code, resp.json().get("id") + + results = _concurrent(worker) + for code, _id in results: + assert code in (200, 201) + ids.append(_id) + assert len(set(ids)) == 1 + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + rows = db.execute( + text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() + create_audits = _audit_count(db, "incomes", "create", ids[0]) + finally: + db.close() + assert rows == 1 # UNIQUE backstop: exactly one row landed + assert create_audits == 1 + + +# -------------------------------------------------------------------------- +# income confirm / reverse +# -------------------------------------------------------------------------- + + +def test_income_confirm_sequential_and_concurrent_x10( + http_server, admin_headers, lease_id, test_engine +): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + # sequential x10 on one income + inc1 = c.post(f"{API}/incomes", json=_income_payload(lease_id, f"cf-s-{time.time_ns()}")).json() + for _ in range(10): + resp = c.post(f"{API}/incomes/{inc1['id']}/confirm") + assert resp.status_code == 200, resp.text + assert resp.json()["status"] == "confirmed" + # concurrent x10 on a second income + inc2 = c.post(f"{API}/incomes", json=_income_payload(lease_id, f"cf-c-{time.time_ns()}")).json() + + def worker(i): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: + return cc.post(f"{API}/incomes/{inc2['id']}/confirm") + + resps = _concurrent(worker) + for r in resps: + assert r.status_code == 200, r.text + assert r.json()["status"] == "confirmed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + for inc_id in (inc1["id"], inc2["id"]): + status = db.execute( + text("SELECT status FROM incomes WHERE id=:i"), {"i": inc_id} + ).scalar() + assert status == "confirmed" + assert _audit_count(db, "incomes", "confirm", inc_id) == 1 + finally: + db.close() + + +def test_income_reverse_sequential_and_concurrent_x10( + http_server, admin_headers, lease_id, test_engine +): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + # create as confirmed (admin), then reverse sequential x10 + inc1 = c.post( + f"{API}/incomes", json=_income_payload(lease_id, f"rv-s-{time.time_ns()}", status="confirmed") + ).json() + for _ in range(10): + resp = c.post(f"{API}/incomes/{inc1['id']}/reverse") + assert resp.status_code == 200, resp.text + assert resp.json()["status"] == "reversed" + # concurrent x10 on a second income + inc2 = c.post( + f"{API}/incomes", json=_income_payload(lease_id, f"rv-c-{time.time_ns()}", status="confirmed") + ).json() + + def worker(i): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: + return cc.post(f"{API}/incomes/{inc2['id']}/reverse") + + resps = _concurrent(worker) + for r in resps: + assert r.status_code == 200, r.text + assert r.json()["status"] == "reversed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + for inc_id in (inc1["id"], inc2["id"]): + status = db.execute( + text("SELECT status FROM incomes WHERE id=:i"), {"i": inc_id} + ).scalar() + assert status == "reversed" + assert _audit_count(db, "incomes", "reverse", inc_id) == 1 + finally: + db.close() + + +# -------------------------------------------------------------------------- +# expense approve / reject / pay / reverse +# -------------------------------------------------------------------------- + + +def test_expense_approve_reject_pay_reverse_sequential_and_concurrent_x10( + http_server, admin_headers, manager_headers, test_engine, lease_id +): + """FIX1 (ownership anchor): lease_id fixture guarantees ORG+PROPERTY+UNIT rows + exist in the test-engine DB; we then look up a valid unit_id for expense payloads.""" + _S = sessionmaker(bind=test_engine, expire_on_commit=False) + with _S() as _s: + _unit_id = _s.execute(text("SELECT id FROM units ORDER BY id LIMIT 1")).scalar() + admin_key = admin_headers["Authorization"].split()[-1] + with _new_client(http_server, admin_key) as c: + def _mk_expense(): + resp = c.post( + f"{API}/expenses", + json=_expense_payload(unit_id=_unit_id), + headers=manager_headers, + ) + assert resp.status_code == 201, resp.text + return resp.json()["id"] + + # --- approve: sequential x10 then concurrent x10 --- + e1 = _mk_expense() + for _ in range(10): + r = c.post(f"{API}/expenses/{e1}/approve") + assert r.status_code == 200 and r.json()["status"] == "approved" + e2 = _mk_expense() + + def approve(i): + with _new_client(http_server, admin_key) as cc: + return cc.post(f"{API}/expenses/{e2}/approve") + + for r in _concurrent(approve): + assert r.status_code == 200 and r.json()["status"] == "approved" + + # --- reject: concurrent x10 --- + e3 = _mk_expense() + + def reject(i): + with _new_client(http_server, admin_key) as cc: + return cc.post(f"{API}/expenses/{e3}/reject") + + for r in _concurrent(reject): + assert r.status_code == 200 and r.json()["status"] == "rejected" + + # --- pay: concurrent x10 (approve first) --- + e4 = _mk_expense() + assert c.post(f"{API}/expenses/{e4}/approve").json()["status"] == "approved" + + def pay(i): + with _new_client(http_server, admin_key) as cc: + return cc.post(f"{API}/expenses/{e4}/pay") + + for r in _concurrent(pay): + assert r.status_code == 200 and r.json()["status"] == "paid" + + # --- reverse: concurrent x10 (approve+pay first) --- + e5 = _mk_expense() + c.post(f"{API}/expenses/{e5}/approve") + c.post(f"{API}/expenses/{e5}/pay") + + def reverse_exp(i): + with _new_client(http_server, admin_key) as cc: + return cc.post(f"{API}/expenses/{e5}/reverse") + + for r in _concurrent(reverse_exp): + assert r.status_code == 200 and r.json()["status"] == "reversed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e1}).scalar() == "approved" + assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e2}).scalar() == "approved" + assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e3}).scalar() == "rejected" + assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e4}).scalar() == "paid" + assert db.execute(text("SELECT status FROM expenses WHERE id=:i"), {"i": e5}).scalar() == "reversed" + # each transition audited exactly once per expense + assert _audit_count(db, "expenses", "approve", e1) == 1 + assert _audit_count(db, "expenses", "approve", e2) == 1 + assert _audit_count(db, "expenses", "reject", e3) == 1 + assert _audit_count(db, "expenses", "pay", e4) == 1 + assert _audit_count(db, "expenses", "reverse", e5) == 1 + finally: + db.close() + + +# -------------------------------------------------------------------------- +# commission settlement confirm +# -------------------------------------------------------------------------- + + +def _rule(client, headers): + resp = client.post( + f"{API}/commission/rules", + json={"name": "Rent commission", "rule_type": "percentage", "value": "10", "agent_role": "出租"}, + headers=headers, + ) + assert resp.status_code == 201, resp.text + return resp.json()["id"] + + +def test_settlement_confirm_sequential_and_concurrent_x10( + http_server, admin_headers, agent, lease_id, test_engine +): + admin_key = admin_headers["Authorization"].split()[-1] + with _new_client(http_server, admin_key) as c: + rule_id = _rule(c, admin_headers) + s1 = c.post( + f"{API}/commission/settlements", + json={"agent_id": agent[0].id, "lease_id": lease_id, "rule_id": rule_id}, + headers=admin_headers, + ).json()["id"] + for _ in range(10): + r = c.post(f"{API}/commission/settlements/{s1}/confirm") + assert r.status_code == 200 and r.json()["status"] == "confirmed" + s2 = c.post( + f"{API}/commission/settlements", + json={"agent_id": agent[0].id, "lease_id": lease_id, "rule_id": rule_id}, + headers=admin_headers, + ).json()["id"] + + def confirm(i): + with _new_client(http_server, admin_key) as cc: + return cc.post(f"{API}/commission/settlements/{s2}/confirm") + + for r in _concurrent(confirm): + assert r.status_code == 200 and r.json()["status"] == "confirmed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + for sid in (s1, s2): + status = db.execute( + text("SELECT status FROM commission_settlements WHERE id=:i"), {"i": sid} + ).scalar() + assert status == "confirmed" + assert _audit_count(db, "commission_settlements", "confirm", sid) == 1 + finally: + db.close() + + +# -------------------------------------------------------------------------- +# timeout-after-commit + stale callback replay +# -------------------------------------------------------------------------- + + +def test_timeout_after_commit_replay(http_server, admin_headers, lease_id, test_engine): + """DB committed but HTTP response lost: retry with the same key must + return the existing row, never a second one.""" + key = f"timeout-{time.time_ns()}" + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + first = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + assert first.status_code == 201 + income_id = first.json()["id"] + # simulate the client never receiving the response: retry same key + retry = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + assert retry.status_code == 200 + assert retry.json()["id"] == income_id + + # timeout-after-commit on confirm as well + conf1 = c.post(f"{API}/incomes/{income_id}/confirm") + assert conf1.status_code == 200 + conf2 = c.post(f"{API}/incomes/{income_id}/confirm") + assert conf2.status_code == 200 and conf2.json()["status"] == "confirmed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + assert db.execute( + text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() == 1 + assert _audit_count(db, "incomes", "create", income_id) == 1 + assert _audit_count(db, "incomes", "confirm", income_id) == 1 + finally: + db.close() + + +def test_stale_callback_replay(http_server, admin_headers, lease_id, test_engine): + """A stale card replayed after the event fully landed must not re-create + anything and must return the current state.""" + key = f"stale-{time.time_ns()}" + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + created = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + assert created.status_code == 201 + income_id = created.json()["id"] + assert c.post(f"{API}/incomes/{income_id}/confirm").json()["status"] == "confirmed" + + # stale replay of the whole flow: create (same key) + confirm + replay = c.post(f"{API}/incomes", json=_income_payload(lease_id, key)) + assert replay.status_code == 200 + assert replay.json()["id"] == income_id + assert replay.json()["status"] == "confirmed" + replay_confirm = c.post(f"{API}/incomes/{income_id}/confirm") + assert replay_confirm.status_code == 200 + assert replay_confirm.json()["status"] == "confirmed" + + Session = sessionmaker(bind=test_engine, expire_on_commit=False) + db = Session() + try: + assert db.execute( + text("SELECT count(*) FROM incomes WHERE idempotency_key=:k"), {"k": key} + ).scalar() == 1 + assert _audit_count(db, "incomes", "create", income_id) == 1 + assert _audit_count(db, "incomes", "confirm", income_id) == 1 + finally: + db.close() + + +# -------------------------------------------------------------------------- +# system invariant: N identical commands == executing once +# -------------------------------------------------------------------------- + + +def test_invariant_n_identical_commands_equal_one( + http_server, admin_headers, lease_id, test_engine +): + """N identical commands (sequential or concurrent) must leave the same + final business state as executing the command exactly once.""" + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as c: + # once + key1 = f"once-{time.time_ns()}" + inc_once = c.post(f"{API}/incomes", json=_income_payload(lease_id, key1)).json() + c.post(f"{API}/incomes/{inc_once['id']}/confirm") + state_once = _final_state(test_engine, key1, inc_once["id"]) + + # sequential x10 + key2 = f"seq-{time.time_ns()}" + inc_seq = c.post(f"{API}/incomes", json=_income_payload(lease_id, key2)).json() + for _ in range(10): + c.post(f"{API}/incomes", json=_income_payload(lease_id, key2)) + c.post(f"{API}/incomes/{inc_seq['id']}/confirm") + state_seq = _final_state(test_engine, key2, inc_seq["id"]) + + # concurrent x10 + key3 = f"conc-{time.time_ns()}" + + def full_flow(i): + with _new_client(http_server, admin_headers["Authorization"].split()[-1]) as cc: + created = cc.post(f"{API}/incomes", json=_income_payload(lease_id, key3)).json() + cc.post(f"{API}/incomes/{created['id']}/confirm") + return created["id"] + + ids = _concurrent(full_flow) + state_concurrent = _final_state(test_engine, key3, ids[0]) + + assert len(set(ids)) == 1 + assert state_once == (1, "confirmed", 1, 1) + assert state_seq == state_once + assert state_concurrent == state_once diff --git a/tests/test_milestone_1_org_scope_p0.py b/tests/test_milestone_1_org_scope_p0.py new file mode 100644 index 0000000..49dc9e1 --- /dev/null +++ b/tests/test_milestone_1_org_scope_p0.py @@ -0,0 +1,521 @@ +"""PASAY-MILESTONE-001 Targeted Regression Tests. + +覆盖: Income / Expense / Lease / Repair / Tenant 5 个域的跨组织访问 fail-closed 验证。 + +Pattern (每个域做 T1 + T2)： +- T1 list_isolation: owner_a 创建 OrgA 的 1 条记录；owner_b GET /api/v1/ list + -> 返回空列表，看不见 OrgA 数据（数据泄露路径封闭证据） +- T2 get_cross_org_404: owner_a 创建 OrgA 记录 -> id=X；owner_b GET /api/v1//X + -> 必须 HTTP 404（fail-closed 404，not 403 — 因为存在性不泄露） +- T3 (only for create payloads): owner_a 尝试用 owner_b 下的 org_id 创建记录 + -> 必须 403/409（CrossOrgReference） +""" +from __future__ import annotations + +import pytest + +API = "/api/v1" + + +def _bearer(key: str) -> dict[str, str]: + return {"Authorization": f"Bearer {key}"} + + +def _setup_org_a_property_and_unit(client, owner_a, org_a): + """owner_a 在 OrgA 创建 property + unit，返回 (property_id, unit_id)。""" + owner_a_headers = _bearer(owner_a[1]) + resp = client.post( + f"{API}/properties", + json={ + "organization_id": org_a.id, + "name": "Sunset Tower A", + "address": "1 Roxas Blvd", + "city": "Pasay", + "total_units": 4, + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + property_id_a = resp.json()["id"] + + resp = client.post( + f"{API}/units", + json={ + "property_id": property_id_a, + "unit_number": "101", + "floor": "1", + "size_sqm": "32.50", + "monthly_rent": "12000.00", + "status": "vacant", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + unit_id_a = resp.json()["id"] + return property_id_a, unit_id_a + + +def _setup_org_b_property_and_unit(client, owner_b, org_b): + """owner_b 在 OrgB 创建 property2 + unit2，返回 (property_id, unit_id)。""" + owner_b_headers = _bearer(owner_b[1]) + resp = client.post( + f"{API}/properties", + json={ + "organization_id": org_b.id, + "name": "Sunrise Tower B", + "address": "2 Ayala Ave", + "city": "Makati", + "total_units": 2, + }, + headers=owner_b_headers, + ) + assert resp.status_code == 201, resp.text + property_id_b = resp.json()["id"] + + resp = client.post( + f"{API}/units", + json={ + "property_id": property_id_b, + "unit_number": "201", + "floor": "2", + "size_sqm": "45.00", + "monthly_rent": "18000.00", + "status": "vacant", + }, + headers=owner_b_headers, + ) + assert resp.status_code == 201, resp.text + unit_id_b = resp.json()["id"] + return property_id_b, unit_id_b + + +# --------------------------------------------------------------------------- +# Tenant: T1 (list_isolation), T2 (get_cross_org_404), T3 (cross-org create) +# --------------------------------------------------------------------------- + + +def test_tenant_t1_list_isolation(client, owner_a, owner_b, org_a, org_b): + _setup_org_a_property_and_unit(client, owner_a, org_a) + _setup_org_b_property_and_unit(client, owner_b, org_b) + owner_a_headers = _bearer(owner_a[1]) + owner_b_headers = _bearer(owner_b[1]) + + resp = client.post( + f"{API}/tenants", + json={ + "organization_id": org_a.id, + "full_name": "Tenant A", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + + resp_list_b = client.get(f"{API}/tenants", headers=owner_b_headers) + assert resp_list_b.status_code == 200, resp_list_b.text + items = resp_list_b.json() + assert len(items) == 0 + assert not any(item.get("full_name") == "Tenant A" for item in items) + + +def test_tenant_t2_get_cross_org_404(client, owner_a, owner_b, org_a, org_b): + _setup_org_a_property_and_unit(client, owner_a, org_a) + _setup_org_b_property_and_unit(client, owner_b, org_b) + owner_a_headers = _bearer(owner_a[1]) + owner_b_headers = _bearer(owner_b[1]) + + resp = client.post( + f"{API}/tenants", + json={ + "organization_id": org_a.id, + "full_name": "Tenant A", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + id_t_a = resp.json()["id"] + + resp_get_b = client.get(f"{API}/tenants/{id_t_a}", headers=owner_b_headers) + assert resp_get_b.status_code == 404 + + +def test_tenant_t3_create_cross_org_reference_blocked( + client, owner_a, owner_b, org_a, org_b +): + """Bonus T3: owner_a 尝试用 org_b.id 创建 Tenant -> 403 (ScopeBlocked).""" + _setup_org_a_property_and_unit(client, owner_a, org_a) + _setup_org_b_property_and_unit(client, owner_b, org_b) + owner_a_headers = _bearer(owner_a[1]) + + resp = client.post( + f"{API}/tenants", + json={ + "organization_id": org_b.id, + "full_name": "Tenant Cross Org", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 403 + + +# --------------------------------------------------------------------------- +# Lease: T1 (list_isolation), T2 (get_cross_org_404) +# --------------------------------------------------------------------------- + + +def _create_org_a_tenant_and_lease(client, owner_a, org_a): + owner_a_headers = _bearer(owner_a[1]) + _property_id_a, unit_id_a = _setup_org_a_property_and_unit(client, owner_a, org_a) + + resp = client.post( + f"{API}/tenants", + json={ + "organization_id": org_a.id, + "full_name": "Tenant A", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + tenant_id_a = resp.json()["id"] + + resp = client.post( + f"{API}/leases", + json={ + "unit_id": unit_id_a, + "tenant_id": tenant_id_a, + "start_date": "2026-01-01", + "end_date": "2026-12-31", + "monthly_rent": "12000.00", + "deposit": "24000.00", + "status": "active", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + lease_id_a = resp.json()["id"] + return lease_id_a + + +def test_lease_t1_list_isolation(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + _create_org_a_tenant_and_lease(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_list_b = client.get(f"{API}/leases", headers=owner_b_headers) + assert resp_list_b.status_code == 200, resp_list_b.text + items = resp_list_b.json() + assert len(items) == 0 + + +def test_lease_t2_get_cross_org_404(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + lease_id_a = _create_org_a_tenant_and_lease(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_get_b = client.get(f"{API}/leases/{lease_id_a}", headers=owner_b_headers) + assert resp_get_b.status_code == 404 + + +# --------------------------------------------------------------------------- +# Income: T1 (list_isolation), T2 (get_cross_org_404) +# --------------------------------------------------------------------------- + + +def _create_org_a_confirmed_income(client, owner_a, org_a): + owner_a_headers = _bearer(owner_a[1]) + lease_id_a = _create_org_a_tenant_and_lease(client, owner_a, org_a) + + resp = client.post( + f"{API}/incomes", + json={ + "lease_id": lease_id_a, + "amount": "12000.00", + "received_date": "2026-02-01", + "payment_method": "cash", + "status": "confirmed", + "description": "rent Feb 2026", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + income_id_a = resp.json()["id"] + return income_id_a + + +def test_income_t1_list_isolation(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + _create_org_a_confirmed_income(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_list_b = client.get(f"{API}/incomes", headers=owner_b_headers) + assert resp_list_b.status_code == 200, resp_list_b.text + items = resp_list_b.json() + assert len(items) == 0 + + +def test_income_t2_get_cross_org_404(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + income_id_a = _create_org_a_confirmed_income(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_get_b = client.get(f"{API}/incomes/{income_id_a}", headers=owner_b_headers) + assert resp_get_b.status_code == 404 + + +# --------------------------------------------------------------------------- +# Expense: T1 (list_isolation), T2 (get_cross_org_404) +# --------------------------------------------------------------------------- + + +def _create_org_a_pending_expense(client, owner_a, org_a): + owner_a_headers = _bearer(owner_a[1]) + _property_id_a, unit_id_a = _setup_org_a_property_and_unit(client, owner_a, org_a) + + resp = client.post( + f"{API}/expenses", + json={ + "expense_date": "2026-02-05", + "category": "repair", + "amount": "5000.00", + "payee": "Fix-It Co", + "description": "AC repair", + "unit_id": unit_id_a, + "status": "pending", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + expense_id_a = resp.json()["id"] + return expense_id_a + + +def test_expense_t1_list_isolation(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + _create_org_a_pending_expense(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_list_b = client.get(f"{API}/expenses", headers=owner_b_headers) + assert resp_list_b.status_code == 200, resp_list_b.text + items = resp_list_b.json() + assert len(items) == 0 + + +def test_expense_t2_get_cross_org_404(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + expense_id_a = _create_org_a_pending_expense(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_get_b = client.get(f"{API}/expenses/{expense_id_a}", headers=owner_b_headers) + assert resp_get_b.status_code == 404 + + +# --------------------------------------------------------------------------- +# Repair: T1 (list_isolation), T2 (get_cross_org_404) +# --------------------------------------------------------------------------- + + +def _create_org_a_repair(client, owner_a, org_a): + owner_a_headers = _bearer(owner_a[1]) + _property_id_a, unit_id_a = _setup_org_a_property_and_unit(client, owner_a, org_a) + + resp = client.post( + f"{API}/repairs", + json={ + "issue": "漏水", + "unit_id": unit_id_a, + "created_source": "manual", + }, + headers=owner_a_headers, + ) + assert resp.status_code == 201, resp.text + repair_id_a = resp.json()["id"] + return repair_id_a + + +def test_repair_t1_list_isolation(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + _create_org_a_repair(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_list_b = client.get(f"{API}/repairs", headers=owner_b_headers) + assert resp_list_b.status_code == 200, resp_list_b.text + body = resp_list_b.json() + items = body["items"] + assert len(items) == 0 + assert body["total"] == 0 + + +def test_repair_t2_get_cross_org_404(client, owner_a, owner_b, org_a, org_b): + _setup_org_b_property_and_unit(client, owner_b, org_b) + repair_id_a = _create_org_a_repair(client, owner_a, org_a) + owner_b_headers = _bearer(owner_b[1]) + + resp_get_b = client.get(f"{API}/repairs/{repair_id_a}", headers=owner_b_headers) + assert resp_get_b.status_code == 404 + + +# --------------------------------------------------------------------------- +# FIX2 Blocker regressions (Owner 3 Blocker explicit HTTP/migration contracts) +# --------------------------------------------------------------------------- + + +def test_fix2_blocker2_income_patch_lease_id_none_http_409( + client, owner_a, org_a +): + """FIX2 Blocker 2: IncomeUpdate.lease_id = None (PATCH /incomes/{id}) + MUST return HTTP 409 Conflict (CrossOrgReference through the shared + scope_exception_to_http translator), NOT an unhandled 500. + + (IncomeCreate.lease_id is already required int per Pydantic; the real + place a caller can sneak in a None is via IncomeUpdate.lease_id: + int | None = None on the PATCH endpoint.) + """ + owner_a_headers = _bearer(owner_a[1]) + # Create a perfectly valid pending Income (with a real lease) first. + lease_id_a = _create_org_a_tenant_and_lease(client, owner_a, org_a) + resp_create = client.post( + f"{API}/incomes", + json={ + "lease_id": lease_id_a, + "amount": "12000.00", + "received_date": "2026-03-01", + "payment_method": "cash", + "status": "pending", + "description": "valid income", + }, + headers=owner_a_headers, + ) + assert resp_create.status_code == 201, resp_create.text + income_id = resp_create.json()["id"] + + # Then PATCH with lease_id=None explicitly (keep amount unchanged so + # other fields don't trigger their own 422). The router should convert + # the CrossOrgReference raised by _check_lease() to HTTP 409, never + # let it bubble up as 500. + resp_patch = client.patch( + f"{API}/incomes/{income_id}", + json={ + "lease_id": None, + "amount": "12000.00", + "received_date": "2026-03-01", + "payment_method": "cash", + }, + headers=owner_a_headers, + ) + assert resp_patch.status_code == 409, ( + f"Expected HTTP 409 Conflict for PATCH lease_id=None, " + f"got {resp_patch.status_code}. Body: {resp_patch.text[:500]!r}" + ) + body = resp_patch.json() + detail = str(body.get("detail", "")) + assert ( + "canonical lease ownership" in detail or "lease_id is required" in detail + ), body + + +def test_fix2_blocker2_income_cross_org_idempotency_replay_http_404( + client, owner_a, owner_b, org_a, org_b +): + """FIX2 Blocker 2: Org A creates an Income with idempotency key=K (using + OrgA lease). OrgB then replays the SAME key=K using OrgB's OWN lease — + must return HTTP 404 (fail-closed: idempotency hit must verify that the + FOUND Income belongs to the CALLER's org; otherwise it looks like a miss, + i.e. 404, not returning OrgA's record nor re-creating under same key). + + The previous M1-FIX1 code already guarded this; we keep the hard contract + explicit here so any future regression that reopens the cross-org replay + door (e.g. removing _assert_income_co_org before idempotency return) is + caught in CI. + """ + owner_a_headers = _bearer(owner_a[1]) + owner_b_headers = _bearer(owner_b[1]) + + shared_key = "idem-key-fix2-001" + + # --- OrgA setup: create 1 Income with shared_key using OrgA lease --- + lease_id_a = _create_org_a_tenant_and_lease(client, owner_a, org_a) + resp_a_create = client.post( + f"{API}/incomes", + json={ + "lease_id": lease_id_a, + "amount": "12000.00", + "received_date": "2026-03-01", + "payment_method": "cash", + "status": "confirmed", + "idempotency_key": shared_key, + "description": "OrgA creates with shared key", + }, + headers=owner_a_headers, + ) + assert resp_a_create.status_code == 201, resp_a_create.text + income_a_id = resp_a_create.json()["id"] + # Sanity: OrgA replaying the same key returns the SAME record (HTTP 200) + resp_a_replay = client.post( + f"{API}/incomes", + json={ + "lease_id": lease_id_a, + "amount": "12000.00", + "received_date": "2026-03-01", + "payment_method": "cash", + "status": "confirmed", + "idempotency_key": shared_key, + }, + headers=owner_a_headers, + ) + assert resp_a_replay.status_code == 200, resp_a_replay.text + assert resp_a_replay.json()["id"] == income_a_id + + # --- OrgB setup: create its OWN, perfectly valid lease in OrgB --- + _property_id_b, unit_id_b = _setup_org_b_property_and_unit( + client, owner_b, org_b + ) + tenant_id_b_resp = client.post( + f"{API}/tenants", + json={ + "organization_id": org_b.id, + "full_name": "Tenant B", + "phone": "+639170000002", + }, + headers=owner_b_headers, + ) + assert tenant_id_b_resp.status_code == 201, tenant_id_b_resp.text + tenant_id_b = tenant_id_b_resp.json()["id"] + lease_b_resp = client.post( + f"{API}/leases", + json={ + "unit_id": unit_id_b, + "tenant_id": tenant_id_b, + "start_date": "2026-02-01", + "end_date": "2027-01-31", + "monthly_rent": "15000.00", + "deposit": "30000.00", + "status": "active", + }, + headers=owner_b_headers, + ) + assert lease_b_resp.status_code == 201, lease_b_resp.text + lease_id_b = lease_b_resp.json()["id"] + + # --- ACTUAL TEST: OrgB replays shared_key with OrgB's lease_id. --- + # OrgB's request is perfectly formed (valid OrgB lease), but the key K + # already hit for OrgA's Income → fail-closed: MUST NOT return OrgA's + # record as if it was OrgB's → HTTP 404. + resp_b_replay = client.post( + f"{API}/incomes", + json={ + "lease_id": lease_id_b, + "amount": "15000.00", + "received_date": "2026-03-02", + "payment_method": "bank_transfer", + "status": "pending", + "idempotency_key": shared_key, + "description": "OrgB replay of shared key", + }, + headers=owner_b_headers, + ) + assert resp_b_replay.status_code == 404, ( + f"Expected HTTP 404 fail-closed on cross-org idempotency replay, " + f"got {resp_b_replay.status_code}. Body: {resp_b_replay.text[:600]!r}" + ) + # Belt-and-suspenders: OrgB still cannot read OrgA's record (paranoia). + resp_peek = client.get(f"{API}/incomes/{income_a_id}", headers=owner_b_headers) + assert resp_peek.status_code == 404 diff --git a/tests/test_properties.py b/tests/test_properties.py index 5fc04ce..6b2f9ad 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -1,99 +1,99 @@ -API = "/api/v1" - - -def test_create_property(client, admin_headers): - resp = client.post( - f"{API}/properties", - json={"name": "Seaside", "address": "12 Beach Rd", "city": "Pasay", "total_units": 3}, - headers=admin_headers, - ) - assert resp.status_code == 201 - body = resp.json() - assert body["name"] == "Seaside" - assert body["city"] == "Pasay" - assert body["is_active"] is True - - -def test_manager_can_create_property(client, manager_headers): - resp = client.post( - f"{API}/properties", - json={"name": "Manager Co.", "address": "x", "city": "Pasay"}, - headers=manager_headers, - ) - assert resp.status_code == 201 - - -def test_list_and_get_property(client, admin_headers, property_id): - resp = client.get(f"{API}/properties", headers=admin_headers) - assert resp.status_code == 200 - assert [p["id"] for p in resp.json()] == [property_id] - - resp = client.get(f"{API}/properties/{property_id}", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["name"] == "Sunset Tower" - - -def test_update_property(client, admin_headers, property_id): - resp = client.patch( - f"{API}/properties/{property_id}", json={"city": "Manila"}, headers=admin_headers - ) - assert resp.status_code == 200 - assert resp.json()["city"] == "Manila" - - -def test_delete_property_soft(client, admin_headers, property_id): - resp = client.delete(f"{API}/properties/{property_id}", headers=admin_headers) - assert resp.status_code == 200 - resp = client.get(f"{API}/properties/{property_id}", headers=admin_headers) - assert resp.status_code == 404 - - -def test_get_missing_property_404(client, admin_headers): - resp = client.get(f"{API}/properties/99999", headers=admin_headers) - assert resp.status_code == 404 - - -def test_create_unit_and_read(client, admin_headers, property_id): - resp = client.post( - f"{API}/units", - json={ - "property_id": property_id, - "unit_number": "A-1", - "monthly_rent": "15000.00", - "status": "vacant", - }, - headers=admin_headers, - ) - assert resp.status_code == 201 - assert resp.json()["monthly_rent"] == "15000.00" - - resp = client.get(f"{API}/units/{resp.json()['id']}", headers=admin_headers) - assert resp.status_code == 200 - - -def test_create_unit_unknown_property_404(client, admin_headers): - resp = client.post( - f"{API}/units", - json={"property_id": 99999, "unit_number": "X", "monthly_rent": "100.00"}, - headers=admin_headers, - ) - assert resp.status_code == 404 - - -def test_update_and_delete_unit(client, admin_headers, unit_id): - resp = client.patch( - f"{API}/units/{unit_id}", json={"status": "maintenance"}, headers=admin_headers - ) - assert resp.status_code == 200 - assert resp.json()["status"] == "maintenance" - - resp = client.delete(f"{API}/units/{unit_id}", headers=admin_headers) - assert resp.status_code == 200 - resp = client.get(f"{API}/units/{unit_id}", headers=admin_headers) - assert resp.status_code == 404 - - -def test_agent_can_read_properties(client, agent_headers, property_id): - resp = client.get(f"{API}/properties", headers=agent_headers) - assert resp.status_code == 200 - assert len(resp.json()) == 1 +API = "/api/v1" + + +def test_create_property(client, admin_headers, org_a): + resp = client.post( + f"{API}/properties", + json={"name": "Seaside", "address": "12 Beach Rd", "city": "Pasay", "total_units": 3, "organization_id": org_a.id}, + headers=admin_headers, + ) + assert resp.status_code == 201 + body = resp.json() + assert body["name"] == "Seaside" + assert body["city"] == "Pasay" + assert body["is_active"] is True + + +def test_manager_can_create_property(client, manager_headers, org_a): + resp = client.post( + f"{API}/properties", + json={"name": "Manager Co.", "address": "x", "city": "Pasay", "total_units": 1, "organization_id": org_a.id}, + headers=manager_headers, + ) + assert resp.status_code == 201 + + +def test_list_and_get_property(client, admin_headers, property_id): + resp = client.get(f"{API}/properties", headers=admin_headers) + assert resp.status_code == 200 + assert [p["id"] for p in resp.json()] == [property_id] + + resp = client.get(f"{API}/properties/{property_id}", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["name"] == "Sunset Tower" + + +def test_update_property(client, admin_headers, property_id): + resp = client.patch( + f"{API}/properties/{property_id}", json={"city": "Manila"}, headers=admin_headers + ) + assert resp.status_code == 200 + assert resp.json()["city"] == "Manila" + + +def test_delete_property_soft(client, admin_headers, property_id): + resp = client.delete(f"{API}/properties/{property_id}", headers=admin_headers) + assert resp.status_code == 200 + resp = client.get(f"{API}/properties/{property_id}", headers=admin_headers) + assert resp.status_code == 404 + + +def test_get_missing_property_404(client, admin_headers): + resp = client.get(f"{API}/properties/99999", headers=admin_headers) + assert resp.status_code == 404 + + +def test_create_unit_and_read(client, admin_headers, property_id): + resp = client.post( + f"{API}/units", + json={ + "property_id": property_id, + "unit_number": "A-1", + "monthly_rent": "15000.00", + "status": "vacant", + }, + headers=admin_headers, + ) + assert resp.status_code == 201 + assert resp.json()["monthly_rent"] == "15000.00" + + resp = client.get(f"{API}/units/{resp.json()['id']}", headers=admin_headers) + assert resp.status_code == 200 + + +def test_create_unit_unknown_property_404(client, admin_headers): + resp = client.post( + f"{API}/units", + json={"property_id": 99999, "unit_number": "X", "monthly_rent": "100.00"}, + headers=admin_headers, + ) + assert resp.status_code == 404 + + +def test_update_and_delete_unit(client, admin_headers, unit_id): + resp = client.patch( + f"{API}/units/{unit_id}", json={"status": "maintenance"}, headers=admin_headers + ) + assert resp.status_code == 200 + assert resp.json()["status"] == "maintenance" + + resp = client.delete(f"{API}/units/{unit_id}", headers=admin_headers) + assert resp.status_code == 200 + resp = client.get(f"{API}/units/{unit_id}", headers=admin_headers) + assert resp.status_code == 404 + + +def test_agent_can_read_properties(client, agent_headers, property_id): + resp = client.get(f"{API}/properties", headers=agent_headers) + assert resp.status_code == 200 + assert len(resp.json()) == 1 diff --git a/tests/test_property_channel_p0_025.py b/tests/test_property_channel_p0_025.py index 4b3d705..1890fca 100644 --- a/tests/test_property_channel_p0_025.py +++ b/tests/test_property_channel_p0_025.py @@ -271,9 +271,9 @@ def test_scoped_get_property_must_be_in_my_org( assert prop_x.id == px.id assert membership.role == OrganizationRole.OWNER.value - with pytest.raises(ScopeBlocked): + with pytest.raises(LookupError): scoped_get_property(db_session, py.id, for_user_id=alice.id) - with pytest.raises(ScopeBlocked): + with pytest.raises(LookupError): scoped_get_property(db_session, px.id, for_user_id=bob.id) def test_scoped_get_unit_must_be_in_my_org( @@ -288,7 +288,7 @@ def test_scoped_get_unit_must_be_in_my_org( unit, _m = scoped_get_unit(db_session, ux.id, for_user_id=alice.id) assert unit.id == ux.id - with pytest.raises(ScopeBlocked): + with pytest.raises(LookupError): scoped_get_unit(db_session, uy.id, for_user_id=alice.id) @@ -534,8 +534,8 @@ def test_removed_membership_immediately_loses_access( _mark_removed(db_session, secretary_carol_m) db_session.commit() - # After removal — ScopeBlocked immediately - with pytest.raises(ScopeBlocked): + # After removal — LookupError immediately (fail-closed 404) + with pytest.raises(LookupError): scoped_get_property(db_session, prop.id, for_user_id=carol.id) def test_secretary_cannot_bind_or_revoke_channel( @@ -545,8 +545,8 @@ def test_secretary_cannot_bind_or_revoke_channel( u = _create_unit(db_session, prop, "101") db_session.commit() - # bind — must be OWNER - with pytest.raises(ScopeBlocked): + # bind — must be OWNER (FIX1: SECRETARY has ACTIVE membership so raises OwnerRequired, not ScopeBlocked) + with pytest.raises(OwnerRequired): bind_unit_channel( db_session, unit_id=u.id, @@ -767,7 +767,7 @@ def test_replace_writes_replaced_and_revoked_audits( .order_by(AuditLog.id.asc()) .all() ) - actions = {l.action: l.record_id for l in logs} + actions = {log.action: log.record_id for log in logs} assert AuditAction.unit_channel_bound in actions assert AuditAction.unit_channel_revoked in actions assert actions[AuditAction.unit_channel_revoked] == old.id @@ -852,67 +852,58 @@ def test_identity_regression_user_principal_chain( # =================================================================== -# 10. Legacy Property.organization_id = NULL compatibility (Issue #25) -# =================================================================== +# 10. Property.organization_id NOT NULL schema truth (FIX1 Blocker #2) +# ===================================================================== + +class TestPropertyOrgIdNotNull: + """M1 + FIX1 contract: Property.org_id is NOT NULL both in DB and ORM. + No legacy NULL row can be inserted via ORM anymore (schema truth parity).""" + + def test_orm_insert_without_organization_id_raises_integrity_error( + self, db_session + ): + """FIX1 Blocker #2: ORM Mapped[int] + nullable=False. A direct NULL + insert MUST raise IntegrityError on flush — no ORM/DB lag.""" + from sqlalchemy.exc import IntegrityError -class TestLegacyPropertyCompatibility: - def _create_legacy_null_org_property(self, db_session, *, name="Legacy-Hotel"): - """Insert a Property with organization_id=NULL (pre-migration legacy).""" prop = Property( - organization_id=None, - name=name, - address=f"Old {name} St", - city="OldPasay", + name="No-Org-Hotel", + address="Ghost St", + city="Nowhere", total_units=1, is_active=True, created_by=0, updated_by=0, ) + prop.organization_id = None # force bypass pydantic / constructor typing db_session.add(prop) - db_session.flush() - return prop - - def test_legacy_null_org_property_survives_orm_insert(self, db_session, org_x): - """Constraint-free nullable column lets legacy rows exist untouched.""" - legacy = self._create_legacy_null_org_property(db_session) - db_session.commit() - db_session.refresh(legacy) - assert legacy.id > 0 - assert legacy.organization_id is None - # Confirm the row is still there (no silent delete) - still_there = ( - db_session.query(Property) - .filter(Property.id == legacy.id) - .one() - ) - assert still_there.organization_id is None - assert still_there.name == legacy.name + with pytest.raises(IntegrityError): + db_session.flush() + db_session.rollback() - def test_scoped_list_properties_excludes_legacy_null_org( - self, db_session, alice, org_x + def test_scoped_list_properties_excludes_cross_org_property( + self, db_session, alice, org_x, org_y ): - """Legacy NULL-org properties are naturally excluded from JOINs.""" - modern = _create_property(db_session, org_x, name="Modern-Tower") - legacy = self._create_legacy_null_org_property(db_session, name="Old-Inn") + """A Property bound to org_y MUST NOT appear in alice (org_x) scoped list.""" + org_x_prop = _create_property(db_session, org_x, name="X-Tower") + org_y_prop = _create_property(db_session, org_y, name="Y-Hotel") db_session.commit() alice_visible = scoped_list_properties(db_session, for_user_id=alice.id) visible_ids = {p.id for p in alice_visible} - assert modern.id in visible_ids - # Legacy row is NOT visible (fail-closed via the inner JOIN), but it - # is still physically present (confirmed in the parallel test above). - assert legacy.id not in visible_ids + assert org_x_prop.id in visible_ids + assert org_y_prop.id not in visible_ids - def test_scoped_get_property_fails_closed_on_legacy_null_org( - self, db_session, alice, org_x + def test_scoped_get_property_fails_closed_on_cross_org( + self, db_session, alice, org_y ): - """Direct scoped get on legacy NULL org raises LookupError (→ 404).""" - legacy = self._create_legacy_null_org_property(db_session) + """Direct scoped get on an org_y property as alice (org_x) → LookupError.""" + org_y_prop = _create_property(db_session, org_y, name="Y-Exclusive") db_session.commit() with pytest.raises(LookupError): - scoped_get_property(db_session, legacy.id, for_user_id=alice.id) + scoped_get_property(db_session, org_y_prop.id, for_user_id=alice.id) # =================================================================== @@ -1095,3 +1086,208 @@ def test_unit_channel_binding_read_from_orm_instance( db_session.refresh(revoked) read_rev = UnitChannelBindingRead.model_validate(revoked) assert isinstance(read_rev.revoked_at, datetime) + + +# =================================================================== +# 14. FIX2 Migration regressions (CRITICAL Blocker #1 namespace guard) +# =================================================================== + + +class TestFix2MigrationExpensePropertyIdNamespace: + """FIX2 Blocker #1: Expense.property_id backfill MUST NEVER accept an + organization_id as a property_id. The two IDs are in DIFFERENT SQL + namespaces; numeric coincidence MUST NOT silently collapse them. + + We test the two pure helper functions (no alembic op) that together + decide the backfill value: + A. ``_resolve_org_unique_property`` — when an org has 0 or >1 active + properties, the candidate-property-id list stays empty, so the + main upgrade loop will fail CLOSED with explicit IDs, never + fall through to ``backfill[eid] = orgs[0]`` (which was the bug). + B. When an org has EXACTLY 1 active property, the candidate list is + [property_id], and property_id is a bona fide row from the + properties table (NOT the org id). We force org.id != prop.id + by inserting orgs and properties out of numeric order, proving + the namespace is not collapsed. + """ + + @staticmethod + def _load_m1c_module(): + """Import the m1c migration module from disk (alembic.versions is + not a Python package, so ``from alembic.versions.X import Y`` fails + with ``ModuleNotFoundError``; use ``importlib`` + absolute file path).""" + import importlib.util as _ilu + import os as _os + + path = _os.path.join( + _os.path.dirname(__file__), + "..", + "alembic", + "versions", + "m1_c_expense_add_property_id.py", + ) + spec = _ilu.spec_from_file_location( + "m1_c_expense_add_property_id", _os.path.abspath(path) + ) + if spec is None or spec.loader is None: + raise RuntimeError(f"cannot locate m1c migration at {path!r}") + mod = _ilu.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + def test_helper_org_unique_property_zero_or_multiple_properties_is_ambiguous( + self, db_session, alice, org_x + ): + """When the resolved org has 0 or >=2 active properties, backfill + returns an EMPTY candidate list for that expense — the main + upgrade loop will FAIL CLOSED instead of guessing (BUG FIX 2-1).""" + m1c = self._load_m1c_module() + _resolve_org_unique_property = m1c._resolve_org_unique_property + + # Org-x has 0 active properties (we never insert one) → empty. + by_expense_empty: dict[int, list[int]] = {1: [org_x.id]} + resolved_empty = _resolve_org_unique_property(db_session, by_expense_empty) + assert resolved_empty[1] == [], ( + "0 active properties → empty property candidates (fail closed), " + f"got {resolved_empty[1]!r}" + ) + + # Now give org-x 2 active properties → also empty (ambiguous). + p_a = _create_property(db_session, org_x, name="Tower-A") + p_b = _create_property(db_session, org_x, name="Tower-B") + db_session.commit() + + by_expense_two: dict[int, list[int]] = {2: [org_x.id]} + resolved_two = _resolve_org_unique_property(db_session, by_expense_two) + # The helper lists ALL eligible property IDs under the org; the + # main upgrade loop then checks len(props) == 1 (exactly 1) — if not, + # FAIL CLOSED. So for >=2 properties we expect len(resolved) != 1, + # AND the returned values are bona-fide property IDs (not empty). + assert len(resolved_two[2]) >= 2, ( + f">=2 active properties → helper should list those property IDs " + f"(upgrade loop will fail closed on len != 1), got {resolved_two[2]!r}" + ) + assert sorted(resolved_two[2]) == sorted([p_a.id, p_b.id]), ( + "Property IDs must match the two we inserted (no extras)." + ) + + def test_helper_org_unique_property_exactly_one_picks_real_pid_not_org_id( + self, db_session, alice + ): + """CRITICAL namespace separation. Create an Organization with id=101 + and a single Property with id=202 (we insert with explicit PKs via + raw SQL when possible, otherwise we force the ordering by inserting + OTHER orgs/props first so auto-inc values diverge). Prove that + _resolve_org_unique_property returns [202] NOT [101].""" + m1c = self._load_m1c_module() + _resolve_org_unique_property = m1c._resolve_org_unique_property + from app.models.membership import ( + Membership, + MembershipState, + Organization, + OrganizationRole, + ) + from app.models.property import Property + from app.models.user import User, UserRole + + # --- Insert a USER we'll use to create explicit memberships --- + u = User( + username="fix2-namespace-u", + role=UserRole.admin, + api_key_hash="sha256$placeholder$" + "z" * 40, + ) + db_session.add(u) + db_session.flush() + + # --- Explicitly pick diverging org/property IDs --- + # + # PostgreSQL serials can't be cheaply overridden via ORM flush; + # instead, we INSERT A BUNCH of OTHER orgs (org_1..org_N) and then + # INSERT A BUNCH of OTHER unrelated properties (prop_1..prop_M) + # so the auto-assigned IDs drift apart. We then create the final + # "TARGET org" and immediately its ONE property, reading back + # their assigned IDs. The drift guarantees the IDs won't be the + # same numeric value, which would mask a regression silently. + drift_orgs = 17 + drift_props = 5 + for i in range(drift_orgs): + o = Organization(name=f"fix2-drift-org-{i}") + db_session.add(o) + db_session.flush() + # Give the first drift org a bunch of properties to create the + # property-id drift we need (the target org will then be created + # AFTER these; its id will be further along than the last + # drift-org id, but we'll create properties under drift-org[0] to + # get higher property ids than that target org id). + first_drift_org = ( + db_session.query(Organization) + .filter(Organization.name == "fix2-drift-org-0") + .one() + ) + for i in range(drift_props): + p = Property( + organization_id=first_drift_org.id, + name=f"fix2-drift-prop-{i}", + address=f"{i} Drift St", + city="Pasay", + total_units=1, + ) + db_session.add(p) + db_session.flush() + + # --- TARGET: single target org + single target property --- + target_org = Organization(name="fix2-namespace-target-org") + db_session.add(target_org) + db_session.flush() + # Bind `alice` (or our explicit user) as OWNER so if any downstream + # code re-checks memberships it won't blow up (not strictly needed + # by the helper but keeps the fixture holistic). + db_session.add( + Membership( + organization_id=target_org.id, + user_id=u.id, + role=OrganizationRole.OWNER, + state=MembershipState.ACTIVE, + ) + ) + target_prop = Property( + organization_id=target_org.id, + name="fix2-namespace-target-prop", + address="99 Unique Ave", + city="Pasay", + total_units=2, + ) + db_session.add(target_prop) + db_session.commit() + db_session.refresh(target_org) + db_session.refresh(target_prop) + + # --- THE ASSERTION THAT MATTERS: IDs diverge, no numeric accident --- + # If numeric IDs are accidentally equal, this test's whole premise is + # vacuous; force fail so author notices and increases drift counts. + assert target_org.id != target_prop.id, ( + "Test infrastructure error: target org.id accidentally equals " + f"target property.id ({target_org.id}). Increment drift_* and " + "re-run to guarantee namespace separation in this fixture." + ) + + # Now call the helper for expense_id=911, candidate org=[target_org.id]. + by_expense: dict[int, list[int]] = {911: [target_org.id]} + resolved = _resolve_org_unique_property(db_session, by_expense) + + # Critical contract: + # NOT the org-id → resolved[911] != [target_org.id] + # YES exactly 1 → len(resolved[911]) == 1 + # YES the real PID → resolved[911][0] == target_prop.id + assert resolved[911] != [target_org.id], ( + "NAMESPACE COLLAPSE DETECTED: helper returned org-id " + f"{target_org.id} as property-id. Blocker #1 still broken." + ) + assert len(resolved[911]) == 1, ( + f"Expected exactly 1 property candidate (unique-prop fallback), " + f"got {resolved[911]!r}" + ) + assert resolved[911][0] == target_prop.id, ( + f"Helper should return the only property.id={target_prop.id}, " + f"got {resolved[911][0]!r}" + ) diff --git a/tests/test_tenants.py b/tests/test_tenants.py index 4a6c759..1f498ae 100644 --- a/tests/test_tenants.py +++ b/tests/test_tenants.py @@ -1,36 +1,36 @@ -API = "/api/v1" - - -def test_create_tenant(client, admin_headers): - resp = client.post( - f"{API}/tenants", - json={"full_name": "Maria Santos", "phone": "+639171111111", "email": "maria@example.com"}, - headers=admin_headers, - ) - assert resp.status_code == 201 - assert resp.json()["full_name"] == "Maria Santos" - - -def test_list_and_get_tenant(client, admin_headers, tenant_id): - resp = client.get(f"{API}/tenants", headers=admin_headers) - assert resp.status_code == 200 - assert [t["id"] for t in resp.json()] == [tenant_id] - - resp = client.get(f"{API}/tenants/{tenant_id}", headers=admin_headers) - assert resp.status_code == 200 - assert resp.json()["full_name"] == "Juan Dela Cruz" - - -def test_update_tenant(client, admin_headers, tenant_id): - resp = client.patch( - f"{API}/tenants/{tenant_id}", json={"phone": "+639172222222"}, headers=admin_headers - ) - assert resp.status_code == 200 - assert resp.json()["phone"] == "+639172222222" - - -def test_delete_tenant(client, admin_headers, tenant_id): - resp = client.delete(f"{API}/tenants/{tenant_id}", headers=admin_headers) - assert resp.status_code == 200 - resp = client.get(f"{API}/tenants/{tenant_id}", headers=admin_headers) - assert resp.status_code == 404 +API = "/api/v1" + + +def test_create_tenant(client, admin_headers, org_a): + resp = client.post( + f"{API}/tenants", + json={"full_name": "Maria Santos", "phone": "+639171111111", "email": "maria@example.com", "organization_id": org_a.id}, + headers=admin_headers, + ) + assert resp.status_code == 201 + assert resp.json()["full_name"] == "Maria Santos" + + +def test_list_and_get_tenant(client, admin_headers, tenant_id): + resp = client.get(f"{API}/tenants", headers=admin_headers) + assert resp.status_code == 200 + assert [t["id"] for t in resp.json()] == [tenant_id] + + resp = client.get(f"{API}/tenants/{tenant_id}", headers=admin_headers) + assert resp.status_code == 200 + assert resp.json()["full_name"] == "Juan Dela Cruz" + + +def test_update_tenant(client, admin_headers, tenant_id): + resp = client.patch( + f"{API}/tenants/{tenant_id}", json={"phone": "+639172222222"}, headers=admin_headers + ) + assert resp.status_code == 200 + assert resp.json()["phone"] == "+639172222222" + + +def test_delete_tenant(client, admin_headers, tenant_id): + resp = client.delete(f"{API}/tenants/{tenant_id}", headers=admin_headers) + assert resp.status_code == 200 + resp = client.get(f"{API}/tenants/{tenant_id}", headers=admin_headers) + assert resp.status_code == 404