search

elezbar / notifications_sprint_1

0 stars 0 forks source link

Code review #8

Open BigDeepBlue opened 1 year ago

BigDeepBlue commented 1 year ago

Работа масштабная. В этой связи и ряд недочетов довольно серьезные:

  1. Малоинформативное Swagger описание API в api_notifications/api/v1
  2. В api_notifications/api/v1 не валидируете параметры пагинации 
      limit: Annotated[int, Query(description='Pagination page size, ge=1)] = 10,

    Про валидацию посмотрите тут https://fastapi.tiangolo.com/tutorial/path-params-numeric-validations/#number-validations-greater-than-or-equal это применимо и к Query

  3. Уточните пожалуйста вот такой момент. В api_notifications/api/v1 вы все это разрешаете всем подряд? авторизации я там не нашел. Даже если планируете закрыть от внешнего мира, то между манагерами, доступы нужно также регулировать.
  4. В папке api_notifications/schemas почти все файлы именованы с заглавной буквы. Это не python-style. При импорте можно перепутать с именем класса. https://peps.python.org/pep-0008/#package-and-module-names
  5. В папке worker я бы рекомендовал сделать единый файл с настройками приложения (settings.py, config.py - наследники BaseSettings), а не использовать os.environ.get в коде. Настройки приложения всегда нужно держать в одном месте. Сейчас они все в start.py, но со временем при вашем текущем подходе однозначно расползутся по приложению.
  6. Обратите внимание на подобные мелочи. Скобки лишние
  7. В моделях БД в api_notifications/models/models.py не забывайте про ondelete='CASCADE'. Нам же не нужны сиротливые записи без привязки к чему-либо.
  8. Тоже самое и в sheduler/sheduler/models/models.py
  9. Весь сервис нотификации у вас жестко завязан с email сообщениями. Необходимо предусмотреть и другие способы связи с пользователями. Реализация не нужна, но подготовка в виде абстрактных классов и полей в моделях должна быть.
  10. Для работы с почтой в dev окружении рекомендую https://github.com/mailhog/MailHog. Очень удобный инструмент выступающий в качестве SMTP сервера и web интерфейсом для просмотра всех отправленных сообщений. Вот еще пример https://akrabat.com/using-mailhog-via-docker-for-testing-email/
  11. В sheduler/requirements.txt версии зависимостей лучше все-таки фиксировать, чтобы при добавлении новой, случайно не апнуть что-нибудь, как это недавно случилось у многих при выходе aioredis==2.0.0, где сильно поменялись интерфейсы
  12. Вот это и это точно не будет работать. Вы же не awaite-тите этот асинхронный метод.
BigDeepBlue commented 1 year ago

LGTM