Skip to content

feat: Segment instance error handling#8218

Merged
NicolasMassart merged 4 commits into
feat/batch_1129_segmentfrom
1129_segment_instance_error_handling
Jan 24, 2024
Merged

feat: Segment instance error handling#8218
NicolasMassart merged 4 commits into
feat/batch_1129_segmentfrom
1129_segment_instance_error_handling

Conversation

@NicolasMassart

@NicolasMassart NicolasMassart commented Jan 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Adds error handling on async DefaultPreference.get and DefaultPreference.set
Makes building instance stronger by using the Config type to prevent config mistakes

Related issues

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

Manual testing steps

NA

Screenshots/Recordings

NA

Before

NA

After

NA

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.

@NicolasMassart NicolasMassart mentioned this pull request Jan 8, 2024
Merged
13 tasks
@github-actions

github-actions Bot commented Jan 8, 2024

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 January 8, 2024 19:14
@MetaMask MetaMask deleted a comment from socket-security Bot Jan 8, 2024
@MetaMask MetaMask deleted a comment from codecov-commenter Jan 8, 2024
@NicolasMassart NicolasMassart self-assigned this Jan 8, 2024
@NicolasMassart NicolasMassart marked this pull request as ready for review January 9, 2024 22:14
@NicolasMassart NicolasMassart requested a review from a team as a code owner January 9, 2024 22:14
@metamaskbot metamaskbot added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 9, 2024
@github-actions

github-actions Bot commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/65b8cd61-10bf-4403-9dbd-a124dd16dc4c
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@codecov-commenter

codecov-commenter commented Jan 9, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Comparison is base (054b3b6) 39.95% compared to head (ff6ca2a) 39.95%.

Files Patch % Lines
app/core/Analytics/MetaMetrics.ts 60.52% 13 Missing and 2 partials ⚠️
Additional details and impacted files 
@@                     Coverage Diff                     @@
##           feat/batch_1129_segment    #8218      +/-   ##
===========================================================
- Coverage                    39.95%   39.95%   -0.01%     
===========================================================
  Files                         1236     1236              
  Lines                        29920    29936      +16     
  Branches                      2859     2859              
===========================================================
+ Hits                         11956    11961       +5     
- Misses                       17273    17284      +11     
  Partials                       691      691

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

@NicolasMassart NicolasMassart mentioned this pull request Jan 10, 2024
Merged
13 tasks
@NicolasMassart NicolasMassart requested a review from Cal-L January 10, 2024 15:24
Cal-L
Cal-L approved these changes Jan 10, 2024
View reviewed changes

@Cal-L Cal-L left a comment •
edited
Loading

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.

Code LGTM. Can you quickly verify that deletion is still reflected in Segment? Just for safe measure.

@metamaskbot metamaskbot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 10, 2024
@NicolasMassart NicolasMassart added the DO-NOT-MERGE Pull requests that should not be merged label Jan 17, 2024
@NicolasMassart

NicolasMassart commented Jan 17, 2024

Copy link
Copy Markdown
Contributor Author

Merging after #8090 to limit the amount of conflicts

@NicolasMassart
Merge branch 'feat/batch_1129_segment' into 1129_segment_instance_err…
9abf740 
…or_handling

# Conflicts:
#	app/core/Analytics/MetaMetrics.ts
@NicolasMassart NicolasMassart force-pushed the 1129_segment_instance_error_handling branch from 6055b1e to 9abf740 Compare January 24, 2024 11:18
@NicolasMassart NicolasMassart added No QA Needed Apply this label when your PR does not need any QA effort. and removed DO-NOT-MERGE Pull requests that should not be merged labels Jan 24, 2024
@sonarqubecloud

sonarqubecloud Bot commented Jan 24, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
45.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@NicolasMassart NicolasMassart merged commit 5e871ce into feat/batch_1129_segment Jan 24, 2024
@NicolasMassart NicolasMassart deleted the 1129_segment_instance_error_handling branch January 24, 2024 11:36
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 24, 2024
@NicolasMassart NicolasMassart restored the 1129_segment_instance_error_handling branch January 24, 2024 12:15
@NicolasMassart

NicolasMassart commented Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a9b92771-d2c2-4af4-adf0-c2a2a0c29666

@NicolasMassart NicolasMassart deleted the 1129_segment_instance_error_handling branch January 24, 2024 12:42
@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.

Reviewers

@Cal-L Cal-L Cal-L approved these changes

Assignees

@NicolasMassart NicolasMassart

Labels

in-progress No QA Needed Apply this label when your PR does not need any QA effort. team-mobile-platform Mobile Platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@NicolasMassart @codecov-commenter @Cal-L @gauthierpetetin @metamaskbot