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.

Sign up for GitHub

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

Jump to bottom

List diffusion databases and their attributes #544

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

List diffusion databases and their attributes #544

wants to merge 22 commits into from

Conversation

leonkenneth
Copy link
Contributor

@leonkenneth leonkenneth commented Feb 3, 2025 •
edited
Loading

Endpoint pour lister les bases de données dans lesquelles sont diffusés les ID-RNBs, et le catalogue des attributs qui y sont accessibles.

Screenshot 2025-02-03 at 10-57-37 DiffusionDatabase object (3) Change diffusion database Django site admin

Screenshot 2025-02-03 at 11-09-11 Diffusion Database – Django REST framework

leonkenneth
leonkenneth commented Feb 3, 2025
View reviewed changes
app/api_alpha/urls.py Outdated Show resolved Hide resolved
app/batid/admin.py Outdated Show resolved Hide resolved
app/batid/migrations/0101_add_known_diffusion_databases.py Outdated Show resolved Hide resolved
@leonkenneth leonkenneth changed the title WIP: List diffusion databases and their attributes List diffusion databases and their attributes Feb 3, 2025
leonkenneth
leonkenneth commented Feb 3, 2025
View reviewed changes
app/batid/models.py Outdated Show resolved Hide resolved
@bbaret
Copy link
Collaborator

bbaret commented Feb 3, 2025 •
edited
Loading

Il serait peut-être judicieux de mettre un placeholder dans ce champ JSON Attributes de Django Admin ? Pour guider sur le format attendu.

@fchabouis fchabouis requested a review from a team February 3, 2025 11:02
fchabouis
fchabouis reviewed Feb 3, 2025
View reviewed changes
app/pyproject.toml Outdated Show resolved Hide resolved
fchabouis
fchabouis reviewed Feb 3, 2025
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
@fchabouis
Copy link
Collaborator

Merci pour cette première PR !

2 remarques additionnelles de mon côté :

  • j'ai lancé en local et Django me dit que certaines modification au niveau du modèle ne sont pas refletées dans les migrations. Il est possible que tu aies généré les migrations puis retouché le modèle (au niveau du champ attribute visiblement). Tu peux regénérer les migrations, sinon ça va bloquer ?
  • Pour le moment on n'a jamais inséré de données via une migration dans la base de données. Je ne suis pas radicalement contre, mais je me demande ce qui va se passer quand ces données vont par exemple périmer. Et aussi à l'aspect test, qui fait qu'à chaque fois qu'on va lancer un test, les migrations sont appliquées et qu'on va donc systématiquement insérer X lignes dans cette table alors qu'on en a pas besoin pour tester. Donc spontanément, j'aurais créé le modèle, mais j'aurais mis les données en base via un script, mais sans faire apparaitre ces données quelque part dans notre code. Qu'est ce que tu en penses ?

@leonkenneth
Copy link
Contributor Author

  • Ca explique pourquoi mes modifs sont pas en base :) Merci je vais fix
  • Ca me va. De toutes façons il faudra le faire pour les attributs. Je vais retirer la migration et faire un script

@leonkenneth
Copy link
Contributor Author

Il serait peut-être judicieux de mettre un placeholder dans ce champ JSON Attributes de Django Admin ? Pour guider sur le format attendu.

@bbaret Avec un default c'est un peu compliqué j'ai l'impression ? On pourrait imaginer mettre des indications, mais à ce stade je pense qu'il faudrait soit juste faire une formation ou un Notion externe, soit regarder pour intégrer un éditeur en mode table pour plus de clarté.

@leonkenneth leonkenneth requested a review from fchabouis February 4, 2025 16:35
fchabouis
fchabouis approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@fchabouis fchabouis left a comment

Choose a reason for hiding this comment

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

Belle première PR :)
Essayons là !

app/api_alpha/tests/test_views.py
@@ -547,3 +548,18 @@ def get_content_from_streaming_response(response):
content = list(response.streaming_content)
# each element of the list is a byte string, that we need to decode
return "".join([b.decode("utf-8") for b in content])


class TestDiffusionDatabases(APITestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

on pourra à l'avenir rajouter quelques tests supplémentaires, comme par exemple vérifier que le json validator marche bien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchabouis fchabouis fchabouis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leonkenneth @bbaret @fchabouis