[fix] Ensure site_name displays in password reset email #485
Conversation
…#482 Ensure password-reset email templates have access to current_site so template variables like site_name render correctly. Inject current_site into the email context when missing by calling get_current_site(self.request) if a request is available; use a conservative try/except to avoid raising if the site cannot be determined. Adds a unit test (openwisp_users/tests/test_adapter.py) that asserts the site name appears in password reset emails. Fixes openwisp#482
openwisp_users/accounts/adapter.py
Outdated
| if getattr(self, "request", None): | ||
| context["current_site"] = get_current_site(self.request) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
This entire block looks quite clunky, I'll let @coderabbitai review it.
There was a problem hiding this comment.
@nemesifier, I agree that this block could be improved. Here are the main issues:
-
Overly broad exception handling: The bare
except Exception: passsilently catches all exceptions, which can hide real problems and make debugging difficult. -
Unnecessary nesting: The try-except wrapping a conditional check creates awkward nesting that reduces readability.
-
Silent failures: Errors are completely ignored, which might mask configuration issues or real problems with the request object.
Here's a cleaner approach:
if "current_site" not in context:
request = getattr(self, "request", None)
if request is not None:
try:
context["current_site"] = get_current_site(request)
except Exception as e:
# Log the error for debugging purposes
# Optionally: logger.warning(f"Failed to get current site: {e}")
passOr even more concise if you want to handle specific exceptions:
if "current_site" not in context:
request = getattr(self, "request", None)
if request:
try:
context["current_site"] = get_current_site(request)
except (AttributeError, RuntimeError):
# Expected exceptions when site framework is not properly configured
passBenefits:
- Flatter structure with better readability
- Request is assigned once and reused
- Optional: More specific exception handling or logging for better debugging
- The logic flow is clearer
There was a problem hiding this comment.
I wonder if we could just simplify this to:
try:
context["current_site"] = get_current_site(self.request)
except AttributeError:
passThere was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Django get_current_site function exceptions and behavior with None request
💡 Result:
Short answer
- django.contrib.sites.shortcuts.get_current_site(request) simply forwards to Site.objects.get_current(request) if the sites app is installed; otherwise it returns a RequestSite(request). (django.readthedocs.io)
- If SITE_ID is set in settings, get_current_site returns that Site. If SITE_ID is not set and you pass a request it looks up the Site by request.get_host() (tries host with and without port). If you pass request=None and SITE_ID is not set, Site.objects.get_current raises ImproperlyConfigured. If the domain lookup fails a Site.DoesNotExist will be raised. (docs.djangoproject.com)
Practical implications / how to avoid exceptions
- Ensure django.contrib.sites is in INSTALLED_APPS and set SITE_ID to a valid site id (safe server-side usage without a request). (docs.djangoproject.com)
- When SITE_ID is not used, always pass a valid HttpRequest (or use CurrentSiteMiddleware so request.site is available) so lookups use request.get_host(). (docs.djangoproject.com)
- Catch Site.DoesNotExist or ImproperlyConfigured if you must call get_current_site in contexts where neither SITE_ID nor a request can be guaranteed. (docs.djangoproject.com)
References
- django.contrib.sites.shortcuts.get_current_site implementation and docs. [1]. (django.readthedocs.io)
- django.contrib.sites.models.SiteManager.get_current behavior (raises ImproperlyConfigured when no SITE_ID and no request; looks up by request.get_host(); cached lookup). [2]. (docs.djangoproject.com)
- Sites framework docs describing RequestSite fallback and host/port lookup behavior. [3]. (docs.djangoproject.com)
Citations:
- 1: https://django.readthedocs.io/en/3.1.x/_modules/django/contrib/sites/shortcuts.html?utm_source=openai
- 2: https://docs.djangoproject.com/en/5.0/_modules/django/contrib/sites/models/?utm_source=openai
- 3: https://docs.djangoproject.com/en/dev/ref/contrib/sites/?utm_source=openai
- 4: https://docs.djangoproject.com/en/5.0/_modules/django/contrib/sites/models/?utm_source=openai
- 5: https://docs.djangoproject.com/en/5.0/_modules/django/contrib/sites/models/?utm_source=openai
- 6: https://django.readthedocs.io/en/3.1.x/_modules/django/contrib/sites/shortcuts.html?utm_source=openai
- 7: https://docs.djangoproject.com/en/5.0/_modules/django/contrib/sites/models/?utm_source=openai
- 8: https://docs.djangoproject.com/en/dev/ref/contrib/sites/?utm_source=openai
@nemesifier, that simplification would be risky because it doesn't handle all the failure modes of get_current_site:
-
AttributeError only catches missing
self.requestattribute - but ifself.requestexists and isNone, callingget_current_site(None)raisesImproperlyConfigured(when SITE_ID isn't set), notAttributeError -
Site lookup failures - If the domain lookup fails,
get_current_siteraisesSite.DoesNotExist, which also wouldn't be caught -
Overwrites existing values - Removing the
"current_site" not in contextcheck means you'd overwrite anycurrent_sitethat was explicitly passed in the context
A better middle ground would be:
if "current_site" not in context:
request = getattr(self, "request", None)
if request is not None:
try:
context["current_site"] = get_current_site(request)
except (AttributeError, ImproperlyConfigured, Site.DoesNotExist):
passThis:
- Preserves the context check to avoid overwriting
- Extracts request once for clarity
- Catches the specific exceptions that
get_current_sitecan raise - Still fails gracefully when site framework isn't properly configured
You'd also need to add the import: from django.core.exceptions import ImproperlyConfigured and from django.contrib.sites.models import Site
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughEmailAdapter.send_mail now ensures the email rendering context includes Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openwisp_users/accounts/adapter.py (1)
12-17: Consider adding logging to the exception handler.The logic correctly injects
current_siteinto the context when missing, and the conservative try/except ensures email sending remains robust. However, silently suppressing all exceptions makes it difficult to diagnose when and why site resolution fails.📝 Add logging for better observability
+import logging + +logger = logging.getLogger(__name__) + class EmailAdapter(DefaultAccountAdapter): def send_mail(self, template_prefix, email, context): if "current_site" not in context: try: if getattr(self, "request", None): context["current_site"] = get_current_site(self.request) except Exception: + logger.debug( + "Could not resolve current_site for email %s", template_prefix + ) passAlternatively, narrow the exception types if you want to be more specific:
- except Exception: + except (RuntimeError, AttributeError): passBased on static analysis hints (Ruff BLE001, S110).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_users/accounts/adapter.pyopenwisp_users/tests/test_adapter.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_users/tests/test_adapter.py (3)
openwisp_users/base/models.py (1)
_create_user(23-30)openwisp_users/tests/utils.py (1)
_create_user(33-45)openwisp_users/api/views.py (1)
post(66-67)
🪛 Ruff (0.14.10)
openwisp_users/accounts/adapter.py
16-17: try-except-pass detected, consider logging the exception
(S110)
16-16: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
openwisp_users/tests/test_adapter.py (1)
47-58: LGTM! Test effectively validates the site name injection.The test correctly verifies that the password reset email includes the site name ("example.com") in both plain text and HTML versions. The logic is sound and directly exercises the scenario that issue #482 describes.
openwisp_users/accounts/adapter.py (1)
2-2: LGTM!The import of
get_current_siteis appropriate for resolving the current site from the request.
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
nemesifier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_users/accounts/adapter.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
🔇 Additional comments (1)
openwisp_users/accounts/adapter.py (1)
2-2: LGTM: Standard Django import.The addition of
get_current_siteimport is appropriate for obtaining the current site information.
openwisp_users/accounts/adapter.py
Outdated
| try: | ||
| context["current_site"] = get_current_site(self.request) | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
Check if current_site already exists before overwriting.
The PR description states the intent is to inject current_site "when missing," but the implementation unconditionally overwrites context["current_site"] without checking if it already exists. This could override values intentionally set by the caller.
Additionally, consider the following improvements:
- Broader exception handling:
get_current_sitecan raise exceptions beyondAttributeError(e.g.,ImproperlyConfiguredif the sites framework is misconfigured). - Observability: Silent failure with
passmakes debugging difficult—consider logging when the site cannot be determined.
🔧 Proposed fix with conditional check and improved error handling
+import logging
+
+logger = logging.getLogger(__name__)
+
class EmailAdapter(DefaultAccountAdapter):
def send_mail(self, template_prefix, email, context):
- try:
- context["current_site"] = get_current_site(self.request)
- except AttributeError:
- pass
+ if "current_site" not in context:
+ try:
+ context["current_site"] = get_current_site(self.request)
+ except (AttributeError, Exception) as e:
+ logger.debug(
+ "Could not determine current_site for email context: %s", e
+ )
subject = render_to_string("{0}_subject.txt".format(template_prefix), context)Committable suggestion skipped: line range outside the PR's diff.
nemesifier
force-pushed
the
issues/482-password-reset-not-displaying-site-name
branch
from
4b9dc84 to
8a54a35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @openwisp_users/accounts/adapter.py:
- Around line 12-13: The code in the adapter method sets context["current_site"]
using get_current_site(self.request) without error handling and uses
hasattr(self, "request") which doesn't ensure the request is not None; wrap that
logic in a conservative try/except that first checks self.request is not None
(e.g., if hasattr(self, "request") and self.request is not None) and then
attempts context["current_site"] = get_current_site(self.request) inside a try
block, catching Exception (or Site.DoesNotExist/ImproperlyConfigured plus a
broad fallback) and silently skipping setting current_site on failure so
password reset flow continues; reference the adapter method that contains the
existing context["current_site"] assignment to locate where to add the
try/except.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_users/accounts/adapter.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (1)
openwisp_users/accounts/adapter.py (1)
2-2: LGTM! Standard Django import.The import is correct and necessary for obtaining the current site instance.
| if "current_site" not in context and hasattr(self, "request"): | ||
| context["current_site"] = get_current_site(self.request) |
There was a problem hiding this comment.
Add exception handling as described in the PR description.
The PR description mentions the logic is "wrapped in a conservative try/except to remain safe when a request or site cannot be determined," but the implementation lacks this error handling. Without it, get_current_site() can raise exceptions (e.g., Site.DoesNotExist, ImproperlyConfigured, database errors) that would prevent password reset emails from being sent entirely.
Additionally, consider verifying self.request is not None, as hasattr only checks attribute existence.
🛡️ Proposed fix with exception handling
- if "current_site" not in context and hasattr(self, "request"):
- context["current_site"] = get_current_site(self.request)
+ if "current_site" not in context and hasattr(self, "request") and self.request:
+ try:
+ context["current_site"] = get_current_site(self.request)
+ except Exception:
+ # Silently skip if site cannot be determined
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "current_site" not in context and hasattr(self, "request"): | |
| context["current_site"] = get_current_site(self.request) | |
| if "current_site" not in context and hasattr(self, "request") and self.request: | |
| try: | |
| context["current_site"] = get_current_site(self.request) | |
| except Exception: | |
| # Silently skip if site cannot be determined | |
| pass |
🤖 Prompt for AI Agents
In @openwisp_users/accounts/adapter.py around lines 12 - 13, The code in the
adapter method sets context["current_site"] using get_current_site(self.request)
without error handling and uses hasattr(self, "request") which doesn't ensure
the request is not None; wrap that logic in a conservative try/except that first
checks self.request is not None (e.g., if hasattr(self, "request") and
self.request is not None) and then attempts context["current_site"] =
get_current_site(self.request) inside a try block, catching Exception (or
Site.DoesNotExist/ImproperlyConfigured plus a broad fallback) and silently
skipping setting current_site on failure so password reset flow continues;
reference the adapter method that contains the existing context["current_site"]
assignment to locate where to add the try/except.
There was a problem hiding this comment.
If Site.DoesNotExist, ImproperlyConfigured are raised, they would be raised much sooner during the application life cycle and this code would not even execute, so we can avoid needless redundancy.
There was a problem hiding this comment.
@nemesifier, thank you for the clarification! You're right that Site.DoesNotExist and ImproperlyConfigured would surface during Django's initialization phase rather than at email-sending time, making the exception handling unnecessary here. I appreciate the insight into the application lifecycle behavior.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
The password reset and related email templates expect
current_site.nameto be available as site_name in translated templates. Addcurrent_siteto the email template context so templates can render the ssite name again.Fixes #482
Checklist
Reference to Existing Issue
Closes #482
Description of Changes
current_siteinto the email template context when missing by obtaining it fromget_current-site(self.request), which is guarded with a conservative try/except so the adapter remains safe when a request/site cannot be determined.Screenshot
BEFORE
(example given in the issue description)
AFTER
(output in the terminal)