Recherche employeur : Pouvoir sauvegarder ma recherche et y accéder depuis l'accueil [GEN-2445]

[EwenKorr]

🤔 Pourquoi ?

Retrouver ses recherches fréquentes aisément.

🍰 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]

🌿 Pouvoir sauvegarder ma recherche et y accéder depuis l’accueil

[github-actions]

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

[EwenKorr]

Petite question @hellodeloo :

Soit ce code :

les-emplois/itou/templates/search/siaes_search_results.html

Lines 35 to 43 in 1175c65

	<div class="c-search mt-3 mt-md-4">
	<div class="c-search__bar">
	<form>
	{% include "search/includes/siaes_search_form.html" with form=form is_home=False only %}
	{% if job_seeker %}<input type="hidden" name="job_seeker_public_id" value="{{ job_seeker.public_id }}">{% endif %}
	</form>
	</div>
	</div>
	{% include "search/includes/saved_searches.html" with saved_searches=saved_searches only %}

Pour des raisons liées à HTMX, j'ai besoin d'avoir le div id="savedSearchesList" dans la page, même s'il ne contient rien, pour pouvoir le remplir dynamiquement quand une recherche est sauvegardée. (exemple live)

Mais l'ajout de ce div augmente inutilement les marges :

(sur un grand écran on voit davantage la différence)

Qu'est-ce que tu conseillerais de faire ? Diminuer/supprimer la marge du div suivant peut-être ?

[github-actions]

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

[github-actions]

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

[leo-naeka]

Good !

Quelques retours, à dispo pour en discuter si tu le souhaites.
Fonctionnellement c'est un bon premier jet, je trouve top !

[leo-naeka]

Django crée maintenant les validateurs pour les contraintes de manière implicite, comme il n'y a pas besoin de l'interpoler, l'erreur peut être définie avec violation_error_message.

[EwenKorr]

Oui, merci. J'ai un souci cela dit : avec le violation_error_message et sans le clean_name du formulaire, la ValidationError n'est pas levée. Donc on se retrouve avec une 500 en IntegrityError. 🤔

[xavfernandez]

Je pense que comme user n'est pas dans le formulaire _get_validation_exclusions va exclure user de la validation automatique et ta contrainte va se faire ignorer.
Mais tu peux tout de même appeler validate_constraints dans le clean

[EwenKorr]

En effet. Mes essais :

• on pourrait mettre user dans les champs du formulaire, mais c'est pas top
• dans le clean du formulaire : l'objet n'est pas encore instancié, on ne peut pas valider les contraintes
• dans le clean du modèle : user n'y est pas encore, on ne le rajoute qu'après
• dans le save du formulaire, après le self.instance.user = self.user : les exceptions ne sont plus attrapées pour être affichées dans le formulaire il me semble
• la doc recommande de surcharger Model.clean_fields
  Sauf que là encore, on ne rajoute user qu'après.

Beaucoup de tests pour finalement me dire qu'il faut modifier l'instance plus tôt, et valider les contraintes systématiquement avec user. Donc :

• je mets à jour l'instance dès le __init__
• je surcharge SavedSearch.validate_constraints pour ne pas exclure user

En pratique, ça marche 🎉
Ça vous convient aussi ?

[leo-naeka]

Tu soulèves un bon point et m'a donné l'occasion d'aller fouiner un peu dans le code.

Je remarque que Django ne fait pas de validation implicite dans les cas suivants :

• Un des champs composant la contrainte n'est pas dans le form
• La contrainte a une condition
• La contrainte se base sur des expressions plutôt que les champs

https://github.com/django/django/blob/6e3287408e128e03a6ec5d23c17cdc2eee8760c0/django/db/models/base.py#L1441-L1507
https://github.com/django/django/blob/6e3287408e128e03a6ec5d23c17cdc2eee8760c0/django/db/models/options.py#L976-L990

L'approche de @xavfernandez retenue dans _get_validation_exclusions marche, mais je me demande si l'on ne souhaiterait pas plutôt le faire de manière explicite en le gérant dans le clean (d'autant que ta proposition initiale n'est pas plus "lourde" que le retrait de user dans _get_validation_exclusions).

Désolé pour la perte de temps, tu m'as permis d'apprendre un truc aujourd'hui 🤗

[EwenKorr]

Mmmh…

self.validate_constraints() : AttributeError: 'NewSavedSearchForm' object has no attribute 'validate_constraints' (je ne comprends pas l'erreur d'ailleurs, car BaseModelForm a bien une méthode validate_constraints)

Et de toute façon cela réappellerait _get_validation_exclusion.

self.instance.validate_constraints() : à cette étape, l'instance n'est pas encore complète, il n'y a que le user d'assigné (dans le __init__). Le reste des données est dans form.cleaned_data, que validate_constraints ne regarde pas.

Dans le clean du modèle par contre, ça fonctionne car l'objet est complet.
C'est assez proche de ce qu'on fait pour User, où on appelle validate_constraints dans le Model.save().

[xavfernandez]

Alors à la fin du _post_clean ?

[xavfernandez]

diff --git a/itou/www/search_views/forms.py b/itou/www/search_views/forms.py
index f93b3e42f..89608442f 100644
--- a/itou/www/search_views/forms.py
+++ b/itou/www/search_views/forms.py
@@ -176,14 +176,6 @@ class NewSavedSearchForm(forms.ModelForm):
             "query_params": forms.HiddenInput,
         }
 
-    def _get_validation_exclusions(self):
-        exclude = super()._get_validation_exclusions()
-        if "user" in exclude:
-            # Make sure UniqueConstraint(fields=["user", "name"], ...) is validated in form
-            # despite user field not being in the form
-            exclude.remove("user")
-        return exclude
-
     def __init__(self, *args, user, **kwargs):
         super().__init__(*args, **kwargs)
         self.instance.user = user
@@ -195,3 +187,11 @@ class NewSavedSearchForm(forms.ModelForm):
             raise forms.ValidationError(
                 f"Le nombre maximum de recherches enregistrées ({MAX_SAVED_SEARCHES_COUNT}) a été atteint."
             )
+
+    def _post_clean(self):
+        super()._post_clean()
+        try:
+            # Validate UniqueConstraint(fields=["user", "name"], ...) despite user field not being in the form
+            self.instance.validate_constraints()
+        except forms.ValidationError as e:
+            self._update_errors(e)

Je ne saurais pas dire quelle version est la meilleure 😅

Potentiellement ta version initiale qui faisait un simple SavedSearch.objects.filter(user=self.user, name=name).exists() 😜

[EwenKorr]

   def _post_clean(self):
       super()._post_clean()
       self.instance.validate_constraints()

Oui, la bonne exception est levée, mais elle n'est pas attrapée dans le formulaire 😬

[EwenKorr]

Aah ok, avec un _update_errors 👍

Je ne saurais pas dire quelle version est la meilleure 😅

Réutiliser le message d'erreur de la contrainte est quand même plus DRY.

Je vais garder ta dernière proposition !

[xavfernandez]

Ça me semble bien. Peut-être attendre un second avis de @leo-naeka ?

[EwenKorr]

Ça me semble bien.

Merci pour tes relectures 🙏

Peut-être attendre un second avis de @leo-naeka ?

Oui, de toute façon j'attends encore que les UX/métiers se décident sur la terminologie, comme soulevé par @leo-naeka.

[leo-naeka]

Suggested change

	"disable_save_button": saved_searches and saved_searches.count() >= MAX_SAVED_SEARCHES_COUNT,
	"disable_save_button": saved_searches and len(saved_searches) >= MAX_SAVED_SEARCHES_COUNT,

Comme on l'a déjà évalué dans SavedSearch.add_city_name_attr, on peut s'épargner une requête ici ;)

Et selon la ce qui est tranché ici, on peut encore simplifier :

Suggested change

	"disable_save_button": saved_searches and saved_searches.count() >= MAX_SAVED_SEARCHES_COUNT,
	"disable_save_button": len(saved_searches) >= MAX_SAVED_SEARCHES_COUNT,

[EwenKorr]

Donc, une fois casté en liste ici, on peut effectivement toujours utiliser len(saved_searches) sans vérifier que saved_searches n'est pas None.

Dans les autres vues, j'ai aussi enlevé la vérification :

@@ -372,7 +372,7 @@ def add_saved_search(request):
         "form": form,
         "saved_searches": saved_searches,
         "display_save_button": True,
-        "disable_save_button": saved_searches and saved_searches.count() >= MAX_SAVED_SEARCHES_COUNT,
+        "disable_save_button": len(saved_searches) >= MAX_SAVED_SEARCHES_COUNT,
         "hx_swap_oob": False,
     }
 
@@ -398,7 +398,7 @@ def delete_saved_search(request):
         context={
             "saved_searches": saved_searches,
             "display_save_button": True,
-            "disable_save_button": saved_searches and saved_searches.count() >= MAX_SAVED_SEARCHES_COUNT,
+            "disable_save_button": len(saved_searches) >= MAX_SAVED_SEARCHES_COUNT,
             "hx_swap_oob": False,
         },
         headers=headers,

vu que saved_searches = SavedSearch.add_city_name_attr(request.user.saved_searches.all()), on n'a que du QuerySet, pas du None si je comprends bien.

[leo-naeka]

Pas bien fan de mêler list et QuerySet ici.

Si on voulait préserver des querysets, dans le premier cas tu aurais pu :

Suggested change

	saved_searches = self.request.user.saved_searches.all() if self.request.user.is_authenticated else []
	saved_searches = SavedSearch.add_city_name_attr(saved_searches)
	saved_searches = self.request.user.saved_searches.all() if self.request.user.is_authenticated else SavedSearch.objects.none()

QuerySet.none() ne fait pas de requête et ça s'évalue à False.

Mais dans le cas présent va toutes les lister, on les retraite dans add_city_name_attr, il y en a 20 au max. Bon… par simplicité, je serais d'avis de cast en list pour "annoter" city_name 🤷‍♂️

[EwenKorr]

Bien vu ! Je cast en liste alors.

[leo-naeka]

Top ! 👏

Y'a juste le add_city_name_attr qui me revient pas trop, j'aurais p-e dénormalisé et stocké le nom de la ville pour simplement annotate. Mais bon... c'est pour un besoin d'interface susceptible d'évoluer, c'est très bien ainsi :)

[leo-naeka]

Je ferais plutôt le cast à un seul endroit dans SavedSearch.add_city_name_attr car il est facile d'oublier : https://github.com/gip-inclusion/les-emplois/pull/6757/files#diff-b606b7c5ce6a7bb9b226c4ace363625e83aebd739d1658e5a56cb78acd904846R369

Idéalement pour éviter le polymorphisme, il faudrait ici :

Suggested change

	saved_searches = list(self.request.user.saved_searches.all()) if self.request.user.is_authenticated else []
	saved_searches = self.request.user.saved_searches.all() if self.request.user.is_authenticated else SavedSearch.objects.none()

Puis la signature de SavedSearch.add_city_name_attr serait la suivante :

    def add_city_name_attr(saved_searches: models.QuerySet) -> list[SavedSearch]:
        saved_searches = list(saved_searches)
        # …
        return saved_searches

Qu'on soit d'accord, ça change pas grand chose fondamentalement, mais je préfère passer une liste au contexte qu'un queryset évalué, dont les objets cachés ont été annotés en python.

[EwenKorr]

Je comprends, merci ! Je corrige ainsi. :)

[EwenKorr]

Y'a juste le add_city_name_attr qui me revient pas trop, j'aurais p-e dénormalisé et stocké le nom de la ville pour simplement annotate. Mais bon... c'est pour un besoin d'interface susceptible d'évoluer, c'est très bien ainsi :)

Je vais fusionner tel quel mais je suis curieux de mieux comprendre ce que tu écris à l'occasion.
Tu suggères de ne pas caster en liste et utiliser annotate() en gros ?

[leo-naeka]

Je vais fusionner tel quel mais je suis curieux de mieux comprendre ce que tu écris à l'occasion. Tu suggères de ne pas caster en liste et utiliser annotate() en gros ?

1. Par principe, je préfère éviter de modifier les objets cachés d’un QuerySet, même si la plupart des méthodes retournent un clone

2. J’aime bien l’idée d’annoter pour disposer systématiquement de l’info, quel que soit le QuerySet évalué. Cela reviendrait à stocker la ville en base à la création et à la réutiliser telle quelle ensuite, alors qu’actuellement on la recalcule à chaque affichage.
   Mais comme la fonctionnalité peut encore évoluer, l'approche actuelle convient très bien 👍

3. Enfin, même si Python reste permissif avec son duck typing (on manipule simplement des itérables ici), dans la mesure où tu as adressé le point 1, je préfère garder un type d’objet cohérent en entrée comme en sortie