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

Bugfix/live 5056 error mapping not found entity #5315

Merged

Conversation

ofreyssinet-ledger
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger commented Nov 3, 2023

📝 Description

Improve the error mapping for the 404 response from some endpoints of the manager API, which is due to the provider number being wrong for the requested firmware version.
Currently the message shown is either:

  • in the best case Device firmware is not recognized, please contact Ledger support on LLD if the error comes from the /get_device_version API call.
  • in all other cases not found entity.

The new improved message is:

  • title: Invalid Provider
  • description: You have to change "My Ledger provider" setting. To change it, open Ledger Live "Settings", select "Experimental features", and then select a different provider.

❓ Context

  • Impacted projects: common lld llm
  • Linked resource(s): LIVE-5056

✅ Checklist

  • Test coverage
  • Atomic delivery
  • No breaking changes

📸 Demo

LLD

Screen.Recording.2023-11-03.at.17.25.56.mov

LLM

Simulator.Screen.Recording.-.iPhone.14.-.2023-11-03.at.17.06.38.mp4

LLM old device pairing flow

RPReplay_Final1699269969.MP4

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

Copy link

changeset-bot bot commented Nov 3, 2023

🦋 Changeset detected

Latest commit: 6da823c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
ledger-live-desktop Patch
live-mobile Patch
@ledgerhq/live-common Patch
@ledgerhq/live-cli Patch
web-tools Patch
@ledgerhq/test-utils Patch
dummy-wallet-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2023 8:58am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 8:58am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 8:58am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 8:58am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 8:58am

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common translations Translation files have been touched labels Nov 3, 2023
@ofreyssinet-ledger ofreyssinet-ledger marked this pull request as ready for review November 3, 2023 16:36
@@ -457,9 +465,9 @@ const getDeviceVersion: (targetId: string | number, provider: number) => Promise
target_id: targetId,
},
}).catch(error => {
const status = error && (error.status || (error.response && error.response.status)); // FIXME LLD is doing error remapping already. we probably need to move the remapping in live-common
const status = error?.status || error?.response?.status;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah cleaner :)

Copy link
Contributor

@alexandremgo alexandremgo left a comment

Choose a reason for hiding this comment

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

🦄

Copy link
Contributor

@jdabbech-ledger jdabbech-ledger left a comment

Choose a reason for hiding this comment

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

Nice fix LGTM

@ofreyssinet-ledger ofreyssinet-ledger force-pushed the bugfix/LIVE-5056-error-mapping-not-found-entity branch from 7091bfd to 6da823c Compare November 8, 2023 08:54
@ofreyssinet-ledger ofreyssinet-ledger merged commit 19c7484 into develop Nov 8, 2023
@ofreyssinet-ledger ofreyssinet-ledger deleted the bugfix/LIVE-5056-error-mapping-not-found-entity branch November 8, 2023 11:33
sshmaxime pushed a commit that referenced this pull request Nov 9, 2023
* fix(common, llm, lld): better wording of not found entity error

* chore: changeset

* fix(common/manager api): remap 404 error in get_firmware_version

* fix(lld, llm): wording capitalisation

* fix(llm/pairing/RenderError): properly render wrong provider error

* refactor(common/manager/api): clearer code + remove 4 year old FIXME

* fix(common/manager/api): remove bad practice of checking error message
lvndry pushed a commit that referenced this pull request Nov 14, 2023
* fix(common, llm, lld): better wording of not found entity error

* chore: changeset

* fix(common/manager api): remap 404 error in get_firmware_version

* fix(lld, llm): wording capitalisation

* fix(llm/pairing/RenderError): properly render wrong provider error

* refactor(common/manager/api): clearer code + remove 4 year old FIXME

* fix(common/manager/api): remove bad practice of checking error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants