From e317fe59dd9f6e3f6a5998b225bea581cf82abe4 Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Tue, 6 Oct 2026 04:26:31 +0400 Subject: [PATCH] =?UTF-8?q?fix(consolidation):=20=D1=80=D0=B5=D0=B2=D1=8C?= =?UTF-8?q?=D1=8E=20Task=204=20=E2=80=94=20=D0=BE=D0=B1=D1=89=D0=B8=D0=B9?= =?UTF-8?q?=20lock=5Fout,=20=D1=82=D0=BE=D1=87=D0=BD=D0=B5=D0=B5=20=D1=82?= =?UTF-8?q?=D0=B5=D1=81=D1=82=D1=8B,=20=D1=80=D0=B5=D1=88=D0=B5=D0=BD?= =?UTF-8?q?=D0=B8=D1=8F=20#17=E2=80=93#24=20=D0=B2=20design.md?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Claude-Session: https://claude.ai/code/session_012vJkmBGKKqMoEMzvsXG3y2 --- api/src/services/consolidation_locks.py | 18 ++++-------------- api/src/services/consolidations.py | 5 +++-- api/tests/factories.py | 1 - api/tests/test_api_consolidation_locks.py | 3 ++- api/tests/test_api_consolidation_status.py | 9 +++++++-- .../changes/slice-4-consolidation/design.md | 8 ++++++++ 6 files changed, 24 insertions(+), 20 deletions(-) diff --git a/api/src/services/consolidation_locks.py b/api/src/services/consolidation_locks.py index c834407..5465be1 100644 --- a/api/src/services/consolidation_locks.py +++ b/api/src/services/consolidation_locks.py @@ -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 diff --git a/api/src/services/consolidations.py b/api/src/services/consolidations.py index 759f99e..7e022cd 100644 --- a/api/src/services/consolidations.py +++ b/api/src/services/consolidations.py @@ -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, ) diff --git a/api/tests/factories.py b/api/tests/factories.py index 492a925..0fc5f18 100644 --- a/api/tests/factories.py +++ b/api/tests/factories.py @@ -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) diff --git a/api/tests/test_api_consolidation_locks.py b/api/tests/test_api_consolidation_locks.py index 8e54b93..87a27ab 100644 --- a/api/tests/test_api_consolidation_locks.py +++ b/api/tests/test_api_consolidation_locks.py @@ -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) diff --git a/api/tests/test_api_consolidation_status.py b/api/tests/test_api_consolidation_status.py index 028841b..8753ff0 100644 --- a/api/tests/test_api_consolidation_status.py +++ b/api/tests/test_api_consolidation_status.py @@ -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 diff --git a/openspec/changes/slice-4-consolidation/design.md b/openspec/changes/slice-4-consolidation/design.md index 52acf20..985ce6f 100644 --- a/openspec/changes/slice-4-consolidation/design.md +++ b/openspec/changes/slice-4-consolidation/design.md @@ -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 | в журнале нет отдельной строки «блокировка снята» при аннулировании; реестр — нулевая |