-
Notifications
You must be signed in to change notification settings - Fork 204
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
Monthly activity email cron job #4400
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.
Called out some things to pay attention to.
esbuild.cronjobs.mjs
Outdated
format: "esm", | ||
outdir: "dist/cronjobs", | ||
sourcemap: true, | ||
target: "node20.12", |
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.
@rhelmer Here's another instance of the Node version we'd need to keep up-to-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.
Q: Should we start some document/wiki/README in this repo with all the places we need to bump Node.js versions (considering last week's sec releases and this week's scheduled sequel)?
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 it can be just node
or node20
if you want to be more specific - do you think it's worth specifying it here? The environment running the cron job (k8s CronJob
using the container we build in Dockerfile
) will be what's actually available in the environment.
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 I like about saying 20.12
is that it will show up on a project-wide search for that number, so that we won't forget to update it when updating the other instances. Using just 20
wouldn't be a problem for the coming future, but makes it easy to miss when we upgrade to Node 22.
As for the document/wiki/README, I'd actually like to write a small script that just checks all instances and errors out if they're not aligned, that we can run in CI.
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.
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 had to move this into a separate file (de1c3d1) because the other functions in the module referenced the Next.js-specific l10n module, which calls require.context
, which isn't available in plain Node.
* @param params.countryCode | ||
*/ | ||
export async function getSubscriberBreaches({ | ||
fxaUid, |
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 changed this to fxaUid
from user
because that was the only property we used, and user
was the user's session - which there is none of when running cronjobs.
Other than that I did not change this function.
@@ -102,7 +102,7 @@ export async function triggerMonthlyActivity(emailAddress: string) { | |||
const data = getDashboardSummary( | |||
latestScan.results, | |||
await getSubscriberBreaches({ | |||
user: session.user, | |||
fxaUid: session.user.subscriber?.fxa_uid, |
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.
See my comment at the function definition of getSubscriberBreaches
for why I changed this.
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.
This file is needed because, while import "server-only"
prevents us from loading a module in a client component, it also breaks when running in plain Node.
src/cronjobs/monthlyActivity.tsx
Outdated
process.env.MONTHLY_ACTIVITY_EMAIL_BATCH_SIZE ?? "", | ||
10, | ||
); | ||
const batchSize = Number.isNaN(batchSizeEnvVar) ? 10 : batchSizeEnvVar; |
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 figured I'd start with (configurable) small batches so that we can just run the cron job more often and not overload the server with a single big one, since we're doing multiple queries per user. Since this is limited to Plus users for now, there's not that many accounts we need to get through anyway. Also makes it easier to recover from errors.
src/cronjobs/monthlyActivity.tsx
Outdated
fxaUid: subscriber.fxa_uid, | ||
countryCode: countryCodeGuess, | ||
}); | ||
const data = getDashboardSummary(latestScan.results, subscriberBreaches); |
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 still have to check with Tony whether we want to show totals, or only specifics for the past month.
src/db/tables/subscribers.js
Outdated
query = query.andWhereRaw( | ||
`EXISTS ( | ||
SELECT 1 | ||
FROM jsonb_array_elements(fxa_profile_json->'subscriptions') AS subscription | ||
WHERE subscription::text = '"${MONITOR_PREMIUM_CAPABILITY}"' | ||
)` | ||
); |
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 know pretty little of SQL, so I'm not sure if this is the best way to check for rows where the fxa_profile_json
column has an object with a subscriptions
property containing an array of strings, of which one element is "monitor"
?
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'm not sure if it's any more efficient but there is a ?
operator https://www.postgresql.org/docs/9.4/functions-json.html that can check for existence:
WHERE (fxa_profile_json->'subscriptions')::jsonb ? 'monitor'
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.
Hmm, I think I did try to use that, but it got interpreted as a parameter - and then my PG UI didn't support it either. But you just gave me the push to verify that running it in psql
does work, and then digging through old Knex.js issues to find out how to escape it: ef2a47c
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.
It's probably best to ignore this file for now - it's basically the existing Next.js getL10n
, but with changes to make it compatible with plain Node. I've got a followup PR that untangles the five different runtimes that l10n currently has to take into account, and abstracts away the common parts.
@@ -2,6 +2,7 @@ | |||
* License, v. 2.0. If a copy of the MPL was not distributed with this | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
import React from "react"; |
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.
Next.js auto-imports React in JSX files, but esbuild doesn't (at least no by default, maybe there's an option somewhere).
esbuild.cronjobs.mjs
Outdated
format: "esm", | ||
outdir: "dist/cronjobs", | ||
sourcemap: true, | ||
target: "node20.12", |
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.
Q: Should we start some document/wiki/README in this repo with all the places we need to bump Node.js versions (considering last week's sec releases and this week's scheduled sequel)?
src/cronjobs/monthlyActivity.tsx
Outdated
|
||
async function run() { | ||
const batchSizeEnvVar = Number.parseInt( | ||
process.env.MONTHLY_ACTIVITY_EMAIL_BATCH_SIZE ?? "", |
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'm confused. Why nullish coalesce to empty string which will NaN and then default to 10
below on L28?
Could we default here to "10"
and skip L28, or am I missing something?
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.
Hmm, so I was changing it to your suggestion, but then remembered why I didn't do that in the first place - I did still want to guard against passing in an invalid value for the env var, i.e. check Number.isNaN
. But that does mean we'd ignore the env var value silently, so I'm now just throwing if it's wrong: 32e5b54.
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.
This looks great, I have some quibbles that are pretty minor. I know it's safe as-is but I really think we should always use parameterized queries for knex queries.
esbuild.cronjobs.mjs
Outdated
format: "esm", | ||
outdir: "dist/cronjobs", | ||
sourcemap: true, | ||
target: "node20.12", |
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 it can be just node
or node20
if you want to be more specific - do you think it's worth specifying it here? The environment running the cron job (k8s CronJob
using the container we build in Dockerfile
) will be what's actually available in the environment.
@@ -8,6 +8,7 @@ import AppConstants from '../../appConstants.js' | |||
|
|||
const knex = createDbConnection(); | |||
const { DELETE_UNVERIFIED_SUBSCRIBERS_TIMER } = AppConstants | |||
const MONITOR_PREMIUM_CAPABILITY = "monitor"; |
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.
Should this be in constants.ts
, since it's used in more than one place?
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.
src/db/tables/subscribers.js
Outdated
// Note: This will only match people who had a Plus subscription the last | ||
// time they signed in. We'd have to check with SubPlat directly to | ||
// find out if they have a subscription right 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.
The Monitor database is updated by webhook as well, so we don't necessarily need to wait until the user logs in again.
This comment is relevant for dev environments that don't have webhook handling set up e.g. localhost
.
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.
Oh right, good one - clarified the comment in 9c81d47.
src/db/tables/subscribers.js
Outdated
query = query.andWhereRaw( | ||
`EXISTS ( | ||
SELECT 1 | ||
FROM jsonb_array_elements(fxa_profile_json->'subscriptions') AS subscription | ||
WHERE subscription::text = '"${MONITOR_PREMIUM_CAPABILITY}"' | ||
)` | ||
); |
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'm not sure if it's any more efficient but there is a ?
operator https://www.postgresql.org/docs/9.4/functions-json.html that can check for existence:
WHERE (fxa_profile_json->'subscriptions')::jsonb ? 'monitor'
src/db/tables/subscribers.js
Outdated
let query = knex('subscribers') | ||
.select() | ||
.where((builder) => builder.whereNull("monthly_email_optout").orWhere("monthly_email_optout", false)) | ||
.andWhere(builder => builder.whereNull("monthly_email_at").orWhereRaw('"monthly_email_at" < NOW() - INTERVAL \'30 days\'')); |
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.
So this avoids re-sending because updateMonthlyEmailTimestamp()
is called which sets this, right? I just want to make sure we're thinking through that path if there are any exceptions, so we can't e.g. send duplicate emails because something there fails after the email is sent but before this timestamp is updated.
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.
Yes exactly, and that gets set before we send the email, so if an error occurs, the email won't get sent. Although given #4399 (comment), I'll probably need to create a new query once that lands.
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.
Thinking through this more, what if we:
- drop the columns (and data) of
monthly_email_optout
andmonthly_email_at
- add new cols
monthly_monitor_report: boolean
andmonthly_monitor_report_at:timestamp
Basically do a reset since we don't want to keep the old data
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.
Also I believe the data that exists should be inconsequential (very outdated, at least 6-12 months old)
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.
Yeah that works for me. Wouldn't really change the logic here, just different column names, and a new query to update the timestamp when I send an email. (Although I imagine we do want to keep existing opt-outs of monthly_email_optout
.)
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.
src/cronjobs/monthlyActivity.tsx
Outdated
@@ -0,0 +1,108 @@ | |||
/* This Source Code Form is subject to the terms of the Mozilla Public |
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.
Let's use src/scripts/cronjobs
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.
39704eb
to
f1faf2b
Compare
src/db/tables/subscribers.js
Outdated
// show up as errors when we remove it from the flag list: | ||
/** @type {import("./featureFlags.js").FeatureFlagName} */ | ||
const featureFlagName = "MonthlyActivityEmail"; | ||
const flag = await knex("feature_flags") |
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.
shouldn't we use a feature flag util function for this? If this specific one doesn't exist, maybe we should create it in the util file?
Also, for example, we need to make sure delete_at
is null, since we don't actually delete old feature flags but rather archive them
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.
Oh that's a good point, added that check in a util function in 1c425f7.
The main reason I initially shied away from the util function, is because I had to know both is_enabled
and the allow_list
, and thus was already replicating feature flag-specific logic. I've now created a function that does a check for the deleted_at
, but otherwise just returns the full row. We'll probably replace it by experiments later on anyway.
updated_at: knex.fn.now(), | ||
}) | ||
.where("primary_email", subscriber.primary_email) | ||
.andWhere("id", subscriber.id) |
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.
is it necessary to match both primary_email
and id
? Is there a case where these two won't match?
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.
This is me being overly cautious after our security issues where people were able to have us email other people by adding their email address - by matching on ID in addition to email address, if we're somehow looking at someone else with the same email address, it won't match. But matching on just the ID should be sufficient, of course. There's no practical downside to matching on both though, is that correct?
src/db/tables/subscribers.js
Outdated
/** | ||
* @param {string} email | ||
* @deprecated Only used by the `send-email-to-unresolved-breach-subscribers.js`, which it looks like might not be sent anymore? |
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.
* @deprecated Only used by the `send-email-to-unresolved-breach-subscribers.js`, which it looks like might not be sent anymore? | |
* @deprecated Delete as a part of MNTOR-3077 |
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.
src/db/tables/subscribers.js
Outdated
@@ -296,6 +349,7 @@ async function updateMonthlyEmailTimestamp (email) { | |||
* Unsubscribe user from monthly unresolved breach emails | |||
* | |||
* @param {string} token User verification token | |||
* @deprecated |
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.
* @deprecated | |
* @deprecated Delete as a part of MNTOR-3077 |
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.
src/emails/getEmailL10n.node.ts
Outdated
// const availableLocales = Object.keys(bundleSources); | ||
// const filteredLocales = | ||
// process.env.APP_ENV === "heroku" | ||
// ? availableLocales | ||
// : availableLocales.filter((locale) => supportedLocales?.includes(locale)); | ||
// const currentLocales = negotiateLanguages(languages, filteredLocales, { |
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.
// const availableLocales = Object.keys(bundleSources); | |
// const filteredLocales = | |
// process.env.APP_ENV === "heroku" | |
// ? availableLocales | |
// : availableLocales.filter((locale) => supportedLocales?.includes(locale)); | |
// const currentLocales = negotiateLanguages(languages, filteredLocales, { |
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.
That's the only property that's actually used, and this allows callers to also obtain it from elsewhere. This will especially be relevant when sending emails, when there's no user session, and thus the UID will have to be fetched from the database.
This will allow us to import that function without also importing the other modules imported in that file. This will be extra relevant for cron jobs, which don't run Webpack and thus will trip over `require.context` being present in l10n modules that get transitively included for one of the other functions.
I'll do a pass over the three different l10n loading strategies we have right now and extract the common bits, to hopefully make it a bit more maintainable. Gotta love five different runtimes! (Yes, that's not an exaggeration: plain Node (for emails), server components, client components, Storybook, and Jest.)
49c09b4
to
8c86502
Compare
This this is running in plain Node (i.e. not with the Next.js infrastructure), I had to make a couple of adjustments to some existing files. Specifically, files with JSX code need to explicitly import React, and code that shouldn't be run on the client-side can't use `import "server-only"`, because that only checks that it's being imported in a React server component. Additionally, I've added esbuild to bundle it up into a single file. Theoretically, this isn't really needed for Node.js scripts, since there are no HTTP requests we're trying to optimise. However, plain Node.js can't resolve module imports without file extensions (e.g. `"../db/tables/subscribers"`), so all import specifiers in *any* module that gets loaded by the cron jobs would need to use file extensions that match those of the generated files (i.e. .js rather than .ts), which is bound to cause its own issues in Next.js. Thus, esbuild is a compromise that can resolve these import specifiers for us.
This uses `tsx` ("TS execute") to strip the type annotations on the fly and then run the resulting output in Node. It uses ESBuild behind the scenes, so the results should be consistent with the built JS in production.
8c86502
to
93f3577
Compare
References:
Jira: MNTOR-3067
Figma: N/A
Description
This adds a
MonthlyActivityEmail
flag and a script that will send the monthly activity email to 10 (or$MONTHLY_ACTIVITY_EMAIL_BATCH_SIZE
) Plus subscribers who have that flag enabled, whosemonthly_email_optout
is nottrue
(@mansaj - is this column actually boolean, or is this the one where I need to check for0
or1
or-1
or something) and to whom we've not sent such an email in the last 30 days.Importantly, it also sets up the build infrastructure to compile JSX and TypeScript for cron job scripts (though note that I haven't updated our cron jobs configuration yet to call the script - which will be at
dist/cronjobs/monthlyActivity.js
). The main thing here to realise is that this means that some code is now built by Next.js (using Webpack and SWC) when shipped as part of the website, and by ESBuild when shipped as part of the cron job - and that these have their own peculiarities. For example, JSX files that are also used in cron jobs need to explicitly import React, need to use a wrapper aroundimport "server-only"
, need to load Fluent differently (see also my followup PR), and don't have access to variables from.env
(possibly we should rundotenv
for cron jobs like we do for the e2e tests).I'll call out some more specific things inline.
How to test
Enable the
MonthlyActivityEmail
flag for your user, then runnpm run dev:cronjobs
. You may want to reset the value of theirmonthly_email_at
column afterwards if you want to retry.Checklist (Definition of Done)