-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Autosave or update draft #11251
Merged
Merged
Autosave or update draft #11251
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
be3ad8b
A rough introduction of AutoSavePostIfNotDraftUseCase
oguzkocer 32da8c4
Rough integration of AutoSavePostIfNotDraftUseCase
oguzkocer ab80870
Refactor AutoSavePostIfNotDraftUseCase so it's reusable
oguzkocer 4cf02b7
Update FluxC hash which contains some fixes to fetching post status
oguzkocer 738e71e
Throw IllegalArgumentException if autosave use case is already handli…
oguzkocer 6767c91
Refactor AutoSavePostIfNotDraftUseCase
oguzkocer 0b2a89e
Handle errors in AutoSavePostIfNotDraftUseCase
oguzkocer a70f33e
Merge remote-tracking branch 'origin/develop' into issue/autosave-or-…
oguzkocer d01c3f8
Handle all AutoSavePostIfNotDraftResult types
oguzkocer bb985b1
Observe AutoSavePostIfNotDraftUseCase.onPostChanged on MAIN thread wi…
oguzkocer 63fd0fe
Don't show error notifications for post autosave fails
oguzkocer 1268a78
Add documentation for auto-save use case
oguzkocer 174b742
Sets up AutoSavePostIfNotDraftUseCaseTest and adds fetch post status …
oguzkocer 19fad9d
Adds unit test for PostAutoSaveFailed
oguzkocer a3d06fb
Adds post is draft in remote unit test for AutoSavePostIfNotDraftUseCase
oguzkocer 2c77170
Adds post auto-saved unit test for AutoSavePostIfNotDraftUseCase
oguzkocer 26db1f9
Adds unit test for calling autoSavePostOrUpdateDraft with a local draft
oguzkocer 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
144 changes: 144 additions & 0 deletions
144
WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt
This file contains 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,144 @@ | ||
| package org.wordpress.android.ui.uploads | ||
|
|
||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.GlobalScope | ||
| import kotlinx.coroutines.launch | ||
| 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.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.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 | ||
| import javax.inject.Inject | ||
| import javax.inject.Named | ||
| import kotlin.coroutines.Continuation | ||
| import kotlin.coroutines.resume | ||
|
|
||
| private const val DRAFT_POST_STATUS = "draft" | ||
|
|
||
| interface OnAutoSavePostIfNotDraftCallback { | ||
| fun handleAutoSavePostIfNotDraftResult(result: AutoSavePostIfNotDraftResult) | ||
| } | ||
|
|
||
| sealed class AutoSavePostIfNotDraftResult(open val post: PostModel) { | ||
| // Initial fetch post status request failed | ||
| 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(override val post: PostModel) : AutoSavePostIfNotDraftResult(post) | ||
|
|
||
| // Post status is not `DRAFT` in remote and the post was auto-saved successfully | ||
| 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(override val post: PostModel, val error: PostError) : | ||
| AutoSavePostIfNotDraftResult(post) | ||
| } | ||
|
|
||
| // 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, | ||
| @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher | ||
| ) { | ||
| private val postStatusContinuations = HashMap<LocalId, Continuation<OnPostStatusFetched>>() | ||
| private val autoSaveContinuations = HashMap<LocalId, Continuation<OnPostChanged>>() | ||
|
|
||
| init { | ||
| dispatcher.register(this) | ||
| } | ||
|
|
||
| fun autoSavePostOrUpdateDraft( | ||
| remotePostPayload: RemotePostPayload, | ||
| 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." | ||
| ) | ||
| } | ||
| GlobalScope.launch(bgDispatcher) { | ||
| 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): OnPostStatusFetched { | ||
| val localPostId = LocalId(remotePostPayload.post.id) | ||
| return suspendCancellableCoroutine { cont -> | ||
| postStatusContinuations[localPostId] = cont | ||
| dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(remotePostPayload)) | ||
| } | ||
| } | ||
|
|
||
| private suspend fun autoSavePost(remotePostPayload: RemotePostPayload): AutoSavePostIfNotDraftResult { | ||
| val localPostId = LocalId(remotePostPayload.post.id) | ||
| 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) | ||
| } | ||
| } | ||
|
|
||
| @Subscribe(threadMode = BACKGROUND) | ||
| @Suppress("unused") | ||
| fun onPostStatusFetched(event: OnPostStatusFetched) { | ||
| val localPostId = LocalId(event.post.id) | ||
| postStatusContinuations[localPostId]?.let { continuation -> | ||
| continuation.resume(event) | ||
| postStatusContinuations.remove(localPostId) | ||
| } | ||
| } | ||
|
|
||
| @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 -> | ||
| continuation.resume(event) | ||
| autoSaveContinuations.remove(localPostId) | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains 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
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.
I'm sorry I missed this during the initial review 😞
The previous implementation was subscribed on the MAIN thread and had priority set to 9. We wanted to ensure the PostUploadHandler receives the event before UploadService.
The UploadService invokes
stopServiceIfUploadsCompleteand checksmPostUploadHandler.hasInProgressUploads(). If we don't use the priority, we'll introduce a race condition asmPostUploadHandler.hasInProgressUploads()will return true or false depending on which class received the event first, UploadService vs PostUploadHandler.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 have made this change in bb985b1, but I couldn't test it because I can't reproduce the issue even before the change. Also, as discussed on Slack, I think there is a better way to handle the priority issue, so I'll open an issue for that once this PR is merged.