Added tests for EventFragment and moved FRC implementation from EventFragment into a repository

[johnGachihi]

No description provided.

[wangerekaharun]

Gone through your changes @johnGachihi.

In the WifiDetailsRepoImpl you are using a suspendCancellableCoroutine whereas in PR #52 we introduced runCatching top level function for wrapping all Firebase calls. Then again there is a couple of differences in how you are exposing data to the view. We shall have to discuss this so that we maintain consistency across all layers

[johnGachihi]

About the WifiDetailsRepoImpl, is this a better approach:
https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d

[wangerekaharun]

@johnGachihi we have .await() from ktx so you can just use that one. You can cross check the implementation on other repos too

[johnGachihi]

Nice. I didn't know .await() existed. So doesn't that mean the functions in this file are unnecessary

[johnGachihi]

@johnGachihi we have .await() from ktx so you can just use that one. You can cross check the implementation on other repos too

Is this better: https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d.
Or have I made it worse

[michaelbukachi]

Hi @johnGachihi I've just gone through the snippet and noticed FRC. I think using such initials tends to bring more confusion. Names should be as clear as possible so I suggest you change that. Also, why are you giving the WifiDetailsModel a body. You should create an object instead and put the extra logic there. The model should contain the fields only.

[johnGachihi]

I have edited it @michaelbukachi
https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d

[michaelbukachi]

Cool. Looks better.
One last thing. You should rename createUsingFirebaseRemoteConfigData to just create since WifiDetailsModelFactory.create(firebaseRemoteConfig) paints a clear picture of what is going on.

[wangerekaharun]

The functions in that file were way long before ktx was added so now they are not in use. We are still doing the cleanup to remove unused code

[wangerekaharun]

One question to @michaelbukachi and @johnGachihi . Now that @johnGachihi implemementation exposes the sealed class to the view, should we do a re-write to all the viewmodel layers to follow the same?

[johnGachihi]

I took that approach because it only requires one livedata for all states of data. The current approach requires livedata for each state of data. For example, for organizers data in AboutViewModel there are

• organizersLiveData for successful fetch
• organizersError for unsuccessful fetch

This means the view has to observe the two livedata to know the state of the organizers data. But since the data can't take more than one state (e.g the fetch can't be both successful and unsuccessful), only one of the two livedata will emit data in the lifetime of the view. The other livedata is observed though it never emits a value. If the data had more states e.g loading then there would be more livedata the view observes though they never emit a value in the view's lifetime

With approach I took the view only needs to listen to one livedata.
But I see FirebaseResult should be renamed to lack the word Firebase if it is to be exposed to views.

[michaelbukachi]

But one state carries the message and the other carries the result. Since the types are not the same, I think it's okay to have different livedata. The only change I would make is having one LiveData for handling all errors. We cannot use Result as name since it's already used in Kotlin.

[wangerekaharun]

Using one LiveData for all errors will bring exceptions lets say for example we have one error before then try to update the value with another error. Not sure if the behavior is same with LiveData but for MediatorLiveData you cannot do the same

[michaelbukachi]

I'm lost 😅 . Could you share a snippet to demonstrate this?

[johnGachihi]

@wangerekaharun I not sure I understood correctly but is this what you meant:

livedata.setValue(FirebaseResult.Error("error") // Observer will be updated
...
livedata.setValue(FirebaseResult.Error("error") // Observer will not be updated 
                                                // since value set  is the same as previous one

[wangerekaharun]

So for example this Original File has a couple of livedata for error then as to my understanding of having one error for livedata checkout this gist. @michaelbukachi let me know if this is what your were saying

[wangerekaharun]

@johnGachihi see in my gist the second setting value for the error livedata will cause an exception. Unless the behaviour changed, cause i tried the same back in early 2018 and was getting exceptions

[johnGachihi]

I can't access the gist. The link is taking me to a 404

[wangerekaharun]

I can't access the gist. The link is taking me to a 404

Fixed

[michaelbukachi]

@wangerekaharun Yes that's what I meant. Just all exceptions are delivered as strings and delivered to the view for displaying in Snackbar/Toast have multiple liveData just makes a bit repetitive.

[wangerekaharun]

@wangerekaharun Yes that's what I meant. Just all exceptions are delivered as strings and delivered to the view for displaying in Snackbar/Toast have multiple liveData just makes a bit repetitive.

Let me do a sample and see if there will be any issues especially on rotation change. There is that exception where lets say we have an error in method 1 and you update again to set value in method 2 when there is also an error, you get this exception

If the given LiveData is already added as a source but with a different Observer, IllegalArgumentException will be thrown.

[wangerekaharun]

@johnGachihi You can now commit all changes suggested then we merge the PR

[johnGachihi]

Including the changes on the livedata?

[wangerekaharun]

No the others. Will test on the Livedata part

[johnGachihi]

Alright

[michaelbukachi]

@johnGachihi You can now commit all changes suggested then we merge the PR

Finally. Now, I can go play some games 😆 .