Open
Conversation
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
3 times, most recently
from
6ad30ea to
a2d6891
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Celery-based asynchronous email delivery (tasks, worker, Celery app), pluggable HTTP/SMTP email backends, management commands for waiting and running workers, email templates and utilities, Docker services for Redis/Mailpit/worker, and related config, tests, and GraphQL input/type adjustments. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API
participant Django as Django<br/>View/Serializer
participant DB as Database
participant Tx as Transaction<br/>on_commit()
participant Broker as Celery<br/>Broker (Redis)
participant Worker as Celery<br/>Worker
participant DBQ as DB Query
participant EmailSvc as Email<br/>Service/Backend
participant External as SMTP/API Service
Client->>Django: POST create contact/demo
Django->>DB: Insert instance
DB-->>Django: instance created
Django->>Tx: Register on_commit callback -> schedule task.delay(id)
Django-->>Client: Return response
Note over DB: Transaction commits
Tx->>Broker: task.delay(instance.id)
Broker-->>Worker: Task dispatched
Worker->>DBQ: Fetch instance (+ relations)
DBQ-->>Worker: Instance data
Worker->>EmailSvc: build & send_email(...)
EmailSvc->>External: HTTP POST / SMTP send
External-->>EmailSvc: Success/Failure
EmailSvc-->>Worker: result
Worker-->>Broker: Task complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
templates/email/request_demo.html (2)
3-4: Move{% load custom_tags %}outside the block.Template tag libraries are typically loaded at the top of the file, outside of blocks, for better readability and to ensure they're available throughout the template.
Proposed fix
{% extends "email/base.html" %} +{% load custom_tags %} {% block content %} -{% load custom_tags %} <div style="margin-bottom: 20px;">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/request_demo.html` around lines 3 - 4, Move the template tag load statement out of the block: remove the `{% load custom_tags %}` from inside the `{% block content %}` and place it at the top of the template before any block declarations so the custom tag library is loaded globally for this template; update the file around the `{% block content %}` and ensure `{% load custom_tags %}` appears once at the top of the file.
6-8: Use a descriptivealtattribute for the logo image."image description" is a placeholder. For accessibility, use something meaningful like "RC Select Logo".
Proposed fix
<img - alt="image description" height="auto" + alt="RC Select Logo" height="auto" src="{{ 'images/rc-select-logo.png' | static_full_path }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/request_demo.html` around lines 6 - 8, The img tag in templates/email/request_demo.html uses a placeholder alt ("image description"); replace it with a descriptive alt text like "RC Select Logo" by updating the alt attribute on the <img> element that references src="{{ 'images/rc-select-logo.png' | static_full_path }}", ensuring the logo image has meaningful accessible text.apps/resources/tests/mutation_test.py (2)
87-88: Redundant assertions—assert_has_callsafterassert_called_onceis unnecessary.
assert_called_once_with(contact_request.pk)combines both checks and is more concise.Proposed simplification
- mock_requests.assert_called_once() - mock_requests.assert_has_calls([call(contact_request.pk)]) + mock_requests.assert_called_once_with(contact_request.pk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/resources/tests/mutation_test.py` around lines 87 - 88, Replace the two redundant assertions on mock_requests with a single call that checks both invocation count and arguments: remove mock_requests.assert_called_once() and mock_requests.assert_has_calls([call(contact_request.pk)]) and use mock_requests.assert_called_once_with(contact_request.pk) instead; this change should be made where mock_requests and contact_request.pk are referenced in the test to simplify and consolidate the assertion.
207-208: Same simplification opportunity as above.Proposed simplification
- mock_requests.assert_called_once() - mock_requests.assert_has_calls([call(demo_request.pk)]) + mock_requests.assert_called_once_with(demo_request.pk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/resources/tests/mutation_test.py` around lines 207 - 208, Replace the two assertions using mock_requests.assert_called_once() and mock_requests.assert_has_calls([call(demo_request.pk)]) with a single clearer assertion: call mock_requests.assert_called_once_with(demo_request.pk) to both assert it was called exactly once and with the expected argument (update the test containing mock_requests to use assert_called_once_with).templates/email/contact_request.html (2)
6-8: Use a descriptivealtattribute for the logo image.Same issue as
request_demo.html—replace the placeholder with a meaningful description.Proposed fix
<img - alt="image description" height="auto" + alt="RC Select Logo" height="auto" src="{{ 'images/rc-select-logo.png' | static_full_path }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/contact_request.html` around lines 6 - 8, The img tag in templates/email/contact_request.html uses a placeholder alt value; update the alt attribute on the logo <img> (the tag referencing 'images/rc-select-logo.png') to a meaningful, descriptive string such as "RC Select logo" or "RC Select company logo" so screen readers and email clients convey the image purpose; ensure the alt text is concise and descriptive and replace the placeholder with that text.
3-4: Move{% load custom_tags %}outside the block.Same as the other template—load tags at the top for consistency and readability.
Proposed fix
{% extends "email/base.html" %} +{% load custom_tags %} {% block content %} -{% load custom_tags %} <div style="margin-bottom: 20px;">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/contact_request.html` around lines 3 - 4, Move the template tag load statement out of the block: place the `{% load custom_tags %}` line before the `{% block content %}` declaration so tags are loaded at the top-level (matching other templates) and remove the `{% load custom_tags %}` from inside the `{% block content %}` block.apps/common/templatetags/custom_tags.py (1)
8-15: LGTM! Consider adding a type hint for clarity.The implementation correctly builds absolute URLs using the
ParseResultobject fromAPP_DOMAIN. The logic for handling optional port is appropriate.Optional: Add type hint
`@register.filter`(is_safe=True) -def static_full_path(path): +def static_full_path(path: str) -> str: static_path = static(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/common/templatetags/custom_tags.py` around lines 8 - 15, Add a simple type hint to static_full_path to clarify inputs/outputs: change the signature of static_full_path to static_full_path(path: str) -> str, keeping the body using static() and settings.APP_DOMAIN (a ParseResult) unchanged; add any necessary imports for typing only if your linting requires them (no other logic changes).misc/dev/run_worker.sh (1)
3-3:REDIS_HOST_PORTis assigned but never used.This variable is extracted but not referenced anywhere in this script. If it's not needed, remove it to avoid confusion. If it's intended to be exported for child processes, add
export.Also, quote
$CELERY_REDIS_URLto prevent word splitting.Proposed fix
If the variable is not needed:
-REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') - ./manage.py wait_for_resources --db --celery-queueIf it's needed by child processes:
-REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') +export REDIS_HOST_PORT=$(echo "$CELERY_REDIS_URL" | sed 's|redis://\([^/]*\)/.*|\1|')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/dev/run_worker.sh` at line 3, REDIS_HOST_PORT is assigned but never used; either remove the assignment line entirely to avoid dead code, or if downstream processes need it, change the assignment to export REDIS_HOST_PORT and ensure you quote the input (use "$CELERY_REDIS_URL") to prevent word-splitting — update the line that references REDIS_HOST_PORT and CELERY_REDIS_URL accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/common/management/commands/run_celery_dev.py`:
- Around line 14-17: The restart_celery function should stop using a global
pkill; instead spawn the worker with subprocess.Popen and keep the returned
process handle (replace subprocess.call(CMD) usage with Popen for CMD), then on
restart call proc.terminate(), wait with a small timeout via
proc.wait(timeout=...), and if it doesn't exit call proc.kill(); remove the
"pkill -9 celery" flow and use the stored process handle to perform graceful
shutdown with kill() as a fallback, handling exceptions (e.g., TimeoutExpired)
around wait to trigger the fallback.
In `@apps/common/management/commands/wait_for_resources.py`:
- Around line 42-60: The current flow uses wait_for_redis() for both cache and
Celery broker checks and may call it twice when --all is used; create a separate
wait_for_broker() function that verifies CELERY_BROKER_URL by attempting a real
broker connection (e.g., open a socket or use the broker client/redis-kv client)
and raising on failure, keep wait_for_redis() for cache probing via
cache.get/set, then update the command dispatch logic (where --celery-queue,
--cache, and --all are handled) to call wait_for_redis() and wait_for_broker()
exactly once each (use explicit separate conditionals or a set of resources to
wait for rather than calling the same function twice) so --celery-queue checks
the broker endpoint and --all triggers both checks without duplication.
In `@main/__init__.py`:
- Around line 3-7: Move the django_stubs_ext.monkeypatch() call to run before
importing celery_app: call django_stubs_ext.monkeypatch() at the top of
main/__init__.py so the monkeypatch is applied before importing the symbol
celery_app (which triggers main.celery and its app.autodiscover_tasks()),
ensuring runtime-typed Django field annotations are patched prior to module
import and task discovery.
In `@main/settings.py`:
- Around line 51-61: Set a sensible default for EMAIL_BACKEND (e.g., the SMTP
backend configured for Mailpit) in the settings schema by changing EMAIL_BACKEND
from None to the Django SMTP backend string, and ensure EMAIL_HOST/EMAIL_PORT
match Mailpit defaults; then add early validation after loading email settings
that checks DEFAULT_FROM_EMAIL and EMAIL_TO and raises
django.core.exceptions.ImproperlyConfigured if either is missing so send_email()
calls in apps/resources/tasks.py won't fail at runtime (reference symbols:
EMAIL_BACKEND, DEFAULT_FROM_EMAIL, EMAIL_TO, send_email).
In `@templates/email/base.html`:
- Line 2: Add a HTML5 DOCTYPE declaration at the very top of the email template
to prevent quirks-mode rendering; insert <!DOCTYPE html> immediately before the
existing <html> tag in templates/email/base.html so the document starts with the
standard DOCTYPE declaration.
In `@templates/email/contact_request.html`:
- Around line 14-16: Update the paragraph text inside the <p style="font-size:
14px; margin-bottom: 15px; font-weight: bold; font-family: Arial, sans-serif;
color: `#000000`;"> element (the line that currently reads "You got a new
feedback!") to remove the incorrect article and read "You got new feedback!" (or
alternatively "You've received new feedback!" if you prefer), preserving the
surrounding HTML and styles.
In `@templates/email/request_demo.html`:
- Around line 18-21: The greeting currently uses the requester variable {{name}}
but must address the tool owner; update the greeting in
templates/email/request_demo.html to use the tool-owner variable instead
(replace {{name}} with the template variable that holds the owner, e.g.
{{owner_name}} or {{tool.owner.name}} depending on the RequestDemo/context),
keeping the rest of the template intact so the "User Information" section still
shows the requester’s name.
In `@utils/email/base.py`:
- Around line 128-140: The logs in the exception handler and in _handle_success
currently include raw recipient addresses (message.to); change both spots to
avoid PII by logging a recipient count (e.g. len(message.to)) or an internal
correlation id if available (e.g. message.correlation_id) instead of ",
".join(message.to). Update the logger.exception call in the except
requests.RequestException block and the logger.info call inside _handle_success
to include the chosen non-PII identifier while keeping subject and status code
as before.
- Around line 143-149: _handle_failure currently only logs and returns False,
which is ignored; modify it to raise the HTTP error when delivery should not be
silent: check the instance's fail_silently flag (or add a fail_silently param if
not present) and if fail_silently is False call response.raise_for_status() (or
raise a requests.HTTPError with the response) after logging so the exception
propagates to callers (e.g., EmailMessage.send and Celery tasks); if
fail_silently is True keep the existing warning log and return False.
- Around line 70-85: The __init__ currently ignores the api_endpoint parameter
and naively concatenates the API key into self.endpoint; update __init__ to
prefer the passed api_endpoint argument over settings.EMAIL_API_URL, validate
that an API key (self.api_key via getattr(settings, "EMAIL_API_KEY", None)) is
present and raise ImproperlyConfigured if missing, and construct self.endpoint
using urllib.parse (e.g., parse the base URL, merge or add the apiKey query
param safely) so existing query parameters are preserved and you never end up
with "?apiKey=None" or malformed URLs; keep the same attributes
(self.email_endpoint, self.timeout, self._session) and adjust only how
api_endpoint is chosen, validated, and composed into self.endpoint.
In `@utils/email/docs/email.md`:
- Around line 46-50: Update the docs to match the actual payload shape
implemented in utils/email/base.py: state that ApiEmailBackend currently
hard-codes IsBodyHtml=True and only constructs FromAsBase64, ToAsBase64,
CcAsBase64, SubjectAsBase64, and BodyAsBase64 (no BccAsBase64 and no separate
plain-text body support); remove references to plain-text support and
BccAsBase64, and explicitly document the current limitation and exact field
names produced by the EmailBackend/ApiEmailBackend so readers see the true
payload shape.
In `@utils/email/service.py`:
- Line 8: The text parameter in the send_email (or EmailService.send_email)
signature is accepted but never used; update the email construction logic (e.g.,
in send_email and any helper _build_message/_compose_message) to include text as
the plain-text body — use it as the primary set_content() when no HTML is
provided or add it as the plain-text alternative when HTML is present (ensure
MIME/EmailMessage usage adds both plain and html parts correctly);
alternatively, if you choose not to support a plain-text fallback, remove the
text parameter from send_email and all callers to avoid dead API.
- Around line 11-17: The EmailMultiAlternatives call currently puts HTML into
the body; change it to use the function parameter text as the plain-text
fallback (body=text or "") and keep html only in attach_alternative(html,
"text/html"). Locate the EmailMultiAlternatives instantiation and replace
body=html with body=text or "" (keep subject, to=to_email, cc=cc_email or []),
ensuring EmailMultiAlternatives and its attach_alternative call are the sole
places HTML is attached so plain-text clients receive the plain text.
---
Nitpick comments:
In `@apps/common/templatetags/custom_tags.py`:
- Around line 8-15: Add a simple type hint to static_full_path to clarify
inputs/outputs: change the signature of static_full_path to
static_full_path(path: str) -> str, keeping the body using static() and
settings.APP_DOMAIN (a ParseResult) unchanged; add any necessary imports for
typing only if your linting requires them (no other logic changes).
In `@apps/resources/tests/mutation_test.py`:
- Around line 87-88: Replace the two redundant assertions on mock_requests with
a single call that checks both invocation count and arguments: remove
mock_requests.assert_called_once() and
mock_requests.assert_has_calls([call(contact_request.pk)]) and use
mock_requests.assert_called_once_with(contact_request.pk) instead; this change
should be made where mock_requests and contact_request.pk are referenced in the
test to simplify and consolidate the assertion.
- Around line 207-208: Replace the two assertions using
mock_requests.assert_called_once() and
mock_requests.assert_has_calls([call(demo_request.pk)]) with a single clearer
assertion: call mock_requests.assert_called_once_with(demo_request.pk) to both
assert it was called exactly once and with the expected argument (update the
test containing mock_requests to use assert_called_once_with).
In `@misc/dev/run_worker.sh`:
- Line 3: REDIS_HOST_PORT is assigned but never used; either remove the
assignment line entirely to avoid dead code, or if downstream processes need it,
change the assignment to export REDIS_HOST_PORT and ensure you quote the input
(use "$CELERY_REDIS_URL") to prevent word-splitting — update the line that
references REDIS_HOST_PORT and CELERY_REDIS_URL accordingly.
In `@templates/email/contact_request.html`:
- Around line 6-8: The img tag in templates/email/contact_request.html uses a
placeholder alt value; update the alt attribute on the logo <img> (the tag
referencing 'images/rc-select-logo.png') to a meaningful, descriptive string
such as "RC Select logo" or "RC Select company logo" so screen readers and email
clients convey the image purpose; ensure the alt text is concise and descriptive
and replace the placeholder with that text.
- Around line 3-4: Move the template tag load statement out of the block: place
the `{% load custom_tags %}` line before the `{% block content %}` declaration
so tags are loaded at the top-level (matching other templates) and remove the
`{% load custom_tags %}` from inside the `{% block content %}` block.
In `@templates/email/request_demo.html`:
- Around line 3-4: Move the template tag load statement out of the block: remove
the `{% load custom_tags %}` from inside the `{% block content %}` and place it
at the top of the template before any block declarations so the custom tag
library is loaded globally for this template; update the file around the `{%
block content %}` and ensure `{% load custom_tags %}` appears once at the top of
the file.
- Around line 6-8: The img tag in templates/email/request_demo.html uses a
placeholder alt ("image description"); replace it with a descriptive alt text
like "RC Select Logo" by updating the alt attribute on the <img> element that
references src="{{ 'images/rc-select-logo.png' | static_full_path }}", ensuring
the logo image has meaningful accessible text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7042bc71-fc2c-4496-ad2b-b48fc83985a9
⛔ Files ignored due to path filters (2)
static/images/rc-select-logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.pre-commit-config.yamlapps/common/management/commands/run_celery_dev.pyapps/common/management/commands/wait_for_resources.pyapps/common/templatetags/__init__.pyapps/common/templatetags/custom_tags.pyapps/resources/graphql/inputs.pyapps/resources/migrations/0004_requestdemo.pyapps/resources/serializers.pyapps/resources/tasks.pyapps/resources/tests/mutation_test.pyapps/resources/utils.pydocker-compose.yamlmain/__init__.pymain/celery.pymain/settings.pymisc/dev/run_worker.shpyproject.tomlschema.graphqltemplates/email/base.htmltemplates/email/contact_request.htmltemplates/email/request_demo.htmlutils/email/__init__.pyutils/email/base.pyutils/email/docs/email.mdutils/email/service.py
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
from
491be6b to
c34d82c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
templates/email/base.html (1)
2-2:⚠️ Potential issue | 🟡 MinorAdd missing DOCTYPE at document start.
<!DOCTYPE html>is still missing before<html>, which can trigger quirks mode in some clients.💡 Proposed fix
+<!DOCTYPE html> <html> <head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/base.html` at line 2, Add the HTML5 doctype to the email template by inserting <!DOCTYPE html> at the very top of templates/email/base.html before the existing <html> tag so the document does not render in quirks mode; ensure the doctype is the first line above <html>.templates/email/contact_request.html (1)
15-15:⚠️ Potential issue | 🟡 MinorFix grammar in notification text.
Use “You got new feedback!” (or “You’ve received new feedback!”) instead of “You got a new feedback!”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/contact_request.html` at line 15, The notification text in the email template (templates/email/contact_request.html) is grammatically incorrect ("You got a new feedback!"); update the string to a correct form such as "You got new feedback!" or "You've received new feedback!" wherever that literal appears in the template to fix the grammar.
🧹 Nitpick comments (1)
templates/email/request_demo.html (1)
18-20: Remove stale TODO or align it with actual behavior.Line 18 says tool name is pending, but Line 20 already renders
{{tool}}. Please remove/update the TODO to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/request_demo.html` around lines 18 - 20, Remove the stale TODO comment above the demo request text: the template already uses the {{tool}} variable, so either delete the "<!-- TODO: Add tool name here -->" comment or replace it with a clarifying comment that reflects current behavior (e.g., "<!-- Renders the requesting tool via {{tool}} -->"); update the comment near the div that contains "You've received a new demo request from a user through {{tool}}" so it no longer falsely indicates the tool name is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/email/contact_request.html`:
- Line 7: The img tag in templates/email/contact_request.html currently uses a
placeholder alt="image description"; update that attribute to a meaningful label
(for example alt="RC Select logo") so screen readers and email clients convey
the logo identity—locate the image element in contact_request.html and replace
the alt value accordingly.
In `@templates/email/request_demo.html`:
- Line 7: Replace the generic alt attribute value used on the image tag
(alt="image description") with a specific, descriptive label (e.g., "RC Select
logo" or "RC Select company logo") so email clients and assistive tech show
useful fallback text; update the img element's alt attribute accordingly.
---
Duplicate comments:
In `@templates/email/base.html`:
- Line 2: Add the HTML5 doctype to the email template by inserting <!DOCTYPE
html> at the very top of templates/email/base.html before the existing <html>
tag so the document does not render in quirks mode; ensure the doctype is the
first line above <html>.
In `@templates/email/contact_request.html`:
- Line 15: The notification text in the email template
(templates/email/contact_request.html) is grammatically incorrect ("You got a
new feedback!"); update the string to a correct form such as "You got new
feedback!" or "You've received new feedback!" wherever that literal appears in
the template to fix the grammar.
---
Nitpick comments:
In `@templates/email/request_demo.html`:
- Around line 18-20: Remove the stale TODO comment above the demo request text:
the template already uses the {{tool}} variable, so either delete the "<!--
TODO: Add tool name here -->" comment or replace it with a clarifying comment
that reflects current behavior (e.g., "<!-- Renders the requesting tool via
{{tool}} -->"); update the comment near the div that contains "You've received a
new demo request from a user through {{tool}}" so it no longer falsely indicates
the tool name is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb4b99d4-d393-4979-bf42-e3f9d864bdd0
⛔ Files ignored due to path filters (1)
static/images/rc-select-logo.pngis excluded by!**/*.png
📒 Files selected for processing (6)
apps/common/templatetags/__init__.pyapps/common/templatetags/custom_tags.pymain/settings.pytemplates/email/base.htmltemplates/email/contact_request.htmltemplates/email/request_demo.html
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/common/templatetags/custom_tags.py
- main/settings.py
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
from
c34d82c to
c7cbbe8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
misc/dev/run_worker.sh (1)
3-3: Unused variable and unquoted expansion.
REDIS_HOST_PORTis extracted but never used in this script. Additionally,$CELERY_REDIS_URLshould be quoted to prevent word splitting.Either remove the unused variable or export it if needed elsewhere. If kept, quote the variable:
-REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') +REDIS_HOST_PORT=$(echo "$CELERY_REDIS_URL" | sed 's|redis://\([^/]*\)/.*|\1|')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/dev/run_worker.sh` at line 3, The REDIS_HOST_PORT variable in run_worker.sh is unused and the CELERY_REDIS_URL expansion is unquoted; either remove the REDIS_HOST_PORT assignment or export/use it elsewhere, and if you keep it, change the assignment to quote the input by referencing REDIS_HOST_PORT and CELERY_REDIS_URL (e.g., use "$CELERY_REDIS_URL") so word splitting is prevented and the variable is safe to export or consume later.templates/email/request_demo.html (1)
18-20: Remove stale TODO comment.The TODO comment says "Add tool name here" but the tool name is already being rendered on line 20 via
{{tool}}. Remove the obsolete comment.Proposed fix
-<!-- TODO: Add tool name here --> <div style="font-size: 14px; margin-bottom: 15px; font-family: Arial, sans-serif; color: `#000000`;"> You've received a new demo request from a user through {{tool}} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/request_demo.html` around lines 18 - 20, Remove the stale HTML comment "<!-- TODO: Add tool name here -->" that precedes the message element in the email template; keep the existing div that renders the tool via the {{tool}} template variable (the element containing "You've received a new demo request from a user through {{tool}}") so the message remains unchanged but the obsolete TODO comment is deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@misc/dev/run_worker.sh`:
- Line 3: The REDIS_HOST_PORT variable in run_worker.sh is unused and the
CELERY_REDIS_URL expansion is unquoted; either remove the REDIS_HOST_PORT
assignment or export/use it elsewhere, and if you keep it, change the assignment
to quote the input by referencing REDIS_HOST_PORT and CELERY_REDIS_URL (e.g.,
use "$CELERY_REDIS_URL") so word splitting is prevented and the variable is safe
to export or consume later.
In `@templates/email/request_demo.html`:
- Around line 18-20: Remove the stale HTML comment "<!-- TODO: Add tool name
here -->" that precedes the message element in the email template; keep the
existing div that renders the tool via the {{tool}} template variable (the
element containing "You've received a new demo request from a user through
{{tool}}") so the message remains unchanged but the obsolete TODO comment is
deleted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53817be5-8733-47e8-a488-a45688f228bd
⛔ Files ignored due to path filters (2)
static/images/rc-select-logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.pre-commit-config.yamlapps/common/management/commands/run_celery_dev.pyapps/common/management/commands/wait_for_resources.pyapps/common/templatetags/__init__.pyapps/common/templatetags/custom_tags.pyapps/resources/graphql/inputs.pyapps/resources/migrations/0004_requestdemo.pyapps/resources/serializers.pyapps/resources/tasks.pyapps/resources/tests/mutation_test.pyapps/resources/utils.pydocker-compose.yamlmain/__init__.pymain/celery.pymain/settings.pymisc/dev/run_worker.shpyproject.tomlschema.graphqltemplates/email/base.htmltemplates/email/contact_request.htmltemplates/email/request_demo.htmlutils/email/__init__.pyutils/email/base.pyutils/email/docs/email.mdutils/email/service.py
✅ Files skipped from review due to trivial changes (6)
- .pre-commit-config.yaml
- apps/resources/graphql/inputs.py
- apps/resources/migrations/0004_requestdemo.py
- apps/resources/utils.py
- utils/email/docs/email.md
- main/celery.py
🚧 Files skipped from review as they are similar to previous changes (10)
- schema.graphql
- pyproject.toml
- apps/resources/serializers.py
- main/init.py
- apps/common/templatetags/custom_tags.py
- utils/email/service.py
- apps/common/management/commands/wait_for_resources.py
- apps/resources/tests/mutation_test.py
- apps/common/management/commands/run_celery_dev.py
- apps/resources/tasks.py
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
from
c7cbbe8 to
c156bb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
utils/email/base.py (2)
128-140:⚠️ Potential issue | 🟠 MajorDon’t log raw recipient addresses.
These log lines still retain PII for every send and failure. Logging a recipient count is enough here.
Proposed fix
except requests.RequestException: - logger.exception("Email send failed | subject=%s | to=%s", message.subject, ", ".join(message.to)) + logger.exception( + "Email send failed | subject=%s | recipient_count=%d", + message.subject, + len(message.to), + ) if not self.fail_silently: raise return False @@ logger.info( - "Email sent | subject=%s | to=%s | status code=%s", + "Email sent | subject=%s | recipient_count=%d | status code=%s", message.subject, - ", ".join(message.to), + len(message.to), response.status_code, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/email/base.py` around lines 128 - 140, Replace PII in logs by logging recipient counts instead of raw addresses: in the exception handler that calls logger.exception (within the try/except around send) and in the _handle_success method where logger.info is called, remove ", ".join(message.to) and log len(message.to) (e.g., recipient_count) along with message.subject and response.status_code; keep behavior around self.fail_silently and raising the exception unchanged.
70-85:⚠️ Potential issue | 🟠 MajorUse the
api_endpointoverride and build the URL safely.The constructor still ignores its
api_endpointargument, andf"{self.email_endpoint}?apiKey={self.api_key}"breaks when the base URL already has query params or the key is missing.Proposed fix
+from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit @@ - self.email_endpoint = settings.EMAIL_API_URL + self.email_endpoint = api_endpoint or settings.EMAIL_API_URL self.timeout = getattr(settings, "EMAIL_API_TIMEOUT", 30) self.api_key = getattr(settings, "EMAIL_API_KEY", None) @@ if not self.email_endpoint: raise ImproperlyConfigured( f"{self.__class__.__name__} requires an API endpoint.", ) + if not self.api_key: + raise ImproperlyConfigured( + f"{self.__class__.__name__} requires an API key.", + ) @@ - self.endpoint = f"{self.email_endpoint}?apiKey={self.api_key}" + parts = urlsplit(self.email_endpoint) + query = dict(parse_qsl(parts.query, keep_blank_values=True)) + query["apiKey"] = self.api_key + self.endpoint = urlunsplit(parts._replace(query=urlencode(query)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/email/base.py` around lines 70 - 85, The constructor currently ignores the api_endpoint parameter and builds endpoint with string interpolation which breaks if api_key is missing or the base URL already has query params; update __init__ to use the provided api_endpoint when non-empty (fall back to settings.EMAIL_API_URL), require or gracefully handle a missing api_key (do not append an empty query param), and build self.endpoint using urllib.parse: parse the base URL (email_endpoint/api_endpoint), extract existing query params with parse_qsl, add apiKey only if api_key is not None, then rebuild the URL with urlencode and urlunparse so existing params are preserved and no duplicate '?' is introduced; update references to email_endpoint, api_endpoint, api_key, and endpoint accordingly.templates/email/base.html (1)
1-2:⚠️ Potential issue | 🟡 MinorAdd the missing doctype.
Without
<!DOCTYPE html>, some email clients can fall back to quirks mode.Proposed fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/base.html` around lines 1 - 2, The template is missing the HTML doctype declaration; add the <!DOCTYPE html> declaration as the very first line of templates/email/base.html just before the existing <html> tag so the email renders in standards mode across clients.templates/email/request_demo.html (1)
6-10:⚠️ Potential issue | 🟡 MinorUse meaningful logo alt text.
alt="image description"is still placeholder text and not useful fallback content.Proposed fix
- alt="image description" height="auto" + alt="RC Select logo" height="auto"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/request_demo.html` around lines 6 - 10, Replace the placeholder alt text on the email logo image in the templates/email/request_demo.html img tag (currently alt="image description") with meaningful fallback text that describes the image or brand (e.g., "RC Select logo" or your company name) so screen readers and broken-image scenarios convey useful information; update the alt attribute in that img element accordingly.apps/common/management/commands/run_celery_dev.py (1)
14-17:⚠️ Potential issue | 🟠 MajorTrack the spawned worker instead of using
pkill -9 celery.This can kill unrelated Celery workers on the same host and hard-stop in-flight tasks. Keep the
Popenhandle and terminate that specific process gracefully on reload.Proposed fix
-import shlex import subprocess import typing from pathlib import Path @@ -CMD = "celery -A main worker -E --concurrency=2 -l info" +CMD = ["celery", "-A", "main", "worker", "-E", "--concurrency=2", "-l", "info"] +worker_process: subprocess.Popen[str] | None = None @@ def restart_celery(*args: typing.Any, **kwargs: typing.Any): - kill_worker_cmd = "pkill -9 celery" - subprocess.call(shlex.split(kill_worker_cmd)) # noqa: S603 - subprocess.call(shlex.split(CMD)) # noqa: S603 + global worker_process + if worker_process and worker_process.poll() is None: + worker_process.terminate() + try: + worker_process.wait(timeout=10) + except subprocess.TimeoutExpired: + worker_process.kill() + worker_process.wait(timeout=10) + + worker_process = subprocess.Popen(CMD) # noqa: S603🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/common/management/commands/run_celery_dev.py` around lines 14 - 17, The restart_celery function currently uses a global pkill which kills all celery processes; change it to spawn the worker with subprocess.Popen (use CMD) and keep the Popen handle in a module-level variable (e.g., celery_proc). On restart, if celery_proc exists, call celery_proc.terminate(), wait with celery_proc.wait(timeout=...) and fall back to celery_proc.kill() if it hasn’t exited, then start a new Popen and store the new handle; ensure you use shlex.split(CMD) for the Popen args and check celery_proc.poll() to see if it’s already exited before terminating.main/settings.py (1)
59-60:⚠️ Potential issue | 🟠 MajorFail fast when sender/recipient settings are missing.
DEFAULT_FROM_EMAILis consumed byutils/email/base.py, andEMAIL_TOis used by the Celery email tasks. Leaving either nullable here pushes the failure to send time instead of catching a bad deployment during startup.Proposed fix
+from django.core.exceptions import ImproperlyConfigured @@ DEFAULT_FROM_EMAIL = env("DEFAULT_FROM_EMAIL") EMAIL_TO = env("EMAIL_TO") + +if not DEFAULT_FROM_EMAIL or not EMAIL_TO: + raise ImproperlyConfigured( + "DEFAULT_FROM_EMAIL and EMAIL_TO must be configured for email notifications.", + )Also applies to: 411-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/settings.py` around lines 59 - 60, DEFAULT_FROM_EMAIL and EMAIL_TO are currently declared nullable which defers failures to runtime; make these settings non-nullable so the app fails fast on startup: update the settings declarations for DEFAULT_FROM_EMAIL and EMAIL_TO to remove the (str, None) nullable tuple and require a value (e.g., (str,) or equivalent in your settings schema), and add a startup validation in the settings module (or in the app init) that raises a clear error if DEFAULT_FROM_EMAIL or EMAIL_TO is missing; this change affects the DEFAULT_FROM_EMAIL and EMAIL_TO symbols and prevents downstream failures in utils/email/base.py and the Celery email tasks.templates/email/contact_request.html (2)
6-10:⚠️ Potential issue | 🟡 MinorUse meaningful logo alt text.
alt="image description"is still placeholder text and not useful fallback content.Proposed fix
- alt="image description" height="auto" + alt="RC Select logo" height="auto"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/contact_request.html` around lines 6 - 10, Replace the placeholder alt attribute on the logo image in the contact_request.html template: locate the <img ... src="{{ 'images/rc-select-logo.png' | static_full_path }}" ...> tag and change alt="image description" to a concise, meaningful description (e.g., alt="RC Select logo" or alt="RC Select company logo") that accurately describes the image for accessibility and email clients.
14-16:⚠️ Potential issue | 🟡 MinorPrefer “new feedback” here.
“Feedback” is uncountable in this sentence, so “a new feedback” reads awkwardly in the email copy.
Proposed fix
- You got a new feedback! + You got new feedback!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/contact_request.html` around lines 14 - 16, In the templates/email/contact_request.html template, update the paragraph text that currently reads "You got a new feedback!" (the <p> element shown in the diff) to use uncountable "feedback" — e.g., change it to "You got new feedback!" or "You've received new feedback!" so the wording is grammatically correct; simply replace the string inside that <p> element.apps/common/management/commands/wait_for_resources.py (1)
112-115:⚠️ Potential issue | 🟠 Major
--celery-queuestill only re-runs the cache probe.This path never verifies
CELERY_BROKER_URL, and--allstill waits on the same Redis check twice. Please split broker readiness from cache readiness and invoke each one exactly once.Proposed direction
if _all or kwargs["redis"]: self.wait_for_redis() if _all or kwargs["celery_queue"]: - self.wait_for_redis() + self.wait_for_broker()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/common/management/commands/wait_for_resources.py` around lines 112 - 115, The current branch calls wait_for_redis() for both the redis and celery_queue flags, causing the CELERY_BROKER_URL never to be checked and duplicating the Redis probe; update the command handler so that when kwargs["celery_queue"] is true it calls a dedicated broker readiness helper (e.g., wait_for_celery_broker() or wait_for_broker()) instead of wait_for_redis(), and ensure the _all path invokes wait_for_redis() and the broker check exactly once each; add or implement the new wait_for_celery_broker() method to verify CELERY_BROKER_URL and use that symbol in the call site.
🧹 Nitpick comments (1)
misc/dev/run_worker.sh (1)
3-3: Drop or useREDIS_HOST_PORT.This variable is computed but never consumed, so the line only adds dead shell logic to the worker startup path.
Proposed fix
-REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') - ./manage.py wait_for_resources --db --celery-queue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/dev/run_worker.sh` at line 3, The script computes REDIS_HOST_PORT from CELERY_REDIS_URL but never uses it; either remove the unused assignment or wire it into startup: if you don't need the parsed host:port, delete the line that sets REDIS_HOST_PORT, otherwise export or use REDIS_HOST_PORT (the variable name) where the worker expects a broker host/port (e.g., export it or substitute it into CELERY_BROKER_URL/CELERY_BROKER_HOST before invoking the worker) so the parsed value is actually consumed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/settings.py`:
- Line 56: Update the EMAIL_PORT schema entry so the type is integer instead of
string: replace the tuple for EMAIL_PORT (currently (str, None)) with an integer
type (e.g., (int, None) or (int, <default>)) so env("EMAIL_PORT") yields a
numeric value compatible with Django's SMTP backend; also update the duplicate
EMAIL_PORT definition found later (around the other occurrence) to match, and
check any code reading EMAIL_PORT (calls to env("EMAIL_PORT")) to ensure it no
longer casts/assumes a string.
- Around line 63-66: The CACHES setting is missing so Django's cache falls back
to local memory and readiness checks in wait_for_resources.py (which call
cache.set()/cache.get()) won't validate Redis; update settings.py to define
CACHES using django.core.cache.backends.redis.RedisCache with the backend URL
chosen from TEST_CACHE_REDIS_URL when running tests and CACHE_REDIS_URL
otherwise (use CACHE_REDIS_URL as default), and ensure the keys CACHE_REDIS_URL
and TEST_CACHE_REDIS_URL are used to populate the CACHES['default']['LOCATION']
so the cache client actually connects to Redis during readiness checks.
---
Duplicate comments:
In `@apps/common/management/commands/run_celery_dev.py`:
- Around line 14-17: The restart_celery function currently uses a global pkill
which kills all celery processes; change it to spawn the worker with
subprocess.Popen (use CMD) and keep the Popen handle in a module-level variable
(e.g., celery_proc). On restart, if celery_proc exists, call
celery_proc.terminate(), wait with celery_proc.wait(timeout=...) and fall back
to celery_proc.kill() if it hasn’t exited, then start a new Popen and store the
new handle; ensure you use shlex.split(CMD) for the Popen args and check
celery_proc.poll() to see if it’s already exited before terminating.
In `@apps/common/management/commands/wait_for_resources.py`:
- Around line 112-115: The current branch calls wait_for_redis() for both the
redis and celery_queue flags, causing the CELERY_BROKER_URL never to be checked
and duplicating the Redis probe; update the command handler so that when
kwargs["celery_queue"] is true it calls a dedicated broker readiness helper
(e.g., wait_for_celery_broker() or wait_for_broker()) instead of
wait_for_redis(), and ensure the _all path invokes wait_for_redis() and the
broker check exactly once each; add or implement the new
wait_for_celery_broker() method to verify CELERY_BROKER_URL and use that symbol
in the call site.
In `@main/settings.py`:
- Around line 59-60: DEFAULT_FROM_EMAIL and EMAIL_TO are currently declared
nullable which defers failures to runtime; make these settings non-nullable so
the app fails fast on startup: update the settings declarations for
DEFAULT_FROM_EMAIL and EMAIL_TO to remove the (str, None) nullable tuple and
require a value (e.g., (str,) or equivalent in your settings schema), and add a
startup validation in the settings module (or in the app init) that raises a
clear error if DEFAULT_FROM_EMAIL or EMAIL_TO is missing; this change affects
the DEFAULT_FROM_EMAIL and EMAIL_TO symbols and prevents downstream failures in
utils/email/base.py and the Celery email tasks.
In `@templates/email/base.html`:
- Around line 1-2: The template is missing the HTML doctype declaration; add the
<!DOCTYPE html> declaration as the very first line of templates/email/base.html
just before the existing <html> tag so the email renders in standards mode
across clients.
In `@templates/email/contact_request.html`:
- Around line 6-10: Replace the placeholder alt attribute on the logo image in
the contact_request.html template: locate the <img ... src="{{
'images/rc-select-logo.png' | static_full_path }}" ...> tag and change
alt="image description" to a concise, meaningful description (e.g., alt="RC
Select logo" or alt="RC Select company logo") that accurately describes the
image for accessibility and email clients.
- Around line 14-16: In the templates/email/contact_request.html template,
update the paragraph text that currently reads "You got a new feedback!" (the
<p> element shown in the diff) to use uncountable "feedback" — e.g., change it
to "You got new feedback!" or "You've received new feedback!" so the wording is
grammatically correct; simply replace the string inside that <p> element.
In `@templates/email/request_demo.html`:
- Around line 6-10: Replace the placeholder alt text on the email logo image in
the templates/email/request_demo.html img tag (currently alt="image
description") with meaningful fallback text that describes the image or brand
(e.g., "RC Select logo" or your company name) so screen readers and broken-image
scenarios convey useful information; update the alt attribute in that img
element accordingly.
In `@utils/email/base.py`:
- Around line 128-140: Replace PII in logs by logging recipient counts instead
of raw addresses: in the exception handler that calls logger.exception (within
the try/except around send) and in the _handle_success method where logger.info
is called, remove ", ".join(message.to) and log len(message.to) (e.g.,
recipient_count) along with message.subject and response.status_code; keep
behavior around self.fail_silently and raising the exception unchanged.
- Around line 70-85: The constructor currently ignores the api_endpoint
parameter and builds endpoint with string interpolation which breaks if api_key
is missing or the base URL already has query params; update __init__ to use the
provided api_endpoint when non-empty (fall back to settings.EMAIL_API_URL),
require or gracefully handle a missing api_key (do not append an empty query
param), and build self.endpoint using urllib.parse: parse the base URL
(email_endpoint/api_endpoint), extract existing query params with parse_qsl, add
apiKey only if api_key is not None, then rebuild the URL with urlencode and
urlunparse so existing params are preserved and no duplicate '?' is introduced;
update references to email_endpoint, api_endpoint, api_key, and endpoint
accordingly.
---
Nitpick comments:
In `@misc/dev/run_worker.sh`:
- Line 3: The script computes REDIS_HOST_PORT from CELERY_REDIS_URL but never
uses it; either remove the unused assignment or wire it into startup: if you
don't need the parsed host:port, delete the line that sets REDIS_HOST_PORT,
otherwise export or use REDIS_HOST_PORT (the variable name) where the worker
expects a broker host/port (e.g., export it or substitute it into
CELERY_BROKER_URL/CELERY_BROKER_HOST before invoking the worker) so the parsed
value is actually consumed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad0f7e30-387d-461b-8820-a478b2fff5f8
⛔ Files ignored due to path filters (2)
static/images/rc-select-logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.pre-commit-config.yamlapps/common/management/commands/run_celery_dev.pyapps/common/management/commands/wait_for_resources.pyapps/common/templatetags/__init__.pyapps/common/templatetags/custom_tags.pyapps/resources/graphql/inputs.pyapps/resources/migrations/0004_alter_casestudy_cover_image_requestdemo.pyapps/resources/serializers.pyapps/resources/tasks.pyapps/resources/tests/mutation_test.pyapps/resources/utils.pydocker-compose.yamlmain/__init__.pymain/celery.pymain/settings.pymisc/dev/run_worker.shpyproject.tomlschema.graphqltemplates/email/base.htmltemplates/email/contact_request.htmltemplates/email/request_demo.htmlutils/email/__init__.pyutils/email/base.pyutils/email/docs/email.mdutils/email/service.py
💤 Files with no reviewable changes (1)
- apps/resources/migrations/0004_alter_casestudy_cover_image_requestdemo.py
✅ Files skipped from review due to trivial changes (6)
- .pre-commit-config.yaml
- apps/resources/graphql/inputs.py
- apps/common/templatetags/custom_tags.py
- apps/resources/utils.py
- main/celery.py
- utils/email/docs/email.md
🚧 Files skipped from review as they are similar to previous changes (7)
- main/init.py
- pyproject.toml
- utils/email/service.py
- apps/resources/tests/mutation_test.py
- apps/resources/tasks.py
- docker-compose.yaml
- apps/resources/serializers.py
susilnem
requested changes
Apr 9, 2026
| help="The maximum time (in seconds) the command is allowed to run before timing out. Default is 10 min.", | ||
| ) | ||
| parser.add_argument("--db", action="store_true", help="Wait for DB to be available") | ||
| parser.add_argument("--celery-queue", action="store_true", help="Wait for Celery queue to be available") |
Contributor
Author
There was a problem hiding this comment.
We need this since queue depends on Redis, so we must ensure Redis is up before the Celery queue
misc/dev/run_worker.sh
Outdated
|
|
||
| REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') | ||
|
|
||
| ./manage.py wait_for_resources --db --celery-queue |
Member
There was a problem hiding this comment.
Suggested change
| ./manage.py wait_for_resources --db --celery-queue | |
| ./manage.py wait_for_resources --db --redis |
| # Linter | ||
| - id: ruff | ||
| types_or: [python, pyi, jupyter, toml] | ||
| types_or: [python, pyi, jupyter, pyproject] |
Member
There was a problem hiding this comment.
Also update the version here...
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.15.6
hooks:
# Linter
- id: ruff
types_or: [python, pyi, jupyter, pyproject]
args: [--fix]
# Formatter
- id: ruff-format
types_or: [python, pyi, jupyter, pyproject]
utils/email/base.py
Outdated
| super().__init__(**kwargs) | ||
|
|
||
| self.email_endpoint = settings.EMAIL_API_URL | ||
| self.timeout = getattr(settings, "EMAIL_API_TIMEOUT", 30) |
Member
There was a problem hiding this comment.
I couldn’t find this value in settings.py. Ideally, the default should be defined at the settings level for consistency and easier configuration.
Suggested change
| self.timeout = getattr(settings, "EMAIL_API_TIMEOUT", 30) | |
| self.timeout = settings.EMAIL_API_TIMEOUT |
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
from
c156bb2 to
4aa2c2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
misc/dev/run_worker.sh (1)
3-3: Remove unused variable or export if needed downstream.
REDIS_HOST_PORTis computed but never used. If it's intended for downstream use, export it; otherwise, remove the dead code. Also, quote$CELERY_REDIS_URLto prevent word splitting.Option 1: Remove if unused
#!/bin/bash -e -REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') - ./manage.py wait_for_resources --db --redisOption 2: Export if needed and quote variable
-REDIS_HOST_PORT=$(echo $CELERY_REDIS_URL | sed 's|redis://\([^/]*\)/.*|\1|') +export REDIS_HOST_PORT="${CELERY_REDIS_URL#redis://}" +export REDIS_HOST_PORT="${REDIS_HOST_PORT%%/*}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/dev/run_worker.sh` at line 3, The REDIS_HOST_PORT variable is computed but never used; either remove the unused assignment or export it for downstream use and ensure CELERY_REDIS_URL is quoted to avoid word-splitting; specifically, delete the REDIS_HOST_PORT assignment if not needed, or change the assignment to export REDIS_HOST_PORT and compute it from a quoted "$CELERY_REDIS_URL" (use the existing sed expression against "$CELERY_REDIS_URL") so the variable is available to child processes and safe from word-splitting.apps/resources/tasks.py (1)
35-36: Consider line length for readability.The chained query is quite long. Consider breaking it across multiple lines for better readability.
Proposed formatting
instance = ( - RequestDemo.objects.select_related("tool").prefetch_related("tool__tool_owners").filter(id=request_demo_id).first() + RequestDemo.objects + .select_related("tool") + .prefetch_related("tool__tool_owners") + .filter(id=request_demo_id) + .first() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/resources/tasks.py` around lines 35 - 36, The long chained queryset assignment to instance using RequestDemo.objects.select_related("tool").prefetch_related("tool__tool_owners").filter(id=request_demo_id).first() should be reformatted for readability: split the chain across multiple lines (one call per line) or build the queryset in a variable (e.g., qs = RequestDemo.objects.select_related("tool") then qs = qs.prefetch_related("tool__tool_owners") then instance = qs.filter(id=request_demo_id).first()), keeping the same methods and arguments (select_related, prefetch_related, filter, first) and preserving the request_demo_id usage.templates/email/base.html (1)
46-53: Media queries have limited email client support.CSS media queries (lines 46-53) aren't supported by many email clients (Gmail, Outlook desktop). The responsive layout will only work in clients that support embedded CSS. This is acceptable as progressive enhancement, but be aware the mobile layout may not apply universally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/email/base.html` around lines 46 - 53, The media query block targeting .row, .label, and .value won’t work in many email clients; change the template to use email-safe responsive fallbacks: add email-friendly inline/table-based patterns and Outlook conditional CSS instead of relying solely on the `@media` rule, for example convert the .row layout into a role="presentation" table structure that uses fluid widths (100%) and display:block inline styles on .label/.value for mobile stacking, and include MSO conditional comments (if mso) to ensure Outlook desktop gets the stacked layout; update references to .row, .label, and .value in the markup so their responsive behavior is achieved with inline styles and table-based fallbacks rather than only the media query.docker-compose.yaml (2)
30-33: Minor formatting inconsistency in comment.For consistency with other comments (e.g.,
#.Suggested fix
- `#Celery` + # Celery🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 30 - 33, The comment "#Celery" is missing a space and should be updated for consistency with other section comments; change the comment preceding CELERY_REDIS_URL (currently "#Celery") to "# Celery" so it matches the formatting used for sections like "# Email" and keeps the block with CELERY_REDIS_URL, CACHE_REDIS_URL, and TEST_CACHE_REDIS_URL consistent.
76-77: Consider binding mailpit UI to localhost only.Unlike the
webservice (line 91) which binds to127.0.0.1:8000, the mailpit port is exposed to all network interfaces. This could expose the email testing UI on the network in shared development environments.Suggested fix
ports: - - 8025:8025 + - 127.0.0.1:8025:8025🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 76 - 77, The mailpit service currently exposes port mapping "8025:8025" to all interfaces; change the ports entry for the mailpit service to bind to localhost only by replacing "8025:8025" with "127.0.0.1:8025:8025" (mirror how the web service binds to "127.0.0.1:8000"), so update the ports array under the mailpit service to use the explicit host IP to restrict access to localhost. Ensure the ports key and the string "8025:8025" are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/resources/tasks.py`:
- Around line 20-21: Validate settings.EMAIL_TO before using it as recipient:
check that the variable used as recipient (the local variable recipient in
apps/resources/tasks.py) is not None and is a non-empty string or list of
strings before calling send_email or constructing to_email=[recipient]; if
invalid, either skip sending and log an error/warning or raise a clear
ValueError. Update the code paths around where recipient is set and where
send_email is called (the blocks creating email_subject and calling send_email)
to perform this guard and normalize the value to a proper list[str] when valid.
- Around line 55-56: The cc_email list is being built with
cc_email=[settings.EMAIL_TO] which will include None when settings.EMAIL_TO is
unset; update the email-sending logic (the function that calls cc_email, e.g.,
the task in apps/resources/tasks.py) to validate/filter settings.EMAIL_TO before
adding it to cc_email — only include it when truthy (not None/empty), or build
cc_email as a filtered list (e.g., omit falsy values) so you never pass [None]
into the email backend.
In `@docker-compose.yaml`:
- Around line 65-68: The docker-compose redis service currently uses a floating
tag "image: redis:8"; update the redis service definition (the redis service and
its image setting) to pin a specific Redis patch/minor version (for example
"redis:8.2" or "redis:7.4") instead of "redis:8" to avoid unexpected updates in
production—choose the exact tag that matches your compatibility requirements and
replace the image value accordingly.
---
Nitpick comments:
In `@apps/resources/tasks.py`:
- Around line 35-36: The long chained queryset assignment to instance using
RequestDemo.objects.select_related("tool").prefetch_related("tool__tool_owners").filter(id=request_demo_id).first()
should be reformatted for readability: split the chain across multiple lines
(one call per line) or build the queryset in a variable (e.g., qs =
RequestDemo.objects.select_related("tool") then qs =
qs.prefetch_related("tool__tool_owners") then instance =
qs.filter(id=request_demo_id).first()), keeping the same methods and arguments
(select_related, prefetch_related, filter, first) and preserving the
request_demo_id usage.
In `@docker-compose.yaml`:
- Around line 30-33: The comment "#Celery" is missing a space and should be
updated for consistency with other section comments; change the comment
preceding CELERY_REDIS_URL (currently "#Celery") to "# Celery" so it matches the
formatting used for sections like "# Email" and keeps the block with
CELERY_REDIS_URL, CACHE_REDIS_URL, and TEST_CACHE_REDIS_URL consistent.
- Around line 76-77: The mailpit service currently exposes port mapping
"8025:8025" to all interfaces; change the ports entry for the mailpit service to
bind to localhost only by replacing "8025:8025" with "127.0.0.1:8025:8025"
(mirror how the web service binds to "127.0.0.1:8000"), so update the ports
array under the mailpit service to use the explicit host IP to restrict access
to localhost. Ensure the ports key and the string "8025:8025" are updated
accordingly.
In `@misc/dev/run_worker.sh`:
- Line 3: The REDIS_HOST_PORT variable is computed but never used; either remove
the unused assignment or export it for downstream use and ensure
CELERY_REDIS_URL is quoted to avoid word-splitting; specifically, delete the
REDIS_HOST_PORT assignment if not needed, or change the assignment to export
REDIS_HOST_PORT and compute it from a quoted "$CELERY_REDIS_URL" (use the
existing sed expression against "$CELERY_REDIS_URL") so the variable is
available to child processes and safe from word-splitting.
In `@templates/email/base.html`:
- Around line 46-53: The media query block targeting .row, .label, and .value
won’t work in many email clients; change the template to use email-safe
responsive fallbacks: add email-friendly inline/table-based patterns and Outlook
conditional CSS instead of relying solely on the `@media` rule, for example
convert the .row layout into a role="presentation" table structure that uses
fluid widths (100%) and display:block inline styles on .label/.value for mobile
stacking, and include MSO conditional comments (if mso) to ensure Outlook
desktop gets the stacked layout; update references to .row, .label, and .value
in the markup so their responsive behavior is achieved with inline styles and
table-based fallbacks rather than only the media query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ea91849-5b7d-4578-958e-92d0338a2fae
⛔ Files ignored due to path filters (2)
static/images/rc-select-logo.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.pre-commit-config.yamlapps/common/management/commands/run_celery_dev.pyapps/common/management/commands/wait_for_resources.pyapps/common/templatetags/__init__.pyapps/common/templatetags/custom_tags.pyapps/resources/graphql/inputs.pyapps/resources/migrations/0004_alter_casestudy_cover_image_requestdemo.pyapps/resources/serializers.pyapps/resources/tasks.pyapps/resources/tests/mutation_test.pyapps/resources/utils.pyapps/tool_picker/models.pydocker-compose.yamlmain/__init__.pymain/celery.pymain/graphql/schema.pymain/settings.pymisc/dev/run_worker.shpyproject.tomlschema.graphqltemplates/email/base.htmltemplates/email/contact_request.htmltemplates/email/request_demo.htmlutils/email/__init__.pyutils/email/base.pyutils/email/docs/email.mdutils/email/service.pyutils/graphql/types.py
💤 Files with no reviewable changes (1)
- apps/resources/migrations/0004_alter_casestudy_cover_image_requestdemo.py
✅ Files skipped from review due to trivial changes (7)
- apps/resources/graphql/inputs.py
- main/graphql/schema.py
- utils/graphql/types.py
- .pre-commit-config.yaml
- apps/tool_picker/models.py
- utils/email/docs/email.md
- utils/email/base.py
🚧 Files skipped from review as they are similar to previous changes (9)
- main/init.py
- utils/email/service.py
- apps/common/templatetags/custom_tags.py
- pyproject.toml
- main/celery.py
- apps/common/management/commands/wait_for_resources.py
- apps/resources/tests/mutation_test.py
- apps/common/management/commands/run_celery_dev.py
- apps/resources/utils.py
- Add email notification functionality for tool demo requests to notify the tool owner. - Add email notification for contact to admin
sudip-khanal
force-pushed
the
feat/add-email-notification-setup
branch
from
4aa2c2b to
4f4f1c4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This PR doesn't introduce any:
printThis PR contains valid:
Summary by CodeRabbit
New Features
Chores
Documentation