Skip to content
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.

Sign up for GitHub

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

MNTOR-3863 and MNTOR-3840 - Fix user dashboard state for manual removal flag #5473

Merged
merged 6 commits into from
Jan 14, 2025
+636 −493
Merged

MNTOR-3863 and MNTOR-3840 - Fix user dashboard state for manual removal flag #5473

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fde294e
fix user dashbaord state
codemist Jan 9, 2025
157a21d
fix unit tests
codemist Jan 9, 2025
1dbc11f
add feature flags to manualremovalview and welcome view
codemist Jan 13, 2025
2c60b47
add feature flag to resolution container
codemist Jan 14, 2025
cf67902
add featureflag to monthly activity plus
codemist Jan 14, 2025
dec8f32
Merge branch 'main' into mntor-3863-mntor-3840
codemist Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...uthenticated)/user/(dashboard)/dashboard/DashboardTopBanner/DashboardTopBannerContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ export const DashboardTopBannerContent = (props: DashboardTopBannerProps) => {
);
}

const relevantGuidedStep = getNextGuidedStep(stepDeterminationData);
const relevantGuidedStep = getNextGuidedStep(
stepDeterminationData,
props.enabledFeatureFlags,
);

const contentProps = {
relevantGuidedStep,
Expand Down
3 changes: 3 additions & 0 deletions src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/View.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ export const View = (props: Props) => {
const removalTimeEstimate = isScanResult(exposure)
? props.removalTimeEstimates.find(({ d }) => d === exposure.data_broker)
: undefined;

return (
<li key={exposureCardKey} className={styles.exposureListItem}>
<ExposureCard
Expand Down Expand Up @@ -253,6 +254,7 @@ export const View = (props: Props) => {
const dataSummary = getDashboardSummary(
adjustedScanResults,
props.userBreaches,
props.enabledFeatureFlags,
);

const hasExposures = combinedArray.length > 0;
Expand Down Expand Up @@ -513,6 +515,7 @@ export const View = (props: Props) => {
bannerData={getDashboardSummary(
adjustedScanResults,
props.userBreaches,
props.enabledFeatureFlags,
)}
stepDeterminationData={{
countryCode,
Expand Down
9 changes: 8 additions & 1 deletion src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/fix/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { getScanResultsWithBroker } from "../../../../../../../../db/tables/oner
import { getServerSession } from "../../../../../../../functions/server/getServerSession";
import { refreshStoredScanResults } from "../../../../../../../functions/server/refreshStoredScanResults";
import { hasPremium } from "../../../../../../../functions/universal/user";
import { getEnabledFeatureFlags } from "../../../../../../../../db/tables/featureFlags";

export default async function FixPage() {
const session = await getServerSession();
Expand All @@ -31,6 +32,9 @@ export default async function FixPage() {
if (typeof profileId === "number") {
await refreshStoredScanResults(profileId);
}
const enabledFeatureFlags = await getEnabledFeatureFlags({
email: session.user.email,
});

const scanData = await getScanResultsWithBroker(
profileId,
Expand All @@ -43,6 +47,9 @@ export default async function FixPage() {
latestScanData: scanData,
};

const nextStep = getNextGuidedStep(stepDeterminationData);
const nextStep = getNextGuidedStep(
stepDeterminationData,
enabledFeatureFlags,
);
redirect(nextStep.href);
}
39 changes: 24 additions & 15 deletions src/app/functions/server/dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { OnerepScanResultDataBrokerRow } from "knex/types/tables";
import { BreachDataTypes } from "../universal/breach";
import { RemovalStatusMap } from "../universal/scanResult";
import { SubscriberBreach } from "../../../utils/subscriberBreaches";
import { DataBrokerRemovalStatusMap } from "../universal/dataBroker";
import { FeatureFlagName } from "../../../db/tables/featureFlags";

export type DataPoints = {
// shared
Expand Down Expand Up @@ -96,6 +98,7 @@ export const dataClassKeyMap: Record<keyof DataPoints, string> = {
export function getDashboardSummary(
scannedResults: OnerepScanResultDataBrokerRow[],
subscriberBreaches: SubscriberBreach[],
enabledFeatureFlags?: FeatureFlagName[],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to leave this optional? Feels like everywhere we use this function, we want to take feature flags into account as well to make sure that under-maintenance brokers aren't counted, right?

Suggested change
enabledFeatureFlags?: FeatureFlagName[],
enabledFeatureFlags: FeatureFlagName[],

Copy link
Collaborator Author

@codemist codemist Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vinnl I made it optional because we may remove the flag at some point, and it was tedious having to update all of the test files and also making changes to the function in actions.tsx was throwing an error for me.

Copy link
Collaborator Author

@codemist codemist Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only part of the experience that was flagged by QA, I don't think it would show in the Welcome view, AFAIK a data broker wouldn't immediately be under removal_under_maintenance it would take some time to become such.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to those files anyways: 1dbc11f

I can request for QA to check out those views and verify that the calculations are accurate with the flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm OK, if we're not simply passing it everywhere, then we'd have to judge per call site whether it's necessary to consider under-maintenance status, so I manually visited all the call sites and found the following two that I think would also need updating?

  • ResolutionContainer seems to refer to dataBrokerInProgressDataPointsNum at least - I think for the "In progress" column here?
  • The Plus monthly activity email also shows how many items are in progress, so I think that also shouldn't keep listing brokers that are under maintenance as in progress.

Copy link
Collaborator Author

@codemist codemist Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for going through the trouble of visiting the call sites! I can ping QA to double check those touchpoints.

Added the flag to ResolutionContainer here: 2c60b47
and to Plus monthly activity email: cf67902

It's really tedious wrapping a feature flag around this feature precisely because it touches so many parts of the experience :\ This feature is relatively low risk though, since the altered part of the feature is wrapped behind the flag.

): DashboardSummary {
const summary: DashboardSummary = {
dataBreachTotalNum: 0,
Expand Down Expand Up @@ -204,15 +207,22 @@ export function getDashboardSummary(
r.status === RemovalStatusMap.WaitingForVerification) &&
!isManuallyResolved;

// TODO: Waiting for criteria for data brokers under maintenance to be determined
// const isRemovalUnderMaintenance =
// r.broker_status === DataBrokerRemovalStatusMap.RemovalUnderMaintenance;
// TODO: MNTOR-3886 - Remove EnableRemovalUnderMaintenanceStep feature flag
// If the flag is disabled, include the data.
// If the flag is enabled, include the data only if the broker status is not
const isRemovalUnderMaintenance =
r.broker_status === DataBrokerRemovalStatusMap.RemovalUnderMaintenance;

// The condition ensures that removal under maintenance is only considered when the flag is enabled.
/* c8 ignore next 3 */
const countRemovalUnderMaintenanceData =
Comment on lines +213 to +218
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a nit I guess, but it took me a while to understand the logic below because of the countRemova... name, e.g. I read lines 222 and 223 as "if this scan result is in progress and we're counting brokers that are under maintenance, then increment the number of in progress data brokers"?

In the end, isn't a data broker under maintenance just not in progress? i.e. instead of this code and the additional ifs, couldn't we update line 205:

      const isInProgress =
        (r.status === RemovalStatusMap.OptOutInProgress ||
          r.status === RemovalStatusMap.WaitingForVerification) &&
-        !isManuallyResolved;
+        !isManuallyResolved && (
+          // TODO: MNTOR-3886 - Remove EnableRemovalUnderMaintenanceStep feature flag
+          !enabledFeatureFlags?.includes("EnableRemovalUnderMaintenanceStep") ||
+          r.broker_status !== DataBrokerRemovalStatusMap.RemovalUnderMaintenance
+        );

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vinnl I think that the status of it could still read in_progress but the broker_status with removal_under_maintenance should ultimately take precedence in it's overall state (from the perspective of the user).

!enabledFeatureFlags?.includes("EnableRemovalUnderMaintenanceStep") ||
!isRemovalUnderMaintenance;

if (isInProgress) {
// TODO: Waiting for criteria for data brokers under maintenance to be determined
// if (!isRemovalUnderMaintenance) {
summary.dataBrokerInProgressNum++;
// }
if (countRemovalUnderMaintenanceData) {
summary.dataBrokerInProgressNum++;
}
} else if (isAutoFixed) {
summary.dataBrokerAutoFixedNum++;
} else if (isManuallyResolved) {
Expand All @@ -234,14 +244,13 @@ export function getDashboardSummary(
summary.allDataPoints.familyMembers += r.relatives.length;

if (isInProgress) {
// TODO: Waiting for criteria for data brokers under maintenance to be determined
// if (!isRemovalUnderMaintenance) {
summary.inProgressDataPoints.emailAddresses += r.emails.length;
summary.inProgressDataPoints.phoneNumbers += r.phones.length;
summary.inProgressDataPoints.addresses += r.addresses.length;
summary.inProgressDataPoints.familyMembers += r.relatives.length;
summary.dataBrokerInProgressDataPointsNum += dataPointsIncrement;
// }
if (countRemovalUnderMaintenanceData) {
summary.inProgressDataPoints.emailAddresses += r.emails.length;
summary.inProgressDataPoints.phoneNumbers += r.phones.length;
summary.inProgressDataPoints.addresses += r.addresses.length;
summary.inProgressDataPoints.familyMembers += r.relatives.length;
summary.dataBrokerInProgressDataPointsNum += dataPointsIncrement;
}
}

// for fixed data points: email, phones, addresses, relatives, full name (1)
Expand Down
Loading
Loading