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

[WIP] Grand ménage dans les paramètres #4814

Closed
wants to merge 2 commits into
base: dev
from

Conversation

Projects
None yet
4 participants
@Situphen
Contributor

Situphen commented Dec 12, 2017

Grand ménage dans les paramètres ! Work In Progress

Je propose ce découpage :

  • Modifier mon profil (page pouvant être modérée par le staff)
    • Avatar
    • Biographie
    • Signature
    • Site web
  • Modifier mon compte
    • Pseudo
    • Mot de passe
  • Modifier mon courriel
    • Courriel
    • Checkbox avec :
      • Affichage du courriel
      • Envoi d'un courriel lors d'une réponse à un MP
  • Autres paramètres
    • Choix de la licence par défaut pour les contenus
    • Checkbox avec :
      • Changements visuels
      • Menu au survol
      • Aide Markdown
      • Affichage des signatures

Je n'ai pas touché aux pages du token GH ou des casquettes.

parametres

WIP :

  • Créer les nouveaux formulaires, vues et gabarits (en utilisant ModelForm et UpdateView)
  • Supprimer les anciens formulaires, vues et gabarits
  • Adapter le front (modifier les anciens liens)
  • Modifier les tests

QA : PAS A JOUR

  • Aucune modification du front, ni de la base de donnée donc il suffit de lancer le serveur
  • Vérifier que tous les paramètres peuvent être modifiés, sont bien gardés en mémoire et font bien effet comme attendu
  • Vérifier qu'on peut ajouter une image comme avatar depuis la galerie
  • Vérifier qu'un membre staff peut modérer le profil d'un autre membre (aller sur le profil de ce membre et cliquer sur le lien de la barre latérale)
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 12, 2017

Coverage Status

Coverage decreased (-14.9%) to 74.626% when pulling ee197dd on Situphen:refacto-settings into ca158f1 on zestedesavoir:dev.

coveralls commented Dec 12, 2017

Coverage Status

Coverage decreased (-14.9%) to 74.626% when pulling ee197dd on Situphen:refacto-settings into ca158f1 on zestedesavoir:dev.

@motet-a

C’est cool 👍

Faudrait juste bien mettre des espaces insécables et des apostrophes typographiques partout (protip : Normalement, en Python, tu n’as pas besoin d’utiliser des guillemets doubles pour les chaînes si tu met bien des apostrophes typographiques, vu que est différent de ')

{% if user.profile == profile %}
{% trans "Modifier mon profil" %}
{% else %}
{% trans "Modérer le profil de" %} {{ profile.user.username }}

This comment has been minimized.

@motet-a

motet-a Dec 13, 2017

Member
{% block content %}
{% if user.profile != profile %}
<p class="alert-box warning">
{% trans "Le profil que vous éditez n'est pas le votre, soyez prudent lors de l'édition de celui-ci !" %}

This comment has been minimized.

@motet-a

motet-a Dec 13, 2017

Member

Espaces insécables et apostrophes typographiques SVP

@motet-a

motet-a Dec 13, 2017

Member

Espaces insécables et apostrophes typographiques SVP

max_length=Profile._meta.get_field('site').max_length,
widget=forms.TextInput(
attrs={
'placeholder': _('Lien vers votre site web personnel (ne pas oublier le http:// ou https:// devant).')

This comment has been minimized.

@motet-a

motet-a Dec 13, 2017

Member

C’est un détail mais il vaut mieux privilégier HTTPS. Peut-être le mettre en premier…

@motet-a

motet-a Dec 13, 2017

Member

C’est un détail mais il vaut mieux privilégier HTTPS. Peut-être le mettre en premier…

@Situphen

This comment has been minimized.

Show comment
Hide comment
@Situphen

Situphen Dec 13, 2017

Contributor

Merci pour tes remarques !

Pour l'instant j'ai seulement découpé les paramètres d'une autre manière sans rien changer niveau front mais je ferais probablement un commit pour changer quelques points à ce niveau là (notamment les histoires de guillemets typographiques) :)

Contributor

Situphen commented Dec 13, 2017

Merci pour tes remarques !

Pour l'instant j'ai seulement découpé les paramètres d'une autre manière sans rien changer niveau front mais je ferais probablement un commit pour changer quelques points à ce niveau là (notamment les histoires de guillemets typographiques) :)

@motet-a

This comment has been minimized.

Show comment
Hide comment
@motet-a

motet-a Dec 14, 2017

Member

Ah okay. Vu les stats du diff j'ai crû que tu avais écrit tout ça toi même.

Member

motet-a commented Dec 14, 2017

Ah okay. Vu les stats du diff j'ai crû que tu avais écrit tout ça toi même.

@motet-a

This comment has been minimized.

Show comment
Hide comment
@motet-a

motet-a Dec 14, 2017

Member

Par contre ça risque d'être embêtant à merger avec #4725 : https://mm.3.141.ovh/merge/ee197dda90854c9e/5389ce6b58d8f687

Member

motet-a commented Dec 14, 2017

Par contre ça risque d'être embêtant à merger avec #4725 : https://mm.3.141.ovh/merge/ee197dda90854c9e/5389ce6b58d8f687

@Situphen

This comment has been minimized.

Show comment
Hide comment
@Situphen

Situphen Dec 14, 2017

Contributor

Oh j'avais plus en mémoire cette PR :D Je pense que les deux sont complémentaires. @gcodeur je propose qu'on ne garde qu'une des deux PRs et qu'on reporte les modifications de l'autre à la main sur celle que l'on garde. Je pense que ça évitera un rebase très douloureux :D

Contributor

Situphen commented Dec 14, 2017

Oh j'avais plus en mémoire cette PR :D Je pense que les deux sont complémentaires. @gcodeur je propose qu'on ne garde qu'une des deux PRs et qu'on reporte les modifications de l'autre à la main sur celle que l'on garde. Je pense que ça évitera un rebase très douloureux :D

@gcodeur

This comment has been minimized.

Show comment
Hide comment
@gcodeur

gcodeur Dec 14, 2017

Member

C'est ma faute, j'ai pris beaucoup de retard sur cette PR.

Le but de ma PR est principalement de revoir les vues existantes en utilisant des fonctionnalités pratiques de Django (les ModelForm, UpdateView, etc). J'ai aussi quelques autres modifications mineures (fix de #4545 surtout).

Du coup @Situphen, vu que tu réécris à peu près toutes les vues, je propose que tu utilises ce que j'ai fait pour adapter ton travail à l'utilisation de ces fonctionnalités et qu'on ferme ma PR. Je pense que c'est de loin la solution la plus simple. Si tu as besoin d'aide, n'hésite pas.

Member

gcodeur commented Dec 14, 2017

C'est ma faute, j'ai pris beaucoup de retard sur cette PR.

Le but de ma PR est principalement de revoir les vues existantes en utilisant des fonctionnalités pratiques de Django (les ModelForm, UpdateView, etc). J'ai aussi quelques autres modifications mineures (fix de #4545 surtout).

Du coup @Situphen, vu que tu réécris à peu près toutes les vues, je propose que tu utilises ce que j'ai fait pour adapter ton travail à l'utilisation de ces fonctionnalités et qu'on ferme ma PR. Je pense que c'est de loin la solution la plus simple. Si tu as besoin d'aide, n'hésite pas.

@Situphen

This comment has been minimized.

Show comment
Hide comment
@Situphen

Situphen Dec 14, 2017

Contributor

Ça me va !

Contributor

Situphen commented Dec 14, 2017

Ça me va !

@gcodeur

👍

Quelques suggestions.

<h3>{% trans "Nouvelle navigation" %}</h3>
<ul>
<li><a href="{% url "update-main-settings" %}">{% trans "Paramètres généraux" %}</a></li>

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

C'est un détail, mais je verrais mieux les "Paramètres généraux" à la fin vu que c'est une catégorie fourre-tout).

@gcodeur

gcodeur Dec 14, 2017

Member

C'est un détail, mais je verrais mieux les "Paramètres généraux" à la fin vu que c'est une catégorie fourre-tout).

This comment has been minimized.

@Situphen

Situphen Dec 14, 2017

Contributor

Je vais changer ça. Il faudrait réfléchir à la page sur laquelle on arrive quand on clique sur "Paramètres" dans le dropdown en haut à droite, car pour le profil ça sera les paramètres du profil mais celui là je ne vois pas trop :/

@Situphen

Situphen Dec 14, 2017

Contributor

Je vais changer ça. Il faudrait réfléchir à la page sur laquelle on arrive quand on clique sur "Paramètres" dans le dropdown en haut à droite, car pour le profil ça sera les paramètres du profil mais celui là je ne vois pas trop :/

<ul>
<li><a href="{% url "update-main-settings" %}">{% trans "Paramètres généraux" %}</a></li>
<li><a href="{% url "update-profile-settings" %}">{% trans "Modifier mon profil" %}</a></li>
<li><a href="{% url "update-account-settings" %}">{% trans "Modifier mon compte" %}</a></li>

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

"mes identifiants" ?

@gcodeur

gcodeur Dec 14, 2017

Member

"mes identifiants" ?

<li><a href="{% url "update-main-settings" %}">{% trans "Paramètres généraux" %}</a></li>
<li><a href="{% url "update-profile-settings" %}">{% trans "Modifier mon profil" %}</a></li>
<li><a href="{% url "update-account-settings" %}">{% trans "Modifier mon compte" %}</a></li>
<li><a href="{% url "update-email-settings" %}">{% trans "Modifier mon courriel" %}</a></li>

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

Je n'aime pas trop ça vu qu'un courriel est le nom donné au message et pas vraiment celui de l'adresse. Je propose quelque chose comme "Mon adresse e-mail".

@gcodeur

gcodeur Dec 14, 2017

Member

Je n'aime pas trop ça vu qu'un courriel est le nom donné au message et pas vraiment celui de l'adresse. Je propose quelque chose comme "Mon adresse e-mail".

This comment has been minimized.

@Situphen

Situphen Dec 14, 2017

Contributor

J'aime pas le mot "e-mail" :D (mais c'est subjectif) Plus sérieusement tu as raison, il faudrait choisir entre :

  • "Modifier mon adresse email"
  • "Modifier mon adresse de courriel"
  • "Modifier mes paramètres email"
  • "Modifier mes paramètres de courriel"

Je suis pour "paramètres" car même si c'est long ça montre bien que c'est pas seulement le courriel en soit mais aussi les paramètres d'envoi.

Je te laisse choisir ce que tu penses être le mieux :)

@Situphen

Situphen Dec 14, 2017

Contributor

J'aime pas le mot "e-mail" :D (mais c'est subjectif) Plus sérieusement tu as raison, il faudrait choisir entre :

  • "Modifier mon adresse email"
  • "Modifier mon adresse de courriel"
  • "Modifier mes paramètres email"
  • "Modifier mes paramètres de courriel"

Je suis pour "paramètres" car même si c'est long ça montre bien que c'est pas seulement le courriel en soit mais aussi les paramètres d'envoi.

Je te laisse choisir ce que tu penses être le mieux :)

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

J'aime bien aussi la dernière suggestion, mais j'ai peur que sur les petits écrans, on se retrouve avec quelque chose comme "Modifier mes paramètres..." et qu'on ne sache plus du tout de quoi il s'agit.

Pour courriel/e-mail, va pour courriel, je trouve aussi ça plus élégant. Mais dans ce cas, change-le aussi dans le label du champ de formulaire par souci de cohérence. ;)

@gcodeur

gcodeur Dec 14, 2017

Member

J'aime bien aussi la dernière suggestion, mais j'ai peur que sur les petits écrans, on se retrouve avec quelque chose comme "Modifier mes paramètres..." et qu'on ne sache plus du tout de quoi il s'agit.

Pour courriel/e-mail, va pour courriel, je trouve aussi ça plus élégant. Mais dans ce cas, change-le aussi dans le label du champ de formulaire par souci de cohérence. ;)

This comment has been minimized.

@Situphen

Situphen Dec 14, 2017

Contributor

En effet c'est assez long. Je propose quelque chose comme <a><span class="wide">Modifier mes</span>paramètres de courriel</a> pour éviter le problème.

Évidemment :)

@Situphen

Situphen Dec 14, 2017

Contributor

En effet c'est assez long. Je propose quelque chose comme <a><span class="wide">Modifier mes</span>paramètres de courriel</a> pour éviter le problème.

Évidemment :)

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

J'y ai pensé, mais on va avoir un problème de majuscule du coup.

Ou alors, on peut envisager une règle CSS qui met toujours une majuscule au début dans ces menus, il ne devrait pas y avoir de cas où elle n'est pas utile.

@gcodeur

gcodeur Dec 14, 2017

Member

J'y ai pensé, mais on va avoir un problème de majuscule du coup.

Ou alors, on peut envisager une règle CSS qui met toujours une majuscule au début dans ces menus, il ne devrait pas y avoir de cas où elle n'est pas utile.

This comment has been minimized.

@motet-a

motet-a Dec 14, 2017

Member

Moi j’enlèverai complètement le “Modifier” dans ce cas, je mettrais simplement “Paramètres de courriel” ou quelque chose de court dans le genre.

Concernant les class="wide", je n’utiliserait pas ce hack dans ce cas personellement.

@motet-a

motet-a Dec 14, 2017

Member

Moi j’enlèverai complètement le “Modifier” dans ce cas, je mettrais simplement “Paramètres de courriel” ou quelque chose de court dans le genre.

Concernant les class="wide", je n’utiliserait pas ce hack dans ce cas personellement.

{% if user.profile.is_dev %}
<li><a href="{% url "update-github" %}">{% trans "Gérer mon token GitHub" %}</a></li>
{% endif %}
<li><a href="{% url "member-warning-unregister" %}">{% trans "Se désinscrire" %}</a></li>

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

"Me désinscrire" vu qu'on a mis la première personne partout ?

@gcodeur

gcodeur Dec 14, 2017

Member

"Me désinscrire" vu qu'on a mis la première personne partout ?

{% block title %}
{% trans "Modifier mon compte" %}

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

(Note : si on fait la modification pour "mes identifiants", il faudra la reporter dans ce fichier.)

@gcodeur

gcodeur Dec 14, 2017

Member

(Note : si on fait la modification pour "mes identifiants", il faudra la reporter dans ce fichier.)

{% block title %}
{% trans "Modifier mon courriel" %}

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

(Note : si on fait la modification pour "Mon adresse e-mail", il faudra la reporter dans ce fichier.)

@gcodeur

gcodeur Dec 14, 2017

Member

(Note : si on fait la modification pour "Mon adresse e-mail", il faudra la reporter dans ce fichier.)

@@ -144,6 +144,303 @@ def throw_error(self, key=None, message=None):
self._errors[key] = self.error_class([message])
class MainSettingsForm(forms.Form):

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

Du coup, il faudra utiliser des ModelForm, ça permettra d'éviter pas mal de duplication de code. :)

@gcodeur

gcodeur Dec 14, 2017

Member

Du coup, il faudra utiliser des ModelForm, ça permettra d'éviter pas mal de duplication de code. :)

)
password_new = forms.CharField(
label=_('Nouveau mot de passe'),

This comment has been minimized.

@gcodeur

gcodeur Dec 14, 2017

Member

Est-il possible de modifier son pseudo sans changer le mot de passe ? Si oui, je propose un "facultatif" entre parenthèses.

@gcodeur

gcodeur Dec 14, 2017

Member

Est-il possible de modifier son pseudo sans changer le mot de passe ? Si oui, je propose un "facultatif" entre parenthèses.

This comment has been minimized.

@Situphen

Situphen Dec 14, 2017

Contributor

Oui ça l'est ! Tu as raison :) De toute façon là je modifie le découpage des pages et la gestion du back mais je ferai probablement quelques modifications du front pour rendre ça + clair

@Situphen

Situphen Dec 14, 2017

Contributor

Oui ça l'est ! Tu as raison :) De toute façon là je modifie le découpage des pages et la gestion du back mais je ferai probablement quelques modifications du front pour rendre ça + clair

@Situphen Situphen closed this Aug 11, 2018

@Situphen Situphen deleted the Situphen:refacto-settings branch Aug 11, 2018

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