feat(request): update a request #93
Conversation
|
| plant_attributes_from_params(@request) | ||
| if @request.update(request_params) |
There was a problem hiding this comment.
En mettant les instructions dans cet ordre, on peut s'exposer à un petit souci de cohérence.
La première méthode appelée, plant_attributes_from_params, regarde si un ID est passé en paramètre, et assigne les champs plant_stage_name et plant_name si c'est le cas.
Puis, la méthode update assigne tous les paramètres passés par l'utilisateur ; dont les fameux plant_stage_name et plant_name.
Ce qui veut dire que dans l'idée, la méthode plant_attributes_from_params peut ici être overridée par les paramètres passés par le front, et donc créer des valeurs "illogiques" (par exemple, si passe l'id d'une rose mais je mets en dur que le nom est une tulipe ; et ça sera enregistré tel quel dans la bdd).
En inversant l'ordre d'appel des méthodes, on évite cette potentielle "faille" :)
| plant_attributes_from_params(@request) | |
| if @request.update(request_params) | |
| @request.assign_attributes(request_params) | |
| plant_attributes_from_params(@request) | |
| if @request.save |
(si tu regardes bien, c'est d'ailleurs bien dans cet ordre que sont appelées les méthodes dans le create, ou qu'elles l'étaient dans la méthode update d'origine, qui a récemment été supprimée dans une autre PR)
| def update? | ||
| if record.request_distributions.exists? | ||
| record.errors.add(:base, 'cannot update a request with associated request distributions') | ||
| false | ||
| else | ||
| true | ||
| end | ||
| end |
There was a problem hiding this comment.
ok pour la logique 👌 si on veut l'écrire de manière un peu plus "rails-ish" et concise, ça pourrait donner quelque chose comme ça :
| def update? | |
| if record.request_distributions.exists? | |
| record.errors.add(:base, 'cannot update a request with associated request distributions') | |
| false | |
| else | |
| true | |
| end | |
| end | |
| def update? | |
| return true unless record.request_distributions.exists? | |
| record.errors.add(:base, 'cannot update a request with associated request distributions') | |
| false | |
| end |
|
|
||
| request.reload |
There was a problem hiding this comment.
vu qu'ici on n'utilise pas l'objet request plus bas, on n'avait pas spécialement besoin de le reload :)
| request.reload |
No description provided.