Offline Support : Pages - PostUploadComplete logic issue

[jd-alexander]

Expected behavior

When the page is uploaded and it needs to be refreshed to show the changes that took place, it should simply run a refresh from the database since it was updated and we are getting the completed event.

WordPress-Android/WordPress/src/main/java/org/wordpress/android/viewmodel/pages/PagesViewModel.kt

Line 210 in 1adbfd7

private suspend fun reloadPages(state: PageListState = REFRESHING) {

The function above could be renamed to fetchPagesFromServerAndRefreshUi to make it clear what it does, since it's possible that this bug was caused by reloadPages and refreshPages being close in name and intent.

Actual behavior

The entire page is basically reloaded; the page is re-fetched from the server which isn't necessary in this case.

Steps to reproduce the behavior

1. Switch to this branch Offline Pages : PageList Item Progress State  #11191
2. Trigger an upload and if you put a breakpoint in the function below you will see that the pages are being fetched and the "local changes" if it exists will have a delay with being refreshed.
   

   WordPress-Android/WordPress/src/main/java/org/wordpress/android/viewmodel/pages/PagesViewModel.kt

   Line 210 in 1adbfd7

   private suspend fun reloadPages(state: PageListState = REFRESHING) {

Tested on Google Pixel 3a, Android 10, WPAndroid #11191

[jd-alexander]

So when I run the refreshPages instead of reloadPages the label local changes label is still shown 🤔

[malinajirka]

If the changes were just remote-auto-saved, then it's a correct behavior, since not all of the changes were uploaded to the server. Remote-auto-save uploads just title, content, excerpt (maybe one more field).
If the changes were really uploaded, PushPost method stores the updated page (with localChanges = false) to the local DB. So I believe it should work.

Let me know if this info explains some of the behavior or if it still doesn't behave as expected. Thanks!

[jd-alexander]

Thanks for that explanation! I will double-check it tomorrow.

[jd-alexander]

This note you left on the PR #11332 was really helpful!

Note:
It seems the progress state and item's ui is sometimes temporarily de-synced (there is +- 3 second delay). So when I for example publish a post, the progress indicator disappears as soon as the post gets published, but the PageItem gets updated after a delay. Therefore "Post will be published when your device is back online" label is shown even though the post has already been published. @jd-alexander If I recall correctly we've already been dealing with this issue before. Any ideas what is causing the delay? (I've looked into the issue, and the root cause seems to be that we update _pages livedate multiple times within a short time period. The UI receives only the latest update which might unfortunately contain out-dated values, if invalidatePages finished after refreshPages. Either case I believe the issue isn't related to this PR.)

What I realized is that when a post is being uploaded the UpdatePost and PostChanged events get triggered during the process so that is what causes the delay. I added breakpoints to both invalidatePages and refreshPages and logged the breakpoints instead of suspending them and I got the same order you described where the invalidatePages finishes after refreshPages because it's being called multiple times while the post is being updated so when the post upload completes it was called shortly before so the invalidation occurs again. I had some solutions in mind but they felt a bit too hacky ( such as disabling invalidatePages during post upload). I went and looked at how the PostList handles this but I wasn't able to track down all of the ListStore logic that does the actual refreshes/invalidation because the PostList has these same events & behavior and works well. So I think we could both go through this issue on a call to see if there is something different about how updates are being on the PageList that's different than the PostList.