Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update from_addr for default from bulk emails #29001

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Update from_addr for default from bulk emails #29001

merged 3 commits into from
Dec 6, 2021

Conversation

Drakko25
Copy link
Contributor

Description

This Pull Request is a draft by now, just to check if the CI passes. It, renames the setting EMAIL_USE_DEFAULT_FROM_FOR_BULK to EMAIL_USE_COURSE_ID_FROM_FOR_BULK and changes the default value to true. These changes were made in order to avoid non existent from address to fail in email servers.

Supporting information

@openedx-webhooks
Copy link

Thanks for the pull request, @Drakko25! I've created OSPR-6129 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 12, 2021
@Drakko25 Drakko25 mentioned this pull request Oct 12, 2021
Closed
@natabene
Copy link
Contributor

@Drakko25 Thank you for your contribution. Please let me know once this is ready for our review.

@regisb
Copy link
Contributor

regisb commented Oct 14, 2021

@Drakko25 thanks for taking this on! By reading your PR I realize that the solution I proposed was unclear. It is not possible/recommended to create a feature toggle that defaults to True. So, to switch the default value of EMAIL_USE_DEFAULT_FROM_FOR_BULK to True, we actually need to replace this toggle by its negative. Thus creating EMAIL_USE_COURSE_ID_FROM_FOR_BULK which will default to False. Only when this toggle is True will we include the course ID in the "from" address.

Am I making sense?

(In my understanding the quality violations reported by CI are not related to your PR)

@Drakko25 Drakko25 force-pushed the kevin/rejected-bulk-course-emails branch from ac0aa8f to a057853 Compare October 21, 2021 14:56
@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Oct 21, 2021
@Drakko25
Copy link
Contributor Author

@regisb sorry for the late response and thanks for the feedback. I added some fixes based on your feedback let me know your thoughts about it. Thanks.

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@natabene
Copy link
Contributor

@regisb Thank you for reviewing. Owning squad is fine with CC working on this PR. Once this is ready, can you find a CC to merge this? List of CCs can be found here: https://openedx.atlassian.net/wiki/spaces/COMM/pages/2800255156/Current+Core+Contributor+Committers+and+Champions

@pdpinch
Copy link
Contributor

pdpinch commented Oct 25, 2021

I'm willing to merge this, but it seems like it will require that edX set the flag EMAIL_USE_COURSE_ID_FROM_FOR_BULK to True in order to maintain edx.org's current functionality. Am I wrong about that?

@pdpinch
Copy link
Contributor

pdpinch commented Oct 29, 2021

@natabene what can you tell me about this CI / Quality Others test that is failing? It doesn't seem to prevent me from merging, and it's failing without any useful errors (that I can tell). Am I allowed to ignore it?

(Note that we're still waiting for an answer from edX about my question from Monday: https://github.com/edx/edx-platform/pull/29001#issuecomment-951058895. I'm not planning to merge without a response on that)

@natabene
Copy link
Contributor

@pdpinch I don't know, so I suggest reaching out to your champions.

@ormsbee
Copy link
Contributor

ormsbee commented Nov 12, 2021

@jristau1984: wanted to make sure this one didn't fall through the cracks

connorhaugh
connorhaugh approved these changes Dec 4, 2021
View reviewed changes
Copy link
Contributor

@connorhaugh connorhaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this looks good to me, and I have A config PR ready for review on Monday morning which will enable edx to keep its desired behavior. Once that is merged, I will plan to merge this. Thank you for both your work and your patience on this item.

@BbrSofiane
Copy link
Member

@pdpinch Looks like this is ready to be merged

@connorhaugh
Copy link
Contributor

@pdpinch This is now ready to be merged on our end-- should I merge it?

@connorhaugh connorhaugh merged commit fa258de into openedx:master Dec 6, 2021
@openedx-webhooks openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. engineering review labels Dec 6, 2021
@openedx-webhooks
Copy link

@Drakko25 🎉 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-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@regisb
Copy link
Contributor

regisb commented Dec 8, 2021

Congrats on merging your first PR @Drakko25! Can you please open the same PR on top of the open-release/maple.master branch, such that your contribution is included in the next major Open edX release?

regisb referenced this pull request in overhangio/tutor Dec 8, 2021
@regisb
fix: incorrect "from" address in course bulk emails
79c6d12 
Upstream PR: https://github.com/edx/edx-platform/pull/29001
See discussion:
https://discuss.overhang.io/t/sending-bulk-email-triggers-smtprecipientsrefused-error/1923
Related issue:
openedx/wg-build-test-release#102
nizarmah pushed a commit to open-craft/edx-platform that referenced this pull request Dec 14, 2021
@nizarmah
fix: update from_addr for default from bulk emails
f3a6e84 
* pull request against `master`: Update from_addr for default from bulk emails (openedx#29001)
* related openedx issue: openedx/wg-build-test-release#102

Co-authored-by: Kevin Valencia <kevin@bitmaker.la>
(cherry picked from commit fa258de)
@nizarmah nizarmah mentioned this pull request Dec 14, 2021
Merged
edx-community-bot added a commit that referenced this pull request Dec 14, 2021
@edx-community-bot
merge(#29586): Update from_addr for default from bulk emails
b35521c 
## Description

Backports course bulk email from address fix to the Maple master branch.

## Supporting information

* Original pull request: #29001 
* Reported issue: openedx/wg-build-test-release#102

## Deadline

* Best merged before December 20, 2021
nizarmah pushed a commit to open-craft/edx-platform that referenced this pull request Dec 17, 2021
@nizarmah
fix: update from_addr for default from bulk emails
e30b94a 
* pull request against `master`: Update from_addr for default from bulk emails (openedx#29001)
* related openedx issue: openedx/wg-build-test-release#102

Co-authored-by: Kevin Valencia <kevin@bitmaker.la>
(cherry picked from commit fa258de)
@nizarmah nizarmah mentioned this pull request Dec 17, 2021
Merged
@pdpinch pdpinch mentioned this pull request Jan 20, 2022
Merged
@mudassir-hafeez mudassir-hafeez mentioned this pull request Feb 16, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@regisb regisb regisb approved these changes

@connorhaugh connorhaugh connorhaugh approved these changes

Assignees
No one assigned
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.