Refactor portal serializer + Add email logic for updates on Content #291
Conversation
vcai122
requested review from
judtinzhang,
tuneerroy,
dr-Jess and
ashleyzhang01
| EMAIL_PORT = os.environ.get("SMTP_PORT", 587) | ||
| EMAIL_HOST_USER = os.environ.get("SMTP_USERNAME", "") | ||
| EMAIL_HOST_PASSWORD = os.environ.get("SMTP_PASSWORD", "") | ||
| DEFAULT_FROM_EMAIL = os.environ.get("SMTP_FROM_EMAIL", EMAIL_HOST_USER) |
There was a problem hiding this comment.
When would DEFAULT_FROM_EMAIL not be EMAIL_HOST_USER?
There was a problem hiding this comment.
host user might be [email protected] and default from might be Penn Mobile <[email protected]>
|
|
||
|
|
||
| def get_backend_manager_emails(): | ||
| if group := Group.objects.filter(name="backend_managers").first(): |
There was a problem hiding this comment.
nit: prefer moving backend_managers into a variable so it can be re-used for other purposes in the future
backend/portal/models.py
Outdated
| prev = self.__class__.objects.filter(id=self.id).first() | ||
| super().save(*args, **kwargs) | ||
| if prev is None: | ||
| return self._on_create() |
There was a problem hiding this comment.
nit: no need for return here since
There was a problem hiding this comment.
wait yes need for return here because i want need to hard stop since prev is None.
it was originally
self._on_create() return
but i changed it to be fancy
There was a problem hiding this comment.
Wait then why don't you just have an elif for the latter clause?
There was a problem hiding this comment.
actually yea i could. I did return because i thought i would have more conditions to send an email
backend/portal/serializers.py
Outdated
| validated_data["status"] = Content.STATUS_DRAFT | ||
|
|
||
| # auto add all target populations of a kind if not specified | ||
| auto_add_kind = [ |
| return super().update(instance, validated_data) | ||
|
|
||
|
|
||
| class PollSerializer(ContentSerializer): |
There was a problem hiding this comment.
nit: Do you think its worth to override the PollSerializer to auto set multiselect=False? It is defaulted to False but just to be safe I'm not sure
There was a problem hiding this comment.
I think its fine, because also this means we don't have to change when we add multiselect. And even if a malicious actor sent us multiselect=True, we need to first approve it and even if we approve it won't do anything cuz the frontend code doesn't use it right now
backend/portal/serializers.py
Outdated
|
|
||
| return super().create(validated_data) | ||
|
|
||
| def update(self, instance, validated_data): | ||
| # if Poll is updated, then approve should be false | ||
| # if Content is updated, then approve should be false | ||
|
|
There was a problem hiding this comment.
For update, I think we also need the TargetPopulations logic you have above (ex. If user changes the target populations to empty then we need to add all of them)
backend/utils/email.py
Outdated
|
|
||
| def get_backend_manager_emails(): | ||
| if group := Group.objects.filter(name="backend_managers").first(): | ||
| return group.user_set.values_list("email", flat=True) |
There was a problem hiding this comment.
If a person doesn't have an email, then the return value with have a None in the list, which I'd imagine isn't ideal? Probably need to filter out the None's
backend/utils/email.py
Outdated
| subject=subject, | ||
| message=message, | ||
| from_email=None, | ||
| recipient_list=recipient_list if isinstance(recipient_list, list) else [recipient_list], |
There was a problem hiding this comment.
Not a huge fan of this line, especially since the name of the variable has "list" in it. Can we just enforce that recipient_list is always of type list?
| success = django_send_mail( | ||
| subject=subject, | ||
| message=message, | ||
| from_email=None, |
There was a problem hiding this comment.
if we set to None, it uses the default one specified in settings (base.py)
|
|
||
| @shared_task(name="utils.send_mail") | ||
| def send_mail(subject, recipient_list, message=None, html_message=None): | ||
| if recipient_list is None: |
There was a problem hiding this comment.
When will this be the case? I feel like if this is ever None we shouldn't fail silently like this (the return value of this function is never checked)
There was a problem hiding this comment.
I did this because I thought this could just be "intended bevahior" for this function for sake of ease lol.
A case where we need this right now is if a post without a user attached onto it (so old posts) are updated, the recipient list would be None.
I guess the real question is whether this should be the intended behavior of this function. I have no preference. We can throw an error instead and do the None checking on the side of the caller of this function instead
There was a problem hiding this comment.
I think I would prefer erroring here. Definitely will save a few minutes if someone needs to debug this code later on
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #291 +/- ##
==========================================
+ Coverage 89.23% 89.65% +0.42%
==========================================
Files 60 61 +1
Lines 2601 2581 -20
==========================================
- Hits 2321 2314 -7
+ Misses 280 267 -13 ☔ View full report in Codecov by Sentry. |
refactor portal serializer to use abstract class + deal with special case for form data
set up email configs + email utils (wrapper around django email)
set up email notifs on content save/update