Fix pagination load more #12545
Fix pagination load more #12545
Conversation
atorresveiga
added
type: bug
|
Found 1 violations: The PR caused the following dependency changes:expand
-+--- org.wordpress:fluxc:trunk-3d8b46e8151493d563fd519c4406a5b83bc95255
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-| | \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.25
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
-| | +--- com.google.crypto.tink:tink-android:1.5.0
-| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
-| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
-| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
-| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-| +--- org.apache.commons:commons-text:1.10.0 (*)
-| +--- androidx.room:room-runtime:2.6.1 (*)
-| +--- androidx.room:room-ktx:2.6.1
-| | +--- androidx.room:room-common:2.6.1 (*)
-| | +--- androidx.room:room-runtime:2.6.1 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 1.9.25 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
-| | +--- androidx.room:room-common:2.6.1 (c)
-| | \--- androidx.room:room-runtime:2.6.1 (c)
-| +--- com.google.dagger:dagger:2.51.1
-| | \--- javax.inject:javax.inject:1
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
-| +--- org.wordpress:wellsql:2.0.0
-| | +--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-| +--- org.wordpress.fluxc:fluxc-annotations:trunk-3d8b46e8151493d563fd519c4406a5b83bc95255
-| +--- org.greenrobot:eventbus-java:3.3.1
-| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| +--- com.android.volley:volley:1.1.1 -> 1.2.0
-| +--- androidx.paging:paging-runtime:2.1.2
-| | +--- androidx.paging:paging-common:2.1.2
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.8.0 (*)
-| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.7.0 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.7.0 (*)
-| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
-| +--- com.goterl:lazysodium-android:5.0.2
-| +--- net.java.dev.jna:jna:5.5.0
-| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
++--- org.wordpress:fluxc:3090-3d6d1df2e25d7ea902359f75488bd95541c23df3
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+| | \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.25
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
+| | \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
+| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+| | +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
+| | +--- com.google.crypto.tink:tink-android:1.5.0
+| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
+| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
+| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
+| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+| +--- org.apache.commons:commons-text:1.10.0 (*)
+| +--- androidx.room:room-runtime:2.6.1 (*)
+| +--- androidx.room:room-ktx:2.6.1
+| | +--- androidx.room:room-common:2.6.1 (*)
+| | +--- androidx.room:room-runtime:2.6.1 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 1.9.25 (*)
+| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
+| | +--- androidx.room:room-common:2.6.1 (c)
+| | \--- androidx.room:room-runtime:2.6.1 (c)
+| +--- com.google.dagger:dagger:2.51.1
+| | \--- javax.inject:javax.inject:1
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+| +--- org.wordpress:wellsql:2.0.0
+| | +--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
+| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+| +--- org.wordpress.fluxc:fluxc-annotations:3090-3d6d1df2e25d7ea902359f75488bd95541c23df3
+| +--- org.greenrobot:eventbus-java:3.3.1
+| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| +--- com.android.volley:volley:1.1.1 -> 1.2.0
+| +--- androidx.paging:paging-runtime:2.1.2
+| | +--- androidx.paging:paging-common:2.1.2
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.8.0 (*)
+| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.7.0 (*)
+| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.7.0 (*)
+| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
+| +--- com.goterl:lazysodium-android:5.0.2
+| +--- net.java.dev.jna:jna:5.5.0
+| \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:trunk-3d8b46e8151493d563fd519c4406a5b83bc95255
- +--- org.wordpress:fluxc:trunk-3d8b46e8151493d563fd519c4406a5b83bc95255 (*)
- +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
- +--- com.google.dagger:dagger:2.51.1 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
- +--- androidx.room:room-runtime:2.6.1 (*)
- +--- org.wordpress:wellsql:2.0.0 (*)
- +--- org.wordpress.fluxc:fluxc-annotations:trunk-3d8b46e8151493d563fd519c4406a5b83bc95255
- +--- androidx.room:room-ktx:2.6.1 (*)
- \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:3090-3d6d1df2e25d7ea902359f75488bd95541c23df3
+ +--- org.wordpress:fluxc:3090-3d6d1df2e25d7ea902359f75488bd95541c23df3 (*)
+ +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+ +--- com.google.dagger:dagger:2.51.1 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+ +--- androidx.room:room-runtime:2.6.1 (*)
+ +--- org.wordpress:wellsql:2.0.0 (*)
+ +--- org.wordpress.fluxc:fluxc-annotations:3090-3d6d1df2e25d7ea902359f75488bd95541c23df3
+ +--- androidx.room:room-ktx:2.6.1 (*)
+ \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
Please review and act accordingly
|
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #12545 +/- ##
=========================================
Coverage 40.60% 40.60%
- Complexity 5629 5632 +3
=========================================
Files 1223 1223
Lines 68595 68600 +5
Branches 9473 9475 +2
=========================================
+ Hits 27851 27856 +5
Misses 38184 38184
Partials 2560 2560 ☔ View full report in Codecov by Sentry. |
| 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( |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
Closes: #12491
Description
This PR fixes the issue that prevented the load more behavior from triggering after reaching the end of the screen. The problem was that because of a timeout on the
getListStatefunction, the function returned aneed-to-refreshstate instead of the required to trigger the load more behaviorcan-load-more.This PR fixes this issue with two main changes:
can-load-moreandfetched.ShouldUpdateOrdersListto consider the list status. With this change, we aim to prevent inconsistent states in case the app is closed before reaching one of the final states.Steps to reproduce
Testing information
The tests that have been performed
Images/gif
Screen_recording_20240905_210837.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: