Skip to content

feat(notifications): use notification services push controller#26448

Merged
Prithpal-Sooriya merged 18 commits intodevelopfrom
notify_840-use-notification-services-push-controller
Aug 16, 2024
Merged

feat(notifications): use notification services push controller#26448
Prithpal-Sooriya merged 18 commits intodevelopfrom
notify_840-use-notification-services-push-controller

Conversation

@Prithpal-Sooriya
Copy link
Copy Markdown
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Aug 15, 2024

Description

This is another incremental migration, to use the shared/core libraries. This replaces the extension push controller for the @metamask/notification-services-controller push service controller.

Open in GitHub Codespaces

Related issues

N/A

Manual testing steps

Test full notifications flow, specifically the push notifications.

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@github-actions
Copy link
Copy Markdown
Contributor

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.

@socket-security
Copy link
Copy Markdown

socket-security bot commented Aug 15, 2024

@socket-security
Copy link
Copy Markdown

socket-security bot commented Aug 15, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Network access npm/@metamask/notification-services-controller@0.2.1 🚫
Network access npm/axios@1.7.4 🚫
Network access npm/contentful@10.14.0 🚫
Network access npm/contentful@10.14.0 🚫

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/@metamask/notification-services-controller@0.2.1
  • @SocketSecurity ignore npm/axios@1.7.4
  • @SocketSecurity ignore npm/contentful@10.14.0

@Prithpal-Sooriya Prithpal-Sooriya added the team-notifications-deprecated DEPRECATED: please use "team-assets" instead label Aug 15, 2024
'app/scripts/constants/**/*.ts',
'app/scripts/platforms/**/*.js',
'app/scripts/controllers/push-platform-notifications/utils/get-notification-message.ts',
'app/scripts/controllers/**/*.ts',
Copy link
Copy Markdown
Contributor Author

@Prithpal-Sooriya Prithpal-Sooriya Aug 15, 2024

Choose a reason for hiding this comment

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

Updating this setting as this file does not exist.

We still have translation strings inside the /controllers folder (e.g. the translations for push notifications, this unfortunately cannot be shared cross platform).

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 74.54545% with 14 lines in your changes missing coverage. Please review.

Project coverage is 70.18%. Comparing base (2d63c0e) to head (e8dec1f).

Files Patch % Lines
...pp/scripts/controllers/push-notifications/index.ts 12.50% 14 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26448      +/-   ##
===========================================
+ Coverage    70.12%   70.18%   +0.06%     
===========================================
  Files         1428     1423       -5     
  Lines        50089    49859     -230     
  Branches     13894    13854      -40     
===========================================
- Hits         35120    34990     -130     
+ Misses       14969    14869     -100     

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

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review August 15, 2024 20:42
@Prithpal-Sooriya Prithpal-Sooriya requested review from a team and kumavis as code owners August 15, 2024 20:42
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [051a485]
Page Load Metrics (65 ± 10 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint71145962110
domContentLoaded96324147
load40105652110
domInteractive96324147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 152.45 KiB (4.06%)
  • ui: 0 Bytes (0.00%)
  • common: 199 Bytes (0.00%)

Copy link
Copy Markdown
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [e8dec1f]
Page Load Metrics (146 ± 148 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint592841106129
domContentLoaded1086272110
load391467146308148
domInteractive1085262110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 152.45 KiB (4.06%)
  • ui: 0 Bytes (0.00%)
  • common: 199 Bytes (0.00%)

@sonarqubecloud
Copy link
Copy Markdown

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [099abdf]
Page Load Metrics (282 ± 287 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint941861262412
domContentLoaded225633105
load612141282597287
domInteractive215632105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 152.45 KiB (4.06%)
  • ui: 0 Bytes (0.00%)
  • common: 199 Bytes (0.00%)

@Prithpal-Sooriya Prithpal-Sooriya merged commit 7916145 into develop Aug 16, 2024
@Prithpal-Sooriya Prithpal-Sooriya deleted the notify_840-use-notification-services-push-controller branch August 16, 2024 14:34
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 16, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-notifications-deprecated DEPRECATED: please use "team-assets" instead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants