search

ivanwolodin / notification

0 stars 0 forks source link

Code review #3

Open BigDeepBlue opened 5 months ago

BigDeepBlue commented 5 months ago

Нужна небольшая доработка:

  1. Вот это не очень хорошо. Никогда не создавайте миграции в автоматическом режиме при запуске контейнера. Это делается только вручную в процессе разработки.
  2. В моделях БД нужно избегать свойство null=True у текстовых полей. Если у текстового поля null=True, значит оно может иметь 2 значения в случаях когда данные отсутствуют "" и null, что не очень хорошо и нарушает single source of truth. Единственный момент, когда у текста может быть null=True, это когда unique=True и blank=True. В этом случае значение null=True нужно для того, чтобы избежать unique constraint violation при сохранении нескольких объектов с пустыми значениями.
  3. Предлагаю в rabbitmq/consumer вынести все настройки в settings.py и использовать BaseSettings. Вы это делали ранее, почему бы и не использовать тут?
  4. Для работы с почтой в dev окружении рекомендую https://github.com/mailhog/MailHog. Очень удобный инструмент выступающий в качестве SMTP сервера и web интерфейсом для просмотра всех отправленных сообщений. Вот еще пример https://akrabat.com/using-mailhog-via-docker-for-testing-email/
  5. Весь сервис нотификации у вас жестко завязан с email сообщениями. Необходимо предусмотреть и другие способы связи с пользователями. Реализация не нужна, но подготовка в виде абстрактных классов и полей в моделях должна быть.
  6. А как же персонифицированные сообщения? Что-то вроде: "Уважаемый, Ивани Арнольдович! ..." Кажется, такое у вас не предусмотрено.
  7. Продолжая п.6. Другие сервисы кинотеатра будут обращаться к сервису нотификации для того или иного уведомления пользователя. Вот ситуация: пользователь изменил пароль в сервисе авторизации, сервис авторизации просит сервис уведомлений отправить пользователю сообщение, что-то вроде "Пароль изменен....". Или в сервисе выдачи контента, что-то произошло и нужно уведомить пользователя. Другие сервисы кинотеатра, которые будут взаимодействовать с этим сервисом уведомлений, не будут иметь полной и актуальной информации о пользователе. У них точно будет id пользователя, а сервис нотификации, должен сходить в сервис авторизации (где у нас лежать актуальные данные пользователя) и забрать всю необходимую информацию о получателе сообщения.
  8. Ну и напоследок, нет тестов. Сделайте хотя бы, несколько для обозначения инфраструктуры.
BigDeepBlue commented 5 months ago

LGTM