Login Refactor 1 #203
Login Refactor 1 #203
Conversation
…into Caleb/LoginRefactor
…into Caleb/LoginRefactor
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Nice job overall, but getting an issue when trying to login which is making this hard to test completely. Lmk what you think!
| @Json(name = "wait_times") val waitTimes: List<WaitTimeDay>? = null, | ||
| @Json(name = "alerts") val alerts: List<Alert>? = null, | ||
| ) { | ||
| // todo - investigate unused methods |
There was a problem hiding this comment.
If there are methods that are completely unused, we can just get rid of them
| val eateries = getAllEateries() | ||
| _eateryFlow.value = EateryApiResponse.Success(eateries) | ||
| eateryApiCache.update { map -> | ||
| eateryApiCache.update { _ -> |
There was a problem hiding this comment.
Nit: we should be able to just remove the _ -> here?
| } | ||
|
|
||
| suspend fun getDeviceId(): String? { | ||
| val id: String? = userPreferencesFlow.first().deviceId |
There was a problem hiding this comment.
Prefer firstOrNull to avoid crashes (the string is nullable anyway!)
| /** | ||
| * The currently loaded user. Null if no user is logged in. | ||
| */ | ||
| var loadedUser: User? = null |
There was a problem hiding this comment.
I think this should be done with a MutableStateFlow<User>, having one that is private to the repository, and then exposing a StateFlow<User> that the VMs can read from to find the loaded user. We definitely should not be setting a public variable on the repository like this.
There was a problem hiding this comment.
Thanks for catching this. Apparently I somehow forgot about flows when I wrote this.
| val bearerToken = "Bearer $sessionId" | ||
| val authorizedUser = networkApi.authorizeUser( | ||
| sessionId = bearerToken, | ||
| loginRequest = LoginRequest(deviceId = deviceId, pin = 1234, fcmToken = fcmToken) |
There was a problem hiding this comment.
what's the deal with this arbitrary pin? Also, what server URL is this using? When I test this I get this bad request 400 error: {"error":"deviceId, pin required"}
There was a problem hiding this comment.
I was told by backend to keep that there for now, but I'll check in with them again on that. I am using https://eatery-dev.cornellappdev.com/ for testing.
| ExperimentalFoundationApi::class, | ||
| ExperimentalAnimationApi::class | ||
| ) | ||
| private fun AccountPageContent( |
There was a problem hiding this comment.
Thank you for extracting this and denesting the UI! Could we get a preview for this 🥺
| onSettingsClicked: () -> Unit, | ||
| accountTypeBalance: AccountBalances, | ||
| accountFilter: TransactionAccountType, | ||
| coroutineScope: CoroutineScope, |
There was a problem hiding this comment.
nit: we can just get this with rememberCoroutineScope
| ) | ||
| _state.value = newState | ||
| } catch (e: Exception) { | ||
| // todo - error state |
There was a problem hiding this comment.
can we make a github issue about this so it isn't forgotten? I feel like an error state for failed login is fairly important.
| val user = userRepository.getUser(sessionId, deviceId, fcmToken) | ||
| userRepository.loadedUser = user |
There was a problem hiding this comment.
we shouldn't have to be doing this setting up of the user. Ideally we should just read the user flow from the repository.
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
This could be working, but due to the BE refactor it cannot be tested. As we discussed this PR will stay in this branch, and you can make another PR into this one for the updated code
Overview
We now use the new backend routes for login logic.
Changes Made
Next Steps
We are still awaiting some minor backend updates
meal_swipesfield (number of meal swipes left)transactionTypefield (+/- of the transaction)