tradein-mvp/backend/app/api/v1/team.py

@@ -205,10 +205,25 @@ def _batch_quota_status(db: Session, usernames: list[str]) -> dict[str, dict[str
     """Батч-версия `account_quota.get_status` для N сотрудников — 2 SQL-запроса
     вместо 2N (было 2N+3 на GET /employees, HIGH/Medium2 review PR #2563).
 
-    Семантика ИДЕНТИЧНА `account_quota.is_unlimited`/`user_limit`/`get_status`:
-    unlimited = admin-роль (roles.yaml, in-memory, без похода в БД) ИЛИ
-    `account_quota_overrides.unlimited=true`; limit = override.monthly_limit,
-    иначе глобальный `account_quota.MONTHLY_LIMIT`.
+    Семантика ИДЕНТИЧНА `account_quota.is_unlimited`/`user_limit`/`get_status`
+    (follow-up review PR #2563 п.2 — предыдущая версия расходилась: батч ВСЕГДА
+    читал `account_quota_overrides.unlimited`, а `is_unlimited` — ТОЛЬКО для
+    username, присутствующего в roles.yaml):
+      - username НЕ в roles.yaml (`get_role` → KeyError) → unlimited=False ВСЕГДА,
+        `account_quota_overrides.unlimited` даже не проверяется (roles.yaml —
+        источник правды "кто вообще может быть unlimited", override — "у кого
+        именно из известных roles.yaml-юзеров"). Сегодня недостижимо для DB-only
+        сотрудников team-API (`_upsert_quota_override` всегда пишет
+        `unlimited=false`), но станет достижимым при ручном UPDATE
+        `account_quota_overrides` или расширении roles.yaml — расхождение с
+        реальным enforcement (`check_and_raise`/`increment`, тот же `is_unlimited`)
+        было бы честной ложью в списке: "без лимита", который движок всё равно
+        считает.
+      - username в roles.yaml и role == admin → unlimited=True (без похода в БД).
+      - username в roles.yaml, role != admin → unlimited = override.unlimited.
+    limit = override.monthly_limit (читается для ЛЮБОГО username, без gate по
+    roles.yaml — так же ведёт себя `account_quota.user_limit`), иначе глобальный
+    `account_quota.MONTHLY_LIMIT`.
     """
     if not usernames:
         return {}
@@ -250,10 +265,17 @@ def _batch_quota_status(db: Session, usernames: list[str]) -> dict[str, dict[str
     for username in usernames:
         override = override_by_username.get(username)
         try:
-            is_admin_role = get_role(username) == "admin"
+            role = get_role(username)
         except KeyError:
-            is_admin_role = False
-        unlimited = is_admin_role or bool(override is not None and override["unlimited"])
+            role = None
+        if role == "admin":
+            unlimited = True
+        elif role is not None:
+            unlimited = bool(override is not None and override["unlimited"])
+        else:
+            # username не в roles.yaml — is_unlimited() короткое замыкание на
+            # False, override НЕ проверяется (см. докстринг выше).
+            unlimited = False
         limit = (
             int(override["monthly_limit"])
             if override is not None and override["monthly_limit"] is not None
@@ -521,12 +543,21 @@ async def update_employee(
 # Два статических варианта WHERE (НЕ f-string/динамическая сборка — Medium/
 # "заодно" review PR #2563: значения биндятся параметрами и без того безопасны,
 # но статические ветки не провоцируют будущие правки в сторону конкатенации SQL).
+#
+# ORDER BY created_at DESC, id DESC — тай-брейкер по `id` ОБЯЗАТЕЛЕН (follow-up
+# review PR #2563 п.1): `created_at DEFAULT now()` — время ТРАНЗАКЦИИ, а bulk-seed
+# (#2557) вставляет много юзеров одной транзакцией → идентичный timestamp у N строк.
+# Без тай-брейкера порядок между страницами (LIMIT/OFFSET) на PostgreSQL для
+# строк-«близнецов» не гарантирован — сотрудники пропадали/дублировались бы при
+# постраничном листании. `id` монотонно растёт (BIGINT IDENTITY) — детерминированный
+# tie-break без доп. индекса (созданные позже = бОльший id, тот же порядок что и
+# намерение DESC-сортировки по времени).
 _LIST_EMPLOYEES_BY_MANAGER_SQL = text(
     """
     SELECT id, username, display_name, org_name, email, is_active, manager_id, created_at
     FROM tradein_users
     WHERE role = 'employee' AND manager_id = :manager_id
-    ORDER BY created_at DESC
+    ORDER BY created_at DESC, id DESC
     LIMIT :limit OFFSET :offset
     """
 )
@@ -536,7 +567,7 @@ _LIST_EMPLOYEES_ALL_SQL = text(
     SELECT id, username, display_name, org_name, email, is_active, manager_id, created_at
     FROM tradein_users
     WHERE role = 'employee'
-    ORDER BY created_at DESC
+    ORDER BY created_at DESC, id DESC
     LIMIT :limit OFFSET :offset
     """
 )

tradein-mvp/backend/tests/test_team_api.py

@@ -61,6 +61,7 @@ class _Store:
         display_name: str | None = None,
         org_name: str | None = None,
         email: str | None = None,
+        created_at: datetime | None = None,
     ) -> int:
         uid = self._next_id
         self._next_id += 1
@@ -74,7 +75,7 @@ class _Store:
             "org_name": org_name,
             "email": email,
             "is_active": is_active,
-            "created_at": datetime.now(UTC),
+            "created_at": created_at or datetime.now(UTC),
         }
         return uid
 
@@ -259,7 +260,10 @@ class _FakeDB:
             rows = [u for u in s.users.values() if u["role"] == "employee"]
             if "manager_id" in p:
                 rows = [u for u in rows if u["manager_id"] == p["manager_id"]]
-            rows = sorted(rows, key=lambda u: u["created_at"], reverse=True)
+            # Mirrors real SQL `ORDER BY created_at DESC, id DESC` — `id` tiebreak
+            # is REQUIRED for deterministic paging when created_at ties (follow-up
+            # review PR #2563 п.1, bulk-seed #2557 inserts many rows in one tx).
+            rows = sorted(rows, key=lambda u: (u["created_at"], u["id"]), reverse=True)
             offset, limit = p.get("offset", 0), p.get("limit", len(rows))
             rows = rows[offset : offset + limit]
             return _Result(
@@ -787,6 +791,51 @@ def test_list_employees_limit_max_200(client: TestClient, store: _Store) -> None
     assert resp.status_code == 422
 
 
+def test_list_employees_pagination_stable_with_identical_created_at(
+    client: TestClient, store: _Store
+) -> None:
+    """Follow-up review PR #2563 п.1: `created_at DEFAULT now()` — время ТРАНЗАКЦИИ,
+    bulk-seed (#2557) вставляет много юзеров одной транзакцией → идентичный
+    timestamp у N+ строк. Без `id DESC` тай-брейкера порядок между страницами
+    на PostgreSQL для строк-«близнецов» не гарантирован — сотрудники пропадали/
+    дублировались бы при постраничном листании. Вставляем 5 сотрудников с
+    ОДИНАКОВЫМ created_at, листаем limit=2 постранично — объединение страниц
+    обязано дать полный набор без дублей и пропусков."""
+    store.add_user("admin1", hash_password("Secret123!"), role="admin")
+    mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager")
+    same_ts = datetime.now(UTC)
+    expected_usernames = set()
+    for i in range(5):
+        username = f"emp_tie_{i}"
+        store.add_user(
+            username,
+            hash_password("Secret123!"),
+            role="employee",
+            manager_id=mgr_id,
+            created_at=same_ts,
+        )
+        expected_usernames.add(username)
+
+    _login(client, "admin1", "Secret123!")
+
+    seen: list[str] = []
+    offset = 0
+    while True:
+        resp = client.get("/api/v1/team/employees", params={"limit": 2, "offset": offset})
+        assert resp.status_code == 200, resp.text
+        page = [e["username"] for e in resp.json()]
+        if not page:
+            break
+        seen.extend(page)
+        offset += 2
+
+    assert len(seen) == len(expected_usernames), (
+        f"page union has {len(seen)} entries (dupes or gaps), expected "
+        f"{len(expected_usernames)}: {seen}"
+    )
+    assert set(seen) == expected_usernames
+
+
 # ---------------------------------------------------------------------------
 # CSRF defense-in-depth — Origin/Referer check on state-changing team routes
 # ---------------------------------------------------------------------------
@@ -846,6 +895,65 @@ def test_patch_employee_origin_mismatch_403(client: TestClient, store: _Store) -
     assert store.users["emp_a"]["display_name"] != "hacked"
 
 
+# ---------------------------------------------------------------------------
+# _batch_quota_status unlimited semantics — must match account_quota.is_unlimited
+# ---------------------------------------------------------------------------
+
+
+def test_batch_quota_unlimited_ignored_for_non_roles_yaml_username(
+    client: TestClient, store: _Store
+) -> None:
+    """Follow-up review PR #2563 п.2: `account_quota.is_unlimited` short-circuits
+    to False for a username NOT in roles.yaml — it never even reads
+    `account_quota_overrides.unlimited`. The batch quota status used by
+    GET /employees must agree, or the list would show "unlimited" for a quota
+    that real enforcement (check_and_raise/increment, same is_unlimited) does
+    NOT honor — a misleading display. `emp_ghost_unlimited` is a fresh DB-only
+    username guaranteed absent from roles.yaml."""
+    store.add_user("admin1", hash_password("Secret123!"), role="admin")
+    mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager")
+    store.add_user(
+        "emp_ghost_unlimited", hash_password("Secret123!"), role="employee", manager_id=mgr_id
+    )
+    store.quota_overrides["emp_ghost_unlimited"] = {
+        "monthly_limit": 15,
+        "unlimited": True,
+        "note": "manual grant via SQL runbook (not through team-api)",
+    }
+
+    _login(client, "admin1", "Secret123!")
+    resp = client.get("/api/v1/team/employees")
+    assert resp.status_code == 200, resp.text
+    entry = next(e for e in resp.json() if e["username"] == "emp_ghost_unlimited")
+    # DB override says unlimited=true, but username is NOT in roles.yaml — real
+    # enforcement would never see it, so the list must NOT claim "unlimited".
+    assert entry["quota"]["unlimited"] is False
+    assert entry["quota"]["limit"] == 15
+
+
+def test_batch_quota_unlimited_honored_for_roles_yaml_username(
+    client: TestClient, store: _Store
+) -> None:
+    """Symmetric positive case: a username actually present in roles.yaml
+    (non-admin role) — `account_quota_overrides.unlimited=true` IS honored, same
+    as `account_quota.is_unlimited`. Uses `kopylov` — real prod pilot-role entry
+    in auth/roles.yaml (see app/core/auth.py module docstring)."""
+    store.add_user("admin1", hash_password("Secret123!"), role="admin")
+    mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager")
+    store.add_user("kopylov", hash_password("Secret123!"), role="employee", manager_id=mgr_id)
+    store.quota_overrides["kopylov"] = {
+        "monthly_limit": 999,
+        "unlimited": True,
+        "note": "existing prod grant",
+    }
+
+    _login(client, "admin1", "Secret123!")
+    resp = client.get("/api/v1/team/employees")
+    assert resp.status_code == 200, resp.text
+    entry = next(e for e in resp.json() if e["username"] == "kopylov")
+    assert entry["quota"]["unlimited"] is True
+
+
 # ---------------------------------------------------------------------------
 # GET /employees/{id}/history
 # ---------------------------------------------------------------------------