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

Ajout d'un bouton "nouveau sujet" dans l'accueil et d'un champ de catagorie dans le TopicForm #3863

Closed
wants to merge 30 commits into from

Conversation

taoufik07
Copy link

@taoufik07 taoufik07 commented Oct 17, 2016

Suite à une suggestion que j'ai posté dans zds j'ai modifier le comportement de la vue TopicNew et TopicEdit ainsi que les templates pour intégrer le bouton 'nouveau sujet' et le champ "catégorie"

Quand on clique sur "nouveau sujet" on sera ramener vers zds.com/forums/sujet/nouveau et le champ catégorie va être vide
Si on clique sur "nouveau sujet" depuis un forum on sera ramener vers zds.com/forums/sujet/nouveau/?forum={{forum.pk}} et forum sera sélectionner dans le champ catégorie avec la possibilité de modifie le choix

P.S C'est mon premier pull request j’espère que c'est comme ça que ca se deroule

@pierre-24
Copy link
Member

J'ai du mal à comprendre exactement ce qui foire, mais pour commencer, j'ai l'impression que t'as rajouté à peu près la vue qu'il fallait, mais t'as pas édité le formulaire qui allait avec. D'où le fait que "section" existe pas, probablement (si je lis bien ce que Travis raconte).

@taoufik07
Copy link
Author

En faite je l'ai nomme caregorie dans le formulaire

@taoufik07
Copy link
Author

taoufik07@3b31927 scroll un peu et tu vas voir le nouveau champ dans TopicFrm

@pierre-24
Copy link
Member

pierre-24 commented Oct 18, 2016

J'avais pas vu, my bad.

Alors à mon avis, ce qui se passe, c'est que t'as pas encore modifié les tests pour envoyer ladite "section", et donc les tests plantent (semblerai que tu doivent chipoter pour faire comprendre à ton formulaire que quand on passe ?forum=pk, il faut que ce forum soit sélectionné, sinon rien n'est sélectionné). Typiquement, il faudrait que tu rajoute quelques lignes dans la fonction clean() du formulaire pour être sur que le champ "section" ne soit pas vide et/ou alors qu'il soit remplis avec la valeur de request.GET['section']. À toi de voir ce que tu imagines.

D'ailleurs, n'oublie pas d'écrire un petit test ;)

@taoufik07
Copy link
Author

Ok, des que je rentre, je verrai ce qui plante

@@ -1,6 +1,7 @@
#############
## Zeste de Savoir
#############
settings.py
Copy link
Member

Choose a reason for hiding this comment

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

Ça, tu ne peux pas faire ;)

Copy link
Author

@taoufik07 taoufik07 Oct 18, 2016

Choose a reason for hiding this comment

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

oui, parce que je travaille en locale alors j'ai j'ai désactivé quelques middlewares pour que ca fonctionne chez moi

Copy link
Member

Choose a reason for hiding this comment

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

Tu n'as pas enlevé ça :-)

zds/settings.py Outdated
'zds.middlewares.profile.ProfileMiddleware',
'zds.middlewares.ForceHttpsMembersMiddleware.ForceHttpsMembersMiddleware',
#'zds.middlewares.ForceHttpsMembersMiddleware.ForceHttpsMembersMiddleware',
Copy link
Member

Choose a reason for hiding this comment

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

J'ai pas compris pourquoi tu as fais ça :)

Copy link
Author

@taoufik07 taoufik07 Oct 18, 2016

Choose a reason for hiding this comment

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

je travaille en locale alors j'ai désactivé quelques middlewares pour que ca fonctionne chez moi

CommonLayoutEditor(),
)

def clean(self):
cleaned_data = super(TopicForm, self).clean()

title = cleaned_data.get('title')
section = cleaned_data.get('section')
Copy link
Member

@pierre-24 pierre-24 Oct 19, 2016

Choose a reason for hiding this comment

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

EDIT: j'ai rien dit.

@pierre-24
Copy link
Member

Ok. Bon.

À priori ton code fonctionne, je viens de tester vite fait. Sauf que ce qui me dérange et que j'arrivais pas à comprendre, c'est que les tests ne passent pas (et tant que t'as pas un petit "v" vers à coté de ton commit, la PR ne peut pas être mergée). Ce qui se passe, c'est qu'il faut que tu rajoute "section": forum.pk un peu partout dans les tests, par exemple ici. Ça permettra de régler une partie des erreurs présentes dans ce log.

Sinon, n'oublie pas d'enlever les commentaires que tu as rajouté. D'ailleurs, à priori, tu n'as pas créé de nouvelle branche pour faire tes modifications. git checkout -b TA_BRANCHE upstream/dev est la commande qui te permet de créer une branche spécifique à ton problème. Je te renvois vers ce topic pour plus d'informations :)

@taoufik07
Copy link
Author

Les tests ne marchent pas parce que mes mofidications ont change le comportement du site.

@pierre-24
Copy link
Member

Les tests ne marchent pas parce que mes modifications ont change le comportement du site.

Ce qui est normal quand on implémente une nouvelle fonctionnalité. Mais du coup, il faut changer les tests en conséquences (et c'est ce que tu as fait, c'est très bien).

Alors, tu y es presque, à priori. D'une part, y'a un test qui passe pas encore (FAIL: test_creation_topic (zds.notification.tests.NotificationForumTest)), j'ai pas regardé pourquoi, d'autre part, il y a plusieurs erreurs de "formatage". En fait, on utilise un outil (flake8), qui vérifie qu'on respecte les "bonnes" règles de formatage de python. Dans ton cas,

./zds/settings.py:114:5: E265 block comment should start with '# '
./zds/settings.py:116:5: E265 block comment should start with '# '
./zds/forum/forms.py:85:1: W293 blank line contains whitespace
./zds/forum/views.py:188:1: E302 expected 2 blank lines, found 1
./zds/forum/views.py:202:1: W293 blank line contains whitespace
./zds/forum/views.py:213:1: W293 blank line contains whitespace
./zds/forum/views.py:218:121: E501 line too long (140 > 120 characters)
./zds/forum/views.py:220:1: W293 blank line contains whitespace
./zds/forum/views.py:232:30: E203 whitespace before ':'
./zds/forum/views.py:258:1: E302 expected 2 blank lines, found 1
./zds/forum/views.py:291:22: E203 whitespace before ':'
./zds/forum/views.py:309:34: E203 whitespace before ':'
./zds/forum/tests/tests.py:179:14: E121 continuation line under-indented for hanging indent
./zds/forum/tests/tests.py:181:14: E123 closing bracket does not match indentation of opening bracket's line

... Des erreurs d'indentations, des espaces en trop ou pas assez. Tu peux tester toi même que ton code est conforme avec flake8 zds/ dans ton terminal préféré. On a pour principe de ne pas accepter de code qui ne passe pas ce test là, juste pour que tout le monde écrive plus ou moins pareil ;)

Et dernière chose, n'oublie pas d'enlever tes modifs à settings.py et .gitignore.

Courage :)

@pierre-24
Copy link
Member

Ah, non, j'ai encore oublié un truc. Il faut que tu rajoutes un test à toi pour tester la fonctionnalité que tu implémente: typiquement, le fait que tu puisses poster dans un forum comme ça, que l'oubli du paramètre section fait planter le machin, et que ce paramètre n'est pas redondant avec ?forum=xxx (auquel cas, si je suis bien, il fait rien planter).

forum_pk = int(self.request.GET.get('forum'))
forum_pk = self.request.GET.get('forum')
if forum_pk is None:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Si forum_pk est à None, il faut jetter une 404.

Copy link
Author

Choose a reason for hiding this comment

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

si forum_pk est à None je permet le choix d'un forum depuis le champ que j'ai ajouté

Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi ne pas avoir réutilisé le paramètre forum dans ce cas ?

Copy link
Author

Choose a reason for hiding this comment

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

??

Copy link
Member

Choose a reason for hiding this comment

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

J'ai besoin d'une information en prérequis à cette information. Si je comprends bien, nous avons un paramètre GET forum et un autre paramètre POST section mais pour la même information. C'est bien ça ?

Copy link
Author

Choose a reason for hiding this comment

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

oui

Copy link
Member

Choose a reason for hiding this comment

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

Est-ce qu'il n'y aurait pas moyen de nommer section du POST en forum et de gérer le paramètre par self.request.REQUEST.get('forum') pour quelque chose de transparent ?

Copy link
Author

Choose a reason for hiding this comment

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

oui, je peux nommer section en forum

Copy link
Member

Choose a reason for hiding this comment

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

Cool parce que j'ai mis du temps avant de comprendre que c'était la même chose. Pour la maintenance, ça sera bien mieux.

Copy link
Author

Choose a reason for hiding this comment

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

C'est fait :)

return get_object_or_404(Forum, pk=forum_pk)

except Forum.DoesNotExist:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Même chose, si le forum n'existe pas, c'est une 404.

Copy link
Author

Choose a reason for hiding this comment

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

Oui, oui
par exemple on forum_pk = X et x est un entier valide mais le forum n'existe pas au lieu de 404 je laisse le champ catégorie(section) vide

return render(request, self.template_name, {'forum': self.object, 'form': self.form_class()})

if self.object is None:
self.object = self.get_object()
Copy link
Member

Choose a reason for hiding this comment

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

Si self.object est initialisé par get_object, comment sa valeur peut changer de None ?

Copy link
Author

Choose a reason for hiding this comment

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

il se peut que le forum soit dans l'url si l'utilisateur a cliqué sur "nouveau sujet" depuis le forum => get_object != None ou bien aucun forum n'est spécifié s'il vient d'autre part => get_object==None et l'utilisateur doit le sélectionner manuellement

"text": request.POST["text"]
}
form = self.form_class(initial=initial)
elif form.is_valid():
self.object = get_object_or_404(Forum, pk=form.data['section'])
Copy link
Member

Choose a reason for hiding this comment

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

self.object n'est pas censé être le forum ?

@@ -32,6 +32,16 @@ class TopicForm(forms.Form):
required=False,
)

section = forms.ModelChoiceField(
Copy link
Member

Choose a reason for hiding this comment

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

Je ne comprends pas ce qu'est une section. On va devoir spécifier tout le temps le forum dans lequel nous sommes ?

Copy link
Author

Choose a reason for hiding this comment

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

Je ne comprends pas ce qu'est une section. On va devoir spécifier tout le temps le forum dans lequel nous sommes ?

Si on clique sur "nouveau sujet" depuis un forum le forum sera sélectionner automatiquement avec la possibilité de changer le forum sans devoir copier le text ouvrir une nouvelle onglet et cliquer sur "Tous les sujet" puis sur le bon forum et coller le text
Si on clique sur "nouveau sujet" depuis la page d'accueil l'utilisateur doit sélectionner le forum

Copy link
Member

Choose a reason for hiding this comment

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

Donc le bon forum est sélectionné automatiquement quand on vient des forums, c'est cool ça ! (:

Copy link
Member

Choose a reason for hiding this comment

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

Rien à voir, mais hier quand j'ai fait le test avec ton code, ce fameux bouton "nouveau sujet" n'était pas encore présent sur la page des forums ;) (tu l'as peut être rajouté depuis).

Copy link
Author

Choose a reason for hiding this comment

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

nn il existe depuis toujours

Copy link
Member

Choose a reason for hiding this comment

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

Me suis mal exprimé. Effectivement, il est en page d’accueil, mais pourquoi pas aussi sur la page de la liste des forums, du coup ? (celle là, donc)

Copy link
Author

Choose a reason for hiding this comment

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

Give that man a jar of cookies !!
je vais le faire

@coveralls
Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage decreased (-0.02%) to 87.585% when pulling ff6e8a1 on taoufik07:dev into bd34531 on zestedesavoir:dev.

Copy link
Contributor

@vhf vhf left a comment

Choose a reason for hiding this comment

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

Merci pour cette PR ! Quelques remarques cosmétiques pour ma part.

@@ -32,6 +32,16 @@ class TopicForm(forms.Form):
required=False,
)

section = forms.ModelChoiceField(
label=_("Categorie"),
Copy link
Contributor

Choose a reason for hiding this comment

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

-        label=_("Categorie"),
+        label=_(u'Catégorie'),

@@ -101,7 +101,8 @@ def test_create_topic(self):
'title': u'Un autre sujet',
'subtitle': u'Encore ces lombards en plein ete',
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
'tags': ''
'tags': '',
'section': self.forum12.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-                'section': self.forum12.pk
+                'section': self.forum12.pk,

@@ -144,7 +145,8 @@ def test_create_topic_failing_param(self):
reverse('topic-new') + '?forum=' + 'abc',
{'title': u'Un autre sujet',
'subtitle': u'Encore ces lombards en plein ete',
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter '
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
'section': 'abc'
Copy link
Contributor

Choose a reason for hiding this comment

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

-             'section': 'abc'
+             'section': 'abc',

@@ -154,7 +156,8 @@ def test_create_topic_failing_param(self):
reverse('topic-new') + '?forum=',
{'title': u'Un autre sujet',
'subtitle': u'Encore ces lombards en plein ete',
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter '
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
'section': 'abc'
Copy link
Contributor

Choose a reason for hiding this comment

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

-             'section': 'abc'
+             'section': 'abc',

@@ -303,7 +315,8 @@ def test_edit_main_post(self):
'title': expected_title,
'subtitle': expected_subtitle,
'text': expected_text,
'tags': ''
'tags': '',
'section': self.forum11.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-                'section': self.forum11.pk
+                'section': self.forum11.pk,

@@ -353,7 +355,8 @@ def test_success_create_topic_with_post(self):
'title': 'Title of the topic',
'subtitle': 'Subtitle of the topic',
'text': 'A new post!',
'tags': ''
'tags': '',
'section': forum.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-            'section': forum.pk
+            'section': forum.pk,

@@ -717,6 +720,7 @@ def test_success_edit_topic_in_preview(self):
'title': 'New title',
'subtitle': 'New subtitle',
'text': 'A new post!',
'section': forum.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-            'section': forum.pk
+            'section': forum.pk,

@@ -735,6 +739,7 @@ def test_success_edit_topic_in_preview_in_ajax(self):
'title': 'New title',
'subtitle': 'New subtitle',
'text': 'A new post!',
'section': forum.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-            'section': forum.pk
+            'section': forum.pk,

@@ -755,6 +760,7 @@ def test_success_edit_topic_information(self):
'title': 'New title',
'subtitle': 'New subtitle',
'text': 'A new post!',
'section': forum.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-            'section': forum.pk
+            'section': forum.pk,

@@ -46,7 +46,8 @@ def test_creation_topic(self):
'title': u'Super sujet',
'subtitle': u'Pour tester les notifs',
'text': u'En tout cas l\'un abonnement',
'tags': ''
'tags': '',
'section': self.forum12.pk
Copy link
Contributor

Choose a reason for hiding this comment

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

-                'section': self.forum12.pk
+                'section': self.forum12.pk,

Copy link
Author

Choose a reason for hiding this comment

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

Merci, je vais les appliquées :)

Copy link
Author

Choose a reason for hiding this comment

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

done ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

T'en as oublié quelques-uns, regarde ici: #3863 (review)

Les commentaires qui s'affichent encore sont ceux que tu n'as pas pris en compte. :)

Les commentaires pris en compte portent un "Show outdated" à droite sur la ligne.

Copy link
Author

Choose a reason for hiding this comment

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

maintenant je vois et maintenant ?

Copy link
Contributor

Choose a reason for hiding this comment

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

screen shot 2016-10-19 at 21 41 29

T'es super, merci ! :)

Copy link
Author

Choose a reason for hiding this comment

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

merci à vous aussi :)

@coveralls
Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage decreased (-0.02%) to 87.585% when pulling a98d877 on taoufik07:dev into bd34531 on zestedesavoir:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-35.0%) to 52.61% when pulling ad1b0f3 on taoufik07:dev into bd34531 on zestedesavoir:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 87.585% when pulling ad1b0f3 on taoufik07:dev into bd34531 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage decreased (-0.02%) to 87.585% when pulling d40ad4a on taoufik07:dev into bd34531 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage decreased (-0.01%) to 87.598% when pulling 4868bdc on taoufik07:dev into bd34531 on zestedesavoir:dev.

@taoufik07
Copy link
Author

Dans ces derniers commits (d40ad4a, 13a71fc, 07be3da, 61dec8e, e08db85, 4868bdc), j'ai accompli :

  1. Renommer le champ "section"(catégorie) en "forum"(catégorie) ( Merci @GerardPaligot ).
  2. Ecriture des tests.
  3. Ajout du bouton "nouveau sujet" dans la page zds.com/forums/{{forum}}/ (Merci @pierre-24 ).
  4. Exclusion des forums sans permission de lecture dans le champ "forum"(catégorie) .

A faire :

  1. Modification du style du champ "forum"(catégorie).

Si vous avez des remarques ou autres modifications n’hésiter pas à me le dire 😉.

@@ -144,7 +145,8 @@ def test_create_topic_failing_param(self):
reverse('topic-new') + '?forum=' + 'abc',
{'title': u'Un autre sujet',
'subtitle': u'Encore ces lombards en plein ete',
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter '
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
'forum': 'abc',
Copy link
Member

Choose a reason for hiding this comment

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

si tu met `'?forum=abc'``en paramètre GET, est ce que tu as besoin de préciser le POST ? (vraie question)

Copy link
Author

Choose a reason for hiding this comment

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

Non,

Copy link
Author

Choose a reason for hiding this comment

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

Retourne un 404

@@ -154,7 +156,8 @@ def test_create_topic_failing_param(self):
reverse('topic-new') + '?forum=',
{'title': u'Un autre sujet',
'subtitle': u'Encore ces lombards en plein ete',
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter '
'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
'forum': 'abc',
Copy link
Member

Choose a reason for hiding this comment

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

Là, il me semble que tu court-circuite le test, qui est "si on précise rien en GET, ça doit planter". C'est même pas forcément logique que tu te prenne une 404 ici, si ?

Copy link
Author

Choose a reason for hiding this comment

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

C'est ce qui se passe

Copy link
Author

Choose a reason for hiding this comment

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

Si on précise rein en GET, ça doit laissé le choix à l'utilisateur, je vais le corriger

Copy link
Member

Choose a reason for hiding this comment

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

Oui. Et même remarque au dessus, du coup !

Copy link
Author

Choose a reason for hiding this comment

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

Yep

'text': u'C\'est tout simplement l\'histoire de la ville de Paris que je voudrais vous conter ',
},
follow=False)
self.assertEqual(result.status_code, 200)
Copy link
Member

Choose a reason for hiding this comment

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

... Et là, c'est encore moins logique pour moi: pourquoi tu te prend une 200 si tu ne précise aucune des 2? Au mieux ça devrait être une 404, au pire une 302, non ?

Copy link
Author

Choose a reason for hiding this comment

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

Si je ne précise aucune des deux je laisse le choix à l'utilisateur de sélectionner un forum depuis le champ.
( Je pense que je vais faire de même si forum=LEVIDE )

Copy link
Member

Choose a reason for hiding this comment

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

Ah, oui, juste. Là, je t'autorise à mettre un commentaire, parce que c'est pas évident :)

Copy link
Author

Choose a reason for hiding this comment

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

Oui capitaine !

@@ -233,6 +233,14 @@ def test_failure_create_topic_with_a_post_with_client_unauthenticated(self):

self.assertEqual(302, response.status_code)

# By me,
Copy link
Member

Choose a reason for hiding this comment

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

Pas de commentaires inutiles ;)

Copy link
Author

Choose a reason for hiding this comment

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

Désolé

profile = ProfileFactory()

self.assertTrue(self.client.login(username=profile.user.username, password='hostel77'))
response = self.client.get(reverse('topic-new') + '?forum={}'.format(178903))
Copy link
Member

Choose a reason for hiding this comment

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

tu devrait tester en follow=False et probablement obtenir une 302.

Copy link
Author

@taoufik07 taoufik07 Oct 20, 2016

Choose a reason for hiding this comment

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

en fait si le pk est un int qui n'existe pas dans la bd je laisse le champ vide sans faire aucune redirection, si vous pensez qu'une redirection c'est mieux je la ferai

Copy link
Member

Choose a reason for hiding this comment

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

Je suis partagé ... @vhf ? Est ce qu'on lève une erreur ou pas ?

Copy link
Member

Choose a reason for hiding this comment

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

et @GerardPaligot ? (qui connait bien le module)

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

self.assertEqual(302, response.status_code)
Copy link
Member

Choose a reason for hiding this comment

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

Ce test devrait ce terminer en comptant le nombre de topic avant et après ton ajout, pour assurer le ledit topic a bien été créé (self.assertEqual(Topic.objects.all().count(), xxx))

Copy link
Author

Choose a reason for hiding this comment

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

C'est fait

@@ -1,6 +1,7 @@
#############
## Zeste de Savoir
#############
settings.py
Copy link
Member

Choose a reason for hiding this comment

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

Tu n'as pas enlevé ça :-)

@coveralls
Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage decreased (-0.01%) to 87.598% when pulling 9665a33 on taoufik07:dev into bd34531 on zestedesavoir:dev.

super(TopicForm, self).__init__(*args, **kwargs)

self.fields['forum'].queryset = Forum.objects.exclude(group__in=Group.objects.filter(user=self.user).all())
Copy link
Member

Choose a reason for hiding this comment

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

Cette vérification ne semble pas être faite à la soumission du formulaire. Il serait donc possible de créer des sujets dans des forums privés si on ne soumet pas par le site.

Copy link
Author

Choose a reason for hiding this comment

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

Je pense pas que c'est une verfication, elle permet juste de filtrer la liste des forums en se basant sur la permission de lecture, la vraie verification se fait dans la vue

return render(request, self.template_name, {'forum': self.object, 'form': self.form_class()})
if self.object is None:
self.object = self.get_object()
if self.object is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Dans la mesure où nous appelons self.get_object() dans la méthode dispatch, je ne comprends toujours pas ces 3 lignes. Comment pouvons-nous récupérer un résultat la deuxième fois pour une même requête ?

Copy link
Author

Choose a reason for hiding this comment

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

mm, vous avez raison, je vais supprimer les lignes 221 et 222

"text": request.POST["text"]
}
form = self.form_class(initial=initial)
elif form.is_valid():
self.object = get_object_or_404(Forum, pk=form.data['forum'])
Copy link
Member

Choose a reason for hiding this comment

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

Simple question : Est-ce que form.data['forum'] peut-il aussi être accédé par request.POST['forum'] dans ce cas ci ?

Copy link
Author

Choose a reason for hiding this comment

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

j'ai pas bien compris

@vhf vhf added QA svp C-Back Concerne le back-end Django C-Front Concerne l'interface du site labels Nov 20, 2016
@gllmc
Copy link
Member

gllmc commented Sep 2, 2017

@taoufik07 : es-tu toujours intéressé par cette pull request ?

@sandhose
Copy link
Contributor

Étant donné l'absence de réponse, je ferme la PR. Si tu veux la reprendre, n'hésites pas à la ré-ouvrir.

@gllmc gllmc closed this Sep 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django C-Front Concerne l'interface du site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants