Skip to content

Commit 34641f9

Browse files
MephisticMephistic
authored
fix(notification-emails): Add pagination/batching for email notification processing, add additional logging for email notifications, use explicit rather than implicit filter to ignore None notification frequency (#1923)
1 parent 5b368b2 commit 34641f9

File tree

2 files changed

+98
-66
lines changed

2 files changed

+98
-66
lines changed

‎functions/src/email/handlebarsHelpers.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ export const registerPartials = (directoryPath: string) => {
3434
}
3535

3636
export const prepareHandlebars = () => {
37+
console.log("Preparing handlebars helpers and partials")
3738
registerHelpers()
3839
registerPartials(path.join(__dirname, PARTIALS_DIR))
40+
console.log("Handlebars helpers and partials prepared")
3941
}

‎functions/src/notifications/deliverNotifications.ts‎

Lines changed: 96 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
import { prepareHandlebars } from "../email/handlebarsHelpers"
1919
import { Frequency } from "../auth/types"
2020

21+
const PROFILE_BATCH_SIZE = 50
2122
const NUM_BILLS_TO_DISPLAY = 4
2223
const NUM_USERS_TO_DISPLAY = 4
2324
const NUM_TESTIMONIES_TO_DISPLAY = 6
@@ -45,89 +46,116 @@ const getVerifiedUserEmail = async (uid: string) => {
4546
const deliverEmailNotifications = async () => {
4647
const now = Timestamp.fromDate(startOfDay(new Date()))
4748

48-
console.log("Preparing handlebars helpers and partials")
4949
prepareHandlebars()
50-
console.log("Handlebars helpers and partials prepared")
5150

52-
const profilesSnapshot = await db
51+
let numProfilesProcessed = 0
52+
53+
let profilesSnapshot = await db
5354
.collection("profiles")
5455
.where("nextDigestAt", "<=", now)
56+
.limit(PROFILE_BATCH_SIZE)
5557
.get()
5658

57-
console.log(
58-
`Processing ${
59-
profilesSnapshot.size
60-
} profiles with nextDigestAt <= ${now.toDate()}`
61-
)
59+
if (profilesSnapshot.empty) {
60+
console.log(
61+
`No profiles found with nextDigestAt <= ${now.toDate()} - sending 0 emails`
62+
)
63+
return
64+
}
6265

63-
const emailPromises = profilesSnapshot.docs.map(async profileDoc => {
64-
const profile = profileDoc.data() as Profile
65-
if (!profile || !profile.notificationFrequency) {
66-
console.log(
67-
`User ${profileDoc.id} has no notificationFrequency - skipping`
68-
)
69-
return
70-
}
66+
do {
67+
console.log(
68+
`Processing batch of ${
69+
profilesSnapshot.size
70+
} profiles with nextDigestAt <= ${now.toDate()} starting with ${
71+
profilesSnapshot.docs[0].id
72+
}`
73+
)
74+
numProfilesProcessed += profilesSnapshot.size
7175

72-
// TODO: Temporarily using email from the profile to test the non-auth issues
73-
// Should only use email from `auth` once that's working
74-
const defaultEmail = profile.email || profile.contactInfo?.publicEmail
75-
const verifiedEmail =
76-
(await getVerifiedUserEmail(profileDoc.id)) || defaultEmail
77-
if (!verifiedEmail) {
78-
console.log(
79-
`Skipping user ${profileDoc.id} because they have no verified email address`
76+
const emailPromises = profilesSnapshot.docs.map(async profileDoc => {
77+
const profile = profileDoc.data() as Profile
78+
if (
79+
!profile ||
80+
!profile.notificationFrequency ||
81+
profile.notificationFrequency === "None"
82+
) {
83+
console.log(
84+
`User ${profileDoc.id} has no notificationFrequency - skipping`
85+
)
86+
return
87+
}
88+
89+
// TODO: Temporarily using email from the profile to test the non-auth issues
90+
// Should only use email from `auth` once that's working
91+
const defaultEmail = profile.email || profile.contactInfo?.publicEmail
92+
const verifiedEmail =
93+
(await getVerifiedUserEmail(profileDoc.id)) || defaultEmail
94+
if (!verifiedEmail) {
95+
console.log(
96+
`Skipping user ${profileDoc.id} because they have no verified email address`
97+
)
98+
return
99+
}
100+
101+
const digestData = await buildDigestData(
102+
profileDoc.id,
103+
now,
104+
profile.notificationFrequency
80105
)
81-
return
82-
}
83106

84-
const digestData = await buildDigestData(
85-
profileDoc.id,
86-
now,
87-
profile.notificationFrequency
88-
)
107+
const batch = db.batch()
89108

90-
const batch = db.batch()
109+
// If there are no new notifications, don't send an email
110+
if (
111+
digestData.numBillsWithNewTestimony === 0 &&
112+
digestData.numUsersWithNewTestimony === 0
113+
) {
114+
console.log(
115+
`No new notifications for ${profileDoc.id} - not sending email`
116+
)
117+
} else {
118+
console.log(
119+
`Sending email to user ${profileDoc.id} with data: ${digestData}`
120+
)
121+
const htmlString = renderToHtmlString(digestData)
91122

92-
// If there are no new notifications, don't send an email
93-
if (
94-
digestData.numBillsWithNewTestimony === 0 &&
95-
digestData.numUsersWithNewTestimony === 0
96-
) {
97-
console.log(
98-
`No new notifications for ${profileDoc.id} - not sending email`
99-
)
100-
} else {
101-
console.log(
102-
`Sending email to user ${profileDoc.id} with data: ${digestData}`
103-
)
104-
const htmlString = renderToHtmlString(digestData)
105-
106-
const email = {
107-
to: [verifiedEmail],
108-
message: {
109-
subject: "Your Notifications Digest",
110-
text: convertHtmlToText(htmlString), // TODO: Just make a text template for this
111-
html: htmlString
112-
},
113-
createdAt: Timestamp.now()
123+
const email = {
124+
to: [verifiedEmail],
125+
message: {
126+
subject: "Your Notifications Digest",
127+
text: convertHtmlToText(htmlString), // TODO: Just make a text template for this
128+
html: htmlString
129+
},
130+
createdAt: Timestamp.now()
131+
}
132+
batch.create(db.collection("emails").doc(), email)
133+
134+
console.log(`Saving email message to user ${profileDoc.id}`)
114135
}
115-
batch.create(db.collection("emails").doc(), email)
116136

117-
console.log(`Saving email message to user ${profileDoc.id}`)
118-
}
137+
const nextDigestAt = getNextDigestAt(profile.notificationFrequency)
138+
batch.update(profileDoc.ref, { nextDigestAt })
139+
await batch.commit()
140+
141+
console.log(
142+
`Updated nextDigestAt for ${profileDoc.id} to ${nextDigestAt?.toDate()}`
143+
)
144+
})
119145

120-
const nextDigestAt = getNextDigestAt(profile.notificationFrequency)
121-
batch.update(profileDoc.ref, { nextDigestAt })
122-
await batch.commit()
146+
// Wait for all email documents to be created
147+
await Promise.all(emailPromises)
123148

124-
console.log(
125-
`Updated nextDigestAt for ${profileDoc.id} to ${nextDigestAt?.toDate()}`
126-
)
127-
})
149+
// Fetch the next batch of profiles
150+
profilesSnapshot = await db
151+
.collection("profiles")
152+
.where("nextDigestAt", "<=", now)
153+
.startAfter(profilesSnapshot.docs[profilesSnapshot.docs.length - 1])
154+
.limit(PROFILE_BATCH_SIZE)
155+
.get()
156+
} while (!profilesSnapshot.empty)
128157

129-
// Wait for all email documents to be created
130-
await Promise.all(emailPromises)
158+
console.log(`Finished processing ${numProfilesProcessed} profiles`)
131159
}
132160

133161
// TODO: Unit tests
@@ -239,6 +267,8 @@ const renderToHtmlString = (digestData: NotificationEmailDigest) => {
239267
path.join(__dirname, EMAIL_TEMPLATE_PATH),
240268
"utf8"
241269
)
270+
// TODO: Can we move the compilation up so we only compile the template
271+
// once per job run instead of once per email?
242272
const compiledTemplate = handlebars.compile(templateSource)
243273
return compiledTemplate(digestData)
244274
}

0 commit comments

Comments
 (0)