-
Notifications
You must be signed in to change notification settings - Fork 1.3k
LocalDraftUploadStarter: Upload from anywhere #9869
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.
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
Merged
shiki
merged 32 commits into
develop
from
issue/9852-localdraftstarter-upload-from-anywhere
May 21, 2019
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
479cd19
Make LocalDraftUploadStarter a Singleton
shiki aa70205
Make LocalDraftUploadStarter a CoroutineScope
shiki 6d70aed
LocalDraftUploadStarter: Add queueing methods
shiki 4119440
Upload drafts from anywhere
shiki 29e6f11
LocalDraftUploadStarter: Upload on foreground
shiki 894563a
Revert PostListViewModel to extend ViewModel only
shiki 71f9742
Fix PostListMainViewModelTest
shiki f8e6f92
Trigger upload drafts when post list is opened
shiki ffca6ef
LocalDraftUploadStarter: Query in parallel
shiki 77e5a86
Add UploadServiceFacade for testability
shiki 467e087
LocalDraftUploadStarter: Add job
shiki f5c491e
Add LocalDraftUploadStarterTest
shiki d3c1200
Add LiveData.skip()
shiki 1bbc2a1
Fix queueUploadForAllSites called twice on boot
shiki cafe486
Clarify LocalDraftUploadStarter usage
shiki 237a8ff
Rename LocalDraftUploadStarter
shiki 52390a8
Fix LocalDraftUploadStarterTest failure
shiki b13c1ad
LocalDraftUploadStarterTest: Test app foreground
shiki 4d6b93c
LocalDraftUploadStarterTest: Single-site uploading
shiki 2bf479a
LocalDraftUploaderTest: Queueing validation
shiki f488e36
Remove unused imports
shiki 1f2d489
Fix typo in LiveDataUtils.kt
shiki c8c90f3
Update 12.5 release notes for LocalDraftUploadStarter changes
shiki 9d25687
Add injected Dispatchers.IO
shiki edf746a
Rename LocalDraftUploadStarter.queueUpload
shiki 3c6705b
Simplify LiveData.skip()
shiki 00ac385
Add comments
shiki 3a11c0e
Add LocalDraftUploadStarterConcurrentTest
shiki 2c6c2f2
Make LocalDraftUploadStarterTest deterministic
shiki 3f384f1
Simplify LocalDraftUploadStarter.upload
shiki 443666a
Add LocalDraftUploadStarter error handling
shiki 3e675a7
LocalDraftUploadStarterConcurrentTest: Add comment
shiki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
117 changes: 107 additions & 10 deletions
117
WordPress/src/main/java/org/wordpress/android/ui/uploads/LocalDraftUploadStarter.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,138 @@ | ||
| package org.wordpress.android.ui.uploads | ||
|
|
||
| import android.arch.lifecycle.Lifecycle.Event | ||
| import android.arch.lifecycle.LifecycleObserver | ||
| import android.arch.lifecycle.LiveData | ||
| import android.arch.lifecycle.OnLifecycleEvent | ||
| import android.arch.lifecycle.ProcessLifecycleOwner | ||
| import android.content.Context | ||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.CoroutineScope | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.coroutineScope | ||
| import kotlinx.coroutines.launch | ||
| import org.wordpress.android.fluxc.model.SiteModel | ||
| import org.wordpress.android.fluxc.store.PostStore | ||
| import org.wordpress.android.fluxc.store.SiteStore | ||
| import org.wordpress.android.modules.BG_THREAD | ||
| import org.wordpress.android.modules.IO_THREAD | ||
| import org.wordpress.android.util.CrashLoggingUtils | ||
| import org.wordpress.android.util.NetworkUtilsWrapper | ||
| import org.wordpress.android.util.skip | ||
| import org.wordpress.android.viewmodel.helpers.ConnectionStatus | ||
| import javax.inject.Inject | ||
| import javax.inject.Named | ||
| import javax.inject.Singleton | ||
| import kotlin.coroutines.CoroutineContext | ||
|
|
||
| /** | ||
| * Provides a way to find and upload all local drafts. | ||
| * Automatically uploads local drafts. | ||
| * | ||
| * Auto-uploads happen when the app is placed in the foreground or when the internet connection is restored. In | ||
| * addition to this, call sites can also request an immediate execution by calling [upload]. | ||
| * | ||
| * The method [activateAutoUploading] must be called once, preferably during app creation, for the auto-uploads to work. | ||
| */ | ||
| @Singleton | ||
| class LocalDraftUploadStarter @Inject constructor( | ||
| /** | ||
| * The Application context | ||
| */ | ||
| private val context: Context, | ||
| private val postStore: PostStore, | ||
| private val siteStore: SiteStore, | ||
| @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, | ||
| @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, | ||
| private val uploadServiceFacade: UploadServiceFacade, | ||
| private val networkUtilsWrapper: NetworkUtilsWrapper, | ||
| private val connectionStatus: LiveData<ConnectionStatus> | ||
| ) : CoroutineScope { | ||
| private val job = Job() | ||
|
|
||
| override val coroutineContext: CoroutineContext get() = job + bgDispatcher | ||
|
|
||
| /** | ||
| * The Coroutine dispatcher used for querying in FluxC. | ||
| * The hook for making this class automatically launch uploads whenever the app is placed in the foreground. | ||
| */ | ||
| @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, | ||
| private val networkUtilsWrapper: NetworkUtilsWrapper | ||
| ) { | ||
| fun uploadLocalDrafts(scope: CoroutineScope, site: SiteModel) = scope.launch(bgDispatcher) { | ||
| private val processLifecycleObserver = object : LifecycleObserver { | ||
| @OnLifecycleEvent(Event.ON_START) | ||
| fun onAppComesFromBackground() { | ||
| queueUploadFromAllSites() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Activates the necessary observers for this class to start auto-uploading. | ||
| * | ||
| * This must be called during [org.wordpress.android.WordPress]' creation like so: | ||
| * | ||
| * ``` | ||
| * mLocalDraftUploadStarter.activateAutoUploading(ProcessLifecycleOwner.get()) | ||
| * ``` | ||
| */ | ||
| fun activateAutoUploading(processLifecycleOwner: ProcessLifecycleOwner) { | ||
| // Since this class is meant to be a Singleton, it should be fine (I think) to use observeForever in here. | ||
| // We're skipping the first emitted value because the processLifecycleObserver below will also trigger an | ||
| // immediate upload. | ||
| connectionStatus.skip(1).observeForever { | ||
| queueUploadFromAllSites() | ||
| } | ||
|
|
||
| processLifecycleOwner.lifecycle.addObserver(processLifecycleObserver) | ||
| } | ||
|
|
||
| private fun queueUploadFromAllSites() = launch { | ||
| val sites = siteStore.sites | ||
| try { | ||
| checkConnectionAndUpload(sites = sites) | ||
| } catch (e: Exception) { | ||
| CrashLoggingUtils.log(e) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Upload all local drafts from the given [site]. | ||
| */ | ||
| fun queueUploadFromSite(site: SiteModel) = launch { | ||
| try { | ||
| checkConnectionAndUpload(sites = listOf(site)) | ||
| } catch (e: Exception) { | ||
| CrashLoggingUtils.log(e) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * If there is an internet connection, uploads all local drafts belonging to [sites]. | ||
| * | ||
| * This coroutine will suspend until all the [upload] operations have completed. If one of them fails, all query | ||
| * and queuing attempts ([upload]) will be canceled. The exception will be thrown by this method. | ||
| */ | ||
| private suspend fun checkConnectionAndUpload(sites: List<SiteModel>) = coroutineScope { | ||
| if (!networkUtilsWrapper.isNetworkAvailable()) { | ||
| return@launch | ||
| return@coroutineScope | ||
| } | ||
|
|
||
| sites.forEach { | ||
| launch(ioDispatcher) { | ||
| upload(site = it) | ||
| } | ||
| } | ||
| } | ||
|
|
||
malinajirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /** | ||
| * This is meant to be used by [checkConnectionAndUpload] only. | ||
| */ | ||
| private fun upload(site: SiteModel) { | ||
| postStore.getLocalDraftPosts(site) | ||
| .filterNot { UploadService.isPostUploadingOrQueued(it) } | ||
| .filterNot { uploadServiceFacade.isPostUploadingOrQueued(it) } | ||
| .forEach { localDraft -> | ||
| val intent = UploadService.getUploadPostServiceIntent(context, localDraft, false, false, true) | ||
| context.startService(intent) | ||
| uploadServiceFacade.uploadPost( | ||
| context = context, | ||
| post = localDraft, | ||
| trackAnalytics = false, | ||
| publish = false, | ||
| isRetry = true | ||
| ) | ||
| } | ||
| } | ||
| } | ||
20 changes: 20 additions & 0 deletions
20
WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadServiceFacade.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package org.wordpress.android.ui.uploads | ||
|
|
||
| import android.content.Context | ||
| import org.wordpress.android.fluxc.model.PostModel | ||
| import javax.inject.Inject | ||
|
|
||
| /** | ||
| * An injectable class built on top of [UploadService]. | ||
| * | ||
| * The main purpose of this is to provide testability for classes that use [UploadService]. This should never | ||
| * contain any static methods. | ||
| */ | ||
| class UploadServiceFacade @Inject constructor() { | ||
| fun uploadPost(context: Context, post: PostModel, trackAnalytics: Boolean, publish: Boolean, isRetry: Boolean) { | ||
| val intent = UploadService.getUploadPostServiceIntent(context, post, trackAnalytics, publish, isRetry) | ||
| context.startService(intent) | ||
| } | ||
|
|
||
| fun isPostUploadingOrQueued(post: PostModel) = UploadService.isPostUploadingOrQueued(post) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about catching a more specific exception type. I believe we should follow the Fail Fast principle and avoid hiding unexpected exceptions. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially thought of failing fast for this class. But realized that it's a class that will be executed on multiple scenarios like app start, app foreground, etc. If the crash is consistent, users would not be able to use the app anymore.
But if you feel that crashes are unlikely, we can remove the exception handling. I think they're unlikely but I opted to be on the safe side.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the specific exception type handling, I only know of the documented
@throwsincontext.startService. What exceptions should we expect from FluxC queries? I only sawRuntimeExceptionfrom WellSql which is not as specific as I wanted it to be. 😬There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
I'm a bit confused - this class doesn't perform any FluxC queries, right?
I think we can expect
the documented @throws in context.startServiceandCancellationException. But I agree with your comment that we don't want to crash the app since this class will be started automatically on app-start, so we might actually want to keep the general "Exception" here.