Skip to content
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.

Sign up for GitHub

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 17 commits into from
Feb 19, 2020
Merged

Autosave or update draft #11251

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
be3ad8b
A rough introduction of AutoSavePostIfNotDraftUseCase
oguzkocer Feb 6, 2020
32da8c4
Rough integration of AutoSavePostIfNotDraftUseCase
oguzkocer Feb 6, 2020
ab80870
Refactor AutoSavePostIfNotDraftUseCase so it's reusable
oguzkocer Feb 6, 2020
4cf02b7
Update FluxC hash which contains some fixes to fetching post status
oguzkocer Feb 6, 2020
738e71e
Throw IllegalArgumentException if autosave use case is already handli…
oguzkocer Feb 10, 2020
6767c91
Refactor AutoSavePostIfNotDraftUseCase
oguzkocer Feb 10, 2020
0b2a89e
Handle errors in AutoSavePostIfNotDraftUseCase
oguzkocer Feb 10, 2020
a70f33e
Merge remote-tracking branch 'origin/develop' into issue/autosave-or-…
oguzkocer Feb 11, 2020
d01c3f8
Handle all AutoSavePostIfNotDraftResult types
oguzkocer Feb 11, 2020
bb985b1
Observe AutoSavePostIfNotDraftUseCase.onPostChanged on MAIN thread wi…
oguzkocer Feb 13, 2020
63fd0fe
Don't show error notifications for post autosave fails
oguzkocer Feb 13, 2020
1268a78
Add documentation for auto-save use case
oguzkocer Feb 13, 2020
174b742
Sets up AutoSavePostIfNotDraftUseCaseTest and adds fetch post status …
oguzkocer Feb 13, 2020
19fad9d
Adds unit test for PostAutoSaveFailed
oguzkocer Feb 13, 2020
a3d06fb
Adds post is draft in remote unit test for AutoSavePostIfNotDraftUseCase
oguzkocer Feb 13, 2020
2c77170
Adds post auto-saved unit test for AutoSavePostIfNotDraftUseCase
oguzkocer Feb 13, 2020
26db1f9
Adds unit test for calling autoSavePostOrUpdateDraft with a local draft
oguzkocer Feb 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/ui/uploads/AutoSavePostIfNotDraftUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
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
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.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 error result types
}

// TODO: Add documentation
// TODO: Add unit tests
class AutoSavePostIfNotDraftUseCase @Inject constructor(
private val dispatcher: Dispatcher,
private val postStore: PostStore
) {
private val postStatusContinuations = HashMap<LocalId, Continuation<String>>()
private val autoSaveContinuations = HashMap<LocalId, Continuation<PostModel>>()

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
malinajirka marked this conversation as resolved.
Show resolved Hide resolved
// We are already handling this post
return
}
// TODO: What scope should we use for this?
GlobalScope.launch {
malinajirka marked this conversation as resolved.
Show resolved Hide resolved
val remotePostStatus: String = suspendCancellableCoroutine { cont ->
postStatusContinuations[localPostId] = cont
dispatcher.dispatch(PostActionBuilder.newFetchPostStatusAction(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
)
)
}
callback.handleAutoSavePostIfNotDraftResult(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)
postStatusContinuations.remove(localPostId)
}
}

// TODO: Handle errors
@Subscribe(threadMode = BACKGROUND)
Copy link
Contributor

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 stopServiceIfUploadsComplete and checks mPostUploadHandler.hasInProgressUploads(). If we don't use the priority, we'll introduce a race condition as mPostUploadHandler.hasInProgressUploads() will return true or false depending on which class received the event first, UploadService vs PostUploadHandler.

Wdyt?

Copy link
Contributor Author

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.

@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)
autoSaveContinuations.remove(localPostId)
}
}
}
}
39 changes: 22 additions & 17 deletions WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -62,7 +61,7 @@

import javax.inject.Inject;

public class PostUploadHandler implements UploadHandler<PostModel> {
public class PostUploadHandler implements UploadHandler<PostModel>, OnAutoSavePostIfNotDraftCallback {
private static ArrayList<PostModel> sQueuedPostsList = new ArrayList<>();
private static Set<Integer> sFirstPublishPosts = new HashSet<>();
private static PostModel sCurrentUploadingPost = null;
Expand All @@ -79,6 +78,7 @@ public class PostUploadHandler implements UploadHandler<PostModel> {
@Inject MediaStore mMediaStore;
@Inject UiHelpers mUiHelpers;
@Inject UploadActionUseCase mUploadActionUseCase;
@Inject AutoSavePostIfNotDraftUseCase mAutoSavePostIfNotDraftUseCase;

PostUploadHandler(PostUploadNotifier postUploadNotifier) {
((WordPress) WordPress.getContext().getApplicationContext()).component().inject(this);
Expand Down Expand Up @@ -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);
malinajirka marked this conversation as resolved.
Show resolved Hide resolved
break;
case DO_NOTHING:
AppLog.d(T.POSTS, "PostUploadHandler - DO_NOTHING. Post: " + mPost.getTitle());
Expand Down Expand Up @@ -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
malinajirka marked this conversation as resolved.
Show resolved Hide resolved
* 2. Set the post status to draft and push post
* 3. Update the local copy of post with the previous post status
malinajirka marked this conversation as resolved.
Show resolved Hide resolved
*/
}
}

/**
* 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.
Expand Down Expand Up @@ -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();
}
}
}
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ buildScan {

ext {
daggerVersion = '2.22.1'
fluxCVersion = 'c5ea220f70c3a77681d376a17ce5c57e26c1c392'
fluxCVersion = '751696ad02d3446bbfe902c21b16747745616990'
}

// Onboarding and dev env setup tasks
Expand Down