-
Notifications
You must be signed in to change notification settings - Fork 2
PIN-7887 add user email resolution through SelfCare API in email sender #2474
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
PIN-7887 add user email resolution through SelfCare API in email sender #2474
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.
Looks good to me! (of course the other handlers must be updated)
if (resp.length > 1) { | ||
loggerInstance.error( | ||
`Multiple users (${resp.length}) found in Selfcare for userId ${userId} and tenant ${tenantId}` | ||
); | ||
return undefined; | ||
} | ||
|
||
const email = resp[0].email; | ||
if (!email) { | ||
loggerInstance.error( | ||
`User ${userId} in tenant ${tenantId} has no email in Selfcare` | ||
); | ||
return undefined; | ||
} |
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 agree with the error handling choices
The only issue could be that, for the logger.error + discard cases, if they occur because of a Selfcare API change that affects all users, we could discard many notifications before we realize the error. However, if they occur for individual users (due to dirty data in the Selfcare source of truth), this approach is best to avoid stopping sending emails entirely. That seems to me the most likely case in which this occurs, so I think it makes sense to treat them as done here. In the less likely scenario of an uncommunicated API schema change, we could always reprocess the events to resend the discarded notifications
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.
👍 just one minor comment
…entToEmailPayload
No description provided.