[1240] email issues #1264

esoterik · 2015-05-28T05:36:42Z

Resolves #1240

I'm not 100% sure that I handled the admin reply-to / from issue in the best way (and it was a bit difficult to test), but I'm pretty sure this works.

orenyk · 2015-05-28T18:24:43Z

The default e-mails that are set when we run rake app:setup still have redundant salutations; that's because they're hard-coded in lib/tasks/setup_application.rake. We should just pull from the same files that we use in the seed script to minimize redundancy.

orenyk · 2015-05-28T18:33:41Z

app/views/user_mailer/_due_today.html.erb

This seems a bit excessive (the gsub) - we should just make sure that our default bodies don't have \n in them so that they don't show up for fresh instances. If an admin wants to put "\n" somewhere in their e-mails, we should let them.

The gsub is necessary because simple_format converts \ns to html linebreaks but does not remove them. I'm not sure how to handle it otherwise, unless there's another way of storing linebreaks in our defaults.

I suppose I could just write our defaults in html and skip the gsub and simple_format?

Not necessary, just replace the \n's with actual line breaks... simple 😄. We still need simple_format since we need to allow admins to enter their own copy, but we can definitely skip the gsub.

orenyk · 2015-05-28T18:47:43Z

app/mailers/user_mailer.rb

@@ -9,21 +9,26 @@ class UserMailer < ActionMailer::Base
  end

  # checks the status of the current reservation and sends the appropriate email
-  # receipt forces a check out receipt to be sent
-  def reservation_status_update(reservation, receipt = false) # rubocop:disable all
+  # force forces a check out receipt to be sent OR an approved request email,


I'm not thrilled with this; it's making the method a little difficult to understand and definitely makes it tricky to modify in the future. Here's an alternative suggestion - default force to an empty string and use that as a forced @status (allowing you to send any of the pre-defined statuses, overriding the current status). Therefore to send a receipt you'd call reservation_status_update(res, 'receipt') and to send the approval e-mail you'd call reservation_status_update(res, 'request approved'). Obviously you'd need some logic to infer from receipt whether nor not it was the checkout or checkin receipt, but this to me seems cleaner. Thoughts?

This sounds much better!

orenyk · 2015-05-28T18:49:34Z

Looks pretty good overall, just a few comments above. In terms of the modification to UserMailer#reservation_status_update, while it would be great if Reservations automatically figured everything out from the status, I feel like it results in less understandable / maintainable code. Let me know what you think, good work!

esoterik · 2015-05-28T21:43:49Z

Okay, so the only thing that should be left is determining the best way of handling linebreaks in our email messages -- I left a reply above

orenyk · 2015-06-09T03:18:34Z

Ok, so based on some tinkering locally you should be able to simply edit the default text and replace \n's with actual line breaks. I noticed that most of those text files are actually indented two spaces - you should get rid of that as well. Finally, I noticed that the default overdue e-mail has Reservation_id@ on line 9 instead of @reservation_id@ - do me a favor and just proofread the defaults to make sure that everything makes sense. Once that's done I'll give this a complete re-review and hopefully we can squash / merge. Good work!

esoterik · 2015-06-20T23:23:20Z

okay, should be good now!

orenyk · 2015-06-21T17:19:31Z

app/mailers/user_mailer.rb


-    return if !receipt && @status == 'returned' && @reservation.overdue &&
-              @reservation.equipment_model.late_fee == 0
+  VALID_STATUSES = ['checked out', 'denied', 'due today', 'missed', 'overdue',


Sorry to nitpick, but let's replace this with a %w() literal instead; I believe that's the preference in the style guide.

Oh yeah, forgot about that haha; I'm sort of surprised that rubocop doesn't check for that

Yea, I'm not sure why the build passed... maybe b/c it spans multiple lines?

Note that this was reverted since we have strings that include spaces within the array.

orenyk · 2015-06-21T18:18:21Z

Ok, basically there. Let's update the array of valid statuses to use the %w literal and add the missing line breaks in all of the templates. I'm pretty sure they'll make the e-mails look a bit more readable:

If you want to try and tackle the integration test, feel free, otherwise get those two changes in and we can squash 😄.

esoterik · 2015-06-21T21:29:11Z

I'll just fix the two smaller changes for now--if I had a bit more time, I'd do the integration test, but I'd rather not leave this hanging any longer

orenyk · 2015-06-21T22:42:55Z

app/mailers/user_mailer.rb


-    return if !receipt && @status == 'returned' && @reservation.overdue &&
-              @reservation.equipment_model.late_fee == 0
+  VALID_STATUSES = %w(checked\ out denied due\ today missed overdue


Ohh... that's probably why Rubocop didn't complain; some of the strings have spaces in them. Sorry, let's revert this and go back to using brackets (my bad!).

orenyk · 2015-06-21T22:44:55Z

Ok, sorry for the confusion. Just change the array back to a traditional bracketed array and squash your commits. I'll deal with the integration test in #1223.

esoterik · 2015-06-22T00:43:28Z

okay, done!

orenyk · 2015-06-22T01:14:33Z

@esoterik 🚨 😞

esoterik · 2015-06-22T01:19:36Z

argh rubocop...

esoterik · 2015-06-22T01:29:08Z

should be actually good to go now

orenyk · 2015-06-23T03:24:17Z

db/default_messages/upcoming_checkout_email

+@equipment_list@
+
+If you fail to return your equipment on time you will be subject to a late fee of @late_fee@ per day. If you have lost the item you may additionally have to pay a replacement fee of @replacement_fee@.
+Log in to Reservations to see if any of your items are eligible for renewal. If you have further questions feel free to contact an employee of @department_name@.


I feel like we could use an extra line break here and above.

orenyk · 2015-06-23T03:25:47Z

Alright this is good to go. I'm just going to add two line breaks to the default check-in and checkout today files and deal with the annoying \r errors, then merge this in. @esoterik, can you update the CHANGELOG? Nice work!

Resolves #1240 - test emails for approved requests that start today - fixed issue with emails for approved requests that start today - delete extra space from user email subject - remove doubled salutations - changed admin mailer from address - fix default emails - rework of user email method

orenyk · 2015-06-23T03:48:48Z

For future reference, I removed the pesky \r's from the following three files in app/views/user_mailer/:

_due_today.html.erb
_missed.html.erb
_starts_today.html.erb

The commands I used were (thanks SO):

tr -d '\r' < PATH/FILE > PATH/NEW_FILE_NAME
mv PATH/NEW_FILE_NAME PATH/FILE

[1240] email issues

orenyk · 2015-06-25T02:28:24Z

@esoterik scratch that, I'm just adding it to the CHANGELOG myself. Thanks!

orenyk added the 4 - Needs Review label May 28, 2015

orenyk changed the title ~~1240 email issues~~ [1240] email issues May 28, 2015

orenyk reviewed May 28, 2015
View reviewed changes

orenyk mentioned this pull request May 28, 2015

Reservation#approved? returns true for open requests #1267

Closed

orenyk reviewed May 28, 2015
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels May 28, 2015

esoterik added 4 - Needs Review and removed 1 - working labels May 28, 2015

orenyk added 1 - working and removed 4 - Needs Review labels Jun 9, 2015

esoterik added 4 - Needs Review and removed 1 - working labels Jun 20, 2015

orenyk reviewed Jun 21, 2015
View reviewed changes

esoterik added 1 - working 4 - Needs Review and removed 4 - Needs Review 1 - working labels Jun 21, 2015

esoterik force-pushed the 1240_email_issues branch from 9463971 to 86ec173 Compare June 21, 2015 21:52

orenyk reviewed Jun 21, 2015
View reviewed changes

esoterik force-pushed the 1240_email_issues branch from 86ec173 to be9efd5 Compare June 22, 2015 00:43

esoterik force-pushed the 1240_email_issues branch from be9efd5 to f227089 Compare June 22, 2015 01:28

orenyk reviewed Jun 23, 2015
View reviewed changes

orenyk force-pushed the 1240_email_issues branch from f227089 to c9ee647 Compare June 23, 2015 03:45

orenyk added a commit that referenced this pull request Jun 23, 2015

Merge pull request #1264 from YaleSTC/1240_email_issues

440904c

[1240] email issues

orenyk merged commit 440904c into master Jun 23, 2015

orenyk removed the 4 - Needs Review label Jun 23, 2015

orenyk deleted the 1240_email_issues branch October 27, 2015 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1240] email issues #1264

[1240] email issues #1264

esoterik commented May 28, 2015

orenyk commented May 28, 2015

orenyk May 28, 2015

esoterik May 28, 2015

esoterik May 28, 2015

orenyk Jun 9, 2015

orenyk May 28, 2015

esoterik May 28, 2015

orenyk commented May 28, 2015

esoterik commented May 28, 2015

orenyk commented Jun 9, 2015

esoterik commented Jun 20, 2015

orenyk Jun 21, 2015

esoterik Jun 21, 2015

orenyk Jun 21, 2015

orenyk Jun 23, 2015

orenyk commented Jun 21, 2015

esoterik commented Jun 21, 2015

orenyk Jun 21, 2015

orenyk commented Jun 21, 2015

esoterik commented Jun 22, 2015

orenyk commented Jun 22, 2015

esoterik commented Jun 22, 2015

esoterik commented Jun 22, 2015

orenyk Jun 23, 2015

orenyk commented Jun 23, 2015

orenyk commented Jun 23, 2015

orenyk commented Jun 25, 2015

[1240] email issues #1264

[1240] email issues #1264

Conversation

esoterik commented May 28, 2015

orenyk commented May 28, 2015

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

orenyk commented May 28, 2015

esoterik commented May 28, 2015

orenyk commented Jun 9, 2015

esoterik commented Jun 20, 2015

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

orenyk commented Jun 21, 2015

esoterik commented Jun 21, 2015

Choose a reason for hiding this comment

orenyk commented Jun 21, 2015

esoterik commented Jun 22, 2015

orenyk commented Jun 22, 2015

esoterik commented Jun 22, 2015

esoterik commented Jun 22, 2015

Choose a reason for hiding this comment

orenyk commented Jun 23, 2015

orenyk commented Jun 23, 2015

orenyk commented Jun 25, 2015