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

Alerte le staff en cas de nouveau fournisseur e-mail et lui permet de le bannir #4331

Merged
merged 27 commits into from May 30, 2017

Conversation

Projects
None yet
5 participants
@gcodeur
Member

gcodeur commented May 6, 2017

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

Cette PR ajoute un système envoyant une alerte aux membres du staff lorsqu'un nouveau fournisseur d'adresses e-mail est utilisé (lors de l'activation d'un compte ou de la modification de l'adresse e-mail). Une interface accessible via le menu utilisateur liste ainsi les nouveaux fournisseurs avec un bouton Approuver (= supprime l'alerte) et un bouton Bannir (= bannit le fournisseur et supprime l'alerte). Une autre page permet de gérer les fournisseurs bannis (et éventuellement d'en ajouter un manuellement).

Pour info, j'en ai profité au passage pour utiliser un ModelForm et une CreateView, ce qui permet de faire un formulaire de création d'objet de façon très simple.

Todo

  • Les tests.
  • Voir comment migrer proprement les données de la liste existante vers la BDD. Si quelqu'un a une idée sur la question, n'hésitez pas.

QA

  • S'inscrire avec un fournisseur inutilisé.
  • Se connecter sur un compte user et changer son e-mail avec encore une fois un fournisseur inutilisé.
  • Se connecter en admin et vérifier que deux alertes ont été créées.
  • Tester les boutons Approuver et Bannir.
  • Vérifier l'ajout et la suppression d'un fournisseur banni.
  • Vérifier qu'on ne peut pas s'inscrire (ou éditer son e-mail) avec un fournisseur banni.
  • Code review.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 6, 2017

Coverage Status

Coverage decreased (-32.9%) to 55.74% when pulling 97fb510 on GCodeur:fix-4313 into 9b12929 on zestedesavoir:dev.

coveralls commented May 6, 2017

Coverage Status

Coverage decreased (-32.9%) to 55.74% when pulling 97fb510 on GCodeur:fix-4313 into 9b12929 on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 6, 2017

Coverage Status

Coverage decreased (-0.08%) to 88.554% when pulling 548dfc5 on GCodeur:fix-4313 into 9b12929 on zestedesavoir:dev.

coveralls commented May 6, 2017

Coverage Status

Coverage decreased (-0.08%) to 88.554% when pulling 548dfc5 on GCodeur:fix-4313 into 9b12929 on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 8, 2017

Coverage Status

Coverage decreased (-0.08%) to 88.665% when pulling 07a6960 on GCodeur:fix-4313 into 10da96f on zestedesavoir:dev.

coveralls commented May 8, 2017

Coverage Status

Coverage decreased (-0.08%) to 88.665% when pulling 07a6960 on GCodeur:fix-4313 into 10da96f on zestedesavoir:dev.

@gcodeur

This comment has been minimized.

Show comment
Hide comment
@gcodeur

gcodeur May 9, 2017

Member

Les tests sont faits et j'ai inclus la feature demandée par @gustavi (recherche des membres utilisant un fournisseur). Par contre, j'ai toujours pas vraiment d'idée sur la manière la plus propre de transférer les fournisseurs actuellement interdits, je vais y réfléchir.

Member

gcodeur commented May 9, 2017

Les tests sont faits et j'ai inclus la feature demandée par @gustavi (recherche des membres utilisant un fournisseur). Par contre, j'ai toujours pas vraiment d'idée sur la manière la plus propre de transférer les fournisseurs actuellement interdits, je vais y réfléchir.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 11, 2017

Coverage Status

Coverage increased (+0.08%) to 88.813% when pulling 0447d0f on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

coveralls commented May 11, 2017

Coverage Status

Coverage increased (+0.08%) to 88.813% when pulling 0447d0f on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 13, 2017

Coverage Status

Coverage increased (+0.08%) to 88.815% when pulling 836e3dc on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

coveralls commented May 13, 2017

Coverage Status

Coverage increased (+0.08%) to 88.815% when pulling 836e3dc on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

@gcodeur gcodeur changed the title from [WIP] Alerte le staff en cas de nouveau fournisseur e-mail et lui permet de le bannir to Alerte le staff en cas de nouveau fournisseur e-mail et lui permet de le bannir May 13, 2017

@gcodeur

This comment has been minimized.

Show comment
Hide comment
@gcodeur

gcodeur May 13, 2017

Member

Normalement, tout y est : tests, migrations, etc. Il ne manque plus que la QA.

PS : la PR n'est pas si énorme qu'elle en a l'air : sur les 2400 "lines changed", il y en a plus de 1600 qui correspondent à l'ancien fichier supprimé et à la migration.

Member

gcodeur commented May 13, 2017

Normalement, tout y est : tests, migrations, etc. Il ne manque plus que la QA.

PS : la PR n'est pas si énorme qu'elle en a l'air : sur les 2400 "lines changed", il y en a plus de 1600 qui correspondent à l'ancien fichier supprimé et à la migration.

@artragis

C'est une chouette PR, juste quelques broutilles à changer je pense.

Show outdated Hide outdated zds/member/views.py Outdated
Show outdated Hide outdated zds/member/views.py Outdated
@require_POST
@login_required
@permission_required('member.change_bannedemailprovider', raise_exception=True)

This comment has been minimized.

@artragis

artragis May 16, 2017

Contributor

Il faudra mettre dans le UPDATE.md que comme il y a eu une nouvelle permission, il faut s'assurer que le staff est bien affiliée à cette permission, sinon ça va pas aller.

@artragis

artragis May 16, 2017

Contributor

Il faudra mettre dans le UPDATE.md que comme il y a eu une nouvelle permission, il faut s'assurer que le staff est bien affiliée à cette permission, sinon ça va pas aller.

This comment has been minimized.

@gcodeur

gcodeur May 16, 2017

Member

C'est déjà fait, ça. ^^

@gcodeur

gcodeur May 16, 2017

Member

C'est déjà fait, ça. ^^

Show outdated Hide outdated zds/member/validators.py Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 16, 2017

Coverage Status

Coverage increased (+0.08%) to 88.816% when pulling 08b6013 on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

coveralls commented May 16, 2017

Coverage Status

Coverage increased (+0.08%) to 88.816% when pulling 08b6013 on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 May 19, 2017

Member

Tient, chaipas dans quelle mesure c'est possible, mais y'aurais pas moyen d'aller retrouver le nombre d'utilisateurs bannis ou en LS utilisant un fournisseur donné ? Comme ça, on vois directement les domaines qui peuvent possiblement poser problème.

Member

pierre-24 commented May 19, 2017

Tient, chaipas dans quelle mesure c'est possible, mais y'aurais pas moyen d'aller retrouver le nombre d'utilisateurs bannis ou en LS utilisant un fournisseur donné ? Comme ça, on vois directement les domaines qui peuvent possiblement poser problème.

@gcodeur

This comment has been minimized.

Show comment
Hide comment
@gcodeur

gcodeur May 19, 2017

Member

@pierre-24 : actuellement, il y a possibilité de rechercher tous les comptes correspondant à un fournisseur interdit. Tu verrais ça comment, un filtre "Comptes sanctionnés uniquement" ?

(Par contre, je sais pas si ça a un sens vu que dans ce cas, ça veut dire que le fournisseur est déjà sur la liste noire. ^^)

Member

gcodeur commented May 19, 2017

@pierre-24 : actuellement, il y a possibilité de rechercher tous les comptes correspondant à un fournisseur interdit. Tu verrais ça comment, un filtre "Comptes sanctionnés uniquement" ?

(Par contre, je sais pas si ça a un sens vu que dans ce cas, ça veut dire que le fournisseur est déjà sur la liste noire. ^^)

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 May 20, 2017

Member

Par contre, je sais pas si ça a un sens vu que dans ce cas, ça veut dire que le fournisseur est déjà sur la liste noire. ^^

Ah, oui, ok, évidement. J'ai rien dit, du coup :p

Member

pierre-24 commented May 20, 2017

Par contre, je sais pas si ça a un sens vu que dans ce cas, ça veut dire que le fournisseur est déjà sur la liste noire. ^^

Ah, oui, ok, évidement. J'ai rien dit, du coup :p

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 22, 2017

Coverage Status

Coverage increased (+0.08%) to 88.817% when pulling 0b7da37 on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

coveralls commented May 22, 2017

Coverage Status

Coverage increased (+0.08%) to 88.817% when pulling 0b7da37 on GCodeur:fix-4313 into 96d91fd on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 25, 2017

Coverage Status

Coverage increased (+0.08%) to 89.03% when pulling 61251db on GCodeur:fix-4313 into 85fb311 on zestedesavoir:dev.

coveralls commented May 25, 2017

Coverage Status

Coverage increased (+0.08%) to 89.03% when pulling 61251db on GCodeur:fix-4313 into 85fb311 on zestedesavoir:dev.

Show outdated Hide outdated templates/member/settings/add_banned_email_provider.html Outdated
Show outdated Hide outdated templates/member/settings/banned_email_providers.html Outdated
Show outdated Hide outdated templates/member/settings/members_with_provider.html Outdated
Show outdated Hide outdated templates/member/settings/new_email_providers.html Outdated
@@ -0,0 +1,10 @@
# coding: utf-8

This comment has been minimized.

@gustavi

gustavi May 26, 2017

Member

Pourquoi mettre ça dans ce fichier ?

@gustavi

gustavi May 26, 2017

Member

Pourquoi mettre ça dans ce fichier ?

This comment has been minimized.

@gcodeur

gcodeur May 26, 2017

Member

C'est ce qu'on a fait pour ce genre de variables jusqu'à présent (exemple).

@gcodeur

gcodeur May 26, 2017

Member

C'est ce qu'on a fait pour ce genre de variables jusqu'à présent (exemple).

"""
class Meta:
verbose_name = 'Fournisseur banni'

This comment has been minimized.

@gustavi

gustavi May 26, 2017

Member

idem

@gustavi

gustavi May 26, 2017

Member

idem

Show outdated Hide outdated zds/member/models.py Outdated
Show outdated Hide outdated zds/member/urls.py Outdated
# create an alert for the staff if it's a new provider
if usr.email:
provider = usr.email.split('@')[-1].lower()

This comment has been minimized.

@gustavi

gustavi May 26, 2017

Member

J'ai pas testé mais ça match "@foo.xyz" si "@foo.*" est banni ?

@gustavi

gustavi May 26, 2017

Member

J'ai pas testé mais ça match "@foo.xyz" si "@foo.*" est banni ?

This comment has been minimized.

@gcodeur

gcodeur May 26, 2017

Member

Pas "@foo.*" mais "@foo.". C'est ce que contient notre fichier actuellement et ce que j'ai mis dans les instructions en haut du formulaire.

@gcodeur

gcodeur May 26, 2017

Member

Pas "@foo.*" mais "@foo.". C'est ce que contient notre fichier actuellement et ce que j'ai mis dans les instructions en haut du formulaire.

Show outdated Hide outdated zds/settings.py Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 26, 2017

Coverage Status

Coverage increased (+0.08%) to 89.033% when pulling 3b26a68 on GCodeur:fix-4313 into b15ddcb on zestedesavoir:dev.

coveralls commented May 26, 2017

Coverage Status

Coverage increased (+0.08%) to 89.033% when pulling 3b26a68 on GCodeur:fix-4313 into b15ddcb on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 27, 2017

Coverage Status

Coverage increased (+0.08%) to 89.033% when pulling 942afe2 on GCodeur:fix-4313 into b15ddcb on zestedesavoir:dev.

coveralls commented May 27, 2017

Coverage Status

Coverage increased (+0.08%) to 89.033% when pulling 942afe2 on GCodeur:fix-4313 into b15ddcb on zestedesavoir:dev.

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis May 29, 2017

Contributor

j'ai testé un cas :

le membre s'inscrit avec un domaine inconnu au bataillon (unnouveaumembre@unnouveaumembre.com).

Je me connecte en tant qu'administrateur. Aucun domaine n'apparaît dans la liste des "nouveaux domaines".

Contributor

artragis commented May 29, 2017

j'ai testé un cas :

le membre s'inscrit avec un domaine inconnu au bataillon (unnouveaumembre@unnouveaumembre.com).

Je me connecte en tant qu'administrateur. Aucun domaine n'apparaît dans la liste des "nouveaux domaines".

@gcodeur

This comment has been minimized.

Show comment
Hide comment
@gcodeur

gcodeur May 29, 2017

Member

@artragis : le fournisseur n'est ajouté qu'à l'activation du compte, tu l'as bien faite ?

Member

gcodeur commented May 29, 2017

@artragis : le fournisseur n'est ajouté qu'à l'activation du compte, tu l'as bien faite ?

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis May 30, 2017

Contributor

QA OK, je merge.

Contributor

artragis commented May 30, 2017

QA OK, je merge.

@artragis artragis merged commit dcb0216 into zestedesavoir:dev May 30, 2017

1 check passed

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

@gcodeur gcodeur removed the QA svp label May 30, 2017

@gustavi gustavi added this to the Version de développement milestone Jun 8, 2017

@gcodeur gcodeur deleted the gcodeur:fix-4313 branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment