Produits Entr'ouvert » Chrono

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

Et ce serait des <select> avec des rôles

Et l'idée serait d'envoyer un mail aux adresses associées au rôle dans authentic ? Comment on les récupère ?

Et l'idée serait d'envoyer un mail aux adresses associées au rôle dans authentic ?

Oui.

Bien sûr j'avais oublié que le provisionning n'enregistrait pas cette info; il pourrait/devrait, mais ça amène une zone nébuleuse de dépendance à hobo.

Peut-être zapper ça pour le moment et juste avoir un champ texte où saisir une adresse.

Je voulais juste m'assurer que je ne passais pas à côté de quelque chose, je pense que c'est une bonne idée de se baser sur l'info déjà présente dans a2, je vais faire le ticket qui va bien dans hobo (et pourquoi pas faire en sorte de ne pas introduire de dépendance ici, en cachant les bons champs au bon moment).

(relecture juste du début, surtout de la forme)

subject = _('Alert: event "%(event)s" is %(status)s') % {

Plutôt avoir des phrases complètes correspondant aux différentes situations, ça fera des phrases plus agréables à lire/traduire.

body = _('You can view it here: %s.') % urljoin(

Passer par un template (2 même, text/plain, text/html), pour pouvoir faire de beaux messages.

Il y aura pour les emails le même soucis que dans #43469 (absence de template_vars dans le contexte).

+from django.conf import settings as django_settings

C'est je trouve plus simple de garder aux imports un nom constant; alors oui arrive plus loin une variable qui s'appelle "settings" mais justement, c'est la partie que je ne trouve pas des masses lisibles du patch,

+        notifications_settings = AgendaNotificationsSettings.objects.all()
+        for settings in notifications_settings.select_related('agenda'):
+            for setting, recipients in settings:

J'inverserais et trouverais plus clair de boucler sur les agendas, et ensuite, il y a un seul AgendaNotificationsSettings par agenda, non ? Ce doute c'est à cause de l'autre boucle, qui vient peut-être parce que :

+    def __iter__(self):
+        for field in self.get_setting_fields():
+            yield (field.name, self.get_recipients(field.name))
+

Je préférerais quelque chose de bien plus explicite, genre

def get_notification_types(self):
    return ['almost_full', 'full', 'cancelled']

sans introspection ici du modèle pour en sortir les champs.

Et puis un

def get_recipients(self, notification_type):
    ...

Et même impression côté gabarit, sur la partie,

+{% for setting, value in object.notifications_settings.display_info %}

mais qui m'amènerait peut-être à vouloir reformuler ce que je notais plus haut,

def get_notification_types(self):
    for notification_type in ['almost_full', 'full', 'cancelled']:
        return NotificationType(notification_type, self)

avec la class NotificationType qui aurait une méthode pour retourner la liste d'adresses/rôles, une méthode pour retourner le libellé, etc.

En fait j'ai l'impression qu'il y a beaucoup d'efforts pour ne pas répéter les trois libellés, à les chercher à travers les champs du modèle, et à mon goût ça complexifie trop l'ensemble, je préfèrerais vraiment avoir une répétition de libellé.

C'est là mon retour principal.

Moins important parce que ça retourne presque juste sur une discussion de nommage,

+    # flags for detecting changes
+    was_almost_full = models.BooleanField(default=False)
+    was_full = models.BooleanField(default=False)
+    was_cancelled = models.BooleanField(default=False)

Si j'ai bien compris, je serais pour mettre almost_full_notification_timestamp etc., avec des datetime, ça évitera de se dire que c'était plein et ça ne l'est plus ?

Petits ajustements pour verdir les tests poussés sur la branche.

Aussi,

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

Si j'ai bien compris, je serais pour mettre almost_full_notification_timestamp etc., avec des datetime, ça évitera de se dire que c'était plein et ça ne l'est plus ?

J'ai appliqué ce changement sans vraiment comprendre l'avantage (le sens de la dernière virgule), du coup je me suis contenté de remplacer les was_full=False par full_notification_timestamp__isnull=True, en passant peut-être à côté d'un comportement plus intelligent à ajouter.

Et dans la même veine, le cas full --> plus full --> full ne déclenche qu'une notification, la première, je pense que si besoin ce cas peut être mieux géré plus tard, je me contente juste de le noter explicitement ici.

"ça évitera de se dire que c'était plein et ça ne l'est plus ?"

C'est ma question quand je lis was_full. Que signifie l'attribut, que c'était plein et que ça ne l'est plus ? (alors que non ça voudrait dire que c'était plein et que ce flag a été mis pour le signaler parce qu'il y a un truc interne de notif qui a besoin de savoir ça mais si jamais l'événement reste plein ça va rester à was_full et s'il n'est plus plein ça va rester à was_full quand même). (si j'ai bien compris)

De là avoir full_notification_timestamp qui dit juste "c'est le moment où la notification a été envoyée signalant l'événement plein".

Il me semble juste rester à compléter un peu, et marquer pour traduction, les messages de notification :

+++ b/chrono/agendas/templates/agendas/event_notification_body.txt (et le .html aussi)
@@ -0,0 +1 @@
+You can view it here: {{ event_url }}.

(compléter en disant bonjour et en reprenant dedans les infos de l'événement, genre)