-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: account activation email site logo is not themed #29764
fix: account activation email site logo is not themed #29764
Conversation
Thanks for the pull request, @dyudyunov! I've created OSPR-6390 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@dyudyunov Thank you for your contribution. We will try to review soon. |
I added myself as reviewer here and read through the code. The fix looks correct. @dyudyunov thanks for the contribution. Could you give this a rebase so that checks run again and the check migration is able to run on the new version? |
I tested it and it works. However, @dyudyunov keep in mind that the whole site_configuration feature is soon being deprecated as per: openedx/platform-roadmap#21 |
Hi @felipemontoya! |
Fix the issue when the email sent from the microsite with its own theme has the logo from the main site. Preconditions: - microsite with its own theme is up and running STR: - Create a new account on the microsite; - Check your email inbox.
7cb9470
to
2ae94d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dyudyunov and @felipemontoya.
Not sure if it's a lot to ask, since the feature is going to be deprecated but do you think adding a test case worth the effort in this pull request?
I leave that to you @dyudyunov, given the simplicity I would be ok merging as is, but Omar does raise a good point with the test. |
Hi, @felipemontoya, @OmarIthawi |
@felipemontoya Please feel free to merge, just be mindful of merging times guidelines for the repo. |
@dyudyunov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
Fix the issue when the email sent from the microsite with its own theme
has the logo from the main site.
Preconditions:
STR:
Related PR to the open-release/maple.master: