Add new message emails #1321

joshsmith · 2017-12-20T07:29:59Z

What's in this PR?

Adds emails for messages.

References

begedin · 2017-12-20T15:25:03Z

lib/code_corps/emails/message_for_project_email.ex

+      conversation_url: project |> conversation_url(conversation),
+      name: user.first_name,
+      project_title: project.title,
+      subject: "#{user.first_name} has posted a reply on #{project.title}"


Different from the user email primarily because this could be a reply by any participant, including another admin/owner.

Feel free to word it differently.

I might change some language here but need to think on it.

Maybe:

"#{user.first_name} replied to a message in #{project.title}"

begedin · 2017-12-20T15:26:13Z

lib/code_corps/messages/emails.ex

+  """
+  @spec notify_of_new_message(Message.t) :: :ok
+  def notify_of_new_message(%Message{initiated_by: "admin"} = message) do
+    message = message |> Repo.preload(@message_preloads)


These explicit preloads are ugly, but it's an "internal" module, so it would be a matter of doing them here, or up higher, within the main messages context.

begedin · 2017-12-20T15:26:50Z

lib/code_corps/messages/emails.ex

+      [message: [:author, [project: :organization]]],
+      :user
+    ]
+  ]


Again, ugly, same as above.

In this case, I might actually prefer doing an explicit sql query instead.

begedin · 2017-12-20T15:29:09Z

lib/code_corps/messages/emails.ex

+  # we match against admin initiated only, because it's unclear who
+  # message.author is for user-initiated messages, or who the email recipient
+  # would be, assuming the message.author is the user who initiated the message
+  @spec send_message_for_project_emails(ConversationPart.t) :: :ok


As the comment says

who gets notified of an email sent by a user to a project? I know we don't have it yet, but still

who gets assigned as the message.author. Having it be identical to conversation.user seems redundant/confusing. Should we allow it blank in that case?

I think our conversation earlier got it right. Can you comment here what we said?

IIRC, the idea was

When a user messages a project, all project admins and owners receive an email.

When a project admin/owner replies to the conversation, from that point on, only participating users receive emails. That means whoever send it initially, whoever replied, and whoever decided to join in at a later time.

begedin · 2017-12-20T15:33:09Z

@joshsmith The logic is in, but I'm hoping it could be done a bit better. Does post mark allow specifying a separate template "submodel" for each recipient? That would allow us to just fetch a list of recipients and send the project email to all of them in one request.

Sparkpost allows something like that, so I was hoping Postmark would to.

begedin · 2017-12-20T15:34:16Z

@joshsmith Also, I'm not sure if only participants of the conversation should be notified of new replies, or if all project members of admin or higher should be. If we want to change that, Messages.Emails.get_project_participants/1 is the place to do it.

joshsmith

This needs the template ids to be added to the config.

This is also generally confusing to me. It looks like we're missing an email for a message from a user to a project (which admittedly we don't have in the front-end yet, so perhaps can come later). But I think the naming conventions here no longer match up with the original intention or the current usage.

It seems like we need three email types:

conversation replies, which go to all participants in a conversation
new message to a project (i.e. initiated_by user), which goes to all a project's admins
new message to some unknown number of n users (i.e. initiated_by admin), which goes to all the conversation's n users

Given that, we should maybe have emails that are:

ReplyToConversationEmail - currently MessageForProjectEmail
MessageInitiatedByProjectEmail - currently MessageForUserEmail
MessageInitiatedByUserEmail - currently not implemented

joshsmith · 2017-12-20T18:43:16Z

lib/code_corps/emails/message_for_project_email.ex

+      conversation_url: project |> conversation_url(conversation),
+      name: user.first_name,
+      project_title: project.title,
+      subject: "#{user.first_name} has posted a reply on #{project.title}"


Maybe:

"#{user.first_name} replied to a message in #{project.title}"

begedin · 2017-12-20T19:01:02Z

I think the 3 new types would make sense. In this case, that would mean

MessageToUserEmail -> MessageInitiatedByProjectEmail
MessageToProjectEmail -> ReplyToConversationEmail

Then we add MessageInitiatedByUserEmail at a later time

The logic would have to change a bit to. The Messages.add_part/1 would only send ReplyToConversationEmail emails, to all participants, except the author of the conversation.

Also note that with conversation part types, we might have to name it differently and word it a bit differently to.

joshsmith · 2017-12-20T19:06:51Z

I’ll get to work making the modifications then and create an issue to do the MessageInitiatedByUserEmail.

joshsmith · 2017-12-20T19:47:52Z

I think the maybe_send_message_for_user_email was overthinking what's needed to send a message to all relevant users in a conversation.

For sending a notification for the conversation part, we need to:

get all the authors for all the conversation parts
get the author of the message
get the user for the conversation

That should be everyone who's participating.

begedin · 2017-12-20T22:29:20Z

@joshsmith I figured it would be simpler with the new email types. Agree with the logic 👍

joshsmith assigned begedin Dec 20, 2017

joshsmith added the in progress label Dec 20, 2017

joshsmith added this to the ✉️ Messages milestone Dec 20, 2017

begedin reviewed Dec 20, 2017

View reviewed changes

joshsmith commented Dec 20, 2017

View reviewed changes

joshsmith assigned joshsmith and unassigned begedin Dec 20, 2017

joshsmith force-pushed the 1311-emails-for-messages branch from 59a8674 to 33fc482 Compare December 20, 2017 20:59

Add emails for messages and improve styles/layout generally

98e1ea0

joshsmith force-pushed the 1311-emails-for-messages branch from 33fc482 to 98e1ea0 Compare December 20, 2017 21:18

joshsmith added ready to merge and removed in progress labels Dec 20, 2017

joshsmith merged commit a95566e into develop Dec 20, 2017

joshsmith deleted the 1311-emails-for-messages branch December 20, 2017 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new message emails #1321

Add new message emails #1321

joshsmith commented Dec 20, 2017 •

edited

Loading

begedin Dec 20, 2017 •

edited

Loading

joshsmith Dec 20, 2017

joshsmith Dec 20, 2017

begedin Dec 20, 2017

begedin Dec 20, 2017

begedin Dec 20, 2017

joshsmith Dec 20, 2017

begedin Dec 20, 2017

begedin commented Dec 20, 2017

begedin commented Dec 20, 2017

joshsmith left a comment •

edited

Loading

joshsmith Dec 20, 2017

begedin commented Dec 20, 2017

joshsmith commented Dec 20, 2017

joshsmith commented Dec 20, 2017

begedin commented Dec 20, 2017

Add new message emails #1321

Add new message emails #1321

Conversation

joshsmith commented Dec 20, 2017 • edited Loading

What's in this PR?

References

begedin Dec 20, 2017 • edited Loading

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

begedin commented Dec 20, 2017

begedin commented Dec 20, 2017

joshsmith left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Dec 20, 2017

joshsmith commented Dec 20, 2017

joshsmith commented Dec 20, 2017

begedin commented Dec 20, 2017

joshsmith commented Dec 20, 2017 •

edited

Loading

begedin Dec 20, 2017 •

edited

Loading

joshsmith left a comment •

edited

Loading