sumeetjain / outcomes-tracker

0 stars 0 forks source link

105 - User notifications on new Event creation #106

Closed halfghaninne closed 8 years ago

halfghaninne commented 8 years ago

Fixes #105

halfghaninne commented 8 years ago

I'm experimenting with Action Mailer on my local version of this project.

I'd expect that every time I add a comment to the single Entry I have locally, a message would be send to both you and I, Sumeet.

No messages are being sent, despite me experimenting with deliver_now AND deliver_later and despite my config/environments/development.rb file including:

  config.action_mailer.raise_delivery_errors = true
  config.action_mailer.perform_deliveries = true
sumeetjain commented 8 years ago

In development, I think messages aren't sent at all but are saved into tmp/ (since it's development). Can you check that folder?

sumeetjain commented 8 years ago

config.action_mailer.delivery_method = :file in config/environments/development.rb makes this happen.

halfghaninne commented 8 years ago

OH YEAH. Thank you. Only one is saved which is a little concerning, but I can figure that out I hope.

sumeetjain commented 8 years ago

For the get_users_in_conversation method in the Event model, I think a simpler approach would be defining an array with the self.entry.user and User.admins, and then removing self.user from the array... If I'm understanding that correctly, that is.

halfghaninne commented 8 years ago

self.user never needs to be in the Array, since the user who added the comment doesn't need to be notified of the comment.

self.entry.user only needs to be included if she is not the User making the the comment/Event (eg if you commented on Michelle's Entry, Michelle would need to be notified), which I think is expressed in:

if self.user != self.entry.user
   group << self.entry.user
end

Please let me know if I'm missing something from your comment!

sumeetjain commented 8 years ago

I follow the logic for why self.user never needs to be in the array. What I was suggesting is an alternative approach to writing the method body. It's not a major problem, but I think it'd be good to practice using more of Ruby's "higher order functions". For example, in this case, we can write the function like so:

def get_users_in_conversation
  group = User.admins + [entry.user]
  group.delete(self.user)
  return group
end

This saves us from the multiple conditionals you wrote to build the same functionality. Give it a read and let me know if you want to go over it. I'm going to start pushing for more idiomatic Ruby, since it's something we don't usually talk about much in our class.