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

feat(jumpstart): add specific error messaging when link is already claimed #5427

Merged
merged 3 commits into from
May 21, 2024

Conversation

kathaypacific
Copy link
Collaborator

Description

As the title

Test plan

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-05-17.at.10.47.07.mp4

Related issues

Backwards compatibility

Y

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented May 17, 2024

Codecov Report

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

Project coverage is 86.27%. Comparing base (c49d3f5) to head (6a5e6df).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5427      +/-   ##
==========================================
+ Coverage   86.20%   86.27%   +0.06%     
==========================================
  Files         748      753       +5     
  Lines       30474    30688     +214     
  Branches     5196     5243      +47     
==========================================
+ Hits        26271    26475     +204     
- Misses       3975     3985      +10     
  Partials      228      228              
Files Coverage Δ
src/jumpstart/jumpstartLinkHandler.ts 90.32% <100.00%> (+1.03%) ⬆️
src/jumpstart/saga.ts 89.80% <100.00%> (+0.06%) ⬆️
src/jumpstart/selectors.ts 100.00% <100.00%> (ø)
src/jumpstart/slice.ts 50.00% <100.00%> (+2.38%) ⬆️
src/jumpstart/JumpstartClaimStatusToasts.tsx 90.62% <83.33%> (-2.48%) ⬇️

... and 29 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c49d3f5...6a5e6df. Read the comment docs.

Copy link
Contributor

@jophish jophish left a comment

Choose a reason for hiding this comment

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

general comment about distinguishing error types, but no changes needed :)

Logger.error(TAG, 'Error handling jumpstart link', error)
ValoraAnalytics.track(JumpstartEvents.jumpstart_claim_failed)
yield* put(jumpstartClaimFailed())
yield* put(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the pattern of parsing the error message to determine the error type is prolly fine here, since we're only discerning between two types of error, though in general this does seem like it has the potential to break stuff if we're not careful... e.g. if i'm adding a new error, i have to know that it can't contain certain substrings else other code will interpret it as a certain "kind" of error. one alternative is having some custom error type that we stick some error enum value on, which eliminates that confusion.

i think for this case it's fine since this is simple and we prolly won't add more error types, but wanted to call it out since i've definitely run into error-parsing-hell when dealing with handling errors from external libs that are only distinguished by their (inconsistently formatted) messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah good shout, this is a fair point. i've been mulling this over but i think there's always going to be some level of brittleness with error handling here because there are multiple points that the claim flow can throw (including when we generate the transaction and when we make the network request to claim the transaction). so we don't have control over every error that can exist, and (afaik) it's not possible to enforce any checks on exactly which errors are thrown (i.e. we can declare a map of custom errors, but that relies on the sagas actually using the map when creating the errors) which at least for now seems like extra effort but still leaves the door open for us to screw this up in the future. so i guess for now my preference would be to leave it as is and think about it more when we add more custom errors - i'm not sure that is likely :)

@kathaypacific kathaypacific added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit 8dd5080 May 21, 2024
16 checks passed
@kathaypacific kathaypacific deleted the kathy/fix-live-links-messaging branch May 21, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants