search

etalab / dashboard_api_entreprise

Dashboard (statut du service et espace client/admin) de API Entreprise
https://dashboard.entreprise.api.gouv.fr
3 stars 3 forks source link

Sanitize incoming user/index data for null #76

Closed Samuelfaure closed 5 years ago

Samuelfaure commented 5 years ago

Instead of guarding-clause every method and ending up with spaghetti code, it seems more logical/clear/concise to sanitize any incoming data against null cases. null or undefined will therefore be replaced with empty string.

(NB : Manière de faire un peu non-orthodoxe ici, je m'attend à ce que cela choque et suis ouvert à la discussion)

brindu commented 5 years ago

Ca fontionne tant qu'on a fonctionnellement pas de différence entre un null et une chaine vide. Ce qui n'est pas la cas ici, mais ça reste pas terrible...

Je comprends pas où est le spaghetti code si jamais on corrige le problème dans la méthode de filtre. C'est pas possible de ne simplement pas appliquer le filtre sur des valeurs null ?

brindu commented 5 years ago

@Samuelfaure Je propose une implémentation dans #77, dis moi ce que tu en penses.

Samuelfaure commented 5 years ago

Le code de #77 est clairement supérieur... Le seul avantage que je vois à #76 (PR ici) c'est qu'on peut réutiliser la méthode si on en a besoin ailleurs, mais pour le moment on en a besoin nul part et cet avantage est assez loin de contrebalancer la différence de qualité.

Par contre en y repensant ça me gêne de merge #76 ou #77 sans tests unitaires. Déjà c'est un use-case rare (valeur null car crée depuis l'API et non le dashboard) auquel je n'ai pas pensé qui nous force à faire cette correction, je voudrais éviter qu'on retombe sur des edges cases étranges.

Donc si c'est OK pour toi je vais finir l'ajout des tests, coder les tests unit pour #77 et ensuite on pourra merge.

Je ferme ici.