-
Notifications
You must be signed in to change notification settings - Fork 3
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
Page de contacts : tri et groupement par département #4750
Conversation
868f069
to
83d3130
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 merci beaucoup Antoine et félicitations pour cette PR du permier jour 👏
C’est super d’avoir pris des initiatives et proposé d’aller plus loin que ce que décrivait le ticket.
Cette page a clairement beaucoup de marge d’amélioration.
Je suis complètement aligné avec tes propositions 👍
Pour info on envisage avec Nesserine de faire évoluer cette page sur deux points principaux :
- afficher les numéros de contact des organisations plutôt que des territoires
- améliorer le bloc de contact de l’équipe technique pour clarifier le message sur notre rôle et le mettre plus en avant.
cf ce ticket notion et ce ticket github
J’ai fait des remarques absolument non-bloquantes, je suis pour déployer cette PR 👍
Donnée de prod
Je suis allé exécuter en prod Territory.where.not(phone_number_formatted: nil).pluck(:departement_number).uniq.compact_blank.sort
Sur RDV SP : ["40", "59", "60", "62", "68", "76", "84", "89", "92", "974"]
tout va bien
Sur RDV Solidarités : ["01", "03", "08", "13", "22", "42", "55", "64", "678", "71", "76", "77", "78", "80", "84", "92", "C30"]
:
C30
c’est le territoire « CDAD du Gard »678
c’est le CDAD de la Haute-Garonne
Peut-être que @victormours ou @francois-ferrandis auront plus d’informations sur ces deux territoires aux numéros de départements surprenants ?
Je comprends qu’il y ait des territoires qui ne représentent pas des départements mais je ne comprends pas pourquoi ces territoires ont un departement_number
volontairement « faux ».
span> Autres départements, voir | ||
= link_to "l'annuaire des services publics", "https://lannuaire.service-public.fr/" | ||
- Territory.where.not(phone_number_formatted: nil).order(:departement_number, :name).group_by(&:departement_number).each do |department_number, territories| | ||
h2 #{department_number} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La syntaxe h2= departement_number
semble utilisée plus fréquemment dans RDVSP.
Je n’ai pas vraiment de préférence dans un sens ou dans l’autre, et c’est tout à fait OK d’avoir les deux dans notre codebase. Je me demandais simplement si c’était un choix ou une habitude @AntoineGirard ?
Et en deuxième remarque plus sur le fond j’aurais bien précédé ce numéro d’un « Département » pour clarifier et donner un peu de cohérence visuelle à la page.
h2 #{department_number} | |
h2 Département #{department_number} |
(en écrivant cette suggestion de changement je me rends compte que la syntaxe que tu as utilisé Antoine est plus facile à faire évoluer, il y a moins de changements à faire lorsqu’on veut introduire du texte et interpoler les variables)
Ce n’est absolument pas une suggestion bloquante pour déployer cette PR, c’est déjà beaucoup mieux que ce qu’il y a actuellement. Et en plus il semble y avoir des territoires avec des departement_number
qui ne sont pas « réels » en prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah aussi, ta solution génère un h2 vide pour les departement_number
nil ou blank
ce n’est pas très problématique mais on peut peut-être faire un peu mieux, par exemple afficher « Autres » ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
en bonus, c'est peut-être possible d'utiliser lib/assets/departements_fr.csv
pour avoir le nom du département en toutes lettres
span> #{territory.name} | ||
= link_to territory.phone_number, "tel:#{territory.phone_number_formatted}" | ||
|
||
h2 Autres contacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 très cool de génériser ce titre
64877cb
to
f4ce938
Compare
Vu avec @victormours ce matin, on préfère retirer l’usage des scopes pour ne pas alourdir le model pour un scope utilisé une seule fois. |
f4ce938
to
2da0352
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Trop bien ! Merci pour le petit refacto.
J'ai mis une petite suggestion pour un scope, mais c'est complètement facultatif.
Bravo pour ta première PR métier sur l'appli !
|
||
territories_without_department = territories_with_phone_number | ||
.where.not(departement_number: Territory::DEPARTEMENTS_NAMES.keys) | ||
.order(:name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu peux utiliser le scope ordered_by_name
pour gérer les accents et les majuscules
561b5e9
to
4be3198
Compare
Closes #4744
Review app : https://demo-rdv-solidarites-pr4750.osc-secnum-fr1.scalingo.io/contact (pas forcément nécessaire ici, mais c’était pour tester le fonctionnement des reviews app pour ma première PR).
Contexte
La page de contact avait plusieurs petits soucis :
Solution
phone_number_formatted
non nul plutôt que d’utiliser lephone_number
qui peut être empty.departement_number
puis parname
departement_number
Checklist
Captures d'écran