Created a mailer for sending weekly assignment notifications by ryannoort · Pull Request #4 · yardstick/MrClean

ryannoort · 2017-10-20T19:58:02Z

This is for my Trello story:

As an employee, I want to receive email notifications, so that I know when I'm scheduled for.

jfeaver

You should try getting this going on production after making some of the changes here. We can ignore the normal workflow of merge to master then deploy to production. There will be some additional SMTP configuration, I imagine. I'd start by trying to implement the SendGrid addon: https://elements.heroku.com/addons/sendgrid

jfeaver · 2017-10-20T20:11:33Z

app/mailers/user_mailer.rb

@@ -0,0 +1,9 @@
+class UserMailer < ApplicationMailer
+  default from: "MrClean@mrclean.ca"


This should go in the ApplicationMailer.

We should talk to Dan about getting an official getyardstick.com email address for Mr. Clean.

jfeaver · 2017-10-20T20:13:19Z

app/mailers/user_mailer.rb

+  def assignment_reminder(employee)
+    @employee = employee
+
+    mail(to: 'smtp://localhost.1025', subject: "Test email")


to: employee.email

Subject should be finalized.

jfeaver · 2017-10-20T20:14:14Z

app/views/user_mailer/assignment_reminder.html.erb

+  <p>You can view the schedule <%= link_to "here", assignments_url %>. </p>
+
+  <p>Thanks,</p>
+  <p>Mr Clean</p>


jfeaver · 2017-10-20T20:14:45Z

app/views/user_mailer/assignment_reminder.html.erb

+  <p>Thanks,</p>
+  <p>Mr Clean</p>
+</body>
+</html>


This email is great!

jfeaver · 2017-10-20T20:17:45Z

app/views/user_mailer/assignment_reminder.html.erb

+<!DOCTYPE html>
+<html>
+<head>
+  <title></title>


This title can be removed.

We should add the content type meta tag here for utf-8. I'm not sure how email clients treat encoding of the email page but better safe than sorry.

jfeaver · 2017-10-20T20:18:09Z

app/views/user_mailer/assignment_reminder.html.erb

+<head>
+  <title></title>
+</head>
+<body>


Everything above here should go into a layout for emails. See the docs: http://guides.rubyonrails.org/action_mailer_basics.html#action-mailer-layouts

jfeaver · 2017-10-20T20:18:35Z

app/views/user_mailer/assignment_reminder.text.erb

@@ -0,0 +1,9 @@
+Hello <%= @employee.first_name %>


jfeaver · 2017-10-20T20:18:56Z

app/views/user_mailer/assignment_reminder.text.erb

+
+Thanks,
+
+Mr Clean


jfeaver · 2017-10-20T20:20:58Z

config/application.rb

    # Don't generate system test files.
    config.generators.system_tests = nil
+
+    config.action_mailer.default_url_options = { host: "localhost:3000" }


Can you also add this to the production config in the environments directory. In production, we should use ENV['HOST'].

jfeaver · 2017-10-20T21:53:12Z

app/views/user_mailer/assignment_reminder.html.erb

-  <p>Mr Clean</p>
-</body>
-</html>
+  <p>Mr. Clean</p>


Please de-indent.

jfeaver · 2017-10-20T21:53:34Z

config/environments/development.rb

  config.file_watcher = ActiveSupport::EventedFileUpdateChecker
+
+  config.action_mailer.delivery_method = :smtp
+  config.action_mailer.smtp_settings = { :address => "localhost", :port => 1025 }


jfeaver · 2017-10-20T22:42:05Z

Looks good as long as emails work on Heroku.

jfeaver · 2017-10-23T20:52:09Z

config/application.rb

    # Don't generate system test files.
    config.generators.system_tests = nil
+
+    #config.action_mailer.default_url_options = { host: "localhost:3000" }


Can you hear this code? It's saying "delete me! Delete me!"

jfeaver · 2017-10-23T21:02:07Z

config/environments/production.rb

+  config.action_mailer.smtp_settings = {
+    :user_name => ENV['SENDGRID_USERNAME'],
+    :password => ENV['SENDGRID_PASSWORD'],
+    :domain => 'https://mr-clean.herokuapp.com',


Can you get away with just providing the ENV['HOST'] here? Or, are there any other environment variables that work?

jfeaver

This seems fine to me if it makes things work. Did you ever figure out why ENV['HOST'] was causing problems?

…be used by heroku scheduler

…to AddedMailer

jfeaver · 2017-10-24T14:46:46Z

lib/tasks/mail_tasks.rake

+namespace :mail_tasks do
+  desc "rake task for heroku schedule to send out the weekly notifications."
+  task send_weekly_notifications: :environment do
+    if DateTime.now.wday == 1   #Heroku schedule only allows for every day task not every week so this ensures its only sent of monday


Wow. I'm really surprised Heroku doesn't let you use cron-based scheduling.

Ideally, I think I'd want to receive an email on Sunday night so that I see it at the start of the day and don't forget to unload the dishes on Monday morning.

if Date.today.sunday?

jfeaver · 2017-10-24T14:49:20Z

lib/tasks/mail_tasks.rake

+      week = Week.upcoming.first
+      assignments = Assignment.where(week_id: week.id)
+
+      assignments.each do |assignment|


Assignment.where(week: week).find_each do |assignment|

jfeaver · 2017-10-24T14:51:30Z

lib/tasks/mail_tasks.rake

+      assignments.each do |assignment|
+        employee = assignment.employee
+
+        UserMailer.assignment_reminder(employee).deliver


An assignment reminder should probably get the assignment. Who knows, maybe it needs more information than just who the employee is. So: UserMailer.assignment_reminder(assignment).deliver

jfeaver · 2017-10-24T14:55:46Z

lib/tasks/mail_tasks.rake

+        employee = assignment.employee
+
+        UserMailer.assignment_reminder(employee).deliver
+       end


Please put everything inside the if sunday? block into a new service called SendsAssignmentReminders. So the task is just:

SendsAssignmentReminders.call if sunday?

This is ideal because this rake task might be replaced by an ActiveJob, Sidekiq Worker, or any number of other task-runner objects. Also, we can decide to send reminders from anywhere in the app (i.e. maybe a manager wants to click a button to send an extra reminder).

jfeaver

I wanted to make some small changes before this went into production (particularly the email address) so I pulled down the code. I thought it would be faster but I got a little more carried away than I thought.

I'm pretty sure the changes should just work but you should pull down the changes and check them. I think the GeneratesNewWeekAssignments service could be improved a little more (might be combine the SQL queries) but it's good for now.

… than sunday

Created a mailer for sending weekly assignment notifications

a6b4015

ryannoort requested a review from jfeaver October 20, 2017 19:58

jfeaver reviewed Oct 20, 2017

View reviewed changes

Various changes

62c4059

ryannoort requested a review from jfeaver October 20, 2017 21:12

jfeaver suggested changes Oct 20, 2017

View reviewed changes

deindented email, and fixed a redirect path

80e1169

ryannoort requested a review from jfeaver October 20, 2017 22:10

ryannoort added 2 commits October 23, 2017 13:45

Added Sendgrid addon

683b573

Changed enviroment config files to add default url option for emails

f8ea85d

jfeaver reviewed Oct 23, 2017

View reviewed changes

ryannoort and others added 3 commits October 23, 2017 15:47

change enviorment variable names

5a4a85e

change domain for mailer to use enviroment variable

8e24817

removed commented out mailer config line

eaa431f

ryannoort requested a review from jfeaver October 23, 2017 22:13

jfeaver approved these changes Oct 23, 2017

View reviewed changes

ryannoort added 2 commits October 23, 2017 16:50

Added a rake task for sending all the emails for the week which will …

20c08c4

…be used by heroku scheduler

Merge branch 'AddedMailer' of https://github.com/ryannoort/MrClean in…

cfc7613

…to AddedMailer

jfeaver suggested changes Oct 24, 2017

View reviewed changes

created service for sending weekly mail notifications

b8b3bfd

ryannoort requested a review from jfeaver October 27, 2017 02:28

some clean up

6191463

jfeaver approved these changes Oct 27, 2017

View reviewed changes

Added rake task to skip conditional sending for testing on days other…

f337e12

… than sunday

ryannoort merged commit 021fe01 into master Oct 27, 2017

		@@ -0,0 +1,9 @@
		class UserMailer < ApplicationMailer
		default from: "MrClean@mrclean.ca"


		Thanks,

		Mr Clean No newline at end of file

Conversation

ryannoort commented Oct 20, 2017

Uh oh!

jfeaver left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfeaver commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfeaver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfeaver left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jfeaver left a comment •

edited

Loading