fix(consolidation): ревью Task 4 — общий lock_out, точнее тесты, решения #17–#24 в design.md
- I-1 (T4.1): lock_out — публичный в services/consolidations.py, копия в consolidation_locks удалена - M-b (T4.3): удалён осиротевший комментарий в tests/factories.py - M-c (T4.4): точная проверка ip_address/user_agent в аудите смены статуса - M-d (T4.5): docstring теста Review Focus 4 ссылается на различающий тест порядка D-9 - design.md: решения без обсуждения #17 (R-8), #18 (R-9), #19–#24 (решения Task 4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vJkmBGKKqMoEMzvsXG3y2
This commit is contained in:
Andrey Epifancev
parent
b2fa59061d
commit
e317fe59dd
@ -25,7 +25,7 @@ from src.models.enums import AuditAction, AuditSection, ConsolidationStatus
|
||||
from src.repositories import consolidations as repo
|
||||
from src.repositories.users import UserWithAccess
|
||||
from src.schemas.consolidations import LockIn, LockOut
|
||||
from src.services.consolidations import OBJECT_TYPE, VERSION_NOT_FOUND
|
||||
from src.services.consolidations import OBJECT_TYPE, VERSION_NOT_FOUND, lock_out
|
||||
|
||||
ACTIVE_LOCK_INDEX = "uq_consolidation_lock_version_active"
|
||||
|
||||
@ -51,16 +51,6 @@ async def _user_name(session: AsyncSession, user_id: uuid.UUID) -> str:
|
||||
return user.full_name if user is not None else ""
|
||||
|
||||
|
||||
def _lock_out(lock: ConsolidationLock, locked_by_name: str) -> LockOut:
|
||||
return LockOut(
|
||||
locked_by=lock.locked_by,
|
||||
locked_by_name=locked_by_name,
|
||||
locked_at=lock.locked_at,
|
||||
taken_over_from=lock.taken_over_from,
|
||||
takeover_reason=lock.takeover_reason,
|
||||
)
|
||||
|
||||
|
||||
class ConsolidationLockService:
|
||||
"""Взятие версии в работу от имени текущего пользователя в его области видимости."""
|
||||
|
||||
@ -86,7 +76,7 @@ class ConsolidationLockService:
|
||||
user = self._principal.user
|
||||
current = await repo.active_lock(self._session, version.id)
|
||||
if current is not None and current.locked_by == user.id:
|
||||
return _lock_out(current, user.full_name), False
|
||||
return lock_out(current, user.full_name), False
|
||||
reason = (data.takeover_reason or "").strip() or None
|
||||
if current is not None and reason is None:
|
||||
raise await self._locked_by_other(current)
|
||||
@ -102,7 +92,7 @@ class ConsolidationLockService:
|
||||
consolidation_version_id=version.id,
|
||||
after_value={"locked_by": user.id, "released": False},
|
||||
)
|
||||
return _lock_out(lock, user.full_name), True
|
||||
return lock_out(lock, user.full_name), True
|
||||
|
||||
previous_owner = current.locked_by
|
||||
await release_active_lock(self._session, version.id, now)
|
||||
@ -117,7 +107,7 @@ class ConsolidationLockService:
|
||||
after_value={"locked_by": user.id},
|
||||
comment=reason,
|
||||
)
|
||||
return _lock_out(lock, user.full_name), True
|
||||
return lock_out(lock, user.full_name), True
|
||||
|
||||
async def release(self, version_id: uuid.UUID) -> None:
|
||||
"""Снять свою активную блокировку (аудит `CONSOLIDATION_LOCK`, `released = true`); чужая — 409
|
||||
|
||||
@ -58,7 +58,8 @@ MIN_PERIMETER = 2 # в периметре консолидации не мен
|
||||
FIRST_VERSION_NO = 1
|
||||
|
||||
|
||||
def _lock_out(lock: ConsolidationLock, locked_by_name: str) -> LockOut:
|
||||
def lock_out(lock: ConsolidationLock, locked_by_name: str) -> LockOut:
|
||||
"""Активная блокировка в ответе API (список, карточка, `POST …/lock`)."""
|
||||
return LockOut(
|
||||
locked_by=lock.locked_by,
|
||||
locked_by_name=locked_by_name,
|
||||
@ -211,7 +212,7 @@ class ConsolidationService:
|
||||
status=version.status,
|
||||
scenario=version.scenario,
|
||||
approach=version.approach,
|
||||
lock=_lock_out(*lock) if lock is not None else None,
|
||||
lock=lock_out(*lock) if lock is not None else None,
|
||||
created_at=version.created_at,
|
||||
members_count=members_count,
|
||||
)
|
||||
|
||||
@ -76,7 +76,6 @@ TABULAR_FILE_TYPES = frozenset({FileType.XLS, FileType.XLSX, FileType.XLSB})
|
||||
FACTORY_BUCKET = "arm-ka"
|
||||
|
||||
|
||||
# статусы вне «в работе» (как фильтр `in_work_only` спеки): `is_in_work = false` ставится только им и аннулированию
|
||||
def _now() -> datetime.datetime:
|
||||
return datetime.datetime.now(datetime.UTC)
|
||||
|
||||
|
||||
@ -384,7 +384,8 @@ async def test_takeover_waits_for_owner_edit_then_owner_loses_lock(
|
||||
"""Review Focus 4: владелец А правит периметр (`consolidation_version FOR UPDATE`), Б в это время перехватывает.
|
||||
|
||||
Перехват ждёт транзакцию А; после её commit правка А зафиксирована, Б — владелец, следующая правка А —
|
||||
409 `LOCKED_BY_OTHER`.
|
||||
409 `LOCKED_BY_OTHER`. Этот сценарий ждал бы и без `FOR UPDATE` версии в `take` (вставка блокировки берёт FK
|
||||
`KEY SHARE` на версию); порядок D-9 различает `test_takeover_during_cancellation_sees_cancelled_version`.
|
||||
"""
|
||||
async with committed_sessionmaker() as setup, setup.begin():
|
||||
version = await _version(setup)
|
||||
|
||||
@ -118,7 +118,11 @@ async def test_cancel_by_lock_owner(auth_client: AuthClientFactory, seeded: Asyn
|
||||
user = await _user(seeded, KA_RF)
|
||||
client = await auth_client(KA_RF)
|
||||
|
||||
response = await client.post(_status_url(version.id), json={"to_status": "CANCELLED", "comment": "Дубль"})
|
||||
response = await client.post(
|
||||
_status_url(version.id),
|
||||
json={"to_status": "CANCELLED", "comment": "Дубль"},
|
||||
headers={"User-Agent": "pytest-status"},
|
||||
)
|
||||
|
||||
assert response.status_code == HTTP_OK, response.text
|
||||
card = response.json()
|
||||
@ -155,7 +159,8 @@ async def test_cancel_by_lock_owner(auth_client: AuthClientFactory, seeded: Asyn
|
||||
{"status": "CANCELLED"},
|
||||
"Дубль",
|
||||
)
|
||||
assert event.ip_address is not None or event.user_agent is not None # контекст запроса передан в аудит
|
||||
# контекст запроса передан в аудит (ASGITransport отдаёт клиента 127.0.0.1)
|
||||
assert (event.ip_address, event.user_agent) == ("127.0.0.1", "pytest-status")
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
|
||||
@ -201,3 +201,11 @@ legal_entity_name, account_code, missing_quarters[]}}`. Фильтры `GET …/
|
||||
| 14 | 2026-10-06 | R-4: `Resolution(entity_id, kind)` определяется в `src/domain/osv_mapping.py`; `repositories/counterparties.py` импортирует и реэкспортирует | `domain/consolidated_osv.py` (mypy strict, без ORM/репозиториев) нуждается в нём в `Resolver`, домен не импортирует репозитории | нулевая (перенос определения) |
|
||||
| 15 | 2026-10-06 | R-5: Task 2 создаёт `domain/checklist.py` с `EntityFilesState(has_in_progress, last_file_status)` и `entity_files_state(session, entity_ids)`; `unconfirmed_documents(...)` и `build` добавляет Task 8 | план описывал `entity_files_state` и в Task 2, и в Task 8; `build` получает и `files_state`, и `unbound` | нулевая (разнесение по задачам) |
|
||||
| 16 | 2026-10-06 | `osv_entry.counterparty_match_kind` — последняя колонка таблицы (в ORM и DBML), а не сразу после `counterparty_id` | `ADD COLUMN` в миграции 0004 добавляет колонку в конец, а `test_migrated_schema_equals_orm_create_all` сравнивает `ordinal_position`; пересоздание таблицы с 200 тыс. строк ради порядка колонок не оправдано | косметика: порядок колонок в `SELECT *`/диаграмме; на поведение не влияет |
|
||||
| 17 | 2026-10-06 | R-8: коды 404 `CONSOLIDATION_NOT_FOUND` / `VERSION_NOT_FOUND` реализованы как текст сообщения при фиксированном коде `NOT_FOUND` | конвенция Срезов 1–2c: `NotFoundError` с фиксированным кодом, как `COMPANY_GROUP_NOT_FOUND` | если нужны отдельные коды — меняется конструктор ошибки и тесты |
|
||||
| 18 | 2026-10-06 | R-9: `in_work_only` убирает период без оставшихся версий; невидимая/несуществующая ГК в списке → 404; неизвестная/невидимая ГСК → 404 (сообщение `RELATED_GROUP_NOT_FOUND`), видимая ГСК чужой ГК или `kind = gsz` → 422 `RELATED_GROUP_MISMATCH` | естественное продолжение policy «невидимое = несуществующее» (404); несоответствие видимой ГСК — ошибка запроса | расхождение с ожиданием фронта по пустому списку |
|
||||
| 19 | 2026-10-06 | `takeover_reason` пустой или из одних пробелов считается отсутствующим: при чужой блокировке — 409 `LOCKED_BY_OTHER`, а не 422; при свободной версии причина не сохраняется (`taken_over_from` NULL, обычное взятие 201) | спека требует «непустую» причину только для перехвата; при свободной версии перехватывать не у кого | фронт не получит 422 на пустую причину; в БД нет «причины» у обычного взятия |
|
||||
| 20 | 2026-10-06 | Комментарий `POST …/status` обрезается по краям; из одних пробелов — пустой (`COMMENT_REQUIRED` для `CANCELLED`); в `cancelled_reason` и историю пишется обрезанный текст | «пустой комментарий» по спеке включает пробельный; обрезка не меняет смысла | косметика: ведущие/хвостовые пробелы комментария теряются |
|
||||
| 21 | 2026-10-06 | `NOT_LOCK_OWNER` отдаёт в `detail` `locked_by` и `locked_at` владельца (по аналогии с `LOCKED_BY_OTHER`) | спека полей не перечисляет; фронту нужно показать, у кого блокировка | нулевая (лишние поля ответа) |
|
||||
| 22 | 2026-10-06 | Перехват пишет одно событие `CONSOLIDATION_LOCK_TAKEOVER` без отдельного `CONSOLIDATION_LOCK` на снятие прежней блокировки | сценарий audit-trail «Перехват» — «одно событие» | в журнале нет отдельной строки «блокировка А снята» при перехвате (она следует из `before_value`) |
|
||||
| 23 | 2026-10-06 | `LOCKED_BY_OTHER` из `ensure_version_editable` (`PUT …/members`, `POST …/status`, `POST …/osv/entries/{id}/counterparty`) отдаёт `locked_by`, `locked_at` без `locked_by_name`; `POST …/lock` отдаёт и имя | доменная функция без БД имени не знает; directory-policy требует только `detail.locked_by`, имя спека требует лишь у `POST …/lock` | фронт для правок версии берёт имя владельца из карточки (`lock.locked_by_name`) или вызывающий сервис дополнит ответ |
|
||||
| 24 | 2026-10-06 | `CANCELLED` снимает активную блокировку без отдельного события `CONSOLIDATION_LOCK` — снятие покрывает `CONSOLIDATION_STATUS_CHANGE`; `uq_consolidation_lock_version_active` зарегистрирован в `UNIQUE_CONSTRAINT_CONFLICTS` как `LOCKED_BY_OTHER` (страховка для вставки в обход сервиса; сервис сам ловит нарушение под SAVEPOINT и добавляет владельца) | таблица audit-trail относит `CONSOLIDATION_LOCK` к действиям «взята в работу, снята» пользователем, а не к побочному эффекту аннулирования; реестр — конвенция гонок 2c | в журнале нет отдельной строки «блокировка снята» при аннулировании; реестр — нулевая |
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user