Skip to content

feat: segment data deletion (4/4)#8090

Merged
NicolasMassart merged 27 commits into
feat/batch_1129_segmentfrom
1403_Segment_data_deletion_request
Jan 23, 2024
Merged

feat: segment data deletion (4/4)#8090
NicolasMassart merged 27 commits into
feat/batch_1129_segmentfrom
1403_Segment_data_deletion_request

Conversation

@NicolasMassart

@NicolasMassart NicolasMassart commented Dec 13, 2023

Copy link
Copy Markdown
Contributor

Description

  1. What is the reason for the change?
    User has to be able to request deletion of their metrics data stored in new segment system.

  2. What is the improvement/solution?

  • adds createDataDeletionTask to MetaMetrics to call the deletion API in MetaMetrics class for method create source regulation
  • adds the checkDataDeletionTaskStatus to MetaMetrics to call deletion API get regulation method
  • calls the createDataDeletionTask and checkDataDeletionTaskStatus from DeleteMetaMetricsData.tsx when user touches the delete button.
  • extract the settings section logic to display allow deletion to a well tested hook.
  • Remove Analytics.js implementation of these features and point only to the new MetaMetrics ones.
  • update unit tests
  • rename in storage.js:
    • METAMETRICS_SEGMENT_REGULATION_ID -> METAMETRICS_DELETION_REGULATION_ID

Related issues

Fixes https://github.com/MetaMask/mobile-planning/issues/1403

Manual testing steps

Note

Testing is non regression testing as the feature should work exactly the same as before.

There's 3 cases here:

  • initial deletion
  • user opted IN and goes back on settings once a deletion request has been initiated
  • user opted OUT and goes back on settings once a deletion request has been initiated
Feature: opted-in or out user deletes MetaMetrics Data for the first time
  Given the app is installed and setup
  And user goes into settings / security & privacy
  And no deletion request has been done before
  And "Delete Metametrics data" section text is "This will delete historical 
    MetaMetrics data associated with your wallet.[...] View the ConsenSys Privacy Policy."

  When user touches the "delete metametrics" button
  And approves deletion

  Then "Delete Metametrics data" section text changes to "You initiated this action on [current date]. 
    This process can take up to 60 days. View the ConsenSys Privacy Policy."
  And "delete metametrics" button is now disabled
  And user still continues to be tracked or not depending on opt-out statusbut
  And the tracking strategy is not changed by the deletion action

Feature: opted-in user is back on MetaMetrics settings screen after deletion
  Given user has opted-in for metrics
  And user asked for metrics data deletion
  And user exit settings screen

  When user comes back to settings / security & privacy
  And users scrolls down to the delete metametrics section

  Then "Delete Metametrics data" section text is "This will delete historical 
    MetaMetrics data associated with your wallet.[...] View the ConsenSys Privacy Policy."
  And user can click the "delete metametrics" button again

Feature: opted-out user is back on MetaMetrics settings screen after deletion
  Given user has opted-out for metrics
  And user asked for metrics data deletion
  And user exit settings screen

  When user comes back to settings / security & privacy
  And users scrolls down to the delete metametrics section

  Then "Delete Metametrics data" section text is "You initiated this action on [current date]. 
    This process can take up to 60 days. View the ConsenSys Privacy Policy."
  And "delete metametrics" button is disabled
  And user can NOT click the "delete metametrics" button again until data deletion task is finished

Screenshots/Recordings

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2024-01-16.at.16.25.15.mp4

"Security and Privacy" settings "Delete Metametrics data" section BEFORE deletion request:
image

"Security and Privacy" settings "Delete Metametrics data" section AFTER deletion request:
image

"Delete MetaMetrics Data Request Submitted" event tracked on Segment and visible in the dashboard:

{
  "context": {
    "library": {
      "name": "source-functions",
      "version": "1.0.0"
    },
    "location": {
      "api_processing_time_ms": 520,
      "country_code": "FR",
      "region": "NOR",
      "timezone": "Europe/Paris"
    }
  },
  "event": "Delete MetaMetrics Data Request Submitted",
  "integrations": {},
  "messageId": "e09b2aac-dd78-44d5-8c48-9ee1464836b1",
  "originalTimestamp": "2023-12-13T23:50:58.790Z",
  "properties": {
    "device_model": "Apple iPhone14,2",
    "os": "ios",
    "os_version": "17.0.1"
  },
  "receivedAt": "2023-12-13T23:51:31.609Z",
  "sentAt": "2023-12-13T23:51:31.609Z",
  "timestamp": "2023-12-13T23:50:58.790Z",
  "type": "track",
  "userId": "35a6b109-d2b4-4d1f-a3ee-7dd454bd8667",
  "writeKey": "REDACTED"
}

Deletion Request appears as "INITIALIZED" in the Segment Deletion Requests dashboard.
You can make sure this is your request by matching the ID with the ID that should have been sent to segment during the onboarding and by the date and time of the request.
image

Before

No change, this PR only makes this work with Segment, but UX should be the same

After

No change, this PR only makes this work with Segment, but UX should be the same

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@NicolasMassart NicolasMassart changed the base branch from main to feat/batch_1129_segment December 14, 2023 00:54
@MetaMask MetaMask deleted a comment from socket-security Bot Dec 14, 2023
@MetaMask MetaMask deleted a comment from socket-security Bot Dec 14, 2023
@MetaMask MetaMask deleted a comment from codecov-commenter Dec 14, 2023
@MetaMask MetaMask deleted a comment from sonarqubecloud Bot Dec 14, 2023
@NicolasMassart NicolasMassart changed the title feat: segment data deletion feat: segment data deletion (4/4) Dec 14, 2023
@NicolasMassart NicolasMassart mentioned this pull request Dec 14, 2023
13 tasks
@NicolasMassart NicolasMassart marked this pull request as ready for review December 14, 2023 21:15
@NicolasMassart NicolasMassart requested a review from a team as a code owner December 14, 2023 21:15
@NicolasMassart NicolasMassart added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 14, 2023
@codecov-commenter

codecov-commenter commented Dec 14, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (8e795c6) 40.33% compared to head (d50986f) 40.50%.

Files Patch % Lines
app/core/Analytics/MetaMetrics.ts 87.34% 8 Missing and 2 partials ⚠️
...ecuritySettings/Sections/DeleteMetaMetricsData.tsx 65.21% 5 Missing and 3 partials ⚠️
app/core/Analytics/Analytics.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                     @@
##           feat/batch_1129_segment    #8090      +/-   ##
===========================================================
+ Coverage                    40.33%   40.50%   +0.16%     
===========================================================
  Files                         1238     1239       +1     
  Lines                        29985    30007      +22     
  Branches                      2882     2890       +8     
===========================================================
+ Hits                         12095    12154      +59     
+ Misses                       17197    17157      -40     
- Partials                       693      696       +3     

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

…n_request

# Conflicts:
#	app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments

Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
Comment thread app/core/Analytics/MetaMetrics.ts Outdated
Comment thread app/core/Analytics/MetaMetrics.ts Outdated
Comment thread app/core/Analytics/MetaMetrics.ts Outdated
Comment thread app/core/Analytics/MetaMetrics.ts
Comment thread app/core/Analytics/MetaMetrics.ts
Comment thread app/core/Analytics/MetaMetrics.ts
@NicolasMassart NicolasMassart self-assigned this Jan 8, 2024

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@NicolasMassart NicolasMassart force-pushed the 1403_Segment_data_deletion_request branch 2 times, most recently from 478642d to 651fccf Compare January 16, 2024 14:55
@NicolasMassart NicolasMassart force-pushed the 1403_Segment_data_deletion_request branch from 651fccf to 2960af7 Compare January 16, 2024 15:00

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments

Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
Comment thread app/components/Views/Settings/SecuritySettings/Sections/DeleteMetaMetricsData.tsx Outdated
@sonarqubecloud

Copy link
Copy Markdown

@NicolasMassart

NicolasMassart commented Jan 17, 2024

Copy link
Copy Markdown
Contributor Author

e2e smoke tests running on https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ed2be129-6fa5-4cad-b493-698b1405b64e

Important

The e2e test fails on tests that are absolutely not related to my changes (token import).
and as it also fails for other branches, we agreed that this should not prevent this PR
to be checked by QA team and then to be approved.

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@NicolasMassart NicolasMassart added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) DO-NOT-MERGE Pull requests that should not be merged Run Smoke E2E labels Jan 18, 2024

@cortisiko cortisiko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did some manual spot checks and noticed no issues with the overall functionality with the app. Furthermore, I went ahead and kicked off regression: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/64641e5f-8c13-4b58-9474-5cdced2d5730 and tests passed. ✅

@cortisiko cortisiko added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 22, 2024
@NicolasMassart NicolasMassart merged commit 8419e09 into feat/batch_1129_segment Jan 23, 2024
@NicolasMassart NicolasMassart deleted the 1403_Segment_data_deletion_request branch January 23, 2024 11:10
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 23, 2024
@gauthierpetetin gauthierpetetin added the team-mobile-platform Mobile Platform team label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed QA testing has been completed and passed team-mobile-platform Mobile Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants