-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add webhook parser for NetCore (email) #2372
feat: add webhook parser for NetCore (email) #2372
Conversation
@scopsy @davidsoderberg the failing check is due to lack of default case in switch-case statement in |
@p-fernandez please take a look and let me know if any change is required. |
@abhilipsasahoo03 to solve the default case issue you can instead use the object as the map between enums like:
and then use it like:
|
@LetItRock The failing Javascript check has been resolved. Thanks for the suggestion, I'll keep that in mind. |
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.
🌟
Co-authored-by: Pablo Fernández <pablo@novu.co>
@p-fernandez I've made the changes. Please take a look. |
@scopsy @p-fernandez kindly review! |
Please add Line 200 in f5b92c5
|
@p-fernandez kindly review! |
The linter for providers integration checks failed:
In the |
@p-fernandez just came across that error in the automated test. I suppose it doesn't support bracket notation. I have made the required changes. |
@p-fernandez @scopsy please take a look. |
@scopsy @p-fernandez @davidsoderberg please take a look |
return EmailEventStatusEnum.UNSUBSCRIBED; | ||
case NetCoreStatusEnum.DROPPED: | ||
return EmailEventStatusEnum.DROPPED; | ||
case NetCoreStatusEnum.INVALID: |
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.
when do this case happen?
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.
According to the docs:
All API requests with syntactically incorrect email ids will be treated as Invalid and no further processing will be done on such ids.
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.
Hm @p-fernandez what do you think of adding an INVALID to our generic email event status enum?
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.
Netcore already explains in their documentation that a syntactically incorrect email generates a hard bounce. So I think we should use our BOUNCED status. They might send invalid just to narrow down the case for internal functionalities.
https://netcorecloud.com/tutorials/troubleshoot-email-bounce/#hard-bounce-email
|
||
return { | ||
status: status, | ||
date: new Date().toISOString(), |
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.
the body does not contain a date we can use?
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.
Here is a sample webhook from the docs:
[ {
"TRANSID": 15536250111702803, "RESPONSE": " - 2.0.0 (success)",
"EMAIL": "test@gmail.com",
"TIMESTAMP": 1553681625,
"FROMADDRESS": "info@mydomain.com",
"EVENT": "sent",
"MSIZE": 2155,
"X-APIHEADER": "UNIQUEID",
"TAGS": "mytag1"
} ]
we do have a timestamp here but no date.
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.
Okay then we keep it like you have it now 👍
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.
We can use the timestamp and generate the date from it.
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 we can do that by passing the timestamp to Date(). Correct me if I'm wrong. I'll look into it. :) @p-fernandez
@davidsoderberg @p-fernandez please review! :) |
👇 Click on the image for a new way to code review
Legend |
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.
🌟
What change does this PR introduce?
Why was this change needed?
Fixes [NV-1005] - Create webhook parser for NetCore (email) #1645
To give better answers why a message was not sent to a subscriber we need to get some info from the provider with a webhook.
Additional Context