manager: make agenda's groups foldable (#85616) #230
Loading…
Reference in New Issue
No description provided.
Delete Branch "wip/85616-make-categories-foldable"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
b1378c5925
to86e25f5fd2
WIP: manager: make agenda's groups foldable (#85616)to manager: make agenda's groups foldable (#85616)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.
manager: make agenda's groups foldable (#85616)to wip: manager: make agenda's groups foldable (#85616)86e25f5fd2
toc5a2671569
c5a2671569
toe24bf800c5
e24bf800c5
to2393f93660
2393f93660
to0c72706ae1
0c72706ae1
to49c5174ac4
49c5174ac4
to617eab9492
617eab9492
to140be56c5b
140be56c5b
tof81dfa9ca6
f81dfa9ca6
to758ef73ae8
wip: manager: make agenda's groups foldable (#85616)to manager: make agenda's groups foldable (#85616)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')),
On préfère les - aux _ dans les URL
Ok ! Fait :)
@ -0,0 +26,4 @@
@csrf_exempt
def save_preference(request):
'''Save a user preference
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 :)
C'est vraiment la docstring qui t'ennuie ?
Te conviendrais mieux ?
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)
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 ;)
@ -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:
Pour faire ça tu peux utiliser le décorateur
login_required
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)
Oui il n'y a pas de raison d'être poli avec les bots :)
@ -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]
Je vois que plus bas tu utilises la forme
user_preferences, _
, pour info ici tu peux aussi faire ça et ça donneraituser_preferences, dummy
(dummy étant un mot clé que pylint ignore au moment de t'engueuler parce que tu as des variables inutilisées)Merci ! J'avais effectivement pas le cheatcode du
dummy
( et_
était déjà utilisé pour gettext ;) )@ -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)
Le
.encode()
n'est pas nécessaireEn effet ! Merci :)
@ -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):
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)
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.
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.
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 ;) ).
@ -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):
Ça me paraîtrait plus pythonique de faire sans ces getter et setter d'une ou deux lignes, et manipuler directement l'attribut
preferences
ailleursPlus 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 !
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é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)
Oui c'est bien ce que j'imaginais aussi de mon côté.
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-
?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)
Ok ! Fait :)
En effet -_- Le boulet....
@ -29,3 +19,1 @@
</li>
{% endfor %}
</ul>
{% with forloop.counter|stringformat:"s" as i %}
Pourquoi ne pas se baser sur l'id de la catégorie ? (tu l'as dans group.grouper.id)
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.
On est d'accord :) Du coup zut, j'ai mal saisie comment c'est fait dans wcs :/ Je patch ça !
@ -0,0 +10,4 @@
def test_user_preferences_api_ok(app, admin_user):
UserPreferences.objects.all().delete()
Pas besoin, les transactions sont automatiquement rollback entre les tests, contrairement à wcs
Cool ! Merci :)
@ -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
Cette assertion couvre les deux précédentes, pour moi il ne devrait y avoir que celle là (remarque valable aussi plus bas)
Ca me va !
@ -0,0 +11,4 @@
return Context({'user': user})
def test_manager_templatetags_get_preference(app, simple_user):
Ce test est redondant avec test_category_fold_preferences il me semble, donc on peut s'en passer
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 ;)
758ef73ae8
tob918664701
b918664701
tob6f173800c
b6f173800c
to87b3beb44d
87b3beb44d
to071bb3203c
071bb3203c
to7f486d3d4b
7f486d3d4b
to37850a288b
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)
On peut faire sans variable intermédiaire ici (et éventuellement utiliser HttpResponseBadRequest)
ok ! bien vu pour HttpResponseBadRequest :)
@ -0,0 +20,4 @@
class UserPreferences(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, null=True)
On est obligés d'avoir ce null=True ?
Oula, au contraire, c'est une super mauvaise idée non :/ ? Navré...
@ -29,3 +19,1 @@
</li>
{% endfor %}
</ul>
{% with group.grouper.id|stringformat:"s" as i %}
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)
Quand je test
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 ;)Et malheureusement, même avec la nouvelle syntaxe l'empilement semble nécessaire :/ Avec
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
Cette ligne n'est pas couverte, ni son homologue plus bas, elles doivent pouvoir être retirées
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 ;)
37850a288b
toa64aa47c1e
a64aa47c1e
to457f6652e5
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', {
Le changement d'URL _ vers - n'a pas été appliqué ici
Bien vu ! Merci
457f6652e5
to570cf81c8e