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

Admin: Correction de l'export des entreprises [GEN-1861] #4416

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

tonial
Copy link
Contributor

@tonial tonial commented Jul 12, 2024

🤔 Pourquoi ?

L'actuel ne marche pas depuis des mises à jour de django-import-export

🍰 Comment ?

On remplace la lib externe par un petit morceau de code qui utilise un serializer DRF pour exporter dans un fichier XLSX

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial requested a review from xavfernandez July 12, 2024 10:27
@tonial tonial self-assigned this Jul 12, 2024
Copy link

itou/gps/admin.py Outdated Show resolved Hide resolved
@tonial tonial force-pushed the alaurent/fix_companies_admin_export branch from 97640f9 to 8a3e99a Compare July 12, 2024 14:08
@tonial tonial changed the title Admin: Correction d'un export [GEN-1861] Admin: Correction de l'export des entreprises [GEN-1861] Jul 12, 2024
@tonial tonial requested a review from xavfernandez July 12, 2024 14:12
@tonial tonial force-pushed the alaurent/fix_companies_admin_export branch from 8a3e99a to 7012a58 Compare July 12, 2024 14:13
Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Ça me semble très bien mais il faudrait rajouter un ou deux petits tests (idéalement avec un assertNumQueries pour vérifier que le select_related se fait bien.

itou/utils/export.py Outdated Show resolved Hide resolved
itou/utils/export.py Outdated Show resolved Hide resolved
@tonial tonial force-pushed the alaurent/fix_companies_admin_export branch from 7012a58 to 2634272 Compare July 13, 2024 04:40
Copy link

socket-security bot commented Jul 13, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
pypi/regex@2023.6.3 eval, unsafe 0 3.11 MB mrabarnett

🚮 Removed packages: pypi/diff-match-patch@20230430, pypi/diff-match-patch@20230430, pypi/django-import-export@4.1.0, pypi/django-import-export@4.1.0, pypi/tablib@3.5.0, pypi/tablib@3.5.0

View full report↗︎

@tonial
Copy link
Contributor Author

tonial commented Jul 13, 2024

C'est tout corrigé.

Je me demande en revanche si le Serializer est la meilleurs méthode pour gérer les exports de l'admin.
C'est sans doute la plus simple si on prévoit que ce sera ré-utilisé dans une autre page, mais ça ne permet pas de gérer l'export de EvaluationCampaignAdmin qui exporte un modèle lié.

Je vois 3 manières de faire les choses, et je veux bien ton avis :

  • soit je garde cette approche qui permet de faire un export simple depuis l'admin (on défini un serializer, et c'est fini)
  • soit on se dit que ça ne sert à rien de rendre le truc générique et je mets l'action directement dans CompanyAdmin
  • Soit on veut aussi gérer l'export EvaluationCampaignAdmin.export_siaes() qui va exporter une relation, et ça veut dire qu'il faut plutôt définir :
    • un header
    • un serializer plus bête sans DRF
    • une méthode get_export_queryset() qui par défaut utilise le queryset de la requête et qui permet de rajouter les select_related, et éventuellement de changer le queryset complètement comme dans EvaluationCampaignAdmin.export_siaes() )

@tonial tonial force-pushed the alaurent/fix_companies_admin_export branch 2 times, most recently from 5f38d2b to d80c19f Compare July 13, 2024 04:57
@tonial tonial requested a review from xavfernandez July 15, 2024 09:12
@xavfernandez
Copy link
Contributor

Je vois 3 manières de faire les choses, et je veux bien ton avis :

* soit je garde cette approche qui permet de faire un export simple depuis l'admin (on défini un serializer, et c'est fini)

* soit on se dit que ça ne sert à rien de rendre le truc générique et je mets l'action directement dans `CompanyAdmin`

* Soit on veut aussi gérer l'export `EvaluationCampaignAdmin.export_siaes()` qui va exporter une relation, et ça veut dire qu'il faut plutôt définir :
  
  * un header
  * un serializer plus bête sans DRF
  * une méthode `get_export_queryset()` qui par défaut utilise le queryset de la requête et qui permet de rajouter les select_related, et éventuellement de changer le queryset complètement comme dans  `EvaluationCampaignAdmin.export_siaes()` )

Si tu n'avais encore rien fait je t'aurais dit de partir sur la solution ad-hoc (la 2) mais maintenant que la 1 est faite on peut également la garder.

Après je suis allé voir l'action export_siaes du contrôle a posteriori et en vrai, je trouve qu'elle est assez efficace avec le queryset et les headers définis au même endroit avec la fonction de sérialisation juste au dessus: ça a le mérite d'être simple à comprendre 😬
Tant qu'on n'a pas beaucoup d'exports, c'est ptet encore ce qu'il y a de plus simple.

@tonial tonial force-pushed the alaurent/fix_companies_admin_export branch from d80c19f to 24b758d Compare July 15, 2024 13:17
@tonial
Copy link
Contributor Author

tonial commented Jul 15, 2024

merci pour l'avis,
J'ai donc refais en partant sur le plus simple: pas de serializer magique, tout dans la vue d'admin qui correspond.

@tonial tonial added this pull request to the merge queue Jul 15, 2024
Merged via the queue into master with commit 4ff6a68 Jul 15, 2024
11 checks passed
@tonial tonial deleted the alaurent/fix_companies_admin_export branch July 15, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants