Breach alert email redesign #5012

Vinnl · 2024-09-03T10:27:16Z

References:

Jira: MNTOR-3433
Figma: https://www.figma.com/design/Z7pfHXJTSG5Szm3UrkYsVx/Emails-v2.0?node-id=1-697&m=dev

Description

Implements a new design for the breach alert email. I still have a couple of open question, but I've made some best guesses at what the answers will be.

I've assigned the whole team for review since it's both UI and backend changes.

Screenshot (if applicable)

Non-US:

US free, no scan:

US free, scan has run:

Plus user:

How to test

Most realistic is using /admin/emails to send a message to yourself - I've added a button there for the redesigned email specifically. Storybook has a bunch of the permutations.

Checklist (Definition of Done)

Localization strings (if needed) have been added.
Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
I've added a unit test to test for potential regressions of this bug.
If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Vinnl · 2024-09-03T10:30:57Z

public/images/email/footer-bg-shapes.png

Of note: theoretically we'll never be able to delete these images, since once sent, emails will keep referring them. However, these backgrounds have OK'ish fallbacks in plain colours, so removing them wouldn't be the end of the world.

Vinnl · 2024-09-03T10:32:39Z

src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/fix/page.tsx

The changes in this file aren't really necessary; at first I thought I had to drop people into the guided flow, and didn't want to redirect to a specific step in the email in case the user resolves it. Thus, I updated plain /fix/ (which we're not really using) to redirect to the most relevant step.

Turns out I didn't need that, but figured this was still better than blindly redirecting to the free scan, regardless of whether the user has already run one, or is in the US.

Vinnl · 2024-09-03T10:35:13Z

src/emails/templates/mjml.d.ts

@@ -186,13 +186,21 @@ declare namespace React.JSX {
        "background-position"?: CSSProperties["backgroundPosition"];
        "background-position-x"?: CSSProperties["backgroundPositionX"];
        "background-position-y"?: CSSProperties["backgroundPositionY"];
-        "background-repeat"?: CSSProperties["backgroundRepeat"];
+        "background-repeat"?: Extract<


MJML threw a couple of errors, such as apparently not supporting repeat-y for background-repeat. Presumably because some email clients don't support it...

Vinnl · 2024-09-03T10:41:57Z

src/scripts/cronjobs/emailBreachAlerts.tsx

+                const scanData = await getLatestOnerepScanResults(
+                  recipient.onerep_profile_id,
+                );
+                const allSubscriberBreaches = await getSubscriberBreaches({
+                  fxaUid: recipient.fxa_uid,
+                  countryCode: assumedCountryCode,
+                });


@rhelmer @mansaj I wanted to draw some extra attention to these - I'm not sure how many subscribers we might be sending emails for in a single poll, but the email can now show the number of exposed data points for the user, which means I have to fetch all scan results and breaches for each of them. Just wondering if this won't cause excessive load and, if so, how best to deal with that?

Here's some background info, I'm going to add advice as a separate comment but in case this is useful:

It's a bit hard to predict how many subscribers we'll email for a single poll() invocation - there's some analysis of the existing HIBP anonymized dataset here https://blog.cloudflare.com/validating-leaked-passwords-with-k-anonymity/

The receivedMessage being processed here is the same hash prefix + suffixes described in that blog post. Our system additionally only sends if certain criteria are met, and also of course each hash must match an email in the Monitor database so in practice we send fewer than the average 478 (max 584) cited there.

However the possible number is quite large:

it is worth noting that as the length of a SHA-1 hash is a total of 40 hexadecimal characters long and 5 characters is utilised by the Hash Prefix, the total number of possible hashes associated with a Hash Prefix is 16^{35} ≈ 1.39E42.

We run the jobs that process these messages in parallel, so it's OK if one takes a while, and it can be safely resumed if interrupted. So I wouldn't worry too much about the edge cases here, although we don't want to have too much of a delay in sending alerts out.

If we're using cached data I wouldn't expect it to be a problem, but if we need to refresh it from the provider that might cause more of an issue. Now that it's easy for us to use Redis again I'd suggest we consider caching data, we could have Redis automatically expire data from the cache after a certain amount of time.

I'm not sure we'd get a very good hit rate on that cache though, the main problem I can foresee is if we're refreshing broker data (I'm assuming that's what you mean by "scan results"?)

I refactored the code to only fetch the above data for free US users who have run a scan, which I think is (at this point in time) a relatively manageable number. I'm still not sure if it was even going to be a problem, but this should make it safer to roll out, just in case: a7d419d.

github-actions · 2024-09-03T10:42:54Z

Preview URL 🚀 : https://blurts-server-pr-5012-mgjlpikfea-uk.a.run.app

flodolo · 2024-09-03T12:26:27Z

locales/en/email-strings.ftl

+email-footer-support-heading = Questions about { -brand-mozilla-monitor }?
+email-footer-support-content = Visit our <support-link>Support Center</support-link> for help
+email-footer-reason-subscriber = You’re receiving this automated email as a subscriber of { -brand-mozilla-monitor }. If you received it in error, no action is required. For more information, please visit <support-link>{ -brand-mozilla } Support</support-link>.
+email-footer-reason-subscriber-one-time = You’ve received this one-time automated email because you are subscribed to { -brand-monitor-plus }. You won’t receive any further emails like this. For more information, please visit <support-link>{ -brand-mozilla } Support</support-link>.


Should this string be exposed in the localizable file, since it talks about Monitor Plus?

Oh good callout, I was a bit too enthusiastic finding unlocalised strings: 2694159

Heads-up: I updated one more string whose change in Figma I had missed (70bc71c), and added two new strings because the footer text changed (57a2329). This new footer will be applied to every email at some point, so I opted to leave the previous one untranslated again to save the localisers some work.

flozia

This is looking really good! Just left a few non-blocking comments inline.

src/app/functions/universal/user.ts

flozia · 2024-09-06T11:47:29Z

src/emails/templates/EmailFooter.tsx

+  const supportLinkUrlObject = new URL(CONST_URL_SUMO_MONITOR_SUPPORT_CENTER);
+  supportLinkUrlObject.searchParams.set("utm_medium", "email");
+  supportLinkUrlObject.searchParams.set("utm_source", "monitor-product");
+  supportLinkUrlObject.searchParams.set("utm_campaign", props.utm_campaign);
+  supportLinkUrlObject.searchParams.set("utm_content", "support-center");


Note: Just setting search params is straightforward enough, but I just wanted to note that we are using the attribution helpers in src/app/functions/universal/attributions.ts in other places.

Ah! Right, I keep misunderstanding how that function works. However, it looks like it also needs a separate searchParams object, in which case it wouldn't actually get shorter while adding a layer of indirection:

Suggested change

const supportLinkUrlObject = new URL(CONST_URL_SUMO_MONITOR_SUPPORT_CENTER);

supportLinkUrlObject.searchParams.set("utm_medium", "email");

supportLinkUrlObject.searchParams.set("utm_source", "monitor-product");

supportLinkUrlObject.searchParams.set("utm_campaign", props.utm_campaign);

supportLinkUrlObject.searchParams.set("utm_content", "support-center");

const supportLinkUrlObject = new URL(CONST_URL_SUMO_MONITOR_SUPPORT_CENTER);

supportLinkUrlObject.search = "?" + modifyAttributionsForUrlSearchParams(

supportLinkUrlObject.searchParams,

{

utm_source: "monitor-product",

utm_campaign: props.utm_campaign,

utm_content: "support-center",

},

{},

).toString();

I don't think that's more readable or error-proof, but if we already use it elsewhere and people are used to it, then maybe I should just align with that?

Either is fine. I don’t think it’s more error-proof, but I don’t dislike the idea of aligning how we are appending searchParams. The current function as is was mostly used for modifying query params, but also can be very well be used for just adding them — we might need to consider renaming or splitting the functions a bit. modifyAttributionsForUrl would work without taking searchParams as an argument.

One approach that could make this simpler for callers would be to refactor the src/app/functions/universal/attributions.ts helpers to instead extend the URL and URLSearchParams interfaces, e.g. export class AttributedURL extends URL and encapsulate the functionality that the current modifyAttributionsForUrl and modifyAttributionsForUrlSearchParams helper functions provide into that class.

Then callers could do something like:

const supportLinkUrlObject = new AttributedURL(CONST_URL_SUMO_MONITOR_SUPPORT_CENTER);

And equivalent for AttributedURLSearchParams (although I think if AttributedURL.searchParams was AttributedURLSearchParams then it would probably not be necessary to export directly, callers could just modify an existing AttributedURL's searchParams and get the behavior they want).

That would allow callers to use the same API but it'd be clear from the class name(s) that these are overridden. These are long-stable interfaces so it seems unlikely to break in some subtle way.

Ah right, I hadn't noticed that these are two different functions. I suppose I could use modifyAttributionsForUrl for a slightly shorter version - I'd assume I'd pass an empty object for the third parameter (defaultValues), but I see many calls to that actually use that for UTM parameters. Does either of you know why those calls don't just override them either? I'm just wondering if we can't just simplify it to a setUrlParams function - i.e. why do we even need a defaultValues parameter?

I'm not sure, blame says that @mansaj landed this so he probably has a better idea.

src/scripts/cronjobs/emailBreachAlerts.tsx

src/emails/templates/EmailHero.tsx

src/emails/templates/breachAlert/BreachAlertEmail.tsx

rhelmer

As we discussed - if it's important that the scan results are up-to-date then I think you want to call refreshStoredScanResults (see the monthly activity report for an example). We know that these are often stale unfortunately so that's the workaround we've been doing so far.

We also talked about using the ./src/scripts/loadtest/hibp.js k6 script to perform load testing on stage and measure our assumptions about how much impact this will have. The "system" and "query" insights tabs for the database should be especially useful, and we can query logs to get good examples of how many notifications tend to come in for a typical data breach as well as the distribution of hash suffixes per prefix.

rhelmer · 2024-09-10T18:26:16Z

src/scripts/cronjobs/emailBreachAlerts.tsx

+                   */
+                  const assumedCountryCode = getSignupLocaleCountry(recipient);
+
+                  const scanData = await getLatestOnerepScanResults(


These may be stale, if that's important then refreshStoredScanResults can be awaited first.

OK, added d0a6996 for the three cases you called out here. One thing I did notice, though, is that we have a lot of calls to getLatestOnerepScanResults across our codebase that do not call this function first (e.g. every step in the guided resolution). I guess that might be fine since all of those go through the dashboard, where I assume it should get refreshed, but... Yeah, cache invalidation is hard.

Also asked a question in Slack about whether I'm running the load testing script correctly, since I'm not sure.

src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/fix/page.tsx

src/app/(proper_react)/(redesign)/(authenticated)/admin/emails/actions.tsx

src/app/(proper_react)/(redesign)/(authenticated)/admin/emails/EmailTrigger.tsx

src/app/(proper_react)/(redesign)/(authenticated)/admin/emails/actions.tsx

rhelmer · 2024-09-16T17:19:30Z

src/app/functions/universal/user.test.ts

+import { hasPremium } from "./user";
+import { Session } from "next-auth";
+
+describe("hasPremium", () => {


Thanks for adding a test!

It's intended to be used in every email, so I moved it out for now. Let's see if we actually get around to updating the other emails 😅

In case they cause too much load, we can easily disable the flag again.

When processing tens of thousands of breach notifications, we might theoretically be hammering the database by running a bunch of queries for each of those notifications. It's currently hard to test whether that load is actually excessive, but to be sure, this refactors it to only load that data if it is absolutely necessary. (Which currently is just for the free US user who has run a scan, where we display the number of unfixed results.)

github-actions · 2024-09-17T10:05:50Z

Cleanup completed - database 'blurts-server-pr-5012' destroyed, cloud run service 'blurts-server-pr-5012' destroyed

Vinnl requested review from rhelmer, codemist, flozia and mansaj September 3, 2024 10:27

Vinnl self-assigned this Sep 3, 2024

Vinnl requested a review from flodolo as a code owner September 3, 2024 10:27

Vinnl commented Sep 3, 2024

View reviewed changes

flodolo reviewed Sep 3, 2024

View reviewed changes

flodolo approved these changes Sep 3, 2024

View reviewed changes

flozia approved these changes Sep 6, 2024

View reviewed changes

Vinnl force-pushed the MNTOR-3433-breach-email-redesign branch from ee33211 to 278c2ba Compare September 9, 2024 11:25

Vinnl mentioned this pull request Sep 10, 2024

Only load scan results if a scan ran #5038

Merged

rhelmer requested changes Sep 10, 2024

View reviewed changes

Vinnl mentioned this pull request Sep 11, 2024

Convert most remaining JS to TS #5043

Merged

Vinnl requested a review from rhelmer September 12, 2024 11:38

codemist reviewed Sep 12, 2024

View reviewed changes

src/app/(proper_react)/(redesign)/(authenticated)/admin/emails/EmailTrigger.tsx Show resolved Hide resolved

codemist reviewed Sep 12, 2024

View reviewed changes

src/app/(proper_react)/(redesign)/(authenticated)/admin/emails/actions.tsx Show resolved Hide resolved

rhelmer reviewed Sep 16, 2024

View reviewed changes

rhelmer approved these changes Sep 16, 2024

View reviewed changes

Redirect to the correct step from /fix

3f33c1d

Vinnl force-pushed the MNTOR-3433-breach-email-redesign branch from a7d419d to 5fc2ddf Compare September 17, 2024 09:39

Vinnl added 5 commits September 17, 2024 11:52

Add redesigned breach alert email (behind flag)

05ece9e

Move redesigned footer into separate component

4efd9eb

It's intended to be used in every email, so I moved it out for now. Let's see if we actually get around to updating the other emails 😅

Limit footer bg colour to bg image width

f00f5ae

Apply redesigned footer text changes

45eb43a

Only fetch scans/breaches with flag enabled

76eda09

In case they cause too much load, we can easily disable the flag again.

Vinnl force-pushed the MNTOR-3433-breach-email-redesign branch from 5fc2ddf to bffd2a6 Compare September 17, 2024 09:52

Vinnl merged commit 55feaf4 into main Sep 17, 2024
15 checks passed

Vinnl deleted the MNTOR-3433-breach-email-redesign branch September 17, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breach alert email redesign #5012

Breach alert email redesign #5012

Vinnl commented Sep 3, 2024

Vinnl Sep 3, 2024

Vinnl Sep 3, 2024 •

edited

Loading

Vinnl Sep 3, 2024

Vinnl Sep 3, 2024

rhelmer Sep 9, 2024

rhelmer Sep 9, 2024 •

edited

Loading

Vinnl Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

flodolo Sep 3, 2024

Vinnl Sep 3, 2024

Vinnl Sep 4, 2024

flozia left a comment

flozia Sep 6, 2024

Vinnl Sep 9, 2024

flozia Sep 9, 2024

rhelmer Sep 9, 2024

Vinnl Sep 12, 2024

rhelmer Sep 16, 2024

rhelmer left a comment

rhelmer Sep 10, 2024

Vinnl Sep 12, 2024 •

edited

Loading

rhelmer Sep 16, 2024

github-actions bot commented Sep 17, 2024

Breach alert email redesign #5012

Breach alert email redesign #5012

Conversation

Vinnl commented Sep 3, 2024

References:

Description

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

Choose a reason for hiding this comment

Vinnl Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Vinnl Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flozia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024

Vinnl Sep 3, 2024 •

edited

Loading

rhelmer Sep 9, 2024 •

edited

Loading

Vinnl Sep 16, 2024 •

edited

Loading

Vinnl Sep 12, 2024 •

edited

Loading