Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permet de masquer des jeux de données #3826

Merged
merged 5 commits into from
Mar 21, 2024
Merged

Conversation

AntoineAugusti
Copy link
Member

@AntoineAugusti AntoineAugusti commented Mar 20, 2024

Fixes #3702

Permet de référencer sur transport.data.gouv.fr des jeux de données "masqués". Ces jeux de données ne sont pas trouvables par le biais de la recherche et sont exclus des éléments grands publics comme les statistiques, les cartes etc.

Un JDD est masqué quand il a le 🏷️ masqué ajouté depuis le backoffice. Ceci a pour effet de changer la valeur de la nouvelle colonne dataset.is_hidden à true. L'utilisation de is_active et is_hidden rend assez immédiat la sélection des jeux de données.

Cette PR :

  • ajoute une colonne is_hidden à DB.Dataset
  • gère le changement de valeur de cette colonne
  • fait les changements nécessaires dans toute la codebase pour inclure ou exclure les jeux de données masqués
  • fait du refactor
  • ajoute des tests

Captures d'écran

image
image
Screenshot 2024-03-20 at 14 47 04

@AntoineAugusti AntoineAugusti marked this pull request as ready for review March 20, 2024 14:05
@AntoineAugusti AntoineAugusti requested a review from a team as a code owner March 20, 2024 14:05
Copy link
Contributor

@ptitfred ptitfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ca me semble bon, autant que je puisse en juger après 3j dans la codebase. La relecture d'un autre membre de l'équipe serait la bienvenue.


Serait-ce intéressant de factoriser DB.Dataset.base_query() |> DB.Dataset.include_hidden_datasets() dans DB.Dataset pour éviter de se répéter et surtout éviter un misusage futur ?

Copy link
Member Author

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptitfred Merci d'avoir regardé ! J'ai pris en compte ta remarque et fait un peu de refactor et bien testé les différents scopes pour DB.Dataset

@AntoineAugusti AntoineAugusti requested a review from ptitfred March 21, 2024 08:56
Copy link
Contributor

@thbar thbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beau boulot franchement, c'est velu, respect !

J'ai pu tester en local (il faut trouver un id qui fonctionne avec demo.data.gouv.fr, point qu'il faudra qu'on améliore à un moment, mais ça c'est un autre sujet), avec succès 😄 (jeu http://localhost:5000/datasets/horaires-theoriques-et-temps-reel-de-la-navette-du-pont-de-saint-nazaire-gtfs-gtfs-rt).

Trois points qui n'empêchent pas le merge ci-dessous:

Impact sur l'API

Je me suis posé la question de l'impact sur l'API et j'ai été vérifier ce point, en cherchant volontairement à ne pas regarder trop le code pour "expliquer ça", mais plutôt recetter en mode utilisateur.

On pourrait croire en première passe que les jeux masqués sont toujours visibles dans l'API, mais ce n'est pas le cas : un jeu masqué disparaît également de l'API (ce qui me paraît pas un souci en soit, il faut juste en être conscient dans l'équipe @etalab/transport-tech et aussi chez @etalab/transport-bizdev)

Un premier test donnera l'impression que le jeu est toujours là après un "masquage", mais c'est lié au cache de 30 secondes ici:

@spec datasets(Plug.Conn.t(), map()) :: Plug.Conn.t()
def datasets(%Plug.Conn{} = conn, _params) do
comp_fn = fn -> prepare_datasets_index_data(conn) end
data = Transport.Cache.API.fetch("api-datasets-index", comp_fn, @cache_ttl)
render(conn, %{data: data})
end

Et donc le jeu disparaît bien de la requête aussi

On peut vérifier ça avec un appel curl du type:

curl --silent --location http://localhost:5000/api/datasets | jq '.[] | select(.id == "6061a66ab05ac8509352aa12")'

Une alternative possible serait d'ajouter le champ is_hidden dans le retour de l'API, de permettre de filtrer etc, ça paraît hors scope là mais c'est à garder en tête pour une éventuelle API v2.

Compréhension générale

Ce que je comprends, c'est que le fait de poser un custom_tag spécifique va au final piloter l'attribut is_hidden, et que ça permet de réutiliser le mécanisme d'UX "pratique" qui est les tags, plutôt que de ré-implémenter une checkbox. C'est bien ça qui explique le choix @AntoineAugusti pour ma bonne compréhension ? -> c'est bien ce qui est expliqué dans la description 🙏

(à garder en tête car du coup is_hidden est de fait un "cache" pour le custom tag, et les deux doivent rester plus ou moins synchro !!! -> point d'attention qui reste valable !!!)

Remarque mineure sur le phrasé

Je me demande si on aurait pu utiliser la traduction is_masked (ça existe sur certaines notions en informatique, ex les services systemd ou autre), pour que le français et l'anglais soient plus proches ? Mais bon pas un gros souci 😄, remarque très mineure pour le coup.

Voilà, ces questions n'empêchent pas du tout une validation, bravo encore c'était un gros morceau :-)

@thbar
Copy link
Contributor

thbar commented Mar 21, 2024

@AntoineAugusti je n'ai pas mergé pour le moment, on fait ça quand il y a du monde dans le coin ?

@AntoineAugusti
Copy link
Member Author

Oui je m'en occupe tranquillement dans l'après-midi après avoir intégré les modifications

@AntoineAugusti
Copy link
Member Author

J'ai mis à jour le commentaire de documentation, merci 🙏 J'en ai profité pour ajouter des tests sur l'API pour le cas des JDDs inactifs/cachés.

@AntoineAugusti AntoineAugusti added this pull request to the merge queue Mar 21, 2024
Merged via the queue into master with commit 32fcdd4 Mar 21, 2024
4 checks passed
@AntoineAugusti AntoineAugusti deleted the hidden_datasets branch March 21, 2024 13:09
@thbar
Copy link
Contributor

thbar commented Mar 21, 2024

J'ai mis à jour le commentaire de documentation, merci 🙏 J'en ai profité pour ajouter des tests sur l'API pour le cas des JDDs inactifs/cachés.

Excellent, merci ! Et GG !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BO] Pouvoir masquer un jeu de données référencé sur le PAN
3 participants