-
Notifications
You must be signed in to change notification settings - Fork 282
MNTOR-5066 - Big data broker FE removal #6314
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
base: main
Are you sure you want to change the base?
Conversation
|
As just discussed: I've unassigned myself as a reviewer for now, as you can focus on removing everything that's related (backend as well), and then re-assign me when the checks succeed and it's ready for review again. |
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.
FYI these cron jobs are handled in #6289, which is scheduled to be merged after Dec 17
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.
Thanks for the heads up, just approved. Please merge it when you can; I’ll rebase right after to avoid more merge conflicts.
Note: since this is part of the deprecation work without a feature flag, we’ll probably want to hold off on any production pushes this week (and next).
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 was planning on waiting to merge until closer to the release date (we can't release before Dec 17). If it's in there, it will make releasing anything else much more complex because we'll have to cherry-pick commits... Thoughts?
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.
@kschelonka Would it make sense to base that PR on this branch, so we can test/release in one go?
|
Build is finally passing. I’m working through the unit tests now, mostly cleanup around the calculation logic. I haven’t touched any of the Plus shutdown logic yet, so I kept the |
| props.experimentData ?? defaultExperimentData["Features"]; | ||
| const enabledFeatureFlags = props.enabledFeatureFlags ?? []; | ||
| return ( | ||
| <AccountsMetricsFlowProvider |
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.
Traced all of this logic back to #4822 which is Plus related.
| it("sends telemetry when a free scan CTA is shown in the viewport", async () => { | ||
| const mockedRecord = useTelemetry(); | ||
| const ComposedDashboard = composeStory(LandingNonUs, Meta); | ||
| render(<ComposedDashboard />); | ||
|
|
||
| // jsdom will complain about not being able to navigate to a different page | ||
| // after clicking the link; suppress that error, as it's not relevant to the test: | ||
| jest.spyOn(console, "error").mockImplementation(() => undefined); | ||
|
|
||
| // The useViewTelemetry ref is attached to the form, not the button | ||
| const submitButton = screen.getAllByRole("button", { | ||
| name: "Get free scan", | ||
| }); | ||
| const form = submitButton[0].closest("form"); | ||
| expect(form).toBeInTheDocument(); | ||
|
|
||
| // Trigger intersection on the form element | ||
| await act(async () => { | ||
| mockIsIntersecting(form!, true); | ||
| }); | ||
|
|
||
| expect(mockedRecord).toHaveBeenCalledWith( | ||
| "ctaButton", | ||
| "view", | ||
| expect.objectContaining({ button_id: "clicked_get_scan_header" }), | ||
| ); | ||
| }); |
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 traced this component back to https://github.com/mozilla/blurts-server/pull/4794/files, so I rewrote the test to only account for the “Get Free Scan” non-US state.
| @@ -0,0 +1,66 @@ | |||
| /* 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.
A lot of the old dashboard US/Plus tests added coverage in places that were hard to separate from the core interactions in the FixView component (which is basically the foundation for all guided resolution flows). It ended up being cleaner to just rewrite this part instead of trying to untangle the old logic.
| isDismissable={true} | ||
| > | ||
| <Dialog | ||
| onDismiss={() => overlayTriggerState.close()} |
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 tracked down why onDismiss wasn’t being passed by default, it was only missing in this dialog component, which seemed inconsistent with our patterns. The dismiss button really should be a non-optional part of the dialog component, so I made it unconditional and added it directly to this component. It looked fine in Storybook, so I also added a test for coverage.
|
Update: I could definitely spend more time fully cleaning up every last piece of code related to sunsetting Plus, but this should be a solid starting point to meet the Dec 17 deadline. There are still a few scattered mentions of Plus/Premium/subscriptions across the codebase, and some additional decoupling of feature flags that we can do later. But overall, I think this is in a good enough state for review (coverage back at 100%). I've tried to leave comments where the reasoning behind a change wasn’t immediately obvious, or where I removed something that might raise questions. I didn’t annotate every file because that would get unwieldy, so if anything feels unclear, please just ping me, I can spend some extra time doing a self-review and adding more context where needed. The remaining follow-up steps I see:
There’s still some potential cleanup around naming conventions in Storybook components and unused utility functions, but that’s starting to veer into tech debt rather than sunset work, so I’m pausing on that for now. Let me know if anything looks off or if you'd like me to walk through parts of it! |
| const session = await getServerSession(); | ||
| if ( | ||
| !session?.user?.email || | ||
| !isAdmin(session.user.email) || |
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.
question: this doesn't seem related to onerep specifically. can you explain the motivation behind the changes here?
| lastScanDate={null} | ||
| // We're not going to run experiments on the feature flag page (it's | ||
| // not user-visible), so no need to fetch experiment data: | ||
| experimentData={defaultExperimentData["Features"]} |
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.
question: the default data is no longer necessary?
| return notFound(); | ||
| } | ||
|
|
||
| const enabledFeatureFlags = await getEnabledFeatureFlags({ |
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'll stop commenting after this one, but just curious why removing feature flag integrations
| if (props.brokers && props.brokers !== "no-scan") { | ||
| const scanInProgress = props.brokers === "scan-in-progress"; | ||
| scanData.scan = scanInProgress ? mockedScanInProgress : mockedScan; | ||
| const scanCount = 0; |
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.
unneeded const maybe?
| parseIso8601Datetime((b as OnerepScanResultRow).created_at); | ||
| const arraySortedByDate = breachesDataArray.sort((a, b) => { | ||
| const dateA = a.addedDate; | ||
| const dateB = b.addedDate; |
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.
nit: this can be onelined
const arraySortedByDate = breachesDataArray.sort((a, b) => b.addedDate.getTime() - a.addedDate.getTime())
| if (!hasUnresolvedExposures && hasFixedExposures) { | ||
| return ( | ||
| <> | ||
| <Image src={AllFixedIllustration} alt="" /> | ||
| <strong> | ||
| {l10n.getString("dashboard-exposures-all-fixed-label")} | ||
| </strong> | ||
| {freeScanCta} | ||
| </> |
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 seems still relevant? if I'm reading it correctly it should also be shown for if you fixed all breaches? or is this broker-specific
| export const FixView = (props: FixViewProps) => { | ||
| const l10n = useL10n(); | ||
| const recordTelemetry = useTelemetry(); | ||
| const isResolutionLayout = [ |
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.
why is this no longer relevant?
| import { CONST_URL_MOZILLA_BASKET } from "../../../../../../../../constants"; | ||
|
|
||
| // The monthly email for free users is currently disabled; see MNTOR-4970. | ||
| const isFreeMonthlyMonitorReportDisabledSeeMNTOR4970 = true; |
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 maxEmails = () => { | ||
| const emails = []; | ||
| for (let i = 1; i <= CONST_MAX_NUM_ADDRESSES; i++) { | ||
| emails.push(mockRandomEmail(i)); | ||
| } | ||
|
|
||
| return emails; | ||
| }; |
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.
nit: there's a nice one-liner for this instead of mutating an array:
Array.from({ length: CONST_MAX_NUM_ADDRESSES }, (_, index) => mockRandomEmail(index));
References:
Jira: MNTOR-5066
Figma:
Description
This PR begins the process of removing all data-broker–related information from the front end while minimizing changes to the backend as much as possible. I started with the Dashboard and am working outward from there, since everything is deeply connected and any component referencing
OneRepScanResultRowor related OneRep types needs to be carefully unraveled.I could be wrong here (and also because we didn't really formally decide on a protocol for the FE removal) but because of the amount of prop drilling and cross-component coupling, doing this in tiny incremental PRs each removal step tends to cascade into linting issues, build errors, or broken types. Instead, this PR represents the first major chunk in a slow, deliberate phase-out. It should be reviewed in sections, and it will definitely require a thorough regression test.
The overall goal is to steadily strip out all OneRep/data-broker UI surfaces without destabilizing unrelated areas of the app. Subsequent PRs will continue this clean-up process once this foundation is in place.
Screenshot (if applicable)
Not applicable.
How to test
Checklist (Definition of Done)