baocntt2602 / survey

0 stars 0 forks source link

[Concern] Handle Authenticator's failure case better #15

Open minhnimble opened 11 months ago

minhnimble commented 11 months ago

It is good to see you implement refreshing token logic with standard practice - using an Authenticator. Now, let's imagine that a user revoked their sign-in in all devices, the Authenticator would likely fail and return a default UserToken() here: https://github.com/baocntt2602/survey/blob/f4ba36619f2d92808c6f861e8b4b38a21b2430d7/core/network/src/main/java/com/nimble/sample/network/api/middleware/TokenAuthenticator.kt#L21-L36

A common UX sense would be to be logout the user, and I saw you had some code to handle it in MainViewModel. However, in fact, state was never used, and you only used the isAuthenticated flag only:

https://github.com/baocntt2602/survey/blob/f4ba36619f2d92808c6f861e8b4b38a21b2430d7/app/src/main/java/com/nimble/survey/MainViewModel.kt#L15-L36

As a result, the user probably has to restart the app to see the Login screen, right?

baocntt2602 commented 11 months ago

As I wanted to focus on the main and optional requirements mentioned in the Test description, I didn't handle this case. The reason I left the code there is to demonstrate one of the advantages of using DataStore rather than SharedPreference, which is to react to change. I have created a PR to continue the logic there, for testing purpose I also added a profile image view on the top right corner of the Home screen which has a clickable event to simulate the log out funtionality