-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Sync blogs upon launch #24243
Sync blogs upon launch #24243
Conversation
@@ -135,6 +135,10 @@ class WordPressAppDelegate: UIResponder, UIApplicationDelegate { | |||
WKWebView.warmup() | |||
} | |||
|
|||
if let account = try? WPAccount.lookupDefaultWordPressComAccount(in: ContextManager.shared.mainContext) { | |||
BlogService(coreDataStack: ContextManager.shared).syncBlogs(for: account, success: { /* Do nothing */ }, failure: { _ in /* Do nothing */ }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Shouldn't it use
BlogSyncFacade
? - I was under the impression that
MySiteViewController
manages blogs sync, but it doesn't appear to be the case, does it? I'm not entirely sure what would be the ideal place for this method, but I would probably lean towards something likeRootViewCoordinator
and not the app delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it use BlogSyncFacade?
Yep, it should. Good catch!
I was under the impression that MySiteViewController manages blogs sync
My guess is that was the case, but at some point that code was deleted by accident. At the moment, the view controller only calls syncBlogs on pull-to-refresh.
I'm not entirely sure what would be the ideal place for this method
Me neither. I don't want to introduce yet another "coordinator"-like things, so I kept it simple in the app-did-launch function. I think we can refactor it later to have a more appropriate type for it? I don't think it should be in a view related type. The app should refresh some basic things (user profile, current site details, general information about all user sites, etc), regardless of the view controller/presenter the app uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can refactor it later to have a more appropriate type for it?
Fair enough. I think it should be in one of the root coordinators.
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr24243-b945158 | |
Version | 25.8 | |
Bundle ID | org.wordpress.alpha | |
Commit | b945158 | |
App Center Build | WPiOS - One-Offs #11727 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr24243-b945158 | |
Version | 25.8 | |
Bundle ID | com.jetpack.alpha | |
Commit | b945158 | |
App Center Build | jetpack-installable-builds #10755 |
8f201f8
to
b945158
Compare
This PR fixes two issues.
The first issue is the app does not sync blogs upon launch. That means the app continues to show the welcome screen after the user creates a new site from WordPress.com.
The second issue is #23344, which is the reverse: if the user deletes a site from WordPress.com, the app should not show the deleted site. Requires wordpress-mobile/WordPressKit-iOS#833.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: