201021 Fix weekly email report #1954
Conversation
|
tagging involved in the incident response @fuziontech, @mariusandra & @fuziontech in case you want to review |
| raw_email = kwargs.pop("raw_email", None) | ||
|
|
||
| if raw_email: | ||
| kwargs["email_hash"] = hashlib.sha256(f"{settings.SECRET_KEY}_{raw_email}".encode()).hexdigest() |
There was a problem hiding this comment.
I don't quite get why the actual email address is destroyed by hashing here. IMO it could actually come in handy to know who in particular wasn't reached by emailing due to a bug or something.
There was a problem hiding this comment.
The reasoning behind this is for privacy-preservation reasons. Having the email here would require extra effort to maintain it so that PII is cleared when it has to. This way, even if user data must be permanently deleted from the system, the model will not contain any. I guess if you were trying to debug some delivery issue and you know the email address you could compute the hash and look up using the hash. POV?
There was a problem hiding this comment.
Hm, that was also my reasoning for keeping messaging records tied to users, they could get cascade deleted then.
|
|
||
| objects = MessagingRecordManager() | ||
|
|
||
| email_hash: models.CharField = models.CharField(max_length=1024) | ||
| campaign_key: models.CharField = models.CharField(max_length=128) | ||
| sent_at: models.DateTimeField = models.DateTimeField(null=True) | ||
| created_at: models.DateTimeField = models.DateTimeField(auto_now_add=True) | ||
|
|
||
| class Meta: | ||
| unique_together = ("email_hash", "campaign_key") # can only send campaign once to each email |
There was a problem hiding this comment.
This looks a lot UserMessagingRecord in posthog-production, but email address- instead of user-centric. Could we maybe do user-centric here as well to use the keep track of messaging using a single scheme across both repos?
There was a problem hiding this comment.
I've already created an issue (#1983) to remind us to remove UserMessagingRecord in favor of this model. As for why no user-centric, I do believe we consider this improvement but it has trade-offs, the reason for doing this email-centric at first was due to the way SMTP connections are processed as a completely separate async task.
Changes
Closes #1937.
Checklist