MNTOR-3863 and MNTOR-3840 - Fix user dashboard state for manual removal flag #5473
Conversation
|
Preview URL 🚀 : https://blurts-server-pr-5473-mgjlpikfea-uk.a.run.app |
There was a problem hiding this comment.
Possibly I'm misunderstanding, but I feel like there are quite a few places in our code where we call getDashboardSummary, that still do not take into account whether data brokers are under maintenance? For example, the monthly activity email, or the "Welcome to Plus" view?
| @@ -96,6 +98,7 @@ export const dataClassKeyMap: Record<keyof DataPoints, string> = { | |||
| export function getDashboardSummary( | |||
| scannedResults: OnerepScanResultDataBrokerRow[], | |||
| subscriberBreaches: SubscriberBreach[], | |||
| enabledFeatureFlags?: FeatureFlagName[], | |||
There was a problem hiding this comment.
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?
| enabledFeatureFlags?: FeatureFlagName[], | |
| enabledFeatureFlags: FeatureFlagName[], |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
ResolutionContainerseems to refer todataBrokerInProgressDataPointsNumat 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.
There was a problem hiding this comment.
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.
| 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 = |
There was a problem hiding this comment.
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
+ );There was a problem hiding this comment.
@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).
|
Cleanup completed - database 'blurts-server-pr-5473' destroyed, cloud run service 'blurts-server-pr-5473' destroyed |
References:
Jira: MNTOR-3853 and MNTOR-3840
Figma:
Description
This PR fixes a few things:
Screenshot (if applicable)
Screen.Recording.2025-01-09.at.10.48.29.AM.mov
How to test
Checklist (Definition of Done)