Automatically upload local drafts #9774

shiki · 2019-05-01T20:34:16Z

Closes #9568.

This adds auto-upload of Local Drafts when any one of these happens.

Scenario	Before	After
Swipe to refresh
Connection is restored
Post List is opened

The auto-upload never happens if the Post List is not open. I think this is one of those things that we will need to have an architectural and design discussion about soon.

Not Covered Yet

For this PR, I wanted to focus on the process flow and architecture so I intentionally did not include these.

Auto-upload in Pages.
Fix auto-upload if the post has a media that was not uploaded.
Auto-upload Locally Changed posts. I was also not sure if this adds more complications for Discarding Local Changes. Ref Discarding Local Changes don't remove the Locally Changed status #8957.

I have updated the description of #9568 to list those that have not been covered yet.

FluxC PR

There is a companion FluxC PR for this: wordpress-mobile/WordPress-FluxC-Android#1243.

Testing

Create a draft post while offline.
Leave the Editor before going back online. Do not Publish the post.
Try the scenarios described above.

Release Notes

If there are user-facing changes, I have added an item to RELEASE-NOTES.txt.

Status

This is waiting for Make ConnectionStatusLiveData distinct #9821 to be reviewed and merged.

The method `LocalDraftUploadStarter.uploadLocalDrafts` is called when: - The internet comes back online - The Posts activity is shown - When the user performs a pull-to-refresh in any of the Posts tabs

- Removed unnecessary `@Provides` 🤦‍♂️ - Using a simple `getPostsForSite()` method for now. This should probably be replaced with a FluxC method

It does not need to be a singleton right now. Perhaps later.

Also added helpers.

In this way, we can guarantee that `site` is not `null`.

The `mocked*` can be misleading because the returned instances are not mocks. It is the parameters that are mocked.

WordPress/src/main/java/org/wordpress/android/ui/uploads/LocalDraftUploadStarter.kt

build.gradle

malinajirka

Thanks @shiki;)! Good job - the code looks good and it works as described! I've left a couple of in-code comments.

The auto-upload never happens if the Post List is not open. I think this is one of those things that we will need to have an architectural and design discussion about soon.

I believe, even if we decide to create a sync service or something similar we'll still be able to reuse most of this code. ;)

For this PR, I wanted to focus on the process flow and architecture so I intentionally did not include these.
Auto-upload in Pages.

I think the LocalDraftUploadStarter will upload even pages as postStore.getLocalDrafts(site) returns both posts and pages. If you meant, that you haven't added the logic to start the LocalDraftUploadStarter from the pages list screen, then ignore this comment :).

SideNote: I've also noticed the ConnectionStatusLiveData emits changes even when the internet connection changes from LTE -> Wifi or other way round. We might want to consider adding getDistinct(). Wdyt?

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt

malinajirka · 2019-05-02T07:18:52Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt


-    fun start(site: SiteModel) {
+    /**
+     * @param postListEventsListenerEnabled This is only provided to make this class testable in unit tests.


I think we might want to consider wrapping fun listenForPostListEvents(..) with an injectable class and inject it into the PostListMainViewModel (inject mocked version in unit tests). I'd rather avoid @ VisibleForTesting unless we really need it. Wdyt?

Thank you for noticing this. This was the part that I didn't like much. An injectable class would probably be better. I might end up making PostListEventListener public or refactoring it a bit. I'll think about it. 🤔

Used a Factory in e26e540.

malinajirka · 2019-05-02T07:32:28Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/LocalDraftUploadStarter.kt

+        }
+
+        val localDrafts = postStore.getLocalDrafts(site)
+        localDrafts.forEach { UploadService.uploadPost(context, it) }


I think the internet connection broadcast receiver can send multiple updates within a short timeframe. If I'm not missing anything this code will put multiple instances of the post into the UploadService queue. Should we check for example PostUploadHandler.isPostUploadingOrQueued. I know it'd be far from fool proof solution, but it might at least reduce the number of duplicate requests? Wdyt?

I might have misread UploadService.unpackPostIntent. I saw the usage of PostUploadHandler.isPostUploadingOrQueued in there and just assumed that it will ignore posts that are currently in the queue. I should have tested that.

My original plan for this class was for it to be a @Singleton and manage its own queue so it will not do a query and upload in a short timeframe. I abandoned that to keep it simple and get your feedback. 🙂 Which turned out to be a good decision. I'll wait for the discussion with Maxime and everyone else before moving forward.

I ended up just using UploadService.isPostUploadingOrQueued in fa94f00. I figured we'll get to the complicated queuing portion in #9846.

shiki · 2019-05-02T12:51:41Z

Great review, @malinajirka!

I think the LocalDraftUploadStarter will upload even pages as postStore.getLocalDrafts(site) returns both posts and pages.

Answered in this FluxC conversation.

I've also noticed the ConnectionStatusLiveData emits changes even when the internet connection changes from LTE -> Wifi or other way round. We might want to consider adding getDistinct(). Wdyt?

Good catch! Discussed in this conversation.

mattmiklic · 2019-05-02T20:23:14Z

Design-wise this looks pretty good, I have two things I noticed:

When the local draft begins to upload, the status changes from "Local Draft" to "Uploading Post." My first reaction was to fear that my draft was being published, not just saved as a draft on the server. Could we change this to "Uploading Draft" so it's clearer that it's still a draft?
I think in the original design of this progress bar, the intention was for it to be flush against the bottom edge of the post's card background. Can we remove the extra space below the progress bar? Here's an example of what it looks like now and what I'm requesting.

malinajirka · 2019-05-03T06:18:49Z

Thanks @mattmiklic! I'll jump in as it's more related to the recent post list changes than to this PR.

When the local draft begins to upload, the status changes from "Local Draft" to "Uploading Post." My first reaction was to fear that my draft was being published, not just saved as a draft on the server. Could we change this to "Uploading Draft" so it's clearer that it's still a draft?

Makes sense;). We can certainly do that, I'd suggest creating a separate ticket as it seems unrelated.

I think in the original design of this progress bar, the intention was for it to be flush against the bottom edge of the post's card background. Can we remove the extra space below the progress bar? Here's an example of what it looks like now and what I'm requesting.

I discussed this issue with Sylvester. The problem is the bottom padding is added there by the system (it's a part of the progress bar). We could somehow workaround it, but we ran out of time for the project so we decided to keep it as it is for now.

shiki · 2019-05-03T14:15:14Z

Thank you, @mattmiklic! I created #9790 for the label rename.

megsfulton · 2019-05-06T15:26:50Z

Agree with @mattmiklic's feedback. Otherwise looks good!

This method is what is used for uploading in Post List → Retry and it fixes the issue with uploads failing if they have a media.

This is so we can mock it in unit tests. It does not currently work in unit tests because of the `EventBus` usage.

…9568-upload-local-drafts # Conflicts: # WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt # build.gradle

Logic is still intact. The dependencies just changed.

# Conflicts: # build.gradle

malinajirka · 2019-05-14T08:04:54Z

@shiki Just to make sure we are on the same page and we are not waiting on each other :P. This is still marked as "Not Ready for Review".

shiki · 2019-05-14T11:20:19Z

@malinajirka Thanks for checking. Yep, I'll fix the conflicts, retest, and let you know when it's ready.

shiki · 2019-05-14T12:34:37Z

@malinajirka This is now ready for review again. 🙂

malinajirka

Thanks @shiki ! I've reviewed the changes and tested the app again and everything looks good to me;)! Good job!

shiki added 14 commits April 29, 2019 14:28

Add hooks for when to auto-upload local drafts

7e0e43d

The method `LocalDraftUploadStarter.uploadLocalDrafts` is called when: - The internet comes back online - The Posts activity is shown - When the user performs a pull-to-refresh in any of the Posts tabs

Make PostListViewModel inherit from ScopedVM

f8dd996

Auto upload local drafts in Posts list

023c1ee

- Removed unnecessary `@Provides` 🤦‍♂️ - Using a simple `getPostsForSite()` method for now. This should probably be replaced with a FluxC method

LocalDraftUploadStarter: Use FluxC call

b612b2e

Remove @singleton from LocalDraftUploadStarter

b88021d

It does not need to be a singleton right now. Perhaps later.

LocalDraftUploadStarter: Add network check

9135289

Add PostListViewModelTest for swipe to refresh

d6f3de3

Convert ConnectionStatus to data class

d2218c9

Also added helpers.

PostListMainVM: Move connection status to start

a2432d2

In this way, we can guarantee that `site` is not `null`.

Make PostListMainViewModel useable in unit tests

1ae0614

Add PostListMainViewModelTest

e8f0a79

Rename mocked* methods to create*

5e47049

The `mocked*` can be misleading because the returned instances are not mocks. It is the parameters that are mocked.

Update FluxC to b6f8aa3

b454336

Update release notes (12.5)

9e73e5f

shiki added [Type] Bug [Status] Needs Design Review A designer needs to sign off on the implemented design. labels May 1, 2019

shiki added this to the 12.5 milestone May 1, 2019

shiki requested review from malinajirka, maxme and megsfulton May 1, 2019 20:34

shiki self-assigned this May 1, 2019

shiki mentioned this pull request May 1, 2019

Add PostStore.getLocalDrafts wordpress-mobile/WordPress-FluxC-Android#1243

Merged

wpmobilebot reviewed May 1, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/uploads/LocalDraftUploadStarter.kt Outdated Show resolved Hide resolved

LocalDraftUploadStarter: Remove unused import

514ce10

shiki commented May 1, 2019

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

diegoreymendez mentioned this pull request May 1, 2019

Offline Support: Local drafts aren't synched when going back online. #9568

Closed

malinajirka self-assigned this May 2, 2019

malinajirka reviewed May 2, 2019

View reviewed changes

shiki added [Status] Needs Discussion The issue needs to be discussed by the team before it can be resolved. and removed [Status] Needs Design Review A designer needs to sign off on the implemented design. labels May 3, 2019

shiki mentioned this pull request May 3, 2019

Uploading a draft shows mildly horrifying "Uploading Post" message #9790

Closed

LocalDraftUploadStarter: Use retry when uploading

bcff503

This method is what is used for uploading in Post List → Retry and it fixes the issue with uploads failing if they have a media.

This was referenced May 6, 2019

Technical solution to trigger new actions when the device goes online #9803

Closed

Trigger an auto-upload or remote auto-save to preview drafts or published posts #9728

Closed

shiki added 2 commits May 8, 2019 15:28

Update FluxC to 5aad0a2

74b26b0

Make PostListEventListener injectable

e26e540

This is so we can mock it in unit tests. It does not currently work in unit tests because of the `EventBus` usage.

shiki mentioned this pull request May 8, 2019

Make ConnectionStatusLiveData distinct #9821

Merged

1 task

Merge branch 'issue/9568-connectionstatuslivedata-revamp' into issue/…

56fb755

…9568-upload-local-drafts # Conflicts: # WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt # build.gradle

shiki added the [Status] Not Ready for Review label May 10, 2019

shiki added 2 commits May 10, 2019 12:37

Fix PostListMainViewModelTest

f44e29e

Logic is still intact. The dependencies just changed.

LocalDraftUploader: Filter posts already in queue

fa94f00

maxme mentioned this pull request May 13, 2019

Transform LocalDraftUploadStarter into the UploadStarter #9846

Closed

6 tasks

Merge branch 'develop' into issue/9568-upload-local-drafts

5aadbd9

# Conflicts: # build.gradle

shiki mentioned this pull request May 13, 2019

Make LocalDraftUploadStarter upload local draft pages #9851

Closed

2 tasks

shiki removed the [Status] Needs Discussion The issue needs to be discussed by the team before it can be resolved. label May 13, 2019

Merge develop into issue/9568-upload-local-drafts

2e106cd

shiki removed the [Status] Not Ready for Review label May 14, 2019

malinajirka approved these changes May 14, 2019

View reviewed changes

malinajirka merged commit 147e3d1 into develop May 14, 2019

malinajirka deleted the issue/9568-upload-local-drafts branch May 14, 2019 13:50

yaelirub added the 5 label Jun 11, 2019

shiki mentioned this pull request Jul 8, 2019

Automatically upload local drafts and locally changed posts and pages #10177

Closed

4 tasks

Automatically upload local drafts #9774

Automatically upload local drafts #9774

Uh oh!

Conversation

shiki commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not Covered Yet

FluxC PR

Testing

Release Notes

Status

Uh oh!

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

malinajirka May 2, 2019

Choose a reason for hiding this comment

Uh oh!

shiki May 2, 2019

Choose a reason for hiding this comment

Uh oh!

shiki May 13, 2019

Choose a reason for hiding this comment

Uh oh!

malinajirka May 2, 2019

Choose a reason for hiding this comment

Uh oh!

shiki May 2, 2019

Choose a reason for hiding this comment

Uh oh!

shiki May 13, 2019

Choose a reason for hiding this comment

Uh oh!

shiki commented May 2, 2019

Uh oh!

mattmiklic commented May 2, 2019

Uh oh!

malinajirka commented May 3, 2019

Uh oh!

shiki commented May 3, 2019

Uh oh!

megsfulton commented May 6, 2019

Uh oh!

malinajirka commented May 14, 2019

Uh oh!

shiki commented May 14, 2019

Uh oh!

shiki commented May 14, 2019

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shiki commented May 1, 2019 •

edited

Loading