fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality #25070

Prithpal-Sooriya · 2024-06-05T15:52:41Z

Description

This ensures that our feature is fully disabled when the basic functionality is disabled.
This is mostly a Hot-Fix, and a more structured UX/UI will be available once discussed with design and product.

Related issues

Fixes: The current Profile Syncing Feature doesn't fully respect Basic Functionality when turned off. This ensures that the notifications feature and profile syncing are actually turned off when Basic Functionality if off.

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Updated/Fixed Onboarding flow for Basic Functionality + Profile Syncing
See vid

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

github-actions · 2024-06-05T15:52:52Z

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.

…ned off.

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

this disabled the profile sync button and also shows some warning text for users that basic functionality is required for profile syncing

Prithpal-Sooriya · 2024-06-06T13:52:42Z

ui/components/multichain/global-menu/global-menu.js

-            flexDirection={FlexDirection.Row}
-            alignItems={AlignItems.center}
-            justifyContent={JustifyContent.spaceBetween}
+      {basicFunctionality && (


We are hiding notifications when basic functionality is disabled. We can rethink this in a future PR to re-engage users to notifications.

Yes, I agree. I'm not a big fan of the idea of completely hiding the functionality. It might be more interesting to follow the flow used for notifications: if the user wants to enable profile sync and basic functionality is turned off, then show a modal that informs the user that to enable profile sync, basic functionality must also be enabled. By clicking the button on the modal, we can cascade the turning on of basic functionality, authentication, and profile sync.

ui/contexts/metamask-notifications/metamask-notifications.tsx

ui/hooks/metamask-notifications/useProfileSyncing.ts

ui/hooks/metamask-notifications/useSwitchNotifications.test.tsx

ui/pages/onboarding-flow/privacy-settings/privacy-settings.js

Prithpal-Sooriya · 2024-06-06T14:01:02Z

ui/pages/settings/security-tab/profile-sync-toggle/profile-sync-toggle.tsx

+      setIsProfileSyncingEnabled(false);
+    }
+  }, [basicFunctionality, setIsProfileSyncingEnabled]);
+}

 const ProfileSyncToggle = () => {


(Prith Note) this is a little annoying, we have basically 2 of the same toggles. 1 for Onboarding, and another here. I wish we could use the same component (but I understand the use-case/logic is different.)

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

metamaskbot · 2024-06-06T15:14:03Z

Builds ready [5fa3bb7]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (52 ± 4 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	102	87	9	4
		domContentLoaded	9	13	10	1	1
		load	41	65	52	8	4
		domInteractive	8	13	10	1	1

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 1.08 KiB (0.02%)
common: 101 Bytes (0.00%)

Prithpal-Sooriya · 2024-06-06T15:52:26Z

privacy-snapshot.json

@@ -49,5 +49,6 @@
  "authentication.api.cx.metamask.io",
  "oidc.api.cx.metamask.io",
  "price.api.cx.metamask.io",
-  "token.api.cx.metamask.io"
+  "token.api.cx.metamask.io",
+  "sepolia.infura.io"


Ran the vault tests locally to make sure onboarding is okay. Needed to update the snapshot to include sepolia. Is this intended? (@MetaMask/notifications we do not touch any network logic)

Vault Test: yarn test:e2e:single test/e2e/vault-decryption-chrome.spec.js --browser=chrome, this flow turns off basic functionality so wanted to make sure the new fix/changes worked as expected.

I don't think this should be needed. You should be able to remove this change and this PR's build will still pass.

Prithpal-Sooriya · 2024-06-06T15:53:25Z

test/e2e/helpers.js

-  await driver.clickElement(
-    '[data-testid="profile-sync-toggle"] .toggle-button',
-  );
-  await driver.clickElement('[data-testid="submit-button"]');

  await Promise.all(
    (
      await driver.findClickableElements(
-        '.toggle-button.toggle-button--on:not([data-testid="basic-functionality-toggle"] .toggle-button):not([data-testid="profile-sync-toggle"] .toggle-button)',


We do not need this e2e setup for onboarding.

Context: This helper goes through onboarding without basic functionality (no additional APIs), and thus also turned off profile syncing. However now we automatically turn off this feature when basic functionality is disabled.

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

metamaskbot · 2024-06-07T11:00:25Z

Builds ready [9337e69]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (252 ± 291 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	149	87	18	8
		domContentLoaded	8	17	11	2	1
		load	42	2161	252	606	291
		domInteractive	8	17	11	2	1

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 964 Bytes (0.01%)
common: 0 Bytes (0.00%)

codecov · 2024-06-07T11:00:37Z

Codecov Report

Attention: Patch coverage is 82.97872% with 8 lines in your changes missing coverage. Please review.

Project coverage is 65.66%. Comparing base (ed5ec2d) to head (b296851).
Report is 3 commits behind head on develop.

Files	Patch %	Lines
...i/components/multichain/global-menu/global-menu.js	33.33%	4 Missing ⚠️
...boarding-flow/privacy-settings/privacy-settings.js	86.36%	3 Missing ⚠️
...ty-tab/profile-sync-toggle/profile-sync-toggle.tsx	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25070      +/-   ##
===========================================
+ Coverage    65.64%   65.66%   +0.02%     
===========================================
  Files         1362     1362              
  Lines        54189    54210      +21     
  Branches     14112    14119       +7     
===========================================
+ Hits         35572    35594      +22     
+ Misses       18617    18616       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matteoscurati

Great work. Just one comment for future development. Other than that, it’s super okay for me

matteoscurati · 2024-06-07T13:31:02Z

ui/components/multichain/global-menu/global-menu.js

-            flexDirection={FlexDirection.Row}
-            alignItems={AlignItems.center}
-            justifyContent={JustifyContent.spaceBetween}
+      {basicFunctionality && (


Yes, I agree. I'm not a big fan of the idea of completely hiding the functionality. It might be more interesting to follow the flow used for notifications: if the user wants to enable profile sync and basic functionality is turned off, then show a modal that informs the user that to enable profile sync, basic functionality must also be enabled. By clicking the button on the modal, we can cascade the turning on of basic functionality, authentication, and profile sync.

danjm · 2024-06-07T13:41:42Z

privacy-snapshot.json

@@ -49,5 +49,6 @@
  "authentication.api.cx.metamask.io",
  "oidc.api.cx.metamask.io",
  "price.api.cx.metamask.io",
-  "token.api.cx.metamask.io"
+  "token.api.cx.metamask.io",
+  "sepolia.infura.io"


I don't think this should be needed. You should be able to remove this change and this PR's build will still pass.

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

we don't actually need this change (CI passes without it)

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

metamaskbot · 2024-06-10T12:23:01Z

Builds ready [b296851]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (50 ± 3 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	104	84	11	5
		domContentLoaded	9	16	11	2	1
		load	41	62	50	6	3
		domInteractive	9	16	11	2	1

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 964 Bytes (0.01%)
common: 0 Bytes (0.00%)

Prithpal-Sooriya · 2024-06-10T12:27:33Z

@danjm yep you are correct, we do not need to update the privacy snapshot. Reverted the snapshot change.

matteoscurati

I approve again!

Prithpal-Sooriya added 2 commits June 3, 2024 21:05

feat: hide notification menu item if basic functionality if not enabled

50f025c

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

9377b25

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

Prithpal-Sooriya added the team-notifications Notifications team label Jun 5, 2024

Prithpal-Sooriya added 6 commits June 5, 2024 21:33

feat: prevent fetching notifications when basic functionality disabled

2436de1

refactor: fix hook usage and also test errors

73ae518

feat: ensure we disable profile syncing if basic functionality is tur…

fff4e06

…ned off.

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

450c02f

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

feat: add turn off functionality for profile sync in security settings

efb95a0

feat: profile sync basic functionally onboarding refactor

2fd0e6b

this disabled the profile sync button and also shows some warning text for users that basic functionality is required for profile syncing

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/contexts/metamask-notifications/metamask-notifications.tsx Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/hooks/metamask-notifications/useProfileSyncing.ts Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/hooks/metamask-notifications/useSwitchNotifications.test.tsx Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/pages/onboarding-flow/privacy-settings/privacy-settings.js Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/pages/onboarding-flow/privacy-settings/privacy-settings.js Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

ui/pages/onboarding-flow/privacy-settings/privacy-settings.js Outdated Show resolved Hide resolved

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

Prithpal-Sooriya added 2 commits June 6, 2024 15:01

refactor: update effect comments

5fa3bb7

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

3d0f071

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

Prithpal-Sooriya changed the title ~~feat(notifications): Ensure notifications feature is disabled/hidden when no basic functionality~~ fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality Jun 6, 2024

Prithpal-Sooriya added 2 commits June 6, 2024 16:49

refactor: modify fix based on Product feedback

4f7e445

test: update e2e vault tests

4327a69

Prithpal-Sooriya commented Jun 6, 2024

View reviewed changes

Prithpal-Sooriya marked this pull request as ready for review June 6, 2024 15:54

Prithpal-Sooriya requested review from a team as code owners June 6, 2024 15:54

Prithpal-Sooriya added 3 commits June 6, 2024 16:54

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

772a250

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

test: update unit tests and snapshots

26fd969

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

9337e69

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

matteoscurati previously approved these changes Jun 7, 2024

View reviewed changes

danjm requested changes Jun 7, 2024

View reviewed changes

Prithpal-Sooriya added 2 commits June 7, 2024 18:03

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

8e673b6

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

revert: remove privacy snapshot changes

86b72f7

we don't actually need this change (CI passes without it)

Prithpal-Sooriya dismissed matteoscurati’s stale review via 86b72f7 June 7, 2024 17:25

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

b296851

… NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled

Prithpal-Sooriya requested a review from danjm June 10, 2024 08:41

danjm approved these changes Jun 10, 2024

View reviewed changes

matteoscurati approved these changes Jun 10, 2024

View reviewed changes

Prithpal-Sooriya merged commit 556b43f into develop Jun 10, 2024
105 checks passed

Prithpal-Sooriya deleted the NOTIFY-724-hide-notifications-when-basic-functionality-is-disabled branch June 10, 2024 15:44

github-actions bot locked and limited conversation to collaborators Jun 10, 2024

metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality #25070

fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality #25070

Prithpal-Sooriya commented Jun 5, 2024 •

edited

Loading

github-actions bot commented Jun 5, 2024

Prithpal-Sooriya Jun 6, 2024

matteoscurati Jun 7, 2024

Prithpal-Sooriya Jun 6, 2024

metamaskbot commented Jun 6, 2024

Prithpal-Sooriya Jun 6, 2024 •

edited

Loading

Prithpal-Sooriya Jun 7, 2024

danjm Jun 7, 2024

Prithpal-Sooriya Jun 6, 2024 •

edited

Loading

metamaskbot commented Jun 7, 2024

codecov bot commented Jun 7, 2024 •

edited

Loading

matteoscurati left a comment

matteoscurati Jun 7, 2024

danjm Jun 7, 2024

metamaskbot commented Jun 10, 2024

Prithpal-Sooriya commented Jun 10, 2024

matteoscurati left a comment

fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality #25070

fix(notifications): Ensure notifications feature is disabled/hidden when no basic functionality #25070

Conversation

Prithpal-Sooriya commented Jun 5, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jun 5, 2024

Prithpal-Sooriya Jun 6, 2024

Choose a reason for hiding this comment

matteoscurati Jun 7, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Jun 6, 2024

Choose a reason for hiding this comment

metamaskbot commented Jun 6, 2024

Prithpal-Sooriya Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Prithpal-Sooriya Jun 7, 2024

Choose a reason for hiding this comment

danjm Jun 7, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Jun 7, 2024

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

matteoscurati left a comment

Choose a reason for hiding this comment

matteoscurati Jun 7, 2024

Choose a reason for hiding this comment

danjm Jun 7, 2024

Choose a reason for hiding this comment

metamaskbot commented Jun 10, 2024

Prithpal-Sooriya commented Jun 10, 2024

matteoscurati left a comment

Choose a reason for hiding this comment

Prithpal-Sooriya commented Jun 5, 2024 •

edited

Loading

Prithpal-Sooriya Jun 6, 2024 •

edited

Loading

Prithpal-Sooriya Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading