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 smileys clems #4408

Merged
merged 21 commits into from
Aug 20, 2017
Merged

Intégration des smileys clems #4408

merged 21 commits into from
Aug 20, 2017

Conversation

A-312
Copy link
Contributor

@A-312 A-312 commented Jul 17, 2017

Q R
Type de modification évolution
Ticket(s) (issue(s)) concerné(s) #4404

QA

Il est possible de tester le résultat ici, avec l'utilisateur user (mdp: user), parce que j'ai changé les mots de passe de admin et staff pour éviter les ennuis, et que les mails ne fonctionnerons pas pour vous inscrire.

  • Admirez les beaux smileys clems ici. En vous connectant, vous pouvez constater que l'éditeur intègre les nouveaux smileys.
  • Rendez vous dans le profil et cochez l'option pour revenir au anciens smileys.
  • Faites un bon CTRL+F5 pour recharger le cache (même si c'est pas forcément utile) et admirez le résultat ici.
  • Vérifiez que ça disparait à la déconnexion (encore une fois, ça peut nécéssiter de vider le cache)

Avant:

2

Et après:

1

@coveralls
Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage remained the same at 89.106% when pulling 138d10b on A-312:feature-4404 into 3053068 on zestedesavoir:dev.

@artragis
Copy link
Member

Je regarde la review plus tard, mais déjà je te remercie pour ton courage. Je suis passé par là pour les galères de rebase et je sais combien c'est démotivant. Bravo !

@pierre-24
Copy link
Member

Tu as besoin d'aide pour la "deuxième partie" ?

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage remained the same at 89.278% when pulling cbf03a3 on A-312:feature-4404 into 82c2e6f on zestedesavoir:dev.

@A-312
Copy link
Contributor Author

A-312 commented Aug 2, 2017

Tu as besoin d'aide pour la "deuxième partie" ?

Oui je n'ai jamais utilisé nginx & l'intégration du back-end semble faire appel à des fonctions spécifiques.

@pierre-24
Copy link
Member

Pour la partie nginx, c'est de toute façon à faire par un admin au moment de la MEP. Il faut juste mettre les instructions. Côté back-end, il "suffit" de placer un cookies, ça ne devrait pas être exceptionnelement compliqué. Je veux bien m'en occuper si nécéssaire.

Note à moi-même (ou quiquonque de motivé): pas oublier d'éditer la page expliquant les cookies ;)

@vhf vhf added the QA svp label Aug 4, 2017
@vhf vhf changed the title Intégration des smileys clems [WIP] Intégration des smileys clems Aug 4, 2017
@vhf vhf added C-Front Concerne l'interface du site C-Infra Concerne l'infrastructure technique sous le site labels Aug 4, 2017
@vhf vhf modified the milestone: Version de développement Aug 6, 2017
@pierre-24
Copy link
Member

En fait, nginx, ça va pas marcher: quid des dévellopeurs qui testent en local ?

@pierre-24
Copy link
Member

Ou bien on laisse les smileys à leur ancienne place, et on utilise nginx pour la redirection vers /static/smileys/clem/xxx. Çan ça peut fonctionner.

@motet-a
Copy link
Contributor

motet-a commented Aug 8, 2017

Il n’y a pas moyen d’utiliser cette histoire de redirection avec nginx juste en production et écrire un bout de Python avec Django pour faire l’équivalent en local ?

@pierre-24
Copy link
Member

Ben non, sinon je tenterai d'utiliser le "bout de python" en production aussi. Mais la gestion des fichiers statiques est un peu étonnante avec Django, du coup l'option ne fonctionnera pas en local. Je dirais que c'est un moindre mal :-°

@motet-a
Copy link
Contributor

motet-a commented Aug 8, 2017

Juste une dernière question : À assez long terme, c’est vraiment pas possible de réécrire (éventuellement progressivement) le HTML caché ? Si c’est possible, on pourrait peut-être se contenter d’un hack temporaire côté client 🙃.

@pierre-24
Copy link
Member

Ça demande quand même le re-passage à la moulinette de tout les messages du forum, des MP, et de tout les contenus validés. Même si c'est pas impossoible, c'est beaucoup de temps pour pas grand chose ^^

@motet-a
Copy link
Contributor

motet-a commented Aug 8, 2017

Je viens de réfléchir un peu à tout ça et le hack côté client ne me semble pas si dégueu, parce que :

  • Si on redirige une URL X vers une URL Y ou une URL Z en fonction d’un cookie, je ne vois pas comment on peut mettre un max-age pour que le client évite d’envoyer une requête pour chaque image qu’il a déjà en cache.

  • Les cookies ne peuvent pas être partagés entre différents domaines. En production c’est facile, mais j’ai peur que ça complique les choses en local.

  • Expliquer à tout le monde comment configurer nginx en local risque d’être douloureux (bon, avec docker-compose c’est plus facile).

  • Avec un bout de JS, on fait juste (en gros) s/vielle-url/nouvelle-url/g sur la page et il n’y a même pas besoin de toucher à nginx, non ?

  • Si des gens ont des problèmes avec le bout de JS qui fait ça, ils resteront avec les anciens emojis et ne se plaindront pas ;-)

@WinXaito
Copy link
Contributor

WinXaito commented Aug 8, 2017

Et le fait de dire <Les anciens messages restent avec les anciens emoji, et les nouveaux messages passe sur les nouveaux emoji> et éventuellement la solution de @motet-a pour les anciens messages avec sont JS.

@A-312
Copy link
Contributor Author

A-312 commented Aug 11, 2017

On n'inverse pas plutôt le contenu des fichiers ?

@WinXaito
Copy link
Contributor

Je préfèrerais aussi les smileys clem par défaut, après il s'agit de goût et de couleur.
Je dois dire que je vois les choses comme ceci:

  • Une personne lambda arrive sur le site, elle aime la beauté et par conséquent de beau smiley, elle n'a pas envie ou ne sais pas comment chercher les smileys dans les options.
  • Le vieux barbu de son côté aime les vieux smileys, avec son air de geek il sera au courant qu'on peut les changer dans les paramètres et s'empressera de trifouiller dans les entrailles du site...

C'est un grand , aucune volonté d'être insultant ✌️

update.md Outdated
+ Ajouter `ZDS_APP['member']['clem_smileys_allowed'] = True` au `settings_prod.py`.
+ Télécharger le fichier [`clem_smileys.conf`](https://github.com/zestedesavoir/zds-site/blob/dev/doc/source/install/configs/nginx/snippets/clem_smileys.conf) et le placer dans `/etc/nginx/snippets/zds/`.
+ Éditer `/etc/nginx/sites-available/zestedesavoir` et ajouter `include snippets/zds/clem_smileys.conf;` dans le bloc `location ~* ^/(static|media|errors)/ {` (après la ligne 66?)
+ Redémarer nginx: `systemctl restart nginx`
Copy link
Contributor

Choose a reason for hiding this comment

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

Recharger nginx: `systemctl reload nginx`

(Redémarrer n'est pas utile ici ;) )

Copy link
Member

Choose a reason for hiding this comment

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

Même si on chipote aux fichiers de config, y'a un auto-reload ? (vraie question, parce que pas activé chez moi ^^)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah non y'a pas d'auto-reload. C'est pour ça qu'il faut dire à systemd de reloader nginx.

@@ -250,6 +253,9 @@ def __init__(self, *args, **kwargs):
self.helper.form_class = 'content-wrapper'
self.helper.form_method = 'post'

if settings.ZDS_APP['member']['clem_smileys_allowed']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Comme ça entre directement dans la codebase, y compris migrations, mets ça directement dans la liste multi_choices l.220 :)

Copy link
Member

Choose a reason for hiding this comment

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

En fait, je part du principe que cette option (ou son inverse si on inverse la logique) ne fonctionne pas si le sysadmin n'applique pas la modification à NGINX. Ce pourquoi je préfère être prudent et laisser une option désactivant l'option si nécessaire :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Je préfère qu'on s'assure sur la beta que ça fonctionne.

migrations.AddField(
model_name='profile',
name='use_clem_smileys',
field=models.BooleanField(default=False, verbose_name='Utilise les smileys Clem ?'),
Copy link
Contributor

@vhf vhf Aug 11, 2017

Choose a reason for hiding this comment

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

default=True comme discuté dans les commentaires ? 😺

@@ -73,6 +73,7 @@ class Meta:
can_write = models.BooleanField("Possibilité d'écrire", default=True)
end_ban_write = models.DateTimeField("Fin d'interdiction d'écrire", null=True, blank=True)
last_visit = models.DateTimeField('Date de dernière visite', null=True, blank=True)
use_clem_smileys = models.BooleanField('Utilise les smileys Clem ?', default=False)
Copy link
Contributor

@vhf vhf Aug 11, 2017

Choose a reason for hiding this comment

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

Donc en fait ici et dans le fichier juste précédent.

return redirect(reverse('homepage'))
response = redirect(reverse('homepage'))
set_clem_smileys_cookie(response, profile)
return response
Copy link
Contributor

Choose a reason for hiding this comment

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

     try:
-        response = redirect(next_page)
-        set_clem_smileys_cookie(response, profile)
-        return response
+        redirect_to = next_page
     except:
-        response = redirect(reverse('homepage'))
-        set_clem_smileys_cookie(response, profile)
-        return response
+        redirect_to = reverse('homepage')
+    response = redirect(redirect_to)
+    set_clem_smileys_cookie(response, profile)
+    return response

Copy link
Member

Choose a reason for hiding this comment

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

Ce code est techniquement sencé rattraper les 404, en fait, 404 qui n'apparaissent que lors du return, d'ou ce code un peu tarabiscoté.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm je suis pas expert en Python mais il me semble pas que ce soit possible. Comment est-ce que le statement return pourrait lancer une exception ?

Copy link
Member

Choose a reason for hiding this comment

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

Pas faux.

Copy link
Contributor

@vhf vhf left a comment

Choose a reason for hiding this comment

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

Beau boulot !

@pierre-24 Tu peux ignorer certains de mes messages, j'ai réalisé que vers la fin que modifier la valeur par défaut de l'option pour les membres n'était pas suffisant et qu'il faudra inverser la logique générale pour que les visiteurs (!membres) aient les "nouveaux" par défaut.

@A-312
Copy link
Contributor Author

A-312 commented Aug 12, 2017

WinXaito tout a fait, je pensais à ça aussi que clem allait rester dans le placard !

@pierre-24
Copy link
Member

Ok, faut donc que j'inverse la logique :)

@coveralls
Copy link

coveralls commented Aug 12, 2017

Coverage Status

Coverage increased (+0.1%) to 89.461% when pulling 6ed671f on A-312:feature-4404 into b6a7185 on zestedesavoir:dev.

@pierre-24
Copy link
Member

Logique inversée, encore une fois merci @A-312 :)

@coveralls
Copy link

coveralls commented Aug 13, 2017

Coverage Status

Coverage increased (+0.1%) to 89.512% when pulling c1d8900 on A-312:feature-4404 into 8662978 on zestedesavoir:dev.

@DevHugo
Copy link
Contributor

DevHugo commented Aug 17, 2017

Tout est bon ici, pour merge ? ça marche bien sur ton instance !

@vhf vhf removed the QA svp label Aug 17, 2017
@vhf
Copy link
Contributor

vhf commented Aug 17, 2017

Il vaut mieux ne pas merger avant le dernier moment parce que c'est pénible à déployer. Je préfère qu'on merge toutes les PR relatives à l'interface des publications avant, qu'on fasse une dernière preview pour s'assurer que tout est bon, puis les derniers fix nécessaires, puis on merge celle-ci et celle d'ES 5.1.1 et on lance la release.

@vhf vhf merged commit f11a134 into zestedesavoir:dev Aug 20, 2017
sandhose pushed a commit that referenced this pull request Sep 18, 2017
* Intégration des smileys clems

* Et un autre mouvement de smileys!

* Ajoute smileys dans gulp

* Génère et gère le cookie

* roses are red...

* Un peu de doc

* Test is love, test is life

* Et on oublie pas la recette !

* Et il faut tester, avant

* fix test

* logique inverse!

* Les derniers petits trucs

* Page des cookies
@A-312 A-312 deleted the feature-4404 branch June 1, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Front Concerne l'interface du site C-Infra Concerne l'infrastructure technique sous le site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants