-
Notifications
You must be signed in to change notification settings - Fork 555
fix: Display unauthorization error while loading events #2110
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
Conversation
| } | ||
| } | ||
| }, { | ||
| if ((it as HttpException).code() == 401) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define constant in HttpErrors.kt.
| } | ||
| } | ||
| }, { | ||
| if ((it as HttpException).code() == 401) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said by codacy, this is an Unsafe cast. You can't know if the exception is HttpException, it can be IOException IllegalArgumentException, anything and you just casting it is waiting for a crash to happen.
Also, it's the developer's responsibility to handle any error except 500, where you can show Unfortunately, something went wrong, because you actually don't know what happened.
All other errors must be handled. As a user, what am I supposed to do with a toast that says HTTP 401 UNAUTHORIZED?
I should either see You must log in to see this {item}, or if I am already logged in, then Your session has expired, please login again
And maybe being redirected to login screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should either see
You must log in to see this {item}, or if I am already logged in, thenYour session has expired, please login again
I was not sure it is because of the user's login state. It shows when I try to change API and do not clear the app data. And events should load if the user is not logged in. I think it is due to the wrong Authorization which needs to re-request the Authenticator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said by codacy, this is an Unsafe cast. You can't know if the exception is HttpException, it can be IOException IllegalArgumentException, anything and you just casting it is waiting for a crash to happen.
Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to change API and do not clear the app data.
Then clear the data when API change happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mean clear the app data programmatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because access token from one API won't work in another. This is very easily handled in Orga App. We simply show login screen anytime we encounter 401. Reason may be expired token or changing API base URL. Fixes every case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It shows in case of login state only. For that error, we can log out the user and reload events. Because events should also load in not logged in state. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fetching settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry wrong PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Fixes #2109
Fixes #2111