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

correctif(notifications): ETQ usager, j'aimerais que les notifications soient fiable #9285

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

mfo
Copy link
Contributor

@mfo mfo commented Jul 7, 2023

hs: https://secure.helpscout.net/conversation/2282297626/2033291/

on a plusieurs problèmes liés aux commentaires générés automatiquement.

  1. quand les mails retry, ça crée a nouveau le commentaire
  2. il semblerait que certains message ne soit jms crée [j'imagine que le mail plante avant]

bref, l'idée c'est de découpler mail/commentaire pour eviter les probs (le contre coup c'est une requete sql de plus sur les actions d'instruction)

En explorant le code autour de ça j'ai deux questions :

  • la transition repasser_en_instruction utilise le DossierMailer qui n'a pas de logique de creation de commentaire. La logique voudrait qu'on remette le commentaire de passage en instruction ds la messagerie. Qu'en pensez-vous ?
  • la transition repasser_en_construction ne declenche aucune notif (ni mail, ni commentaire), ce qui me semble etonnant. La logique voudrait qu'on remette les deux (mail et commentaire). Qu'en pensez-vous ?

@what-the-diff
Copy link

what-the-diff bot commented Jul 7, 2023

PR Summary

  • Improved Email Format
    The change in email formatting tags from '

    ' to '

    ' in 'app/mailers/notification_mailer.rb' ensures better presentation of emails for the users. This translation into paragraph tags ensures there would be pleasing clean breaks in the email content.

  • Introduction of New Test Case
    In 'spec/mailers/notification_mailer_spec.rb', a new testing scenario called 'with failure' has been introduced. This test, when performed, anticipates a scenario in which a standard error might occur while sending out 'en construction' notifications from the 'NotificationMailer'. For such cases, the test ensures that the count of 'Commentaire' does not vary undesirably in the event of the error. This addition improves the reliability of the application by handling possible errors effectively.

@mfo mfo force-pushed the US/fix-notifications-duplicated branch 2 times, most recently from 27c5f7d to a62e260 Compare July 7, 2023 13:52
@mfo mfo changed the title correctif(notifications): ETQ usager, je ne souhaite pas recevoir autant de fois le message d'une notification qui echoue/retry WIP - correctif(notifications): ETQ usager, je ne souhaite pas recevoir autant de fois le message d'une notification qui echoue/retry Jul 7, 2023
@mfo mfo force-pushed the US/fix-notifications-duplicated branch 2 times, most recently from 5c35b86 to 699c31a Compare July 7, 2023 15:48
@mfo mfo changed the title WIP - correctif(notifications): ETQ usager, je ne souhaite pas recevoir autant de fois le message d'une notification qui echoue/retry WIP - correctif(notifications): ETQ usager, j'aimerais que les notifications soient fiable Jul 7, 2023
@mfo mfo force-pushed the US/fix-notifications-duplicated branch from 699c31a to bd65e2a Compare July 7, 2023 16:18
@colinux
Copy link
Member

colinux commented Jul 7, 2023

Pour repasser_en_construction ça me semble normal: l'instructeur a la possibilité, au choix, d'envoyer un message qui lui déclenche l'envoi de l'email. Il y a a priori des cas légitimes de ne pas envoyer d'email ou de message, d'où l'intérêt de distinguer cette action de la demande de correction. Les cas dont j'ai connaissance c'est que le dossier a été passé en instruction, mais soit par erreur, soit parce que finalement l'instructeur ne souhaite finalement pas traiter le dossier à ce moment là, il le remet en construction. Sans qu'il yait nécessairement une raison de prévenir l'usager

@mfo mfo changed the title WIP - correctif(notifications): ETQ usager, j'aimerais que les notifications soient fiable correctif(notifications): ETQ usager, j'aimerais que les notifications soient fiable Jul 11, 2023
@mfo mfo force-pushed the US/fix-notifications-duplicated branch from bd65e2a to 54b0a51 Compare July 11, 2023 09:20
@tchak
Copy link
Member

tchak commented Jul 11, 2023

Pour moi, il y a aussi le fait que le mail repasser_en_instruction devrait pouvoir être personnalisé, comme les autres. Je pense aussi qu'on devrait demander une justification pour cette opération, car elle n'est vraiment pas anodine (on revient en arrière sur une création de droits) et je trouverais ça bien de demander à l'instructeur de justifier (surtout si la décision initiale était d'accepter).

@mfo
Copy link
Contributor Author

mfo commented Jul 11, 2023

Pour repasser_en_construction ça me semble normal: l'instructeur a la possibilité, au choix, d'envoyer un message qui lui déclenche l'envoi de l'email. Il y a a priori des cas légitimes de ne pas envoyer d'email ou de message, d'où l'intérêt de distinguer cette action de la demande de correction.
Merci ! je comprends l'absence de notification par mail. Donc à rajouter ds une PR séparé : #9300

Les cas dont j'ai connaissance c'est que le dossier a été passé en instruction, mais soit par erreur, soit parce que finalement l'instructeur ne souhaite finalement pas traiter le dossier à ce moment là, il le remet en construction. Sans qu'il yait nécessairement une raison de prévenir l'usager

Pareil, direction prochaine PR : #9300

merci @colinux.

Pour moi, il y a aussi le fait que le mail repasser_en_instruction devrait pouvoir être personnalisé, comme les autres. Je pense aussi qu'on devrait demander une justification pour cette opération, car elle n'est vraiment pas anodine (on revient en arrière sur une création de droits) et je trouverais ça bien de demander à l'instructeur de justifier (surtout si la décision initiale était d'accepter).

Carrément ! juste un tout petit message de pas grand chose pour eviter la mauvaise surprise a l'usager. #9301

@mfo mfo force-pushed the US/fix-notifications-duplicated branch from 54b0a51 to e2faeb1 Compare July 11, 2023 09:39
@mfo mfo force-pushed the US/fix-notifications-duplicated branch from e2faeb1 to 1514588 Compare July 11, 2023 12:51
@mfo mfo enabled auto-merge July 11, 2023 12:51
@mfo mfo force-pushed the US/fix-notifications-duplicated branch from 1514588 to 3b3e6dd Compare July 11, 2023 13:11
Martin added 2 commits July 11, 2023 15:11
…c un espace supplémentaire plutôt que deux <br>
…r la messagerie du mailer [trop de prob possible sinon: au retry on dupliquait les commenaitres, si le mail foirait, on commentait pas etc...]
@mfo mfo force-pushed the US/fix-notifications-duplicated branch from 3b3e6dd to ce9dbed Compare July 11, 2023 13:11
@mfo mfo added this pull request to the merge queue Jul 11, 2023
Merged via the queue into demarches-simplifiees:main with commit 543eee5 Jul 11, 2023
@mfo mfo deleted the US/fix-notifications-duplicated branch July 11, 2023 13:32
@mfo mfo self-assigned this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Communiqué, ou a ne pas communiqué
Development

Successfully merging this pull request may close these issues.

3 participants