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

ajoute l'attribut pub date aux flux RSS. #1118

Merged
merged 3 commits into from
Jul 11, 2014

Conversation

artragis
Copy link
Member

@artragis artragis commented Jul 8, 2014

Q R
Correction de bugs ? [oui
Nouvelle Fonctionnalité ? [non]
Tickets concernés [#1113]

Les flux RSS n'avaient pas d'attribut pubdate sur les item.
De plus un mauvaise concaténation venait tronquer le titre du flux
Les flux atom ne sont pas concernés.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 8cffb1c on artragis:add_pub_date into 8183218 on zestedesavoir:master.

@ArnaudCalmettes
Copy link
Contributor

/!\ On est encore en freeze.

@ArnaudCalmettes ArnaudCalmettes added this to the Version v1.x milestone Jul 9, 2014
@ArnaudCalmettes
Copy link
Contributor

Bon puisque le dev est fait, si la QA est en cours sur cette issue, ok pour la merger si ça passe du premier coup, sinon, on laisse de côté jusqu'à la fin du freeze.

@Eskimon
Copy link
Contributor

Eskimon commented Jul 10, 2014

Pour ma part, Freeze oblige je ferais passer mon temps en priorite sur les PR de bug et eventuellement la correction de bug.

Cependant ici les corrections sont vraiment legeres et facile a lire et comprendre. Je peux faire le merge si qqun me confirme que le champ apparait bien dans le feed. (poke @vulser)

@@ -12,7 +12,7 @@
class LastPostsFeedRSS(Feed):
title = u'Derniers messages sur Zeste de Savoir'
link = '/forums/'
description = u'Les derniers messages '
description = u'Les derniers messages '+ \
u'parus sur le forum de Zeste de Savoir.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour un texte multiligne, je te propose plutot de mettre ton texte dans un couple de parenthese plutot que de faire un '+'.
Ca donnerait ca:

description = (u'Les derniers messages '
      u'parus sur le forum de Zeste de Savoir.')

Tu peux trouver des exemples dans la view du member. L'avantage c'est qu'on aura un code plus homogène.

@artragis
Copy link
Member Author

Quand j'ai corrigé la faute, le tag "évolution" n'avait pas été mis, j'avais supposé que c'était un bug.
Comme y'a pas besoin de grosses modif, je vais tenter de faire l'édition directement sur github, et si ça marche pas, je verrai ce soir.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 0091f91 on artragis:add_pub_date into 8183218 on zestedesavoir:master.

@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

J'attends donc ce soir pour faire la QA :)

@artragis
Copy link
Member Author

bah elle a été faite la correction ^^.

@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

Pas vu... ^^ Testé et approuvé !

@Eskimon
Copy link
Contributor

Eskimon commented Jul 10, 2014

Quand tu dis teste c'est que c'est bon a droite a gauche sur plusieurs feed ? Si la reponse est oui je merge :)

@artragis
Copy link
Member Author

Comme ça n'a pas été signalé, je n'y avais pas fait attention, mais la remarque d'@Eskimon m'a rappelé que les articles ont aussi un flux RSS (mais pas lestutos d'après ce que j'ai vu) et que la même modification doit y être mise en place.

@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

@Eskimon La description qui a été changée s'affiche correctement sur les deux flux (messages et sujets). La pubDate s'affiche bien aussi sur ces deux flux. J'ai regardé le RSS généré et la pubDate est au bon format et est insérée dans le bon nœud parent.

EDIT : erreur de ma part

@artragis
Copy link
Member Author

La pubdate s'affiche aussi sur les articles?

@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

Non, c'est moi qui me suis trompé (décidément !). J'ai l'impression que le flux RSS des articles n'est pas complet de toute façon.

@Eskimon
Copy link
Contributor

Eskimon commented Jul 10, 2014

Du coup on est comment la ? Les articles marchent pas du tout ?

@Eskimon Eskimon added the Facile label Jul 10, 2014
@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

Le RSS des articles ne fonctionne pas vu ce qu'il contient : http://zestedesavoir.com/articles/flux/rss/ Mais ça devrait être corrigé dans une autre PR.

@Eskimon
Copy link
Contributor

Eskimon commented Jul 10, 2014

Ok, un ticket existe a ce sujet ?

Si oui, tu peux le linker ici pour le suivi, je merge et je ferme.

Sinon, peux-tu le créer (et linker ici) pour expliquer le souci, puis je merge et ferme :)

@0hexit
Copy link
Contributor

0hexit commented Jul 10, 2014

Done!

@artragis
Copy link
Member Author

Dois-je agrémenter cette pull request de la correction de #1146 ou bien peut-elle être mergée comme correction de #1113

@Eskimon
Copy link
Contributor

Eskimon commented Jul 11, 2014

Le titre/description initiale cible un probleme particulier, je pense que cette PR doit rescpecter son probleme originale (qui est a la limite de la feature non ?). Je merge donc maintenant celle ci et la 1146 (RSS des articles) fera l'objet d'une correction a part entiere. Ca rends les choses plus atomiques, evite d'eparpiller les modifs faites en une PR et simplifie a la fin si on fait des recherches dans l'historique de GH.

En un mot comme en cent : Je merge, merci a tous ! ;)

Eskimon added a commit that referenced this pull request Jul 11, 2014
ajoute l'attribut pub date aux flux RSS.
@Eskimon Eskimon merged commit 613289a into zestedesavoir:master Jul 11, 2014
@artragis artragis deleted the add_pub_date branch July 11, 2014 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Facile Bon ticket pour débuter pour rejoindre le développement !
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants