Prevent duplicate posts in Post List #6102

astralbodies · 2016-10-20T20:02:06Z

Original issue:
Sometimes when publishing a new post a duplicate post will appear in the post list. It was determined that the cause of this is the dismissal of the editor before the post has finished being published. The post list view controller initiated a refresh of posts before the post saves properly to Core Data and then a duplicate appears. If the posting was done within the timeout period of the list view controller (less than 5 minutes) then the issue wouldn't present itself.

Several improvements were made:

Added KVC life cycle calls to AbstractPost to prevent any possible issues with accessing original and revision.
Updated the filter on the post list to only show "original" posts. The filter was showing original and revision posts which can add to the confusion.
Added a method to MediaService to allow for uploading of multiple media and not calling success until they have all finished.
Prevent the editor UI from being dismissed until the publishing (and updating of media) has completed. Added another HUD to indicate an operation is in progress.

To test:

Change postsControllerRefreshInterval in AbstractPostListViewController to a low value like 1.
Publish a new post.
Verify no duplicate posts appear in the list.

Variations of "new post" should include:

Published and Draft posts.
With and without media.
Single and multiple media.

We should also try offline condition testing as I've slightly altered behavior with the failure handlers.

Needs review: @SergioEstevao @nheagy @diegoreymendez @jleandroperez

SergioEstevao · 2016-10-20T21:31:08Z

WordPress/Classes/Services/MediaService.m

+
+    for (Media *media in mediaObjects) {
+        // This effectively ignores any errors presented
+        [self updateMedia:media success:individualOperationSuccess failure:individualOperationSuccess];


Should we ignore the errors at the service level? I prefer to leave that to whom invokes the method. If we have the failure callback we should make use of it.
If not the remove the failure callback and tell in the method comment that all errors are ignored and you callback when all operations finish independently of state.

Definitely agreed - I'll fix this up.

diegoreymendez · 2016-10-21T00:12:38Z

Updated the filter on the post list to only show "original" posts. The filter was showing original and revision posts which can add to the confusion.

One of the reasons why revision posts were being shown in the main list is that they offered visual cues that the post had local changes (that weren't uploaded to the server).

Here's how a post with local / unsaved changes looked before:

Here's how it looks now:

diegoreymendez · 2016-10-20T23:59:26Z

WordPress/Classes/Models/AbstractPost.m

@@ -292,12 +294,20 @@ - (AbstractPost *)latest

 - (AbstractPost *)revision
 {
-    return [self primitiveValueForKey:@"revision"];
+    [self willAccessValueForKey:@"revision"];


This seems like a vanilla implementation of the CoreData version of this getter. The same applies to the getter for original.

I can't recall if this was there for a reason, but it sounds to me like we could simply remove these methods.

Thoughts on this?

I did try replacing the read-only getter with a read/write and drop the getter code. For some reason it majorly screwed up the filtering in the post list view controller. I didn't have time to dig down that rabbit hole so I reverted that change and put the KVC method calls in here.

jleandroperez · 2016-10-21T12:26:18Z

WordPress/Classes/Models/AbstractPost.m

            [self setPrimitiveValue:nil forKey:@"revision"];
+            [self didChangeValueForKey:@"revision"];


You know... THIS was precisely the root of a Simplenote bug which took us months to reproduce.

YAY for this change!

jleandroperez · 2016-10-21T12:39:14Z

UX decisions aside (whether to show Local or not), stepping by to confirm the fix.

IMHO it's way better not to display duplicated entries, rather than rendering extra post details (Local Modifications).

on my side!!

astralbodies · 2016-10-21T13:08:29Z

WordPress/Classes/ViewRelated/Post/PostListViewController.swift

@@ -251,6 +251,7 @@ class PostListViewController : AbstractPostListViewController, UIViewControllerR
        var predicates = [NSPredicate]()

        if let blog = blog {
+            // Show all original posts without a revision & revision posts.
            let basePredicate = NSPredicate(format: "blog = %@ && revision = nil", blog)


@diegoreymendez I reverted the change here and added a comment to emphasize the importance of why revision was selected. 😄

astralbodies · 2016-10-21T13:21:06Z

WordPress/Classes/Services/MediaService.m

+                if (overallSuccess && failedOperations != totalOperations.unsignedIntegerValue) {
+                    overallSuccess();
+                } else if (failure && failedOperations == totalOperations.unsignedIntegerValue) {
+                    NSError *error = [NSError errorWithDomain:WordPressComRestApiErrorDomain


@SergioEstevao I added a condition that if all of the operations fail that the error completion handler is called. This matches my original intended behavior as described in the header of the class.

@astralbodies

Addressed by @astralbodies!

jleandroperez

Github: PLEASE fix your UI

SergioEstevao · 2016-10-21T14:44:54Z

WordPress/Classes/Services/MediaService.m

+                if (overallSuccess && failedOperations != totalOperations.unsignedIntegerValue) {
+                    overallSuccess();
+                } else if (failure && failedOperations == totalOperations.unsignedIntegerValue) {
+                    NSError *error = [NSError errorWithDomain:WordPressComRestApiErrorDomain


diegoreymendez

Nice work @astralbodies!

nheagy

Nice to see this cleaned up!

nheagy · 2016-10-21T21:15:35Z

WordPress/Classes/ViewRelated/Post/WPPostViewController.m

+        hudText = NSLocalizedString(@"Publishing...", @"Text displayed in HUD while a post is being published.");
+    } else {
+        hudText = NSLocalizedString(@"Saving...", @"Text displayed in HUD while a post is being saved as a draft.");
+    }


Nice UX improvement here. This fixes #6079?

@nheagy It does not fix this behavior if this code is broken. We definitely need to work on a state machine for this status stuff - it's super confusing.

astralbodies · 2016-10-24T15:17:08Z

Thanks everyone for the reviews!

astralbodies added 7 commits October 20, 2016 14:32

Add missing KVC lifecycle calls to AbstractPost for original & revision

836d6e4

Code comments cleanup

1c2f650

Add functionality to upload multiple media and wait for completion.

9f92268

Prevent any Core Data issues with temp IDs for new posts.

ef56b2a

Update PostService to use batch media upload method in MediaService.

d707022

Hide revisions from post list - only show originals.

89a22d0

Do not dismiss the post UI until upload has finished.

509e613

astralbodies added Posting/Editing [Status] Not Ready for Review labels Oct 20, 2016

astralbodies added this to the 6.6 milestone Oct 20, 2016

astralbodies assigned nheagy, SergioEstevao, jleandroperez and diegoreymendez Oct 20, 2016

SergioEstevao previously requested changes Oct 20, 2016

View reviewed changes

astralbodies removed the [Status] Not Ready for Review label Oct 20, 2016

diegoreymendez reviewed Oct 21, 2016

View reviewed changes

jleandroperez reviewed Oct 21, 2016

View reviewed changes

Reverting a change to the post list filter.

b174321

astralbodies commented Oct 21, 2016

View reviewed changes

If all media update operations fail, call the error completion block.

415e5d4

astralbodies commented Oct 21, 2016

View reviewed changes

astralbodies modified the milestones: 6.7, 6.6 Oct 21, 2016

jleandroperez approved these changes Oct 21, 2016

View reviewed changes

SergioEstevao approved these changes Oct 21, 2016

View reviewed changes

diegoreymendez approved these changes Oct 21, 2016

View reviewed changes

nheagy approved these changes Oct 21, 2016

View reviewed changes

astralbodies merged commit 02e2d5a into develop Oct 24, 2016

astralbodies deleted the issue/fix-dupe-posts branch October 24, 2016 15:17

kurzee mentioned this pull request Oct 24, 2016

Posts: Duplicate post appears in post list after saving/publishing #5364

Closed

shiki mentioned this pull request Oct 29, 2019

[Offline Posting] Duplicate posts may appear in the Post List #12800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate posts in Post List #6102

Prevent duplicate posts in Post List #6102

astralbodies commented Oct 20, 2016

SergioEstevao Oct 20, 2016

astralbodies Oct 20, 2016

diegoreymendez commented Oct 21, 2016 •

edited

Loading

diegoreymendez Oct 20, 2016

astralbodies Oct 21, 2016

jleandroperez Oct 21, 2016

jleandroperez commented Oct 21, 2016

astralbodies Oct 21, 2016

astralbodies Oct 21, 2016

SergioEstevao Oct 21, 2016

jleandroperez left a comment

SergioEstevao Oct 21, 2016

diegoreymendez left a comment

nheagy left a comment

nheagy Oct 21, 2016

astralbodies Oct 24, 2016

astralbodies commented Oct 24, 2016

		[self setPrimitiveValue:nil forKey:@"revision"];
		[self didChangeValueForKey:@"revision"];

Prevent duplicate posts in Post List #6102

Prevent duplicate posts in Post List #6102

Conversation

astralbodies commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez commented Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleandroperez commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleandroperez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez left a comment

Choose a reason for hiding this comment

nheagy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astralbodies commented Oct 24, 2016

diegoreymendez commented Oct 21, 2016 •

edited

Loading