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

Affichage des comptes sur un même réseau IPV6 dans la page de multi-comptes (#4103) #6120

Closed
wants to merge 0 commits into from
Closed

Conversation

PetitMote
Copy link
Contributor

@PetitMote PetitMote commented May 18, 2021

Modification de la fonction member_from_ip afin qu'elle retourne tous les membres sur un même bloc IPV6. Adaption du template associé. Ajout de tests fonctionnels et d'autorisation.

Numéro du ticket concerné :
#4103

Contrôle qualité

  • Se connecter en tant que staff
  • Aller sur un profil utilisateur et cliquer sur son adresse IP ou se rendre directement sur membres/profil/multi//
  • Vérifier que les utilisateurs soient bien affichés avec une IPV4
  • Donner dans la base à 2 utilisateurs une adresse IPV6 sur le même réseau (par exemple en modifiant le dernier caractère)
  • Vérifier qu'avec une IPV6, on a bien 2 blocs avec la liste utilisant l'adresse ip exacte et la liste utilisant le même réseau IPV6, contenant les deux utilisateurs modifiés précédemment
  • Éventuellement (fonctionnait avant) : se connecter en utilisateur et vérifier que l'accès est verrouillé

Exemple, 2 comptes sur un même réseau IPV6 chez moi :
image

@AmauryCarrade
Copy link
Member

En terme de QA, il ne faut pas aussi donner à deux utilisateurs deux IPv6 différentes mais du même réseau (même préfixe) dans la base, et vérifier qu'iels sont listés ensemble sur la page de multi ?

@PetitMote
Copy link
Contributor Author

PetitMote commented May 18, 2021

En terme de QA, il ne faut pas aussi donner à deux utilisateurs deux IPv6 différentes mais du même réseau (même préfixe) dans la base, et vérifier qu'iels sont listés ensemble sur la page de multi ?

Effectivement, en plus je l'ai fait pendant mes tests, et j'ai oublié que c'était facile et faisable en instance de dev :| J'ai (encore) édité la QA.

@AmauryCarrade
Copy link
Member

Pas de souci ! Merci

@coveralls
Copy link

coveralls commented May 18, 2021

Coverage Status

Coverage increased (+0.006%) to 86.578% when pulling 7c88a14 on PetitMote:IPV6samenetwork into 421eea6 on zestedesavoir:dev.

AmauryCarrade
AmauryCarrade previously approved these changes May 20, 2021
Copy link
Member

@AmauryCarrade AmauryCarrade left a comment

Choose a reason for hiding this comment

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

✔️ QA OK ! Merci pour cette amélioration. Quelques remarques d'amélioration, à toi de voir ce que tu en penses.

templates/member/admin/memberip.html Outdated Show resolved Hide resolved
templates/member/admin/memberip.html Outdated Show resolved Hide resolved
@PetitMote
Copy link
Contributor Author

Si jamais tu as aussi des commentaires sur les tests unitaires, ça me plairait, c'était la première fois que j'en écrivais. Ou même des trucs plus bêtes comme les commentaires de code, s'ils sont bien écrits et compréhensibles.

@AmauryCarrade
Copy link
Member

Je vais regarder ça de près alors :)

@AmauryCarrade AmauryCarrade self-requested a review May 20, 2021 15:13
Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

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

Tu voulais des commentaires sur tes tests. Rien de bien méchant, mais j'espère que tu les trouveras utiles pour réfléchir à ta pratique. :-)

zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
@PetitMote
Copy link
Contributor Author

Merci pour les conseils @Arnaud-D :)

Copy link
Member

@AmauryCarrade AmauryCarrade left a comment

Choose a reason for hiding this comment

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

Quelques petits commentaires sur les tests, vu que tu en voulais plein !

zds/member/tests/tests_views.py Outdated Show resolved Hide resolved
zds/member/tests/tests_views.py Outdated Show resolved Hide resolved

def test_access_rights_to_ip_page_as_non_user(self) -> None:
response = self.client.get(reverse(member_from_ip, args=["0.0.0.0"]))
self.assertEqual(response.status_code, 302)
Copy link
Member

Choose a reason for hiding this comment

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

Si on voulait vraiment être tranquille, on pourrait vouloir vérifier qu'on redirige bien vers la page de connexion. Mais c'est peut-être un peu overkill ^^ .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je savais pas comment faire alors j'ai fait comme ça x) Sinon, il aurait fallu que je fouille pour faire un reverse qui fonctionne, et j'ai toujours du mal à comprendre la fonction reverse...

Comment on lines 1818 to 1821
def test_template_used_by_ip_page(self) -> None:
self.client.force_login(self.staff)
response = self.client.get(reverse(member_from_ip, args=["0.0.0.0"]))
self.assertTemplateUsed(response, "member/admin/memberip.html")
Copy link
Member

Choose a reason for hiding this comment

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

C'est amusant, c'est pas un test qu'on fait souvent ça, ayant tendance à “faire confiance” à Django pour les cas simples. Mais pourquoi pas !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En fait, j'ai cherché comment faire un test pour un render Django, et je suis tombé sur un StackOverflox où la réponse proposait ça en partie (et les 2 autres types de test que j'ai mis). J'ai trouvé ça pas si bête, et je me suis dit que ça coûtait rien d'être sûr.

@PetitMote
Copy link
Contributor Author

D'ailleurs j'ai une question, je ne sais pas si vous avez lancé les tests en local, mais comme je teste une 403, j'ai des erreurs Django dans la ligne de commande puisqu'il refuse l'accès à la page. C'est normal, mais est-ce que c'est voulu, est-ce qu'il y a moyen de ne pas les afficher pour éviter de polluer l'affichage des tests ?

@PetitMote PetitMote closed this May 23, 2021
@Arnaud-D
Copy link
Contributor

C'est normal d'avoir de voir des erreurs s'afficher dans les logs quand on teste des choses qui génèrent des erreurs. Ce qu'on veut éviter, ce sont les erreurs correspondant aux échecs de test, mais ce ne sont pas les mêmes. ;-) Si les tests passent, peu importe ce qui affiché dans la console lors du déroulement des tests. Je ne m'en sers que quand les tests ne passent pas, pour investiguer le problème.

@PetitMote PetitMote deleted the IPV6samenetwork branch May 26, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Staff : page d'utilisateurs avec la même IP → avec le même réseau en IPv6
4 participants