Candidature: Stockage du CV dans un objet de la base de donnée [GEN-2443] by tonial · Pull Request #6087 · gip-inclusion/les-emplois

tonial · 2025-05-06T08:07:41Z

🤔 Pourquoi ?

Cela nous permettra a terme de mieux gérer les fichiers du S3

Le lien entre candidature et File est un OneToOneField pour s'assurer qu'on n'utilise pas le même fichier sur plusieurs candidatures.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?
Ajouter l'étiquette « Bug » ?

🏝️ 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

notion-workspace · 2025-05-06T08:07:45Z

🛠️ Meilleur gestion des CV dans la base de donnée

francoisfreitag

Ça m’a l’air très bien parti.

tonial · 2025-05-06T13:47:43Z

C'est bon, la première étape est prête.

Prochaine PR: lier toutes les candidatures existantes aux File correspondants.
Normalement il ne devrait pas y avoir de fichier manquant car tous les fichiers du S3 sont ajoutés en tant que File pour être scannés

francoisfreitag

Très content de voir du progrès sur ce sujet ☺️

francoisfreitag · 2025-05-06T14:21:14Z

+
+        new_key = str(pathlib.Path(self.key).with_stem(str(uuid.uuid4())))
+        copy_file(self.key, new_key)
+        return File.objects.create(key=new_key)


Suggested change

return File.objects.create(key=new_key)

return type(self).objects.create(key=new_key)

Je suis parti sur self.__class__ parce qu'il me semble que c'est plus la façon de faire de Django, mais je me trompe peut être.

francoisfreitag · 2025-05-06T14:26:30Z

+    mocker.patch(
+        "itou.files.models.uuid.uuid4",
+        return_value=uuid.UUID("22222222-2222-2222-2222-222222222222"),
+    )


On peut même faire sans mock, non ?

On vérifie juste que new_file.key != key. On pourrait aller jusqu’à définir content puis comparer le contenu de new_file avec content.

Je veux vérifier que seul le stem change (qu'on garde l'extension et le "resume/" au début)
Je pourrais utiliser pathlib, ou une regex, mais je trouvais ça aussi simple de mocker (comme on le fait dans le parcours de candidature)

Pas du tout convaincu par le mock d'un symbole de la stdlib mais seulement celui importé dans un module précis 😨.
Surtout que foncièrement on s'en fiche de tester que le stem change, ce qu'on veux vraiment tester c'est que .copy() crée un nouveau File() et ne réutilise pas le fichier d'origine donc que key change.

Pour moi, c'est important qu'on s'assure que le fichier est différent (key différent) mais aussi que l'extension et le repertoire est le même.
Sinon, on pourrait juste renommer le fichier en uuid.uuid4() et les personnes qui ne téléchargeraient aurait un fichier "illisible" car sans extension.
J'ai donc mis une regex

Je me note de nettoyer tous les mock similaires quand j'aurai retiré le champ resume_link

francoisfreitag · 2025-05-06T14:27:57Z

    hiring_end_at = factory.LazyFunction(lambda: datetime.now(UTC).date() + relativedelta(years=2))
-    resume_link = "https://server.com/rockie-balboa.pdf"
+    resume = factory.SubFactory(FileFactory)
+    resume_link = factory.LazyAttribute(lambda o: default_storage.url(o.resume.key))


Je me rends compte qu'il faut storages["public"] ici, je corrige

francoisfreitag · 2025-05-06T14:28:34Z

    expected_message = f"Le 15/07/2024 à 11h52, Pierre DUPONT a écrit :\n\n{job_application.message}"
    assert response.context["form"].initial["message"] == expected_message

+    mocker.patch("itou.files.models.copy_file", return_value=None)


Pourquoi mocker ? On a un S3 fonctionnel.

la factory ne crée pas le fichier sur le s3 ^^'
Je vais ajouter un post_generation pour le faire et éviter le mock

Bon, la copie via l'api s3 fait des trucs bizarre (la key est préfixée par un uuid, ce qui n'est pas le cas en prod, et c'est donc probablement lié à une séparation entre tests)
J'ai donc modifié le code pour se baser sur l'api storages de Django (on doit donc télécharger le fichier pour le saver à nouveau sous un autre nom).
Et j'ai du modifier test_copy car c'est pareil : en écrivant le fichier avec le client s3, impossible d'y accéder via storages.

Ah, c'est à cause de storage_prefix_per_test...
Vu le faible nombre de copie qu'on a à faire, je pense que je vais laisser l'utilisation de storages plutôt que du s3_client.

francoisfreitag · 2025-05-06T14:29:48Z

    # CHeck back_url
    assertContains(response, transfer_step_2_url)

+    mocker.patch("itou.files.models.copy_file", return_value=None)


Idem, on doit pouvoir faire sans mock. (et ce faisant, on doit pouvoir supprimer le test spécifique test_copy 🤷)

j'ai pu virer le mock, mais je propose de laisser test_copy qui s'assure que le contenu est le même

vincentporte

j'ai l'impression qu'il me manque une pièce du puzzle. La gestion des fichiers du S3 me semble plus simple sur la commu, sans trop savoir d'où ça vient.

vincentporte · 2025-05-07T09:08:48Z

+        if create and extracted:
+            public_storage = storages["public"]
+            public_storage.save(self.resume.key, extracted)
+


Que se passe-t-il qd un test utilise la factory sans le param with_file ?
resume contient la FK du File lié mais sans objet dans le S3 ?

Oui, c'est bien ça
À part quand on doit copier le fichier, on n'a pas besoin d'y accéder

Mais est-ce qu'on a besoin d'avoir un resume par défaut ?
Les tests explosent comment dans ce cas là ? 😁

Je ne sais pas, je veux bien regarder quand on aura basculé sur resume (il va y avoir beaucoup de tests à adapter)

vincentporte · 2025-05-07T09:14:26Z

+import pathlib
+import uuid
+
+from django.core.files.storage import default_storage


La commu utilise from storages.backends.s3boto3 import S3Boto3Storage, j'ai le souvenir qu'il n'y avait pas besoin d'utiliser default_storage.save() ni de déclarer public_storages puis public_storages.save().
Vous rappelez-vous des raisons du choix de default_storage ?

Dans les settings il y a :

STORAGES = { "default": { "BACKEND": "storages.backends.s3.S3Storage", }, "public": { "BACKEND": "itou.utils.storage.s3.PublicStorage", }, "staticfiles": { "BACKEND": "django.contrib.staticfiles.storage.ManifestStaticFilesStorage", }, }

Sachant que le PublicStorage, c'est un S3Storage avec querystring_auth = False

Du coup on permet à django de gérer les fichiers sans avoir besoin de tout faire à la main.
Ça nous permet aussi de "tricher" en ajoutant un sous répertoire dans le bucket pour compartimenter les fichiers des tests grâce à cette fixture

from storages.backends.s3boto3 import S3Boto3Storage est un alias sur storages.backends.s3.S3Storage donc on utilise exactement la même chose que la communauté en dessous.

rsebille

A priori j'avais pas envoyé, j'espère n'avoir rien oublié ou perdu :/

rsebille · 2025-05-07T15:36:54Z

+    mocker.patch(
+        "itou.files.models.uuid.uuid4",
+        return_value=uuid.UUID("22222222-2222-2222-2222-222222222222"),
+    )


Pas du tout convaincu par le mock d'un symbole de la stdlib mais seulement celui importé dans un module précis 😨.
Surtout que foncièrement on s'en fiche de tester que le stem change, ce qu'on veux vraiment tester c'est que .copy() crée un nouveau File() et ne réutilise pas le fichier d'origine donc que key change.

rsebille · 2025-05-07T15:41:07Z

+        if create and extracted:
+            public_storage = storages["public"]
+            public_storage.save(self.resume.key, extracted)
+


Mais est-ce qu'on a besoin d'avoir un resume par défaut ?
Les tests explosent comment dans ce cas là ? 😁

tonial requested review from francoisfreitag and vincentporte May 6, 2025 08:07

tonial self-assigned this May 6, 2025

tonial added the modifié Modifié dans le changelog. label May 6, 2025

tonial marked this pull request as draft May 6, 2025 08:16

francoisfreitag approved these changes May 6, 2025

View reviewed changes

francoisfreitag reviewed May 6, 2025

View reviewed changes

Comment thread itou/www/apply/views/process_views.py Outdated

tonial force-pushed the alaurent/job_applications_cv branch from e7f04e6 to 7e85902 Compare May 6, 2025 13:43

tonial marked this pull request as ready for review May 6, 2025 13:46

tonial requested a review from francoisfreitag May 6, 2025 14:00

francoisfreitag reviewed May 6, 2025

View reviewed changes

tonial force-pushed the alaurent/job_applications_cv branch from 7e85902 to 3bf6838 Compare May 6, 2025 20:00

vincentporte reviewed May 7, 2025

View reviewed changes

tonial force-pushed the alaurent/job_applications_cv branch from 3bf6838 to ad7281e Compare May 8, 2025 12:34

rsebille reviewed May 12, 2025

View reviewed changes

job_applications: Add ForeignKey to files.File

af7264b

tonial force-pushed the alaurent/job_applications_cv branch 2 times, most recently from f1cddfc to 4ab16d5 Compare May 12, 2025 07:37

xavfernandez approved these changes May 12, 2025

View reviewed changes

Comment thread tests/job_applications/factories.py Outdated

tonial added 3 commits May 12, 2025 10:35

job_applications: Adapt factory with new resume field

c116323

files: allow to copy a File

eb5b1e4

apply: File job_application.resume field

44f8d45

tonial force-pushed the alaurent/job_applications_cv branch from 4ab16d5 to 44f8d45 Compare May 12, 2025 08:35

tonial added this pull request to the merge queue May 12, 2025

Merged via the queue into master with commit a2ce41b May 12, 2025

tonial deleted the alaurent/job_applications_cv branch May 12, 2025 09:48

	return File.objects.create(key=new_key)
	return type(self).objects.create(key=new_key)

Uh oh!

Conversation

tonial commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester ?

💻 Captures d'écran

Uh oh!

notion-workspace Bot commented May 6, 2025

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonial commented May 6, 2025

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsebille left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial commented May 6, 2025 •

edited

Loading

tonial May 6, 2025 •

edited

Loading

tonial May 6, 2025 •

edited

Loading

tonial May 12, 2025 •

edited

Loading

tonial May 7, 2025 •

edited

Loading