feat: Add endpoint to send newsletter consent email #645
Conversation
Needed to prevent potential infinite loop
|
✅ Tests will run for this PR. Once they succeed it can be merged. |
Not quite sure, what this value is for, but it is better to have it like this for now
sashko9807
added
the
run tests
github-actions
bot
removed
the
run tests
| } | ||
|
|
||
| private updateMailListMap( | ||
| regUser: Person[], |
| regUser: Person[], | ||
| contacts: ContactsMap, | ||
| skipAfterDate: Date, | ||
| unregisteredConsent: UnregisteredNotificationConsent[], |
There was a problem hiding this comment.
unregisteredConsent -> unregisteredConsents
| // Remove email if it belongs to user created after the change has been deployed, as they had already decided | ||
| // whether to give consent or not. | ||
| if (contacts.get(registeredUser.email as string) && createdAt > skipAfterDate) { | ||
| Logger.debug(`Removing email ${registeredUser.email} from list`) |
There was a problem hiding this comment.
Can we add a bit more in the log:
removing email XXX from the list because of date
and same later below:
removing email XXX because the user don't want them.
|
|
||
| do { | ||
| Logger.debug('Waiting export to be finished') | ||
| await new Promise((r) => setTimeout(r, SENDGRID_EXPORT_TIMEOUT)) |
There was a problem hiding this comment.
I don't understand how this works... so we wait 10 seconds every time?
Seems very strange
There was a problem hiding this comment.
Sendgrid doesn't have API to extract emails from list as of now, thus we need first to create export file, which takes approx. ~10-15 seconds.
| `Couldn't export contacts within the limit. Try again later.`, | ||
| ) | ||
| } | ||
| const exportUrl = exportStatusResponse.urls[0] |
There was a problem hiding this comment.
can we check if the urls list is non empty?
There was a problem hiding this comment.
Will do, though if export status is succeeded it always returns an url.
apps/api/src/notifications/providers/notifications.sendgrid.provider.ts
Outdated
Show resolved
Hide resolved
| const currentDate = new Date() | ||
| contactsMap.forEach((contacts, index) => { | ||
| //Schedule batches in a minute difference | ||
| currentDate.setMinutes(currentDate.getMinutes() + index) |
There was a problem hiding this comment.
This will result in current date with the following values (say it is now 11:00):
11:00
11:01
11:03
11:06
11:10
11:15
11:21
...
If I am reading this correctly.
It will probably work, I am just not sure this was the intent, especially if we expect this list to be big...
There was a problem hiding this comment.
Will have to revisit whether the scheduling part works as intended , but the idea is to schedule each call in a minute difference, due to Sendgrid Rate limiting. eg:
First batch of emails: 10:00
Second batch of emails: 10:01
Third batch of emails: 10:02
Big lists are not a problem since those emails are sent in bulk. What the implementation does is the following:
Get contacts data from list -> Split contact's data into chunks, with each chunk having up to 1000 emails -> Schedule mail send for the emails within each chunk.
Though thinking about it we should probably have a way to cancel the mail sent at some point.
There was a problem hiding this comment.
Lets test this in staging/prod environment? We already have a private contact list, to test this internally first?
…ndgrid-marketing-email
sashko9807
added
the
run tests
github-actions
bot
removed
the
run tests
sashko9807
added
the
run tests
github-actions
bot
removed
the
run tests
Adds an endpoint to send newsletter consent mails.