# FEEDBACK TO ARCHITECT — v23 > **Spec technique reviewée** : `001-histometeo-mvp.tech.v23.md` > **Spec fonctionnelle source** : `001-histometeo-mvp.md` > **Date review** : 2026-03-14 --- ## 1) Functional Compliance | AC | Statut | Justification | | ------------------------------------------------------------------------------------------ | ------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | AC1–AC46 | ✅ OK | Inchangés. Les 143 tests hérités (≤ v22) passent tous — aucune régression fonctionnelle. | | AC47 — Tout appel HTTP externe logué avec ou sans `search_id` | ✅ OK | Le guard dans `weather_service.py`, `normals_service.py` et `commune_service.py` est bien `if tracker:` (sans `and search_id`). Vérifié : 6 occurrences modifiées (2 par service × 3 services). `search_id=None` est passé tel quel à `log_api_call()`. | | AC48 — `GET /api/normals` cache miss → 3 entrées `search_id = NULL`, `service = "normals"` | ✅ OK | Test `test_normals_api_tracks_without_search_context` (`test_admin_api.py`) vérifie exactement ce scénario avec assertions `len(logs) == 3` et `all(row["search_id"] is None)`. | | AC49 — `GET /api/normals/annual` cache miss → 3 entrées idem | ✅ OK | Test `test_annual_normals_api_tracks_without_search_context` identique au précédent. | | AC50 — `GET /api/og-image/…` cache miss → entrées `search_id = NULL` | ✅ OK | Test `test_weather_in_og_image_tracks_without_search_context` vérifie `len(logs) >= 1` avec `search_id IS NULL`. | | AC51 — Dashboard retourne `attributed` + `standalone` dans `api_calls_today` | ✅ OK | `get_dashboard()` exécute une requête SQL distincte avec `SUM(CASE WHEN search_id IS NOT NULL …)` / `SUM(CASE WHEN search_id IS NULL …)`. Tests T54 (`test_dashboard_attributed_vs_standalone`) et T59 (`test_dashboard_api_includes_standalone`) vérifient les valeurs. Admin JS affiche via `ensureApiAttributionSummary()` avec `escapeHtml()`. | --- ## 2) Contract Compliance ### Scope respecté ? ✅ **OUI** — Les fichiers modifiés sont strictement dans le périmètre contractuel : | Fichier | Modifié ? | Conforme au scope ? | | -------------------------------- | --------- | -------------------------------------------------------------- | | `src/tracking_service.py` | ✅ | ✅ Migration schéma, signature `log_api_call`, dashboard query | | `src/weather_service.py` | ✅ | ✅ Guard `if tracker:` (2 occurrences) | | `src/normals_service.py` | ✅ | ✅ Guard `if tracker:` (2 occurrences dans `_fetch_chunk`) | | `src/commune_service.py` | ✅ | ✅ Guard `if tracker:` (2 occurrences dans `search_communes`) | | `admin/admin.js` | ✅ | ✅ Affichage `attributed`/`standalone` | | `tests/test_tracking_service.py` | ✅ | ✅ 5 nouveaux tests (T51–T55) | | `tests/test_admin_api.py` | ✅ | ✅ 4 nouveaux tests (T56–T59) | | `README.md` | ✅ | ✅ Régularisation R1 v22 (dans périmètre v23) | ### Forbidden changes respectés ? ✅ **OUI** — Vérification exhaustive : - `public/app.js`, `public/index.html`, `public/style.css` — **aucune modification** - `src/cache.py`, `src/prefetch_service.py`, `src/og_service.py`, `src/main.py`, `src/config.py` — **aucune modification** - `admin/index.html` — **aucune modification** - Tests existants hérités (≤ v22, 143 tests) — **tous passent sans modification sémantique** ### Invariants préservés ? | Invariant | Statut | Note | | ------------------------------------------------------ | ------ | ------------------------------------------------------------------------------------------------------------ | | INV-1 à INV-43 | ✅ | Aucune régression — confirmé par la suite de tests complète | | INV-34 (absorption des exceptions tracking) | ✅ | `log_api_call()` conserve son `try/except` absorbant. Aucune exception ne peut remonter au service appelant. | | INV-44 (MODIFIÉ) — guard `if tracker:` | ✅ | Les 6 guards dans les 3 services sont bien `if tracker:` sans `and search_id`. | | INV-45 (NOUVEAU) — `search_id` nullable | ✅ | Schéma `CREATE TABLE` : `search_id TEXT` (sans `NOT NULL`). Migration PRAGMA détecte et recrée la table. | | INV-46 (NOUVEAU) — `_count_api_calls` exclut les NULL | ✅ | La requête SQL `WHERE search_id = ?` exclut naturellement les `NULL` (SQL : `NULL != ?`). | | INV-47 (NOUVEAU) — Dashboard agrège toutes les entrées | ✅ | La requête dashboard ne filtre pas par `search_id` presence — elle compte tout. | ### Done When | # | Statut | Vérification | | ------- | ------ | ------------------------------------------------------------------------------------- | | D33–D66 | ✅ | Acquis v22, 143 tests hérités passent | | D67 | ✅ | Test T56 vérifie 3 entrées normals avec `search_id = NULL` pour `/api/normals` | | D68 | ✅ | Test T57 vérifie 3 entrées normals avec `search_id = NULL` pour `/api/normals/annual` | | D69 | ✅ | Test T58 vérifie ≥ 1 entrée avec `search_id = NULL` pour `/api/og-image/…` | | D70 | ✅ | Dashboard `api_calls_today` agrège les appels avec et sans `search_id` | | D71 | ✅ | `_count_api_calls(search_id)` exclut les NULL — test T52 le vérifie | | D72 | ✅ | Dashboard JSON retourne `attributed` et `standalone` — tests T54, T59 | | D73 | ✅ | 152 tests passent (143 hérités + 9 nouveaux) | --- ## 3) Technical Quality ### Complexité inutile ? ✅ **Non** — Les modifications sont mécaniques et minimales : - 6 modifications de guards (retrait de `and search_id`) — strictement identiques - 1 migration SQLite ciblée avec détection PRAGMA - 1 requête SQL complémentaire pour le dashboard - Le tout représente un diff très compact ### Dette technique introduite ? ✅ **Non** — Pas de dette ajoutée. La migration `_migrate_search_id_nullable()` utilise l'approche PRAGMA + recreate table, qui est la méthode standard SQLite pour modifier les contraintes de colonnes. Le placement dans `_init_db()` est cohérent avec la migration v22 existante (`ADD COLUMN service`). L'accès par index positionnel (`col[1]`, `col[3]`) pour PRAGMA est robuste et conforme à la recommandation de la spec (piège §5.2). ### Duplication ? ✅ **Aucune** — Le pattern de tracking dans les services est répétitif par nature (chaque service a ses propres paramètres), mais il n'y a pas de duplication nouvelle introduite. ### Incohérences ? ✅ **Aucune** détectée. **Point positif** : la migration utilise `INSERT INTO … SELECT * FROM` avec colonnes explicitement listées, ce qui est plus robuste que `SELECT *` en cas d'ajout futur de colonnes. --- ## 4) Test Coverage ### Tests suffisants ? ✅ **OUI** — 9 nouveaux tests couvrent tous les aspects de la spec : | Test | Couverture | | ------------------------------------------------------------ | ------------------------------------------------------------------------------ | | T51 `test_log_api_call_with_null_search_id` | Insertion avec `search_id = None` + vérification en base | | T52 `test_count_api_calls_excludes_null_search_id` | `_count_api_calls` n'inclut pas les NULL | | T53 `test_dashboard_counts_standalone_calls` | Total dashboard inclut les standalone | | T54 `test_dashboard_attributed_vs_standalone` | Compteurs `attributed`/`standalone` corrects | | T55 `test_migration_search_id_nullable` | Migration v22 → v23 complète (schéma + insertion NULL OK + flag `notnull = 0`) | | T56 `test_normals_api_tracks_without_search_context` | `/api/normals` → 3 entrées NULL | | T57 `test_annual_normals_api_tracks_without_search_context` | `/api/normals/annual` → 3 entrées NULL | | T58 `test_weather_in_og_image_tracks_without_search_context` | `/api/og-image/…` → ≥ 1 entrée NULL | | T59 `test_dashboard_api_includes_standalone` | Dashboard API retourne `attributed`/`standalone` | ### Edge cases oubliés ? Aucun edge case bloquant. Deux observations mineures (non bloquantes) : - T58 vérifie `len(logs) >= 1` plutôt que le nombre exact d'appels pour `og-image`. C'est acceptable car le nombre dépend du cache state des sous-services (communes + weather). L'important est que ≥ 1 appel soit tracké. - Pas de test dédié pour `GET /api/resolve/{slug}` hors contexte (il passe par `search_communes` qui est couvert). Couverture transitive suffisante. --- ## 5) UX Consistency Check ### Incohérences flagrantes ? ⚠️ **OUI** — Deux incohérences significatives observées en test live sur le dashboard admin : #### Incohérence 1 — Commune absente pour toutes les recherches SPA Dans `public/app.js`, la fonction `fetchWeatherForCommune()` (L1324) n'envoie que `lat`, `lon`, `start`, `end` à `/api/weather`. Les paramètres optionnels `commune` et `slug` ne sont **jamais** transmis. Conséquence : toutes les recherches initiées par le SPA ont `commune_1 = NULL` et `commune_1_slug = NULL` dans `search_logs`. La colonne commune affiche "-" dans la liste des recherches et les recherches coûteuses. Seules les routes SEO (`/meteo/...`, `/ville/...`, `/comparaison/...`) peuplent la commune (résolution côté serveur). Cela rend le tracking SPA inutilisable pour identifier les communes recherchées. **Impact** : le top communes du dashboard ne reflète que les crawlers SEO, pas l'usage réel des visiteurs SPA. Fichiers concernés : `public/app.js` (hors scope v23). #### Incohérence 2 — `total_api_calls` ne reflète que les appels weather Le champ `total_api_calls` de `search_logs` est calculé par `_count_api_calls(search_id)`, qui ne compte que les entrées de `api_call_logs` avec ce `search_id` spécifique. Or, les appels normals (3 par recherche) et communes (autocomplete) sont des requêtes SPA séparées sans `search_id` → `search_id = NULL`. Conséquences observées sur le dashboard live : | Métrique | Valeur affichée | Valeur réelle | | ---------------------------------------- | --------------- | -------------------------- | | Appels API (Total dashboard) | **6** ✅ | 6 | | Attribués | **2** | 2 (uniquement weather) | | Non-attribués | **4** | 4 (3 normals + 1 communes) | | `total_api_calls` recherche individuelle | **1** | 4 (1 weather + 3 normals) | De plus, `get_search_detail()` requête `WHERE search_id = ?`, donc la vue détail d'une recherche ne montre que le ou les appels weather — les appels normals et communes associés à la même session utilisateur sont invisibles. **Impact** : le tableau "Recherches coûteuses" sous-estime le coût réel d'une recherche SPA d'un facteur ~4x. Un administrateur ne peut pas diagnostiquer quelle recherche utilisateur a consommé le plus de quota. ### Comportement inattendu ? ⚠️ **OUI** — Cf. les deux incohérences ci-dessus. L'utilisateur final n'est pas impacté (pas de changement frontend public), mais l'administrateur voit un dashboard dont les chiffres par-recherche sont trompeurs malgré des totaux globaux corrects. ### Détails admin JS ✅ L'affichage `attributed`/`standalone` via `ensureApiAttributionSummary()` est correct et bien échappé (`escapeHtml()`). Le layout est cohérent avec le style muted existant. --- ## 6) Required Corrections **Aucune correction obligatoire.** --- ## 7) Recommended Improvements (non bloquantes) | # | Recommandation | Impact | Priorité | | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------- | ----------- | | R1 | Envisager un test dédié pour `GET /api/communes?q=…` hors contexte de recherche (autocomplete SPA). Le guard modifié couvre bien ce cas mais aucun test ne l'exerce directement via la route `/api/communes`. La couverture indirecte via `resolve_slug` est suffisante pour la v23, mais un test explicite renforcerait la confiance. | Faible | Basse | | R2 | Les DeprecationWarning `asyncio.get_event_loop_policy` (Python 3.16 slated removal) dans `pytest-asyncio` méritent un suivi pour une future mise à jour. | Nul actuellement | Basse | | **R3** | **Commune non transmise par le SPA (pré-existant, à traiter en v24)** — `public/app.js` `fetchWeatherForCommune()` doit transmettre `commune` et `slug` en query params à `/api/weather`. Sans cela, toutes les recherches SPA ont `commune_1 = NULL` et ne remontent pas dans le top communes. Fichiers impactés : `public/app.js` (ajout de 2 params), `src/main.py` (déjà prêt — les params `commune` et `slug` sont optionnels). | **Élevé** | **Haute** | | **R4** | **`total_api_calls` sous-estime le coût réel (pré-existant, à traiter en v24)** — Les appels normals/communes initiés par le SPA ont `search_id = NULL` et ne sont pas comptabilisés dans `total_api_calls` d'une recherche. Deux approches possibles : (a) propager le `search_id` du frontend (cookie/header) aux appels `/api/normals` et `/api/communes` suivants, ou (b) corréler côté serveur par fenêtre temporelle + coordonnées. L'approche (a) est plus simple mais nécessite des modifications dans `public/app.js` et `src/main.py`. | **Élevé** | **Haute** | | **R5** | **Vue détail incomplète (conséquence directe de R4)** — `get_search_detail()` requête `WHERE search_id = ?`, rendant les appels standalone invisibles dans le détail d'une recherche. Si R4 est résolu, R5 est automatiquement résolu. Sinon, envisager une vue "appels orphelins du jour" dans l'interface admin. | **Moyen** | **Moyenne** | --- ## 🧭 Décision finale # ⚠️ Validé avec réserves **L'implémentation v23 est conforme à sa propre spec technique.** Les 51 Acceptance Criteria (AC1–AC51) sont satisfaits. Les 7 Done When (D67–D73) sont vérifiés. Les invariants sont respectés. Les fichiers interdits n'ont pas été modifiés. Les 152 tests passent (143 hérités + 9 nouveaux). Le delta v23 comble efficacement l'angle mort de tracking dashboard identifié en review v22 (R2) avec un changement minimal, mécanique et bien testé. La migration SQLite est robuste. **Réserves (observées en test live, hors scope v23 mais à traiter en priorité) :** 1. **R3 — Commune absente pour 100% des recherches SPA** : `public/app.js` ne transmet pas `commune`/`slug` à `/api/weather`. Le top communes du dashboard ne reflète que le trafic SEO. Impact direct sur l'exploitation du tracking. 2. **R4 — `total_api_calls` sous-estimé d'un facteur ~4x pour les recherches SPA** : les appels normals (3) et communes ne sont pas attribués à la recherche. Le tableau "Recherches coûteuses" est trompeur. La vue détail d'une recherche ne montre que les appels weather. Ces deux points ne sont pas des régressions v23 — ils sont pré-existants et résultent de l'architecture SPA (requêtes indépendantes sans propagation de contexte). Mais la v23 ayant rendu les appels standalone visibles dans le dashboard, l'écart entre totaux globaux corrects et totaux per-search incomplets devient plus flagrant et potentiellement confus pour l'administrateur. **Routage recommandé** : retour Architecte pour spec v24 intégrant R3, R4, R5 avec périmètre élargi à `public/app.js` et `src/main.py`.