chore(mobile): refactor authentication #14322
Conversation
alextran1502
requested review from
zackpollard,
jrasm91,
fyfrey,
danieldietzler and
shenlong-tanwen
|
I looked through it briefly and it looks good overall. |
| UserAdminResponseDto? userResponse; | ||
| UserPreferencesResponseDto? userPreferences; | ||
| try { | ||
| final responses = await Future.wait([ |
There was a problem hiding this comment.
Prefer using a record here with .wait at the end and destructure it on the left side. The typing is much better.
| await db.assets.clear(); | ||
| await db.exifInfos.clear(); | ||
| await db.albums.clear(); | ||
| await db.eTags.clear(); | ||
| await db.users.clear(); |
There was a problem hiding this comment.
These can all be done concurrently. The function can also just return this future without being async.
| } catch (error, stackTrace) { | ||
| _log.severe("Error logging out", error, stackTrace); | ||
| } finally { | ||
| await clearLocalData(); |
There was a problem hiding this comment.
This can throw an exception as well.
There was a problem hiding this comment.
Throwing exception here will cause the logout timeout to not finish and prevent the app to return to the login screen. You can test this scenario by logging in an instance, then change the instance IP address and logout.
There was a problem hiding this comment.
No, I meant that clearLocalData calls the DB, so it could also throw an exception and needs to be try/cached.
| passwordController.text, | ||
| ); | ||
|
|
||
| if (result.shouldChangePassword && !result.isAdmin) { |
There was a problem hiding this comment.
Not sure why we wouldn't make them change their password even if they are an admin. If that flag is set, they should be changing their password. The server probably shouldn't even let them login which I think currently it does.
There was a problem hiding this comment.
This is the remnant of good ole "SAW". Technically, when the admin user is created, the shouldChangePassword flag should be set to false for that record. It is currently set to true. So we will need to logic the server to refactor this piece
Refactor authentication to follow the repository pattern, add tests
The last method in the authProvider that has not been refactored yet is
saveAuthInfo, which will need subsequent PRs to refactor other modules such as user and user's preferences