#issue-144/user create security#153
Conversation
sciolkowski
left a comment
sciolkowski
left a comment
There was a problem hiding this comment.
Dokończę to review w przyszłym tygodniu
| use Perseids.Web, :controller | ||
| import Perseids.Gettext | ||
|
|
||
| defstruct( |
There was a problem hiding this comment.
@gorzki IMO lepiej będzie zrobić model bez kolekcji w bazie (analogicznie jak modele PORO w Rails), a dla tych co mają bazę i będziemy je chcieli walidować po prostu dodamy defstruct. Generalnie spoko pomysł z wykorzystanie structa 👍 Po prostu nie róbmy go dla całego kontrolera.
| defp maybe_put_key(true, params, key, value), do: Map.put(params, key, value) | ||
| defp maybe_put_key(false, params, _key, _value), do: params | ||
|
|
||
| defp check_is_valid?(struct, errors \\ []) do |
There was a problem hiding this comment.
@gorzki Całą logikę walidacji trzeba umieścić w oddzielnym module, który będzie działał na zasadzie przyjmowania structa i paramsów i zwracał co trzeba. Dzięki temu będziemy go mogli używać gdzie zechcemy w jasny sposób.
sciolkowski
left a comment
sciolkowski
left a comment
There was a problem hiding this comment.
Ogólnie nie za bardzo wiem co o tym myśleć.
Na pewno działa, ale kod wydaje mi się tak zawiły, że będzie nieutrzymywalny.
Sam pomysł ze structem, do którego będzie porównywany dany changeset (bo to wszystko powinno trafić do modelu), jest wg mnie bardzo dobry, natomiast implementacja mnie przeraża :) Nie chodzi o całokształt, ale głównie o funkcje get_key i check_key - zwyczajnie nie jestem w stanie załapać co robią.
Nie chcę tego dołączać do mastera, a jednocześnie nie chcę tego tracić, bo to, w połączeniu z walidacjami w order.ex może być niezłym punktem wyjścia do solidnego modułu walidacji.
Proponowałbym Ci w "wolnej chwili" jakoś uprościć te dwie funkcje o których wspomniałem a potem spróbować wyodrębnić to do oddzielnego modułu, przenieść structa do modelu, w modelu użyć tej walidacji w stylu struct vs changeset. Jeśli wtedy zadziała wszystko ładnie - lećmy w to. Jeśli nie to trzeba to zrobić inaczej.
2 participants
Pytanie ->
W chwili obecnej ogarnia pojedyncze obiekty, jeżeli obiekt zawiera listę obiektów sprawdzi tylko jeden. Będzie do rozbudowy w (Order, Products)? gdzie wysyłana jest lista. W takim przypadku trzeba dodac jeszcze jedna metode która sprawdzi czy to jest lista argumentów i odpowiednio przekieruje do pozostałych metod