Repository navigation
Conversation
suiflex-bot
Bot
added
commit: feat
mulhamna
left a comment
mulhamna
left a comment
There was a problem hiding this comment.
Thanks for the PR! The core logic and test coverage look good, but there are a few important issues that need to be addressed before merging:
- Personal Information (PII) in tests: Real personal email address (
rejaisme@gmail.com) is committed in unit tests instead of RFC 2606 example domains. - Architecture diagram alignment: Box drawing border in
docs/ARCHITECTURE.mdgot broken and misaligned. - Default
mail_fromdomain: Hardcodingketon.idas default will cause 403 errors on self-hosted setups unless overridden. - Client lifecycle & timeout: Creating a new
httpx.AsyncClientper call and blocking the request handler with a 10s timeout. - HTML escaping:
reset_urlshould be escaped before rendering in email template.
Please see the inline comments for details.
| ) | ||
|
|
||
| sent = await send_password_reset_email( | ||
| to_email="rejaisme@gmail.com", |
There was a problem hiding this comment.
Please avoid using real personal email addresses in test cases. Use a standard RFC 2606 example address like user@example.com or alice@example.com instead of rejaisme@gmail.com. Also consider using https://suitest.local or example.com for the reset URL domain in tests.
| ▼ ▼ | ||
| ┌──────────────────────────────┐ | ||
| │ apps/api (FastAPI) │ | ||
| | `apps/api` (FastAPI) │ |
There was a problem hiding this comment.
The ASCII box-drawing border is misaligned here:
- Line 24 replaced the box-drawing character
│with an ASCII pipe|and added backticks inside the diagram. - Line 28 (
│ • Transactional Mailer (Resend)│) exceeds the 30-character box width, pushing the right border out of alignment.
Please adjust the width or shorten the label so the box-drawing border remains straight and consistent.
|
|
||
| # Transactional Mailer (Resend API) | ||
| resend_api_key: str = Field(default="", repr=False) | ||
| mail_from: str = Field(default="Suitest <sui@keton.id>") |
There was a problem hiding this comment.
Defaulting to sui@keton.id will cause API errors (403 Forbidden) for self-hosted instances when users provide their own SUITEST_RESEND_API_KEY without verifying keton.id on Resend.
Consider using a generic placeholder like Suitest <noreply@suitest.local> or Resend's sandbox domain Suitest <onboarding@resend.dev> as the fallback default, and document in .env.deploy.example that users must configure their verified sender domain.
|
|
||
| try: | ||
| async with httpx.AsyncClient(timeout=10.0) as client: | ||
| response = await client.post(RESEND_API_URL, json=payload, headers=headers) |
There was a problem hiding this comment.
Instantiating a new httpx.AsyncClient on every email send bypasses connection pooling. Additionally, with a 10-second timeout directly inside the synchronous password-reset request lifecycle, any upstream Resend latency or outage will stall the user's HTTP request.
Consider reducing the timeout (e.g. 5s) or offloading the delivery to a background task / reusable client.
| <div class="card"> | ||
| <h2>Reset your password</h2> | ||
| <p>We received a request to reset your password for your Suitest account. Click the button below to choose a new password:</p> | ||
| <p><a href="{reset_url}" class="button">Reset Password</a></p> |
There was a problem hiding this comment.
reset_url is directly interpolated into the HTML template without escaping. Please wrap it with html.escape(reset_url) before rendering to follow defensive HTML templating practices.
|
Ah thanks for the catch,, lemme fix that |
This branch has not been deployed
2 participants
Summary
MailerServiceusing Resend REST API (https://api.resend.com/emails) viahttpx.SUITEST_RESEND_API_KEY(optional: if omitted, mailer gracefully no-ops and keeps existing behavior).SUITEST_MAIL_FROM(defaults toSuitest <sui@keton.id>).send_password_reset_emailintoUserManager.on_after_forgot_passwordinapps/api/src/suitest_api/auth/manager.py..env.deploy.example,compose.deploy.yml,docs/ARCHITECTURE.md, anddocs/ROADMAP.md.respxinapps/api/tests/test_mailer.pyadhering to strict TDD and typing guidelines.Verification
uv run ruff check apps/apipassed.uv run ruff format --check apps/apipassed.uv run mypyon affected modules passed.uv run pytest apps/api/tests/test_mailer.py apps/api/tests/test_m1e_passwords.py(7 passed).Dockerfile.api).