From be3ad8b8fa896bfff67b2c7c1e5de271f7d66f68 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 6 Feb 2020 11:33:08 -0500 Subject: [PATCH 01/16] A rough introduction of AutoSavePostIfNotDraftUseCase --- .../AutosavePostOrUpdateDraftUseCase.kt | 85 +++++++++++++++++++ build.gradle | 2 +- 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt new file mode 100644 index 000000000000..d717398c93e4 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt @@ -0,0 +1,85 @@ +package org.wordpress.android.ui.uploads + +import kotlinx.coroutines.suspendCancellableCoroutine +import org.greenrobot.eventbus.Subscribe +import org.greenrobot.eventbus.ThreadMode.BACKGROUND +import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.generated.PostActionBuilder +import org.wordpress.android.fluxc.model.CauseOfOnPostChanged.RemoteAutoSavePost +import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId +import org.wordpress.android.fluxc.model.PostModel +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.store.PostStore +import org.wordpress.android.fluxc.store.PostStore.OnPostChanged +import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched +import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote +import javax.inject.Inject +import kotlin.coroutines.Continuation +import kotlin.coroutines.resume + +// TODO: Not be so verbose in naming + +private const val DRAFT_POST_STATUS = "draft" + +interface OnAutoSavePostIfNotDraftCallback { + fun handleAutoSavePostIfNotDraftResult(result: AutoSavePostIfNotDraftResult) +} + +sealed class AutoSavePostIfNotDraftResult { + data class PostIsDraftInRemote(val post: PostModel): AutoSavePostIfNotDraftResult() + data class PostAutoSaved(val post: PostModel): AutoSavePostIfNotDraftResult() +} + +// TODO: Add documentation +// TODO: Add unit tests +class AutoSavePostIfNotDraftUseCase @Inject constructor( + val dispatcher: Dispatcher, + val postStore: PostStore, + val callback: OnAutoSavePostIfNotDraftCallback +) { + private var postStatusPair: Pair>? = null + private var autoSavePair: Pair>? = null + + // TODO: Shouldn't be a suspend function + suspend fun autoSavePostOrUpdateDraft(site: SiteModel, post: PostModel) { + val remotePostPayload = RemotePostPayload(post, site) + val remotePostStatus: String = suspendCancellableCoroutine { cont -> + postStatusPair = Pair(LocalId(post.id), cont) + dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) + } + if (remotePostStatus != DRAFT_POST_STATUS) { + dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) + } else { + callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(post)) + } + } + + // TODO: Handle errors + // TODO: Do we need to observe the event in `MAIN` thread? (Probably not) + @Subscribe(threadMode = BACKGROUND) + @Suppress("unused") + fun onPostStatusFetched(event: OnPostStatusFetched) { + postStatusPair?.let { + if (event.post.id == it.first.value) { + it.second.resume(event.remotePostStatus) + postStatusPair = null + } + } + } + + // TODO: Handle errors + // TODO: Do we need to observe the event in `MAIN` thread? (Probably not) + @Subscribe(threadMode = BACKGROUND) + @Suppress("unused") + fun onPostChanged(event: OnPostChanged) { + autoSavePair?.let { + if (event.causeOfChange is RemoteAutoSavePost) { + val postLocalId = (event.causeOfChange as RemoteAutoSavePost).localPostId + val post: PostModel = postStore.getPostByLocalPostId(postLocalId) + callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(post)) + } + } + } +} diff --git a/build.gradle b/build.gradle index 40696e81a567..4172ba244006 100644 --- a/build.gradle +++ b/build.gradle @@ -96,7 +96,7 @@ buildScan { ext { daggerVersion = '2.22.1' - fluxCVersion = 'c5ea220f70c3a77681d376a17ce5c57e26c1c392' + fluxCVersion = 'bc810ea94e47a7f56184848fbb7e0f46a4de1d53' } // Onboarding and dev env setup tasks From 32da8c436600eaa69a02c69b606efaaebce80869 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 6 Feb 2020 12:37:35 -0500 Subject: [PATCH 02/16] Rough integration of AutoSavePostIfNotDraftUseCase --- ...se.kt => AutoSavePostIfNotDraftUseCase.kt} | 48 ++++++++++++------- .../android/ui/uploads/PostUploadHandler.java | 39 ++++++++------- 2 files changed, 52 insertions(+), 35 deletions(-) rename WordPress/src/main/java/org/wordpress/android/ui/uploads/{AutosavePostOrUpdateDraftUseCase.kt => AutoSavePostIfNotDraftUseCase.kt} (63%) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt similarity index 63% rename from WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt rename to WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index d717398c93e4..d911b98b87c1 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutosavePostOrUpdateDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -1,5 +1,7 @@ package org.wordpress.android.ui.uploads +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode.BACKGROUND @@ -8,7 +10,6 @@ import org.wordpress.android.fluxc.generated.PostActionBuilder import org.wordpress.android.fluxc.model.CauseOfOnPostChanged.RemoteAutoSavePost import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId import org.wordpress.android.fluxc.model.PostModel -import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.PostStore import org.wordpress.android.fluxc.store.PostStore.OnPostChanged import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched @@ -28,31 +29,41 @@ interface OnAutoSavePostIfNotDraftCallback { } sealed class AutoSavePostIfNotDraftResult { - data class PostIsDraftInRemote(val post: PostModel): AutoSavePostIfNotDraftResult() - data class PostAutoSaved(val post: PostModel): AutoSavePostIfNotDraftResult() + data class PostIsDraftInRemote(val post: PostModel) : AutoSavePostIfNotDraftResult() + data class PostAutoSaved(val post: PostModel) : AutoSavePostIfNotDraftResult() + // TODO: Add error result types } // TODO: Add documentation // TODO: Add unit tests class AutoSavePostIfNotDraftUseCase @Inject constructor( - val dispatcher: Dispatcher, - val postStore: PostStore, - val callback: OnAutoSavePostIfNotDraftCallback + private val dispatcher: Dispatcher, + private val postStore: PostStore ) { + // TODO: Make it reusable private var postStatusPair: Pair>? = null private var autoSavePair: Pair>? = null - // TODO: Shouldn't be a suspend function - suspend fun autoSavePostOrUpdateDraft(site: SiteModel, post: PostModel) { - val remotePostPayload = RemotePostPayload(post, site) - val remotePostStatus: String = suspendCancellableCoroutine { cont -> - postStatusPair = Pair(LocalId(post.id), cont) - dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) - } - if (remotePostStatus != DRAFT_POST_STATUS) { - dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) - } else { - callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(post)) + fun autoSavePostOrUpdateDraft( + remotePostPayload: RemotePostPayload, + callback: OnAutoSavePostIfNotDraftCallback + ) { + // TODO: What scope should we use for this? + GlobalScope.launch { + val remotePostStatus: String = suspendCancellableCoroutine { cont -> + postStatusPair = Pair(LocalId(remotePostPayload.post.id), cont) + dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) + } + + if (remotePostStatus == DRAFT_POST_STATUS) { + callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(remotePostPayload.post)) + } else { + val updatedPost: PostModel = suspendCancellableCoroutine { cont -> + autoSavePair = Pair(LocalId(remotePostPayload.post.id), cont) + dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) + } + callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(updatedPost)) + } } } @@ -78,7 +89,8 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( if (event.causeOfChange is RemoteAutoSavePost) { val postLocalId = (event.causeOfChange as RemoteAutoSavePost).localPostId val post: PostModel = postStore.getPostByLocalPostId(postLocalId) - callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(post)) + it.second.resume(post) + autoSavePair = null } } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java index 1217be8a107c..e95e147f2658 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java @@ -14,14 +14,13 @@ import org.greenrobot.eventbus.EventBus; import org.greenrobot.eventbus.Subscribe; import org.greenrobot.eventbus.ThreadMode; +import org.jetbrains.annotations.NotNull; import org.wordpress.android.R; import org.wordpress.android.WordPress; import org.wordpress.android.analytics.AnalyticsTracker.Stat; import org.wordpress.android.fluxc.Dispatcher; import org.wordpress.android.fluxc.generated.MediaActionBuilder; import org.wordpress.android.fluxc.generated.PostActionBuilder; -import org.wordpress.android.fluxc.model.CauseOfOnPostChanged; -import org.wordpress.android.fluxc.model.CauseOfOnPostChanged.RemoteAutoSavePost; import org.wordpress.android.fluxc.model.MediaModel; import org.wordpress.android.fluxc.model.MediaModel.MediaUploadState; import org.wordpress.android.fluxc.model.PostImmutableModel; @@ -31,12 +30,12 @@ import org.wordpress.android.fluxc.store.MediaStore; import org.wordpress.android.fluxc.store.MediaStore.UploadMediaPayload; import org.wordpress.android.fluxc.store.PostStore; -import org.wordpress.android.fluxc.store.PostStore.OnPostChanged; import org.wordpress.android.fluxc.store.PostStore.OnPostUploaded; import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload; import org.wordpress.android.fluxc.store.SiteStore; import org.wordpress.android.ui.posts.PostUtils; import org.wordpress.android.ui.prefs.AppPrefs; +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved; import org.wordpress.android.ui.uploads.PostEvents.PostUploadStarted; import org.wordpress.android.ui.utils.UiHelpers; import org.wordpress.android.util.AppLog; @@ -62,7 +61,7 @@ import javax.inject.Inject; -public class PostUploadHandler implements UploadHandler { +public class PostUploadHandler implements UploadHandler, OnAutoSavePostIfNotDraftCallback { private static ArrayList sQueuedPostsList = new ArrayList<>(); private static Set sFirstPublishPosts = new HashSet<>(); private static PostModel sCurrentUploadingPost = null; @@ -79,6 +78,7 @@ public class PostUploadHandler implements UploadHandler { @Inject MediaStore mMediaStore; @Inject UiHelpers mUiHelpers; @Inject UploadActionUseCase mUploadActionUseCase; + @Inject AutoSavePostIfNotDraftUseCase mAutoSavePostIfNotDraftUseCase; PostUploadHandler(PostUploadNotifier postUploadNotifier) { ((WordPress) WordPress.getContext().getApplicationContext()).component().inject(this); @@ -286,7 +286,7 @@ protected UploadPostTaskResult doInBackground(PostModel... posts) { break; case REMOTE_AUTO_SAVE: AppLog.d(T.POSTS, "PostUploadHandler - REMOTE_AUTO_SAVE. Post: " + mPost.getTitle()); - mDispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(payload)); + mAutoSavePostIfNotDraftUseCase.autoSavePostOrUpdateDraft(payload, PostUploadHandler.this); break; case DO_NOTHING: AppLog.d(T.POSTS, "PostUploadHandler - DO_NOTHING. Post: " + mPost.getTitle()); @@ -604,6 +604,23 @@ private String uploadImageFile(MediaFile mediaFile, SiteModel site) { } } + // TODO: document? + @Override + public void handleAutoSavePostIfNotDraftResult(@NotNull AutoSavePostIfNotDraftResult result) { + // TODO: handle other cases + if (result instanceof AutoSavePostIfNotDraftResult.PostAutoSaved) { + mPostUploadNotifier + .incrementUploadedPostCountFromForegroundNotification(((PostAutoSaved) result).getPost()); + finishUpload(); + } else if (result instanceof AutoSavePostIfNotDraftResult.PostIsDraftInRemote) { + /** + * 1. If the post has a status that's not draft in local, remember that + * 2. Set the post status to draft and push post + * 3. Update the local copy of post with the previous post status + */ + } + } + /** * Has priority 9 on OnPostUploaded events, which ensures that PostUploadHandler is the first to receive * and process OnPostUploaded events, before they trickle down to other subscribers. @@ -660,16 +677,4 @@ public void onPostUploaded(OnPostUploaded event) { finishUpload(); } - - @SuppressWarnings("unused") - @Subscribe(threadMode = ThreadMode.MAIN, priority = 9) - public void onPostChanged(OnPostChanged event) { - if (event.causeOfChange instanceof CauseOfOnPostChanged.RemoteAutoSavePost) { - int postLocalId = ((RemoteAutoSavePost) event.causeOfChange).getLocalPostId(); - PostModel post = mPostStore.getPostByLocalPostId(postLocalId); - SiteModel site = mSiteStore.getSiteByLocalId(post.getLocalSiteId()); - mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); - finishUpload(); - } - } } From ab808707a579b25c543e8d9bdf423a9cb8d38f2f Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 6 Feb 2020 15:19:35 -0500 Subject: [PATCH 03/16] Refactor AutoSavePostIfNotDraftUseCase so it's reusable --- .../uploads/AutoSavePostIfNotDraftUseCase.kt | 49 ++++++++++++------- build.gradle | 2 +- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index d911b98b87c1..fbdd660763c7 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -40,18 +40,28 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( private val dispatcher: Dispatcher, private val postStore: PostStore ) { - // TODO: Make it reusable - private var postStatusPair: Pair>? = null - private var autoSavePair: Pair>? = null + private val postStatusContinuations = HashMap>() + private val autoSaveContinuations = HashMap>() + + init { + dispatcher.register(this) + } fun autoSavePostOrUpdateDraft( remotePostPayload: RemotePostPayload, callback: OnAutoSavePostIfNotDraftCallback ) { + val localPostId = LocalId(remotePostPayload.post.id) + if (postStatusContinuations.containsKey(localPostId) || + autoSaveContinuations.containsKey(localPostId)) { + // TODO: Consider canceling the previous job instead + // We are already handling this post + return + } // TODO: What scope should we use for this? GlobalScope.launch { val remotePostStatus: String = suspendCancellableCoroutine { cont -> - postStatusPair = Pair(LocalId(remotePostPayload.post.id), cont) + postStatusContinuations[localPostId] = cont dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) } @@ -59,8 +69,12 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(remotePostPayload.post)) } else { val updatedPost: PostModel = suspendCancellableCoroutine { cont -> - autoSavePair = Pair(LocalId(remotePostPayload.post.id), cont) - dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) + autoSaveContinuations[localPostId] = cont + dispatcher.dispatch( + PostActionBuilder.newRemoteAutoSavePostAction( + remotePostPayload + ) + ) } callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(updatedPost)) } @@ -68,29 +82,26 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( } // TODO: Handle errors - // TODO: Do we need to observe the event in `MAIN` thread? (Probably not) @Subscribe(threadMode = BACKGROUND) @Suppress("unused") fun onPostStatusFetched(event: OnPostStatusFetched) { - postStatusPair?.let { - if (event.post.id == it.first.value) { - it.second.resume(event.remotePostStatus) - postStatusPair = null - } + val localPostId = LocalId(event.post.id) + postStatusContinuations[localPostId]?.let { continuation -> + continuation.resume(event.remotePostStatus) + postStatusContinuations.remove(localPostId) } } // TODO: Handle errors - // TODO: Do we need to observe the event in `MAIN` thread? (Probably not) @Subscribe(threadMode = BACKGROUND) @Suppress("unused") fun onPostChanged(event: OnPostChanged) { - autoSavePair?.let { - if (event.causeOfChange is RemoteAutoSavePost) { - val postLocalId = (event.causeOfChange as RemoteAutoSavePost).localPostId - val post: PostModel = postStore.getPostByLocalPostId(postLocalId) - it.second.resume(post) - autoSavePair = null + if (event.causeOfChange is RemoteAutoSavePost) { + val localPostId = LocalId((event.causeOfChange as RemoteAutoSavePost).localPostId) + autoSaveContinuations[localPostId]?.let { continuation -> + val post: PostModel = postStore.getPostByLocalPostId(localPostId.value) + continuation.resume(post) + autoSaveContinuations.remove(localPostId) } } } diff --git a/build.gradle b/build.gradle index 4172ba244006..2593bd8965d9 100644 --- a/build.gradle +++ b/build.gradle @@ -96,7 +96,7 @@ buildScan { ext { daggerVersion = '2.22.1' - fluxCVersion = 'bc810ea94e47a7f56184848fbb7e0f46a4de1d53' + fluxCVersion = '1d2a933ea35ba0ca0286c805a12785b59d219d7f' } // Onboarding and dev env setup tasks From 4cf02b79d19bce3dd044c169d171cfad004c529a Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 6 Feb 2020 17:51:54 -0500 Subject: [PATCH 04/16] Update FluxC hash which contains some fixes to fetching post status --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 2593bd8965d9..2d3032213fa6 100644 --- a/build.gradle +++ b/build.gradle @@ -96,7 +96,7 @@ buildScan { ext { daggerVersion = '2.22.1' - fluxCVersion = '1d2a933ea35ba0ca0286c805a12785b59d219d7f' + fluxCVersion = '751696ad02d3446bbfe902c21b16747745616990' } // Onboarding and dev env setup tasks From 738e71ef8887efa651923ac8bdf44276a09972ab Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Mon, 10 Feb 2020 12:25:42 -0500 Subject: [PATCH 05/16] Throw IllegalArgumentException if autosave use case is already handling a post --- .../android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index fbdd660763c7..5f14f968bb1c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -16,6 +16,7 @@ import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote +import java.lang.IllegalArgumentException import javax.inject.Inject import kotlin.coroutines.Continuation import kotlin.coroutines.resume @@ -54,9 +55,8 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( val localPostId = LocalId(remotePostPayload.post.id) if (postStatusContinuations.containsKey(localPostId) || autoSaveContinuations.containsKey(localPostId)) { - // TODO: Consider canceling the previous job instead - // We are already handling this post - return + throw IllegalArgumentException("This post is already being processed. Make sure not to start an autoSave " + + "or update draft action while another one is going on.") } // TODO: What scope should we use for this? GlobalScope.launch { From 6767c91dc1638619cdf49cd701ecd06f5f589fdd Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Mon, 10 Feb 2020 14:16:23 -0500 Subject: [PATCH 06/16] Refactor AutoSavePostIfNotDraftUseCase --- .../uploads/AutoSavePostIfNotDraftUseCase.kt | 55 ++++++++++++------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index 5f14f968bb1c..1a5a9b81751d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -1,5 +1,6 @@ package org.wordpress.android.ui.uploads +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine @@ -13,16 +14,17 @@ import org.wordpress.android.fluxc.model.PostModel import org.wordpress.android.fluxc.store.PostStore import org.wordpress.android.fluxc.store.PostStore.OnPostChanged import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched +import org.wordpress.android.fluxc.store.PostStore.PostError import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload +import org.wordpress.android.modules.BG_THREAD import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote import java.lang.IllegalArgumentException import javax.inject.Inject +import javax.inject.Named import kotlin.coroutines.Continuation import kotlin.coroutines.resume -// TODO: Not be so verbose in naming - private const val DRAFT_POST_STATUS = "draft" interface OnAutoSavePostIfNotDraftCallback { @@ -30,16 +32,22 @@ interface OnAutoSavePostIfNotDraftCallback { } sealed class AutoSavePostIfNotDraftResult { + // Initial fetch post status request failed + data class PostStatusCheckFailed(val post:PostModel, val error: PostError) : AutoSavePostIfNotDraftResult() + // Post status is `DRAFT` in remote which means we'll want to update the draft directly data class PostIsDraftInRemote(val post: PostModel) : AutoSavePostIfNotDraftResult() + // Post status is not `DRAFT` in remote and the post was auto-saved successfully data class PostAutoSaved(val post: PostModel) : AutoSavePostIfNotDraftResult() - // TODO: Add error result types + // Post status is not `DRAFT` in remote but the post auto-save failed + data class PostAutoSaveFailed(val post:PostModel, val error: PostError) : AutoSavePostIfNotDraftResult() } -// TODO: Add documentation +// TODO: Add documentation (add shortcode for the p2 discussion) // TODO: Add unit tests class AutoSavePostIfNotDraftUseCase @Inject constructor( private val dispatcher: Dispatcher, - private val postStore: PostStore + private val postStore: PostStore, + @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) { private val postStatusContinuations = HashMap>() private val autoSaveContinuations = HashMap>() @@ -53,34 +61,41 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( callback: OnAutoSavePostIfNotDraftCallback ) { val localPostId = LocalId(remotePostPayload.post.id) + if (remotePostPayload.post.isLocalDraft) { + throw IllegalArgumentException("Local drafts should not be auto-saved") + } if (postStatusContinuations.containsKey(localPostId) || autoSaveContinuations.containsKey(localPostId)) { throw IllegalArgumentException("This post is already being processed. Make sure not to start an autoSave " + "or update draft action while another one is going on.") } - // TODO: What scope should we use for this? - GlobalScope.launch { - val remotePostStatus: String = suspendCancellableCoroutine { cont -> - postStatusContinuations[localPostId] = cont - dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) - } - + GlobalScope.launch(bgDispatcher) { + val remotePostStatus = fetchRemotePostStatus(remotePostPayload) if (remotePostStatus == DRAFT_POST_STATUS) { callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(remotePostPayload.post)) } else { - val updatedPost: PostModel = suspendCancellableCoroutine { cont -> - autoSaveContinuations[localPostId] = cont - dispatcher.dispatch( - PostActionBuilder.newRemoteAutoSavePostAction( - remotePostPayload - ) - ) - } + val updatedPost = autoSavePost(remotePostPayload) callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(updatedPost)) } } } + private suspend fun fetchRemotePostStatus(remotePostPayload: RemotePostPayload): String { + val localPostId = LocalId(remotePostPayload.post.id) + return suspendCancellableCoroutine { cont -> + postStatusContinuations[localPostId] = cont + dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) + } + } + + private suspend fun autoSavePost(remotePostPayload: RemotePostPayload): PostModel { + val localPostId = LocalId(remotePostPayload.post.id) + return suspendCancellableCoroutine { cont -> + autoSaveContinuations[localPostId] = cont + dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) + } + } + // TODO: Handle errors @Subscribe(threadMode = BACKGROUND) @Suppress("unused") From 0b2a89e48a8fcfd93a512156d99c1a5b0135b788 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Mon, 10 Feb 2020 15:10:12 -0500 Subject: [PATCH 07/16] Handle errors in AutoSavePostIfNotDraftUseCase --- .../uploads/AutoSavePostIfNotDraftUseCase.kt | 61 +++++++++++++------ 1 file changed, 41 insertions(+), 20 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index 1a5a9b81751d..e9580f4b364a 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -17,6 +17,8 @@ import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched import org.wordpress.android.fluxc.store.PostStore.PostError import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload import org.wordpress.android.modules.BG_THREAD +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.FetchPostStatusFailed +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaveFailed import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote import java.lang.IllegalArgumentException @@ -33,13 +35,18 @@ interface OnAutoSavePostIfNotDraftCallback { sealed class AutoSavePostIfNotDraftResult { // Initial fetch post status request failed - data class PostStatusCheckFailed(val post:PostModel, val error: PostError) : AutoSavePostIfNotDraftResult() + data class FetchPostStatusFailed(val post: PostModel, val error: PostError) : + AutoSavePostIfNotDraftResult() + // Post status is `DRAFT` in remote which means we'll want to update the draft directly data class PostIsDraftInRemote(val post: PostModel) : AutoSavePostIfNotDraftResult() + // Post status is not `DRAFT` in remote and the post was auto-saved successfully data class PostAutoSaved(val post: PostModel) : AutoSavePostIfNotDraftResult() + // Post status is not `DRAFT` in remote but the post auto-save failed - data class PostAutoSaveFailed(val post:PostModel, val error: PostError) : AutoSavePostIfNotDraftResult() + data class PostAutoSaveFailed(val post: PostModel, val error: PostError) : + AutoSavePostIfNotDraftResult() } // TODO: Add documentation (add shortcode for the p2 discussion) @@ -49,8 +56,8 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( private val postStore: PostStore, @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) { - private val postStatusContinuations = HashMap>() - private val autoSaveContinuations = HashMap>() + private val postStatusContinuations = HashMap>() + private val autoSaveContinuations = HashMap>() init { dispatcher.register(this) @@ -66,21 +73,32 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( } if (postStatusContinuations.containsKey(localPostId) || autoSaveContinuations.containsKey(localPostId)) { - throw IllegalArgumentException("This post is already being processed. Make sure not to start an autoSave " + - "or update draft action while another one is going on.") + throw IllegalArgumentException( + "This post is already being processed. Make sure not to start an autoSave " + + "or update draft action while another one is going on." + ) } GlobalScope.launch(bgDispatcher) { - val remotePostStatus = fetchRemotePostStatus(remotePostPayload) - if (remotePostStatus == DRAFT_POST_STATUS) { - callback.handleAutoSavePostIfNotDraftResult(PostIsDraftInRemote(remotePostPayload.post)) - } else { - val updatedPost = autoSavePost(remotePostPayload) - callback.handleAutoSavePostIfNotDraftResult(PostAutoSaved(updatedPost)) + val onPostStatusFetched = fetchRemotePostStatus(remotePostPayload) + val result = when { + onPostStatusFetched.isError -> { + FetchPostStatusFailed( + post = remotePostPayload.post, + error = onPostStatusFetched.error + ) + } + onPostStatusFetched.remotePostStatus == DRAFT_POST_STATUS -> { + PostIsDraftInRemote(remotePostPayload.post) + } + else -> { + autoSavePost(remotePostPayload) + } } + callback.handleAutoSavePostIfNotDraftResult(result) } } - private suspend fun fetchRemotePostStatus(remotePostPayload: RemotePostPayload): String { + private suspend fun fetchRemotePostStatus(remotePostPayload: RemotePostPayload): OnPostStatusFetched { val localPostId = LocalId(remotePostPayload.post.id) return suspendCancellableCoroutine { cont -> postStatusContinuations[localPostId] = cont @@ -88,34 +106,37 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( } } - private suspend fun autoSavePost(remotePostPayload: RemotePostPayload): PostModel { + private suspend fun autoSavePost(remotePostPayload: RemotePostPayload): AutoSavePostIfNotDraftResult { val localPostId = LocalId(remotePostPayload.post.id) - return suspendCancellableCoroutine { cont -> + val onPostChanged: OnPostChanged = suspendCancellableCoroutine { cont -> autoSaveContinuations[localPostId] = cont dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) } + return if (onPostChanged.isError) { + PostAutoSaveFailed(remotePostPayload.post, onPostChanged.error) + } else { + val updatedPost = postStore.getPostByLocalPostId(localPostId.value) + PostAutoSaved(updatedPost) + } } - // TODO: Handle errors @Subscribe(threadMode = BACKGROUND) @Suppress("unused") fun onPostStatusFetched(event: OnPostStatusFetched) { val localPostId = LocalId(event.post.id) postStatusContinuations[localPostId]?.let { continuation -> - continuation.resume(event.remotePostStatus) + continuation.resume(event) postStatusContinuations.remove(localPostId) } } - // TODO: Handle errors @Subscribe(threadMode = BACKGROUND) @Suppress("unused") fun onPostChanged(event: OnPostChanged) { if (event.causeOfChange is RemoteAutoSavePost) { val localPostId = LocalId((event.causeOfChange as RemoteAutoSavePost).localPostId) autoSaveContinuations[localPostId]?.let { continuation -> - val post: PostModel = postStore.getPostByLocalPostId(localPostId.value) - continuation.resume(post) + continuation.resume(event) autoSaveContinuations.remove(localPostId) } } From d01c3f8c09e6d00735650c86ca813d3d3a0d6904 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Tue, 11 Feb 2020 16:09:58 -0500 Subject: [PATCH 08/16] Handle all AutoSavePostIfNotDraftResult types --- .../uploads/AutoSavePostIfNotDraftUseCase.kt | 16 ++++---- .../android/ui/uploads/PostUploadHandler.java | 39 +++++++++++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index e9580f4b364a..e8cba70e0d7d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -33,20 +33,20 @@ interface OnAutoSavePostIfNotDraftCallback { fun handleAutoSavePostIfNotDraftResult(result: AutoSavePostIfNotDraftResult) } -sealed class AutoSavePostIfNotDraftResult { +sealed class AutoSavePostIfNotDraftResult(open val post: PostModel) { // Initial fetch post status request failed - data class FetchPostStatusFailed(val post: PostModel, val error: PostError) : - AutoSavePostIfNotDraftResult() + data class FetchPostStatusFailed(override val post: PostModel, val error: PostError) : + AutoSavePostIfNotDraftResult(post) // Post status is `DRAFT` in remote which means we'll want to update the draft directly - data class PostIsDraftInRemote(val post: PostModel) : AutoSavePostIfNotDraftResult() + data class PostIsDraftInRemote(override val post: PostModel) : AutoSavePostIfNotDraftResult(post) // Post status is not `DRAFT` in remote and the post was auto-saved successfully - data class PostAutoSaved(val post: PostModel) : AutoSavePostIfNotDraftResult() + data class PostAutoSaved(override val post: PostModel) : AutoSavePostIfNotDraftResult(post) // Post status is not `DRAFT` in remote but the post auto-save failed - data class PostAutoSaveFailed(val post: PostModel, val error: PostError) : - AutoSavePostIfNotDraftResult() + data class PostAutoSaveFailed(override val post: PostModel, val error: PostError) : + AutoSavePostIfNotDraftResult(post) } // TODO: Add documentation (add shortcode for the p2 discussion) @@ -108,7 +108,7 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( private suspend fun autoSavePost(remotePostPayload: RemotePostPayload): AutoSavePostIfNotDraftResult { val localPostId = LocalId(remotePostPayload.post.id) - val onPostChanged: OnPostChanged = suspendCancellableCoroutine { cont -> + val onPostChanged: OnPostChanged = suspendCancellableCoroutine { cont -> autoSaveContinuations[localPostId] = cont dispatcher.dispatch(PostActionBuilder.newRemoteAutoSavePostAction(remotePostPayload)) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java index e95e147f2658..933fe226c0e7 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java @@ -35,7 +35,10 @@ import org.wordpress.android.fluxc.store.SiteStore; import org.wordpress.android.ui.posts.PostUtils; import org.wordpress.android.ui.prefs.AppPrefs; +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.FetchPostStatusFailed; +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaveFailed; import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved; +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote; import org.wordpress.android.ui.uploads.PostEvents.PostUploadStarted; import org.wordpress.android.ui.utils.UiHelpers; import org.wordpress.android.util.AppLog; @@ -189,7 +192,7 @@ private void finishUpload() { } private enum UploadPostTaskResult { - PUSH_POST_DISPATCHED, ERROR, NOTHING_TO_UPLOAD + PUSH_POST_DISPATCHED, ERROR, NOTHING_TO_UPLOAD, AUTO_SAVE_OR_UPDATE_DRAFT } private class UploadPostTask extends AsyncTask { @@ -287,7 +290,7 @@ protected UploadPostTaskResult doInBackground(PostModel... posts) { case REMOTE_AUTO_SAVE: AppLog.d(T.POSTS, "PostUploadHandler - REMOTE_AUTO_SAVE. Post: " + mPost.getTitle()); mAutoSavePostIfNotDraftUseCase.autoSavePostOrUpdateDraft(payload, PostUploadHandler.this); - break; + return UploadPostTaskResult.AUTO_SAVE_OR_UPDATE_DRAFT; case DO_NOTHING: AppLog.d(T.POSTS, "PostUploadHandler - DO_NOTHING. Post: " + mPost.getTitle()); // A single post might be enqueued twice for upload. It might cause some side-effects when the @@ -607,17 +610,31 @@ private String uploadImageFile(MediaFile mediaFile, SiteModel site) { // TODO: document? @Override public void handleAutoSavePostIfNotDraftResult(@NotNull AutoSavePostIfNotDraftResult result) { - // TODO: handle other cases - if (result instanceof AutoSavePostIfNotDraftResult.PostAutoSaved) { - mPostUploadNotifier - .incrementUploadedPostCountFromForegroundNotification(((PostAutoSaved) result).getPost()); + PostModel post = result.getPost(); + SiteModel site = mSiteStore.getSiteByLocalId(post.getLocalSiteId()); + if (result instanceof FetchPostStatusFailed) { + mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); + mPostUploadNotifier.updateNotificationErrorForPost(post, site, post.error.message, 0); + finishUpload(); + } else if (result instanceof PostAutoSaveFailed) { + mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); + mPostUploadNotifier.updateNotificationErrorForPost(post, site, post.error.message, 0); finishUpload(); - } else if (result instanceof AutoSavePostIfNotDraftResult.PostIsDraftInRemote) { - /** - * 1. If the post has a status that's not draft in local, remember that - * 2. Set the post status to draft and push post - * 3. Update the local copy of post with the previous post status + } else if (result instanceof PostAutoSaved) { + mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); + finishUpload(); + } else if (result instanceof PostIsDraftInRemote) { + /* + * If the post is a draft in remote, we'll update it directly instead of auto-saving it. Please see + * documentation of `AutoSavePostIfNotDraftUseCase` for details. + * + * We opted not to restore the current status after the post is uploaded to avoid its complexity and to + * replicate `UPLOAD_AS_DRAFT`. We may change this in the future. */ + post.setStatus(PostStatus.DRAFT.toString()); + mDispatcher.dispatch(PostActionBuilder.newPushPostAction(new RemotePostPayload(post, site))); + } else { + throw new IllegalStateException("All AutoSavePostIfNotDraftResult types must be handled"); } } From bb985b123f331f84691c7101cc118ae5bc968bdf Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 12:22:51 -0500 Subject: [PATCH 09/16] Observe AutoSavePostIfNotDraftUseCase.onPostChanged on MAIN thread with priority 9 --- .../android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index e8cba70e0d7d..3ba72655a796 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -6,6 +6,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode.BACKGROUND +import org.greenrobot.eventbus.ThreadMode.MAIN import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.generated.PostActionBuilder import org.wordpress.android.fluxc.model.CauseOfOnPostChanged.RemoteAutoSavePost @@ -21,7 +22,6 @@ import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.FetchPostSt import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaveFailed import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostAutoSaved import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult.PostIsDraftInRemote -import java.lang.IllegalArgumentException import javax.inject.Inject import javax.inject.Named import kotlin.coroutines.Continuation @@ -130,7 +130,7 @@ class AutoSavePostIfNotDraftUseCase @Inject constructor( } } - @Subscribe(threadMode = BACKGROUND) + @Subscribe(threadMode = MAIN, priority = 9) @Suppress("unused") fun onPostChanged(event: OnPostChanged) { if (event.causeOfChange is RemoteAutoSavePost) { From 63fd0fef5443e7b563ae2ea1e3cdc48872635898 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 13:12:16 -0500 Subject: [PATCH 10/16] Don't show error notifications for post autosave fails --- .../android/ui/uploads/PostUploadHandler.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java index 933fe226c0e7..e0a659b9be24 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java @@ -611,16 +611,9 @@ private String uploadImageFile(MediaFile mediaFile, SiteModel site) { @Override public void handleAutoSavePostIfNotDraftResult(@NotNull AutoSavePostIfNotDraftResult result) { PostModel post = result.getPost(); - SiteModel site = mSiteStore.getSiteByLocalId(post.getLocalSiteId()); - if (result instanceof FetchPostStatusFailed) { - mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); - mPostUploadNotifier.updateNotificationErrorForPost(post, site, post.error.message, 0); - finishUpload(); - } else if (result instanceof PostAutoSaveFailed) { - mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); - mPostUploadNotifier.updateNotificationErrorForPost(post, site, post.error.message, 0); - finishUpload(); - } else if (result instanceof PostAutoSaved) { + if (result instanceof FetchPostStatusFailed + || result instanceof PostAutoSaveFailed + || result instanceof PostAutoSaved) { mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); finishUpload(); } else if (result instanceof PostIsDraftInRemote) { @@ -632,6 +625,7 @@ public void handleAutoSavePostIfNotDraftResult(@NotNull AutoSavePostIfNotDraftRe * replicate `UPLOAD_AS_DRAFT`. We may change this in the future. */ post.setStatus(PostStatus.DRAFT.toString()); + SiteModel site = mSiteStore.getSiteByLocalId(post.getLocalSiteId()); mDispatcher.dispatch(PostActionBuilder.newPushPostAction(new RemotePostPayload(post, site))); } else { throw new IllegalStateException("All AutoSavePostIfNotDraftResult types must be handled"); From 1268a78cab7f67c5dfa8f11ecc3d79b2fab0ba30 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 13:25:22 -0500 Subject: [PATCH 11/16] Add documentation for auto-save use case --- .../ui/uploads/AutoSavePostIfNotDraftUseCase.kt | 15 +++++++++++++-- .../android/ui/uploads/PostUploadHandler.java | 5 ++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt index 3ba72655a796..0be56604f391 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt @@ -49,8 +49,19 @@ sealed class AutoSavePostIfNotDraftResult(open val post: PostModel) { AutoSavePostIfNotDraftResult(post) } -// TODO: Add documentation (add shortcode for the p2 discussion) -// TODO: Add unit tests +/** + * This is a use case that auto-saves a post if it's not a DRAFT in remote and returns various results depending + * on the remote post status and whether network requests were successful. + * + * The reason auto-save is tied to post status is that the `/autosave` REST endpoint will override the changes to + * a `DRAFT` directly rather than auto-saving it. While doing so, it'll also disable comments for the post due to + * a bug. Both the fact that the endpoint does something we are not expecting and the bug that results from it is + * avoided by only calling the `/autosave` endpoint for posts that are not in DRAFT status. We update DRAFTs directly + * just as the endpoint would have, but that makes the logic more clear on the client side while avoiding the + * comments getting disabled bug. + * + * See p3hLNG-15Z-p2 for more info. + */ class AutoSavePostIfNotDraftUseCase @Inject constructor( private val dispatcher: Dispatcher, private val postStore: PostStore, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java index e0a659b9be24..59339e081561 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java @@ -607,13 +607,16 @@ private String uploadImageFile(MediaFile mediaFile, SiteModel site) { } } - // TODO: document? @Override public void handleAutoSavePostIfNotDraftResult(@NotNull AutoSavePostIfNotDraftResult result) { PostModel post = result.getPost(); if (result instanceof FetchPostStatusFailed || result instanceof PostAutoSaveFailed || result instanceof PostAutoSaved) { + /* + * If we fail to check the status of the post or auto-save fails, we deliberately don't show an error + * notification since it's not a user initiated action. We'll retry the action later on. + */ mPostUploadNotifier.incrementUploadedPostCountFromForegroundNotification(post); finishUpload(); } else if (result instanceof PostIsDraftInRemote) { From 174b742b0d40226a6f848d837542ec035375b9f8 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 15:34:28 -0500 Subject: [PATCH 12/16] Sets up AutoSavePostIfNotDraftUseCaseTest and adds fetch post status error test --- .../AutoSavePostIfNotDraftUseCaseTest.kt | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt new file mode 100644 index 000000000000..ec9eaaad7988 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt @@ -0,0 +1,101 @@ +package org.wordpress.android.ui.posts + +import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.nhaarman.mockitokotlin2.any +import com.nhaarman.mockitokotlin2.whenever +import kotlinx.coroutines.InternalCoroutinesApi +import org.junit.Rule +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.junit.MockitoJUnitRunner +import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.model.PostModel +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.store.PostStore +import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched +import org.wordpress.android.fluxc.store.PostStore.PostError +import org.wordpress.android.fluxc.store.PostStore.PostErrorType.UNKNOWN_POST +import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult +import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftUseCase +import org.wordpress.android.ui.uploads.OnAutoSavePostIfNotDraftCallback +import java.lang.reflect.Constructor +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.wordpress.android.TEST_DISPATCHER +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit + +private const val FETCH_POST_STATUS_ERROR_MESSAGE = "FETCH_POST_STATUS_ERROR_MESSAGE" +private const val DRAFT_STATUS = "DRAFT" + +@InternalCoroutinesApi +@RunWith(MockitoJUnitRunner::class) +class AutoSavePostIfNotDraftUseCaseTest { + @Rule + @JvmField val rule = InstantTaskExecutorRule() + + @Mock lateinit var dispatcher: Dispatcher + @Mock lateinit var postStore: PostStore + + @Test + fun `error while fetching post status will result in FetchPostStatusFailed`() { + val useCase = createUseCase() + val error = PostError(UNKNOWN_POST, FETCH_POST_STATUS_ERROR_MESSAGE) + val remotePostPayload = createRemotePostPayload() + whenever(dispatcher.dispatch(any())).then { + useCase.onPostStatusFetched( + createOnPostStatusFetchedEvent( + post = remotePostPayload.post, + status = DRAFT_STATUS, + error = error + ) + ) + } + useCase.autoSavePostOrUpdateDraftAndAssertResult(remotePostPayload) { result -> + assertThat(result).isInstanceOf(AutoSavePostIfNotDraftResult.FetchPostStatusFailed::class.java) + } + } + + private fun createUseCase() = AutoSavePostIfNotDraftUseCase( + dispatcher = dispatcher, + postStore = postStore, + bgDispatcher = TEST_DISPATCHER + ) + + private fun createRemotePostPayload() = RemotePostPayload(PostModel(), SiteModel()) + + /** + * Since the `OnPostStatusFetched` is package-private we utilize reflection for the test. Without being able to + * create `OnPostStatusFetched` event, we can't write most of the unit tests for `AutoSavePostIfNotDraftUseCase`. + */ + private fun createOnPostStatusFetchedEvent( + post: PostModel, + status: String, + error: PostError? = null + ): OnPostStatusFetched { + val constructor: Constructor = OnPostStatusFetched::class.java.getDeclaredConstructor( + PostModel::class.java, + String::class.java, + PostError::class.java + ) + constructor.isAccessible = true + return constructor.newInstance(post, status, error) + } +} + +private fun AutoSavePostIfNotDraftUseCase.autoSavePostOrUpdateDraftAndAssertResult( + remotePostPayload: RemotePostPayload, + assertionBlock: (AutoSavePostIfNotDraftResult) -> Unit +) { + val countDownLatch = CountDownLatch(1) + autoSavePostOrUpdateDraft( + remotePostPayload, + object : OnAutoSavePostIfNotDraftCallback { + override fun handleAutoSavePostIfNotDraftResult(result: AutoSavePostIfNotDraftResult) { + assertionBlock(result) + countDownLatch.countDown() + } + }) + assertThat(countDownLatch.await(1, TimeUnit.SECONDS)).isTrue() +} From 19fad9d7893be52ccb7014345a6843671e443bd7 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 16:28:49 -0500 Subject: [PATCH 13/16] Adds unit test for PostAutoSaveFailed --- .../AutoSavePostIfNotDraftUseCaseTest.kt | 81 ++++++++++++++----- 1 file changed, 60 insertions(+), 21 deletions(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt index ec9eaaad7988..440d19640a93 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt @@ -1,17 +1,25 @@ package org.wordpress.android.ui.posts import androidx.arch.core.executor.testing.InstantTaskExecutorRule -import com.nhaarman.mockitokotlin2.any +import com.nhaarman.mockitokotlin2.argWhere +import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.whenever import kotlinx.coroutines.InternalCoroutinesApi +import org.assertj.core.api.Assertions.assertThat import org.junit.Rule +import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock import org.mockito.junit.MockitoJUnitRunner +import org.wordpress.android.TEST_DISPATCHER import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.action.PostAction +import org.wordpress.android.fluxc.annotations.action.Action +import org.wordpress.android.fluxc.model.CauseOfOnPostChanged import org.wordpress.android.fluxc.model.PostModel import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.PostStore +import org.wordpress.android.fluxc.store.PostStore.OnPostChanged import org.wordpress.android.fluxc.store.PostStore.OnPostStatusFetched import org.wordpress.android.fluxc.store.PostStore.PostError import org.wordpress.android.fluxc.store.PostStore.PostErrorType.UNKNOWN_POST @@ -20,14 +28,13 @@ import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftUseCase import org.wordpress.android.ui.uploads.OnAutoSavePostIfNotDraftCallback import java.lang.reflect.Constructor -import org.assertj.core.api.Assertions.assertThat -import org.junit.Test -import org.wordpress.android.TEST_DISPATCHER import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit private const val FETCH_POST_STATUS_ERROR_MESSAGE = "FETCH_POST_STATUS_ERROR_MESSAGE" +private const val AUTO_SAVE_POST_ERROR_MESSAGE = "AUTO_SAVE_POST_ERROR_MESSAGE" private const val DRAFT_STATUS = "DRAFT" +private const val PUBLISH_STATUS = "PUBLISH" @InternalCoroutinesApi @RunWith(MockitoJUnitRunner::class) @@ -35,33 +42,59 @@ class AutoSavePostIfNotDraftUseCaseTest { @Rule @JvmField val rule = InstantTaskExecutorRule() - @Mock lateinit var dispatcher: Dispatcher @Mock lateinit var postStore: PostStore @Test fun `error while fetching post status will result in FetchPostStatusFailed`() { - val useCase = createUseCase() - val error = PostError(UNKNOWN_POST, FETCH_POST_STATUS_ERROR_MESSAGE) val remotePostPayload = createRemotePostPayload() - whenever(dispatcher.dispatch(any())).then { - useCase.onPostStatusFetched( - createOnPostStatusFetchedEvent( - post = remotePostPayload.post, - status = DRAFT_STATUS, - error = error - ) - ) - } + val onPostStatusFetched = createOnPostStatusFetchedEvent( + post = remotePostPayload.post, + status = DRAFT_STATUS, + error = PostError(UNKNOWN_POST, FETCH_POST_STATUS_ERROR_MESSAGE) + ) + val useCase = createUseCase(onPostStatusFetched) useCase.autoSavePostOrUpdateDraftAndAssertResult(remotePostPayload) { result -> assertThat(result).isInstanceOf(AutoSavePostIfNotDraftResult.FetchPostStatusFailed::class.java) } } - private fun createUseCase() = AutoSavePostIfNotDraftUseCase( - dispatcher = dispatcher, - postStore = postStore, - bgDispatcher = TEST_DISPATCHER - ) + @Test + fun `error while auto-saving will result in PostAutoSaveFailed`() { + val remotePostPayload = createRemotePostPayload() + val onPostStatusFetched = createOnPostStatusFetchedEvent( + post = remotePostPayload.post, + status = PUBLISH_STATUS + ) + val onPostChanged = createOnPostChangedEvent( + remotePostPayload.post, + error = PostError(UNKNOWN_POST, AUTO_SAVE_POST_ERROR_MESSAGE) + ) + val useCase = createUseCase(onPostStatusFetched, onPostChanged) + useCase.autoSavePostOrUpdateDraftAndAssertResult(remotePostPayload) { result -> + assertThat(result).isInstanceOf(AutoSavePostIfNotDraftResult.PostAutoSaveFailed::class.java) + } + } + + private fun createUseCase( + onPostStatusFetched: OnPostStatusFetched, + onPostChanged: OnPostChanged? = null + ): AutoSavePostIfNotDraftUseCase { + val dispatcher = mock() + val useCase = AutoSavePostIfNotDraftUseCase( + dispatcher = dispatcher, + postStore = postStore, + bgDispatcher = TEST_DISPATCHER + ) + whenever(dispatcher.dispatch(argWhere> { it.type == PostAction.FETCH_POST_STATUS })).then { + useCase.onPostStatusFetched(onPostStatusFetched) + } + onPostChanged?.let { onPostChangedEvent -> + whenever(dispatcher.dispatch(argWhere> { it.type == PostAction.REMOTE_AUTO_SAVE_POST })).then { + useCase.onPostChanged(onPostChangedEvent) + } + } + return useCase + } private fun createRemotePostPayload() = RemotePostPayload(PostModel(), SiteModel()) @@ -82,6 +115,12 @@ class AutoSavePostIfNotDraftUseCaseTest { constructor.isAccessible = true return constructor.newInstance(post, status, error) } + + private fun createOnPostChangedEvent(post: PostModel, error: PostError? = null): OnPostChanged { + val event = OnPostChanged(CauseOfOnPostChanged.RemoteAutoSavePost(post.id), 0) + error?.let { event.error = it } + return event + } } private fun AutoSavePostIfNotDraftUseCase.autoSavePostOrUpdateDraftAndAssertResult( From a3d06fbfe3ac851ee46e6e7b5faf8d52c8098de7 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 16:33:28 -0500 Subject: [PATCH 14/16] Adds post is draft in remote unit test for AutoSavePostIfNotDraftUseCase --- .../AutoSavePostIfNotDraftUseCaseTest.kt | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt index 440d19640a93..11f2b3efb638 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt @@ -33,8 +33,8 @@ import java.util.concurrent.TimeUnit private const val FETCH_POST_STATUS_ERROR_MESSAGE = "FETCH_POST_STATUS_ERROR_MESSAGE" private const val AUTO_SAVE_POST_ERROR_MESSAGE = "AUTO_SAVE_POST_ERROR_MESSAGE" -private const val DRAFT_STATUS = "DRAFT" -private const val PUBLISH_STATUS = "PUBLISH" +private const val DRAFT_STATUS = "draft" +private const val PUBLISH_STATUS = "publish" @InternalCoroutinesApi @RunWith(MockitoJUnitRunner::class) @@ -49,7 +49,7 @@ class AutoSavePostIfNotDraftUseCaseTest { val remotePostPayload = createRemotePostPayload() val onPostStatusFetched = createOnPostStatusFetchedEvent( post = remotePostPayload.post, - status = DRAFT_STATUS, + status = PUBLISH_STATUS, error = PostError(UNKNOWN_POST, FETCH_POST_STATUS_ERROR_MESSAGE) ) val useCase = createUseCase(onPostStatusFetched) @@ -58,6 +58,19 @@ class AutoSavePostIfNotDraftUseCaseTest { } } + @Test + fun `post is draft in remote`() { + val remotePostPayload = createRemotePostPayload() + val onPostStatusFetched = createOnPostStatusFetchedEvent( + post = remotePostPayload.post, + status = DRAFT_STATUS + ) + val useCase = createUseCase(onPostStatusFetched) + useCase.autoSavePostOrUpdateDraftAndAssertResult(remotePostPayload) { result -> + assertThat(result).isInstanceOf(AutoSavePostIfNotDraftResult.PostIsDraftInRemote::class.java) + } + } + @Test fun `error while auto-saving will result in PostAutoSaveFailed`() { val remotePostPayload = createRemotePostPayload() From 2c771707c27b81ffad085fa16d9a896c5edd646e Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 16:43:02 -0500 Subject: [PATCH 15/16] Adds post auto-saved unit test for AutoSavePostIfNotDraftUseCase --- .../posts/AutoSavePostIfNotDraftUseCaseTest.kt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt index 11f2b3efb638..6a3f206d3324 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt @@ -1,6 +1,7 @@ package org.wordpress.android.ui.posts import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.argWhere import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.whenever @@ -88,6 +89,21 @@ class AutoSavePostIfNotDraftUseCaseTest { } } + @Test + fun `post auto-saved`() { + whenever(postStore.getPostByLocalPostId(any())).thenReturn(PostModel()) + val remotePostPayload = createRemotePostPayload() + val onPostStatusFetched = createOnPostStatusFetchedEvent( + post = remotePostPayload.post, + status = PUBLISH_STATUS + ) + val onPostChanged = createOnPostChangedEvent(remotePostPayload.post) + val useCase = createUseCase(onPostStatusFetched, onPostChanged) + useCase.autoSavePostOrUpdateDraftAndAssertResult(remotePostPayload) { result -> + assertThat(result).isInstanceOf(AutoSavePostIfNotDraftResult.PostAutoSaved::class.java) + } + } + private fun createUseCase( onPostStatusFetched: OnPostStatusFetched, onPostChanged: OnPostChanged? = null From 26db1f9e3b6ea4882c73f4a65124dd6d1d5fdcd7 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 13 Feb 2020 16:52:44 -0500 Subject: [PATCH 16/16] Adds unit test for calling autoSavePostOrUpdateDraft with a local draft --- .../ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt index 6a3f206d3324..1da553ebc67f 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/AutoSavePostIfNotDraftUseCaseTest.kt @@ -28,6 +28,7 @@ import org.wordpress.android.fluxc.store.PostStore.RemotePostPayload import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftResult import org.wordpress.android.ui.uploads.AutoSavePostIfNotDraftUseCase import org.wordpress.android.ui.uploads.OnAutoSavePostIfNotDraftCallback +import java.lang.IllegalArgumentException import java.lang.reflect.Constructor import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit @@ -45,6 +46,19 @@ class AutoSavePostIfNotDraftUseCaseTest { @Mock lateinit var postStore: PostStore + @Test(expected = IllegalArgumentException::class) + fun `local draft throws IllegalArgumentException`() { + val useCase = AutoSavePostIfNotDraftUseCase(mock(), postStore, TEST_DISPATCHER) + val post = PostModel() + post.setIsLocalDraft(true) + useCase.autoSavePostOrUpdateDraft( + RemotePostPayload(post, SiteModel()), + object : OnAutoSavePostIfNotDraftCallback { + override fun handleAutoSavePostIfNotDraftResult(result: AutoSavePostIfNotDraftResult) { + } + }) + } + @Test fun `error while fetching post status will result in FetchPostStatusFailed`() { val remotePostPayload = createRemotePostPayload()