Remove example.com text from html emails
#884
Merged
Conversation
This file contains 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
This resolved by replacing: `Site.objects.get_current()` (Django) --> `get_current_site()` (edX's) Because the Django `get_current` returns the default site (with settings.SITE_ID) _always_. The edX function is more site aware and returns the current site by using the `request.site` value which is the desired behaviour and would prevent the HTML emails from showing `example.com` in the email body as described in RED-1886.
Both Site.objects.get_current() and theming_helpers.get_current_site() should be run in request context to get the site reliably. Running either in celery context is error-prone and not multi-tenant. This commit fixes the errors that were introduced in the two PRs below: - https://github.com/edx/edx-platform/pull/22091 - https://github.com/edx/edx-platform/pull/22042 - https://github.com/edx/edx-platform/pull/18928 The problem is that using `emulate_http_request` will not get the real site the had original issue, but instead the `settings.SITE_ID`. The fix to use `get_current_site()` _outside_ the celery context, and pass the `get_base_template_context` which ensures that `emulate_http_request` is no longer needed. I've also removed a couple of duplicate context variables from `generate_activation_email_context` since they were provided by `get_base_template_context`.
OmarIthawi
requested review from
estherjsuh,
johnbaldwin,
melvinsoft and
thraxil
thraxil
approved these changes
Apr 22, 2021
OmarIthawi
added a commit
that referenced
this pull request
May 27, 2021
The error happened because of the incomplete fix in #884 This commit fixes the issue by getting back the mandatory `emulate_http_request` but now with a better method to know the site using our `appsembler.sites.utils` helpers. This should work on celery tasks while printing the correct site domain instead of `example.com` as it used to do before RED-1886 was fixed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Remove
example.comfrom emailThis resolved by replacing:
Site.objects.get_current()(Django) -->get_current_site()(edX's)Because the Django
get_currentreturns the default site (withsettings.SITE_ID) always.The edX function is more site aware and returns the current site by using the
request.sitevalue which is the desired behaviour and would prevent the HTML emails from showingexample.comin the email body as described in RED-1886.Refactor account activation email context logic
Both
Site.objects.get_current()andtheming_helpers.get_current_site()should be run in request context to get the site reliably.Running either in celery context is error-prone and not multi-tenant.
This commit fixes the errors that were introduced in the three upstream PRs below:
The problem is that using
emulate_http_requestwill not get the real site the had original issue, but instead thesettings.SITE_ID.The fix to use
get_current_site()outside the celery context, and pass theget_base_template_contextwhich ensures thatemulate_http_requestis no longer needed.I've also removed a couple of duplicate context variables from
generate_activation_email_contextsince they were provided byget_base_template_context.TODO
example.comtext from html emails #885