1031 fix missed emails #1115
1031 fix missed emails #1115
Conversation
| @@ -6,7 +6,8 @@ task delete_missed_reservations: :environment do | |||
| unless AppConfig.first.blank? || AppConfig.first.res_exp_time.blank? | |||
| time = AppConfig.first.res_exp_time | |||
| missed_reservations = Reservation.where( | |||
| 'checked_out IS NULL and due_date < ?', Date.current - time.days) | |||
There was a problem hiding this comment.
Won't this only delete past reservations that weren't processed requests? In that case, all requests will just pile up in our database. We might want to approach this by dealing with the mailer task.
|
Ok, this exposed a fundamental issue with our dealings with missed reservations. We currently only send the missed reservation e-mail when the reservation is being deleted, not when it's been missed (since there might be something other than zero in |
|
working on that now |
|
We want all reservations (regardless of approval status) past the expiration time to be deleted, correct? |
|
Almost. Basically, any reservation that has not been checked out before its due date and its due date is however many days ago as set in |
|
Yes. I just want to clarify one thing: we only want to send emails when the reservation is missed, and not when it is deleted, right? Or are we sending emails both the day after the reservation is missed and then the missed reservation is deleted? |
|
Yup, no need to send an e-mail when the reservations are deleted. |
|
Okay, this should be working now! |
| # get all reservations that are approved and missed, send an email to | ||
| # user to inform them | ||
| missed_reservations = Reservation.where( | ||
| "checked_out IS NULL and approval_status <> 'denied' and due_date < ? ", |
There was a problem hiding this comment.
I think we want to exclude requests that were both denied (i.e. approval_status = 'denied') and those that simply weren't processed (i.e. approval_status = 'requested'). That means that we'll only be including those with an approval status of auto or approved. You can structure the query either way, I'd say it's probably better to use the equality since that way we don't unexpectedly send e-mails to people if we introduce a different approval_status option (e.g. pending).
|
Much better! I left a few inline comments; one thing that also needs to be done is to add the new rake task to our cron jobs so that it gets run nightly. The relevant file is |
|
Implemented changes from the comments, except I wasn't able to use the update method because the Reservation model has a different update method defined; I used update_attributes instead. |
|
Good catch; I know the method as |
|
@orenyk has code reviewed this |
test if a pending reservation will be deleted missed reservations query ignores pending reservations remove test for deleting pending reservations tests for sending emails for missed reservations renamed UserMailer#missed_reservation_deleted_notification to missed_reservation_notification new task for emailing about missed reservations delete_missed_reservations task no longer sends emails updated test to reflect only sending missed rervation emails for approved and auto reservations updated missed reservations email task to query for reservations that were approved or auto only check AppConfig to see if we want to send emails about missed reservations added email_missed_reservations task to cron jobs use scopes don't spam users Use rails logger instead of puts
esoterik
force-pushed
the
1031_fix_missed_emails
branch
from
3efdfcb to
bad8c03
Compare
Resolves #1031