manager: make agenda's groups foldable (#85616) #230

Merged
yweber merged 1 commits from wip/85616-make-categories-foldable into main 2024-04-15 14:49:40 +02:00
Owner
No description provided.
yweber added 1 commit 2024-03-21 12:12:27 +01:00
yweber force-pushed wip/85616-make-categories-foldable from b1378c5925 to 86e25f5fd2 2024-03-21 12:12:58 +01:00 Compare
yweber changed title from WIP: manager: make agenda's groups foldable (#85616) to manager: make agenda's groups foldable (#85616) 2024-03-21 12:26:40 +01:00
fpeters requested changes 2024-04-01 21:10:40 +02:00
fpeters left a comment
Owner

Pour que data-section-folded-pref-name fonctionne, pour mémoriser ce qui est dé/plié, il faut également la partie js (présente dans wcs/qommon/static/js/qommon.admin.js) et l'API qui l'accompagne (/api/user/preferences), ce qui signifiera aussi l'ajout du modèle pour stocker ces préférences.

Pour que data-section-folded-pref-name fonctionne, pour mémoriser ce qui est dé/plié, il faut également la partie js (présente dans wcs/qommon/static/js/qommon.admin.js) et l'API qui l'accompagne (/api/user/preferences), ce qui signifiera aussi l'ajout du modèle pour stocker ces préférences.
yweber changed title from manager: make agenda's groups foldable (#85616) to wip: manager: make agenda's groups foldable (#85616) 2024-04-02 09:54:22 +02:00
yweber force-pushed wip/85616-make-categories-foldable from 86e25f5fd2 to c5a2671569 2024-04-02 18:31:44 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from c5a2671569 to e24bf800c5 2024-04-03 09:17:13 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from e24bf800c5 to 2393f93660 2024-04-03 10:34:43 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 2393f93660 to 0c72706ae1 2024-04-03 10:41:16 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 0c72706ae1 to 49c5174ac4 2024-04-03 10:50:48 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 49c5174ac4 to 617eab9492 2024-04-03 11:14:38 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 617eab9492 to 140be56c5b 2024-04-03 11:17:05 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 140be56c5b to f81dfa9ca6 2024-04-03 11:18:23 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from f81dfa9ca6 to 758ef73ae8 2024-04-03 11:23:08 +02:00 Compare
yweber changed title from wip: manager: make agenda's groups foldable (#85616) to manager: make agenda's groups foldable (#85616) 2024-04-03 11:36:08 +02:00
yweber requested review from fpeters 2024-04-03 11:36:14 +02:00
vdeniaud requested changes 2024-04-10 15:32:16 +02:00
vdeniaud left a comment
Owner

Quelques idées de trucs qui peuvent être améliorés

Quelques idées de trucs qui peuvent être améliorés
@ -156,4 +156,5 @@ urlpatterns = [
path('statistics/', views.statistics_list, name='api-statistics-list'),
path('statistics/bookings/', views.bookings_statistics, name='api-statistics-bookings'),
path('ants/', include('chrono.apps.ants_hub.api_urls')),
path('user_preferences/', include('chrono.apps.user_preferences.api_urls')),
Owner

On préfère les - aux _ dans les URL

On préfère les - aux _ dans les URL
Author
Owner

Ok ! Fait :)

Ok ! Fait :)
yweber marked this conversation as resolved
@ -0,0 +26,4 @@
@csrf_exempt
def save_preference(request):
'''Save a user preference
Owner

Ce serait mieux sans docstring, dans chrono elles sont plutôt le signe de code imbitable autour de la génération de créneau à la volée :)

Ce serait mieux sans docstring, dans chrono elles sont plutôt le signe de code imbitable autour de la génération de créneau à la volée :)
Author
Owner

C'est vraiment la docstring qui t'ennuie ?

def save_preference(request):
    # Expect a request with a json dict of len 1 as body { str: bool }

Te conviendrais mieux ?

C'est vraiment la docstring qui t'ennuie ? ```python def save_preference(request): # Expect a request with a json dict of len 1 as body { str: bool } ``` Te conviendrais mieux ?
Owner

Nope c'était bien le principe d'avoir un commentaire pour décrire une méthode, perso je trouve que ça devrait être réservé aux endroits compliqués (et du point de vue de l'homogénéité avec ce qui est fait dans chrono, ça l'est à quelques exceptions près)

Nope c'était bien le principe d'avoir un commentaire pour décrire une méthode, perso je trouve que ça devrait être réservé aux endroits compliqués (et du point de vue de l'homogénéité avec ce qui est fait dans chrono, ça l'est à quelques exceptions près)
Author
Owner

Ok ! Personnellement je ne trouve pas ça très pratique d'autant qu'avec le retrait des tests qui étaient fait sur le contenu il devient compliqué de trouver ce que cette API attend. Mais c'est parti j’homogénéise ;)

Ok ! Personnellement je ne trouve pas ça très pratique d'autant qu'avec le retrait des tests qui étaient fait sur le contenu il devient compliqué de trouver ce que cette API attend. Mais c'est parti j’homogénéise ;)
yweber marked this conversation as resolved
@ -0,0 +31,4 @@
Given a JSON with a single boolean value identified by a str, save the
key <-> value association in UserPreference
'''
if not request.user or not request.user.is_authenticated:
Owner

Pour faire ça tu peux utiliser le décorateur login_required

Pour faire ça tu peux utiliser le décorateur `login_required`
Author
Owner

Merci !

Ca convient que le décorateur renvoie une 302 au lieu d'une 403 ? (vu l'URL en question j'aurais penché pour la 403)

Merci ! Ca convient que le décorateur renvoie une 302 au lieu d'une 403 ? (vu l'URL en question j'aurais penché pour la 403)
Owner

Oui il n'y a pas de raison d'être poli avec les bots :)

Oui il n'y a pas de raison d'être poli avec les bots :)
yweber marked this conversation as resolved
@ -0,0 +33,4 @@
'''
if not request.user or not request.user.is_authenticated:
raise PermissionDenied()
user_pref = models.UserPreferences.objects.get_or_create(user=request.user)[0]
Owner

Je vois que plus bas tu utilises la forme user_preferences, _, pour info ici tu peux aussi faire ça et ça donnerait user_preferences, dummy (dummy étant un mot clé que pylint ignore au moment de t'engueuler parce que tu as des variables inutilisées)

Je vois que plus bas tu utilises la forme `user_preferences, _`, pour info ici tu peux aussi faire ça et ça donnerait `user_preferences, dummy` (dummy étant un mot clé que pylint ignore au moment de t'engueuler parce que tu as des variables inutilisées)
Author
Owner

Merci ! J'avais effectivement pas le cheatcode du dummy ( et _ était déjà utilisé pour gettext ;) )

Merci ! J'avais effectivement pas le cheatcode du `dummy` ( et `_` était déjà utilisé pour gettext ;) )
yweber marked this conversation as resolved
@ -0,0 +36,4 @@
user_pref = models.UserPreferences.objects.get_or_create(user=request.user)[0]
if len(request.body) > 1000:
return HttpResponse(_('Payload is too large').encode(), status=400)
Owner

Le .encode() n'est pas nécessaire

Le `.encode()` n'est pas nécessaire
Author
Owner

En effet ! Merci :)

En effet ! Merci :)
yweber marked this conversation as resolved
@ -0,0 +45,4 @@
if not isinstance(prefs, dict) or len(prefs) != 1:
return bad_fmt_response
name, value = list(prefs.items())[0]
if not isinstance(name, str) or not isinstance(value, bool):
Owner

Perso ça m'irait de coller à ce qui est fait côté wcs, avec un bête user_pref.update(prefs) sans chercher à en savoir trop sur ce que contient prefs, la vraie sécurité étant la vérification de la taille du payload plus haut.

En gardant juste la vérification que prefs est un dictionnaire, qui évite effectivement de crasher (je ne sais pas si tu as le contexte mais ici l'enjeu c'est de se "protéger" des outils automatisés que les gens lancent sur Publik lors des audits de sécu, et qui balancent n'importe quoi sur toutes les URL, et nous inondent de traces)

Perso ça m'irait de coller à ce qui est fait côté wcs, avec un bête `user_pref.update(prefs)` sans chercher à en savoir trop sur ce que contient prefs, la vraie sécurité étant la vérification de la taille du payload plus haut. En gardant juste la vérification que prefs est un dictionnaire, qui évite effectivement de crasher (je ne sais pas si tu as le contexte mais ici l'enjeu c'est de se "protéger" des outils automatisés que les gens lancent sur Publik lors des audits de sécu, et qui balancent n'importe quoi sur toutes les URL, et nous inondent de traces)
Author
Owner

Autant je comprend que la vérification du type de la clé soit superflu (je l'ai enlevé), autant je trouverais ça potentiellement embêtant qu'on stock autre chose qu'un booléen (un autre dict ?) dans ce JSONField qui n'a aucune garantie forte de ne pas grossir à l'infini.

Autant je comprend que la vérification du type de la clé soit superflu (je l'ai enlevé), autant je trouverais ça potentiellement embêtant qu'on stock autre chose qu'un booléen (un autre dict ?) dans ce JSONField qui n'a aucune garantie forte de ne pas grossir à l'infini.
Owner

embêtant qu'on stock autre chose qu'un booléen

Au contraire c'est bien pratique de pouvoir stocker autre chose; par exemple on pourrait imaginer une évolution où l'agent peut réordonner les catégories, et la valeur pourrait être une liste des id de catégories; l'idée côté wcs de ces "préférences" était d'offrir cette liberté d'utilisations futures.

> embêtant qu'on stock autre chose qu'un booléen Au contraire c'est bien pratique de pouvoir stocker autre chose; par exemple on pourrait imaginer une évolution où l'agent peut réordonner les catégories, et la valeur pourrait être une liste des id de catégories; l'idée côté wcs de ces "préférences" était d'offrir cette liberté d'utilisations futures.
Author
Owner

par exemple on pourrait imaginer une évolution

Oui désolé, je me suis mal exprimé, je voulais dire que je trouverai ça potentiellement embêtant qu'on stock autre chose qu'un booléen alors que ce n'est pas prévu.

L'idée était bien de limiter l'utilisation tout en laissant la porte entre-ouverte aux évolutions futures, mais comme ça semble vous convenir à tous les deux je vais la laisser grande ouverte ( navré pour l'analogie pourrie, j'ai pas trouvé mieux dans l'instant ;) ).

> par exemple on pourrait imaginer une évolution Oui désolé, je me suis mal exprimé, je voulais dire que je trouverai ça potentiellement embêtant qu'on stock autre chose qu'un booléen alors que ce n'est pas prévu. L'idée était bien de limiter l'utilisation tout en laissant la porte entre-ouverte aux évolutions futures, mais comme ça semble vous convenir à tous les deux je vais la laisser grande ouverte ( navré pour l'analogie pourrie, j'ai pas trouvé mieux dans l'instant ;) ).
yweber marked this conversation as resolved
@ -0,0 +23,4 @@
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, null=True)
preferences = models.JSONField(_('Preferences'), default=dict)
def get_preference(self, name):
Owner

Ça me paraîtrait plus pythonique de faire sans ces getter et setter d'une ou deux lignes, et manipuler directement l'attribut preferences ailleurs

Ça me paraîtrait plus pythonique de faire sans ces getter et setter d'une ou deux lignes, et manipuler directement l'attribut `preferences` ailleurs
Author
Owner

Plus pythonique je ne sais pas, mais si je vois comme avantage à la suppression de ces méthodes l'économie de 8 lignes de codes, j'y trouve aussi l'énorme inconvénient de perdre "l'explication" de la manière dont ce modèle est censé être utilisé.

Mais j'identifie cette problématique à celle des commentaires : il suffit de git grep UserPreferences et regarder partout ou elle est utilisée pour en déduire la manière dont on est censé l'utiliser (je me suis retenu fort pour ne pas indiquer que ce modèle ne servait qu'a stocker les préférences de fold etc.) ;)

De mon côté c'est ce JSONField qui me faisait un peu peur (et qui m'a poussé à inviter à limiter l'utilisation de ce champ), comme visiblement mes craintes ne sont pas partagés je supprime les deux méthodes !

Plus pythonique je ne sais pas, mais si je vois comme avantage à la suppression de ces méthodes l'économie de 8 lignes de codes, j'y trouve aussi l'énorme inconvénient de perdre "l'explication" de la manière dont ce modèle est censé être utilisé. Mais j'identifie cette problématique à celle des commentaires : il suffit de git grep UserPreferences et regarder partout ou elle est utilisée pour en déduire la manière dont on est censé l'utiliser (je me suis retenu fort pour ne pas indiquer que ce modèle ne servait qu'a stocker les préférences de fold etc.) ;) De mon côté c'est ce JSONField qui me faisait un peu peur (et qui m'a poussé à inviter à limiter l'utilisation de ce champ), comme visiblement mes craintes ne sont pas partagés je supprime les deux méthodes !
Owner

il suffit de git grep UserPreferences et regarder partout ou elle est utilisée pour en déduire la manière dont on est censé l'utiliser

Yep tu lis dans mes pensées, d'ailleurs dans cette situation plutôt que git grep je git log -SUserPreferences et je lis le commit qui l'a ajouté

De mon côté c'est ce JSONField qui me faisait un peu peur (et qui m'a poussé à inviter à limiter l'utilisation de ce champ), comme visiblement mes craintes ne sont pas partagés je supprime les deux méthodes !

En vrai pour moi l'évolution du code, si c'est étendu à d'autres préférences, ce sera d'ajouter de nouveaux champs aux modèles, et pas de stocker tout et n'importe quoi dans le JSONField (une stratégie différente de wcs où on est moins à l'aise à faire évoluer les modèles), et ce champ preferences sera renommé en un truc plus explicite, genre category_folding_states. Mais ce serait peut-être un peu trop anticiper que de faire ça maintenant, donc la version actuelle me va. (juste une simplification ça pourrait être de stocker {category_id: True/False} dans le JSON, plutôt que {'prefixe-long-category_id': True/False}, car dans l'évolution que j'imagine le préfixe ne sera jamais utile)

> il suffit de git grep UserPreferences et regarder partout ou elle est utilisée pour en déduire la manière dont on est censé l'utiliser Yep tu lis dans mes pensées, d'ailleurs dans cette situation plutôt que git grep je `git log -SUserPreferences` et je lis le commit qui l'a ajouté > De mon côté c'est ce JSONField qui me faisait un peu peur (et qui m'a poussé à inviter à limiter l'utilisation de ce champ), comme visiblement mes craintes ne sont pas partagés je supprime les deux méthodes ! En vrai pour moi l'évolution du code, si c'est étendu à d'autres préférences, ce sera d'ajouter de nouveaux champs aux modèles, et pas de stocker tout et n'importe quoi dans le JSONField (une stratégie différente de wcs où on est moins à l'aise à faire évoluer les modèles), et ce champ preferences sera renommé en un truc plus explicite, genre category_folding_states. Mais ce serait peut-être un peu trop anticiper que de faire ça maintenant, donc la version actuelle me va. (juste une simplification ça pourrait être de stocker {category_id: True/False} dans le JSON, plutôt que {'prefixe-long-category_id': True/False}, car dans l'évolution que j'imagine le préfixe ne sera jamais utile)
Author
Owner

En vrai pour moi l'évolution du code, si c'est étendu à d'autres préférences, ce sera d'ajouter de nouveaux champs aux modèles, et pas de stocker tout et n'importe quoi dans le JSONField (une stratégie différente de wcs où on est moins à l'aise à faire évoluer les modèles), et ce champ preferences sera renommé en un truc plus explicite, genre category_folding_states.

Oui c'est bien ce que j'imaginais aussi de mon côté.

(juste une simplification ça pourrait être de stocker {category_id: True/False} dans le JSON, plutôt que {'prefixe-long-category_id': True/False}, car dans l'évolution que j'imagine le préfixe ne sera jamais utile)

Je me disais que le prefix pourrait être pratique si on voulait stocker d'autres préférences foldable ? Après j'ai repris un prefix identique à wcs, mais peut être qu'un prefix plus parlant serait mieux ? foldable-manager-category-group- ?

> En vrai pour moi l'évolution du code, si c'est étendu à d'autres préférences, ce sera d'ajouter de nouveaux champs aux modèles, et pas de stocker tout et n'importe quoi dans le JSONField (une stratégie différente de wcs où on est moins à l'aise à faire évoluer les modèles), et ce champ preferences sera renommé en un truc plus explicite, genre category_folding_states. Oui c'est bien ce que j'imaginais aussi de mon côté. >(juste une simplification ça pourrait être de stocker {category_id: True/False} dans le JSON, plutôt que {'prefixe-long-category_id': True/False}, car dans l'évolution que j'imagine le préfixe ne sera jamais utile) Je me disais que le prefix pourrait être pratique si on voulait stocker d'autres préférences foldable ? Après j'ai repris un prefix identique à wcs, mais peut être qu'un prefix plus parlant serait mieux ? `foldable-manager-category-group-` ?
Owner

Oui t'as raison gardons les préfixes, et le nom que tu suggères est clairement mieux

(btw dans le patch il reste encore ces méthodes même si elles ne sont plus utilisées)

Oui t'as raison gardons les préfixes, et le nom que tu suggères est clairement mieux (btw dans le patch il reste encore ces méthodes même si elles ne sont plus utilisées)
Author
Owner

Oui t'as raison gardons les préfixes, et le nom que tu suggères est clairement mieux

Ok ! Fait :)

(btw dans le patch il reste encore ces méthodes même si elles ne sont plus utilisées)

En effet -_- Le boulet....

> Oui t'as raison gardons les préfixes, et le nom que tu suggères est clairement mieux Ok ! Fait :) > (btw dans le patch il reste encore ces méthodes même si elles ne sont plus utilisées) En effet -_- Le boulet....
yweber marked this conversation as resolved
@ -29,3 +19,1 @@
</li>
{% endfor %}
</ul>
{% with forloop.counter|stringformat:"s" as i %}
Owner

Pourquoi ne pas se baser sur l'id de la catégorie ? (tu l'as dans group.grouper.id)

Pourquoi ne pas se baser sur l'id de la catégorie ? (tu l'as dans group.grouper.id)
Author
Owner

J'avais compris que la logique dans wcs c'était d'utiliser des identifiants HTML propices aux collisions pour ne pas avoir à nettoyer les préférences.

Ici, au pire, on stockera X préférences avec X le nombre maximal de catégories ayant existé un jour (le bémol étant que la suppression/ajout d'une catégorie "décale" les préférences).

J'avais compris que la logique dans wcs c'était d'utiliser des identifiants HTML propices aux collisions pour ne pas avoir à nettoyer les préférences. Ici, au pire, on stockera X préférences avec X le nombre maximal de catégories ayant existé un jour (le bémol étant que la suppression/ajout d'une catégorie "décale" les préférences).
Owner

J'avais compris que la logique dans wcs c'était d'utiliser des identifiants HTML propices aux collisions pour ne pas avoir à nettoyer les préférences.

Ici, au pire, on stockera X préférences avec X le nombre maximal de catégories ayant existé un jour (le bémol étant que la suppression/ajout d'une catégorie "décale" les préférences).

C'est un gros bémol, ça veut dire qu'un agent qui gagne un accès à une nouvelle catégorie voit sa configuration chamboulée; il faut utiliser un identifiant stable (comme l'id de l'objet) pour faire référence à la catégorie.

> J'avais compris que la logique dans wcs c'était d'utiliser des identifiants HTML propices aux collisions pour ne pas avoir à nettoyer les préférences. > > Ici, au pire, on stockera X préférences avec X le nombre maximal de catégories ayant existé un jour (le bémol étant que la suppression/ajout d'une catégorie "décale" les préférences). C'est un gros bémol, ça veut dire qu'un agent qui gagne un accès à une nouvelle catégorie voit sa configuration chamboulée; il faut utiliser un identifiant stable (comme l'id de l'objet) pour faire référence à la catégorie.
Author
Owner

C'est un gros bémol

On est d'accord :) Du coup zut, j'ai mal saisie comment c'est fait dans wcs :/ Je patch ça !

> C'est un gros bémol On est d'accord :) Du coup zut, j'ai mal saisie comment c'est fait dans wcs :/ Je patch ça !
yweber marked this conversation as resolved
@ -0,0 +10,4 @@
def test_user_preferences_api_ok(app, admin_user):
UserPreferences.objects.all().delete()
Owner

Pas besoin, les transactions sont automatiquement rollback entre les tests, contrairement à wcs

Pas besoin, les transactions sont automatiquement rollback entre les tests, contrairement à wcs
Author
Owner

Cool ! Merci :)

Cool ! Merci :)
yweber marked this conversation as resolved
@ -0,0 +20,4 @@
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref
assert fake_id in user_pref.preferences
assert user_pref.preferences[fake_id] is True
Owner

Cette assertion couvre les deux précédentes, pour moi il ne devrait y avoir que celle là (remarque valable aussi plus bas)

Cette assertion couvre les deux précédentes, pour moi il ne devrait y avoir que celle là (remarque valable aussi plus bas)
Author
Owner

Ca me va !

Ca me va !
yweber marked this conversation as resolved
@ -0,0 +11,4 @@
return Context({'user': user})
def test_manager_templatetags_get_preference(app, simple_user):
Owner

Ce test est redondant avec test_category_fold_preferences il me semble, donc on peut s'en passer

Ce test est redondant avec test_category_fold_preferences il me semble, donc on peut s'en passer
Author
Owner

Personnellement je trouve ça un peu dommage de supprimer ce test simple et spécifique. Mais j'imagine que le reste de la couverture de test est pensée de cette manière alors je m’aligne ;)

Personnellement je trouve ça un peu dommage de supprimer ce test simple et spécifique. Mais j'imagine que le reste de la couverture de test est pensée de cette manière alors je m’aligne ;)
yweber marked this conversation as resolved
yweber force-pushed wip/85616-make-categories-foldable from 758ef73ae8 to b918664701 2024-04-10 17:42:48 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from b918664701 to b6f173800c 2024-04-10 17:43:19 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from b6f173800c to 87b3beb44d 2024-04-10 17:47:00 +02:00 Compare
yweber requested review from vdeniaud 2024-04-10 17:50:01 +02:00
yweber force-pushed wip/85616-make-categories-foldable from 87b3beb44d to 071bb3203c 2024-04-10 18:33:23 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 071bb3203c to 7f486d3d4b 2024-04-10 18:47:29 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from 7f486d3d4b to 37850a288b 2024-04-11 15:16:21 +02:00 Compare
vdeniaud requested changes 2024-04-11 17:13:41 +02:00
vdeniaud left a comment
Owner

Des minis trucs puis ce sera bon pour moi :)

Des minis trucs puis ce sera bon pour moi :)
@ -0,0 +31,4 @@
if len(request.body) > 1000:
return HttpResponse(_('Payload is too large'), status=400)
bad_fmt_response = HttpResponse(_('Bad format'), status=400)
Owner

On peut faire sans variable intermédiaire ici (et éventuellement utiliser HttpResponseBadRequest)

On peut faire sans variable intermédiaire ici (et éventuellement utiliser HttpResponseBadRequest)
Author
Owner

ok ! bien vu pour HttpResponseBadRequest :)

ok ! bien vu pour HttpResponseBadRequest :)
yweber marked this conversation as resolved
@ -0,0 +20,4 @@
class UserPreferences(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, null=True)
Owner

On est obligés d'avoir ce null=True ?

On est obligés d'avoir ce null=True ?
Author
Owner

Oula, au contraire, c'est une super mauvaise idée non :/ ? Navré...

Oula, au contraire, c'est une super mauvaise idée non :/ ? Navré...
yweber marked this conversation as resolved
@ -29,3 +19,1 @@
</li>
{% endfor %}
</ul>
{% with group.grouper.id|stringformat:"s" as i %}
Owner

La conversion explicite me paraît superflue, ça marcherait sans (aussi je crois que la syntaxe with a=b est plus moderne, et on peut affecter plusieurs valeurs dans le même tag, pas besoin d'empiler)

La conversion explicite me paraît superflue, ça marcherait sans (aussi je crois que la syntaxe with a=b est plus moderne, et on peut affecter plusieurs valeurs dans le même tag, pas besoin d'empiler)
Author
Owner

La conversion explicite me paraît superflue, ça marcherait sans

Quand je test

-        {% with 'folded-admin-forms-group-'|add:i as foldname %}
+        {% with 'folded-admin-forms-group-'|add:group.grouper.id as foldname %}

Je me prend un django.template.base.VariableDoesNotExist: Failed lookup for key [id] in None, j'avoue que je ne comprend pas trop, mais j'en avais déduis que la conversion explicite était nécessaire ;)

(aussi je crois que la syntaxe with a=b est plus moderne, et on peut affecter plusieurs valeurs dans le même tag, pas besoin d'empiler)

Et malheureusement, même avec la nouvelle syntaxe l'empilement semble nécessaire :/ Avec

-      {% with group.grouper.id|stringformat:"s" as i %}
-        {% with 'folded-admin-forms-group-'|add:i as foldname %}
+      {% with i=group.grouper.id|stringformat:"s" foldname='folded-admin-forms-group-'|add:i %}
           <div class="section foldable {% if user|get_preference:foldname %}folded{% endif %}" data-section-folded-pref-name="{{foldname}}">
-        {% endwith %}
       {% endwith %}

je me prend un django.template.base.VariableDoesNotExist: Failed lookup for key [i] in ...

Du coup je laisse à l'identique, en remplaçant les as par des =

> La conversion explicite me paraît superflue, ça marcherait sans Quand je test ```diff - {% with 'folded-admin-forms-group-'|add:i as foldname %} + {% with 'folded-admin-forms-group-'|add:group.grouper.id as foldname %} ``` Je me prend un `django.template.base.VariableDoesNotExist: Failed lookup for key [id] in None`, j'avoue que je ne comprend pas trop, mais j'en avais déduis que la conversion explicite était nécessaire ;) > (aussi je crois que la syntaxe with a=b est plus moderne, et on peut affecter plusieurs valeurs dans le même tag, pas besoin d'empiler) Et malheureusement, même avec la nouvelle syntaxe l'empilement semble nécessaire :/ Avec ```diff - {% with group.grouper.id|stringformat:"s" as i %} - {% with 'folded-admin-forms-group-'|add:i as foldname %} + {% with i=group.grouper.id|stringformat:"s" foldname='folded-admin-forms-group-'|add:i %} <div class="section foldable {% if user|get_preference:foldname %}folded{% endif %}" data-section-folded-pref-name="{{foldname}}"> - {% endwith %} {% endwith %} ``` je me prend un `django.template.base.VariableDoesNotExist: Failed lookup for key [i] in ...` Du coup je laisse à l'identique, en remplaçant les `as` par des `=`
@ -34,0 +38,4 @@
@register.filter
def get_preference(user, pref_name):
if not user:
return None
Owner

Cette ligne n'est pas couverte, ni son homologue plus bas, elles doivent pouvoir être retirées

Cette ligne n'est pas couverte, ni son homologue plus bas, elles doivent pouvoir être retirées
Author
Owner

Ok ! J'ai rapidement essayé de trouver comment appeler ce filtre d'une manière un peu tordue, mais j'ai pas trouvé :P
Je te fais confiance sur le fait que ce cas ne peut pas se produire ;)

Ok ! J'ai rapidement essayé de trouver comment appeler ce filtre d'une manière un peu tordue, mais j'ai pas trouvé :P Je te fais confiance sur le fait que ce cas ne peut pas se produire ;)
yweber marked this conversation as resolved
yweber force-pushed wip/85616-make-categories-foldable from 37850a288b to a64aa47c1e 2024-04-11 17:37:58 +02:00 Compare
yweber force-pushed wip/85616-make-categories-foldable from a64aa47c1e to 457f6652e5 2024-04-11 17:45:51 +02:00 Compare
yweber requested review from vdeniaud 2024-04-11 17:54:49 +02:00
vdeniaud approved these changes 2024-04-15 12:55:10 +02:00
vdeniaud left a comment
Owner

Tout bon pour moi à part une dernière remarque, tu peux corriger et merger ensuite !

Tout bon pour moi à part une dernière remarque, tu peux corriger et merger ensuite !
@ -2,0 +6,4 @@
if (old_folded == new_folded) { return; }
var pref_message = Object();
pref_message[mu.target.dataset.sectionFoldedPrefName] = new_folded;
fetch('/api/user_preferences/save', {
Owner

Le changement d'URL _ vers - n'a pas été appliqué ici

Le changement d'URL _ vers - n'a pas été appliqué ici
Author
Owner

Bien vu ! Merci

Bien vu ! Merci
yweber marked this conversation as resolved
yweber force-pushed wip/85616-make-categories-foldable from 457f6652e5 to 570cf81c8e 2024-04-15 14:44:41 +02:00 Compare
yweber merged commit 570cf81c8e into main 2024-04-15 14:49:40 +02:00
yweber deleted branch wip/85616-make-categories-foldable 2024-04-15 14:49:40 +02:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/chrono#230
No description provided.