#161750658: Design User Invite Email template #163
Conversation
ipheghe
force-pushed
the
ft-create-email-template-161750658
branch
2 times, most recently
from
07dd568
to
4176a1e
Compare
There was a problem hiding this comment.
Great job on this implementation @ipheghe. I left some comments on how we may improve what we currently have. Also, a high level question which I want to direct to @brianbirir and @andela-sjames is:
Is it cool/okay for us to send emails to everyone interested in a category when an event is created in that category?
🥂
| sendNotifications=True, body=payload).execute() | ||
|
|
||
|
|
||
| def send_email(andela_user, event, receiver, info): |
There was a problem hiding this comment.
Great job identifying the need to make this function standalone. So I was thinking this function is better named send_event_invite_email as that reflects its current responsibility more accurately. The EmailMessage class already handles the core of email sending responsibility. Also, I was thinking we may want to separate the message sending responsibility from the message composition responsibility. As we can see below, the send_mass_email function repeats a lot of the message composition code in the send_email function. In order to ensure that we stay DRY, we may have a compose_invite_email function that addresses the message composition concern.
| return sent | ||
|
|
||
|
|
||
| async def send_mass_email(andela_user, event, info): |
There was a problem hiding this comment.
Just like above, we can have this method named as send_event_invite_emails since the implementation is very specific to sending event invite emails in bulk
| user__email=invitee['email']) | ||
| invite_hash = Hasher.gen_hash([ | ||
| event.id, receiver.user.id, sender.user.id]) | ||
| invite_url = info.build_absolute_uri( |
There was a problem hiding this comment.
I am not sure about the intended user flow here. I am thinking we should make this invite_url point to the frontend? or what do you think?
There was a problem hiding this comment.
Well, this was what was available before and it takes the user to the Invite Page on the Client side when it is clicked.
| for invitee in invitees] | ||
|
|
||
| for invitee in invitee_list: | ||
| receiver = AndelaUserProfile.objects.get( |
There was a problem hiding this comment.
We could prevent multiple database calls here by reusing the evaluated Queryset in line 130. To do this, our invitee_list could contain user objects instead of just emails.
| msg.content_subtype = 'html' | ||
| message_list.append(msg) | ||
|
|
||
| for item in message_list: |
There was a problem hiding this comment.
This loop may be very expensive for our requirements as it opens new connections to the email server for each message item. Thankfully, Django provides a way to handle bulk emails with a single connection. See here: https://docs.djangoproject.com/en/dev/topics/email/#sending-multiple-emails
| 'title': event.title, | ||
| 'imgUrl': event.featured_image, | ||
| 'venue': event.venue, | ||
| 'startDate': parser.parse(str( |
There was a problem hiding this comment.
Is there a way we can factor in the timezone of the invitee here? Also I am thinking we may not need the seconds part of the formatted date string %S
| @@ -232,7 +234,7 @@ def mutate_and_get_payload(cls, root, info, **input): | |||
| try: | |||
| receiver = AndelaUserProfile.objects.get( | |||
| user__email=receiver_email) | |||
| event = Event.objects.get(id=from_global_id(event_id)[1]) | |||
| event = Event.objects.get(id=event_id) | |||
There was a problem hiding this comment.
I am not sure why we changed this, but to the best of my knowledge, we should be sending/receiving global ids (like RXZlbnROb2RlOjQ0) to and from the client. This is why we have the from_global_id helper function from graphql_relay to resolve the expected global ids to actual database ids. Is that pattern still valid here?
There was a problem hiding this comment.
Yea true. Had issues testing the mutation with Insomnia GraphQL Playground, will change it back.
That's a good point @olusoladavid It's a good idea but this would work better if we enable users to opt in to receive emails in regards to the categories they're interested in. We should get their consent first. Maybe have a check box input field when they choose a category/ categories during sign up. It would have a message like this for example:
All in all, most times people are interested in being notified of events that are of interest to them. meetup.com for example, sends me emails of tech events occurring in Nairobi. In addition, there should be a settings section for managing email updates e.g. option for sending updates about new or suggested categories or group events, turn on or off email updates etc. |
| "username": "olusola.oguntimehin", | ||
| "first_name": "Olusola", | ||
| "last_name": "Oguntimehin", | ||
| "email": "olusola.oguntimehin@andela.com", |
There was a problem hiding this comment.
When committing code to a repo, it's advisable not to add actual data such as configurations or personal information. What's the purpose of adding this email and its user? I'm assuming this JSON file is just for pre-populating the database with information to test the application.
There was a problem hiding this comment.
@brianbirir The JSON file is to seed the database with information for testing purpose.
ipheghe
force-pushed
the
ft-create-email-template-161750658
branch
from
4176a1e
to
c15b1e8
Compare
This change addresses the need by: 1. Create html email template 2. Implement functionality to send email invite notification to users in the same category when a new event under the category is created. Delivers #161750658
ipheghe
force-pushed
the
ft-create-email-template-161750658
branch
from
c15b1e8
to
65e2194
Compare
What Does This PR Do?
This PR delivers the Invite Email Template
Description Of Task To Be Completed
Any Background Context You Want To Provide?
N/A
How can this be manually tested?
What are the relevant pivotal tracker stories?
#161750658
Screenshot(s)