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

Handle error gracefully when app url scheme is not found #2515

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

denniskniep
Copy link

Can we add an error handler to the AmazonAppUri processing? So that if someone does not have the app yet installed it is not crashing the authentication process? Then an hint can be displayed, that the app should be installed.

cc: @shahzaibj

@denniskniep denniskniep requested a review from a team as a code owner October 3, 2024 21:50
@denniskniep denniskniep force-pushed the feature/handle-error-when-activity-not-found branch from dc9262a to 44d6d00 Compare October 3, 2024 21:55
@denniskniep
Copy link
Author

@microsoft-github-policy-service agree

@shahzaibj
Copy link
Contributor

@denniskniep I'm not sure what a hint could be for installing the app. Is the app on PlayStore and/or is there a URL for downloading this app?

@denniskniep
Copy link
Author

denniskniep commented Oct 9, 2024

Then an hint can be displayed, that the app should be installed.

@shahzaibj What I meant with that was:
Currently the authentication process crashes entirely, which means the actual website is not visible anymore to the user, therefore the authentication process can not render any guidance to the user to resolve that issue. A potential guidance from the authentication process could be a hint, that the user should install an app.

But that is nothing I see in that library here. It must be implemented by the individual authentication process.
However, to enable this, it would be good to catch that error.

changelog.txt Outdated Show resolved Hide resolved
@denniskniep
Copy link
Author

@shahzaibj implemented your suggestions

@denniskniep
Copy link
Author

Hi @shahzaibj,
can you propose someone who can be the second reviewer?

@denniskniep
Copy link
Author

@rpdome any further feedback? Can we merge this?

@denniskniep denniskniep force-pushed the feature/handle-error-when-activity-not-found branch from 1bb6050 to 06ca6b7 Compare November 20, 2024 19:39
@denniskniep
Copy link
Author

@rpdome I kept up with recent changes in repo with rebase

changelog.txt Outdated Show resolved Hide resolved
@iamgusain
Copy link
Contributor

@denniskniep how did you test this?
Can that be written as an automated test?

Copy link
Contributor

@iamgusain iamgusain left a comment

Choose a reason for hiding this comment

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

Approved with suggestion.

@denniskniep
Copy link
Author

denniskniep commented Nov 25, 2024

how did you test this? Can that be written as an automated test?

@iamgusain I didn´t wrote a unit-test so far for this case. If you want me to write a test for that catch block, I can look into that.

@denniskniep
Copy link
Author

Hi,
I merged dev into that pr to resolve conflicts

@iamgusain @shahzaibj There are now two approvals, can we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants