droidconKE / droidconKE2019App

Android app for the second Android Developer conference-droidcon in Nairobi 2019
https://appetize.io/app/wavc552uqmn76jaz7bd2bf890r
MIT License
16 stars 15 forks source link

Firebase remote config fetch failure causes `WifiDetailsRepoImpl` not to return default or cached values for wifi details #72

Open johnGachihi opened 4 years ago

johnGachihi commented 4 years ago

I noticed that when a firebase remote config (frc) fetch fails in WifiDetailsRepoImpl, instead of falling back to and returning the default or cached frc wifi details, FirebaseResult.error() is returned. This happens because the await() ext. function throws an exception whenever the fetch fails. And since it is wrapped in runCatching, a FirebaseResult.error() is returned.

Ideally, whenever an frc fetch fails, the frc defaults or cached values should be returned.

My approach to solve this would be to wrap this in a try-catch like so:

try {
      firebaseRemoteConfig.fetchAndActivate().await()
} catch (e: Exception) {}

so that even when the fetch fails this will be called

WifiDetailsModelFactory.create(firebaseRemoteConfig)

and return the default or cached values

wangerekaharun commented 4 years ago

Will check on this and see the best possible way to handle it with your suggestion in mind

johnGachihi commented 4 years ago

We could also use kotlin Flow to first emit the cached data then (in the background) refresh the cached data by fetching and emit the data again if it changes. This way even if the fetch fails the UI will already contain the cached data

// In WifiDetailsRepo
override fun fetchWifiDetails(): Flow<FirebaseResult<WifiDetailsModel>> = flow {
    val cachedTravelInfo = runCatching { TravelInfoModelFactory.create(firebaseRemoteConfig) }
    emit(cachedTravelInfo)

    val refreshedTravelInfo = runCatching {
      firebaseRemoteConfig.fetchAndActivate().await()
      TravelInfoModelFactory.create(firebaseRemoteConfig)
    }

    if (cachedTravelInfo != refreshedTravelInfo)
       emit(freshTravelInfo)
}

// In the ViewModel
fun getWifiDetails() = viewModelScope.launch(Dispatchers.IO) {
    wifiDetailsRepo.fetchWifiDetails().collect { value ->
      when (value) {
        is FirebaseResult.Success -> wifiDetails.postValue(value.data)
        is FirebaseResult.Error -> fetchError.postValue(value.exception)
      }
    }
}
wangerekaharun commented 4 years ago

@johnGachihi Anyway to reproduce the said error?

johnGachihi commented 4 years ago

Disconnect your phone from internet Then open the info page

wangerekaharun commented 4 years ago

Okay will test this and get back to you

wangerekaharun commented 4 years ago

On this case, am trying to investigate why it's not loading the default values in case of an error at the very least.

michaelbukachi commented 4 years ago

RemoteConfig doesn't work sometimes. I have faced this issue before.

wangerekaharun commented 4 years ago

Okay we can go with what @johnGachihi is suggesting

michaelbukachi commented 4 years ago

Haha. Flow to the rescue once again.

wangerekaharun commented 4 years ago

Haha. Flow to the rescue once again. @johnGachihi you can do that pull request.

Here are my thoughts on the implementation on EventTypeViewModel. I think the also function is not necessary on the wifiDetails variable.


    val wifiDetails: MutableLiveData<FirebaseResult<WifiDetailsModel>> by lazy {
        MutableLiveData<FirebaseResult<WifiDetailsModel>>().also {
            fetchWifiDetails()
        }
    }

Or what was your thinking in making it to be this way?

johnGachihi commented 4 years ago

Without it, the method fetchWifiDetails() would have to be called in EventFragment (as with fetchSession()) . So I saw that it will be called every time EventFragment is recreated.

wangerekaharun commented 4 years ago

Without it, the method fetchWifiDetails() would have to be called in EventFragment (as with fetchSession()) . So I saw that it will be called every time EventFragment is recreated.

Yes am aware of that #73 is aimed at improving that and avoid the methods being called on fragment recreation. Thank you for your clarification!

johnGachihi commented 4 years ago

Sorry, closed it by mistake

wangerekaharun commented 4 years ago

I have re-done the TravelFragment Remote config architecture, with what we were following previously with the app and it works without showing the error. Seems the problem is with your implementation bit. Let me do re-write and see if the error persists

wangerekaharun commented 4 years ago

@johnGachihi Latest commit fixes the issue

johnGachihi commented 4 years ago

@wangerekaharun, in TravelDetailsRepo, you left out await() after fetchAndActivate(). It's the one that was causing the issue in my code