fix(mobile): mobile logging out randomly #11431
Conversation
|
Approved. But also I wrote a lot of the code so let's get at least one review from the mobile guys 🙂 |
There was a problem hiding this comment.
I don't really follow the logic of how this improves the random logouts, but I'm reviewing on my phone without adequate coffee haha! It looks very good and clean. I would like to see some more refactoring. Perhaps some comments about where specifically in this code the random logouts are fixed?
Thank you, I hope my review helps.
| error, | ||
| stackTrace, | ||
| ); | ||
| UserAdminResponseDto? userResponse; |
There was a problem hiding this comment.
This variable will be re-assigned and checked for null.
| stackTrace, | ||
| ); | ||
| UserAdminResponseDto? userResponse; | ||
| UserPreferencesResponseDto? userPreferences; |
There was a problem hiding this comment.
These variables will be re-assigned and checked for null.
| ]); | ||
| userResponse = responses[0] as UserAdminResponseDto; | ||
| userPreferences = responses[1] as UserPreferencesResponseDto; | ||
| } on ApiException catch (error, stackTrace) { |
There was a problem hiding this comment.
How does Future.wait handle exceptions? Will this be thrown in this manner?
There was a problem hiding this comment.
Any exceptions will be presented in the same way. When the await for the Future.wait resolves or rejects
| @@ -19,45 +18,22 @@ class SplashScreenPage extends HookConsumerWidget { | |||
| Widget build(BuildContext context, WidgetRef ref) { | |||
| final apiService = ref.watch(apiServiceProvider); | |||
| final serverUrl = Store.tryGet(StoreKey.serverUrl); | |||
| final endpoint = Store.tryGet(StoreKey.serverEndpoint); | |||
| final accessToken = Store.tryGet(StoreKey.accessToken); | |||
| final log = Logger("SplashScreenPage"); | |||
|
|
|||
| void performLoggingIn() async { | |||
There was a problem hiding this comment.
Is there any way to put this function somewhere else? I typically don't expect to find lots of business logic in the build function of a widget. Maybe we can add this function to a relevant provider. The ui should really just be watching the state of the notifier and updating accordingly.
There was a problem hiding this comment.
I appreciate that, but also this is the current state, we are trying to fix the problem without creating more problems by refactoring stuff too much.
Cheers for the review. To give a brief idea of the changes. Firstly, we no longer call to the well-known endpoint on every start, reducing one network call. This also removes a bunch of logic dealing with online or offline mode there. In general, the code now assumes internet connectivity and will callback to the local stored values if the network calls fail. Previously we did all kinds of checks for if the calls succeed or fail and whether things are in stores and it was just needlessly complicated. So now the main logic pulls the value from the store, then does login and updates the values if those calls succeed, then there is a single check for if the user is set and if it is we move on to the main screen. Another major change was just handling all exceptions from these calls. We now ONLY logout if we don't have the local values we need, or the server returns 401 (the token is revoked). Anything else we absorb and assume the server will come back eventually, if not the user can logout, for example if the server URL has changed. Feel free to ask any more questions here or on discord 🙂 |
|
I am wondering if that change caused #11518. |
Fixes #2980
Fixes #9745
Fixes #8146
This PR simplifies and fixes the issue of occasionally logging out the user when the
.well-knowendpoint or the getMyUser endpoint returns a status code other than503when it cannot be reached. We should now always login provided we have a local version of the user (one successful login has occurred previously) and the API doesn't return a 401 (Auth token has been cancelled)