Validation NeTEx automatique #4204

ptitfred · 2024-09-19T16:47:07Z

Validation automatique des resources NeTEx historisées.

Test ponctuel

def validate_resource_history(resource_history_id) do
  %{
    "validator" => Atom.to_string(Transport.Validators.NeTEx),
    "resource_history_id" => resource_history_id
  }
  |> Transport.Jobs.ResourceHistoryValidationJob.new()
  |> Oban.insert!()
end

Le validateur NeTEx accepte des ResourceHistory pour se conformer au job de validation. Ceci active de fait la validation automatique de resources NETEx.

thbar

Notes rapides après une première passe:

@ptitfred je me demandais si j'allais tenter de faire tourner ça en local ; vu tes travaux précédents, est-ce que tu as réussi à travailler en local avec un bucket etc ?
je me note d'aller voir la couverture de tests, ça m'aidera à rendre ma lecture plus pertinente

Super boulot en première passe !

ptitfred · 2024-09-25T14:45:26Z

@ptitfred je me demandais si j'allais tenter de faire tourner ça en local ; vu tes travaux précédents, est-ce que tu as réussi à travailler en local avec un bucket etc ?

Oui, le screenshot est tiré de mon setup local. J'ai l'upload S3 depuis un bon moment (pour tester le GTFS diff, et pour la validation OnDemand).

ptitfred · 2024-09-25T15:05:35Z

~~Cette PR n'affiche pas le résumé dans la page du dataset, ça viendra ultérieurement pour ne pas surcharger la review inutilement. (Et parce que j'avais oublié.)~~ Fait dans #4227.

…kground-validation

Fixes #3025.

…kground-validation

thbar

J'ai fait une première relecture hier, et des tests en local.

J'ai spotté un cas qui amène une situation pas bonne en locale, mais je ne suis pas sûr du tout si c'est lié au code de la PR ou pas, je vais refaire un tour et documenter ce que j'ai.

Je refais une passe en fin de journée si tout va bien, et j'apporterais + d'éléments, là trop court niveau timing.

…kground-validation

thbar

Beau boulot ❤️ ! J'ai bien aimé le splitting propre entre NeTEx et GTFS pour généraliser, en particulier.

J'ai procédé à différentes choses, voilà le détail.

Test en local

J’ai pris un dump de production, et j’ai cherché les NeTEx de FLUO (pas le même que celui utilisé par @ptifred sur le screenshot, pour changer un peu).

J’ai démarré avec avec un setup minio local (https://github.com/etalab/transport-site/blob/master/.miniorc.template) mais au final ça n’a pas du être utile sur ce tour, mais ça je ne l’ai su qu’après.

J’ai mis ENROUTE_TOKEN (dont je me dis qu'il gagnerait à être renommé en ENROUTE_CONVERSION_TOKEN à présent, et qu'il ne m'a en fait pas servi) et ENROUTE_VALIDATION_TOKEN à leurs valeurs de production (comme dit par @ptitfred, pas d'environnement de tests sur le validateur).

J’ai ensuite identifié le resource_id d’un NeTEx fluo, et trouvé le dernier resource_history_id en SQL.

Puis lancé via iex comme partagé par @ptitfred:

%{
    "validator" => Atom.to_string(Transport.Validators.NeTEx),
    "resource_history_id" => resource_history_id
  }
  |> Transport.Jobs.ResourceHistoryValidationJob.new()
  |> Oban.insert!()

Ça a tourné comme il faut, et j’ai pu retrouver sur la page de détail de la resource, le rapport qui va bien.

Test sur `prochainement`

(au final sans gros lien avec cette PR, mais ça améliore le setup de test)

J’ai déployé sur la recette, ajouté ENROUTE_VALIDATION_TOKEN au worker comme au site (même si c’est probablement utile qu’au worker sur ce tour a priori), et j’ai pu faire une validation à la demande (scope différent, mais qui était touché par la PR légèrement).

⚠️ je me suis aperçu que si la variable n’est pas là, on obtient (logique) une erreur (401) sur la validation à la demande, mais si on redémarre avec, on conserve (mais de façon silencieuse pour l’utilisateur j’ai l’impression) ce résultat, pendant quelques temps (il doit y avoir un cache quelque part, je n’ai pas regardé). Peut-être un point à améliorer sur les messages là bas (ne concerne que le “on demand” et pas cette PR).

Je n’ai pas testé l’historisation, mais c’était quand même intéressant pour voir que la partie erreur apparaissait bien à l’écran sur le on demand.

Revue de code

Voir les commentaires, il y a un petit point où je me suis demandé si on voulait ou pas ajouter un test sur le cas d'erreur, en particulier.

Clarté d’un message

J’ai mis un peu de temps à comprendre Les avertissements sont temporaires (et j’ai trouvé la traduction plus claire). Peut-être à remplacer par quelque chose du genre “les avertissements affichés sont en beta et vont évoluer” ?

Conclusion et question importante

Un petit pas pour nous, un grand pas pour le PAN, que d’avoir ces validations faites en systématique.

Je ne vois pas le déclenchement via le job automatique, est-ce déjà “intégré” indirectement, ou est-ce que ça fera l’objet d’une autre PR ?

thbar · 2024-10-29T10:43:41Z

apps/transport/lib/jobs/on_demand_validation_job.ex

@@ -76,7 +76,7 @@ defmodule Transport.Jobs.OnDemandValidationJob do
    validator = NeTEx.validator_name()

    case NeTEx.validate(url, []) do
-      {:error, msg} ->
+      {:error, %{message: msg}} ->


J'ai testé ce chemin de code sur la recette (prochainement), après avoir déployé là bas et ajouté la variable ENROUTE_VALIDATION_TOKEN.

Le coverage indique que cette partie n'est pas couverture par les tests, je me suis demandé si on voulait tester ça ou pas.

Le changement est en effet révélateur que c'était pas testé. Cependant j'en démordrai jamais : ça relève du travail d'un type checker et pas de tests unitaires (si on utilisait systématiquement des structs plutôt que des maps).

Ca va venir, ça va venir !!! https://x.com/josevalim/status/1850480238330892636

thbar · 2024-10-29T10:46:16Z

apps/transport/lib/transport_web/controllers/resource_controller.ex

    conn
    |> assign(:related_files, Resource.get_related_files(resource))
    |> assign(:resource, resource)
    |> assign(:other_resources, Resource.other_resources(resource))
    |> assign(:issues, Scrivener.paginate(issues, config))
-    |> assign(:data_vis, encoded_data_vis(issue_type, validation))


NDR: déplacé dans le refacto spécifique au GTFS plus haut, et pas encore disponible sur NeTEx.

thbar · 2024-10-29T10:54:29Z

apps/transport/lib/transport_web/templates/resource/gtfs_details.html.heex

@@ -114,14 +114,16 @@ locale = get_session(@conn, :locale) %>
            <%= raw(
              dgettext(
                "validations",


Le dgettext ci-dessous est couvert par les tests (donc si on introduit une typo, ça lève une erreur, parfait).

apps/transport/lib/transport_web/views/resource_view.ex

apps/transport/lib/transport_web/controllers/resource_controller.ex

thbar · 2024-10-29T11:29:26Z

(et donc je vois aussi le souci "mix dialyzer --plt" qui coince @ptitfred sur le CI !)

thbar · 2024-10-29T13:49:45Z

(et donc je vois aussi le souci "mix dialyzer --plt" qui coince @ptitfred sur le CI !)

J'ai remis à jour la branche, et cette fois... plus de timeout, c'est passé.

ptitfred · 2024-10-29T13:51:49Z

(et donc je vois aussi le souci "mix dialyzer --plt" qui coince @ptitfred sur le CI !)

J'ai remis à jour la branche, et cette fois... plus de timeout, c'est passé.

Je relance le build directement dans ces cas-là

AntoineAugusti · 2024-10-29T13:53:32Z

Même problème sur pas mal de PRs avec CircleCI. Je relance et ça passe. C'est assez récent et j'ai jamais enquêté. Peut-être qu'on mange trop de ressources et que CircleCI a été plus agressif pour tuer des process gourmands dernièrement.

ptitfred · 2024-10-30T11:52:57Z

J’ai mis un peu de temps à comprendre Les avertissements sont temporaires (et j’ai trouvé la traduction plus claire). Peut-être à remplacer par quelque chose du genre “les avertissements affichés sont en beta et vont évoluer” ?

Oui j'ai pas été très inspiré.

ptitfred · 2024-10-30T11:54:12Z

Je ne vois pas le déclenchement via le job automatique, est-ce déjà “intégré” indirectement, ou est-ce que ça fera l’objet d’une autre PR ?

Il faut que je reteste mais autant que je me souvienne (1 mois est passé déjà) le validateur NeTEx est listé à côté des autres et devrait donc bien être déclenché par la validation à l'historisation.

thbar · 2024-10-31T15:04:53Z

Après discussion avec @ptitfred on va temporiser le déploiement à lundi. Principalement à cause d'Halloween.

ptitfred · 2024-10-31T15:09:34Z

Après discussion avec @ptitfred on va temporiser le déploiement à lundi. Principalement à cause d'Halloween.

Je vais refaire quelques tests en local avant de partir en week-end, notamment pour être serein quant à l'historisation.

…kground-validation

ptitfred force-pushed the netex-validator/background-validation branch from 7343117 to 4ac870b Compare September 19, 2024 16:48

ptitfred mentioned this pull request Sep 19, 2024

[Epic] Validation NeTEx #4153

Open

23 tasks

ptitfred force-pushed the netex-validator/background-validation branch 4 times, most recently from 50ba5c5 to 088cfc8 Compare September 25, 2024 13:25

ptitfred added 2 commits September 25, 2024 15:31

NeTEx: fixe l’affichage d’un timeout

548509a

Validation NeTEx automatique

f5c334b

Le validateur NeTEx accepte des ResourceHistory pour se conformer au job de validation. Ceci active de fait la validation automatique de resources NETEx.

ptitfred force-pushed the netex-validator/background-validation branch from 088cfc8 to f5c334b Compare September 25, 2024 13:42

ptitfred marked this pull request as ready for review September 25, 2024 13:42

ptitfred requested a review from a team as a code owner September 25, 2024 13:42

thbar reviewed Sep 25, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

6850ff2

…kground-validation

ptitfred mentioned this pull request Sep 26, 2024

Validation NeTEx : statistiques dans les cartes de ressources NeTEx d'un dataset #4227

Merged

ptitfred added 5 commits September 30, 2024 10:49

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

a3f224d

…kground-validation

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

c56cf32

…kground-validation

Pluriel des résumés de validation

7b706f9

Fixes #3025.

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

9a863f3

…kground-validation

Fix merge

d568f4d

thbar reviewed Oct 8, 2024

View reviewed changes

ptitfred and others added 4 commits October 8, 2024 10:06

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

c96325d

…kground-validation

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

e3963fd

…kground-validation

Merge branch 'master' into netex-validator/background-validation

4a50631

Merge branch 'master' into netex-validator/background-validation

6415e88

thbar approved these changes Oct 29, 2024

View reviewed changes

Merge branch 'master' into netex-validator/background-validation

a4ee1af

Merge remote-tracking branch 'origin/master' into netex-validator/bac…

3495dc4

…kground-validation

ptitfred added this pull request to the merge queue Nov 4, 2024

Merged via the queue into master with commit 8b4f026 Nov 4, 2024
4 checks passed

ptitfred deleted the netex-validator/background-validation branch November 4, 2024 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation NeTEx automatique #4204

Validation NeTEx automatique #4204

ptitfred commented Sep 19, 2024 •

edited

Loading

thbar left a comment

ptitfred commented Sep 25, 2024

ptitfred commented Sep 25, 2024 •

edited

Loading

thbar left a comment

thbar left a comment

thbar Oct 29, 2024

ptitfred Oct 30, 2024 •

edited

Loading

thbar Oct 30, 2024

thbar Oct 29, 2024

thbar Oct 29, 2024

thbar commented Oct 29, 2024

thbar commented Oct 29, 2024

ptitfred commented Oct 29, 2024

AntoineAugusti commented Oct 29, 2024

ptitfred commented Oct 30, 2024

ptitfred commented Oct 30, 2024

thbar commented Oct 31, 2024

ptitfred commented Oct 31, 2024

Validation NeTEx automatique #4204

Validation NeTEx automatique #4204

Conversation

ptitfred commented Sep 19, 2024 • edited Loading

thbar left a comment

Choose a reason for hiding this comment

ptitfred commented Sep 25, 2024

ptitfred commented Sep 25, 2024 • edited Loading

thbar left a comment

Choose a reason for hiding this comment

thbar left a comment

Choose a reason for hiding this comment

Test en local

Test sur prochainement

Revue de code

Clarté d’un message

Conclusion et question importante

thbar Oct 29, 2024

Choose a reason for hiding this comment

ptitfred Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

thbar Oct 30, 2024

Choose a reason for hiding this comment

thbar Oct 29, 2024

Choose a reason for hiding this comment

thbar Oct 29, 2024

Choose a reason for hiding this comment

thbar commented Oct 29, 2024

thbar commented Oct 29, 2024

ptitfred commented Oct 29, 2024

AntoineAugusti commented Oct 29, 2024

ptitfred commented Oct 30, 2024

ptitfred commented Oct 30, 2024

thbar commented Oct 31, 2024

ptitfred commented Oct 31, 2024

ptitfred commented Sep 19, 2024 •

edited

Loading

ptitfred commented Sep 25, 2024 •

edited

Loading

Test sur `prochainement`

ptitfred Oct 30, 2024 •

edited

Loading