[NOTIFICATION] informer les parties prenantes des nouvelles réponses #381
Conversation
There was a problem hiding this comment.
J'ai fait des petites remarques à gauche et à droite mais en gros, je ne suis pas certain de comprendre suffisamment bien le projet et ce qui s'y passe pour entrer vraiment dans le fond des choses. J'ai donné quelques conseils génériques concernant les choses qui me semblent dangereuses, mais pour le reste je me dis que si ça fonctionne, c'est bien ?
En attendant j'ai l'impression que beaucoup de lignes amènent à du code qui risque à terme d'être plus difficile à lire et maintenir, donc attention :)
lacommunaute/notification/utils.py
Outdated
| ) # .values(email=F("username")) | ||
| excluded = topic.last_post.username or topic.last_post.poster.email | ||
|
|
||
| return sorted(email for email in authenticated_qs.union(anonymous_qs) if email != excluded) |
There was a problem hiding this comment.
je suis assez étonné de cette fonction.
J'en aurais créé deux:
- une qui retourne les emails à notifier
- une qui retourne les usernames (anonymes) à notifier
Je trouve toujours bizarre qu'une fonction get_bananes retourne à la fois des pommes et des bananes. Non ? A mon avis c'est assez rapidement parti pour cré&er des bugs et incompréhensions. Le assertIn(username, get_emails()) m'a fait tiquer plus haut, genre, est-ce un bug ?
Par ailleurs, ces deux fonctions devraient absolument, à mon avis, être sur l'objet Topic, ça me semblerait tellement plus adapté sur le modèle (par exemple topic.get_notified_emails() ) que dans un helper. Non ?
There was a problem hiding this comment.
Remplacé par la méthode mails_to_notify dans le modèle Topic. En essayant de la rendre plus explicite.
Reste ce point délicat, les Post créés par un utilisateur anonyme ont un username valorisé, avec leur adresse email.
| .values_list("poster__email", flat=True) | ||
| ) | ||
| ) | ||
| ), |
There was a problem hiding this comment.
je trouve ce sorted(list(set(queryset) complexe à lire. Possible de l'extraire dans une fonction temporaire pour mieux comprendre ce que l'on cherchez à vérifier ici ?
lacommunaute/notification/utils.py
Outdated
| # vincentporte : assumed disapproved post are counted | ||
| return [ | ||
| ( | ||
| def collect_following_replies() -> Generator[tuple[str, str, List[str], str], Any, None]: |
There was a problem hiding this comment.
pourquoi cet utilistaire ? Si c'est utilisé dans une vue, tu pourrais le faire directement là bas et tester la vue. Mais je connais mal la communauté encore à ce stade.
En attendant j'ai l'impression que l'on fait un gros générateur pour finalement ne sortir que quelques propriétés accessibles rapidement sur l'objet topic, non ?
There was a problem hiding this comment.
Il s'agit d'alimenter la tâche d'envoi de mails send_notifs_on_following_replies. Je l'ai séparé pour simplifier les tests. Avec tes suggestions de queryset et de méthode sur le modèle Topic, la lecture est beaucoup facile 👏🏿
lacommunaute/notification/utils.py
Outdated
| content_type = ContentType.objects.get_for_model(Post) | ||
|
|
||
| for topic in get_topics_with_following_replies(last_notification(kind=EmailSentTrackKind.FOLLOWING_REPLIES)): | ||
| yield ( | ||
| f"{settings.COMMU_PROTOCOL}://{settings.COMMU_FQDN}{topic.get_absolute_url()}", |
There was a problem hiding this comment.
je me demande si le get_absolute_url ne donne pas déjà le FQDN et le protocole quand on est en prod. A vérifier, sinon ça risque de faire redite.
There was a problem hiding this comment.
Je n'ai trouvé que des méthodes exploitant request pour obtenir le chemin complet.
https://docs.djangoproject.com/en/4.2/ref/request-response/#django.http.HttpRequest.get_full_path
Ici, les données sont générées dans une tâche async. J'ai ajouté with_fqdn pour factoriser un peu plus le code. Si tu vois encore mieux, je t'écoute
|
|
||
| EmailSentTrackFactory(kind=EmailSentTrackKind.FOLLOWING_REPLIES) | ||
|
|
||
| self.assertEqual(len(list(collect_following_replies())), 0) |
There was a problem hiding this comment.
j'ai du mal à comprendre ce test, parce que je pense que j'ai du mal à suivre le code.
Donc on crée un topiuc, deux posts, et si on liste les topics avec réponses, on en a bien un.
Mais si quelqu'un décide de créer à un moment un EmailSentTrack de type "FOLLOWING_REPLIES" alors on ne collecte plus rien. C'est ça ?
There was a problem hiding this comment.
C'est l'idée. A chaque fois qu'une notification est envoyée pour un Topic, un objet EmailSentTrack est créé.
Le traitement cherche les Post créés après la dernière notif.
La limite actuelle : les notifs et les EmailSentTrack sont créés en async, donc tous les Topic avec de nouveaux messages sont traités en même temps. Les EmailSentTrack ont, à la milliseconde près, le même datetime d'enrigistrement.
Par facilité, la tâche async récupère le datetime du dernier EmailSentTrack. Ça ne fonctionnerait plus avec des actions synchrones.
|
(note: j'ai lu les commentaires concernant le EmailSentTrack mais j'avoue que sans cloner le code, regarder précisément comment fonctionne le cron, etc; j'ai un peu du mal à comprendre exactement tous les tenants et aboutissants !) |
vincentporte
force-pushed
the
360-notifs-prevenir-les-participants-à-un-topic-des-nouvelles-réponses
branch
from
61fe2b9 to
3ff3fc9
Compare
vincentporte
force-pushed
the
360-notifs-prevenir-les-participants-à-un-topic-des-nouvelles-réponses
branch
from
9049085 to
66e4275
Compare
…ute_url topic method
…thods and qs filter
vincentporte
force-pushed
the
360-notifs-prevenir-les-participants-à-un-topic-des-nouvelles-réponses
branch
from
66e4275 to
4dd2c31
Compare
vincentporte
deleted the
360-notifs-prevenir-les-participants-à-un-topic-des-nouvelles-réponses
branch
Description
🎸 Notifier toutes les parties prenantes d'un
Topiclors des nouveaux messages.🎸 Les parties prenantes sont les
postersanonymes et authentifiés, leslikerset lesupvoters.🎸 Le
posterdu dernierpostdutopicn'est pas notifiés.🎸 Deux types de notifications :
Type de changement
🎢 Evolution de la fonctionnalité (changement non cassant).
Points d'attention
🦺 L'objet
EmailSentTrackpermet d'enregistrer la date du dernier traitement de notification. Ne sont considérés que lesTopicmis à jour après.🦺 notifications de premiere reponse (tache cron hourly)
lacommunaute.notification.utils.collect_first_repliesEmailSentTrack, kind :EmailSentTrackKind.FIRST_REPLY🦺 notifications de reponse suivante (tache cron daily)
lacommunaute.notification.utils.collect_following_repliesEmailSentTrack, kind :EmailSentTrackKind.FOLLOWING_REPLIES