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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+352 −147
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
901c616
refactor portal serializer + add email logic for updates on content
vcai122 Apr 25, 2024
5a19321
uncomment test stuff
vcai122 Apr 25, 2024
b17840b
tests + clean everything else up
vcai122 Apr 25, 2024
eec7b9f
justin pr comments
vcai122 Apr 28, 2024
f4eb3c1
fix failing test
vcai122 Apr 28, 2024
085f007
fix test
vcai122 Apr 29, 2024
698cba3
fix failing tests
vcai122 May 7, 2024
01b2858
black
vcai122 May 7, 2024
5149d2d
fix yarn lint
vcai122 May 7, 2024
7dfc292
email tests
vcai122 May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions backend/pennmobile/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,11 @@
AWS_QUERYSTRING_AUTH = False
AWS_S3_FILE_OVERWRITE = False
AWS_DEFAULT_ACL = "public-read"

EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
EMAIL_HOST = os.environ.get("SMTP_HOST", "")
EMAIL_USE_TLS = True
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)
Copy link
Member

Choose a reason for hiding this comment

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

When would DEFAULT_FROM_EMAIL not be EMAIL_HOST_USER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

host user might be accounts@pennlabs.org and default from might be Penn Mobile <pennmobile@pennlabs.org>

20 changes: 20 additions & 0 deletions backend/pennmobile/templates/email.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Email Notification</title>
</head>
<body style="font-family: Arial, sans-serif; line-height: 1.6; margin: 0; padding: 0;">

<div>
<p>{{ message|linebreaksbr }}</p>

<hr style="border: 0; border-top: 1px solid #ddd; margin: 30px 0 20px 0;">

<em style="font-size: 12px; color: #666;">Please do not reply to this email. Replies to this email address are not monitored.</em>

</div>

</body>
</html>
36 changes: 36 additions & 0 deletions backend/portal/migrations/0016_poll_creator_post_creator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Generated by Django 4.2.9 on 2024-04-17 04:44

import django.db.models.deletion
from django.conf import settings
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
("portal", "0015_auto_20240226_2236"),
]

operations = [
migrations.AddField(
model_name="poll",
name="creator",
field=models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
to=settings.AUTH_USER_MODEL,
),
),
migrations.AddField(
model_name="post",
name="creator",
field=models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
to=settings.AUTH_USER_MODEL,
),
),
]
41 changes: 39 additions & 2 deletions backend/portal/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from django.db.models import Q
from django.utils import timezone

from utils.email import get_backend_manager_emails, send_automated_email


User = get_user_model()

Expand Down Expand Up @@ -48,17 +50,52 @@ class Content(models.Model):
admin_comment = models.CharField(max_length=255, null=True, blank=True)
target_populations = models.ManyToManyField(TargetPopulation, blank=True)
priority = models.IntegerField(default=0)
creator = models.ForeignKey(User, on_delete=models.SET_NULL, null=True, blank=True)

class Meta:
abstract = True

def _get_email_subject(self):
return f"[Portal] {self.__class__._meta.model_name.capitalize()} #{self.id}"

def _on_create(self):
send_automated_email.delay_on_commit(
self._get_email_subject(),
get_backend_manager_emails(),
(
f"A new {self.__class__._meta.model_name} for {self.club_code}"
f"has been created by {self.creator}"
),
)

def _on_status_change(self):
send_automated_email.delay_on_commit(
self._get_email_subject(),
getattr(self.creator, "email", None),
f"Your {self.__class__._meta.model_name} status for {self.club_code} has been"
+ "changed to {self.status}."
+ (
f"\n\nAdmin comment: {self.admin_comment}"
if self.admin_comment and self.status == self.STATUS_REVISION
else ""
),
)

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

Choose a reason for hiding this comment

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

nit: no need for return here since

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

yep up to you

if self.status != prev.status:
self._on_status_change()


class Poll(Content):
question = models.CharField(max_length=255)
multiselect = models.BooleanField(default=False)

def __str__(self):
return f"{self.id} - {self.club_code} - {self.question}"
return self.question


class PollOption(models.Model):
Expand All @@ -85,4 +122,4 @@ class Post(Content):
image = models.ImageField(upload_to="portal/images", null=True, blank=True)

def __str__(self):
return f"{self.id} - {self.club_code} - {self.title}"
return self.title
186 changes: 47 additions & 139 deletions backend/portal/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from django.http.request import QueryDict
from rest_framework import serializers

from portal.logic import check_targets, get_user_clubs, get_user_populations
from portal.models import Poll, PollOption, PollVote, Post, TargetPopulation
from portal.models import Content, Poll, PollOption, PollVote, Post, TargetPopulation


class TargetPopulationSerializer(serializers.ModelSerializer):
Expand All @@ -10,81 +11,69 @@ class Meta:
fields = "__all__"


class PollSerializer(serializers.ModelSerializer):
class ContentSerializer(serializers.ModelSerializer):
class Meta:
model = Poll
fields = (
"id",
"club_code",
"question",
"created_date",
"start_date",
"expire_date",
"multiselect",
"club_comment",
"admin_comment",
"status",
"target_populations",
)
read_only_fields = ("id", "created_date")
abstract = True

def create(self, validated_data):
club_code = validated_data["club_code"]
user = self.context["request"].user
# ensures user is part of club
if club_code not in [
x["club"]["code"] for x in get_user_clubs(self.context["request"].user)
]:
if not any([x["club"]["code"] == club_code for x in get_user_clubs(user)]):
raise serializers.ValidationError(
detail={"detail": "You do not access to create a Poll under this club."}
Copy link
Member

Choose a reason for hiding this comment

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

Have to use something like self.__class__._meta.model_name instead of just Poll here

)

# ensuring user cannot create an admin comment upon creation
validated_data["admin_comment"] = None
validated_data["status"] = Poll.STATUS_DRAFT

# TODO: toggle this off when multiselect functionality is available
validated_data["multiselect"] = False

year = False
major = False
school = False
degree = False

for population in validated_data["target_populations"]:
if population.kind == TargetPopulation.KIND_YEAR:
year = True
elif population.kind == TargetPopulation.KIND_MAJOR:
major = True
elif population.kind == TargetPopulation.KIND_SCHOOL:
school = True
elif population.kind == TargetPopulation.KIND_DEGREE:
degree = True

if not year:
validated_data["target_populations"] += list(
TargetPopulation.objects.filter(kind=TargetPopulation.KIND_YEAR)
)
if not major:
validated_data["target_populations"] += list(
TargetPopulation.objects.filter(kind=TargetPopulation.KIND_MAJOR)
)
if not school:
validated_data["target_populations"] += list(
TargetPopulation.objects.filter(kind=TargetPopulation.KIND_SCHOOL)
)
if not degree:
validated_data["target_populations"] += list(
TargetPopulation.objects.filter(kind=TargetPopulation.KIND_DEGREE)
validated_data["status"] = Content.STATUS_DRAFT

# auto add all target populations of a kind if not specified
auto_add_kind = [
Copy link
Member

Choose a reason for hiding this comment

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

Nice

kind
for kind, _ in TargetPopulation.KIND_OPTIONS
if not any(
population.kind == kind for population in validated_data["target_populations"]
)
]
validated_data["target_populations"] += TargetPopulation.objects.filter(
kind__in=auto_add_kind
)

validated_data["creator"] = user

return super().create(validated_data)

def update(self, instance, validated_data):
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it some more, I think if only the club_comment is changed, we shouldn't make it as a draft (Like if they just want to message us about something). Not sure if it is worth the hassle though. But regardless, we should tag the club_comment on the email if it is changed

Copy link
Member

Choose a reason for hiding this comment

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

Oh and on top of that, we definitely should not change the status/send an email if priority gets changed, since that is our own internal field. A few lines down there's the if not self.context["request"].user.is_superuser: ... That should prevent status update/sending email, but we probably need to change it to the mobile admin group instead of superuser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The serializer will only act on things coming in through the route, so operations directly through the shell or through admin portal does not go through the serializer. So admin comment/priority does not trigger the serializer.

With regards to club_comment change, I agree, probably don't need to set it to draft but it seems like a bit of an edge case (can't imagine people writing comments after post is approved), and kinda don't want to deal with writing that code + it'll be an extra query to db to get the previous object.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. Okay yea sounds good for the priority and admin comment. And sure we can keep as is for now

# if Poll is updated, then approve should be false
# if Content is updated, then approve should be false

Copy link
Member

Choose a reason for hiding this comment

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

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)

if not self.context["request"].user.is_superuser:
validated_data["status"] = Poll.STATUS_DRAFT
validated_data["status"] = Content.STATUS_DRAFT
return super().update(instance, validated_data)


class PollSerializer(ContentSerializer):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

class Meta(ContentSerializer.Meta):
model = Poll
fields = (
*ContentSerializer.Meta.fields,
"question",
"multiselect",
)


class PollOptionSerializer(serializers.ModelSerializer):
class Meta:
model = PollOption
Expand Down Expand Up @@ -204,7 +193,7 @@ class Meta:
)


class PostSerializer(serializers.ModelSerializer):
class PostSerializer(ContentSerializer):

image = serializers.ImageField(write_only=True, required=False, allow_null=True)
image_url = serializers.SerializerMethodField("get_image_url")
Expand All @@ -223,106 +212,25 @@ def get_image_url(self, obj):
else:
return image.url

class Meta:
class Meta(ContentSerializer.Meta):
model = Post
fields = (
"id",
"club_code",
*ContentSerializer.Meta.fields,
"title",
"subtitle",
"post_url",
"image",
"image_url",
"created_date",
"start_date",
"expire_date",
"club_comment",
"admin_comment",
"status",
"target_populations",
)
read_only_fields = ("id", "created_date", "target_populations")
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this out locally on the frontend (uploading a post with only a few target populations)? I don't remember precisely but I could've sworn there was a reason I put target_populations as read_only

Copy link
Member

Choose a reason for hiding this comment

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

Similarly would suggest testing sending over an image with a non-empty target_populations list

Copy link
Member

Choose a reason for hiding this comment

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

Also suggest test updating the image as well as the target_populations to both an empty and non-empty list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason you put target_population as read_only for Posts is because otherwise, the built in serializer tries to validate / set it, but it cannot succesfully validate the data coming from a form causing errors. (hence you marked it as read_only and then manually did it yourself after the fact).

I've tested post with image + non-empty and empty list together... after dealing with the edge case in is_valid in the Post serializer, it should be treating the target_population as any other field now

Copy link
Member

Choose a reason for hiding this comment

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

Bet thanks


def parse_target_populations(self, raw_target_populations):
if isinstance(raw_target_populations, list):
ids = raw_target_populations
else:
ids = (
list()
if len(raw_target_populations) == 0
else [int(id) for id in raw_target_populations.split(",")]
)
return TargetPopulation.objects.filter(id__in=ids)

def update_target_populations(self, target_populations):
year = False
major = False
school = False
degree = False

for population in target_populations:
if population.kind == TargetPopulation.KIND_YEAR:
year = True
elif population.kind == TargetPopulation.KIND_MAJOR:
major = True
elif population.kind == TargetPopulation.KIND_SCHOOL:
school = True
elif population.kind == TargetPopulation.KIND_DEGREE:
degree = True

if not year:
target_populations |= TargetPopulation.objects.filter(kind=TargetPopulation.KIND_YEAR)
if not major:
target_populations |= TargetPopulation.objects.filter(kind=TargetPopulation.KIND_MAJOR)
if not school:
target_populations |= TargetPopulation.objects.filter(kind=TargetPopulation.KIND_SCHOOL)
if not degree:
target_populations |= TargetPopulation.objects.filter(kind=TargetPopulation.KIND_DEGREE)

return target_populations

def create(self, validated_data):
club_code = validated_data["club_code"]
# Ensures user is part of club
if club_code not in [
x["club"]["code"] for x in get_user_clubs(self.context["request"].user)
]:
raise serializers.ValidationError(
detail={"detail": "You do not access to create a Poll under this club."}
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(
(
map(int, self.initial_data["target_populations"].split(","))
if "target_populations" in self.initial_data
else []
),
)

# Ensuring user cannot create an admin comment upon creation
validated_data["admin_comment"] = None
validated_data["status"] = Post.STATUS_DRAFT

instance = super().create(validated_data)

# Update target populations
# If none of a categories were selected, then we will auto-select
# all populations in that categary
data = self.context["request"].data
raw_target_populations = self.parse_target_populations(data["target_populations"])
target_populations = self.update_target_populations(raw_target_populations)

instance.target_populations.set(target_populations)
instance.save()

return instance

def update(self, instance, validated_data):
# if post is updated, then approved should be false
if not self.context["request"].user.is_superuser:
validated_data["status"] = Post.STATUS_DRAFT

data = self.context["request"].data

# Additional logic for target populations
if "target_populations" in data:
target_populations = self.parse_target_populations(data["target_populations"])
data = self.context["request"].data
raw_target_populations = self.parse_target_populations(data["target_populations"])
target_populations = self.update_target_populations(raw_target_populations)

validated_data["target_populations"] = target_populations

return super().update(instance, validated_data)
return super().is_valid(*args, **kwargs)
Loading
Loading