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
13 changed files with 323 additions and 14 deletions

View File

@ -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')),
yweber marked this conversation as resolved Outdated

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

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

Ok ! Fait :)

Ok ! Fait :)
]

View File

View File

@ -0,0 +1,23 @@
# chrono - agendas system
# Copyright (C) 2024 Entr'ouvert
#
# This program is free software: you can redistribute it and/or modify it
# under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
from django.urls import path
from . import api_views
urlpatterns = [
path('save', api_views.save_preference, name='api-user-preferences'),
]

View File

@ -0,0 +1,43 @@
# chrono - agendas system
# Copyright (C) 2024 Entr'ouvert
#
# This program is free software: you can redistribute it and/or modify it
# under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import json
from django.contrib.auth.decorators import login_required
from django.http import HttpResponse, HttpResponseBadRequest
from django.utils.translation import gettext_lazy as _
from django.views.decorators.csrf import csrf_exempt
from . import models
@csrf_exempt
@login_required
def save_preference(request):
yweber marked this conversation as resolved Outdated

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 :)

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 ?

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)

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 ;)
user_pref, dummy = models.UserPreferences.objects.get_or_create(user=request.user)
if len(request.body) > 1000:
return HttpResponseBadRequest(_('Payload is too large'))
try:
yweber marked this conversation as resolved Outdated

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

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)

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 :)

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

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

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

ok ! bien vu pour HttpResponseBadRequest :)

ok ! bien vu pour HttpResponseBadRequest :)
prefs = json.loads(request.body)
except json.JSONDecodeError:
yweber marked this conversation as resolved Outdated

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)

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 ;) )
return HttpResponseBadRequest(_('Bad format'))
if not isinstance(prefs, dict) or len(prefs) != 1:
return HttpResponseBadRequest(_('Bad format'))
yweber marked this conversation as resolved Outdated

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

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

En effet ! Merci :)

En effet ! Merci :)
user_pref.preferences.update(prefs)
user_pref.save()
return HttpResponse('', status=204)

View File

@ -0,0 +1,32 @@
# Generated by Django 3.2.18 on 2024-04-11 15:30
import django.db.models.deletion
from django.conf import settings
from django.db import migrations, models
class Migration(migrations.Migration):
initial = True
dependencies = [
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
]
operations = [
migrations.CreateModel(
name='UserPreferences',
fields=[
(
'id',
models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'),
),
('preferences', models.JSONField(default=dict, verbose_name='Preferences')),
(
'user',
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL
),
),
],
),
]

View File

@ -0,0 +1,24 @@
# chrono - agendas system
# Copyright (C) 2024 Entr'ouvert
#
# This program is free software: you can redistribute it and/or modify it
# under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
from django.conf import settings
from django.db import models
from django.utils.translation import gettext_lazy as _
class UserPreferences(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
yweber marked this conversation as resolved Outdated

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

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

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

Oula, au contraire, c'est une super mauvaise idée non :/ ? Navré...
preferences = models.JSONField(_('Preferences'), default=dict)

View File

@ -1,4 +1,23 @@
$(function() {
const foldableClassObserver = new MutationObserver((mutations) => {
mutations.forEach(mu => {
const old_folded = (mu.oldValue.indexOf('folded') != -1);
const new_folded = mu.target.classList.contains('folded')
if (old_folded == new_folded) { return; }
var pref_message = Object();
pref_message[mu.target.dataset.sectionFoldedPrefName] = new_folded;
fetch('/api/user-preferences/save', {
yweber marked this conversation as resolved Outdated

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

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

Bien vu ! Merci

Bien vu ! Merci
method: 'POST',
credentials: 'include',
headers: {'Accept': 'application/json', 'Content-Type': 'application/json'},
body: JSON.stringify(pref_message)
});
});
});
document.querySelectorAll('[data-section-folded-pref-name]').forEach(
elt => foldableClassObserver.observe(elt, {attributes: true, attributeFilter: ['class'], attributeOldValue: true})
);
$('[data-total]').each(function() {
var total = $(this).data('total');
var booked = $(this).data('booked');

View File

@ -1,5 +1,5 @@
{% extends "chrono/manager_base.html" %}
{% load i18n thumbnail %}
{% load i18n thumbnail chrono %}
{% block appbar %}
{% include 'chrono/includes/application_appbar_fragment.html' with title_no_application=_('Agendas outside applications') title_object_list=_('Agendas') %}
@ -16,19 +16,23 @@
{% if object_list %}
{% regroup object_list by category as agenda_groups %}
{% for group in agenda_groups %}
<div class="section">
{% if group.grouper %}<h3>{{ group.grouper }}</h3>{% elif not forloop.first %}<h3>{% trans "Misc" %}</h3>{% endif %}
<ul class="objects-list single-links">
{% for object in group.list %}
<li>
<a href="{% url 'chrono-manager-agenda-view' pk=object.id %}">
<span class="badge">{{ object.get_real_kind_display }}</span>
{% include 'chrono/includes/application_icon_fragment.html' %}
{{ object.label }}{% if user.is_staff %} <span class="identifier">[{% trans "identifier:" %} {{ object.slug }}]{% endif %}</span>
</a>
</li>
{% endfor %}
</ul>
{% with i=group.grouper.id|stringformat:"s" %}
yweber marked this conversation as resolved Outdated

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)

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).

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.

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 !

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)

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 `=`
{% with foldname='foldable-manager-category-group-'|add:i %}
<div class="section foldable {% if user|get_preference:foldname %}folded{% endif %}" data-section-folded-pref-name="{{foldname}}">
{% endwith %}
{% endwith %}
{% if group.grouper %}<h3>{{ group.grouper }}</h3>{% elif not forloop.first %}<h3>{% trans "Misc" %}</h3>{% endif %}
<ul class="objects-list single-links">
{% for object in group.list %}
<li>
<a href="{% url 'chrono-manager-agenda-view' pk=object.id %}">
<span class="badge">{{ object.get_real_kind_display }}</span>
{% include 'chrono/includes/application_icon_fragment.html' %}
{{ object.label }}{% if user.is_staff %} <span class="identifier">[{% trans "identifier:" %} {{ object.slug }}]{% endif %}</span>
</a>
</li>
{% endfor %}
</ul>
</div>
{% endfor %}
{% elif not no_application %}

View File

@ -17,6 +17,8 @@
from django import template
from django.utils.formats import date_format
from chrono.apps.user_preferences.models import UserPreferences
register = template.Library()
@ -31,3 +33,9 @@ def human_date_range(date_start, date_end):
date_start_format = 'd'
return '%s %s' % (date_format(date_start, date_start_format), date_format(date_end, date_end_format))
@register.filter
def get_preference(user, pref_name):
user_preferences, dummy = UserPreferences.objects.get_or_create(user=user)
return user_preferences.preferences.get(pref_name) or False
yweber marked this conversation as resolved Outdated

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

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 ;)

View File

@ -63,6 +63,7 @@ INSTALLED_APPS = (
'chrono.apps.ants_hub',
'chrono.apps.export_import',
'chrono.apps.snapshot',
'chrono.apps.user_preferences',
)
MIDDLEWARE = (

View File

@ -0,0 +1,71 @@
import json
import pytest
from django.urls import reverse
from chrono.apps.user_preferences.models import UserPreferences
from tests.utils import login
pytestmark = pytest.mark.django_db
def test_user_preferences_api_ok(app, admin_user):
login(app)
yweber marked this conversation as resolved Outdated

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

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

Cool ! Merci :)

Cool ! Merci :)
fake_id = 'fake-id-1'
url = reverse('api-user-preferences')
app.post_json(url, params={fake_id: True}, status=204)
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref.preferences[fake_id] is True
app.post_json(url, params={fake_id: False}, status=204)
yweber marked this conversation as resolved Outdated

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)

Ca me va !

Ca me va !
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref.preferences[fake_id] is False
fake_id2 = 'fake-id-2'
app.post_json(url, params={fake_id2: False}, status=204)
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref.preferences[fake_id] is False
assert user_pref.preferences[fake_id2] is False
app.post_json(url, params={fake_id2: False}, status=204)
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref.preferences[fake_id] is False
assert user_pref.preferences[fake_id2] is False
app.post_json(url, params={fake_id2: True}, status=204)
user_pref = UserPreferences.objects.get(user=admin_user)
assert user_pref.preferences[fake_id] is False
assert user_pref.preferences[fake_id2] is True
@pytest.mark.parametrize(
'bad_body',
(
json.dumps({'fake-id-1': True, 'fake-id-2': False}),
'"not a dict"',
'[1,2,3]',
'{\'fake-id-1\': true',
),
)
def test_user_preferences_api_invalid(app, admin_user, bad_body):
login(app)
url = reverse('api-user-preferences')
app.post(url, params=bad_body, status=400)
def test_user_preferences_api_large_payload(app, admin_user):
login(app)
url = reverse('api-user-preferences')
app.post(url, params='a' * 1024, status=400)
app.post_json(url, params={'b' * 1024: True}, status=400)
def test_user_preferences_api_unauthorized(app):
url = reverse('api-user-preferences')
app.post(url, params={'toto': True}, status=302)

View File

@ -4,11 +4,17 @@ from django.test.utils import CaptureQueriesContext
from chrono.agendas.models import Agenda, Category
from chrono.apps.snapshot.models import CategorySnapshot
from chrono.apps.user_preferences.models import UserPreferences
from tests.utils import login
pytestmark = pytest.mark.django_db
def update_preference(user_preference, name, value):
user_preference.preferences.update({name: value})
user_preference.save()
def test_list_categories_as_manager(app, manager_user):
agenda = Agenda(label='Foo Bar')
agenda.view_role = manager_user.groups.all()[0]
@ -97,3 +103,80 @@ def test_inspect_category(app, admin_user):
with CaptureQueriesContext(connection) as ctx:
resp = resp.click('Inspect')
assert len(ctx.captured_queries) == 3
def test_category_fold_preferences(app, admin_user):
category1 = Category.objects.create(label='Foo bar')
category2 = Category.objects.create(label='Toto')
pref_name1 = f'foldable-manager-category-group-{category1.id}'
pref_name2 = f'foldable-manager-category-group-{category2.id}'
Agenda.objects.create(label='Foo bar', category=category1)
agenda2 = Agenda.objects.create(label='Titi', category=category2)
app = login(app)
resp = app.get('/manage/')
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name1}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name2}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
user_prefs = UserPreferences.objects.get(user=admin_user)
update_preference(user_prefs, pref_name1, True)
resp = app.get('/manage/')
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name1}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' in elt[0].classes
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name2}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
# Order is preserved when adding a new category : preferences are preserved
category_temp = Category.objects.create(label='Tata0')
category3 = Category.objects.create(label='Tata')
pref_name3 = f'foldable-manager-category-group-{category3.id}'
category_temp.delete()
Agenda.objects.create(label='Titi', category=category3)
update_preference(user_prefs, pref_name1, False)
update_preference(user_prefs, pref_name2, True)
resp = app.get('/manage/')
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name1}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name2}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' in elt[0].classes
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name3}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
# Preferences are not "shifted" when a category is deleted
agenda2.delete()
resp = app.get('/manage/')
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name1}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes
elt = resp.pyquery.find(f'div[data-section-folded-pref-name={pref_name3}]')
assert len(elt) == 1
assert 'foldable' in elt[0].classes
assert 'folded' not in elt[0].classes