-
Notifications
You must be signed in to change notification settings - Fork 5k
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
chore: add privacy query params to portfolio navigation #25958
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [2b321a9]
Page Load Metrics (173 ± 180 ms)
Bundle size diffs
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25958 +/- ##
===========================================
+ Coverage 69.68% 69.69% +0.01%
===========================================
Files 1405 1405
Lines 49701 49723 +22
Branches 13738 13741 +3
===========================================
+ Hits 34630 34650 +20
- Misses 15071 15073 +2 ☔ View full report in Codecov by Sentry. |
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.
PR Summary
Added metricsEnabled
and marketingEnabled
query parameters to URLs for portfolio navigation to reflect user privacy preferences.
- Updated
test/e2e/tests/bridge/bridge-click-from-asset-overview.spec.ts
to verify new query parameters in portfolio URL. - Modified
ui/components/app/wallet-overview/coin-buttons.tsx
to include new query parameters in generated URLs. - Enhanced
ui/helpers/utils/portfolio.js
to append privacy preferences to portfolio URLs. - Adjusted
ui/hooks/ramps/useRamps/useRamps.ts
to include new query parameters in Ramps buy page URL. - Updated relevant e2e and unit tests to incorporate the new query parameters.
14 file(s) reviewed, 37 comment(s)
Edit PR Review Bot Settings
const EXPECTED_PORTFOLIO_URL = | ||
'https://portfolio.metamask.io/bridge?metametricsId=null&metricsEnabled=false&marketingEnabled=false'; |
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.
Info: Added EXPECTED_PORTFOLIO_URL
constant to include new query parameters.
'https://portfolio.metamask.io/bridge?metametricsId=null', | ||
); | ||
|
||
await bridgePage.verifyPortfolioTab(EXPECTED_PORTFOLIO_URL); |
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.
Info: Updated URL verification to include metricsEnabled
and marketingEnabled
query parameters.
await bridgePage.verifyPortfolioTab( | ||
'https://portfolio.metamask.io/bridge?metametricsId=null', | ||
); | ||
await bridgePage.verifyPortfolioTab(EXPECTED_PORTFOLIO_URL); |
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.
Info: Updated URL verification to include metricsEnabled
and marketingEnabled
query parameters.
@@ -19,7 +19,7 @@ describe('Click bridge button from wallet overview @no-mmi', function (this: Sui | |||
await logInWithBalanceValidation(driver, ganacheServer); | |||
await bridgePage.load(); | |||
await bridgePage.verifyPortfolioTab( | |||
'https://portfolio.metamask.io/bridge?metametricsId=null', | |||
'https://portfolio.metamask.io/bridge?metametricsId=null&metricsEnabled=false&marketingEnabled=false', |
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.
Info: Added metricsEnabled
and marketingEnabled
query parameters to the URL verification.
const currentUrl = await driver.getCurrentUrl(); | ||
const expectedUrl = | ||
'https://portfolio.metamask.io/?metamaskEntry=ext_portfolio_button&metametricsId=null&metricsEnabled=false&marketingEnabled=false'; | ||
assert.equal(currentUrl, expectedUrl); |
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.
Info: Updated the expected URL to include metricsEnabled
and marketingEnabled
query parameters.
isMetaMetricsEnabled, | ||
isMarketingEnabled, | ||
); |
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.
Info: Included new query parameters for privacy settings in the Portfolio URL.
const EXPECTED_BUY_URL = | ||
'https://portfolio.test/buy?metamaskEntry=ext_buy_sell_button&chainId=0x5'; | ||
'https://portfolio.test/buy?metamaskEntry=ext_buy_sell_button&chainId=0x5&metricsEnabled=false'; |
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.
Info: Updated EXPECTED_BUY_URL to include metricsEnabled query parameter.
getHardwareWalletType, | ||
getIsBridgeChain, | ||
getMetaMetricsId, | ||
getParticipateInMetaMetrics, | ||
getDataCollectionForMarketing, | ||
} from '../../../selectors'; |
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.
Info: Added selectors to fetch user privacy preferences for MetaMetrics and marketing data collection.
const currentCurrency = useSelector(getCurrentCurrency); | ||
const fetchingQuotes = useSelector(getFetchingQuotes); | ||
const loadingComplete = !fetchingQuotes && areQuotesPresent; | ||
const isMetaMetricsEnabled = useSelector(getParticipateInMetaMetrics); | ||
const isMarketingEnabled = useSelector(getDataCollectionForMarketing); |
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.
Info: Fetched user privacy preferences using the newly added selectors.
'bridge', | ||
'ext_bridge_prepare_swap_link', | ||
metaMetricsId, | ||
isMetaMetricsEnabled, | ||
isMarketingEnabled, | ||
); |
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.
Info: Passed the fetched privacy preferences to the getPortfolioUrl
function.
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.
PR Summary
(updates since last review)
Added metricsEnabled
and marketingEnabled
query parameters to URLs for portfolio navigation to reflect user privacy preferences.
- Updated
app/scripts/controllers/user-storage/user-storage-controller.ts
to allownull
values forisProfileSyncingEnabled
. - Added migration script
app/scripts/migrations/120.1.ts
and corresponding testapp/scripts/migrations/120.1.test.ts
. - Modified
app/scripts/migrations/index.js
to include the new migration script120.1
. - Added end-to-end tests for SIWE in
test/e2e/tests/confirmations/signatures/siwe.spec.ts
. - Updated state snapshots in
test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json
anderrors-after-init-opt-in-ui-state.json
to reflect new privacy settings.
11 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Quality Gate passedIssues Measures |
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.
PR Summary
(updates since last review)
Added metricsEnabled
and marketingEnabled
query parameters to URLs for portfolio navigation to reflect user privacy preferences.
- Updated
app/scripts/background.js
to introduce test-specific overrides for keyrings. - Added
app/scripts/lib/hardware-keyring-builder-factory.ts
to includeFakeKeyringBridge
for testing. - Modified
app/scripts/metamask-controller.js
to refactor hardware keyring handling. - Added migration script
app/scripts/migrations/122.ts
and corresponding testapp/scripts/migrations/122.test.ts
. - Updated e2e tests in
test/e2e/tests/metrics/signature-approved.spec.js
to include redesigned confirmation settings.
45 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Builds ready [4ae034a]
Page Load Metrics (446 ± 402 ms)
Bundle size diffs
|
Description
Added
metricsEnabled
andmarketingEnabled
query params to reflect user privacy preferences on user navigation to Portfolio. These settings, sent from extension, will be consumed by the Portfolio to update and align privacy preferences.Updated all relevant e2e and unit tests to include the params.
Context: Our current privacy opt-in rate has been low since some recent updates to our privacy flow, so we are aligning our privacy settings with the other platforms.
Related issues
Fixes:
Manual testing steps
Example URL:
https://portfolio.metamask.io/?metamaskEntry=ext_portfolio_button&metametricsId=0xbcd1a47f61d820aac3158af775dfee88828e5330aae11f3048767aab7e99b474&metricsEnabled=false&marketingEnabled=false
Screenshots/Recordings
Before
After
With query params attached:
https://www.loom.com/share/7f372324de1e452a837e5faa9860ee94
Pre-merge author checklist
Pre-merge reviewer checklist