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.

Sign up for GitHub

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

Jump to bottom

Fix pagination load more #12545

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Fix pagination load more #12545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3555e8
update FluxC version
Sep 6, 2024
e4fdcf2
check list state to update the list
Sep 6, 2024
424ab5e
refactor listId -> listDescriptor
Sep 6, 2024
ef9b1aa
update unit tests
Sep 6, 2024
0dfce14
fix detekt issues
Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/OrderListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ class OrderListViewModel @Inject constructor(
)
val listId = listDescriptor.uniqueIdentifier.value
launch {
if (shouldUpdateOrdersList(listId)) {
if (shouldUpdateOrdersList(listDescriptor)) {
fetchOrdersAndOrderDependencies()
} else {
// List is displayed from cache
Expand Down
15 changes: 12 additions & 3 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/ShouldUpdateOrdersList.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,21 @@ package com.woocommerce.android.ui.orders.list

import com.woocommerce.android.background.LastUpdateDataStore
import kotlinx.coroutines.flow.first
import org.wordpress.android.fluxc.model.list.ListDescriptor
import org.wordpress.android.fluxc.model.list.ListState
import org.wordpress.android.fluxc.store.ListStore
import javax.inject.Inject

class ShouldUpdateOrdersList @Inject constructor(private val lastUpdateDataStore: LastUpdateDataStore) {
suspend operator fun invoke(listId: Int): Boolean {
return lastUpdateDataStore.getLastUpdateKeyByOrdersListId(listId).let { key ->
class ShouldUpdateOrdersList @Inject constructor(
Copy link
Contributor

@malinajirka malinajirka Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atorresveiga I'm wondering if we truly need this, it feels a bit off or at least unexpected to me. One of the main goals of the ListStore component is to manage lists for us automatically and avoid handling refreshes from the client. The ListState.NEEDS_REFRESH is an event emitted and consumed automatically within fluxC which should take care of updating the list for us and propagating that to the UI. We shouldn't need to check this state here and act on it. Could you please elaborate on why we need to handle this manually here?

P.S. I read the PR description, but I'm either misunderstanding it or it doesn't explain why a client side fix is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @malinajirka 👋 thank for sharing your concerns.

One of the main goals of the ListStore component is to manage lists for us automatically and avoid handling refreshes from the client.

While the ListStore component could provide many handy tools to implement pagination and the common logic to fetch fresh data, I feel that it should be up to the client apps to decide whether this data is stale. This is what happened with the Background Tasks project.

The Background Task project aimed to improve the app's perceived performance by relying more on cached data. As part of this project, we wanted to decide whether the cached data was outdated. For this, we defined a 30-minute threshold. If the cached data is not older than 30 minutes, we present that data; otherwise, we fetch the info from the API.

Could you please elaborate on why we need to handle this manually here?

Sure! As mentioned above, we wanted to check if the data in the DB was older than 30 minutes and decide if we needed to re-fetch everything or rely on that data cached. The decision of whether the data is stale or not is now owned by Woo. With this goal in mind, I implemented the ShouldUpdateOrdersList. This use case checks if the last update timestamp for the list description is still valid and returns a boolean that we use later to fetch the order's info or not.

Now, why do I need to check the ListState.NEEDS_REFRESH status?
In FluxC, we have implemented an expired logic for list states, you can check the function isListStateOutdated.

Here is the description for that function:

A helper function that returns whether it has been more than a certain time has passed since it's lastModified.
Since we keep the state in the DB, in the case of application being closed during a fetch, it'll carry over to the next session. To prevent such cases, we use a timeout approach. If it has been more than a certain time since the list is last updated, we should ignore the state.

So, there could be edge cases where the app is closed when the list is in an intermediary state (like fetching-frist-page). In those scenarios, I would want to refresh everything even if I have a valid timestamp. Does that make sense?
To sum up, I want the last update timestamp to be the source of truth when deciding whether to update the list or not, unless the list is in an inconsistent state. Let me know if this information helps! It's important to mention that as part of the FluxC PR, I prevented the final states can_load_more and fetched from expiring. This means we can still use them even if they are older than 1 minute, and because they are final states, I don't think there was a reason to discard them.

private val lastUpdateDataStore: LastUpdateDataStore,
private val listStore: ListStore
) {
suspend operator fun invoke(listDescriptor: ListDescriptor): Boolean {
val listId = listDescriptor.uniqueIdentifier.value
val shouldUpdateByState = listStore.getListState(listDescriptor) == ListState.NEEDS_REFRESH
val shouldUpdateByCache = lastUpdateDataStore.getLastUpdateKeyByOrdersListId(listId).let { key ->
lastUpdateDataStore.shouldUpdateData(key).first()
}
return shouldUpdateByState || shouldUpdateByCache
}
}
55 changes: 50 additions & 5 deletions ...test/kotlin/com/woocommerce/android/ui/orders/list/ShouldUpdateOrdersListByStoreIdTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,78 @@ import org.mockito.kotlin.doReturn
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
import org.wordpress.android.fluxc.model.WCOrderListDescriptor
import org.wordpress.android.fluxc.model.list.ListDescriptorUniqueIdentifier
import org.wordpress.android.fluxc.model.list.ListState
import org.wordpress.android.fluxc.store.ListStore
import kotlin.test.assertTrue

@OptIn(ExperimentalCoroutinesApi::class)
class ShouldUpdateOrdersListByStoreIdTest : BaseUnitTest() {
private val lastUpdateDataStore: LastUpdateDataStore = mock()
val sut = ShouldUpdateOrdersList(lastUpdateDataStore)
private val lisStore: ListStore = mock()
val sut = ShouldUpdateOrdersList(lastUpdateDataStore, lisStore)

@Test
fun `when should update return true, then the result is the expected`() = testBlocking {
fun `when should update return true and list state is not refresh, then the result is the expected`() = testBlocking {
val listId = 1
val key = "key-1"
val listDescriptor: WCOrderListDescriptor = mock {
on { uniqueIdentifier }.doReturn(ListDescriptorUniqueIdentifier(listId))
}
whenever(lastUpdateDataStore.getLastUpdateKeyByOrdersListId(eq(listId))).doReturn(key)
whenever(lastUpdateDataStore.shouldUpdateData(eq(key), any())).doReturn(flowOf(true))
whenever(lisStore.getListState(eq(listDescriptor))).doReturn(ListState.CAN_LOAD_MORE)

val result = sut.invoke(listId)
val result = sut.invoke(listDescriptor)

assertTrue(result)
}

@Test
fun `when should update return false, then the result is the expected`() = testBlocking {
fun `when should update return true and list state is refresh, then the result is the expected`() = testBlocking {
val listId = 1
val key = "key-1"
val listDescriptor: WCOrderListDescriptor = mock {
on { uniqueIdentifier }.doReturn(ListDescriptorUniqueIdentifier(listId))
}
whenever(lastUpdateDataStore.getLastUpdateKeyByOrdersListId(eq(listId))).doReturn(key)
whenever(lastUpdateDataStore.shouldUpdateData(eq(key), any())).doReturn(flowOf(true))
whenever(lisStore.getListState(eq(listDescriptor))).doReturn(ListState.NEEDS_REFRESH)

val result = sut.invoke(listDescriptor)

assertTrue(result)
}

@Test
fun `when should update return false and list state is refresh, then the result is the expected`() = testBlocking {
val listId = 1
val key = "key-1"
val listDescriptor: WCOrderListDescriptor = mock {
on { uniqueIdentifier }.doReturn(ListDescriptorUniqueIdentifier(listId))
}
whenever(lastUpdateDataStore.getLastUpdateKeyByOrdersListId(eq(listId))).doReturn(key)
whenever(lastUpdateDataStore.shouldUpdateData(eq(key), any())).doReturn(flowOf(false))
whenever(lisStore.getListState(eq(listDescriptor))).doReturn(ListState.NEEDS_REFRESH)

val result = sut.invoke(listDescriptor)

assertTrue(result)
}

@Test
fun `when should update return false and list state is not refresh, then the result is the expected`() = testBlocking {
val listId = 1
val key = "key-1"
val listDescriptor: WCOrderListDescriptor = mock {
on { uniqueIdentifier }.doReturn(ListDescriptorUniqueIdentifier(listId))
}
whenever(lastUpdateDataStore.getLastUpdateKeyByOrdersListId(eq(listId))).doReturn(key)
whenever(lastUpdateDataStore.shouldUpdateData(eq(key), any())).doReturn(flowOf(false))
whenever(lisStore.getListState(eq(listDescriptor))).doReturn(ListState.CAN_LOAD_MORE)

val result = sut.invoke(listId)
val result = sut.invoke(listDescriptor)

assertFalse(result)
}
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ tasks.register("installGitHooks", Copy) {
}

ext {
fluxCVersion = 'trunk-3d8b46e8151493d563fd519c4406a5b83bc95255'
fluxCVersion = '3090-3d6d1df2e25d7ea902359f75488bd95541c23df3'
glideVersion = '4.16.0'
coilVersion = '2.1.0'
constraintLayoutVersion = '1.2.0'
Expand Down
Loading