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
dburkhart07
left a comment
There was a problem hiding this comment.
Couple last things.
IMPORTANT: Wait for Yurika's clarification before making some changes to updateRequestStatus
| }); | ||
|
|
||
| describe('createOrder', () => { | ||
| describe('create', () => { |
There was a problem hiding this comment.
Can we add tests for Manufacturer has no donations path
|
|
||
| const orders = request.orders || []; | ||
|
|
||
| if (!orders.length) { |
There was a problem hiding this comment.
This check here really doesn't make sense to me. This would only happen in the case where a request is closed, but has no orders which should be impossible. I think in the case where we try to update a request that has no orders, we should honestly through a bad request exception saying that we can't update a request with no orders yet. Can we add a test for this too?
| if (request.status !== FoodRequestStatus.CLOSED) { | ||
| const wasAlreadyClosed = request.status === FoodRequestStatus.CLOSED; | ||
|
|
||
| if (!wasAlreadyClosed) { |
There was a problem hiding this comment.
Same thing here, this logic doesn't really make sense to me. I think if the request is already closed, then we should just throw a BadRequestException and return.
We should then remove the !wasAlreadyClosed check in the conditional, and set the status in the if statement, save the request, and then try catch with the email sending.
| }); | ||
| } | ||
|
|
||
| async updateRequestStatus(requestId: number): Promise<void> { |
There was a problem hiding this comment.
@Yurika-Kan are we even using this function anymore? its not currently used anywhere in our code.
| @@ -767,11 +882,53 @@ describe('RequestsService', () => { | |||
| }); | |||
|
|
|||
| it('should not reopen a closed request when updateRequestStatus is called', async () => { | |||
There was a problem hiding this comment.
Pending Yurika's clarification, I think we can probably delete this test. Regardless, if we do keep it, this logic should be tested for in the updateRequestStatus and should not be needed here.
ℹ️ 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):