Ensure that SMTP message body is not empty #6313
Conversation
Email notifications of both the start of a deploy, and of a failed deploy, have empty message bodies. Azure Communication Services rejects emails with empty message bodies with: > smtplib.SMTPDataError: (501, b'5.6.0 Request body validation error. Need either non-empty Html or PlainText body to be present.')
| :param subject: Email subject | ||
| :param message: Email message | ||
| :param to_admins: True if mail should be sent to Django admins | ||
| :param recipients: List of additional addresses to send mail to | ||
| """ | ||
| if environment.fab_settings_config.email_enabled: | ||
| print(color_summary(f">> Sending email: {subject}")) | ||
|
|
||
| if not message: | ||
| # Azure Communication Services require a message body |
There was a problem hiding this comment.
Oh, its only for Azure.
I'd say we don't make any change for this.
To cater to a restriction to on email provider, we are populating emails for all providers.
& I don't believe that empty email body is unusual, so its not clear why Azure isn't happy about it.
If we absolutely need to fix this, we could just catch the exception from provider and give a nicer message though the exception itself seems totally okay too since this is not because of any issues with commcare-cloud.
So, I'd say we don't address this error.
There was a problem hiding this comment.
Oh, its only for Azure.
According to HG Insights:
In 2024, Azure market share reached 24% of the global cloud computing market and their customer base grew by 14.2% from 2023 to 2024.
...
I don't believe that empty email body is unusual, so its not clear why Azure isn't happy about it.
Microsoft's implementations of Internet standards is an old and interesting discussion, but it seems only academic. I think we have to be pragmatic. What is the best way to deal with the situation we have?
I think there are two facts that persuade me that this change is good:
- Azure has a quarter of the cloud market, so it is not unrealistic to expect that some of Dimagi's self-hosting partners will use Azure to host HQ.
- An important partner is using Azure to host HQ right now.
I don't understand why you are recommending that we prevent our self-hosting partners from receiving notifications. That sounds like a footgun to me.
Alternatively, if you feel that this specific implementation of a solution is incorrect, would you be amenable to changing the notifications for start and failure to have message bodies, since the notification for success has a message body already?
There was a problem hiding this comment.
Hey @kaapstorm
Solid facts about the Azure usage, I was not aware its that widely used.
I don't understand why you are recommending that we prevent our self-hosting partners from receiving notifications. That sounds like a footgun to me.
Definitely not saying we should not notify. I see that commcare cloud is attempting to send the notification, but its failing due to restrictions at the email provider. I am aligned that we should make reasonable changes to cater for requirements of different integrations we have. My hesitation was to modify implementations for all email providers to cater for one email provider.
would you be amenable to changing the notifications for start and failure to have message bodies
I see that would resolve the concern currently for these emails. In future, if another email is added without a message body, the same error is going to appear. And I see that the change you are making takes care of that.
If here we acknowledge that some email providers block emails that don't have a body & in commcare cloud we want to take care of that generally & to cater for that we would use the subject as the email body if its missing, I am okay with that.
An alternate could be to
- add email body for the emails that don't have one
- raise an error if message body is missing so that the dev can add one
Moreover, happy to go with whatever the team decides.
This reverts commit 9694dfd.
Set the subject to the message because the message is short.
|
I'm curious what others think, @dannyroberts @gherceg ? How do you feel about setting a message body if it is empty? Alternatively, should we pass a message body to |
|
Bump. @snopoke and @AmitPhulera I noticed you've worked in this module. Any thoughts or opinions on this change? |
|
@kaapstorm The change looks reasonable to me. I also looked briefly into it while going through the forum and I had a similar approach in mind for the fix. |
|
Feels a bit weird to have the same subject and body for an email, but otherwise I think it is a simple enough change that it is worth doing for the reasons you outlined above. |
There was a problem hiding this comment.
Thanks for the redirection @kaapstorm
Also, I will be cool if you would like to drop the commit and the revert commit to keep this PR shorted with relevant commits only
GitHub makes this easy with "Squash and merge" :) It did squash the 3rd and 4th commits too, but I think that's okay. |
Context: Forum
Email notifications of both the start of a deploy, and of a failed deploy, have empty message bodies. Azure Communication Services rejects emails with empty message bodies with the following exception:
Environments Affected
Only affects environments that use Azure Communication Services for SMTP. No Dimagi environments are affected.