SSF-188 Automated Order Lifecycle Emails#162
Conversation
sam-schu
left a comment
There was a problem hiding this comment.
For sending the emails:
- For consistency with our other emails, can we send the emails outside of the transaction, so the order is still created even if an email fails to send? (We do plan to add better handling for failed emails across the board)
- Can we try sending both emails, even if one fails?
sam-schu
left a comment
There was a problem hiding this comment.
Confirmed the emails are properly sent! A few comments on the contents:
- Can we link the text "log into the platform" rather than including the link at the end of the email?
- Can we turn the coordinator email into a link?
- For the manufacturer email, can we make sure the different address lines appear on different lines in the email body, and add a comma between city and state?
814 Cedar Hollow Way
[Unit 1]
Madison, WI 53711
[US]
dburkhart07
left a comment
There was a problem hiding this comment.
sorry this is so many comments lol. should hopefully be good after this <3
| for (const donationItem of donationItems) { | ||
| const id = donationItem.itemId; | ||
| const quantityToAllocate = itemAllocations.get(id)!; | ||
| const invalidItems = donationItems.filter( |
There was a problem hiding this comment.
I'm thinking about the edge case where we have a bunch of fulfilled donations. In the instance where we have donation items part of this filled donation, we will get an issue about the desired quantity being more than the fulfilled quantity. Can we get either @sam-schu or @Yurika-Kan's thoughts on this? should we keep it the way it is, or throw a potentially more helpful error that no donation item ids can belong to fulfilled donations, in addition to needing to be from donations from this food manufacturer.
There was a problem hiding this comment.
@dburkhart07 To clarify, are both of the following true:
- this is a preexisting error that does not have to do with the changes from this PR for the email
- the existing error message does not provide incorrect information
If so, I don't think the additional effort to improve the error message is necessary at this point, particularly since the full text of the error is not shown in the frontend (and this error should only occur from bad API calls)
| await this.dataSource.transaction(async (transactionManager) => { | ||
| validateId(manufacturerId, 'Food Manufacturer'); | ||
| validateId(requestId, 'Request'); | ||
|
|
There was a problem hiding this comment.
Can we also validate early on that all the allocation values are greater than 0?
| `, | ||
| }), | ||
|
|
||
| pantryRequestClosed: (params: { |
There was a problem hiding this comment.
for this email, can we also send to pantry coordinator ie volunteer here?
client note in the email doc: ensure both pantry coordinator that is assigned to the donation and the pantry is attached to this email.
we should make it so we only send one at a time, so that recipients will not be able to see all other recipients
ℹ️ Issue
Closes https://vidushimisra.atlassian.net/jira/software/projects/SSF/boards/1?jql=assignee%20%3D%20712020%3A10ef9ad9-e290-4bbd-8c4b-cb215e8d449a&selectedIssue=SSF-188
📝 Description
Email templates for:
✔️ Verification
BEFORE TESTING: Add 2 new environment variables:
AWS_SES_SENDER_EMAIL (set this to one of your emails, this will be the address that sends the emails for you to verify)
SEND_AUTOMATED_EMAILS (switch this to true to turn on Cognito account creation and email sending permissions)
Add your email that you put in the AWS_SES_SENDER_EMAIL into the following AWS SES Identities: https://us-east-2.console.aws.amazon.com/ses/home?region=us-east-2#/identities
Tested each workflow to ensure the proper sender, subject, message, attachments were all there.
Same testing as #127
I verified it worked via new tests and testing email functionality with postman.
🏕️ (Optional) Future Work / Notes
For food request closed the functionality is as follows (per Yurika):
For manual close (closeRequest endpoint):
For auto close (updateRequestStatus):