665 consolidate emails #1161

esoterik · 2015-02-20T02:24:45Z

Resolves #665 and #938

orenyk · 2015-02-20T02:42:38Z

app/views/app_configs/_form.erb

@@ -59,6 +59,7 @@
    <%= f.input :enable_renewals, label: 'Enable renewals', hint: 'Allow users to renew reservations.' %>
  </fieldset>

+<<<<<<< HEAD


this looks leftover from a merge conflict :-P

orenyk · 2015-02-20T06:15:00Z

This looks really nice. I left a bunch of inline comments, the biggest of which is the refactoring of the two reservations controller methods (checkin_email and checkout_email) into one and the re-introduction of that functionality to our views. The only other comment I have is that I encountered an ERB warning in both _due_today.html.erb:1 and _starts_today.html.erb:1:

_starts_today.html.erb:1: warning: encountered \r in the middle of line, treated as a mere space

Not sure what that's all about, see if it's something we need to deal with. Great work!

orenyk · 2015-02-20T06:33:50Z

Also note that the query for the send_upcoming_checkin_reminder task in mailmain.rake has a typo (line 6). We need to add a space at the end of the line (after and) to make it work. Since we're fixing e-mails here I think it falls within scope.

esoterik · 2015-02-21T20:10:14Z

I fixed most of the issues you pointed out in the comments. I've refactored the checkin/checkout email methods, but it's failing quite a few tests so I haven't committed it yet. The parsing issues in the email bodies also have yet to be corrected.

esoterik · 2015-02-22T02:58:03Z

The tests no longer fail because of the refactored send_receipt method; tests still need to be written for the new method though.

esoterik · 2015-02-22T04:08:52Z

also--it seems like the \r issue is from a windows line ending character in the default messages for those two emails

orenyk · 2015-02-22T06:22:15Z

db/default_messages/upcoming_checkin_email

  @equipment_list@\n\n\
-  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 additioally have to pay a replacement fee of Replacement_fee@.\n\
+  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@.\n\


ha nice catch :-)

orenyk · 2015-02-22T06:43:50Z

Also, HTML line breaks in the app config settings are being escaped improperly in the e-mail body:

orenyk · 2015-02-22T06:48:13Z

Also, the checkout receipt fails when the reservation is due back today or is overdue but hasn't yet been returned (since the status is now "due today" or "overdue"). We might need to pass a receipt parameter into UserMailer#reservation_status_update to enforce the checked out status for those calls.

Note also that my comment above also applies to the logic determining whether or not to display the "send receipt" button.

orenyk · 2015-02-22T06:50:54Z

app/controllers/reservations_controller.rb

-  def checkin_email
-    if UserMailer.checkin_receipt(@reservation).deliver
+  def send_receipt
+    if UserMailer.reservation_status_update(@reservation).deliver
      redirect_to :back


This is no good since if we call this from the receipt view that is rendered by the checkout or checkin methods, then it tries to run checkout or checkin again resulting in a RecordNotFound exception. Why don't we redirect to the reservation in both cases?

orenyk · 2015-02-22T06:55:44Z

Nice job dealing with round 1, I left a few more inline comments and found a couple more issues while manually testing out this functionality (see above). Once those are addressed you should also play around with this branch and make sure everything works as expected (all buttons / links and all rake tasks). Hopefully we can get this done in tomorrow's meeting!

esoterik · 2015-02-28T04:03:27Z

okay, everything is fixed and working locally, about to force push onto this branch to fix the git issues, let's hope it works

esoterik · 2015-02-28T23:44:54Z

I think this is all set?

orenyk · 2015-03-01T04:45:28Z

Thanks, I'll have to review this tomorrow but hopefully we'll be able to get it in as well. What was your ultimate solution to fixing all the Git weirdness?

esoterik · 2015-03-02T00:31:30Z

I made a new branch locally, merged in 938, cherrypicked from 665, then force pushed to this branch haha

orenyk · 2015-03-02T01:45:07Z

app/mailers/user_mailer.rb

+      @status = 'due today'
+    end
+
+    if receipt && (@status == 'due today' || @status == 'overdue')


We should probably also check to make sure it's been checked out if the receipt parameter is true. An alternative way to structure this would be to write a second method send_receipt that runs that validation (e.g. return unless reservation.checked_out) and then calls reservation_status_update(reservation, true).

On that note, what happens if it's returned overdue? As written, it's not called a "receipt", but we can fix that in the view below (I'll leave a comment there).

perhaps the name "receipt" for the variable is misleading--I only wanted to use it to force sending a checkout receipt if requested from the reservations controller; other cases should work without the additional parameter

No I understand that, what I'm saying is that someone could end up issuing a request to the get_receipt route, passing a reservation that isn't yet checked out and therefore it should fail. Unless I missed it, we're not currently checking for that anywhere (so if you end up calling reservation_status_update(res, true) and res.checked_out == nil the e-mail view will probably fail since it will try to call strftime on nil). Does that make more sense?

right yeah, that was in response to returned overdue not being called a receipt. I think we're on the same page

orenyk · 2015-03-02T02:10:32Z

Once we've dealt with validating the receipt requests, I think this should be good to go (whew).

esoterik · 2015-03-02T02:26:58Z

once travis finishes this should be good!

orenyk · 2015-03-02T02:34:31Z

app/mailers/user_mailer.rb

+    end
+
+    if checkout_receipt && (@status == 'due today' || @status == 'overdue') &&
+       @reservation.checked_out


Hmm, I almost feel like we should return here if it's not checked out. As I see it:

If checkout_receipt (or receipt, I actually prefer the latter since we made one controller method for both cases)

If checked out and status weirdness then fix status

If checked out and status is checked out then do nothing

If not checked out, don't send the e-mail

If checked in, do nothing

Does that make sense?

orenyk · 2015-03-02T03:53:19Z

Oh no! What's the merge conflict? In either case, you can just squash rebase, then rebase onto master to resolve it. Good work!

generalized email methods spec for overhauled mailer replaced email methods everywhere don't send fine emails if there's no fine upcoming check out email added to unified email method add email checkout reminders task to the heroku scheduler refactored check in / out receipt methods

Resolves #665 and resolves #938

esoterik added the 4 - Needs Review label Feb 20, 2015

orenyk reviewed Feb 20, 2015
View reviewed changes

orenyk reviewed Feb 22, 2015
View reviewed changes

esoterik force-pushed the 665_consolidate_emails branch from 3058f59 to b88351b Compare February 28, 2015 04:06

orenyk mentioned this pull request Mar 2, 2015

1009 equipment status in category #1174

Merged

orenyk reviewed Mar 2, 2015
View reviewed changes

orenyk mentioned this pull request Mar 2, 2015

Make helper method for "send receipt" button #1178

Open

esoterik force-pushed the 665_consolidate_emails branch 2 times, most recently from 94dfc8d to 9304e63 Compare March 2, 2015 04:30

esoterik force-pushed the 665_consolidate_emails branch from 9304e63 to 404634c Compare March 2, 2015 04:52

orenyk added a commit that referenced this pull request Mar 2, 2015

Merge pull request #1161 from YaleSTC/665_consolidate_emails

3159bf4

Resolves #665 and resolves #938

orenyk merged commit 3159bf4 into master Mar 2, 2015

orenyk removed the 4 - Needs Review label Mar 2, 2015

orenyk deleted the 665_consolidate_emails branch April 22, 2015 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

665 consolidate emails #1161

665 consolidate emails #1161

esoterik commented Feb 20, 2015

orenyk Feb 20, 2015

orenyk commented Feb 20, 2015

orenyk commented Feb 20, 2015

esoterik commented Feb 21, 2015

esoterik commented Feb 22, 2015

esoterik commented Feb 22, 2015

orenyk Feb 22, 2015

orenyk commented Feb 22, 2015

orenyk commented Feb 22, 2015

orenyk Feb 22, 2015

orenyk commented Feb 22, 2015

esoterik commented Feb 28, 2015

esoterik commented Feb 28, 2015

orenyk commented Mar 1, 2015

esoterik commented Mar 2, 2015

orenyk Mar 2, 2015

esoterik Mar 2, 2015

orenyk Mar 2, 2015

esoterik Mar 2, 2015

orenyk commented Mar 2, 2015

esoterik commented Mar 2, 2015

orenyk Mar 2, 2015

orenyk commented Mar 2, 2015

665 consolidate emails #1161

665 consolidate emails #1161

Conversation

esoterik commented Feb 20, 2015

Choose a reason for hiding this comment

orenyk commented Feb 20, 2015

orenyk commented Feb 20, 2015

esoterik commented Feb 21, 2015

esoterik commented Feb 22, 2015

esoterik commented Feb 22, 2015

Choose a reason for hiding this comment

orenyk commented Feb 22, 2015

orenyk commented Feb 22, 2015

Choose a reason for hiding this comment

orenyk commented Feb 22, 2015

esoterik commented Feb 28, 2015

esoterik commented Feb 28, 2015

orenyk commented Mar 1, 2015

esoterik commented Mar 2, 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 Mar 2, 2015

esoterik commented Mar 2, 2015

Choose a reason for hiding this comment

orenyk commented Mar 2, 2015