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

Intégration des comptes réseaux sociaux #1424

Closed
wants to merge 9 commits into from

Conversation

firm1
Copy link
Contributor

@firm1 firm1 commented Sep 2, 2014

Q R
Correction de bugs ? Non
Nouvelle Fonctionnalité ? Oui
Tickets concernés #381

HS : une PR plus longue a rédiger que de taper le code \o/

Cette PR intègre au site la connexion via les réseaux sociaux Facebook, Twitter, et Google.

Le principe est assez simple. Un utilisateur qui veut s'inscrire ou se loguer sur le site, a désormais 4 possibilité

  • saisir son login, mail et mot de passe
  • utiliser son compte facebook
  • utiliser son compte googleplus
  • utiliser son compte twitter.

Par défaut lorsque l'on s'inscrit via un compte réseau social, le compte est crée dans la base des utilisateurs de zds. Et on est directement connecté avec ce nouveau compte. On peut changer par la suite notre pseudo et toutes les autres informations de profil.

Les authentifications sont faites directement via les API Oauth2 d'authentifications, donc tout est sécurisé.

Note pour QA
Cette PR est assez complexe a tester, mais ça vaut le coup je pense. Etant donné que je ne peux pas renseigner les clés des applications sociales des vrais comptes du site, la QA va bosser un peu. Il faut donc après avoir crée un fichier vide dans zds-site/zds/settings_prod.py.

  1. Sur un compte facebook :
    • allez sur https://developers.facebook.com/apps/?action=create et cliquer sur "Create New App" en vert
    • Dans les paramètre de l'application crée cliquez sur “Add Platform”. Dans les options fournies, choisissez Web, et remplissez l'url du site avec "http://127.0.0.1:8000" (adaptez l'ip en fonction de l'adresse sur laquelle vous testez zds en local)
    • dans votre fichier settings_prod.py rajouter les variables SOCIAL_AUTH_FACEBOOK_KEY = "clé"
      et SOCIAL_AUTH_FACEBOOK_SECRET = "secret" obtenu via l'application facebook
  2. Sur un compte twitter :
    • allez sur https://apps.twitter.com/app/new et creez une nouvelle application
    • remplissez les informations, et dans votre url de callback pensez à renseigner http://127.0.0.1:8000/complete/twitter/ (adaptez l'ip en fonction de l'adresse sur laquelle vous testez zds en local)
    • dans votre fichier settings_prod.py rajouter les variables SOCIAL_AUTH_TWITTER_KEY = "clé"
      et SOCIAL_AUTH_TWITTER_SECRET = "secret" obtenu via l'application twitter
  3. Sur un compte google plus :
    • allez sur https://console.developers.google.com/ et creez une nouvelle application
    • remplissez les informations, et dans votre url de callback pensez à renseigner http://127.0.0.1:8000/complete/google-oauth2/ (adaptez l'ip en fonction de l'adresse sur laquelle vous testez zds en local)
    • dans votre fichier settings_prod.py rajouter les variables SOCIAL_AUTH_GOOGLE_OAUTH2_KEY = "clé" et SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET = "secret" obtenu via l'application google

Après ce paramétrage, essayez de vous connecter via chaque compte, et jouez avec.

Je laisse un screen du rendu.

capture d ecran de 2014-09-02 23 49 09
capture d ecran de 2014-09-02 23 48 51

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling 37ed37f on firm1:social-integration into 68383f7 on zestedesavoir:dev.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 2, 2014

  1. C'est Google+ et non Google Plus
  2. l'intégration est moche
  3. c'est pas ergonomique
  4. j'imagine que ça n'est pas responsive
  5. le CSS a été rajouté de manière random dans le code : pourquoi c'est au milieu des modales au lieu d'être avec les autres boutons ? Et qu'on me dise pas "c'est parce qu'il n'y a pas de doc" alors qu'un simple Ctrl+F avec la chaîne btn suffit à tomber dessus
  6. tel que c'est exposé là, on dirait que ça ne te dérangerait pas de laisser ça comme ça
  7. tu demande rien aux gens du front (sandhose ou moi) en balançant un "voilà le rendu"

Au delà de ça, on est que deux à avoir discuté de ça, on a pas l'avis d'autres gens, ça me semble léger, même si je suis plutôt pour cette intégration.

@firm1
Copy link
Contributor Author

firm1 commented Sep 2, 2014

-3
-4
-5
-6

La doc du code front est inexistante. Je ne peux pas deviner ou mettre les bons éléments (j'ai déjà eu du mal à retrouver le css qu'il fallait modifier pour avoir un minimum de rendu).

J'attends des PR sur cette branche pour fixer le front. Une doc serait encore mieux, car là c'est un peu le bordayle dans les scss.

Au delà de ça, on est que deux à avoir discuté de ça, on a pas l'avis d'autres gens, ça me semble léger, même si je suis plutôt pour cette intégration.

bah si ça ne plait pas a ne sera pas une grande perte, vu le temps que ça m'a pris.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 2, 2014

C'est pas un problème de doc... faut revoir l'interface pour intégrer les boutons de façon réfléchie, pas balancés à l'arrache au dessus du formulaire.

@firm1
Copy link
Contributor Author

firm1 commented Sep 2, 2014

C'est pas un problème de doc...

Doc ou complétude du front. Là j'ai regardé dans les sprites, et j'ai aucune image pour les reseaux sociaux, aucune gestion correcte des grilles dans les pages. Donc partant de là, difficile de faire un truc élégant sans revoir encore chaque page.

'fin bref ... j'attend une PR pour améliorer le front, ici le but c'est surtout de tester que coté back tout va bien.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 3, 2014

j'ai aucune image pour les reseaux sociaux

Pourquoi on devrait les avoir ? On ne s'en sert nul part.

aucune gestion correcte des grilles dans les pages

On ne s'en sert nul part

difficile de faire un truc élégant sans revoir encore chaque page.

Peut-être parce que, pour le coup, il faut revoir les pages ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling cff1b41 on firm1:social-integration into 68383f7 on zestedesavoir:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling cff1b41 on firm1:social-integration into 68383f7 on zestedesavoir:dev.

@SpaceFox
Copy link
Contributor

SpaceFox commented Sep 3, 2014

Il faudrait en discuter, mais pour moi si on présente cette fonctionnalité (je n'en vois pas l'utilité mais je n'ai rien contre non plus), c'est la connexion standard qui devrait être mise en valeur.

Quant à la doc, je ne comprends pas bien : les dernières informations que j'avais dessus, c'est qu'elle était super rapide à faire.

@@ -33,3 +33,4 @@ https://github.com/zestedesavoir/Python-ZMarkdown/archive/2.4.1-zds.10.zip
flake8
autopep8
easy-thumbnails
python-social-auth
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 la PR #1418, il faudrait préciser la version utilisée

@cgabard
Copy link
Contributor

cgabard commented Sep 3, 2014

@firm1 : Sera t'il possible a un compte existant de lié son compte actuel avec un de ceux des RS ?

@cgabard
Copy link
Contributor

cgabard commented Sep 3, 2014

C'est pas un problème de doc...

Si @Alex-D . Ce n'est pas parce que ton code est évident pour toi que c'est le cas pour tout le monde. Sans documentation tu ne peux pas reprocher aux gens de faire comme ils peuvent avec ce qu'ils ont. Je ne sais pas si @firm1 a fait l'effort ou non, qu'il l'a fait a l'arrache ou a vraiment cherché, mais tu ne peux pas lui repprocher tant qu'on a aucune documentation ni guideline.

Donc de là, plusieurs solutions :

  • Toi ou Sandhose corrigez le design de cette branche,
  • Tu fais la doc rapide que tu avais promis et firm1 corrige,
  • On garde ainsi.

@Eskimon
Copy link
Contributor

Eskimon commented Sep 3, 2014

On garde ainsi.

Non.

On va pas se mettre a merger des trucs sans cohérence graphique, ca rime a rien.

@firm1
Copy link
Contributor Author

firm1 commented Sep 3, 2014

j'ai aucune image pour les reseaux sociaux

Pourquoi on devrait les avoir ? On ne s'en sert nul part.

aucune gestion correcte des grilles dans les pages

On ne s'en sert nul part

difficile de faire un truc élégant sans revoir encore chaque page.

Peut-être parce que, pour le coup, il faut revoir les pages ?

C'est bien ça le souci le design est assez pauvre en élements graphiques (n'importe quel pack d'icone propose des icones de reseaux sociaux) et pas flexible, du coup chaque nouveau truc à rajouter est une plaie.

Il faudrait en discuter, mais pour moi si on présente cette fonctionnalité (je n'en vois pas l'utilité mais je n'ai rien contre non plus), c'est la connexion standard qui devrait être mise en valeur.

Je suppose que tu parle de mise en valeur graphique ? Si c'est le cas, c'est un problème de front-end, mais le design en l'état étant vraiment trop contraignant, et pas flexible. Pour rajouter un nouveau bouton dans une page il faut revoir complètement les css, donc je laisse la main au front sur ce point.

@firm1 : Sera t'il possible a un compte existant de lié son compte actuel avec un de ceux des RS ?

En soit tout est possible. Mais ce que tu demandes c'est l'objet d'une ZEP pas encore finalisée il me semble. Là je me suis juste attaqué à l'issue

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling 09c587f on firm1:social-integration into 68383f7 on zestedesavoir:dev.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 3, 2014

C'est bien ça le souci le design est assez pauvre en élements graphiques (n'importe quel pack d'icone propose des icones de reseaux sociaux) et pas flexible, du coup chaque nouveau truc à rajouter est une plaie.

Cette phrase illustre parfaitement que tu es très certainement un bon dev back, mais un très très mauvais dev front. Pourquoi ? Parce que le front doit être le plus léger possible, c'est ce qu'on envoie au client, ça bouffe du réseau sur le serveur et chez le client. Si on commence à ajouter 150 icônes dans le sprite, il fera 150Mo alors qu'on a besoin que de 5 icônes. Donc, après te voir nous dire "il faudrait compresser le sprite" c'est juste LOLesque, parce que ce sont deux idées opposées : d'un côté ça ne te dérange pas d'avoir des icônes inutiles, mais de l'autre tu veux quand même optimiser l'image pour gagner quelques ko VS les Mo qu'on perd d'un autre côté.

Donc la réponse simple à cela est : on a jamais eu besoin d'icônes de réseaux sociaux jusque là, c'est donc tout à fait logique qu'on ne les ait pas dans le sprite.

Avant de dire "y a pas de doc" faut aussi réfléchir à comment est conçu le truc. C'est pour cette raison même que je n'aime pas les frameworks CSS : ils sont beaucoup trop lourds pour rien.

Nul part sur le site on utilise des grilles, parce que tout est trop spécifique et rien ne se ressemble. C'est pour ça que les seules grilles existantes sont là pour faire du 2 ou 3 colonnes et encore.

Enfin bref, c'est un POC back qui nécessite de refaire partiellement le front pour l'intégrer.

@firm1
Copy link
Contributor Author

firm1 commented Sep 3, 2014

Pourquoi ? Parce que le front doit être le plus léger possible [...]

En général quand on travaille avec des sprites, on en fait plusieurs. Typiquement un sprite social pour regrouper toutes les icones sociales et qui ne seront pas nécessairement utilisées sur chaque page du site. C'est le principe de la découpe flexible. ça n'apporte que des avantages et c'est customisable a souhait. Tout les framework css bien fait propose l'intégration par module pour éviter justement de charger des trucs dont on a pas besoin.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 3, 2014

Il y a alors deux écoles :

  • celle que tu énonce qui augmente le nombre de requêtes HTTP
  • celle que je préfère qui consiste à n'avoir que ce dont on a besoin, le tout dans le minimum de fichiers : actuellement on a une requête HTTP pour le CSS, une pour le sprite et deux pour le JS (vendors et code custom du site).

L'aisance de développement ne doit pas entraver les performances.

@Alex-D Alex-D added the Back label Sep 4, 2014
@firm1
Copy link
Contributor Author

firm1 commented Sep 8, 2014

Bon bah conflit ici ... :(

Ceci dit, toujours en attente d'une PR pour le design de cette page (c'est vraiment vraiment pénible de devoir revoir les css pour chaque nouveau bouton rajouter ... vraiment pénible)

@Alex-D
Copy link
Contributor

Alex-D commented Sep 8, 2014

C'était une issue v2, on bosse sur d'autres choses tandis qu'ici ça ne presse pas. Pas la peine d'en rajouter une louche à chaque message.

@cgabard
Copy link
Contributor

cgabard commented Sep 8, 2014

on bosse sur d'autres choses

Chouette ! On va avoir notre doc !

@firm1
Copy link
Contributor Author

firm1 commented Sep 15, 2014

Après un rebase et une mise à jour de la PR. Les screens deviennent

capture d ecran de 2014-09-15 02 48 34


capture d ecran de 2014-09-15 02 48 30

@Eskimon
Copy link
Contributor

Eskimon commented Sep 15, 2014

C'est normal que les images soient pas centrées dans les boutons ?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6bcb8d6 on firm1:social-integration into * on zestedesavoir:dev*.

@Alex-D
Copy link
Contributor

Alex-D commented Sep 16, 2014

C'est normal que les images soient pas centrées dans les boutons ?

Oui, les icônes doivent être accompagnées d'un texte lorsqu'elles sont présentes dans un bouton, comme présenté dans la documentation tout en bas dans la section "Icônes".

Tels qu'ils sont placés là, ça n'est de toute façon pas du tout ergonomique, ces boutons sont un moyen alternatif de connexion, ils doivent être visuellement séparés du formulaire traditionnel.

@firm1
Copy link
Contributor Author

firm1 commented Oct 26, 2014

Rebasé et corrigé.

QA can continue

@firm1 firm1 added Validation and removed En cours labels Oct 26, 2014
@firm1
Copy link
Contributor Author

firm1 commented Oct 27, 2014

QA or not QA ? That's the question

@GerardPaligot
Copy link
Member

Je l'aurais bien fait mais je n'ai pas tous les comptes des réseaux sociaux que tu utilises.

@Alex-D
Copy link
Contributor

Alex-D commented Oct 27, 2014

Fais la QA pour G+ et Twitter, quelqu'un d'autre fera Facebook. Ca sera toujours ça de fait.

@Eskimon
Copy link
Contributor

Eskimon commented Oct 28, 2014

Bon, perso j'ai essayé les 3, je me suis vautré pour créer les comptes sur les trois...

  • Facebook me demande mon téléphone : How about No...
  • La page de Twitter marche pas (peux pas me loguer)
  • Google c'est un bordel sans nom j'ai pas réussi à trouver les trucs, j'ai pas envie d'y passer 2 heures

@firm1
Copy link
Contributor Author

firm1 commented Nov 4, 2014

Bon, pour ceux qui veulent QA, j'ai crée un compte bidon pour Google et les infos sont :

#zds/settings_prod.py
SOCIAL_AUTH_GOOGLE_OAUTH2_KEY="651539647051-fnd4cqkb6vv2egeq4b5qqtvenuoreq5c.apps.googleusercontent.com"
SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET="4isKwv7_MaNlp5hryfOPvqAa "

Pensez à lancer votre serveur python sur 127.0.0.1:8000

@Alex-D
Copy link
Contributor

Alex-D commented Nov 4, 2014

J'essayerais de QA ça vu qu'à priori personne n'est familier avec les API de ces outils, les ayant déjà utilisé tous les trois, je devrais pouvoir m'en sortir \o/

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Bon, j'essairai de regarder pour google... Mais sinon je suppose qu'on va devoir faire ca en mode "je fais confiance a firm1" et confirmer sur la preprod non ? Car sinon cette PR ne verra jamais le merge...

form.helper.form_action += "?next=" + next_page
else:
form = LoginForm()
form.helper.form_action = reverse("zds.member.views.login_view")
Copy link
Contributor

Choose a reason for hiding this comment

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

Il faut une indentation la non ? (je suis en train de rebaser pour faire un test propre)

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 suis incapable de le dire comme ça, mais je crois que ce que j'ai fait a l'époque là dessus était bon

Copy link
Contributor

Choose a reason for hiding this comment

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

Si tu regardes bien, la ligne 647 ecraserait ce que tu fais à la ligne 644 dans tout les cas... Ce serait pas logique

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Bon, J'ai fait ZE rebase, je fais la QA pour google via les données filé par @firm1 et je ferais une branche que je mergerais moi-même car j'ai pas envie de refaire cette QA. Est-ce que ca va a tout le monde ?

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

ça me va perso

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

@firm1 avec ton truc google j'ai une 401 (401 Client Error: Unauthorized)

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

Arf, tu démarre bien sur le 127.0.0.1 ?

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Yep. Je viens de forcer le demarrage du serveur sur l'ip/port pour etre sur et toujours pareil

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

Re-check avec ça stp :

#zds/settings_prod.py
SOCIAL_AUTH_GOOGLE_OAUTH2_KEY="696570367703-r6hc7mdd27t1sktdkivpnc5b25i0uip2.apps.googleusercontent.com"
SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET="mApWNh3stCsYHwsGuWdbZWP8"

Toujours sur le 127.0.0.1 et sur le port 8000

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

erreur 400. That’s an error.

Error: redirect_uri_mismatch

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

Et là ?

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Toujours erreur 400...

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

The redirect URI in the request: http://127.0.0.1:8000/complete/google-oauth2/ did not match a registered redirect URI.

Request Details

response_type=code
scope=email profile
redirect_uri=http://127.0.0.1:8000/complete/google-oauth2/
state=sQZ4P5odreRTqN9IhOdSpC53wYrHjpVu
client_id=696570367703-r6hc7mdd27t1sktdkivpnc5b25i0uip2.apps.googleusercon

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

Allez un autre essai ? Voilà pourquoi je n'aime pas laisser dormier les PRs longtemps, on fini par oublier comment on avait fait

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Je passe google mais de nouveau une 403 quand ca redirige vers le site

@firm1
Copy link
Contributor Author

firm1 commented Nov 25, 2014

Je passe google mais de nouveau une 403 quand ca redirige vers le site

Arf, bon si tu peux me PR ton rebase, je vais revérifier ça.

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

ok je fais la branche de suite !

@Eskimon
Copy link
Contributor

Eskimon commented Nov 25, 2014

Voili voila : #1799

@Eskimon Eskimon closed this Nov 25, 2014
@firm1 firm1 deleted the social-integration branch November 5, 2019 08:53
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants