Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
johnGachihi opened this issue Dec 24, 2019 · 16 comments

Comments

@johnGachihi
Copy link
Contributor

johnGachihi commented Dec 24, 2019

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
Copy link
Collaborator

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

@johnGachihi
Copy link
Contributor Author

johnGachihi commented Jan 11, 2020

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
Copy link
Collaborator

@johnGachihi Anyway to reproduce the said error?

@johnGachihi
Copy link
Contributor Author

johnGachihi commented Jan 13, 2020

Disconnect your phone from internet
Then open the info page

@wangerekaharun
Copy link
Collaborator

Okay will test this and get back to you

@wangerekaharun
Copy link
Collaborator

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
Copy link
Collaborator

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

@wangerekaharun
Copy link
Collaborator

Okay we can go with what @johnGachihi is suggesting

@michaelbukachi
Copy link
Collaborator

Haha. Flow to the rescue once again.

@wangerekaharun
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

Sorry, closed it by mistake

@johnGachihi johnGachihi reopened this Jan 16, 2020
@wangerekaharun
Copy link
Collaborator

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 added a commit that referenced this issue Jan 17, 2020
…r for reomote config when there is no internet. This addresses #73 and #72
@wangerekaharun
Copy link
Collaborator

@johnGachihi Latest commit fixes the issue

@johnGachihi
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants