Conversation
sam-schu
left a comment
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
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]
| transactionManager, | ||
| ); | ||
|
|
||
| const associatedDonationIdsSet = |
There was a problem hiding this comment.
Correct me if Im wrong, but I think the associatedDonationIdSet can just be found from the earlier donationItems in this function, since that is used from the itemAllocations that get created right before this.
| <p> | ||
| Good news! Your recent food request through Securing Safe Food has been successfully matched to an order and is now moving forward toward delivery. | ||
| </p> | ||
| <p> |
There was a problem hiding this comment.
I think we need to pull the <ul> element outside of the <p> element (same with the fmDonationMatchedOrder below)
| Array.from(associatedDonationIdsSet), | ||
| transactionManager, | ||
| if (emailErrors.length > 0) { | ||
| this.logger.warn( |
There was a problem hiding this comment.
For consistency right now, perhaps we should do what all other email errors are doing, which is an InternalServerErrorException instead?
| donationItemIds, | ||
| ); | ||
| try { | ||
| const pantryAddress = [ |
There was a problem hiding this comment.
2 things:
Should this not be the shipping address information instead, if it exists and is different from the mailing address? I'd assume the food, if it has a different address, should be getting shipped there.
Rather than this filter with a Boolean, since not everything needs a
element, can we just make it into:
const pantryAddress = ${request.pantry.mailingAddressLine1}
${request.pantry.mailingAddressCity}, ${request.pantry.mailingAddressState} ${request.pantry.mailingAddressZip}${request.pantry.mailingAddressCountry ?
${request.pantry.mailingAddressCountry} : ''};
This should have the address on one line, city, state, and zip on another, and country on a separate line if provided.
| }); | ||
|
|
||
| const pantry = request!.pantry; | ||
| const pantryAddress = [ |
There was a problem hiding this comment.
We will need to make the same change here as in the order.service file
|
|
||
| // Testing emails section | ||
|
|
||
| const assignee = await usersRepo.findOne({ where: { id: userId } }); |
There was a problem hiding this comment.
Can we cast each of these to a User, Request, and FoodManufacturer so we dont need to keep using ! below?
| [manufacturer!.foodManufacturerRepresentative.email], | ||
| fmSubject, | ||
| fmBodyHtml, | ||
| ); |
There was a problem hiding this comment.
Can we also add 2 more tests? One where either or of the pantry email and manufacturer email fails (you choose) and one where they both fail? For both of these, we should make sure that the order is still created.
| [manufacturer!.foodManufacturerRepresentative.email], | ||
| fmSubject, | ||
| fmBodyHtml, | ||
| ); |
There was a problem hiding this comment.
Can we also add tests for the allocations being created, and the donation service doing proper matching? We don't need to test the actual output, but rather just make sure that the functions are called once each and with the proper parameters that we expect them to be.
| [manufacturer!.foodManufacturerRepresentative.email], | ||
| fmSubject, | ||
| fmBodyHtml, | ||
| ); |
There was a problem hiding this comment.
Sorry for so many additional tests requests. Can we also test for transaction doing a rollback for all changes if an error occurs on allocation creation (and that the order is not actually created in this case)?
| await requestRepo.save(request); | ||
|
|
||
| validCreateOrderDto.foodRequestId = 2; | ||
| await expect( |
There was a problem hiding this comment.
Can we combine these into one expect for a full BadRequestException with the right error message?
3 participants
ℹ️ 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
N/A