Implement email service#600
Conversation
|
[diff-counting] Significant lines: 1011. This diff might be too big! Developer leads are invited to review the code. |
anika-4444
left a comment
anika-4444
left a comment
There was a problem hiding this comment.
I think separating rejecting and cancelling might be a good idea. For the user, they convey different information (your ride not being scheduled vs your ride not happening when you expect it to) so perhaps that difference should also be reflected in the flow state?
Also, another thought for the future: now that we are adding an email service, should we think about other times an email could be auto-sent to users? When a user is first registered, campus wide closings, and holidays immediately come to mind.
| import nodemailer, { Transporter } from "nodemailer"; | ||
| import { format } from "date-fns"; | ||
|
|
||
| let transporter: Transporter; |
There was a problem hiding this comment.
More documentation for this page might be good? It's slightly unclear what the transporter does
…us from student page, removed report button
…cation email, cancellation
selenaliu1
changed the title
selenaliu1
changed the title
mjaydenkim
left a comment
mjaydenkim
left a comment
There was a problem hiding this comment.
This looks incredible, great work! Not approving just yet b/c I noticed a couple of small bugs including one seemingly breaking one, but this looks just about there -- great work <3
Also, side note -- do we wanna add emails for when rides are assigned to drivers? I think it's less necessary but if it's easy to implement it might be worth it. Not sure though.
| // Scheduling state - separate from operational status | ||
| export enum SchedulingState { | ||
| SCHEDULED = 'scheduled', | ||
| SCHEDULED_WITH_MODIFICATION = 'scheduled_with_modification', |
There was a problem hiding this comment.
when i went into the ride overview for a ride from the admin rides table and changed the ride from "scheduled" to "scheduled with modification" without making any other changes, i got the following error:
Screen.Recording.2025-12-04.145557.mp4
TypeError: Cannot read properties of undefined (reading 'startTime')```
There was a problem hiding this comment.
Thanks for the catch! Kind of debating what use there is to have this scheduling state here in the first place, since it should be done automatically and could potentially cause confusion, but I'm hesitant to fully remove it in cases when automatic detection might not correctly catch a scheduling state change, or other edge cases I haven't fully thought through. The error should be fixed now, and the logic now is that even if the scheduling state changes to scheduled with modification, it doesn't send an email unless an actual change relevant to the rider (time, pickup or dropoff location) is made.
There was a problem hiding this comment.
Yeah, I think that makes sense -- I think we should lean towards giving admins more leverage given how few there are and how flexible the current system is, but that solution sounds good to me.
| notify(ride, body, userType) | ||
| .then(() => res.send(ride)) | ||
| .catch(() => res.send(ride)); | ||
| notify(updatedRide, body, userType) |
There was a problem hiding this comment.
Updated rides seem to be sent as approved rides -- since these emails don't contain all the details this is a little confusing. Maybe add a new type of email for updated/modified rides? Probably not necessary though, might be more work than it's worth
There was a problem hiding this comment.
Good point! I changed the logic so that a scheduled --> scheduled ride with no change in scheduling state should not send another email (ie. scheduled --> scheduled), and only a meaningful change (scheduled --> scheduled with modification) would prompt an email. Similarly, for scheduled with modification --> scheduled with modification, an email is only sent if a meaningful change is made, otherwise don't send.
There was a problem hiding this comment.
That makes sense! I also noticed that changing the state back from "Scheduled_with_modifications" to "Scheduled" triggers a new email; I honestly don't think we need to worry about this though, it's a pretty small error.
mjaydenkim
left a comment
mjaydenkim
left a comment
There was a problem hiding this comment.
Did some more testing and everything looks great! This is such incredible work, tysm :') left like one or two comments but there's no real concerns
4 participants
Summary
This pull request is the first step towards implementing an email service to automate emails when a ride is approved, approved with modification, rejected, or cancelled.
Test Plan
Fermin created a VPN to test this on, I successfully received the emails! You won't get the emails unless you have access to the VPN and are connected to it.
Cancelled ride:
rejected ride:
scheduled with modification:
scheduled ride:
Notes
Kept console.logs in for now for debugging.
Breaking Changes