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

Permet la désinscription #1400

Closed
wants to merge 76 commits into from
Closed

Conversation

artragis
Copy link
Member

Q R
Correction de bugs ? Oui
Nouvelle Fonctionnalité ? Oui
Tickets concernés #818, #1468

descirption fonctionnelle

côté front

  • le lien de désinscription ne doit apparaître que pour l'utilisateur connecté
  • le lien de désinscription doit mener à une page d'explication
  • une modale de confirmation doit s'afficher avant l'act de désinscription

côté back

La désinscription entraîne :

  • la supression du profile et donc la libération du pseudo et de l'email
  • la déconnexion du compte
  • l'anonymisation des données produites par l'utilisateur durant la période où il était inscrit sur le site :
    • les messages sur le forums sont envoyés au compte "anonyme" et toute trace d'édition par le membre est supprimée
    • les topics restent ouverts mais le nouvel auteur est "anonyme"
    • les articles et les tutoriels suivent cette règle :
      • si le tutoriel a été écrit par plusieurs personnes le membre se désinscrivant est juste enlevée de la liste des auteurs. Il sera de la responsabilité des autres auteurs de le citer si ce dernier accepte ou le demande;
      • si le tutoriel est publié, il passe à "Auteur externe" le membre devra faire une demande expresse s'il désire aussi retirer le tutoriel
      • si le tutoriel n'est pas publié, peu importe l'état dans lequel il se trouve (rédaction, bêta...) il est supprimer et la galerie associée aussi
    • les galeries non liées à un tutoriel sont données à "Auteur externe" (puisque l'image peut être considérée comme venant d'un "auteur") avec droit de lecture et d'écriture
    • les MP sont anonymisés, l'utilisateur quitte la discussion
    • les commentaires des tutoriels et articles sont transférés à "Anonyme".

@transaction.atomic
def unregister(request):
"""allow members to unregister"""
anonymous = get_object_or_404(User, username="anonymous")
Copy link
Contributor

Choose a reason for hiding this comment

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

je pense qu'il faudrait plutot que le nom de l'utilisateur anonyme soit dans une variable contenu de le setting. Ainsi, même ne dev, on peut facilement tester la fonctionnalité.

Copy link
Member Author

Choose a reason for hiding this comment

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

je voyais plutôt une fixture à charger dès le départ. De toute façon il va falloir passer par là, je pense. (idem pour external)

Copy link
Contributor

Choose a reason for hiding this comment

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

La fixture ne résoud pas le problème. Si demain on décide que l'utilisateur anonyme ne s'appelle pas ainsi, il faudrait pouvoir le modifier sans devoir modifier le code.

Copy link
Member

Choose a reason for hiding this comment

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

D'autant plus que l'utilisateur s'appelle Anonyme et qu'il existe déjà en prod (on l'a réservé avec Eskimon pour éviter les surprises)

message.save()
for topic in Topic.objects.filter(author = request.user):
topic.author = anonymous
topic.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

problème d'indentation par ici

@firm1
Copy link
Contributor

firm1 commented Aug 26, 2014

Plus généralement, il faudrait en plus :

  • supprimer les galleries de l'utilisateur
  • empecher d'envoyer un MP à l'utilisateur annonyme (il doit même pas être sélectionnable dans l'autocompletetion)
  • sortir l'utilisateur de ses MPs

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.24%) when pulling f14fc75 on artragis:desinscription into 79a459b on zestedesavoir:master.

@Eskimon Eskimon added Back and removed Bloquant labels Aug 26, 2014
# Constant for anonymisation

ANONYMOUS_USER_PK = 5
EXTERNAL_USER_PK = 6
Copy link
Contributor

Choose a reason for hiding this comment

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

pour rester cohérent avec le formalisme des users paramétrés, il serait préférable de renseigner des username dans le settings et non des clés. cf le cas de l'utilisateur bot qui envoi les MP

Copy link
Member Author

Choose a reason for hiding this comment

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

je modifierai ça plus tard

@firm1
Copy link
Contributor

firm1 commented Aug 28, 2014

De manière générale aussi, il faudrait des tests associés à une feature aussi sensible.

artragis and others added 17 commits September 7, 2014 20:11
- Pour les galeries des tutos non publiés, elles doivent être supprimées. En effet, plus personne n'y sera rattaché (même pas auteur externe) et leurs tutos respectifs sont viré du disque dur. Du coup il faut les virer aussi pour éviter les données fantôme.

- Pour les MP. Prenons le cas d'un MP a trois personnes, A, B et C. Ils s’échangent tous plein de MP dans un fil à tout les trois. Si B quitte le site, A et C doivent pouvoir continuer le MP sans le perdre. Donc un membre partant doit juste quitter le MP sans qu'il soit supprimé.
…into desinscription

Conflicts:
	zds/member/views.py
@SpaceFox
Copy link
Member

SpaceFox commented Sep 7, 2014

OK, je pense qu'on a un problème : là tout de suite cette PR est cassée, et les dernières informations que j'ai donnent ceci :

artragis and others added some commits 2 hours ago

L'avant-dernière ligne de mon extrait parle de corriger l'issue #1468.

Que les choses soient bien claire : cette correction n'a absolument rien à faire sur cette PR, qui est déjà monstrueuse et ultra-sensible !

Je rappelle à tout hasard que cette PR est importante parce que :

  • Elle permet de nous simplifier énormément un besoin légal
  • Son merge déclenchera la sortie de la v1.1, qui est déjà gigantesque en terme de nombre de PR fusionnées.

Il est donc totalement exclus d'alourdir encore cette PR en rajoutant des corrections qui n'y sont pas strictement attachées, quand bien même ces corrections paraîtraient triviales.

D'autre part, d'après ce que je comprends de l'historique Git, la PR corrective de #1468 est basée sur dev et non master contrairement à cette PR... ce qui ramène tout dev à un moment où on a vraiment pas besoin de ça.

Ce qu'il y a à faire maintenant

  1. Sauvegarder le dépôt dans un coin
  2. Surtout ne rien rajouter sur le code de @artragis
  3. Resetter la branche au commit précédent le merge de la chose de @Alex-D
  4. Vérifier que le reset n'a rien cassé
  5. Prier / sacrifier un poulet (je m'occupe du corps, ne vous inquiétez pas !)
  6. Pousser ce reset sur Github pour mettre à jour cette PR

@pierre-24
Copy link
Member

Sinon, j'ai une proposition "alternative" : vu l’ampleur de la tâche, vu qu'il faudra a un moment ou un autre repasser dev dedans, vu qu'il y a encore un peu de front à faire, je propose de créer une branche dédiée à cette feature sur le dépot principal du zds (et non plus chez artragis) puis ensuite de faire un "cherrypick" de la branche d'artragis vers cette nouvelle branche principale. L'avantage, c'est qu'on peut alors appliquer le zestflow sur cette branche dédiée (PR, QA et tout ça) puis ensuite, quand c'est ok, merger la branche en entier et non plus une PR qui commence à prendre une ampleur démesurée.

(à noter que dans mon souvenir, le zestflow prévoyais ce genre de cas pour la mise en place de ZEP et autres features de grandes tailles, en tout cas, c'est le cas dans le git flow, je n'arrive plus à retrouver une discution qui énonce que c'est également le cas dans le Zestflow)

EDIT: ça n'empêche qu'on aura surement besoin d'un sacrifice de poulet pour mener a bien la tâche.

@artragis
Copy link
Member Author

artragis commented Sep 7, 2014

Personnellement, dès demain matin, comme je serai au calme, je ferai une PR basée sur la v1.1 avec tout ce qui a été fait aujourd'hui et sans le problème amené par @Alex-D . (merci pour tout mec)

Cela réglera aussi le problème de l'historique qui était assez foireux car j'avais parfois des typo, parfois des incompréhensions, parfois des choses non faites etc.

Donc, voilà je refais tout demain (08/09/2014) matin.

@artragis artragis closed this Sep 7, 2014
@SpaceFox
Copy link
Member

SpaceFox commented Sep 7, 2014

je ferai une PR basée sur la v1.1

Sur dev plutôt, puisque la v1.1 n'existe pas encore (on attend ta PR pour la lancer) :)

@artragis
Copy link
Member Author

artragis commented Sep 7, 2014

il est 22h30 et la france vient de se prendre un but con. Désolé,
confusion de ma part.
Le 07/09/2014 22:25, SpaceFox a écrit :

je ferai une PR basée sur la v1.1

Sur |dev| plutôt, puisque la v1.1 n'existe pas encore (on attend ta PR
pour la lancer) :)


Reply to this email directly or view it on GitHub
#1400 (comment).

@Alex-D
Copy link
Contributor

Alex-D commented Sep 7, 2014

Alors, vu que je suis visiblement encore le problème, alors qu'il suffisait très clairement de faire un

git reset --hard <le-commit-où-revenir>

mais qu'il a été préféré de faire un rebase, outil non adéquat dans cette situation, je me propose de faire le gros rebase + compatibilité avec la branche dev. Je ferais aussi une PR séparée pour réparer les autres choses non liées à cette PR.

Donc voilà, je m'y met et dès que j'ai un truc qui me semble bien, je crée une branche sur le repo zds directement et la PR qui va bien. Il s'agit typiquement du cas où c'est une grosse feature et le fait que ce soit sur le repo d'un des devs limite les possibilités.

Keep calm, des bisous, et à plus tard.

/me lance le git rebase de la mort numéro 2.

@pierre-24
Copy link
Member

alors qu'il suffisait très clairement de faire un

git reset --hard <le-commit-où-revenir>

mais qu'il a été préféré de faire un rebase

Très, très, très (mais vraiment très, j'ai les logs) objectivement, artragis et Eskimon on essayé, avec le résultat que ... Ben ça n'as pas marché ^^ (vraiment). Du coup, quand on laisse des devs qui métrisent pas git tenter de rattraper tout ça (et c'est pas une critique, j'en fait partie), ben ça emplois le bazooka pour tuer la mouche. Bim.

Il s'agit typiquement du cas où c'est une grosse feature et le fait que ce soit sur le repo d'un des devs limite les possibilités.

Me disais bien que ma proposition n'était pas folle ^^

@Alex-D
Copy link
Contributor

Alex-D commented Sep 7, 2014

artragis et Eskimon on essayé, avec le résultat que ... Ben ça n'as pas marché ^^ (vraiment).

arf :/ C'est bizarre, je viens de le faire et ça fonctionne. C'est pas bien grave, je nettoie tout ça petit à petit, j'arrive à un résultat plutôt satisfaisant là :)

@Alex-D Alex-D mentioned this pull request Sep 7, 2014
@SpaceFox SpaceFox modified the milestone: Version 1.1 Sep 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bloquant Ticket qui doit être traité avant la prochaine mise à jour C-Back Concerne le back-end Django
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants