Skip to content
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

fix: 9559 issue android json parse #9568

Merged
merged 32 commits into from
May 21, 2024
Merged

Conversation

Jonathansoufer
Copy link
Contributor

@Jonathansoufer Jonathansoufer commented May 7, 2024

Description

Fix issue on Android while handling notifications (JSON parse)

Related issues

Fixes: 9559

Manual testing steps

  1. Enable notifications feature (feature flag)
  2. Submit any transaction within the app

Screenshots/Recordings

Before

After

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.

@Jonathansoufer Jonathansoufer added team-mobile-platform Run Smoke E2E Triggers smoke e2e on Bitrise team-notifications Notifications team labels May 7, 2024
@Jonathansoufer Jonathansoufer self-assigned this May 7, 2024
@Jonathansoufer Jonathansoufer requested a review from a team as a code owner May 7, 2024 11:15
Copy link
Contributor

github-actions bot commented May 7, 2024

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.

Copy link
Contributor

github-actions bot commented May 7, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 2e67dfb
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bb0f5cef-ce58-4d6b-bf32-ce88df6dc7d1

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 46.78%. Comparing base (678c2f6) to head (b8c7c7a).
Report is 5 commits behind head on main.

Files Patch % Lines
app/util/notifications/hooks/index.ts 26.31% 14 Missing ⚠️
app/components/Nav/Main/index.js 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9568      +/-   ##
==========================================
+ Coverage   46.66%   46.78%   +0.12%     
==========================================
  Files        1343     1344       +1     
  Lines       32805    32803       -2     
  Branches     3527     3525       -2     
==========================================
+ Hits        15307    15347      +40     
+ Misses      16554    16511      -43     
- Partials      944      945       +1     

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

@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 9, 2024
Copy link
Contributor

github-actions bot commented May 9, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: d57cff4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/26355779-bd92-4bca-811e-0404f2e09202

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@Cal-L
Copy link
Contributor

Cal-L commented May 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions 23.3% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

Let's increase test coverage for the notifications hook. That should be able to clear the threshold.

Jonathansoufer and others added 2 commits May 9, 2024 21:11
Co-authored-by: Cal Leung <cleun007@gmail.com>
Co-authored-by: Cal Leung <cleun007@gmail.com>
.yarnrc Outdated Show resolved Hide resolved
@legobeat legobeat requested review from Gudahtt and a team May 20, 2024 17:25
@sethkfman sethkfman added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 21, 2024
Copy link
Contributor

github-actions bot commented May 21, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f22f949
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/81339d1e-90f7-442f-a5e1-f13f655de3df

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@NicolasMassart
Copy link
Contributor

hi @Jonathansoufer, could you fix this PR description and an explicit unit test to prove that #9559 is fixed by this PR please?
I would expect a unit test that uses a mock JSON similar to the one that made the app fail before to make sure we can consider this PR as a fix for the parsing issue. Thanks!

Copy link

sonarcloud bot commented May 21, 2024

Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman merged commit 6c5f55c into main May 21, 2024
30 of 31 checks passed
@sethkfman sethkfman deleted the fix/9559-issue-android-JSON-parse branch May 21, 2024 21:21
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2024
@metamaskbot metamaskbot added the release-7.24.0 Issue or pull request that will be included in release 7.24.0 label May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.24.0 Issue or pull request that will be included in release 7.24.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Sentry] SyntaxError: JSON Parse error: Unexpected token: o
7 participants