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.

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: add email error handler #575

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

trowik
Copy link
Member

@trowik trowik commented Jan 3, 2023

Resolves #571

@trowik trowik force-pushed the error-mail-handler branch from 22767c6 to 67c8383 Compare January 3, 2023 13:52
@trowik trowik requested review from anehx and open-dynaMIX January 3, 2023 14:00
@trowik trowik marked this pull request as ready for review January 3, 2023 14:00
Copy link
Member

@anehx anehx left a comment

Choose a reason for hiding this comment

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

We need to make this handler "opt-in" since not everyone will want this. Please make sure this will only be added if a certain env var is given. Also, the new env vars need to be documented.

@trowik trowik force-pushed the error-mail-handler branch from 67c8383 to 5039b4b Compare January 6, 2023 10:07
document_merge_service/settings.py Outdated Show resolved Hide resolved
CONFIGURATION.md Show resolved Hide resolved
@trowik trowik force-pushed the error-mail-handler branch from 5039b4b to c9b8c9d Compare January 6, 2023 13:12
@anehx
Copy link
Member

anehx commented Jan 6, 2023

Awesome! Good job 💯

@anehx anehx merged commit 012a893 into adfinis:main Jan 6, 2023
@open-dynaMIX
Copy link
Member

@trowik We are integrating django-generic-api-permissions right now. This will lead to extension code that possibly wants to use the django send_mail functionality. For this reason I would prefer to set the mailing settings no matter what, but only add the email logging handler if the env var ENABLE_ADMIN_EMAIL_LOGGING is True. This would more correspond to the expected behaviour of this env var IMO and would still allow sending emails.

The situation right now could lead to following scenario: I want to send mails from an extension. I use a hacky way of setting the mailing settings in django, but this would implicitly enable email logging.

@anehx
Copy link
Member

anehx commented Jan 10, 2023

@open-dynaMIX Very good point, I didn't think of this when reviewing.. @trowik could you do that?

@trowik
Copy link
Member Author

trowik commented Jan 10, 2023

@open-dynaMIX you're absolutely right.
I'm on it @anehx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error reporting
3 participants