Nouvelle version markdown supportant @ping #3945
Conversation
cgabard
changed the title
|
Congrats @cgabard pour ce taff ! J'ai hâte de débloquer du temps pour développer le ping. Tu aurais une doc technique quelque part pour l'usage du ping dans zds-site ? |
|
Je ne sais pas trop comment et quelles fonctions sont utilisées par qui dans zds-site mais en soit il faut :
J'ai assez peu de connaissance du fonctionnement du site mais avec ça tu devrais pouvoir t'en sortir : il faut fournir une fonction qui dit si le nom est valide et le markdown renvoie les noms trouvés en plus du texte. |
|
N'hésite pas à me dire/contacter si tu as des questions et/ou si tu as besoin d'autre chose pour te faciliter la vie. Globalement c'est la reprise de ton commit légèrement complété. |
|
Hum, il y a un truc qui me gène actuellement et je crois que je vais changer un détail. Je pense que la fonction va devoir me renvoyer une url si le ping est possible et Tu en pense quoi ? |
|
Que c'est une super idée. (: |
| @@ -21,14 +21,14 @@ | |||
| __MD_ERROR_PARSING = _(u'Une erreur est survenue dans la génération de texte Markdown. Veuillez rapporter le bug.') | |||
|
|
|||
|
|
|||
| def get_markdown_instance(inline=False, js_support=False): | |||
| def get_markdown_instance(inline=False, js_support=False, is_pingeable=None): | |||
There was a problem hiding this comment.
Plutôt pingable, donc ici is_pingable, que pingeable ;)
Idem plus bas.
There was a problem hiding this comment.
Oui du coup la fonction/paramètre va probablement être renommée ping_url ou un truc du genre.
cgabard
added
S-BUG
|
Ok, nouvelle version. La seule différence est que la fonction, maintenant nommée Le reste c'est du ressort de zds et n'a pas besoin de venir dans cette PR |
|
@cgabard Alors on a un bug dans Python-ZMarkdown. J'ai commencé à développer les notifications pour les pings et j'ai donc voulu passer ma fonction à l'extension J'ai un peu regardé comment sont placées les valeurs dans le dictionnaire Voilà. J'espère que j'ai été clair. (: |
|
@vhf ta "request change" est réglé. Pour moi c'est pret pour la QA. Je laisse @GerardPaligot donner des indications sur la QA du ping, c'est lui qui s'en est occupé ! |
|
Ok. Par contre, QA faisant, va falloir rebaser :-° |
|
@GerardPaligot tu veux t'en occuper vu que ce sont tes modifs ou tu veux que je le fasse ? |
|
@pierre-24 Bon j'ai fait le rebase |
|
Bon bin j'ai raté mon rebase on dirait... |
| md_instance = get_markdown_instance(ping_url=ping_url) | ||
| self.text_html = render_markdown(md_instance, self.text) | ||
| self.save() | ||
| for username in md_instance.metadata.get("ping", []): |
There was a problem hiding this comment.
Je serais très favorable à un garde-fou, quitte à le retirer plus tard, voire à l'avoir dans les settings.
Je connais des sites communautaires qui ont introduit un système de ping similaire, et très vite des marioles en ont profité pour pinger 500 membres par messages.
Est-ce qu'on pourrait limiter à 10 ou 15 le nombres d'utilisateurs pingés dans un même message ? Genre les 10-15 premiers sont pingés, pas les suivants ? Vu qu'on a une limite d'un post par 15 minutes, ça me semblerait assez judicieux.
There was a problem hiding this comment.
La limite des 15 minutes n'est appliquable que sur un sujet. Tu peux créer plusieurs sujets et faire chier des membres avec des pings dans chacun de ces sujets.
C'est un problème.
There was a problem hiding this comment.
Yep. Mais je pense que ça vaut la peine d'implémenter cette limite de pingables/message dans cette PR.
Je pensais pas particulièrement à des gens qui veulent embêter, je pensais plus à des gens qui font du bruit en utilisant 'correctement' cette feature, correctement d'après son implémentation.
Si quelqu'un veut embêter et ping par exemple tous les membres sauf le staff dans un message, ça sera embêtant pour tous les membres et pas rapidement détectable par le staff. Si quelqu'un veut embêter en créant 1000 topics, tout le monde le verra et il sera vite viré.
There was a problem hiding this comment.
L'argument se tient. Je fais ça dès que possible.
There was a problem hiding this comment.
Petite question : Est-ce que l'on veut que les pings sans notification (car la limite atteinte) soient tout de même transformés en liens vers les auteurs ? Si oui pas de soucis, sinon il faudra une modif dans le markdown.
Au passage, on pourrait pas imaginer créer une alerte de validation automatiquement si la limite haute est atteinte ? Comme ça les modos seraient au courant que quelqu'un a mit plein de monde.
Enfin, vous en avez pas parlé mais coté markdown je renvoie un "set" et non une liste. Ça ne devrait pas avoir d'impact mais j'ai oublié de prévenir (ça me semblait le plus logique, l'ordre et le nombre de fois où une personne est pingué n'a pas d'intéret pour zds). La conséquence est aussi que plusieurs citations de la meme personne dans un meme message ne provoquera qu'une seule notif.
|
|
||
| def get_notification_title(self, answer): | ||
| assert hasattr(answer, 'author') | ||
| assert hasattr(answer, "get_notification_title") |
There was a problem hiding this comment.
(idem ici zds/notification/models.py)
| @receiver(content_read, sender=Post) | ||
| def mark_comment_read(sender, **kwargs): | ||
| comment = kwargs.get('instance') | ||
| user = kwargs.get('user') |
There was a problem hiding this comment.
Que se passe-t-il s'il manque un de ces deux ou ces deux arguments ?
There was a problem hiding this comment.
Sans doute un comportement non souhaité. La meilleure solution est l'invocation d'assert pour vérifier que les valeurs sont bien renseignées ?
| def answer_comment_event(sender, **kwargs): | ||
| comment = kwargs.get('instance') | ||
| user = kwargs.get('user') | ||
| by_email = kwargs.get('by_email') |
There was a problem hiding this comment.
A quel moment est-ce que by_email peut être True ?
There was a problem hiding this comment.
Aujourd'hui, ça sera jamais True.
There was a problem hiding this comment.
Du coup si c'est pas utile, il vaut mieux l'enlever.
There was a problem hiding this comment.
Le signal new_content est utilisé pour d'autres modèles et by_email n'est pas forcément à False dans ces cas là.
| assert hasattr(answer, 'author') | ||
| assert hasattr(answer, "get_notification_title") | ||
|
|
||
| return _(u'{0} vous a mentionné sur {1}.').format(answer.author, answer.get_notification_title()) |
There was a problem hiding this comment.
Je pense les tests plantent à cause de cette ligne.
There was a problem hiding this comment.
Au début de mon développement, les tests passaient et je suis quasi certain que cette ligne y était.
Tu serais dire pourquoi tu penses que c'est cette ligne qui pose problème ?
There was a problem hiding this comment.
Les logs travis disent TypeError: int() argument must be a string or a number, not 'SimpleLazyObject', et je pense que si tu vires l'i18n ici ça passe. Mais j'ai pas tiré ta PR en local pour tester, c'est qu'une hypothèse.
Et il vaut pas virer l'i18n si c'est le cas, il faut trouver une meilleure solution.
There was a problem hiding this comment.
En fait je reviens sur mon idée, c'est probablement pas là.
| @@ -172,6 +175,8 @@ def get_context_data(self, **kwargs): | |||
| else: | |||
| context['user_can_modify'] = [post.pk for post in context['posts'] if post.author == self.request.user] | |||
|
|
|||
| for post in posts: | |||
| signals.content_read.send(sender=post.__class__, instance=post, user=self.request.user) | |||
There was a problem hiding this comment.
Plutôt ici. Quand l'utilisateur n'est pas connecté, request.user est un SimpleLazyObject.
There was a problem hiding this comment.
Oh bien vu. Je corrige ça dès que possible.
|
Selon @GerardPaligot les tests vont fails mais il ne sait pas pourquoi, si quelqu'un a une idée... |
|
Plusieurs choses à savoir sur les 2 fails :
Ce dernier point peut être important, je soupçonne de plus en plus la dernière mise à jour de Python-ZMarkdown. |
|
Travis dit qu'il y a un conflit de migration. |
| for username in md_instance.metadata.get("ping", []): | ||
| signals.new_content.send( | ||
| sender=self.__class__, instance=self, user=User.objects.get(username=username), by_email=False) | ||
| for username in list(md_instance.metadata.get("ping", []))[:settings.ZDS_APP['comment']['max_pings']]: |
There was a problem hiding this comment.
ok, je vais le faire
|
Il suffit de passer Le dim. 20 nov. 2016 19:07, Gérard Paligot notifications@github.com a
|
|
C'est normal qu'il y ait pas de test sur le ping ici ? ça éviterai des regressions dans le futur. |
C'est parfait pour nous. Y'a des trucs à adapter de notre côté pour supporter cette nouvelle version de zmarkdown ? Si c'est le cas et que c'est déjà géré dans cette PR, pour moi on peut mettre un feature flag sur les pings et merger cette PR, quand elle aura les tests qui vont bien. Si c'est pas le cas, autant laisser la PR ouverte en attendant la release v21 et on fait passer les pings après si tout est stable. |
|
Non il n'y a aucune contre indication à merger cette pr. On peut facilement Le dim. 20 nov. 2016 22:15, victor felder notifications@github.com a
|
|
Super ! Alors je propose que vous ajoutiez un feature flag dans le settings.py qui soit True par défaut pour activer les pings. Si le flag n'est pas True, on met simplement Comme ça on a les pings derrière le flag, et travis passe dessus, et on s'évite un gros rebase, et on s'évite de l'instabilité en prod. |
| @@ -515,6 +515,9 @@ | |||
| 'topic': { | |||
| 'home_number': 6, | |||
| }, | |||
| 'comment': { | |||
| 'max_pings': 15 | |||
There was a problem hiding this comment.
J'ai pas compris ce commentaire.
There was a problem hiding this comment.
Il aimerait juste que tu ajoutes une virgule à la fin, comme après le 'home_number': 6,
|
Je vais m'occuper des modifs. Je vais faire ça dans la semaine, je vais en profiter pour mettre a jour le markdown car je suis tombé sur deux petits bug genant hier. Du coup je vais intégrer les modifs ici et désactiver les "nouveautés" (j'en ferait un ticket a part car il y a des modifs de codes à faire). Du coup je vais rajouter un |
|
Je sais pas si on est sur la même longueur d'onde @cgabard . Cette PR est bien pour moi, et c'est volontiers si tu la modifies avec une nouvelle version du markdown qui répare les 2 petits bugs que t'as trouvé. Par contre je comprends pas bien l'idée d'une autre PR ou de désactiver les nouveautés hors des settings. Je propose de garder le tout, mais de simplement désactiver le ping dans les settings du projet zds-site. Une variable dans les settings, et un check dans le code qui mets Comme ça on profite de toutes les nouveautés, le ping est simplement désactivé côté |
|
En fait il se trouve que depuis j'ai avancé sur le markdown et avec la version qui arriverait cela rajouterai des ancres sur les titres. Le truc est que j'aimerai pas l'activer pour l'instant car dans l'idéal pour ça, et pour les footnote, il faudrait a minima que le code zds fournisse un identifiant unique aux extrait à transformer (l'idée étant d'éviter les collisions d'id et de références quand il y a plusieurs extraits sur une meme page, dans un tuto ou sur le forum). Comme ça demande d'autre dev back, et que c'est le plus simple pour moi, je pensais le désactiver temporairement. Coté zds je ferait ce que tu as proposé pour les ping. Pour zds il n'y aurait que les corrections de bugs (et les modifs typographiques) qui seraient activés par cette PR. Le reste (ping et ancres) seraient dispo mais désactiver pour permettre leur développement a part. |
|
Bref on est bien sur la même longueur d'onde, la mise a jour pour corriger les bugs va juste m'obliger a pousser une autre nouvelle feature que je vais désactiver dans cette PR. |
|
Ok, on va attendre les tests mais normalement les pings sont désactivés et zmarkdown à jour |
|
Quelqu'un pour QA ça ? A priori il a juste a vérifier que le markdown fonctionne et que le ping ne fonctionne pas. D'ailleurs pour le ping, faudra bien vérifier que la prévisualisation n'en génère pas. Mais bref c'est hors-scope de cette PR du coup. |
|
C'est noté, QA pour probablement ce soir, sauf si quelqu'un est plus motivé d'ici là :) |
|
Bon,
|
|
Oui chrome les transforme. Essaie d'afficher la source au lieu d'utiliser l'inspecteur. Si c'est une entité HTML, tu la verras dans la source. Si c'est pas le cas et que c'est unicode, tu peux copier-coller le caractère dans https://codepoints.net/ et regarder ce que c'est. |
|
Ok, effectivement, quand on regarde le code de la page, ça le fait bien :)
Donc si on me confirme que mon bug de tableau est bien dû à mon CSS défaillant, on est bon :) |
|
@pierre-24 j'ai pas d'info sur le tableau, du coup je merge et si le build est correct sur la beta, c'est bon. Je te rappellerai de bien vérifier direct sur la beta, parce que je sais pas exactement quel est le problème que t'as rencontré. |
|
y'a du rebase à faire. |
|
Si quelqu'un veut le faire car je ne sais pas quand j'aurais un pc sous la
main pour le faire
Le mar. 29 nov. 2016 19:00, artragis <notifications@github.com> a écrit :
… y'a du rebase à faire.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3945 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF0jfJz97TWxMbG78rxeT0eqwNiy_mb3ks5rDGhCgaJpZM4Kqr3q>
.
|
|
Closed via 9709f1b, nécessaire à la résolution de conflit de cette PR. Merci @cgabard pour l'énorme travail, et @GerardPaligot pour l'énorme travail (2). Et @pierre-24 pour la QA. Ce sera en beta dès que le build est terminé. |
|
Il semble en effet y avoir un problème de centrage : https://scaleway.zestedesavoir.com/forums/sujet/7457/test-tableaux/#p132984 MAIS c'est encore pire en prod car la légende ne s'applique meme pas. En inspectant vite fait, je comprends pas trop le soucis de CSS. Je suis pas persuadé que ce soit la faute du markdown |
|
Tient, y'a aussi un problème avec les tableaux (#3879). Mais c'est probablement pas la faute de ZMarkdown, parce qu'il y a bien le
|
C'est @vhf qui l'a mise sur le topic de beta |
QA
Beaucoup de modifications. La principale est le support du ping grâce à @GerardPaligot
Concernant le markdown, il y a beaucoup de corrections de bugs, principalement :