-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add reply to support for aws ses provider #3131
fix: add reply to support for aws ses provider #3131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to add test for this addition for confidence.
@@ -12,6 +12,7 @@ const mockConfig = { | |||
|
|||
const mockNovuMessage = { | |||
to: ['test@test2.com'], | |||
replyTo: 'test@test1.com', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is not enough to test if replyTo
is being configured in SES.
In line 96 we should modify the spy to check that the proper params and values are passed by checking with the method toHaveBeenCalledWith
and the values passed in the mockNovuMessage
.
Unfortunately it wasn't done properly before but if not doing it we can't validate this change. 🙁
btw. @p-fernandez Not sure if you have been notified, but I have updated the PR, can you please check? |
Looks great! |
What change does this PR introduce?
Why was this change needed?
replyTo
wasn't working for AWS SES provider. This PR fixes that bug.Closes #3122