-
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
MNTOR-2867 - Differentiate exposure card description depending on status #4215
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.
Does this need a default
case to catch any unexpected scanResult.status
values? (or is that already covered elsewhere by TypeScript types?)
{ | ||
elems: { | ||
data_broker_profile: dataBrokerProfileLink, | ||
upsell_link: upsellLink, |
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.
Not sure this needs the upsell_link
element.
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you</data_broker_profile>. Remove this profile to protect your privacy. |
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.
Good catch! fixed in 59f3a88
resolutionCta: ReactNode; | ||
isPremiumUser: boolean; | ||
isExpanded: boolean; | ||
isOnManualRemovePage?: boolean; |
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.
Added a prop here to check if the data broker object is on the manual removal page, which is handled by the RemovalCard.tsx
component, since the description within the cards on that page reflect different content than the ones on the dashboard.
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.
If components or functions have exceptions for a single caller, that's often an indication that the abstraction might not be a great fit - they can make things harder to follow, as half of the code you're looking at might be irrelevant to the particular situation you're investigating.
A way to avoid that is by lifting the logic up to the caller, i.e. have it pass in the string to render directly. That way, there's no need for an if
anywhere.
(This isn't a blocker, but it's a good exercise to get a better feel for software architecture 🙂 )
export const DataBrokerRemoved: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemRemoved, | ||
}, | ||
}; | ||
|
||
export const DataBrokerManualRemoved: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemManualRemoved, | ||
}, | ||
}; | ||
|
||
export const DataBrokerInProgress: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemInProgress, | ||
}, | ||
}; | ||
|
||
export const DataBreachActionNeeded: Story = { | ||
args: { | ||
exposureImg: TwitterImage, | ||
exposureData: BreachMockItemNew, | ||
}, | ||
}; | ||
|
||
export const DataBreachFixed: Story = { | ||
args: { | ||
exposureImg: TwitterImage, | ||
exposureData: BreachMockItem, | ||
exposureData: BreachMockItemRemoved, | ||
}, | ||
}; |
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.
Each exposure card permutation can be viewed on storybook.
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.
Nice!
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's the reason to keep around the obsolete strings?
locales/en/exposure-card.ftl
Outdated
@@ -24,13 +24,27 @@ exposure-card-credit-card = Credit Card | |||
exposure-card-password = Password | |||
exposure-card-ip-address = IP Address | |||
exposure-card-other = Other | |||
# 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.
Obsolete
is more accurate.
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.
Not entirely sure - I was following a previous pattern that we had, but I'll bring it up in a tech meeting how we may go about deleting obsolete strings (if we should prune them on the go, or do a clean-up every once in a while).
locales/en/exposure-card.ftl
Outdated
exposure-card-description-info-for-sale-part-two = Remove this profile to protect your privacy. | ||
exposure-card-description-info-for-sale-action-needed-manual-fix-page = This site is selling <data_broker_profile>these details about you.</data_broker_profile> Contact the site for removal, or <upsell_link>subscribe to { -brand-monitor-plus }</upsell_link> and we’ll do it for you. |
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 exposed to all locales? Removal is only US.
locales/en/exposure-card.ftl
Outdated
exposure-card-description-info-for-sale-action-needed-manual-fix-page = This site is selling <data_broker_profile>these details about you.</data_broker_profile> Contact the site for removal, or <upsell_link>subscribe to { -brand-monitor-plus }</upsell_link> and we’ll do it for you. | ||
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you.</data_broker_profile> Remove this profile to protect your privacy. | ||
exposure-card-description-info-for-sale-in-progress = We’ve started our auto-removal process of <data_broker_profile>this profile</data_broker_profile> to protect your information. <removal_info>Removals typically take 7-14 days.</removal_info> | ||
exposure-card-description-info-for-sale-fixed = As a { -brand-premium } member, we’ve <data_broker_profile>removed this profile</data_broker_profile> for you and will continually monitor to make sure they don’t add you back. |
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 assume we shouldn't be using Premium 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.
Added -brand-plus
in 7648ce8
locales/en/exposure-card.ftl
Outdated
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you.</data_broker_profile> Remove this profile to protect your privacy. | ||
exposure-card-description-info-for-sale-in-progress = We’ve started our auto-removal process of <data_broker_profile>this profile</data_broker_profile> to protect your information. <removal_info>Removals typically take 7-14 days.</removal_info> | ||
exposure-card-description-info-for-sale-fixed = As a { -brand-premium } member, we’ve <data_broker_profile>removed this profile</data_broker_profile> for you and will continually monitor to make sure they don’t add you back. | ||
exposure-card-description-info-for-sale-fixed-manually-fixed = You marked this profile as fixed. Be sure you’ve followed all instructions on <data_broker_profile>the site</data_broker_profile> to ensure they remove your personal info. |
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.
No double space after a period.
); | ||
} | ||
if (props.isOnManualRemovePage) { | ||
/* c8 ignore start */ |
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 noticed we don't run any unit tests on ExposureCard
, I captured that tech debt here: https://mozilla-hub.atlassian.net/jira/software/c/projects/MNTOR/boards/447?selectedIssue=MNTOR-2981
We do have tests for the overall dashboard, but since we're expecting new permutations (and telemetry) for the various exposure card description states, it's probably best to have unit tests for just that component specifically.
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.
They're mostly part of Dashboard.test.tsx
, though this one should probably be part of ManualRemove.test.tsx
. Could you try adding one there?
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.
Going to add the tests as part of another ticket, will probably couple in the re-abstraction of the ScanResultCard
in that too.
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 ExposureCard.tsx was beginning to become unwieldy in length, so I decided to break it up into two files, one for the data broker instance, and the other for data breach.
It was getting unwieldy indeed, thanks for that! Since the three components and pretty closely related, maybe it makes sense to group them together in an ExposureCard/
folder?
But the changes look good to me! I do think you can write the unit test though 😁
exposure-card-description-info-for-sale-action-needed-manual-fix-page = This site is selling <data_broker_profile>these details about you.</data_broker_profile> Contact the site for removal, or <upsell_link>subscribe to { -brand-monitor-plus }</upsell_link> and we’ll do it for you. | ||
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you.</data_broker_profile> Remove this profile to protect your privacy. |
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, but the .
s shouldn't be part of the links :)
exposure-card-description-info-for-sale-action-needed-manual-fix-page = This site is selling <data_broker_profile>these details about you.</data_broker_profile> Contact the site for removal, or <upsell_link>subscribe to { -brand-monitor-plus }</upsell_link> and we’ll do it for you. | |
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you.</data_broker_profile> Remove this profile to protect your privacy. | |
exposure-card-description-info-for-sale-action-needed-manual-fix-page = This site is selling <data_broker_profile>these details about you</data_broker_profile>. Contact the site for removal, or <upsell_link>subscribe to { -brand-monitor-plus }</upsell_link> and we’ll do it for you. | |
exposure-card-description-info-for-sale-action-needed-dashboard = This site is publicly publishing and selling <data_broker_profile>these details about you</data_broker_profile>. Remove this profile to protect your privacy. |
locales/en/app.ftl
Outdated
@@ -871,6 +871,7 @@ ad-unit-6-before-you-complete = Before you complete that next signup, use an ema | |||
-brand-relay = Firefox Relay | |||
-brand-mozilla-monitor = Mozilla Monitor | |||
-brand-monitor-plus = Monitor Plus | |||
-brand-plus = Plus |
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.
Might be worth a check with Mel whether we want to use "Plus" as a standalone term - it feels a bit more awkward than "Premium" to me.
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.
Checked with Mel - reverted back to Monitor Plus
here. Good catch!
locales/en/exposure-card.ftl
Outdated
# obsolete | ||
exposure-card-description-info-for-sale-part-one = This site is selling and publishing <data_broker_link>details about you.</data_broker_link> | ||
# obsolete | ||
exposure-card-description-info-for-sale-part-two = Remove this profile to protect your privacy. | ||
# obsolete | ||
# Variables: | ||
# $data_breach_company is the company associated with the data breach. | ||
# $data_breach_date is the date of the data breach. | ||
exposure-card-description-data-breach-part-one = Your information was exposed in the <data_breach_link>{ $data_breach_company } data breach on { $data_breach_date }.</data_breach_link> | ||
# obsolete | ||
exposure-card-description-data-breach-part-two = We’ll walk you through the steps to fix it. |
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 you removed the references to these, you can just remove them right away. (We couldn't do that in Relay because it was in a separate repository, so it could happen that the changes to the .ftl
files would be merged before the removal of references to them were merged. But since both happen in the same PR now, we should be good.)
# obsolete | |
exposure-card-description-info-for-sale-part-one = This site is selling and publishing <data_broker_link>details about you.</data_broker_link> | |
# obsolete | |
exposure-card-description-info-for-sale-part-two = Remove this profile to protect your privacy. | |
# obsolete | |
# Variables: | |
# $data_breach_company is the company associated with the data breach. | |
# $data_breach_date is the date of the data breach. | |
exposure-card-description-data-breach-part-one = Your information was exposed in the <data_breach_link>{ $data_breach_company } data breach on { $data_breach_date }.</data_breach_link> | |
# obsolete | |
exposure-card-description-data-breach-part-two = We’ll walk you through the steps to fix it. |
@@ -48,14 +48,14 @@ export const RemovalCard = (props: Props) => { | |||
} | |||
|
|||
return ( | |||
<ExposureCard | |||
exposureData={{ | |||
<ScanResultCard |
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.
Praise: oh heh, this is neat - of course we never need a data breach card on the data broker removal card, nice cleanup.
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.
Small heads-up: I can't see in the diff here what is just code that is moved, and what are changes that are actually specific to the PR. So apologies for potentially bringing up things that were already there.
(And potentially for the future: if there had been a separate commit at the start that split them out, then I could've chosen to view just those commits that followed it. Rebasing is a Git concept that makes that easier, if you're looking for something to study :) )
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.
Update: OK well, that wasn't too bad. It's a good refactoring anyway, this is a far more reasonable component!
); | ||
} | ||
if (props.isOnManualRemovePage) { | ||
/* c8 ignore start */ |
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.
They're mostly part of Dashboard.test.tsx
, though this one should probably be part of ManualRemove.test.tsx
. Could you try adding one there?
resolutionCta: ReactNode; | ||
isPremiumUser: boolean; | ||
isExpanded: boolean; | ||
isOnManualRemovePage?: boolean; |
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.
If components or functions have exceptions for a single caller, that's often an indication that the abstraction might not be a great fit - they can make things harder to follow, as half of the code you're looking at might be irrelevant to the particular situation you're investigating.
A way to avoid that is by lifting the logic up to the caller, i.e. have it pass in the string to render directly. That way, there's no need for an if
anywhere.
(This isn't a blocker, but it's a good exercise to get a better feel for software architecture 🙂 )
export const DataBrokerRemoved: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemRemoved, | ||
}, | ||
}; | ||
|
||
export const DataBrokerManualRemoved: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemManualRemoved, | ||
}, | ||
}; | ||
|
||
export const DataBrokerInProgress: Story = { | ||
args: { | ||
exposureImg: FamilyTreeImage, | ||
exposureData: ScanMockItemInProgress, | ||
}, | ||
}; | ||
|
||
export const DataBreachActionNeeded: Story = { | ||
args: { | ||
exposureImg: TwitterImage, | ||
exposureData: BreachMockItemNew, | ||
}, | ||
}; | ||
|
||
export const DataBreachFixed: Story = { | ||
args: { | ||
exposureImg: TwitterImage, | ||
exposureData: BreachMockItem, | ||
exposureData: BreachMockItemRemoved, | ||
}, | ||
}; |
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.
Nice!
References:
Jira: MNTOR-2867
Figma: https://www.figma.com/file/CaEKIhvSJqf6KNIMzSkt40/Concepts-for-Monitor-MVP-Redesign?type=design&node-id=1398-75640&mode=design&t=Dnr6IkRIcYtUko9j-4
Description
Contextualizes the exposure card description for both data broker and data breach depending on the exposure card's status, as well as (in the case of data broker cards), whether the user is on the manually resolve page or the dashboard.
The
ExposureCard.tsx
was beginning to become unwieldy in length, so I decided to break it up into two files, one for the data broker instance, and the other for data breach.How to test
Checklist (Definition of Done)