Retire blog.isWPcom #3725
Retire blog.isWPcom #3725
Conversation
Uses BlogFeatureVisibility to check if a blog can be hidden. I had to duplicate some logic due to SQLite-backed core data not being compatible with block predicates. I added some cross-comments about this hoping that it won't break in the future.
Logic was reversed
In #3635 we switched to a unique list for all the sites in the app. Update the blog selector to match that
Also, it uses the primary blog first if there is one Fixes #2599
Created some helper methods to reuse fetch request and predicate creation
Only supported if the associated wp.com account is the defatult
I think that, in theory, this should work with Jetpack sites as well, but: 1. I'm not sure if stats notifications are generated for Jetpack sites 2. The call to -[BlogService blogByBlogId:] won't find the right site for non-REST Jetpack sites, as blogID will contain the self hosted ID.
We are already checking supports:BlogFeatureStats, there's no need to check isWPcom
Since a blog can belong to a wp.com account but be hosted somewhere else, store the `jetpack` value from REST into `blog.isJetpack` so we can properly implement `isHostedAtWPcom`.
For accuracy and ugliness!
|
I noticed this one has #3704 merged in, so ignore the changes in |
|
Core Data changes look good! |
| @@ -8,6 +8,29 @@ | |||
| @class WPAccount; | |||
| @class WordPressComApi; | |||
|
|
|||
| typedef NS_ENUM(NSUInteger, BlogFeature) { | |||
|
Notifications changes look good!. |
on my side!
| NSMutableArray *mButtons = [NSMutableArray arrayWithObjects:self.metaButtonLeft, self.metaButtonRight, nil]; | ||
| if ([self.contentProvider numberOfComments] > 0) { | ||
| if ([self.contentProvider supportsComments] && [self.contentProvider numberOfComments] > 0) { |
There was a problem hiding this comment.
Not sure what's gained by adding the supportsComments and supportsLikes checks since the count would just be 0 if not supported.
There was a problem hiding this comment.
You could say the same about the previous isWPcom check 😉
I don't know, they're probably not necessary, but it seemed good to add the explicit check
There was a problem hiding this comment.
🤷 maybe? Seems like its just adding noise if there is no real purpose. If the next dev who comes along would wonder why both checks were being made maybe we don't need it.
There was a problem hiding this comment.
Dropped in 77b4fad
\o/ for less code 😄
|
|
Conflicts: WordPress/Classes/Models/Blog.h WordPress/Classes/ViewRelated/Blog/EditSiteViewController.m
|
YAY! |
blog.isWPcomwas usually not the right question.Now there's
blog. isHostedAtWPcomto check if a blog is really hosted at WordPress.com, but most checks are done with[blog supports:feature]There's a more detailed list of all the checks that changed in #3700
@jleandroperez I limited push notification support to the default account, since I don't think it'll work if a Jetpack site is connected to a different account:
NotificationsManageronly uses the default account, and the toggle push notifications switch on site settings won't find non-default blog IDs on thenotification_preferencesdictionary.@astralbodies are you the guardian of new data models? Because there's a new one 💂♂️
@aerych reader views seem to behave fine, and comment likes, reblogs,... seem to be allowed when they need to (except #3724)
Fixes #3700
Needs Review: @jleandroperez @astralbodies @aerych