From 37190049db75f1353787af148e3118119996220f Mon Sep 17 00:00:00 2001 From: claude-bot Date: Mon, 1 Jun 2026 12:36:30 +0000 Subject: [PATCH] review(ET-008): findings - 1xP0, 4xP1, 3xP2, 4xP3 --- docs/work-items/ET-008/12-review.md | 649 +++++++++++++++++++++------- 1 file changed, 501 insertions(+), 148 deletions(-) diff --git a/docs/work-items/ET-008/12-review.md b/docs/work-items/ET-008/12-review.md index 477ba47..06771f4 100644 --- a/docs/work-items/ET-008/12-review.md +++ b/docs/work-items/ET-008/12-review.md @@ -2,188 +2,541 @@ type: code-review work_item_id: ET-008 title: "Review: GPS-треки с публичных платформ на карте" -version: 1 +version: 2 status: REQUEST_CHANGES created_at: 2026-06-01 +updated_at: 2026-06-01 authors: - "agent:reviewer" reviewed_branch: feature/ET-008-gps base_branch: main +reviewed_commits: + - 0060003 "feat(gps-tracks): ET-008 публичные GPS-треки с публичных платформ" + - 3734b98 "feat(ET-008): GPS tracks pipeline, API, frontend layer" verdict: REQUEST_CHANGES findings_summary: P0: 1 - P1: 0 - P2: 0 - P3: 0 + P1: 4 + P2: 3 + P3: 4 +changelog: + - "v1 (2026-06-01): REQUEST_CHANGES — на ветке отсутствовал код." + - "v2 (2026-06-01): код появился (~7700 LOC). Проведено code-review против ТЗ v2, AC v2 и ADR-005..011." --- -# Code Review — ET-008 +# Code Review — ET-008 (v2) ## Verdict: **REQUEST_CHANGES** -Причина: на ветке `feature/ET-008-gps` **нет ни одной строки кода**, -реализующего ТЗ. PR содержит только artefacts стадий Analyst и Architect -(BRD, ТЗ, AC, TestPlan, UI-кейсы, 7 ADR, infra/data/risks). Development -stage не произвёл выхода — review.review невозможно провести как код-ревью, -потому что нет кода. Отдельные оси (соответствие ТЗ / соответствие ADR / -качество кода / качество тестов) — все «не выполнено», т.к. артефактов -реализации не существует. +На ветке появилась реализация (commits `0060003`, `3734b98`): backend +пакет `src/api/gps_tracks/`, pipeline `scripts/gps_collect.py`, фронт +`src/web/gps_tracks.js`, миграция, YAML-конфиги, docker-compose сервис +`gps-collector`, тесты, fixtures. Архитектурно сборка следует ADR-005…008 +и REQ-F-01…F-03. Однако обнаружено **одно блокирующее (P0) расхождение** +с ТЗ, ломающее основной сценарий просмотра на детальном zoom, и +несколько P1-несоответствий контракту API. ## Что проверено -Согласно регламенту reviewer'а: - -1. ✅ `docs/work-items/ET-008/02-trz.md` (v2, status: draft) — прочитан. -2. ✅ `docs/work-items/ET-008/03-acceptance-criteria.md` (v2, draft) — прочитан. -3. ✅ `docs/work-items/ET-008/06-adr/` — все 7 ADR (ADR-005…011) прочитаны. -4. ✅ `CLAUDE.md` — прочитан. -5. ✅ Git diff `main...feature/ET-008-gps` — проанализирован. - -## Состояние ветки - -`git diff main...feature/ET-008-gps --name-status` (19 файлов): - -| Статус | Файл | -|--------|------| -| M | `.task.md` | -| M | `docs/architecture/README.md` | -| M | `docs/architecture/adr/README.md` | -| M | `docs/work-items/ET-008/00-business-request.md` | -| M | `docs/work-items/ET-008/01-brd.md` | -| M | `docs/work-items/ET-008/02-trz.md` | -| M | `docs/work-items/ET-008/03-acceptance-criteria.md` | -| M | `docs/work-items/ET-008/04-test-plan.yaml` | -| M | `docs/work-items/ET-008/04b-ui-test-cases.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-005-storage-schema.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-006-dedup-algorithm.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-007-pipeline-architecture.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-008-tile-vs-geojson-strategy.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-009-osm-licensing.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-010-enduro-russia-licensing.md` | -| A | `docs/work-items/ET-008/06-adr/ADR-011-ttrails-licensing.md` | -| A | `docs/work-items/ET-008/07-infra-requirements.md` | -| A | `docs/work-items/ET-008/08-data-requirements.md` | -| A | `docs/work-items/ET-008/10-tech-risks.md` | - -Команды для воспроизведения: -``` -git -C enduro-trails diff main...feature/ET-008-gps -- src/ tests/ migrations/ scripts/ config/ -# (empty output — нет ни одного изменения вне docs/) -``` - -Итого: **0 строк** в `src/`, `tests/`, `migrations/`, `scripts/`, `config/`, -`docker-compose.yml`. Все изменения — в `docs/` и `.task.md`. +1. ✅ `docs/work-items/ET-008/02-trz.md` v2 (draft) — REQ-F-01…F-20, REQ-NF-01…NF-07. +2. ✅ `docs/work-items/ET-008/03-acceptance-criteria.md` v2 (draft) — AC-01…AC-17. +3. ✅ `docs/work-items/ET-008/06-adr/` — ADR-005…011 (5 accepted, 2 proposed/blocking). +4. ✅ `CLAUDE.md` — конвенции, фазы, правила для агентов. +5. ✅ Git diff `main...feature/ET-008-gps` — 53 файла, +7705/-1218. +6. ✅ Прочитан исходник backend (config.py, db.py, dedup.py, endpoint.py, + mvt.py, models.py, sources/{base,osm,enduro_russia,ttrails}.py). +7. ✅ Прочитан исходник frontend (gps_tracks.js целиком, изменения в + app.js, app.css, index.html). +8. ✅ Прочитана миграция, scripts/gps_collect.py, оба YAML, requirements.txt, + docker-compose.yml. +9. ✅ Прочитаны 4 тест-файла (dedup, endpoint, mvt, sources/osm) и fixtures. ## Findings -### F-01 [P0]: Отсутствует реализация ТЗ — все 20 REQ-F и все REQ-NF не выполнены +### F-01 [P0]: GeoJSON-слой полностью скрывается из-за рассогласования имён properties -**Severity:** P0 (blocker — «не реализовано требование ТЗ»). +**Severity:** P0 (blocker — нарушено REQ-F-13/F-14, AC-04, AC-08; основной +визуальный сценарий слоя на z ≥ 12 не работает). -**Ссылка на правило:** регламент Reviewer'а, секция Severity: «не реализовано -требование ТЗ → P0». +**Где:** +- `src/api/gps_tracks/endpoint.py`, `_row_to_geojson_feature()` (стр. 51–84) +- `src/web/gps_tracks.js`, `applyGpsFilter()` (стр. 246–267) и + `_buildColorExpression()` (стр. 71–88) -**Что ожидалось** (по ТЗ §5 «Файловая структура изменений», ст. 742–799): +**Что обнаружено:** -Backend (Python): -- `src/api/gps_tracks/__init__.py` -- `src/api/gps_tracks/models.py` — Pydantic + `ACTIVITY_TYPES` (REQ-F-07) -- `src/api/gps_tracks/db.py` — SQLite/Spatialite обвязка (REQ-F-09, ADR-005) -- `src/api/gps_tracks/dedup.py` — `compute_dedup_key()` (REQ-F-08, ADR-006) -- `src/api/gps_tracks/mvt.py` — MVT-генерация (REQ-F-11, ADR-008) -- `src/api/gps_tracks/endpoint.py` — FastAPI routes для REQ-F-10/11/12 -- `src/api/gps_tracks/config.py` — загрузка YAML -- `src/api/gps_tracks/sources/{base,osm,enduro_russia,ttrails}.py` — - REQ-F-04/05/06 (ADR-009/010/011) -- Регистрация роутов в `src/api/main.py` — не сделана -- Обновление `src/api/requirements.txt` (`defusedxml`, `lxml`) — не сделано +GeoJSON endpoint отдаёт в `properties` поля `activity_type` и `sources` +(массив): -Pipeline: -- `scripts/gps_collect.py` — CLI-entry для REQ-F-03 (ADR-007) — отсутствует +```python +"properties": { + ... + "activity_type": row["activity_type"], + ... + "sources": sources, # list + "external_urls": ext_urls, + ... +} +``` -Frontend (Web): -- `src/web/gps_tracks.js` — слой/фильтры/popup (REQ-F-13…F-18) — отсутствует -- Правки `src/web/app.js` — `restorePublicTracksState()` (REQ-F-19), - расширение `rebuildMapOverlays()`, halo по паттерну ET-007 (REQ-F-17, §7.2) — не сделаны -- Правки `src/web/index.html` — чекбокс «Публичные треки» и - `#sheet-gps-filters` (REQ-F-13, REQ-F-14, ТЗ §3) — не сделаны -- Правки `src/web/app.css` — `.terrain-link-btn`, `.gps-filter-grid`, - `.gps-stats-row` (ТЗ §3.1) — не сделаны -- Правки `src/web/style.json`, `src/web/style-dark.json` — halo для - спутника (REQ-F-17) — не сделаны +MVT-эндпойнт (правильно по ТЗ §4.3) отдаёт `activity` (скаляр) и `source` +(скаляр первой sources): -Конфигурация: -- `config/gps_sources.yaml` (REQ-F-01) — отсутствует -- `config/gps_regions.yaml` (REQ-F-02) — отсутствует -- Директория `config/` вообще не существует в репозитории +```python +props = { + ... + "activity": row["activity_type"] or "other", + "source": first_source, + "sources": sources_str, # comma-string + ... +} +``` -Миграции: -- `migrations/gps_tracks_001_init.sql` (REQ-F-09, ADR-005) — отсутствует +Клиентский фильтр в `applyGpsFilter()` использует **только** имена из +MVT-схемы: -Тесты (ТЗ §5, ст. 782–787): -- `tests/api/test_gps_tracks_endpoint.py` — отсутствует -- `tests/api/test_gps_tracks_mvt.py` — отсутствует -- `tests/api/test_gps_tracks_dedup.py` — отсутствует -- `tests/api/test_gps_tracks_sources_osm.py` — отсутствует -- `tests/web/gps_tracks.test.js` — отсутствует +```js +const filter = ['all', + ['in', ['get', 'activity'], ['literal', activities]], + ['in', ['get', 'source'], ['literal', sources]] +]; +map.setFilter(window.gpsTracksLayer.layerGeoId, filter); +``` -Инфраструктура (`07-infra-requirements.md`): -- Сервис `gps-collector` в `docker-compose.yml` с `profiles: ["batch"]` - (`docs/architecture/README.md` ст. 12, 41–46) — не добавлен -- Cron-конфигурация на mva154 Mon/Thu 03:00 UTC — не сделана +Для feature из GeoJSON-source-а `get('activity')` и `get('source')` +возвращают `null` → `['in', null, ['literal', […]]]` = `false` → **все +features фильтруются из показа**. То же касается `line-color`: +`_buildColorExpression('source')` matches по `['get', 'source']` → +GeoJSON-features попадают в fallback `'#808080'`. -**Покрытие AC** (`03-acceptance-criteria.md`): из всех Gherkin-сценариев -AC-01…AC-NN ни один не может быть исполнен — нет ни runtime, ни pipeline, -ни схемы БД. +**Воспроизведение:** +1. Включить чекбокс «Публичные треки». +2. Увеличить zoom до 12+. (`_syncGpsLayersVisibility` делает + `gps-tracks-layer-geo` видимым и скрывает MVT-слой.) +3. На карте — ни одного публичного трека, хотя `/api/gps-tracks?bbox=…` + отдаёт >0 features (`returned > 0`, `total_in_bbox > 0`). Toast + «Показаны N треков из M…» возможен, но карта пустая. -**Действие:** вернуть в стадию Development. Реализовать в порядке, -рекомендованном ADR-007 §pipeline-architecture (config + db/migration → -dedup → один source (OSM, ADR-009 accepted) → endpoints → MVT → UI), -затем добавить тесты по `04-test-plan.yaml` и запустить ревью повторно. +**Ссылка на правило:** Reviewer severity «не реализовано требование ТЗ» → P0. +Затронуты: +- REQ-F-14 (фильтры мгновенно действуют через setFilter). +- REQ-F-17 (стили `gps-tracks-layer` с paint `match`). +- AC-04 Scenario «Поля feature.properties» — `feature.properties` обязан + содержать `length_km`, см. также F-02 ниже. +- AC-08 «Фильтрация по активности»: «на карте отображаются только enduro + и moto треки» — невозможно, т.к. на z ≥ 12 ВСЕ треки отфильтрованы. -## Замечания на полях (НЕ блокирующие, к доработке вне этого ревью) +**Что починить:** +Унифицировать contract. Один из двух вариантов: +- (рекомендуется) В `_row_to_geojson_feature` добавить дублирующие + поля `activity` (= `activity_type`) и `source` (= `sources[0]`) — + не ломая существующих потребителей. Параллельно проверить попап: + `_renderTrackPopupHtml` уже читает `props.activity_type || props.activity` + — менять не нужно. +- Либо переписать `applyGpsFilter`/`_buildColorExpression` так, чтобы они + ветвились по `['has', 'activity']` vs `['get', 'activity_type']` + для + source использовать `['in', 'osm', ['get', 'sources']]` через + index-of / contains (MapLibre expressions поддерживают `in` с массивом + правой части). -Эти пункты НЕ являются P-findings против реализации (её нет), просто -служат напоминанием для следующего ревью, когда код появится: - -- ADR-010 (EnduroRussia) и ADR-011 (ttrails) согласно - `docs/architecture/README.md` ст. 53–60 и ТЗ ст. 59 имеют статус - `proposed/blocked`. Согласно ADR-007 §6 «licensing guard» — pipeline - ОБЯЗАН пропускать источник без `status: accepted`. Это поведение должно - иметь покрытие тестом (`test_pipeline_skips_unaccepted_source`). -- В `src/api/main.py` (`route-line`, `trails-*` слои) при подключении - нового слоя `gps-tracks-layer` важно сохранить z-order, который ТЗ - фиксирует в §7.1: `gps-tracks-layer < gpx-layer-*`. В коде должен быть - явный `beforeId` в `map.addLayer(...)`. -- В `applyBaseLayer()` (ET-007) необходимо добавить шаг по ТЗ §7.2. - Reviewer проверит, что halo `gps-tracks-halo-satellite` переключается - по тому же паттерну, что `trails-track-halo-satellite`. -- Bbox-валидация на endpoint (REQ-NF-01): диапазон координат + площадь. - Должен быть unit-тест на отказ при невалидном bbox. -- `defusedxml` для серверного GPX-парсинга (REQ-NF-01): обязательное - требование, проверится grep'ом на `lxml.etree.parse` без defusedxml. - -## Subjective / Style - -Не применимо — кода нет. - -## Тесты - -Не применимо — тестов нет. После реализации reviewer проверит покрытие -относительно `04-test-plan.yaml` и `04b-ui-test-cases.md`. - -## Заметки по соответствию ADR - -Не применимо для кода. Сами ADR-005…011 не ревьюились в этом проходе -(это работа архитектора/Owner); по PR-changes — приняты как контекст. - -## Рекомендация для CI - -- `04-test-plan.yaml` ссылается на тесты, которых нет → CI должен упасть - на сборе (pytest collection error) или на отсутствии модулей. -- `make lint` пройдёт (нет нового Python-кода), `make test` упадёт. -- Не закрывать ET-008 в Plane как Done. +Тест, который должен поймать это (отсутствует): +`tests/web/gps_tracks.test.js` — feature `{activity_type:'enduro'}` после +`applyGpsFilter` остаётся видимой. Добавить в test-plan. --- -**Итог:** REQUEST_CHANGES. Возврат на стадию Development. +### F-02 [P1]: GeoJSON `feature.properties` отдаёт `length_m`, ТЗ требует `length_km` + +**Severity:** P1 (must-fix — нарушение контракта API, описанного в +REQ-F-10 и AC-04; UI-попап показывает «—»). + +**Где:** `src/api/gps_tracks/endpoint.py`, `_row_to_geojson_feature()` +(стр. 51–84). + +**ТЗ REQ-F-10** (ст. 280–305): +```json +"properties": { + "name": "...", + "activity_type": "...", + "user": "...", + "created_at": "...", + "length_km": 47.3, + "sources": [...], + "external_urls": [...] +} +``` + +**AC-04 Scenario «Поля feature.properties»:** +> Then каждая feature содержит: name, activity_type, user, created_at, +> length_km, sources (array), external_urls (array) + +**Имеется в коде:** +```python +"length_m": row["length_m"], +# и нет ни одного "length_km" +``` + +**Последствие:** В `_renderTrackPopupHtml` (`gps_tracks.js` стр. 325): +```js +const lengthKm = typeof props.length_km === 'number' ? props.length_km.toFixed(1) : '—'; +``` +для GeoJSON-feature (z ≥ 12) `props.length_km` всегда `undefined` → в +попапе постоянно «📏 — км». MVT-features (z 8…11) показывают правильно, +т.к. MVT-builder уже считает `length_km` (`mvt.py` стр. 148). + +**Что починить:** добавить в properties `length_km` (как минимум). +Поле `length_m` оставить, если используется. Аналогично уточнить +`points_count` и `created_at` для popup (см. также F-04). + +--- + +### F-03 [P1]: REQ-F-04 не реализован полностью — все OSM-треки сохраняются как `activity_type='other'` + +**Severity:** P1 (must-fix — функциональный пробел; пользователь не +сможет осмысленно использовать фильтр по активности, который явно +закреплён в AC-08). + +**Где:** `src/api/gps_tracks/sources/osm.py`, `_parse_gpx_trackpoints()` +(стр. 154–284). + +**ТЗ REQ-F-04** (ст. 119–144): +> Для треков с gpx_id — дополнительный запрос +> `GET /api/0.6/gpx/` для метаданных (name, description, tags, user, +> timestamp). Этот запрос делаем отложенно в batch: накопить 100 id → +> запросить. Тип активности — из tags GPX-трека (`Tag: enduro/moto/mtb/hike/...`). + +**Реализовано:** +- batch-запрос метаданных НЕ сделан; +- `name`, `description`, `tags`, `user` всегда `None`/`[]`; +- `activity_type` явно зашит как `"other"`: + ```python + track = TrackInsert(..., activity_type="other", ...) + ``` +- константа `OsmParser.MAPPING` (стр. 25–39) объявлена, но `map_activity` + не вызывается — мёртвый код. + +**Последствие:** в БД ВСЕ треки от OSM (единственного включённого +источника) попадают как `activity_type='other'`. Фильтр по активности +теряет смысл — пользователь видит только «Другое». AC-08 Scenario +«Фильтрация по активности» не может пройти на реальных данных. + +**Что починить:** реализовать batch-fetch на `/api/0.6/gpx/` по +накоплению ID, как описано в ТЗ. Использовать `self.map_activity(...)` +для tags. Если решено отложить — оформить ADR/коммент с явной отметкой +«частичная реализация REQ-F-04, follow-up tracked в …» и понизить +ожидания AC-08 в ТЗ (но это работа аналитика, не разработчика). + +--- + +### F-04 [P1]: Health endpoint не соответствует REQ-F-12 / AC-06 + +**Severity:** P1 (must-fix — нарушение контракта; AC-06 явно перечисляет +обязательные поля, которых нет). + +**Где:** `src/api/gps_tracks/endpoint.py`, `gps_health()` (стр. 196–232). + +**ТЗ REQ-F-12 / AC-06** требуют поля: +| Поле | Тип | +| ------------------ | ------ | +| `db_path` | str | +| `db_size_mb` | float | +| `tracks_total` | int | +| `tracks_by_source` | dict | +| `tracks_by_activity` | dict | +| `last_pipeline_run` | object с полями started/finished/regions/sources_ok/sources_error | +| `tile_cache_size` | int | + +**Имеется:** +```python +return { + "status": "ok", + "db_path": db_path, + "total_tracks": total_tracks, # должно быть tracks_total + "by_activity": by_activity, # должно быть tracks_by_activity + "recent_pipeline_runs": recent_runs, # должно быть last_pipeline_run (объект) +} +``` + +Отсутствуют: `db_size_mb`, `tracks_by_source`, `tile_cache_size`. +Переименованы: `tracks_total → total_tracks`, `tracks_by_activity → +by_activity`, `last_pipeline_run → recent_pipeline_runs` (массив, не +объект). + +Также `recent_pipeline_runs` отдаёт «10 последних запусков», а ТЗ +требует ОДИН последний (агрегированно). Это влияет на UI/админский +view. + +**Что починить:** привести JSON-схему к контракту. Минимум — добавить +`tracks_by_source` (вычислить по `sources_json` агрегацией в Python или +JSON_EACH в SQL), `db_size_mb` (через `os.path.getsize`), `tile_cache_size` +(через `len(_gps_tile_cache)` из `mvt.py`), `last_pipeline_run` объект +(берём первую строку из `pipeline_runs ORDER BY started_at DESC`, +агрегируем `sources_ok`/`sources_error` по последнему region). + +Tests `test_i40_health_endpoint` сейчас закреплены на текущей неправильной +схеме — их тоже придётся обновить. + +--- + +### F-05 [P1]: Z-order ET-006 не зафиксирован — `gps-tracks-layer` может оказаться выше `gpx-layer-*` + +**Severity:** P1 (must-fix — нарушение REQ-F-17 paint requirements §7.1 +и AC-10 Scenario «личный трек визуально выше публичных»). + +**Где:** `src/web/gps_tracks.js`, `_findGpsInsertPosition()` (стр. 191–196). + +**ТЗ §7.1:** +> На карте оба видны параллельно; z-order: +> `gps-tracks-layer` < `gpx-layer-*` (личные треки выше). + +**AC-10 Scenario «Совместимость с ET-006»:** +> Then оба видны параллельно +> And личный трек визуально выше публичных + +**Имеется:** +```js +function _findGpsInsertPosition(map) { + const style = map.getStyle && map.getStyle(); + if (!style || !style.layers) return undefined; + const routeLayer = style.layers.find(l => l.id === 'route-line' || l.id.startsWith('route-')); + return routeLayer ? routeLayer.id : undefined; +} +``` + +Функция ищет только `route-*`. Если `gpx-layer-*` уже добавлен в стиль +(ET-006), но route-line ещё нет, gps-tracks-layer добавится **в конец** +(`before = undefined` → addLayer без beforeId → поверх всего стиля), +**в том числе поверх gpx-layer**. Это нарушает обязательное правило +из §7.1 / AC-10. + +**Что починить:** +```js +function _findGpsInsertPosition(map) { + const style = map.getStyle && map.getStyle(); + if (!style || !style.layers) return undefined; + // Приоритет beforeId: gpx-layer-* (ET-006), затем route-* (если нет gpx). + const gpxLayer = style.layers.find(l => l.id.startsWith('gpx-layer')); + if (gpxLayer) return gpxLayer.id; + const routeLayer = style.layers.find(l => l.id === 'route-line' || l.id.startsWith('route-')); + return routeLayer ? routeLayer.id : undefined; +} +``` + +Соответствующий unit-тест добавить в `tests/web/gps_tracks.test.js` +(в test-plan он есть как WEB-INTEG). + +--- + +### F-06 [P2]: Валидация bbox по площади отсутствует (REQ-NF-01) + +**Severity:** P2 (should-fix). + +**Где:** `src/api/gps_tracks/endpoint.py`, `_parse_bbox()` (стр. 17–48). + +**ТЗ REQ-NF-01:** +> Bbox-параметр валидируется (диапазон координат, площадь). + +**Имеется:** валидация диапазона `[-180,180]/[-90,90]`, проверка +`west= _GPS_TILE_CACHE_MAX: + _gps_tile_cache.pop(next(iter(_gps_tile_cache))) # FIFO, не LRU + _gps_tile_cache[(z, x, y)] = data +``` + +ТЗ REQ-NF-04: «LRU-кэш в памяти процесса FastAPI: 1024 записи». Текущая +реализация — FIFO. Для тайлов это особо ухудшает: часто запрашиваемый +тайл, попавший в кэш первым, будет вытеснен раньше, чем редкий тайл, +попавший позже. + +**Что починить:** `from functools import lru_cache` нельзя из-за +mutable invalidation; использовать `collections.OrderedDict` с +`move_to_end` при чтении, либо `cachetools.LRUCache`. Совместимая идея — +сделать `get_gps_cached_tile`: +```python +def get_gps_cached_tile(z, x, y): + key = (z, x, y) + if key in _gps_tile_cache: + _gps_tile_cache.move_to_end(key) # OrderedDict + return _gps_tile_cache[key] + return None +``` + +--- + +### F-09 [P3]: Мёртвое поле конфигурации `save_user_field` + +**Severity:** P3 (nice-to-have). + +**Где:** `config/gps_sources.yaml` — поле задано (`save_user_field: +true|false`); ни один модуль его не читает. AC-16 Scenario «Pipeline не +сохраняет запрещённые поля» рассчитывает, что это поле уважается. + +**Что починить:** в `OsmParser`/upsert обрабатывать `save_user_field=false` +→ `user=None`. Сейчас OSM всё равно ставит `user=None` (см. F-03), но +поле должно работать как контракт для будущих источников. + +--- + +### F-10 [P3]: Лишний импорт `pytest_asyncio` в `tests/api/test_gps_tracks_endpoint.py` + +**Severity:** P3. + +`import pytest_asyncio` есть, но `@pytest_asyncio.fixture` нигде не +используется (только `@pytest.mark.asyncio`). Не блокирует, но в чистом +коде убирается. + +--- + +### F-11 [P3]: `MockRow(dict)` в `tests/api/test_gps_tracks_mvt.py` + +**Severity:** P3. + +Тесты используют `class MockRow(dict)` как замену `sqlite3.Row`. Работает +для текущего кода, но `sqlite3.Row` не поддерживает `__contains__` так +же, как dict. Если в `mvt.py` появится `if "x" in row:`, тесты +разойдутся с продом. Безопаснее использовать `sqlite3.Row` напрямую через +`open_db + INSERT + fetchone()`. + +--- + +### F-12 [P3]: Лишняя проверка `"source_priority" in existing.keys()` в `db.py` + +**Severity:** P3. + +`src/api/gps_tracks/db.py` стр. 116: +```python +existing_priority = existing["source_priority"] if "source_priority" in existing.keys() else 999 +``` +Колонка `source_priority` объявлена в миграции (`migrations/gps_tracks_001_init.sql` +ст. 22) с `NOT NULL DEFAULT 999`. Проверка избыточна (read-protection +осталась от какой-то ранней итерации). Лучше убрать — иначе создаётся +впечатление, что колонка опциональна. + +--- + +## Соответствие ADR + +| ADR | Status | Соблюдение в коде | Замечания | +|-----|--------|-------------------|-----------| +| ADR-005 storage-schema | accepted | ✅ соблюдено | таблица `tracks` (+`source_priority`), индексы и `pipeline_runs` совпадают | +| ADR-006 dedup-algorithm | accepted | ✅ соблюдено | `compute_dedup_key` 1-в-1; покрыт unit-тестами U-10…U-14 | +| ADR-007 pipeline-architecture | accepted | ✅ соблюдено | сервис `gps-collector` с `profiles:["batch"]`, license-guard в `_check_license_adr` | +| ADR-008 tile-vs-geojson | accepted | ⚠️ частично | переключение по zoom есть; но contract фич GeoJSON vs MVT расходится (см. F-01) | +| ADR-009 osm-licensing | accepted | ✅ соблюдено | attribution в source, рабочий парсер | +| ADR-010 enduro-russia (proposed) | proposed | ✅ соблюдено | parser — заглушка, `enabled: false`, license-guard сработает | +| ADR-011 ttrails (proposed) | proposed | ✅ соблюдено | то же | + +License-guard в `scripts/gps_collect.py` `_check_license_adr()` корректно +читает YAML front-matter ADR. Покрытия unit-тестом нет — рекомендую +добавить (`test_pipeline_skips_unaccepted_source`). + +## Тесты — оценка + +| Тест-файл | Покрытие test-plan | Качество | +|-----------|-------------------|----------| +| `test_gps_tracks_dedup.py` | U-10…U-14 | ✅ хорошо | +| `test_gps_tracks_mvt.py` | U-50…U-52 | ✅ адекватно (см. F-11) | +| `test_gps_tracks_endpoint.py` | I-20…I-23, I-30…I-31, I-40 | ⚠️ AC-06 не покрыт корректно — тест зафиксирован на неверной схеме (F-04) | +| `test_gps_tracks_sources_osm.py` | U-42…U-44 | ✅ defusedxml проверен фикстурой xxe-payload.gpx | + +**Не покрыто тестами:** +- `tests/web/gps_tracks.test.js` — заявлен в ТЗ §5 ст. 787 и + `04-test-plan.yaml`, ОТСУТСТВУЕТ. Был бы первой защитой от F-01. +- pipeline (`scripts/gps_collect.py`) — нет ни одного теста на + `_check_license_adr`, `_collect_source_for_region`, dry-run, exit-code. + AC-02 Scenarios «Падение одного источника не валит остальные» и + «Dry-run» по факту не верифицированы. +- bbox area-валидация — отсутствует и в коде (F-06), и в тестах. + +## Замечания, не доходящие до P-finding + +- `_simplify_coords` в `mvt.py` дублирует `simplify_coords` из `src/api/main.py`. + Не критично, но напрашивается общая утилита. +- `src/api/main.py` ст. 17–20: `GPS_TRACKS_DB_PATH` вычисляется **до** + импорта shapely/fastapi, в строгом смысле это «нечистый» импорт-time + side-effect. Не блок. +- В `endpoint.py` `init_db` вызывается на каждый запрос (`_get_conn`). + Это означает, что `executescript` выполняется на каждый запрос. SQL + использует `IF NOT EXISTS`, так что функционально ок, но это лишний + I/O. Рекомендую инициализировать БД один раз при `create_gps_router`. +- `src/api/main.py` ст. 1255: `app.include_router(gps_router)` хорошо + встроено перед `StaticFiles` mount — порядок правильный. + +## Воспроизведение P0 для разработчика + +```bash +# 1. Запустить с тестовой БД (см. test_i20_geojson_basic): +pytest tests/api/test_gps_tracks_endpoint.py::test_i20_geojson_basic -q + +# 2. Вытащить feature.properties — наблюдать "activity_type", "sources" (list), +# отсутствие "length_km". + +# 3. Открыть DevTools в браузере на dev-стенде, проверить: +window._map.queryRenderedFeatures({layers:['gps-tracks-layer-geo']}) +# → пустой массив на z >= 12, потому что setFilter с ['get','activity'] всё скрыл. + +# 4. Временный обход для отладки: +window._map.setFilter('gps-tracks-layer-geo', null) +# → треки появятся, но серые (line-color fallback) — что подтверждает F-01. +``` + +## Рекомендация для CI + +- `pytest tests/api/` сейчас зелёный, потому что: + - `test_i40_health_endpoint` фиксирует текущую неправильную схему + (F-04) → нужно поправить вместе с фиксом; + - frontend-тесты отсутствуют (F-01 не отлавливается). +- `make lint` ожидаемо проходит. +- Перед закрытием задачи CI должен прогонять и frontend-тесты + (`tests/web/gps_tracks.test.js`), которые на данный момент не написаны. +- Не закрывать ET-008 в Plane как Done. + +## Итог + +**REQUEST_CHANGES.** Минимальный объём правок: +1. F-01 (P0) — унифицировать имена properties между MVT и GeoJSON, + + web-тест на `applyGpsFilter`. +2. F-02 (P1) — добавить `length_km` в GeoJSON. +3. F-03 (P1) — реализовать batch-fetch `/api/0.6/gpx/` и mapping + `activity_type` (или явная декларация частичной реализации REQ-F-04). +4. F-04 (P1) — выровнять `gps_health()` под REQ-F-12 / AC-06. +5. F-05 (P1) — корректный `beforeId` в `_findGpsInsertPosition`. + +После исправлений P0/P1 — повторить ревью; P2/P3 могут пройти отдельным +PR follow-up.