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

Permet de marquer ses notifications comme lues #4079

Merged
merged 3 commits into from Dec 29, 2016

Conversation

@gcodeur
Copy link
Member

commented Dec 15, 2016

Q R
Type de modification nouvelle fonctionnalité
Ticket(s) (issue(s)) concerné(s) ce sujet

Cette pull request ajoute une nouvelle fonctionnalité : il est maintenant possible de marquer toutes ses notifications comme lues.

C'est ma première « grosse pull request », donc il y a peut-être des choses que j'ai mal faites. N'hésitez pas à les signaler pour que j'améliore ça.

QA

  • Créer des notifications pour un utilisateur (n'hésitez pas à tester différents biais : forums, contenus, ...) ;
  • Vérifier que la nouvelle fonctionnalité fonctionne correctement et marque ces notifications comme lues ;
  • Vérifier que rien d'étrange ne semble avoir été introduit sur les notifications ;
  • Vérifier que le test zds.notification.tests.tests_basics.NotificationTest.test_mark_notifications_as_read fonctionne.
@@ -769,3 +769,21 @@ def test_new_cowritten_content_without_doubly_notif(self):
self.assertIsNotNone(auto_user_1_sub)
notifs = list(Notification.objects.get_notifications_of(author1.user))
self.assertEqual(1, len(notifs))

def test_mark_notifications_as_read(self):
category = CategoryFactory(position=1)

This comment has been minimized.

Copy link
@Anto59290

Anto59290 Dec 15, 2016

Contributor

Je pense que tu peux faire plus condensé en faisant ainsi

category, forum = create_category()
topic = add_topic_in_a_forum(forum, profile)

Tu peux trouver des exemples dans /zds/forums/api/tests.py

This comment has been minimized.

Copy link
@gcodeur

gcodeur Dec 15, 2016

Author Member

(On en a déjà parlé sur IRC, mais je le note ici pour ceux qui n'auraient pas lu la discu.)

J'ai utilisé le même code que sur les précédents tests pour créer ces objets, donc c'est au final plus cohérent de laisser tel quel. ;)

@coveralls

This comment has been minimized.

Copy link

commented Dec 15, 2016

Coverage Status

Coverage increased (+0.01%) to 87.101% when pulling fcf83a8 on GCodeur:marquer_notifs_lues into 7d5e49c on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Copy link

commented Dec 15, 2016

Coverage Status

Coverage decreased (-32.7%) to 54.419% when pulling fcf83a8 on GCodeur:marquer_notifs_lues into 7d5e49c on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2016

Coverage Status

Coverage increased (+0.01%) to 87.101% when pulling 33f17d8 on GCodeur:marquer_notifs_lues into ab1275d on zestedesavoir:dev.

@Anto59290

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2016

QA : OK
Par contre je n'aurais pas placé l'action là. En fait sans regarde le code je pense que je l'aurait jamais trouvé. Peut-on imagine un bouton style android ou iphone directement dans la liste des notifs pour les marquer comme lu. Je pense à un truc dans ce style la :
http://www.cs.dartmouth.edu/~campbell/cs65/lecture05/images/notificationsscreen.png

J'en appelle à @FanJiyong ! Aujourd'hui c'est implémenté comme ça, sur la page des notifications. Qu'en pense-tu ?
notifs

@WinXaito

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2016

Merci @gcodeur tu me vends tu rêve <3

@gcodeur

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2016

Je pensais qu'il était plutôt évident de le mettre ici, car c'est là qu'on a mis toutes les actions sur les pages (pages de profil, sujets, etc). Du coup, je le vois assez mal en haut, d'autant que ça devrait être assez chargé vu qu'il n'y aura rien à gauche.

De plus, je maîtrise assez mal le front, donc pas sûr que je saurais m'en occuper. :/

En tout cas, merci d'avoir fait la QA. :)

@FanJiyong

This comment has been minimized.

Copy link

commented Dec 23, 2016

@FanJiyong

This comment has been minimized.

Copy link

commented Dec 23, 2016

<_<

161223_notifications

@Anto59290

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2016

Sympa la maquette, c'est plus aéré que ce que l'on a aujourd'hui. Dans ce cas cette PR à réussi son objectif et on ferra une deuxième itération pour refondre l'interface. Qu'en pensez vous ?

@FanJiyong

This comment has been minimized.

Copy link

commented Dec 23, 2016

@gcodeur

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2016

OK, partons là-dessus. :)

J'aime également beaucoup ta maquette, mais n'est-ce pas un peu trop de le faire jour par jour ? Sur les sujets suivis, on a aujourd'hui, hier, puis les 7 derniers jours et je trouve ça pas mal. Je pense qu'on peut créer un sujet sur le forum pour en discuter.

@Emeric54 Emeric54 merged commit af67e28 into zestedesavoir:dev Dec 29, 2016

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

Merci !

@FanJiyong

This comment has been minimized.

Copy link

commented Jan 4, 2017

Pour les jours :

  • aujourd'hui,
  • hier,
  • puis les 7 derniers jours
    Ok,

En suite par mois ça me va.

@gcodeur gcodeur deleted the gcodeur:marquer_notifs_lues branch Jul 17, 2017

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