-
Notifications
You must be signed in to change notification settings - Fork 5
feat: added new patch to add email settings #32
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
feat: added new patch to add email settings #32
Conversation
2487361 to
09b17a3
Compare
farhaanbukhsh
left a comment
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.
@AhtishamShahid Thank you for this PR I think we should allow a way for people using tutor-contrib-notification to set the value for NOTIFICATIONS_DEFAULT_FROM_EMAIL, We need to add this in plugin.py and assign a default value to this.
And then use the value from here to set this in the file, also instead of setting it in two different places you can set it in openedx-common-settings and this should apply everywhere.
b6c3ab7 to
bfccbac
Compare
Hi , I have updated the PR and tested it with. tutor config save --set . Now users can update the email if needed. |
farhaanbukhsh
left a comment
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.
👍
✅ I tested this: on tutor devstack, the changes are made in the config and settings files
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
|
@AhtishamShahid A small nit is to add the same info in the readme file as well. |
feat: updated docs for changes
2db42bd to
a5c7f7d
Compare
Added new patch to add email settings, this makes sure that
NOTIFICATIONS_DEFAULT_FROM_EMAILusesCONTACT_EMAILif email value is not setThis resolves this issue
#27
FYI @bmtcril @saraburns1