-
Notifications
You must be signed in to change notification settings - Fork 28
feat(backend): password recovery #33 #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Добавил Celery с брокером Redis. Отправка письма добавляется в очередь задач |
|
Поправьте конфликты |
|
Поправьте конфликты |
|
Поправьте конфликты ) |
maxjamchuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Отличная работа, есть несколько замечаний.
Посмотрите внимательно на те что я пометил как Caution и Warning. Их обязательно нужно поправить.
Note, Tip и Information скорее на будущее.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Круто, что добавили *.session и .mypy_cache в .gitignore — меньше мусора в репо.
| # Maximum number of retries | ||
| MAX_RETRIES = 3 | ||
| # Name of log file | ||
| LOG_FILE = "app/services/auth/password_reset/logs/password_reset.log" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лог, кажется, пишется в путь внутри исходников (.../services/.../logs/...).
В проде такой путь может не существовать. Или вообще быть readonly.
Рекомендация: логировать через стандартный Django logging (handlers) или хотя бы писать в общий LOG_DIR/BASE_DIR с гарантией, что директория создаётся.
.env.example
Outdated
| EMAIL_PORT=587 | ||
| EMAIL_USE_TLS=True | ||
| EMAIL_HOST_USER=[email protected] | ||
| EMAIL_HOST_PASSWORD=secret123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В .env.example лучше держать только плейсхолдеры (без secret123), чтобы не закреплять плохую практику “секреты в репе”. Например: EMAIL_HOST_PASSWORD=your_password_here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Проблемы миграции следуют из проблем моделей.
После того как поправите модель можно будет либо пересоздать миграцию, либо создать новую миграцию добавляющую необходимые изменения.
Так как это первая миграция, я бы порекомендовал пересоздать миграцию, чтобы было меньше > мусорных файлов и меньше операций по созданию таблиц.
|
|
||
| class PasswordResetToken(models.Model): | ||
| user_id = models.ForeignKey(User, on_delete=models.CASCADE, null=True) | ||
| token_hash = models.CharField(255, null=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь ошибка.
Сейчас Django скорее всего видит это поле как verbose_name=255 (это, кстати видно в файле миграции) — скорее всего вы имели в виду max_length=255. Сейчас миграция создаёт поле с неправильными параметрами.
| ) | ||
|
|
||
|
|
||
| def redirect_mail_link(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning
Перед редиректом стоит проверить, что token вообще присутствует, иначе получится /reset?token=None. Можно вернуть ErrorPage/400 или редиректить на страницу “ссылка недействительна”.
app/settings.py
Outdated
| DEFAULT_FROM_EMAIL = os.environ.get("DEFAULT_FROM_EMAIL", "") | ||
|
|
||
| # Celery settings | ||
| CELERY_BROKER_URL = "redis://localhost:6379" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
В settings.py лучше не хардкодить Redis на localhost. В докере и проде это почти всегда другой хост.
Лучше: CELERY_BROKER_URL = os.getenv("CELERY_BROKER_URL", "redis://localhost:6379")
app/settings.py
Outdated
|
|
||
| # Celery settings | ||
| CELERY_BROKER_URL = "redis://localhost:6379" | ||
| CELERY_RESULT_BACKEND = "redis://localhost:6379" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
В settings.py лучше не хардкодить Redis на localhost. В докере и проде это почти всегда другой хост.
Лучше: CELERY_RESULT_BACKEND = os.getenv("CELERY_RESULT_BACKEND", "redis://localhost:6379")
app/urls.py
Outdated
| path("telegram/", include("app.services.telegram.telegram_channels.urls")), | ||
| path("auth/", include("app.services.auth.users.urls")), | ||
| path("account/", include("app.services.account.urls")), | ||
| path("reset-password", redirect_mail_link, name="link_in_mail"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Название link_in_mail не очень описательное. Может лучше password_reset_link или password_reset_redirect — по смыслу?
app/urls.py
Outdated
| path("telegram/", include("app.services.telegram.telegram_channels.urls")), | ||
| path("auth/", include("app.services.auth.users.urls")), | ||
| path("account/", include("app.services.account.urls")), | ||
| path("reset-password", redirect_mail_link, name="link_in_mail"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Для консистентности лучше reset-password/ (со слешем). Сейчас не критично, но желательно единообразие.
|
учел все рекомендации |
Ренедеринг с использованием inertia.