Hexlet / hexlet-correction

Typo Reporter
https://fixit.hexlet.io/
GNU Affero General Public License v3.0
41 stars 72 forks source link

[#260] fix workspace email validation message interpolation with test #278

Open d1z3d opened 3 weeks ago

d1z3d commented 3 weeks ago

Добрый день!

В dto при валидации email-а была ошибка в интерполяции. Внес изменения в dto и в тест. Билд запустил, ошибок нет (приложил скрин ниже).

В других dto также неправильно указана интерполяция. В них будет выводиться "The email "Email" is not valid". Нужно внести изменения во все остальные dto?

Снимок экрана 2024-06-16 в 00 49 01
bazilval commented 3 weeks ago

@d1z3d отлично, получается удачно выловили баг вашим тестом! Давайте этим пр сразу поправим все сообщения валидации, где ошибка в интерполяции. Посмотрите сразу, возможно не только в валидации емейла такое встречается, но и в прочих

d1z3d commented 3 weeks ago

@bazilval, поправил dto по созданию и обновлению аккаунта. Добавил тесты, а также некоторые обновил. Решил не добавлять тесты по обновлению аккаунта с некорректным email, а изменить существующие, но они были неправильные. В ответе возвращался 401 http статус.

Визуально не заметил, что есть где-то еще похожие проблемы при подстановки динамических параметров. Есть различия в отображение валидации на фронте. Здесь нужны правки?

Снимок экрана 2024-06-16 в 21 48 20 Снимок экрана 2024-06-16 в 21 49 05
bazilval commented 3 weeks ago

@d1z3d Отлично! Наверное хорошо бы к одному виду привести, второй вариант выглядит опрятнее!

И ещё, давайте сделаем все сообщения с большой буквы, а то неоднообразно как-то Ну и в идеале, конечно, сделать их все локализованными, чтобы подтягивались из файлов локализации для русского и английского языков. Насколько помню, на данный момент, они захардкожены прямо в аннотациях.

d1z3d commented 2 weeks ago

@bazilval, добрый день! Привел к единому формату стили в отображении ошибок. Подключил faker и instancio, переписал тесты в accountController. Написал утилитарный класс для проверки ошибок валидации через resource bundle.

@bazilval, в jakarta validation используется свой resource bundle, в зависимости от используемого языка.

render - https://hexlet-correction-ac3h.onrender.com

bazilval commented 1 week ago

@d1z3d если честно, мне показалось, что с instancio тесты стали менее читаемы, а значит сложнее в поддержании. В данном случае я пока не понял насколько нам действительно нужно это, потому что в случае со старыми тестами мы сразу видим что именно проверяется и какими данными.

Можешь немного пояснить почему решил добавить?

d1z3d commented 1 week ago

@bazilval, здравствуйте!

d1z3d commented 1 week ago

Хотя подумав над 3 пунктом снова понял, что это не так, так что он некорректный)

bazilval commented 1 week ago

@d1z3d хорошо, тогда дождёмся ревью от @Malcom1986 мб имеет смысл вынести это в отдельный PR, так как это немного меняет подход в тестировнии и возможно в каких-то других тестах его тоже следует применять

посмотрите ещё выше тут, я некоторые моменты отметил (формулировка в одном месте и вопрос по bundleSource)

Malcom1986 commented 6 days ago

Идея с Instancio неплохая, но я б ее на самом деле вынес в отдельный ПР. Тут бы все тесты тогда доработать. А с текущей задачей оно не связано по смыслу получается

Malcom1986 commented 12 hours ago

@d1z3d Андрей, поправите? Под использование instancio можно отдельный тикет создать. Но эта задача не особо критична, скорее просто улучшалка такая. Плюс еще можно поизучать, возможно есть готовые решения, чтобы маппить класс на параметры формы

d1z3d commented 6 hours ago

@Malcom1986, Максим, добрый день! Да, внесу правки. Смогу сделать ближе к выходным.