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

Refactorisation des vues du module des MPs #2398

Merged
merged 8 commits into from Mar 12, 2015

Conversation

@GerardPaligot
Copy link
Member

commented Mar 4, 2015

Q R
Correction de bugs ? Non
Nouvelle Fonctionnalité ? Non
Tickets (issues) concernés Pre-ZEP-23

Passage de toutes les vues à l'utilisation des vues génériques.

QA : Vérifier que le module des MPs fonctionne toujours correctement. Voici les fonctionnalités :

  • Lister les MPs
  • Quitter une liste de MPs
  • Créer un MP
  • Quitter un MP donné
  • Ajouter des participants à un MP
  • Afficher la liste messages d'un MP
  • Ajouter un message à un MP
  • Editer le dernier message d'un MP
@landscape-bot

This comment has been minimized.

Copy link

commented Mar 4, 2015

Code Health
Repository health increased by 0.03% when pulling 39846e0 on GerardPaligot:refacto_mp into 5098d3f on zestedesavoir:dev.

# Home
url(r'^$', 'zds.mp.views.index'),
# Posts.
url(r'^(?P<pk>\d+)/messages/$', PrivatePostList.as_view(), name='posts-private-list'),

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

L'utilisation de "posts" me choque. "post" est aussi utilisé pour parler des messages du forum ou des commentaires !

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

Techniquement, il n'y a quasi aucune différence entre un sujet sur les forums et un MP. Du coup, c'est normal de retrouver Post dans les MPs. D'ailleurs, on parle plus souvent de PrivatePost.

Une fois que nous aurons refactorisé le module forum, nous pourrions d'ailleurs voir s'il y a moyen de réutiliser du code, voire de fusionner les deux modules (bien que j'ai des doutes pour sur ce dernier point).

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

Ah, j'avais pas capté le "-private" après ! AMHA, ce serait plus clair avec "private-posts" ;)

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

Effectivement, je peux changer ça.

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

On ne met plus le slug ?

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

AMHA le mieux serait de ne mettre que le slug mais je sais pas s'il sont unique ou pas dans la base de donnée. Actuellement on met les deux pour presque tout (tutoriels, articles, sujets du forum) mais bientôt les tutoriels et articles n'auront que le slug je crois.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

Non, je pense que dans tous les cas l'identifiant est nécessaire justement.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 6, 2015

Author Member

ping des avis pour ce point.

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Mar 6, 2015

Member

C'est des URLs internes, donc globalement on s'en fout un peu. Cela dit :

  1. La solution retenue doit être cohérente avec le reste du site
  2. S'il y a ID + Slug, une URL de type ID + mauvais SLUG doit renvoyer vers la vraie URL

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 6, 2015

Author Member

Dans tous les cas, le slug ne sera là que pour l'utilisateur. Il ne sera pas utilisé dans le back-end. Donc un mauvais slug ne renverra pas vers une 404 mais vers le MP de l'identifiant donné. Est-ce bien un comportement souhaité ?

url(r'^$', 'zds.mp.views.index'),
# Posts.
url(r'^(?P<pk>\d+)/messages/$', PrivatePostList.as_view(), name='posts-private-list'),
url(r'^(?P<pk>\d+)/message/creer/$', PrivatePostAnswer.as_view(), name='posts-private-new'),

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

Pourquoi ne pas mettre un "s", ce qui donnerait "/messages/" ? Surtout que dans le nom de l'url, "posts" est au pluriel !

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

C'est une erreur. Je vais corriger ça.

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

D'ailleurs, il y a des modifications d'url, non ? SI oui, il faut faire des redirections.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

Ces URLs sont privées. Les moteurs de recherche n'y ont pas accès. Des redirections ne sont pas nécessaires.

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

Ça dépend si on prend en compte ou pas la possibilité que certains utilisateurs ont ajouté des liens de favoris.

This comment has been minimized.

Copy link
@GerardPaligot

GerardPaligot Mar 5, 2015

Author Member

On va faire des redirections pour les 2-3 personnes qui ont mis des MPs en favoris ?

This comment has been minimized.

Copy link
@Situphen

Situphen Mar 5, 2015

Contributor

@SpaceFox : un avis ?

This comment has been minimized.

Copy link
@SpaceFox

SpaceFox Mar 6, 2015

Member

On va faire des redirections pour les 2-3 personnes qui ont mis des MPs en favoris ?

Non. Ça me paraît complètement overkill.

@Situphen

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2015

Ce n'est pas vraiment le sujet de la PR mais je propose que puisque tu vas développer l'API des mps, tu renommes le dossier en "pm", ce qui correspondrait à "private message" et qui est mieux que l'actuel "mp" français pour un projet ou tout le code est en anglais !

@SpaceFox

This comment has been minimized.

Copy link
Member

commented Mar 5, 2015

Ca, c'est pas idiot !

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2015

C'est quelque chose que je peux faire oui. :)

@ChantyTaguan

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

Hello,

Rapport de QA

Je viens de tester toute les fonctionnalités, et tout a l'air correct !

Par contre, c'est normal qu'on puisse éditer un message même une fois qu'il a été lu ? J'ai vérifié en prod et ça a l'air d'être la même chose mais il me semblait qu'on ne pouvait pas. Changement ou régression en prod (et ici du coup) ?

@SpaceFox

This comment has been minimized.

Copy link
Member

commented Mar 7, 2015

Il me semble qu'on peut toujours éditer le dernier message même s'il a été lu.

@ChantyTaguan

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

J'ai rien dit, petit bug !

Quand on utilise l'auto complétion, une virgule et un espace sont automatiquement rajoutés après le pseudo choisi. Si on mets plus qu'un user lorsque l'on crée la conversation, ça provoque une erreur lors de la vérification des pseudo. L'espace est pris en compte comme s'il faisait partie du pseudo.

Il faudrait rajouter un trim() après le split (y en a un avant).

@SpaceFox, tu veux dire que c'est normal ou que tu as repéré le bug aussi ???

@SpaceFox

This comment has been minimized.

Copy link
Member

commented Mar 7, 2015

Pour moi c'était le comportement normal.

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2015

Je confirme. Le dernier message lu peut toujours être édité.

Pour résumé ce que je dois faire encore sur cette PR :

  • Nommer les URLs sur les messages private-post-* plutôt que post-private-*.
  • Renommer les URLs {id}/{slug}/ plutôt que l'identifiant tout court.
  • Rajouter un "s" à "message" dans les URLs.
  • Renommer le module pm.
  • Corriger l'erreur lorsque nous mettons qu'un utilisateur dans la conversation sans la virgule.

Pour ce dernier point, j'étais quasi sûr de l'avoir géré mais sous un malentendu, c'est peut-être resté. Merci pour vos QA en tout cas, je fais ça dès lundi !

@GerardPaligot GerardPaligot force-pushed the GerardPaligot:refacto_mp branch from 39846e0 to f76bc32 Mar 8, 2015

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2015

Bien, j'ai appliqué toutes les modifications sauf 2 :

  • Pas moyen de reproduire ton bug @ChantyTaguan. Est-ce que tu pourrais être plus précise ?
  • Faute de ne pas savoir comment renommer proprement un module Django, je ne l'ai pas renommé.

Pour le reste, le tout devrait être corrigé.

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2015

Si je rebase ce soir, est-ce mergable ici ? Je ne peux pas commencer l'API tant que cette PR n'est pas mergée.

@ChantyTaguan

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

Toujours le même bug chez moi, si j'ai "user, admin" (espace après la virgule) dans les participants quand je crée un mp avec staff, j'ai une erreur.

Si je mets "user,admin" (sans espace), ça passe. J'aurais bien proposé une PR mais IntelliJ me gonfle, faut absolument que je passe à PyCharm...

ChantyTaguan added a commit to ChantyTaguan/zds-site that referenced this pull request Mar 11, 2015
GerardPaligot added a commit to GerardPaligot/zds-site that referenced this pull request Mar 11, 2015
Merge pull request #13 from ChantyTaguan/refacto_mp
PR zestedesavoir#2398 : fix bug when new MP with several recipients
@landscape-bot

This comment has been minimized.

Copy link

commented Mar 11, 2015

Code Health
Repository health increased by 0.04% when pulling 1dfb88e on GerardPaligot:refacto_mp into 0b6276f on zestedesavoir:dev.

GerardPaligot and others added 8 commits Feb 18, 2015
refactor(mp): Displays list of MPs and list of messages of a MP given.
1. Use generic classes for views to list of MPs and to list of messages.
2. Changes URLs of these features.
3. Improves ZdSPagingListView to build list with previous item in a list pagined.
refactor(mp): Creates a new MP with participants.
1. Used generic based view to create a new MP.
2. Uses validators to make checks on fields of the form.
refactor(mp): Adds a new participant in a MP.
1. Changes view for a generic view.
2. Merges edit and add_participant methods.
refactor(mp): Leaves a MP.
Changes view for a generic view.
refactor(mp): Leaves a list of MPs.
Changes view for a generic view.
refactor(mp): Answer at a MP.
Changes view with generic view CreateView.
refactor(mp): Edit the last post of a MP.
Changes view with generic view UpdateView.

@GerardPaligot GerardPaligot force-pushed the GerardPaligot:refacto_mp branch from 1dfb88e to 46eecbe Mar 11, 2015

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2015

Rebase fait. Peut être mergé quand travis sera ok.

@landscape-bot

This comment has been minimized.

Copy link

commented Mar 11, 2015

Code Health
Repository health increased by 0.04% when pulling 46eecbe on GerardPaligot:refacto_mp into 732bb29 on zestedesavoir:dev.

@GerardPaligot

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

Travis est passé. La PR peut être mergée.

@SpaceFox

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

Alors mergeons.

SpaceFox added a commit that referenced this pull request Mar 12, 2015
Merge pull request #2398 from GerardPaligot/refacto_mp
Refactorisation des vues du module des MPs

@SpaceFox SpaceFox merged commit 7527fe6 into zestedesavoir:dev Mar 12, 2015

1 check passed

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

@SpaceFox SpaceFox added this to the Version 1.7 milestone Mar 12, 2015

firm1 added a commit to firm1/zds-site that referenced this pull request Mar 17, 2015
firm1 added a commit to firm1/zds-site that referenced this pull request Mar 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.