Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZEP-24 : Centre de notifications #3301

Merged
merged 20 commits into from Mar 5, 2016

Conversation

@GerardPaligot
Copy link
Member

commented Jan 15, 2016

Q R
Correction de bugs ? Non
Nouvelle Fonctionnalité ? Oui
Tickets (issues) concernés ZEP-24, #2515, #2657, #2905, #3306

Hello,

Voici venir la première PR pour la ZEP-24 ! Cette première PR vient poser le socle technique et baser l'existant dessus. Aujourd'hui, la ZEP gère les notifications pour les messages d'un sujet, les réactions d'un contenu et les conversations privées.

Pour la QA :

Important

  • Pensez à faire la migration : python manage.py migrate.
  • Pensez à faire la migration des anciennes données : python manage.py migrate_subscriptions.
  • Aucune commande makemigrations ne doit être nécessaire !

Pour les messages d'un sujet

  • Vérifiez que les notifications sont bien conservées avant la migration vers cette PR.
  • S'inscrire et se désinscrire à un sujet doit générer une souscription (regardez dans l'espace admin).
  • Recevoir des notifications dans un sujet souscrit.
  • Être notifié par mail ou non à un sujet.
  • Recevoir un mail et une notification dans un sujet souscrit.
  • Perdre la souscription lorsqu'un sujet est déplacé dans un forum non visible.
  • Souscription automatique au sujet bêta de tous les auteurs d'un contenu lors de sa mise en bêta.
  • Perdre les souscriptions et les notifications à la suppression d'un compte. Pour le vérifier, connectez-vous après avec un compte admin et rendez-vous dans l'espace admin.
  • Marqué comme lu une notification sur un message masqué.

Pour les réactions d'un contenu

  • Vérifiez que les notifications sont bien conservées avant la migration vers cette PR.
  • Poster dans un contenu doit créer une souscription (à vérifier dans l'espace admin).
  • Recevoir des notifications dans un contenu souscrit.
  • Perdre les souscriptions et les notifications à la suppression d'un compte. Pour le vérifier, connectez-vous après avec un compte admin et rendez-vous dans l'espace admin.

Pour les conversations privées

Note : Ne testez pas avec le MP généré grâce aux fixtures. Cette ressource là est toute pourrie. Limite, supprimez là dès que vous avez installé vos fixtures quand vous êtes sur la branche dev au tout début de votre QA.

  • Vérifiez que les notifications sont bien conservées avant la migration vers cette PR.
  • Les notifications sur les messages doivent s'afficher uniquement dans l'espace dédié dans l'en-tête du site.
  • La création d'un sujet avec des participants entrainent la création de notifications pour les participants.
  • Poster un message dans un message privé génère des notifications pour les autres (participants ou auteur).
  • Ajouter un nouveau membre dans une conversation doit générer une notification vers le dernier message d'une conversation privée.
  • Quitter une conversation privée doit supprimer la potentielle notification en cours (mais les autres doivent garder leurs notifications actives).
  • Si l'utilisateur a paramétré dans son profil qu'il voulait recevoir des mails à chaque réponse, le système doit envoyer des mails pour tous les cas précédemment cités.
  • Perdre les souscriptions et les notifications à la suppression d'un compte. Pour le vérifier, connectez-vous après avec un compte admin et rendez-vous dans l'espace admin.

@GerardPaligot GerardPaligot force-pushed the GerardPaligot:new_zep_24 branch from 15699ee to 2bfb79a Jan 15, 2016

@gustavi

This comment has been minimized.

Copy link
Member

commented Jan 15, 2016

Joli travail. Je vois que tu utilises quelques fonctionnalités sympas de Django et j'aime ça ! J'ai rapidement regardé le code (#notime) et ça me semble plutôt pas mal :)

Attention toutefois, un rebase s'impose (et t'en aura un joli poste django 1.9 aussi :p) !

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2016

J'avais déjà rebase avant de faire ma PR mais je vais sans doute être amené à le refaire pas mal de fois. :)


class PrivateTopicAnswerSubscription(AnswerSubscription, SingleNotificationMixin):
"""
Subscription to new answer in a topic

This comment has been minimized.

Copy link
@poulp

poulp Jan 16, 2016

Contributor

in a private topic

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 16, 2016

Author Member

Nooooon, j'ai pas fais du copié/collé durant le dev. :-°

class AnswerSubscription(Subscription):
"""
Subscription to new answer, either in a topic, a article or a tutorial
NOT used directly, use one of its subtype

This comment has been minimized.

Copy link
@poulp

poulp Jan 16, 2016

Contributor

Mettre le modèle en abstrait pour éviter de créer une table ?

class Meta:
        abstract = True

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 16, 2016

Author Member

Ah super, on s'était posé la question au début du dev si c'était possible mais on avait juste regardé au niveau du langage, pas de django.

ping @ChantyTaguan :)

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Après quelques tests, c'est pas si simple. Je reviendrais dessus si j'ai du temps mais c'est pas urgent.

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2016

@firm1 Si tu pouvais faire une relecture avant d'être au merge, j'apprécierais. :-°

@@ -18,7 +18,7 @@ restera propre et lisible au cours du temps !

#!/bin/sh

flake8 --exclude=migrations,urls.py,settings.py --max-line-length=120 zds
flake8 --exclude=migrations,urls.py,urls_contents.py,settings.py --max-line-length=120 zds

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

pourquoi?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

C'est un peu complexe. En fait, nous avons un fichier qui se nomme receivers.py qui contient que des receivers. Ces receivers sont appelés nul part dans notre code source. C'est géré en interne par Django. Du coup, comme personne ne fait d'import sur ce fichier, l'usage (d'après nos observations à @ChantyTaguan et moi sur divers ressources) est un import inutilisé dans les fichiers urls.py pour charger le fichier.

@gustavi m'a proposé une autre solution mais ni lui, ni moi ne savons si ça va fonctionner. En gros, ça serait de placer les receivers dans un dossier avec un fichier __init__.py pour que Django le charge sans besoin d'importer quoi que ce soit dans un fichier. A tester pour savoir si ça fonctionne.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Ok donc j'ai mon "pourquoi", je suis content.

Le 18 janvier 2016 à 10:25, Gérard Paligot notifications@github.com a
écrit :

In doc/source/utils/git-pre-hook.rst
#3301 (comment)
:

@@ -18,7 +18,7 @@ restera propre et lisible au cours du temps !

 #!/bin/sh
  • flake8 --exclude=migrations,urls.py,settings.py --max-line-length=120 zds
  • flake8 --exclude=migrations,urls.py,urls_contents.py,settings.py --max-line-length=120 zds

C'est un peu complexe. En fait, nous avons un fichier qui se nomme
receivers.py qui contient que des receivers. Ces receivers sont appelés
nul part dans notre code source. C'est géré en interne par Django. Du coup,
comme personne ne fait d'import sur ce fichier, l'usage (d'après nos
observations à @ChantyTaguan https://github.com/ChantyTaguan et moi sur
divers ressources) est un import inutilisé dans les fichiers urls.py pour
charger le fichier.

@gustavi https://github.com/gustavi m'a proposé une autre solution mais
ni lui, ni moi ne savons si ça va fonctionner. En gros, ça serait de placer
les receivers dans un dossier avec un fichier init.py pour que Django
le charge sans besoin d'importer quoi que ce soit dans un fichier. A tester
pour savoir si ça fonctionne.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r49975669.

This comment has been minimized.

Copy link
@gustavi

gustavi Jan 21, 2016

Member

@GerardPaligot tu as testé ma solution ?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

Pas encore eu le temps. Quand ça sera le cas, tu seras mis au courant. :)

@@ -279,11 +279,11 @@
{# MESSAGERIE PRIVEE #}
<div>
{% with topics=user|interventions_privatetopics %}
{% with unread_topics=topics.unread %}
{% with total_topics=topics.total %}
{% with notifications=topics.notifications %}

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

est-on sûr que cet objet est préchargé? J'ai pas envie qu'on se retrouve avec un lazyloading sur ça.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Tu es bien plus compétent que moi sur la question. Je serais ravi d'avoir ton avis sur la requête qui est faite dans la vue.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Tu peux me linker la ligne stp, que je puisse regarder ça quand j'aurai
letemps, ma pause vient de se terminer :p

2016-01-18 10:26 GMT+01:00 Gérard Paligot notifications@github.com:

In templates/base.html
#3301 (comment)
:

@@ -279,11 +279,11 @@
{# MESSAGERIE PRIVEE #}


{% with topics=user|interventions_privatetopics %}

  •                                {% with unread_topics=topics.unread %}
    
  •                                {% with total_topics=topics.total %}
    
  •                                {% with notifications=topics.notifications %}
    

Tu es bien plus compétent que moi sur la question. Je serais ravi d'avoir
ton avis sur la requête qui est faite dans la vue.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r49975790.

@@ -104,6 +111,8 @@ def perform_hide_message(request, post, user, data):
post.text_hidden = data.get('text_hidden', '')

messages.success(request, _(u'Le message est désormais masqué.'))
for user in Notification.objects.get_users_for_unread_notification_on(post):
signals.content_read.send(sender=post.topic.__class__, instance=post.topic, user=user)

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Juste pour dire : j'aime !


subscriptions = TopicAnswerSubscription.objects.get_subscriptions(topic)
for subscription in subscriptions:
if not forum.can_read(subscription.profile.user):

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

J'ai la vague impression que l'ensemble "condition + desactivate + mark as read" est un bloc unitaire qui peut être réutilisable à plein d'endroit. Je ne serais pas contre un "extract method => move to utils".

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

J'y ai pensé aussi mais c'est pas toujours le cas. Du coup, j'ai préféré ne pas le faire mais ça peut changer.

@firm1

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

@firm1 Si tu pouvais faire une relecture avant d'être au merge, j'apprécierais. :-°

Dès que je trouve une bonne demie heure. Mais ça ne sera pas pour aujourd'hui. :(

@@ -364,9 +359,12 @@ def first_unread_post(self):
"""
# TODO: Why 2 nearly-identical functions? What is the functional need of these 2 things?

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

donné qu'on est dans une zep : la réponse à ce TODO est-elle trouvée?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

C'est quoi l'autre fonction ? Dans le cadre de cette ZEP, je ne fais appel à cette fonction que pour le script de migration des anciennes données vers les nouvelles.

self.user.username)
tops = []
cpt = 1
for topic in topics:

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

A remplacer par un collection.Counter puis prendre les 5 premiers éléments.
De plus ce genre de choses, ça doit être paramétré dans le ZDS_APP.

Enfin : il y a peu j'ai fait joujou avec cette requête : elle est plutôt facile à régler avec un annotate.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Comme le dit le TODO, cette méthode n'est pas utilisée. Nulle part. Même pas dans ma ZEP. Elle est dans le diff uniquement parce que j'ai changé de place TopicFollowed et Git n'a pas été assez intelligent pour me faire un diff correct.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

En fait la question est là car comme on est dans une ZEP, c'est le meilleur
moment pour décrasser un code mal fichu. Si tu dis que la méthode est
inutile (vérifie bien qu'elle n'est pas appelée dans les templates)
dézingue là, les zep sont là pour ça.

Le 18 janvier 2016 à 10:30, Gérard Paligot notifications@github.com a
écrit :

In zds/forum/models.py
#3301 (comment)
:

  • def unicode(self):
  •    return u'<Sujet "{0}" suivi par {1}>'.format(self.topic.title,
    
  •                                                 self.user.username)
    
  • tops = []
  • cpt = 1
  • for topic in topics:

Comme le dit le TODO, cette méthode n'est pas utilisée. Nulle part. Même
pas dans ma ZEP. Elle est dans le diff uniquement parce que j'ai changé de
place TopicFollowed et Git n'a pas été assez intelligent pour me faire un
diff correct.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r49976252.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Les ZEP ne sont pas forcément là pour ça. On peut faire des PR clean code et puisque cette méthode n'a rien à voir avec ma ZEP, je préferais faire une autre PR.

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Jan 18, 2016

Member

Je suis 100% pour le nettoyage de code, mais c'est sans doute plus simple de le faire dans une autre PR -- tant que ça ne nécessite pas de maintenir du code inutile pendant la ZEP.

TopicFollowed(topic=topic1, user=user1, email=True).save()
TopicFollowed(topic=topic1, user=user2, email=True).save()
TopicFollowed(topic=topic1, user=self.user, email=True).save()
follow_by_email(topic1, user1)

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

J'aime vraiment ton travail :)

except TopicFollowed.DoesNotExist:
pass
self.assertIsNotNone(TopicAnswerSubscription.objects.get_existing(
profile, topic, is_active=True, by_email=False))

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Peut-on faire une autre assertion sur une vue?

I.E faire un client.get et ensuite un assertIn?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Dans le cadre des anciens tests, j'ai vraiment tenté de reproduire le plus fidèlement possible les anciens tests. Je pourrais le faire mais j'aimerais me tenir à cette règle puisque les vrais tests concernant les souscriptions et les notifications figurent dans leur nouveau module.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Ok, parfait :p

2016-01-18 10:32 GMT+01:00 Gérard Paligot notifications@github.com:

In zds/forum/tests/tests_views.py
#3301 (comment)
:

     response = self.client.post(reverse('topic-edit'), data, follow=False)

     self.assertEqual(302, response.status_code)
  •    try:
    
  •        TopicFollowed.objects.get(topic=topic, user=profile.user, email=True)
    
  •        self.fail()
    
  •    except TopicFollowed.DoesNotExist:
    
  •        pass
    
  •    self.assertIsNotNone(TopicAnswerSubscription.objects.get_existing(
    
  •        profile, topic, is_active=True, by_email=False))
    

Dans le cadre des anciens tests, j'ai vraiment tenté de reproduire le plus
fidèlement possible les anciens tests. Je pourrais le faire mais j'aimerais
me tenir à cette règle puisque les vrais tests concernant les souscriptions
et les notifications figurent dans leur nouveau module.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r49976431.

@@ -407,7 +408,8 @@ def unregister(request):
for topic in Topic.objects.filter(author=current):
topic.author = anonymous
topic.save()
TopicFollowed.objects.filter(user=current).delete()
Subscription.objects.filter(profile=request.user.profile).delete()

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Puisque tu utilises les événements, je pense que ce genre de chose c'est exactement ce qu'on veut faire avec cette technique, non? En plus ça permettra dans le futur de dire "j'ai visité telle page".

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Tu parles d'un receiver proche de ce que tu as fais pour les galeries d'un contenu ? Pourquoi pas, c'est vrai que ce n'est pas une mauvaise idée.

En plus ça permettra dans le futur de dire "j'ai visité telle page".

Par contre, je comprends pas cette phrase.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Exactement.

2016-01-18 10:33 GMT+01:00 Gérard Paligot notifications@github.com:

In zds/member/views.py
#3301 (comment)
:

@@ -407,7 +408,8 @@ def unregister(request):
for topic in Topic.objects.filter(author=current):
topic.author = anonymous
topic.save()

  • TopicFollowed.objects.filter(user=current).delete()
  • Subscription.objects.filter(profile=request.user.profile).delete()

Tu parles d'un receiver proche de ce que tu as fais pour les galeries d'un
contenu ? Pourquoi pas, c'est vrai que ce n'est pas une mauvaise idée.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r49976522.

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Par contre, je comprends pas cette phrase.

pour l'instant quand un message est visité, il suffit de visiter une des pages pour que la notification disparaisse complètement. Une bonne chose serait de ne faire disparaître la notif que lorsqu'on clique sur la dernière page sinon la mettre à jour pour dire "page suivante".

C'est hors scope de cette PR. Mais créer un événement/receiver ça permettra de limiter le besoin de modification de code le jour où on l'implémentera. En plus c'est plus "élégant" et "orienté django" comme solution.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 18, 2016

Author Member

Ici, tu es quand même sur la vue qui désincrit le membre quand même. Donc, c'est pas super pertinent "J'ai visité telle page" mais je vois un peu l'idée. Par contre, je dépend des méthodes mark_read et chacune à sa propre implémentation.

Par exemple, pour le module des forums, il me semble bien que c'est marqué comme lu quand on arrive sur la page avec le message en question mais peut-être pas pour les autres comme les réactions d'un contenu que tu connais mieux que moi.

Comme tu dis, c'est hors scope de toute façon.

@@ -132,23 +113,12 @@ def interventions_topics(user):

@register.filter('interventions_privatetopics')
def interventions_privatetopics(user):

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

Il faudra lui ajouter un docstring + une doc dans frontend/templatetags

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

Done (mais Git pas assez intelligent).

{% with p=notification.sender %}
<img src="{{ p.get_avatar_url|remove_url_protocole }}" alt="" class="avatar">
{% endwith %}
<span class="username">{{ notification.sender.user.username }}</span>

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

notification.sender <=> p

Custom notification manager.
"""

def get_unread_notifications_of(self, profile):

This comment has been minimized.

Copy link
@artragis

artragis Jan 18, 2016

Member

à remplacer par :

    def get_unread_notifications_of(self, profile):
        """ get all notification for a user whose profile is passed as argument

        :param profile: user's profile object
        :type profile: zds.members.models.Profile
        :return: an iterable over notifications with user data already loaded
        :rtype: QuerySet
        """"
        return self.filter(subscription__profile=profile, is_read=False)\
                        .select_related("sender")\
                        .select_related("sender__user")

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

Done (mais Git pas assez intelligent).

verbose_name = _(u'Abonnement')
verbose_name_plural = _(u'Abonnements')

profile = models.ForeignKey(Profile, related_name='subscriptor', db_index=True)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor
  1. subscriptor n'est pas très anglais. Je suppose qu'on veut parler de subscriber ici ?
  2. Personnellement (et sans vouloir rentrer dans un débat), j'aurai tendanc à renseigner ici un User à la place d'un Profile pour les raisons suivantes :
    • Conserver l'uniformité avec le reste du code
    • lorsqu'on charge naturellement un Profile on charge tout plein de colonnes pas forcément utile pour le centre de notifications. Ce qui force à se discipliner à ne pas appeler n'importe comment une instance de Subscription jusqu'au jour ou on oublie de se discipliner ...
    • On rend fortement dépendant le centre de notifications aux code de ZdS, ce qui le rend inutilisable dans un autre contexte. Alors qu'en liant ceci au modèle User, on pourrait en faire un module qui ne dépend que de Django. Avoir des modules indépendant permet très souvent de mieux gérer leur interaction avec le code
    • Au final passer par le modèle User ne change pas grand chose puisque depuis une instance de User il suffit d'appeler user.profile pour avoir son instance de profile (vu qu'on a une relation OneToOne).

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Jan 19, 2016

Member

Au final passer par le modèle User ne change pas grand chose puisque depuis une instance de User il suffit d'appeler user.profile pour avoir son instance de profile (vu qu'on a une relation OneToOne).

Attention avec ce genre de chose : c'est facile côté code, mais ça a tendance à lancer une requête SQL à chaque fois et donc à exploser les perfs. À utiliser en connaissance de cause donc.

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

C'est bien le probleme. On voit tres nettement dans le reste du code que l'on passe surtout du temps a ecrire profile.user. ce qui me fait penser qu'on utilise pas le profile en vrai


msg = EmailMultiAlternatives(subject, message_txt, from_email, [receiver.email])
msg.attach_alternative(message_html, "text/html")
msg.send()

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Il faudrait catcher l'exception rencontrée si jamais le serveur mail à un souci.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Quelle exception ?

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Il faut vérifier mais tu peux avoir une SMTPException.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Super, merci.

content_type = models.ForeignKey(ContentType)
object_id = models.PositiveIntegerField()
content_object = GenericForeignKey('content_type', 'object_id')
is_read = models.BooleanField(_(u'Lue'), default=False, db_index=True)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Je m'interroge sur ce champ (is_read). Dans quel cas serait-il utile ? Avec ceci on a 3 cas :

  1. L'object notification n'existe pas => personne ne voit cette notification
  2. L'object notification existe, et la notification n'est pas lue => l'abonné voit cette notification
  3. L'object notification existe, et la notification est lue => personne ne vois cette notification

Pour moi le cas 1 et 3 sont semblable, et j'aurai tendance à penser que la lecture d'une notification devrait supprimer cet enregistrement dans la base. Et donc il faut voir la fonction mark_notification_read décrite plus haut

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Tu ne conserves aucun historique. C'est contraire à la spec de la ZEP.

This comment has been minimized.

Copy link
@vhf

vhf Jan 19, 2016

Contributor

Je connais pas les détails de la ZEP en question, mais je trouve très, très utile ce type de page : https://zestedesavoir.com/forums/notifications/

Et si on se met à supprimer les notifs lues, je vois pas comment on affiche ce genre d'infos.

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Je ne l'ai pas lu dans les détails, mais ça me surprend qu'on ait besoin de retrouver l'ensemble des notifications qui on déjà été lus. Tu pourrais me dire rapidement à quoi elle sont censées servir ?

This comment has been minimized.

Copy link
@vhf

This comment has been minimized.

Copy link
@vhf

vhf Jan 19, 2016

Contributor

Perso je préfère la solution 2, car on évite en même temps pour 1 topic de 15 pages pour lequel j'ai eu 14 notifications d'avoir dans mon historique 14 notifications non lues (ça ne me semble contre productif).

Bah, suffit d'ajouter un unique quelque part après avoir trié par -date.

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Jan 19, 2016

Member

Je saurais pas dire les détails techniques, je ne suis pas assez rentré dans le code pour ça.

Fonctionnellement, il faudrait un système qui :

  1. Permette une page de notification comme l'actuelle, i.e. une notification par élément (topic, …). Les notifications à la OC, avec N notifications par topic (une nouvelle à chaque fois qu'on a vu le topic) sont inutilisables. De plus, ça limite naturellement la taille de la table en base.
  2. Qui ne fasse pas des milliards de requêtes. Le système actuel est fonctionnellement très bon sur les forums, mais est atroce en terme de performances.

Un petit calcul : aujourd'hui on a environ 4000 utilisateurs, 140 tutos, 110 articles, 5000 topics et 85000 posts. En moyenne 16 posts par membre (donc max 16 topics suivis par membre en moyenne).

Voyons dans le futur ambitieux : 50 000 utilisateurs, qui ont chacun en moyenne 300 notifications. On se retrouve, avec ces hypothèses larges, avec une table de 21 millions de lignes, ce qui commence à être velu mais reste tout à fait gérable par MySQL -- surtout que si on en arrive à de telles volumétries, on aura un serveur plus costaud que celui qu'on a aujourd'hui.

Donc, on a pas besoin de se prendre la tête avec des calculs qui font des tonnes de requêtes ou des montages tordus pour limiter la taille de la table.

Est-ce que ça vous convient comme réponse ?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

Alors je propose la modification suivante :

  • J'ai aucune notification pour un contenu notifiable donné : Je crée une notification.
  • J'ai une notification marquée comme non lue : Je ne fais rien.
  • J'ai une notification marquée comme lue : Je la récupère, je la met à jour et je la marque non lue.

Résultat : la requête sera super simple, performante et on aura pas une multiplication des notifications à outrance (à la OC).

On part là dessus ?

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Jan 21, 2016

Member

Ok
Le 21 janv. 2016 18:48, "Gérard Paligot" notifications@github.com a
écrit :

In zds/notification/models.py
#3301 (comment)
:

+class Notification(models.Model):

  • """
  • A notification
  • """
  • class Meta:
  •    verbose_name = _(u'Notification')
    
  •    verbose_name_plural = _(u'Notifications')
    
  • subscription = models.ForeignKey(u'Subscription', related_name='subscription', db_index=True)
  • pubdate = models.DateTimeField(_(u'Date de création'), auto_now_add=True, db_index=True)
  • content_type = models.ForeignKey(ContentType)
  • object_id = models.PositiveIntegerField()
  • content_object = GenericForeignKey('content_type', 'object_id')
  • is_read = models.BooleanField(_(u'Lue'), default=False, db_index=True)

Alors je propose la modification suivante :

  • J'ai aucune notification pour un contenu notifiable donné : Je crée
    une notification.
  • J'ai une notification marquée comme non lue : Je ne fais rien.
  • J'ai une notification marquée comme lue : Je la récupère, je la met
    à jour et je la marque non lue.

Résultat : la requête sera super simple, performante et on aura pas une
multiplication des notifications à outrance (à la OC).

On part là dessus ?


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r50435772.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 22, 2016

Author Member

Done.

object_id = models.PositiveIntegerField()
content_object = GenericForeignKey('content_type', 'object_id')
is_read = models.BooleanField(_(u'Lue'), default=False, db_index=True)
url = models.CharField('Titre', max_length=200)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor
  1. Pourquoi l'url s'appelle Titre ?
  2. Pourquoi le champ url est limité à 200 caractère ? on a des liens ( celui-ci par exemple) de tutoriels qu'on aimerait pouvoir suivre qui peuvent aller au delà

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Qu'est-ce que je mets comme maximum ?

This comment has been minimized.

Copy link
@vhf

vhf Jan 19, 2016

Contributor

Si c'est un titre, la même limite que pour les autres titres. Si c'est une url, c'est URLField avec un max_length de 255. C'est le max qui est garanti par django pour CharField et URLField sauf erreur. Mais faudrait ajouter un check en amont pour être sûr qu'on dépasse jamais 255. Ou juste aller jeter un coup d'oeil au slugifier et faire quelques calculs.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Le problème, c'est qu'on dispose de pages avec des URL bien plus grandes, cf. le lien donné par @firm1.

This comment has been minimized.

Copy link
@vhf

vhf Jan 19, 2016

Contributor

Plus grande de -35 caractères. ;)

Quelques options :

  • Vérifier que les routes ne peuvent pas dépasser 255 ancre comprise (faisable mais galère, doit prendre en compte le NDD utilisé et prévoir une marge suffisante pour les PKs)
  • Utiliser un TextField ( 👊 )
  • Prioriser la ZEP 38 (qui est implémentable en 2-2)

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

@vhf Je te propose ceci : Dans un premier temps, on monte la limite à 255 caractères. Dans un second temps, on utilisera les URL raccourcis quand la ZEP 38 sera développée. Parce qu'il faut bien se dire qu'arriver à une URL de 255 caractères est difficilement faisable. L'exemple cité par @firm1 n'est pas valide puisque son URL pointe vers un contenu non notifiable.

This comment has been minimized.

Copy link
@vhf

vhf Jan 21, 2016

Contributor

👍 pour moi @GerardPaligot, et c'est de toute façon à @SpaceFox de prendre la décision.

content_object = GenericForeignKey('content_type', 'object_id')
is_read = models.BooleanField(_(u'Lue'), default=False, db_index=True)
url = models.CharField('Titre', max_length=200)
sender = models.ForeignKey(Profile, related_name='sender', db_index=True)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Idem ici, je passerait plutôt par User à la place Profile

This comment has been minimized.

Copy link
@pierre-24

pierre-24 Jan 19, 2016

Member

Justement, non. Je rappelle qu'on est toujours sous le coup de la désynchronisation entre User/Profile.

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Plus maintenant depuis que l'utilisateur décal à fait son apparition dans le code.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Faut statuer une bonne fois pour toute sur l'issue associée.

This comment has been minimized.

Copy link
@vhf

vhf Jan 19, 2016

Contributor

Je rappelle qu'on est toujours sous le coup de la désynchronisation entre User/Profile.

On sera toujours sous le coup de cette désynchronisation. Je l'ai déjà dit, mais cette désynchronisation est normale, logique, et attendue. Les SGBD utilisés ici ne permettent pas de placer des contraintes de synchronisation de PK entre des tables. Je suppose même qu'aucun SGBD n'offre ce genre de feature inutile.

Comme l'a dit @firm1, utiliser User.pk permet de réduire le coupling app<->ZdS.

This comment has been minimized.

Copy link
@pierre-24

pierre-24 Jan 19, 2016

Member

Alors effectivement, autant se décider une bonne fois pour toute ;)

(juste que je suis quasi certain qu'on avait choisi le chemin opposé à un précédent ZM, à savoir tout basculer en Profile. Soit, peu importe de toute façon :) )

self.user.username)


def follow(topic, user=None):

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Peut-être en profiter pour renommer cette fonction follow_or_unfollow car c'est bien ce qu'elle fait en réalité.

This comment has been minimized.

Copy link
@artragis

artragis Jan 19, 2016

Member

toogle_following_state car c'est ça qu'elle fait. Elle ne choisit pas au hasard de suivre ou de ne pas suivre.


assert hasattr(self, "module")

subject = _(u"{} - {} : {}").format(settings.ZDS_APP['site']['litteral_name'], self.module, notification.title)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

on a un module qui s'appelle Tutorialv2 (voir plus bas), est-ce que ça va de mettre ça dans l'objet du mail à envoyer ? De plus les autres modules sont toujours écrits en anglais, ça risque de faire bizarre de recevoir un mail de ZdS donc l'objet en en anglais.

This comment has been minimized.

Copy link
@gustavi

gustavi Jan 19, 2016

Member

oui on a un tutorialv2, c'est moche, il faudra le renommer.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

@gustavi C'est pas forcément lié au réel nom du module.
@firm1 Je vais améliorer ça.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 21, 2016

Author Member

Done.

'author': reaction.author,
'title': content.title,
'url': reaction.get_absolute_url()})
private_topic = ContentType.objects.get_for_model(PrivateTopic)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

ça me parait bizarre cette ligne. On se trouve dans la fonction interventions_topics mais malgré ça, on charge un private_topic. Mauvais copié/coller ?

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Jan 19, 2016

Author Member

Non, ça va me permettre d'exclure les notifications avec ce Content-Type.

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Effectivement, je n'ai pas vu le exclude

This comment has been minimized.

Copy link
@artragis

artragis Jan 19, 2016

Member

Bon du coup que mon "non." n'est pas passé sur gmail, va falloir que je dise un truc intelligent :

  • du coup plutôt que "private_topic" nomme_le "excluded_content_type".
  • ou bien ajoute un commentaire car même si j'avais compris, c'est vrai que ça fait bizarre comme ça au milieu de intervention_topics
private_topic = ContentType.objects.get_for_model(PrivateTopic)
notifications = list(Notification.objects
.get_unread_notifications_of(Profile.objects.get(user=user))
.filter(subscription__content_type=private_topic)

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

Ici on est à l'intérieur d'un filter, la condition subscription__content_type=private_topic devrait s'écrire subscription__content_type="private_topic"

This comment has been minimized.

Copy link
@artragis

artragis Jan 19, 2016

Member

Non.

2016-01-19 16:24 GMT+01:00 firm1 notifications@github.com:

In zds/utils/templatetags/interventions.py
#3301 (comment)
:

  •    select distinct t.*
    
  •    from mp_privatetopic t
    
  •    left outer join mp_privatetopic_participants p on p.privatetopic_id = t.id
    
  •    left outer join mp_privatetopicread r on r.user_id = %s and r.privatepost_id = t.last_message_id
    
  •    where (t.author_id = %s or p.user_id = %s)
    
  •      and r.id is null
    
  •    order by t.pubdate desc''',
    

- [user.id, user.id, user.id])

  • "total" re-do the query, but there is no other way to get the length as len is not available on raw queries.

  • topics = list(privatetopics_unread)
  • return {'unread': topics, 'total': len(topics)}
  • private_topic = ContentType.objects.get_for_model(PrivateTopic)
  • notifications = list(Notification.objects
  •                     .get_unread_notifications_of(Profile.objects.get(user=user))
    
  •                     .filter(subscription__content_type=private_topic)
    

Ici on est à l'intérieur d'un filter, la condition
subscription__content_type=private_topic devrait s'écrire
subscription__content_type="private_topic"


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3301/files#r50125104.

@firm1

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

Autres remarques de manière générale sur la PR actuelle :

  • Elle ne builde pas coté travis (il semble manquer des migrations)
  • Les noms de templates sont un poil difficiles à lire je pense par exemple à templates/email/notification/topicanswersubscription.html qui (selon la snake_case à la python ) devrait être templates/email/notification/topic_answer_subscription.html
sender=private_topic.last_message.author.profile,
send_email=participant.profile.email_for_answer)

if action == 'post_remove' and not reverse:

This comment has been minimized.

Copy link
@firm1

firm1 Jan 19, 2016

Contributor

c'est de la micro optimisation, mais dans ce genre de cas, le elif vaut toujours mieux qu'une suite de if

@GerardPaligot GerardPaligot force-pushed the GerardPaligot:new_zep_24 branch from 8e90b4b to 60a9b96 Mar 4, 2016

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2016

PR rebased et j'ai corrigé la petite erreur de @DevHugo

@SpaceFox

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

Dans dev non ? À moins que @GerardPaligot ou @ChantyTaguan aient un avis contraire ?

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2016

Pas du tout, dev c'est très bien.

@DevHugo

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2016

@gustavi on compte sur toi pour la qa. J'aurais le temps de rien faire moi

@gustavi

This comment has been minimized.

Copy link
Member

commented Mar 5, 2016

Ça sera check auj.


def get_users_for_unread_notification_on(self, content_object):
"""
Gets all users which have an notification unread on the given content object.

This comment has been minimized.

Copy link
@vhf

vhf Mar 5, 2016

Contributor
-which
+who
class TopicFollowedManager(models.Manager):
def get_followers_by_email(self, topic):
"""
:return: the set of users that follows this topic by email.

This comment has been minimized.

Copy link
@vhf

vhf Mar 5, 2016

Contributor
-that follows
+who follow
def is_followed(self, topic, user=None):
"""
Checks if the user follows this topic.
:param user: An user. If undefined, the current user is used.

This comment has been minimized.

Copy link
@vhf

vhf Mar 5, 2016

Contributor
-An
+A
gustavi added a commit that referenced this pull request Mar 5, 2016
Merge pull request #3301 from GerardPaligot/new_zep_24
ZEP-24 : Centre de notifications

@gustavi gustavi merged commit 9326809 into zestedesavoir:dev Mar 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gustavi

This comment has been minimized.

Copy link
Member

commented Mar 5, 2016

Voilà voilà <3

@gustavi gustavi added this to the Version de développement milestone Mar 5, 2016

@GerardPaligot GerardPaligot deleted the GerardPaligot:new_zep_24 branch Mar 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.