Refactor portal serializer + Add email logic for updates on Content #291

vcai122 · 2024-04-25T05:45:52Z

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

judtinzhang

Looks good! Some comments and questions

judtinzhang · 2024-04-25T18:05:41Z

backend/pennmobile/settings/base.py

+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)


When would DEFAULT_FROM_EMAIL not be EMAIL_HOST_USER?

host user might be [email protected] and default from might be Penn Mobile <[email protected]>

judtinzhang · 2024-04-25T18:07:25Z

backend/utils/email.py

+
+
+def get_backend_manager_emails():
+    if group := Group.objects.filter(name="backend_managers").first():


nit: prefer moving backend_managers into a variable so it can be re-used for other purposes in the future

judtinzhang · 2024-04-25T23:20:50Z

backend/portal/models.py

+        prev = self.__class__.objects.filter(id=self.id).first()
+        super().save(*args, **kwargs)
+        if prev is None:
+            return self._on_create()


nit: no need for return here since

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

Wait then why don't you just have an elif for the latter clause?

actually yea i could. I did return because i thought i would have more conditions to send an email

yep up to you

judtinzhang · 2024-04-25T23:32:56Z

backend/portal/serializers.py

+        validated_data["status"] = Content.STATUS_DRAFT
+
+        # auto add all target populations of a kind if not specified
+        auto_add_kind = [


judtinzhang · 2024-04-25T23:35:20Z

backend/portal/serializers.py

        return super().update(instance, validated_data)


+class PollSerializer(ContentSerializer):


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

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

Sounds good

judtinzhang · 2024-04-25T23:53:38Z

backend/portal/serializers.py


        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
+


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)

judtinzhang · 2024-04-25T23:57:52Z

backend/utils/email.py

+
+def get_backend_manager_emails():
+    if group := Group.objects.filter(name="backend_managers").first():
+        return group.user_set.values_list("email", flat=True)


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

judtinzhang · 2024-04-26T00:00:07Z

backend/utils/email.py

+        subject=subject,
+        message=message,
+        from_email=None,
+        recipient_list=recipient_list if isinstance(recipient_list, list) else [recipient_list],


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?

judtinzhang · 2024-04-26T00:00:31Z

backend/utils/email.py

+    success = django_send_mail(
+        subject=subject,
+        message=message,
+        from_email=None,


Any reason this is None?

if we set to None, it uses the default one specified in settings (base.py)

judtinzhang · 2024-04-26T00:01:55Z

backend/utils/email.py

+
+@shared_task(name="utils.send_mail")
+def send_mail(subject, recipient_list, message=None, html_message=None):
+    if recipient_list is None:


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)

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

I think I would prefer erroring here. Definitely will save a few minutes if someone needs to debug this code later on

codecov · 2024-05-07T06:03:50Z

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.86%. Comparing base (d2e691c) to head (6949f0a).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
backend/portal/models.py	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   90.41%   90.86%   +0.45%     
==========================================
  Files          62       63       +1     
  Lines        2681     2661      -20     
==========================================
- Hits         2424     2418       -6     
+ Misses        257      243      -14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tuneerroy

didn't test it but your test cases look comprehensive so i trust it! ow lgtm :<

tuneerroy · 2024-09-30T02:03:58Z

backend/portal/models.py


    class Meta:
        abstract = True

+    def _get_email_subject(self):


if these are meant to be private methods, they should start with two underscores, not one.

hm
https://stackoverflow.com/questions/1301346/what-is-the-meaning-of-single-and-double-underscore-before-an-object-name

damn u right, that's so weird, I've always assumed the name mangling is a good way to enforce private

wait i didn't say i preferred single underscore. tbh i don't know which i prefer was hoping for u to choose one lol.

lol i'd just stick with SO's opinion, single underscore for private methods, double underscore if you're concerned about inheritance overriding

tuneerroy · 2024-09-30T02:05:06Z

backend/portal/models.py

+        )
+
+    def _on_status_change(self):
+        if email := getattr(self.creator, "email", None):


don't need to pass in the last param for getattr since the default return value if not found is None.

also... should do if _ is not None ... more Pythonic :)

no u need the default else it errors.

but we have this pattern everywhere in our code...

o u right

argh fine keep it

tuneerroy · 2024-09-30T02:10:23Z

backend/portal/serializers.py

+    def is_valid(self, *args, **kwargs):
+        if isinstance(self.initial_data, QueryDict):
+            self.initial_data = self.initial_data.dict()
+            self.initial_data["target_populations"] = list(


hot damn, a list and then a map...

tuneerroy · 2024-09-30T02:11:14Z

backend/portal/serializers.py

+            self.initial_data = self.initial_data.dict()
+            self.initial_data["target_populations"] = list(
+                (
+                    map(int, self.initial_data["target_populations"].split(","))


could also probably just do

self.initial_data["target_population"] = [int(v) for v in self.initial_data.get("target_populations", "").split(",")]

this is better but im lazy to change if theres no other changes hehehe

minghansun1 · 2024-09-30T17:15:31Z

backend/portal/models.py

@@ -48,17 +50,54 @@ class Content(models.Model):
    admin_comment = models.CharField(max_length=255, null=True, blank=True)


In line 50, created_date should probably be immutable.
created_date = models.DateTimeField(auto_now_add=True)

Update save to
def save(self, *args, **kwargs):
if not self.pk:
self.created_date = timezone.now() # Set created_date to the current time
else:
if 'update_fields' in kwargs and 'created_date' in kwargs['update_fields']:
kwargs['update_fields'].remove('created_date')

prev = self.__class__.objects.filter(id=self.id).first() super().save(*args, **kwargs) if prev is None: self._on_create() return if self.status != prev.status: self._on_status_change()

If you look in the serializer for Content, you'll see that created_date is read only so it is enforced there.
Technically an admin could change the created date, but there is no way to prevent this since you can always change a row in a db. In other words you can't enforce atomicity at the db level

minghansun1

General feedback: Everything looks logical to me, but I'm not familiar with Django best practices so it's hard for me to find specific issues.

Design Consideration:
For PollVotes, we could give users the option to be anonymous or named, and let us filter results based anonymous votes, named votes, and overall votes.

Specific Issues: Make created_date immutable, do _auto_add_target_population using set difference instead of nested loops

minghansun1 · 2024-09-30T17:49:33Z

backend/portal/serializers.py

+
+    def _auto_add_target_population(self, validated_data):
+        # auto add all target populations of a kind if not specified
+        if target_populations := validated_data.get("target_populations"):


May be more efficient to do this using set difference instead of nested loops

Good thought, although it doesn't matter here since if you look Kind has like 4 values. Probably the overhead from sets is more costly anyway

merging master in

refactor portal serializer + add email logic for updates on content

901c616

vcai122 requested review from judtinzhang, tuneerroy, dr-Jess and ashleyzhang01 April 25, 2024 05:45

vcai122 added 2 commits April 25, 2024 01:54

uncomment test stuff

5a19321

tests + clean everything else up

b17840b

judtinzhang reviewed Apr 26, 2024

View reviewed changes

vcai122 added 5 commits April 28, 2024 12:42

justin pr comments

eec7b9f

fix failing test

f4eb3c1

fix test

085f007

fix failing tests

698cba3

black

01b2858

vcai122 added 2 commits May 7, 2024 02:07

fix yarn lint

5149d2d

email tests

7dfc292

vcai122 requested a review from minghansun1 September 27, 2024 22:07

tuneerroy approved these changes Sep 30, 2024

View reviewed changes

minghansun1 reviewed Sep 30, 2024

View reviewed changes

vcai122 added 2 commits November 7, 2024 13:35

fix email message typo

5196663

Merge remote-tracking branch 'origin/master' into portal-email-notif

6949f0a

merging master in

vcai122 merged commit 85e98f0 into master Nov 7, 2024
9 checks passed

vcai122 deleted the portal-email-notif branch November 7, 2024 19:09



		def get_backend_manager_emails():
		if group := Group.objects.filter(name="backend_managers").first():

		return super().update(instance, validated_data)


		class PollSerializer(ContentSerializer):

		@@ -48,17 +50,54 @@ class Content(models.Model):
		admin_comment = models.CharField(max_length=255, null=True, blank=True)

Refactor portal serializer + Add email logic for updates on Content #291

Refactor portal serializer + Add email logic for updates on Content #291

Conversation

vcai122 commented Apr 25, 2024 • edited Loading

judtinzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

tuneerroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuneerroy Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghansun1 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

vcai122 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

minghansun1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcai122 commented Apr 25, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading

tuneerroy Sep 30, 2024 •

edited

Loading

minghansun1 Sep 30, 2024 •

edited

Loading

vcai122 Sep 30, 2024 •

edited

Loading