From 716bff31268ad310b1bafcc915561e6b94309be4 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Wed, 3 Jun 2026 22:53:53 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=67 --- docs/work-items/ET-011/12-review.md | 250 ++++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) create mode 100644 docs/work-items/ET-011/12-review.md diff --git a/docs/work-items/ET-011/12-review.md b/docs/work-items/ET-011/12-review.md new file mode 100644 index 0000000..612d4c6 --- /dev/null +++ b/docs/work-items/ET-011/12-review.md @@ -0,0 +1,250 @@ +--- +type: review +work_item_id: ET-011 +verdict: REQUEST_CHANGES +version: 1 +--- + +# Review ET-011 — GPX-download из popup публичного трека + +**Branch:** `feature/ET-011-popup-enduro-trails` +**HEAD:** `7d8407a` (fix(ci): hoist imports to satisfy E402 + declare runtime/test deps in pyproject) +**Build commit:** `eea6c84` (feat(gps-tracks): GPX download from public track popup) +**Reviewer:** agent:reviewer +**Дата:** 2026-06-03 + +--- + +## Сводка + +PR полностью реализует backend (`/api/gps-tracks/{track_id}/download`, +`export.py`) и frontend (кнопка в popup, `_downloadPublicTrack`, +обработчик ошибок), описанные в ADR-014 и ADR-015. Тесты UT-01..05, +IT-01..08 написаны и проходят; ничего из старого функционала не +сломано (89/89 API-тестов, 24/24 JS-тестов). + +Главное замечание — **отсутствует артефакт `tests/web/test_track_download.spec.ts`**, +явно требуемый ADR-014 §4 и test-plan §E2E-01..04. Это оставляет +без автоматизированного покрытия AC-1 / AC-2 / AC-13 (по +`coverage_matrix` они покрываются исключительно E2E-тестами). + +--- + +## Что проверено + +| Срез | Результат | +|---|---| +| `02-trz.md` REQ-F-01..F-07, REQ-NF-01..NF-07 | см. таблицу ниже | +| `03-acceptance-criteria.md` AC-1..AC-15 | см. таблицу AC ниже | +| `06-adr/ADR-014` / `ADR-015` | соответствует решениям A2/B1/C1/D1/E1/F/G/H/I/J и A2/B1/C1/D/F/G | +| Линт (`ruff check`) | новые файлы — clean; pre-existing F401/E741 в `tests/unit/test_base_layer.py` и `tests/api/test_gps_tracks_sources_osm.py` — не относятся к ET-011 | +| Тесты API (`pytest tests/api`) | **89/89 PASS** | +| JS-тесты (`node --test tests/web/gps_tracks.test.js`) | **24/24 PASS** | +| Документация архитектуры | обновлены `docs/architecture/README.md` и `docs/architecture/adr/README.md` (ADR-014/015 в индексе) | +| Конфиг `config/gps_sources.yaml` | расширен `download_allowed` для всех 4 источников; OSM=true, остальные=false (соответствует ADR-015 §D) | + +--- + +## Findings + +### P1 (must-fix) + +**P1-01. Отсутствует Playwright-спецификация `tests/web/test_track_download.spec.ts`** + +`docs/work-items/ET-011/04-test-plan.yaml` тесты E2E-01..E2E-04 явно +размещают в файле `tests/web/test_track_download.spec.ts` (≈ 200 строк +согласно ADR-014 §4 «Конвенция размещения нового кода»). Файл в PR +отсутствует: + +``` +$ ls tests/web/test_track_download.spec.ts → MISSING +``` + +`coverage_matrix` test-plan показывает, что эти E2E-кейсы — единственное +автоматизированное покрытие следующих AC: + +| AC | Покрытие по plan'у | Реализовано | +|---|---|---| +| AC-1 (кнопка в popup, REQ-F-01) | E2E-01, E2E-02 | **нет** | +| AC-2 (клик → файл, REQ-F-02/F-03/F-05) | E2E-01 | частично (IT-01 покрывает только HTTP-сторону) | +| AC-13 (mobile UX, REQ-NF-04) | E2E-02 | **нет** | +| AC-7 (toast «не найден») | IT-02, **E2E-03** | только серверная часть (IT-02), UI-обработка `_handleDownloadError` без теста | + +UI-кейсы в `04b-ui-test-cases.md` — ручные/визуальные, не заменяют +автоматизацию. + +→ Требуется создать `tests/web/test_track_download.spec.ts` минимум +для E2E-01 (happy-path download) и E2E-02 (mobile viewport visible +button). E2E-03 (mock 404 → toast) можно оформить через +`page.route(...)`. E2E-04 опционален (gated `enabled_if`). + +Если установка Playwright в CI блокирована — допустимая альтернатива: +тесты-юниты на JS (Node `--test`) на функции +`_parseFilenameFromCD`, `_handleDownloadError` и проверка структуры +HTML, возвращаемой `_renderTrackPopupHtml` (как кнопка добавлена, +aria-label корректен). Это закроет AC-1 / AC-7 (UI-сторона), но +AC-13 (mobile bbox) остаётся без покрытия — пометить как ручную +проверку в `04b` (manual smoke на release). + +### P2 (should-fix) + +**P2-01. Контракт ответа 403 расходится с ADR-015 §G** + +ADR-015 §G документирует формат: +```json +{ "detail": "source_forbidden", "external_urls": [...] } +``` + +Фактически (`endpoint.py:384`): +```python +raise HTTPException( + status_code=403, + detail={"detail": "source_forbidden", "external_urls": external_urls}, +) +``` + +FastAPI оборачивает это в: +```json +{ "detail": { "detail": "source_forbidden", "external_urls": [...] } } +``` + +`tests/api/test_gps_tracks_download.py::test_it05_source_forbidden_403` +явно делает `body.get("detail", body)` → `detail.get("external_urls")`, +т.е. подтверждает удвоенную вложенность. На frontend +`_handleDownloadError` тоже учитывает обе формы. По факту контракт +работает, но **расходится с документированным в ADR-015 §G**. + +→ Либо обновить ADR-015 §G и AC-11 на реальный формат +`{"detail":{"detail":"source_forbidden","external_urls":[...]}}`, +либо использовать `JSONResponse(status_code=403, content={...})` +напрямую, чтобы получить однослойную форму. Не блокер, но создаёт +расхождение «doc vs runtime» для будущих интеграторов. + +**P2-02. `_handleDownloadError` для 400 не учтён в REQ-F-05** + +REQ-F-05.4 описывает обработку 403 / 404 / 5xx. Frontend дополнительно +обрабатывает 400 («Неподдерживаемый формат файла»), что само по себе +полезно. Но в UI-флоу 400 невозможен (кнопка всегда дёргает дефолтный +`?format=gpx`), а в обычной popup-кнопке нет UI для выбора формата. +→ Косметика; можно оставить как defensive. + +### P3 (nice-to-have) + +**P3-01.** Логирование через `logging.getLogger("uvicorn.access")` +сольёт `track_download id=… size_bytes=…` в access-log формате (INFO). +Для production-фильтрации удобнее отдельный logger +(`logger = logging.getLogger("enduro_trails.gps_tracks")`); ADR-014 §J +рекомендует именно access-logger, так что формальных претензий нет. + +**P3-02.** В `build_gpx` связь `external_urls[i] ↔ sources[i]` по +индексу хрупкая. Если списки в БД разной длины (что возможно при +dedup-merge), label для `` будет вырожденно `sources[0]`. +Документация (ADR-014 §G) этот edge-case не уточняет, тестов нет — +явно не блокер, но стоит зафиксировать поведение тестом, чтобы при +будущих рефакторингах не сломать молча. + +**P3-03.** `_renderTrackPopupHtml` использует строковую интерполяцию +для `${name}`, `${user}`, `${url}` (источники). Это пре-existing +ET-008 поведение, **не введено в ET-011** (новый блок `actionsHtml` +содержит только числовой `trackId`, валидированный `Number.isFinite`). +Out-of-scope, но отмечаю для будущей safety-итерации. + +--- + +## REQ ↔ реализация + +| REQ | Реализация | Статус | +|---|---|---| +| REQ-F-01 (кнопка в popup) | `gps_tracks.js:498–509` — `actionsHtml`, aria-label «Скачать GPX», 32×32 CSS px из `app.css:1311–1338` | ✓ | +| REQ-F-02 (endpoint, статусы) | `endpoint.py:332–435` — порядок 400 → 404 → 413 → 403 → 200 как в ADR-014 §H | ✓ | +| REQ-F-03 (GPX 1.1) | `export.py::build_gpx` + UT-01..03 (XSD-валидация в `tests/fixtures/gpx-1.1/gpx.xsd`) | ✓ | +| REQ-F-04 (имя файла) | `export.py::safe_filename` + UT-04 (10 кейсов) | ✓ | +| REQ-F-05 (UX клика) | `_downloadPublicTrack`, `_parseFilenameFromCD`, `_handleDownloadError` | ✓ автоматического E2E-теста нет (см. P1-01) | +| REQ-F-06 (license 403) | `endpoint.py:382–390` + `config.py::load_download_allowed_sources` + IT-05 | ✓ (контракт см. P2-01) | +| REQ-F-07 (логирование) | `endpoint.py:419–424` через `uvicorn.access` | ✓ | +| REQ-NF-01 (perf 300 ms p95) | не измерено в авто-тестах (AC-12 = manual perf check); IT-01 проходит за < 1 сек на 10 точек | ✓ (manual) | +| REQ-NF-02 (cap 200k → 413) | `MAX_POINTS_FOR_DOWNLOAD = 200_000` + IT-04 | ✓ | +| REQ-NF-03 (XSD валидация) | UT-03 + IT-07 (XSD 30 КБ закоммичен в fixtures) | ✓ | +| REQ-NF-04 (mobile UX) | CSS 32×32, popup `maxWidth:'300px'`; **AC-13 без авто-проверки** (см. P1-01) | частично | +| REQ-NF-05 (RFC 5987) | IT-06 проверяет `filename*=UTF-8''` и ASCII-fallback | ✓ | +| REQ-NF-06 (наблюдаемость) | uvicorn access + logger.info | ✓ | +| REQ-NF-07 (безопасность) | `Path(..., ge=1)` для int-id; `safe_filename` чистит ФС-символы; CORS не трогается | ✓ | + +--- + +## AC ↔ покрытие + +| AC | Авто-тест | Статус | +|---|---|---| +| AC-1 | (по plan: E2E-01/02) | **gap — нет E2E** | +| AC-2 | (по plan: E2E-01) — IT-01 покрывает только HTTP | **gap — нет UI-теста** | +| AC-3 | IT-01 | ✓ | +| AC-4 | UT-04 + IT-06 | ✓ | +| AC-5 | UT-03 + IT-07 | ✓ | +| AC-6 | manual smoke | вне scope авто | +| AC-7 | IT-02 (✓), E2E-03 — нет | частично | +| AC-8 | IT-03 | ✓ | +| AC-9 | IT-04 | ✓ | +| AC-10 | UT-01 + `test_ut01_osm_copyright_present` | ✓ | +| AC-11 | IT-05 + `test_it05_dual_source_with_osm_passes` | ✓ (E2E-04 conditional) | +| AC-12 | manual | вне scope | +| AC-13 | (по plan: E2E-02) | **gap** | +| AC-14 | UI-cases манульно | вне scope авто | +| AC-15 | IT-08 + регрессионные API-тесты | ✓ (89/89) | + +--- + +## ADR conformance + +**ADR-014 (GPX endpoint).** +- §A решение A2 (fetch+Blob+a.download) — ✓ `_downloadPublicTrack`. +- §B решение B1 (`xml.etree.ElementTree`) — ✓ `export.py:11`. +- §C решение C1 (`Response(...)`, in-memory) — ✓ `endpoint.py:426–435`. +- §D решение D1 (popup остаётся открытым) — ✓ (нет close-call в `_downloadPublicTrack`). +- §E решение E1 (`export.py` модуль) — ✓. +- §F sanitization — ✓ полностью (`_sanitize_for_filesystem`, `_truncate_utf8`, `_ascii_fallback`, см. UT-04). +- §G GPX-структура — ✓ (порядок metadata-children name/desc/author/copyright/link/time, 6 знаков precision, OSM copyright, тест `test_trkpt_coordinate_precision_6_digits`). +- §H порядок проверок — ✓ (format → SELECT → points_count → license → coords → build). +- §I регистрация route — ✓ (после `/health`, конфликта префиксов нет). +- §J logging — ✓. + +**ADR-015 (Source redistribution).** +- §A решение A2 (поле в YAML, runtime-кэш) — ✓ `load_download_allowed_sources`. +- §B решение B1 (ANY-правило) — ✓ `endpoint.py:383`, тест `test_it05_dual_source_with_osm_passes`. +- §C решение C1 (default-deny) — ✓ `config.py:141`. +- §D финальный whitelist — ✓ `config/gps_sources.yaml` (osm=true, остальные=false). +- §F валидация — ✓ в route-handler, кэш в closure router'а. +- §G ответ 403 — расходится с реализованным форматом, см. P2-01. + +--- + +## Регрессия + +- `pytest tests/api` — **89/89 PASS** (включая `test_gps_tracks_endpoint.py` 20 кейсов для существующих маршрутов). +- `node --test tests/web/gps_tracks.test.js` — **24/24 PASS**. +- `tests/web/test_gps_tracks.py` (статика) — PASS (косвенно через pytest). + +Существующих маршрутов / структуры popup-полей / sheet-route::downloadGPX — +ничего не сломано (IT-08 + ручная проверка отсутствия `closeOnClick`-регресса). + +--- + +## Итог + +| Категория | Кол-во | +|---|---| +| P0 | 0 | +| P1 | 1 (отсутствует `tests/web/test_track_download.spec.ts`) | +| P2 | 2 (контракт 403, defensive 400-toast) | +| P3 | 3 (logger выбор, edge-case link-text, pre-existing XSS-вектор) | + +**Verdict: REQUEST_CHANGES** — есть один P1 (отсутствие E2E-спеки, +требуемой test-plan §E2E-01..04 и ADR-014 §4). Это оставляет AC-1, +AC-2, AC-13, и UI-часть AC-7 без автоматизированного покрытия. + +После добавления E2E-теста (или эквивалентного JS-юнита для DOM-структуры ++ обработчиков, с явным фиксированием mobile-проверки как manual smoke) +— PR готов к approve. P2-01 (контракт 403) стоит закрыть в той же +итерации (правка ADR-015 §G **или** перейти на `JSONResponse`), потому +что иначе frontend и backend связаны хрупким workaround'ом +`detail.detail.external_urls`.