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.

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

Fix issue where two identical drafts were created #10009

Merged
merged 1 commit into from Jun 7, 2019

Conversation

@malinajirka
Copy link
Contributor

commented Jun 7, 2019

Fixes #9347

Two identical drafts were being created under certain circumstances.

I've noticed that before we add a post into the sQueuedPostsList we check if it's already there and when it is, we just update it. However, the issue is that if the post is being uploaded at the moment, it's no longer present in the sQueuedPostsList.
When we finish the upload of the first instance of the post, the upload of the second instance starts. However, it's still marked as localDraft and it doesn't have valid remotePostId. This PR fixes the issue and updates these two fields when the upload of the first instance of the post finishes.

To test:

  1. Go to My Site -> Blog Posts
  2. Click on Create post button
  3. Enter title + content
  4. Click on toolbar overflow menu -> Save as draft
  5. Press Up button (arrow) before the upload finishes
  6. Notice the app doesn't create two drafts

  1. Go to My Site -> Blog Posts
  2. Click on Create post button
  3. Enter "123" into title
  4. Click on toolbar overflow menu -> Save as draft
  5. Edit title to "1234"
  6. Press Up button (arrow) before the upload finishes
  7. Notice the app doesn't create two drafts and the most recent version "1234" gets uploaded

Update release notes:

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

Before/After gifs
duplicated-draft
fixed-duplicated-draft

@malinajirka malinajirka added this to the 12.7 milestone Jun 7, 2019

@malinajirka malinajirka requested review from maxme, shiki and mzorz Jun 7, 2019

@mzorz

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Nice catch @malinajirka ! Will check and test later 👍

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Btw I was also thinking about adding something like this

             // firstly check if the post which is currently being uploaded 
            // isn't equal to the post we are about to enqueue
            if (sCurrentUploadingPost != null
                && sCurrentUploadingPost.getId() == post.getId()
                && sCurrentUploadingPost.equals(post)
            ) {
                AppLog.i(T.POSTS, "Equal post is currently being uploaded - skipping.");
                return;
            }

here.

However, the posts aren't equal - they have different dateLocallyChanged field. I want to think about it for a bit before I make the necessary changes. Let me know if you have any thoughts about it;). Thanks

@mzorz

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

However, the posts aren't equals - they have different dateLocallyChanged field.

For the sake of comparing, I think you could just check for sCurrentUploadingPost.getId() == post.getId() and forget about equality of other fields.

However, at that point, if another post with same id is currently being uploaded then it makes sense to enqueue the Post being passed as parameter now (as the latest version / modification should be coming in anyway). Wdyt?

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

For the sake of comparing, I think you could just check for sCurrentUploadingPost.getId() == post.getId() and forget about equality of other fields.
However, at that point, if another post with same id is currently being uploaded then it makes sense to enqueue the Post being passed as parameter now (as the latest version / modification should be coming in anyway). Wdyt?

If you follow the test scenario #1, an identical version of the post is enqueue twice - in steps 4 and 5. I would like to prevent the second upload as it's pointless. However, I actually need to do "full" comparison as otherwise the test scenario #2 wouldn't work -> we wouldn't update the post with "1234".

@mzorz

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

If you follow the test scenario #1, an identical version of the post is enqueue twice - in steps 4 and 5. I would like to prevent the second upload as it's pointless. However, I actually need to do "full" comparison as otherwise the test scenario #2 wouldn't work -> we wouldn't update the post with "1234".

Oh that's right, I see that now. It may be worth it to check every other field except dateLocallyChanged . If every other field is equal, then while technically these are not the same, their relevant properties are, hence in practice it doesn't make sense to update the Post with properties that are of identical state. Wdyt?

@mzorz

mzorz approved these changes Jun 7, 2019

Copy link
Contributor

left a comment

PR LGTM, this takes care of the issue.
Optimisation being discussed in the comments can be continued elsewhere / even here after merge happens.

@mzorz mzorz merged commit 86b0db4 into develop Jun 7, 2019

4 checks passed

Peril All green. Nice work.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@mzorz mzorz added this to Done (PRs) in Offline Support: Posting [Android] via automation Jun 7, 2019

@mzorz mzorz deleted the issue/9347-duplicated-drafts branch Jun 7, 2019

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Thanks @mzorz for the review!;)

It may be worth it to check every other field except dateLocallyChanged . If every other field is equal, then while technically these are not the same, their relevant properties are

I agree, but comparing them manually seems a bit clumsy + if we ever add a new field, we might not remember to update this comparison. We could create a second equals in the PostModel, but it just doesn't seem right to me. I'd probably vote for keeping the current behavior. The double-upload won't happen that often and the only downside is that we upload the post twice. I believe it's not such a big deal. Wdyt?

@mzorz

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

The double-upload won't happen that often and the only downside is that we upload the post twice. I believe it's not such a big deal. Wdyt?

Agreed, that's a reasonable trade off 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.