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

Système de casquettes #4387

Merged
merged 24 commits into from
Aug 6, 2017
Merged

Système de casquettes #4387

merged 24 commits into from
Aug 6, 2017

Conversation

gllmc
Copy link
Member

@gllmc gllmc commented Jun 27, 2017

Q R
Type de modification nouvelle fonctionnalité
Ticket(s) (issue(s)) concerné(s) forum

Cette PR implémente le système de casquettes et supprime le badge staff.

TODO

  • Régler les problèmes de design
  • Faire le ménage dans les migrations
  • Mettre des casquettes sur les MP automatiques
  • La documentation
  • Les tests

QA

  • Reconstruire le front et lancer les migrations.
  • Tester l'ajout et la suppression de casquettes via le profil (réservé aux membres avec la permission utils.change_hat).
  • Vérifier que les casquettes sont listées correctement sur le profil (et que rien n'apparaît s'il n'y a pas de casquettes).
  • Vérifier que l'envoi d'un message fonctionne comme avant lorsque l'on n'a pas de casquettes.
  • Vérifier qu'on a la possibilité de choisir une casquette lors de l'envoi d'un message (forum, contenus, MP) quand on en a au moins une et que le message est correctement envoyé avec.
  • Vérifier que l'affichage d'une casquette sur un message est correct.
  • Tester l'ajout en masse d'une casquette à un groupe avec la commande python manage.py add_hat_to_group (ex : python manage.py add_hat_to_group 'dev' 'Équipe technique' ajoute la casquette "Équipe technique" au groupe "dev").
  • Code review.

@gllmc gllmc added C-Back Concerne le back-end Django C-Front Concerne l'interface du site S-Évolution labels Jun 27, 2017
@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-0.3%) to 88.827% when pulling eab47b9 on gcodeur:hats into c708844 on zestedesavoir:dev.

Copy link
Member

@artragis artragis left a comment

Choose a reason for hiding this comment

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

C'est une belle PR :)

@@ -701,6 +701,58 @@ def remove_banned_email_provider(request, provider_pk):
return redirect('banned-email-providers')


@require_POST
@login_required
@permission_required('utils.change_hat', raise_exception=True)
Copy link
Member

Choose a reason for hiding this comment

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

je serais tenté d'ajoujter un @transaction.atomic


user = get_object_or_404(User, pk=user_pk)

if not request.POST.get('hat'):
Copy link
Member

Choose a reason for hiding this comment

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

pour éviter les erreurs 500 :

    hat = request.POST.get('hat', None)
    if not hat:
        # error
    elif len(hat) > 40:  #elif très important
        #error
    else:

Copy link
Member Author

Choose a reason for hiding this comment

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

En effet, j'ai oublié le elif, bien vu !


user.profile.hats.remove(hat)

if hat.profile_set.count() == 0:
Copy link
Member

Choose a reason for hiding this comment

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

pas très d'accord avec ça.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pas d'accord avec la suppression ? Ça me paraît utile pour ne pas garder des objets inutiles.

Au cas où tu n'aurais pas compris ça : les messages utilisant la casquette la stockent sous format texte, donc ils la garderont. ;)

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 supprimer un truc qui est important pour le site.


@require_POST
@login_required
@permission_required('utils.change_hat', raise_exception=True)
Copy link
Member

Choose a reason for hiding this comment

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

je verrais bien un @transaction.atomic

@@ -517,6 +517,9 @@ def __init__(self, content, reaction, *args, **kwargs):
Field('last_note') if not last_note else Hidden('last_note', last_note)
)

if reaction is None:
Copy link
Member

Choose a reason for hiding this comment

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

pourquoi reaction?

Copy link
Member Author

Choose a reason for hiding this comment

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

C'est pour vérifier s'il s'agit d'une édition ou d'un nouveau commentaire. Dans le premier cas, on n'ajoute pas le choix de la casquette.

Copy link
Member

Choose a reason for hiding this comment

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

ah bien vu. tu peux ajouter un commentaire pour dire ça stp?



def get_hat_from_request(request):
if not request.POST.get('hat'):
Copy link
Member

Choose a reason for hiding this comment

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

if 'hat' not in request.POST

Copy link
Member Author

Choose a reason for hiding this comment

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

Pas forcément, car hat se trouvera dans la requête avec pour valeur '' si la personne possède une casquette mais n'a rien sélectionné. ;)

Copy link
Member

Choose a reason for hiding this comment

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

donc if not request.POST.get('hat', '')

hat = Hat.objects.get(pk=int(request.POST.get('hat')))
assert hat in request.user.profile.hats.all()
return hat.name
except (ValueError, Hat.DoesNotExist, AssertionError):
Copy link
Member

Choose a reason for hiding this comment

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

un petit logger.warning permettrait d'avoir des infos bien utiles :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, j'avoue que j'ai jamais utilisé ça, mais les exemples d'utilisation ne manquent pas dans le code.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-0.3%) to 88.824% when pulling 9cdcbd9 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.3%) to 88.836% when pulling da97a56 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.3%) to 88.836% when pulling 3ee0e1c on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.967% when pulling f0bd882 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.967% when pulling 5db2864 on gcodeur:hats into c708844 on zestedesavoir:dev.

that a moderation message was posted by a staff member.
"""

class Meta:
Copy link
Contributor

Choose a reason for hiding this comment

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

Juste pour *****er les mouches, on préfère mettre class Meta: après les champs (oui je sais, c'est pas comme ça ailleurs il faut changer).

cf https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#model-style

Copy link
Member Author

Choose a reason for hiding this comment

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

Du coup, je mets ça après pour ce modèle-ci ?

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.967% when pulling d7c914b on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.967% when pulling 1a480f2 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.967% when pulling ea521c4 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-0.03%) to 89.074% when pulling 21ee1aa on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 89.118% when pulling be581f2 on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 89.118% when pulling 5be53d6 on gcodeur:hats into c708844 on zestedesavoir:dev.

@gllmc gllmc changed the title [WIP] Système de casquettes Système de casquettes Jun 29, 2017
@gllmc gllmc added the QA svp label Jun 29, 2017
@gllmc
Copy link
Member Author

gllmc commented Jun 29, 2017

Le code est terminé ! Il y a normalement ce qu'il faut : tests et documentation.

J'ai mis à jour les instructions de QA dans le premier message.

Pour ajouter une casquette à tous les membres d'un groupe, une commande ``django-admin`` a été créée. Par exemple, la commande à taper (à la racine du projet) pour ajouter la casquette « Équipe technique » à tous les membres du groupe « dev » est ``python manage.py add_hat_to_group 'dev' 'Équipe technique'``.

Les casquettes sont ajoutées aux MP automatiques en fonction des paramètres ``ZDS_APP['member']['validation_hat']`` et ``ZDS_APP['member']['moderation_hat']`` renseignés dans le fichier ``settings.py``.

Copy link
Contributor

Choose a reason for hiding this comment

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

La doc 😍

@@ -11,7 +11,7 @@ class ProfileAdmin(admin.ModelAdmin):
list_display = ('user', 'last_ip_address', 'can_read', 'end_ban_read', 'can_write', 'end_ban_write', 'last_visit')
list_filter = ('can_read', 'can_write')
search_fields = ['user__username']
raw_id_fields = ('user',)
raw_id_fields = ('user', 'hats')
Copy link
Contributor

Choose a reason for hiding this comment

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

On n'aura jamais des milliers de hats, tu peux le mettre dans un select au lieu d'un raw_id_field ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sait-on jamais, on a peut-être le prochain Facebook entre nos mais :p

@@ -517,6 +517,9 @@ def __init__(self, content, reaction, *args, **kwargs):
Field('last_note') if not last_note else Hidden('last_note', last_note)
)

if reaction is None: # it's a new comment
Copy link
Contributor

Choose a reason for hiding this comment

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

if not reaction: # we're not editing an existing comment ? :)

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 89.118% when pulling ae4f0aa on gcodeur:hats into c708844 on zestedesavoir:dev.

@coveralls
Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+0.01%) to 89.118% when pulling 36999fd on gcodeur:hats into c708844 on zestedesavoir:dev.

@AmarOk1412
Copy link
Member

@gcodeur tu peux me reping dimanche pour la QA ? J'aurais surement la possibilité d'y consacrer du temps

@gllmc
Copy link
Member Author

gllmc commented Jul 2, 2017

@AmarOk1412 : ping pour la QA, merci de ton aide :)

Avant de merger, @gustavi m'avait dit sur IRC avoir quelques remarques à faire.

@AmarOk1412
Copy link
Member

Suggestion : pouvoir enlever/modifier sa casquette à l'edit d'un message.

@AmarOk1412
Copy link
Member

Sinon pour le moment @gcodeur QA: OK

@gllmc
Copy link
Member Author

gllmc commented Jul 2, 2017

Merci pour la QA @AmarOk1412 !

Pour la modification de la casquette utilisée, ça me paraît assez complexe pour une première version : les casquettes utilisables sont celles que l'on a au moment où on poste. Si on peut éditer la casquette sur un ancien message, ça pose problème vu qu'on ne peut pas reconstituer quelles casquettes la personne avait quand elle a posté.

@coveralls
Copy link

coveralls commented Aug 6, 2017

Coverage Status

Coverage increased (+0.01%) to 89.275% when pulling cbdf6cf on gcodeur:hats into 9de6e5b on zestedesavoir:dev.

@pierre-24
Copy link
Member

Rapport de QA: rien à redire, vraiment, beau boulot :)

@pierre-24 pierre-24 self-requested a review August 6, 2017 13:47
Copy link
Contributor

@motet-a motet-a left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

coveralls commented Aug 6, 2017

Coverage Status

Coverage increased (+0.01%) to 88.886% when pulling 0304dd7 on gcodeur:hats into d9794b1 on zestedesavoir:dev.

@vhf vhf merged commit 4d1bec5 into zestedesavoir:dev Aug 6, 2017
@gllmc gllmc deleted the hats branch August 6, 2017 14:18
@gllmc gllmc added this to the Version de développement milestone Aug 6, 2017
@gllmc gllmc removed the QA svp label Aug 12, 2017
sandhose pushed a commit that referenced this pull request Sep 18, 2017
* Modèles et affichage d'une casquette

* Liste et modification des casquettes

* Supprime le badge staff + ajout en masse à un groupe

* Permet de choisir sa casquette en postant

* PEP-8

* QA d'@artragis

* Commentaires de QA

* Ajout de la documentation sur les casquettes

* Test de l'ajout en masse et utilisation de get_or_create

* Emploi des casquettes pour les MP automatiques

* Meta après les champs

* Aspect des casquettes sur les messages

* Réorganise les migrations

* Tests sur la gestion des casquettes et leur visibilité sur le profil

* Test d'envoi avec une casquette

* Tests d'envoi avec casquettes

* Revue de @vhf

* Affichage correct sur les MP

* Utilisation de la v25

* Remplace created par _

* PEP-8

* Erreur dans le rebase

* PEP-8
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.

None yet

9 participants