-
Notifications
You must be signed in to change notification settings - Fork 0
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
[QA] Call to a member function getCodeSuivi() on null / fix duplicate email notif #2304
[QA] Call to a member function getCodeSuivi() on null / fix duplicate email notif #2304
Conversation
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.
Tests et relecture OK
@@ -38,7 +38,9 @@ public function validationResponseSignalement( | |||
$statut = Signalement::STATUS_ACTIVE; | |||
$description = 'validé'; | |||
$signalement->setValidatedAt(new DateTimeImmutable()); | |||
$signalement->setCodeSuivi(md5(uniqid())); | |||
if (!$signalement->getCodeSuivi()) { |
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.
Si on ne le change plus, ça vaut peut-être le coup de le complexifier un peu, non ? sur le nombre de signalement :)
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.
y'a d'autres endroits où le setCodeSuivi
est fait dans tous les cas ; est-ce qu'on peut mettre la condition dans la fonction setCodeSuivi
, directement ?
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.
Le seul endroit ou la vérification n'est pas faite c'est à la création du signalement (nouveau et ancien formulaire mais l'ancien va sauter sous peu) l'idée est bonne mais ca veux dire qu'on pérennise l'idée que cela ne devra jamais changer au cours de la vie d'un signalement ? dans ce cas j'irais même plus loin et je mettrais la génération du code suivi dans le constructeur (comme pour l'uuid)
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.
vos avis @hmeneuvrier @sfinx13
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.
oui dans le constructeur
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.
Ok c'est fait dans le constructeur via uuid
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.
Quelques retours. Pas encore testé.
@@ -470,7 +469,7 @@ public function suiviProcedure( | |||
|
|||
return $this->redirectToRoute('front_suivi_signalement'); | |||
} | |||
$this->addFlash('error', 'Le lien utilisé est expiré ou invalide, verifier votre saisie.'); | |||
$this->addFlash('error', 'Le lien utilisé est invalide, vérifier votre saisie.'); |
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.
j'avais proposé vérifiez
(ez à la fin)
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.
Oups, j'ai cru que tu relevais uniquement l'absence d'accent, c'est corrigé.
@@ -1293,7 +1294,7 @@ public function getCodeSuivi(): ?string | |||
return $this->codeSuivi; | |||
} | |||
|
|||
public function setCodeSuivi(?string $codeSuivi): self | |||
public function setCodeSuivi(string $codeSuivi): self |
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.
ça risque pas de poser souci pour les existants qui sont déjà NULL
?
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.
Bien vu merci, j'ai ajouté une migration
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.
Retours typo et une question :)
|
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.
OK lecture et test !
Ticket
#1324
Description
Tests