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

ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur #9425

Merged
merged 9 commits into from
Sep 11, 2023

Conversation

krichtof
Copy link
Contributor

@krichtof krichtof commented Aug 29, 2023

close #9356

Ce que permet cette PR

  • l'instructeur d'une procédure routée peut ajouter, modifier ou supprimer un service spécifique au groupe instructeur auquel il appartient

  • lorsqu'un administrateur clone une procédure routée qui lui appartient, il clone également les groupes instructeurs et leur service spécifique

  • l'usager voit dans le pied de page les informations de contact du groupe instructeur qui gère son dossier. Si le service spécifique au groupe instructeur ne contient pas de téléphone, c'est le téléphone du service associé à la procédure qui est affiché à l'usager.

  • désormais, le téléphone du service et ses horaires d'ouverture sont tout le temps affichés (même si le dossier n'est plus en brouillon)

Impressions écran

L'instructeur peut ajouter ou modifier un service spécifique à un groupe instructeur depuis la page du groupe instructeur.

Capture d’écran 2023-08-29 à 17 32 02 Capture d’écran 2023-08-29 à 17 35 32 Capture d’écran 2023-08-29 à 17 36 21

@krichtof krichtof force-pushed the 9356-service-gi branch 2 times, most recently from 366d550 to 8aab038 Compare August 30, 2023 11:24
@emsnytech
Copy link
Contributor

est ce que le numéro SIRET est obligatoire ? cela poserait des pb car dans les cas où la notion de service traitant ne s'attache pas forcément au groupe d'instructeur... cela risque tout simplement d'être ... bloquant... a partager avec des exemples concrets... ensuite, au début en effet... on est pas obligé de remplir ... wait and see

@krichtof
Copy link
Contributor Author

krichtof commented Aug 31, 2023

@emsnytech Actuellement, j'ai mis le siret obligatoire mais ça peut être changé.

cela poserait des pb car dans les cas où la notion de service traitant ne s'attache pas forcément au groupe d'instructeur.

Pas sûr de comprendre. Cette PR attache au groupe instructeur un service.
Est-ce qu'il faudrait se limiter pour les groupes instructeurs aux informations de contact ? (nom, adresse, tel, horaires)
Et demander un siret obligatoire uniquement au niveau du service associé à la procédure ?

Ou c'est bien un service avec un nom, un organisme, un type d'organisme, un siret et des infos de contact. Et la seule différence avec le service associé à une procédure, c'est que le siret n'est pas obligatoire ?

@krichtof krichtof marked this pull request as ready for review September 4, 2023 10:32
@E-L-T
Copy link
Contributor

E-L-T commented Sep 5, 2023

Très chouette cette feature 👍 Juste un petit retour : si je suis connecté etq admin, je crée un groupe_service -> je me retrouve connecté etq instructeur.
Et une question pas liée à cette PR : si je clone une démarche dont je suis l'admin, est-ce qu'il y a une raison pour laquelle on ne clone pas aussi le service de la procédure initiale ?

@krichtof
Copy link
Contributor Author

krichtof commented Sep 5, 2023

@E-L-T

si je suis connecté etq admin, je crée un groupe_service -> je me retrouve connecté etq instructeur.

Hmm, effectivement, je regarde si je peux corriger cela.

si je clone une démarche dont je suis l'admin, est-ce qu'il y a une raison pour laquelle on ne clone pas aussi le service de la procédure initiale ?

  • Dans la PR en l'état, si je clone une démarche dont je suis admin, je clone les groupe instructeur services.
  • pour ce qui est du service de la procédure initiale (pas couvert dans cette PR), j'avoue que je ne sais pas. Et que je serai d'avis comme toi a priori à le cloner.

Mais en fouillant un peu, j'ai trouvé ce commit de @mfo : 6e31dec

Il semblerait que cela apporte de la confusion. Du coup, peut-être ne pas cloner non plus les services associés aux groupe instructeurs ? @mfo un avis ?

Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

Comme d'hab, PR très sympa à relire grâce a pitis commits 🤗 .

Mise à part le coup de la redirection, mentionné par Eric, en instructeur alors que l'on rajoute des informations de contacte etq administrateur et qui risque d etre un poil compliqué à fixer, j'ai que des retours mineurs.

}

validates :nom, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :nom, uniqueness: { scope: :groupe_instructeur, message: 'existe déjà' }
Copy link
Member

Choose a reason for hiding this comment

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

bien joué le scope !

validates :organisme, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :siret, siret_format: true
validates :type_organisme, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :email, presence: { message: 'doit être renseigné' }, allow_nil: false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
validates :email, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :email, format: { with: Devise.email_regexp }, presence: { message: 'doit être renseigné' }, allow_nil: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai rebasé. Intégré dans ce commit : c49c994

Comment on lines 5 to 13
t.text :adresse
t.string :email
t.jsonb :etablissement_infos, default: {}
t.decimal :etablissement_lat, precision: 10, scale: 6
t.decimal :etablissement_lng, precision: 10, scale: 6
t.text :horaires
t.string :nom, null: false
t.string :organisme
t.string :siret
Copy link
Member

Choose a reason for hiding this comment

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

Pour être raccord avec les contraintes coté model

Suggested change
t.text :adresse
t.string :email
t.jsonb :etablissement_infos, default: {}
t.decimal :etablissement_lat, precision: 10, scale: 6
t.decimal :etablissement_lng, precision: 10, scale: 6
t.text :horaires
t.string :nom, null: false
t.string :organisme
t.string :siret
t.text :adresse, null: false
t.string :email, null: false
t.jsonb :etablissement_infos, default: {}
t.decimal :etablissement_lat, precision: 10, scale: 6
t.decimal :etablissement_lng, precision: 10, scale: 6
t.text :horaires, , null: false
t.string :nom, null: false
t.string :organisme, null: false
t.string :siret, null: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. J'ai rebasé. Intégré dans ce commit : c49c994 (et avec la modif du db/schema.rb)

Comment on lines 1 to 2
require 'rails_helper'

Copy link
Member

Choose a reason for hiding this comment

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

je crois que tu peux le faire sauter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Effectivement :)
J'ai rebasé. Intégré dans ce commit : c49c994

private

def assign_procedure_and_groupe_instructeur
@procedure = Procedure.find params[:procedure_id]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@procedure = Procedure.find params[:procedure_id]
@procedure = current_instructeur.procedures.find params[:procedure_id]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai rebasé. Suggestion intégrée dans ce commit : f9453b5

class APIEntreprise::GroupeInstructeurServiceJob < APIEntreprise::ServiceJob
private

def find_service(service_id)
Copy link
Member

Choose a reason for hiding this comment

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

si tu peux rajouter un ptit commentaire genre #override find_service from ServiceJob to work with groupe_instructeur_service ca aiderait le prochain moi, car j'ai mis pas mal de temps à comprendre 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui, complètement d'accord !
J'ai rebasé. Suggestion intégrée dans 4eeac56 (et j'en ai profité pour modifier également les paramètres en groupe_service_id)

.card-title Informations de contact
- service = groupe_instructeur.groupe_instructeur_service
- if service.nil?
= "Le groupe #{groupe_instructeur.label} n'a pas de service associé. Les informations de contact affichées à l'usager seront celles du service de la procédure"
Copy link
Member

Choose a reason for hiding this comment

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

Peut être ?

Suggested change
= "Le groupe #{groupe_instructeur.label} n'a pas de service associé. Les informations de contact affichées à l'usager seront celles du service de la procédure"
= "Le groupe #{groupe_instructeur.label} n'a pas d'informations de contact. L’usager ne pourra contacter que le service national."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'attends le retour de Philippe sur la notion de service ou d'infos de contact.
Sur "service national", ce n'est pas forcément le cas. On peut avoir une procédure locale, avec plusieurs groupe instructeurs.

= "Le groupe #{groupe_instructeur.label} n'a pas de service associé. Les informations de contact affichées à l'usager seront celles du service de la procédure"
%p.mt-3
- if groupe_instructeur.instructeurs.include?(current_administrateur.user.instructeur)
= link_to "+ Nouveau service", new_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"
Copy link
Member

Choose a reason for hiding this comment

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

Peut etre que le terme service peut être remplacé ici par contact (pas sur).

Suggested change
= link_to "+ Nouveau service", new_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"
= link_to "Ajouter des informations de contact", new_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Même commentaire que precedemment sur service ou info de contact

- else
%p.mt-3
- if groupe_instructeur.instructeurs.include?(current_administrateur.user.instructeur)
= link_to "Modifier le service #{service.nom}", edit_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"
Copy link
Member

Choose a reason for hiding this comment

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

subjectif: plus simple (y'a pas d'ambiguité ici)

Suggested change
= link_to "Modifier le service #{service.nom}", edit_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"
= link_to "Modifier", edit_instructeur_groupe_groupe_instructeur_service_path(procedure_id: procedure.id, groupe_id: groupe_instructeur.id), class: "fr-btn"

@krichtof krichtof force-pushed the 9356-service-gi branch 2 times, most recently from ac02d5b to 2c60f2f Compare September 6, 2023 07:12
@krichtof
Copy link
Contributor Author

krichtof commented Sep 6, 2023

@E-L-T @LeSim avec ce nouveau commit, un admin, une fois qu'il a créé ou édité un service associé à un groupe instructeur, est bien redirigé vers la page groupe instructeur de la procédure en tant qu'admin : 3f6c23a

@krichtof krichtof force-pushed the 9356-service-gi branch 2 times, most recently from 3f6c23a to 8c5465e Compare September 6, 2023 12:02
@krichtof
Copy link
Contributor Author

krichtof commented Sep 6, 2023

@E-L-T J'ai fait la modif pour que le téléphone du service de la procédure ne soit plus affiché par défaut lorsqu'on crée un nouveau service associé à un groupe instructeur.
Dans ce commit : 098a87c

@krichtof krichtof changed the title ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur NE PAS MERGER // ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur Sep 6, 2023
Comment on lines 41 to 34
= link_to 'Supprimer',
instructeur_groupe_groupe_instructeur_service_path(procedure_id: @procedure.id, groupe_id: @groupe_instructeur.id),
method: :delete,
data: { confirm: "Confirmez vous la suppression de ce service ?" },
class: 'fr-btn fr-btn--secondary'
Copy link
Contributor

@E-L-T E-L-T Sep 6, 2023

Choose a reason for hiding this comment

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

Comme le new appelle cette partial, on a le bouton "Supprimer" dans le formulaire new (et on a une 500 si on clique dessus)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bien vu ! J'ai corrigé. Voir commentaire suivant :)

@krichtof krichtof force-pushed the 9356-service-gi branch 3 times, most recently from 4a9b66c to 1a65163 Compare September 7, 2023 07:34
@krichtof
Copy link
Contributor Author

krichtof commented Sep 7, 2023

@LeSim @E-L-T

Nouvelles modifs (et peut-être dernieres 😊 ) suite au dernier rebase :

  • je n'ai gardé que les informations de contact (supprimé le siret, l'organisme, le type d'organisme)
  • j'ai supprimé le job api_entreprise (et le commit associé) qui récupérait les données d'établissement à partir du siret
  • dans le wording, je ne parle plus de service mais d'informations de contact
  • le bouton de suppression n'est visible que pour les actions edit et update (80371f2)
Capture d’écran 2023-09-07 à 09 42 01

Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

tip top !


= render Dsfr::InputComponent.new(form: f, attribute: :nom, input_type: :text_field) do |c|
- c.with_hint do
= "Indiquez le nom à utiliser pour contacter le groupe instructeur"
Copy link
Member

Choose a reason for hiding this comment

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

Je me demande si le nom est encore pertinent, je ne verrai pas quoi remplir de dedans. Un exemple m'aiderait.

validates :nom, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :nom, uniqueness: { scope: :groupe_instructeur, message: 'existe déjà' }
validates :email, format: { with: Devise.email_regexp, message: "n'est pas valide" }, presence: { message: 'doit être renseigné' }, allow_nil: false
validates :telephone, phone: { possible: true, allow_blank: true }
Copy link
Member

Choose a reason for hiding this comment

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

Je me dis, que maintenant, si tu veux faire des informations de contacts avancées, soit tu remplis tout, soit tu remplis rien.

tu peux pas surcharger les horaires sans modifier le téléphone, sinon un groupe local pourrait modifier les horaires d'ouverture du standard nationale

Suggested change
validates :telephone, phone: { possible: true, allow_blank: true }
validates :telephone, phone: { possible: true, allow_blank: false }
  • la modif sur la migration


= render Dsfr::CalloutComponent.new(title: "Informations de contact") do |c|
- c.body do
Votre démarche sera hébergée par #{APPLICATION_NAME} – mais nous ne pouvons pas assurer le support des démarches. Et malgré la dématérialisation, les usagers se poseront parfois des questions légitimes sur le processus administratif.
Copy link
Member

Choose a reason for hiding this comment

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

subjectif: simplification

Suggested change
Votre démarche sera hébergée par #{APPLICATION_NAME} – mais nous ne pouvons pas assurer le support des démarches. Et malgré la dématérialisation, les usagers se poseront parfois des questions légitimes sur le processus administratif.
Votre démarche est hébergée par #{APPLICATION_NAME} – mais nous n’assurons pas le support des démarches. Malgré la dématérialisation, les usagers se posent des questions légitimes sur le processus administratif.

par le moyen de leur choix s’ils ont des questions sur votre démarche.
%br
%br
Ces informations de contact seront visibles par les utilisateurs de la démarche, affichées dans le menu « Aide », ainsi qu’en pied de page lors du dépôt d’un dossier. En cas d’informations invalides, #{APPLICATION_NAME} se réserve le droit de suspendre la publication de la démarche.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ces informations de contact seront visibles par les utilisateurs de la démarche, affichées dans le menu « Aide », ainsi qu’en pied de page lors du dépôt d’un dossier. En cas d’informations invalides, #{APPLICATION_NAME} se réserve le droit de suspendre la publication de la démarche.
Ces informations de contact seront visibles par les utilisateurs de la démarche, affichées dans le menu « Aide », ainsi qu’en pied de page lors du dépôt d’un dossier.
%br
%br
⚠️ En cas d’informations invalides, #{APPLICATION_NAME} se réserve le droit de suspendre la publication de la démarche.

@@ -21,6 +21,7 @@
= I18n.t('users.procedure_footer.contact.email.link')
= link_to service.email, "mailto:#{service.email}", class: "fr-footer__top-link"

- if service.present?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- if service.present?

il me semble que tu peux supprimer cette ligne, car au pire on prend le service de la procédure (ligne 2)

Comment on lines 45 to 47
redirect_to admin_procedure_groupe_instructeur_path(@groupe_instructeur, procedure_id: @procedure.id), notice: notice
else
redirect_to instructeur_groupe_path(@groupe_instructeur, procedure_id: @procedure.id), notice: notice
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
redirect_to admin_procedure_groupe_instructeur_path(@groupe_instructeur, procedure_id: @procedure.id), notice: notice
else
redirect_to instructeur_groupe_path(@groupe_instructeur, procedure_id: @procedure.id), notice: notice
redirect_to admin_procedure_groupe_instructeur_path(@procedure, @groupe_instructeur), notice: notice
else
redirect_to instructeur_groupe_path(@procedure, @groupe_instructeur), notice: notice

= render SimpleFormatComponent.new(service.adresse, class_names_map: {paragraph: 'fr-footer__content-desc'})
= service.email
- if service.telephone.present?
%p= service.telephone
Copy link
Member

Choose a reason for hiding this comment

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

En regardant la doc de notre librairie de parsage de tel, j'ai vu qu'il proposait de jolis formatages

nationaux : Phonelib.parse(' 0 47 2561 47 3 ').national => "04 72 56 14 73"

internationaux: Phonelib.parse(' 0 4 72 561 473 ').e164 => "+33472561473" , intéressant pour les urls car ca marcherait avec de tel étranger

Du coup, je me dis qu'on pourrait faire un concern utilisé dans Service et dans GroupeInstructeurService qui proposerait def telephone_display et def telephone_url et modifier tous nos affichages pour avoir un super rendu.

Je suis conscient que c'est pas trop le sujet de cette pr, mais je me dis que c'est l'occasion.

T'en penses quoi ?

@@ -0,0 +1,21 @@
class GroupeInstructeurService < ApplicationRecord
Copy link
Member

Choose a reason for hiding this comment

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

Du coup

Suggested change
class GroupeInstructeurService < ApplicationRecord
class ContactInformation < ApplicationRecord

Je sais que c'est relou, mais j'ai l'impression que ça éviterait des confusions lorsqu'on parlera de service

@@ -16,7 +16,7 @@
= "Indiquez le nom à utiliser pour contacter le groupe instructeur"

= render Dsfr::InputComponent.new(form: f, attribute: :email, input_type: :email_field)
= render Dsfr::InputComponent.new(form: f, attribute: :telephone, input_type: :telephone_field)
= render Dsfr::InputComponent.new(form: f, attribute: :telephone, input_type: :telephone_field, required: false)
Copy link
Member

Choose a reason for hiding this comment

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

si t'es d'accord avec le telephone obligatoire, il faut le mettre aussi ici

end

private

def redirect_to_groupe_instructeur(notice)
Copy link
Member

Choose a reason for hiding this comment

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

super nice !

after creating or editing groupe instructeur service
@krichtof krichtof changed the title NE PAS MERGER // ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur Sep 11, 2023
@krichtof krichtof added this pull request to the merge queue Sep 11, 2023
Merged via the queue into main with commit 5aabce4 Sep 11, 2023
@krichtof krichtof deleted the 9356-service-gi branch September 11, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETQ Usager, je veux voir dans mon dossier les informations de contact de mon groupe instructeur
4 participants