Produits Entr'ouvert » Chrono

    def delete(self, *args, **kwargs):
        if self.settings_slug is not None:
            self.disable()
            return 0, {}
        return super().delete(*args, **kwargs)

Je suis pas super fan de la méthode delete qui ne fait pas un delete dans certains cas; je préfèrerais que ce soit plus explicite: on appelle la méthode disable dans la vue delete, ou on ajoute carrément une vue disable qui fait ce qu'elle annonce: elle disable :) (on peut aussi avoir une vue toggle qui enable/disable en fonction de l'état de la ressource)

Autre remarque: la vue TimePeriodExceptionSourceEnableView doit vérifier que:
- la ressource est bien du type qui permet un enable (même si c'est vérifié dans la méthode enable, ceinture et bretelles) (raise 404 si c'est pas le cas)
- éventuellement que la ressource est bien disabled (elle est dans le bon état)

j'ai une juste mini remarque:

            _(
                'Exception source %(source)s has been %(status)s on desk %(desk)s.'
                % {
                    'source': source,
                    'status': 'enabled' if source.enabled else 'disabled',
                    'desk': source.desk,
                }
            ),

Je crois qu'il vaut mieux faire 2 messages distincts, pour le bien-être des traducteurs

Sinon ça me semble OK, je laisse une autre personne valider :)

Enfin à tester, c'est curieux de ne pas avoir la ligne dans la popup pour les agendas existants.

Pas fan de l'icône "switch" (notamment parce qu'on ne s'attend du coup pas à une fermeture de la popup), je me dis qu'un libellé expliciter (activer) / (désactiver) serait plus clair.

Une interrogation m'est venue de mon côté, la vue toggle fait quand même pas mal d'opérations en db, est-ce qu'un GET suffit vraiment ou un POST serait préférable ?

Pas choqué par ça.

(il y a rebase avec déplacement de la migration à faire)

Frédéric Péters a écrit :

Enfin à tester, c'est curieux de ne pas avoir la ligne dans la popup pour les agendas existants.

Pas faux, donc carrément un truc du genre

--- a/chrono/agendas/management/commands/sync_desks_timeperiod_exceptions_from_settings.py
+++ b/chrono/agendas/management/commands/sync_desks_timeperiod_exceptions_from_settings.py
@@ -18,3 +18,3 @@ from django.core.management.base import BaseCommand

-from chrono.agendas.models import TimePeriodExceptionSource
+from chrono.agendas.models import Desk

@@ -25,3 +25,3 @@ class Command(BaseCommand):
     def handle(self, **options):
-        for source in TimePeriodExceptionSource.objects.filter(settings_slug__isnull=False):
-            source.desk.import_timeperiod_exceptions_from_settings()
+        for desk in Desk.objects.all():
+            desk.import_timeperiod_exceptions_from_settings()

À faire tourner manuellement une fois ? (par exemple dans la foulée de l'ajout du cron qui va devoir l'appeler ~tous les ans)

Frédéric Péters a écrit :

Pas fan de l'icône "switch" (notamment parce qu'on ne s'attend du coup pas à une fermeture de la popup), je me dis qu'un libellé expliciter (activer) / (désactiver) serait plus clair.

Et aussi, quelle classe je peux mettre au <a> pour avoir un rendu propre ? (« (activer) » en lieu et place de l'icône et pas une ligne en dessous)

Tu peux rebaser la branche ?

Et aussi, quelle classe je peux mettre au <a> pour avoir un rendu propre ? (« (activer) » en lieu et place de l'icône et pas une ligne en dessous)

Je viens de créer #41192 pour te donner une classe link-action-text.

→ <li><a href="...">plop</a> <a class="link-action-text" href="...">activer</a></li>

Reste à décider ce qu'on fait pour les traductions, il y a une PR quasi finie, avec les chaînes fr, qui a bougé pour la dernière fois il y deux semaines ici https://github.com/peopledoc/workalendar/pull/466. Les .po se copient collent probablement, en attendant le merge/release.

En plus de ça, je me suis aperçu qu'il était toujours permis de supprimer ces exceptions individuellement, ce qui je pense est une bonne chose (le lundi de Pentecôte, tout ça). Mais du coup, la commande de mise à jour ne doit pas toucher aux exceptions déjà là, juste en ajouter de nouvelles.

Je me dis que plutôt non on ne devrait pas permettre la suppression d'exceptions importées depuis des sources externes; mais si on veut le permettre, ça passerait alors par un flag posé sur l'exception, la marquant "supprimée", plutôt qu'une vraie suppression.

Et je pense que ce ticket pourrait se limiter à ne pas le permettre, et qu'un ticket ultérieur apporterait la possibilité de "supprimer".

Reste à décider ce qu'on fait pour les traductions, il y a une PR quasi finie [...]

Ok, là-dessus à réfléchir je serais pour merger mais avec EXCEPTIONS_SOURCES vide tant qu'il n'y a pas les traductions upstream; ça évite les rebase successifs, pénibles avec des traductions, et ça n'expose pas de bouts pas traduits.

Frédéric Péters a écrit :

En plus de ça, je me suis aperçu qu'il était toujours permis de supprimer ces exceptions individuellement, ce qui je pense est une bonne chose (le lundi de Pentecôte, tout ça). Mais du coup, la commande de mise à jour ne doit pas toucher aux exceptions déjà là, juste en ajouter de nouvelles.

Je me dis que plutôt non on ne devrait pas permettre la suppression d'exceptions importées depuis des sources externes; mais si on veut le permettre, ça passerait alors par un flag posé sur l'exception, la marquant "supprimée", plutôt qu'une vraie suppression.

Et je pense que ce ticket pourrait se limiter à ne pas le permettre, et qu'un ticket ultérieur apporterait la possibilité de "supprimer".

Et ne pas le permettre veut dire ajouter un flag la marquant comme « non supprimable », je me demande si ça ne vaut pas le coup de le faire directement. Et si « source externe » inclut les ics, toucher à ça dans un sens ou dans l'autre sort déjà pas mal du ticket...

Mais je suis aussi curieux de savoir ce qui ne convient pas dans le fonctionnement actuel : on peut supprimer les exceptions, oui elles sont vraiment supprimées, et ? Activer/désactiver la source permet de les remettre comme avant, si on avait posé un flag « supprimé » on pourrait en réactivant ne pas faire ça et garder les exceptions supprimées, mais ce cas d'usage va déjà chercher loin.

La situation présente permet d'avoir "jours fériés" activée mais des jours fériés absents, parce que supprimés manuellement, je trouve ça bizarre; et/ou des jours supprimés mais qui vont réapparaitre automatiquement parce qu'un cron sera passé par là.

workalendar et ses dépendances sont désormais packagées; tu pourrais rebaser la branche ?

Si la question de la traduction des jours est toujours présente, soit on maintient la liste des libellés + traductions ici, soit on n'affiche que la date et pas le libellé.

0002: on ne change rien sur la page de gestion des exception ? On a toujours "Importer des exceptions" en titre par exemple

0003: ça serait pas mal d'ajouter un filter external=False dans le queryset au niveau de la vue delete d'une exception, par sécurité.