Issues/3786 post list sorting #3810

aerych · 2015-06-05T21:00:13Z

Requires Issues/3786 issues with local posts #3805 *

This PR builds on work done in #3805. The relevant changes begin in dd7fa34.

The goal is to add two new helper or "meta" properties to help with sorting posts and pages returned from an NSFetchRequest. The new properties are automatically updated in response to changes to a posts date created or its remote status.

To ensure any existing posts get the correct values set, this PR includes a core data revision, mapping model, and migration policy for posts and pages.

TESTING

Setup: Prepare a local only post in the simulator BEFORE switching to this branch. For convenience, be sure to use a blog with very view posts Follow the steps outlined in this video to create a local-only post https://cloudup.com/iOZ-BQXHqyx

Test 1:

Launch the simulator. Confirm there are no errors launching the app and you do not have to sign in again.
View the post list. The local only post should be first in the list.

Test 2:

Use a link conditioner to reduce your network speed. A slow connection will help you verify this is working.
Edit an existing post. Its easier if its near the top of the list, but not THE first one in the list.
Set it to publish immediately and save your changes.
Watch closely and confirm that it is the first post in the list when publishing.

Needs Review: @koke @astralbodies

The default postID is -1, not 0.

Shows publish immedately when the date is nil. Shows a "Local" badge. Shows a placeholder title if there is no title or content.

If a post or page has a revision (local changes) display the relevant bits for the revision instead of the original. Let both Post and Page implement WPPostContentViewProvider instead of just Post by moving the relevant bits into AbstractPost. Updates Page related views to support WPPostContentViewProvider instead of WPContentViewProvider.

Adds a core data revision: 31 Adds a new migration mapping and entity migration policy.

aerych · 2015-06-05T21:01:23Z

@astralbodies Would really appreciate your eyes on the migration policy to make sure its a good approach.

koke · 2015-06-09T14:28:34Z

WordPress/Classes/Models/AbstractPost.m

+    NSString *key = @"date_created_gmt";
+    [self willChangeValueForKey:key];
+    [self setPrimitiveValue:date_created_gmt forKey:key];
+    [self didChangeValueForKey:key];


Since you're supporting KVO, I recently learned that you should register dependent keys with keyPathsForValuesAffecting*

https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/KeyValueObserving/Articles/KVODependentKeys.html#//apple_ref/doc/uid/20002179-BAJEAIEE

And I think the self.meta* = ... should be between the willChange and the didChange

keyPathsForValuesAffecting

Nice find.

koke · 2015-06-09T15:02:29Z

I might be missing something, but the metaPublishImmediately property looks redundant. In my head, local posts is something that's there so you don't lose content if the app gets killed, so it should only ever be one or two of those. Having a second sort descriptor feels like adding more complexity than it's worth.

…OS into issues/3786-post-list-sorting

astralbodies · 2015-06-09T17:52:17Z

@aerych This looks okay. I did add a very BASIC unit test to cover migrations from 19 through 31. This does not ensure the data is migrated properly, however. Can you update the unit test to be something similar to testMigrate27to28preservesCategories in CoreDataMigrationTests to make sure your intended behavior happens?

…mobile/WordPress-iOS into issues/3786-post-list-sorting

aerych · 2015-06-09T17:56:05Z

I did add a very BASIC unit test to cover migrations from 19 through 31

Awesome, thanks for that! I'll have a look at the test you mentioned.

aerych · 2015-06-09T18:06:10Z

I might be missing something, but the metaPublishImmediately property looks redundant

Its intended to cover the edge case when a non-local post (e.g. a draft that already has a remote) is edited to publish immediately, and encounters an error when publishing leaving it in a failed state. In this case the create date would still be nil, but the meta property would be flagged to indicate the correct sort order.

aerych · 2015-06-09T18:58:59Z

@astralbodies I've updated the test. Ready for another round.

astralbodies · 2015-06-10T16:25:59Z

WordPress/Classes/Utility/Migrations/PostToPost30To31.m

@@ -0,0 +1,54 @@
+#import "PostToPost30To31.h"
+#import "AbstractPost.h"


Make sure to not use any model objects in the migration as they are only valid on the most recent model version. I'd remove this import even though its not used.

astralbodies · 2015-06-10T16:54:51Z

aerych · 2015-06-10T17:14:37Z

Thanks guys!!

Issues/3786 post list sorting

aerych added 5 commits June 4, 2015 11:49

Fixes issue deleting local-only posts.

3de6e4e

The default postID is -1, not 0.

Do not support stats if a post has no remote.

ef3e729

Improves appearance of local-only posts.

5e79cc1

Shows publish immedately when the date is nil. Shows a "Local" badge. Shows a placeholder title if there is no title or content.

Adds "meta" properties to act as sorting helpers for post/pages.

dd7fa34

Adds a core data revision: 31 Adds a new migration mapping and entity migration policy.

aerych added this to the 5.3 milestone Jun 5, 2015

koke reviewed Jun 9, 2015
View reviewed changes

aerych and others added 2 commits June 9, 2015 12:31

Merge branch 'release/5.3' of github.com:wordpress-mobile/WordPress-i…

234c09f

…OS into issues/3786-post-list-sorting

Added simple migration test for 19 through 31 model versions

d9d8a3f

aerych and others added 4 commits June 9, 2015 12:52

Use intended variable instead of const

39654c3

My mother always told me I am bad at names

3863b9e

Remove comments. Improve KVO standards adherence.

2e30b0e

Merge branch 'issues/3786-post-list-sorting' of github.com:wordpress-…

cba0606

…mobile/WordPress-iOS into issues/3786-post-list-sorting

Adding data integrity checks to migration test.

6aafc0b

astralbodies reviewed Jun 10, 2015
View reviewed changes

aerych added a commit that referenced this pull request Jun 10, 2015

Merge pull request #3810 from /issues/3786-post-list-sorting

5eaeff1

Issues/3786 post list sorting

aerych merged commit 5eaeff1 into release/5.3 Jun 10, 2015

aerych deleted the issues/3786-post-list-sorting branch June 10, 2015 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues/3786 post list sorting #3810

Issues/3786 post list sorting #3810

aerych commented Jun 5, 2015

aerych commented Jun 5, 2015

koke Jun 9, 2015

koke Jun 9, 2015

aerych Jun 9, 2015

koke commented Jun 9, 2015

astralbodies commented Jun 9, 2015

aerych commented Jun 9, 2015

aerych commented Jun 9, 2015

aerych commented Jun 9, 2015

astralbodies Jun 10, 2015

aerych Jun 10, 2015

astralbodies commented Jun 10, 2015

aerych commented Jun 10, 2015

		@@ -0,0 +1,54 @@
		#import "PostToPost30To31.h"
		#import "AbstractPost.h"

Issues/3786 post list sorting #3810

Issues/3786 post list sorting #3810

Conversation

aerych commented Jun 5, 2015

TESTING

aerych commented Jun 5, 2015

koke Jun 9, 2015

Choose a reason for hiding this comment

koke Jun 9, 2015

Choose a reason for hiding this comment

aerych Jun 9, 2015

Choose a reason for hiding this comment

koke commented Jun 9, 2015

astralbodies commented Jun 9, 2015

aerych commented Jun 9, 2015

aerych commented Jun 9, 2015

aerych commented Jun 9, 2015

astralbodies Jun 10, 2015

Choose a reason for hiding this comment

aerych Jun 10, 2015

Choose a reason for hiding this comment

astralbodies commented Jun 10, 2015

aerych commented Jun 10, 2015