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.

Sign up for GitHub

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

Blaze: Update blaze instance from the correct thread #20413

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Blaze: Update blaze instance from the correct thread #20413

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef93b89
Fix: update blog instance from the correct thread
Mar 27, 2023
20eddd8
Fix: don’t call completion closure during core data ops
Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions WordPress/Classes/Services/BlazeService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,31 +33,41 @@ import WordPressKit
completion: (() -> Void)? = nil) {

guard BlazeHelper.isBlazeFlagEnabled() else {
updateBlog(blog, isBlazeApproved: false, completion: completion)
updateBlogWithID(blog.objectID, isBlazeApproved: false, completion: completion)
return
}

guard let siteId = blog.dotComID?.intValue else {
DDLogError("Invalid site ID for Blaze")
updateBlog(blog, isBlazeApproved: false, completion: completion)
updateBlogWithID(blog.objectID, isBlazeApproved: false, completion: completion)
return
}

remote.getStatus(forSiteId: siteId) { result in
switch result {
case .success(let approved):
self.updateBlog(blog, isBlazeApproved: approved, completion: completion)
self.updateBlogWithID(blog.objectID, isBlazeApproved: approved, completion: completion)
case .failure(let error):
DDLogError("Unable to fetch isBlazeApproved value from remote: \(error.localizedDescription)")
self.updateBlog(blog, isBlazeApproved: false, completion: completion)
self.updateBlogWithID(blog.objectID, isBlazeApproved: false, completion: completion)
}
}
}

private func updateBlog(_ blog: Blog, isBlazeApproved: Bool, completion: (() -> Void)? = nil) {
private func updateBlogWithID(_ objectID: NSManagedObjectID,
isBlazeApproved: Bool,
completion: (() -> Void)? = nil) {
contextManager.performAndSave({ context in

guard let blog = context.object(with: objectID) as? Blog else {
DDLogError("Unable to update isBlazeApproved value for blog")
completion?()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completion block can't be called here, otherwise it'll cause the same issue in #20196.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be declaring contextManager as CoreDataStackSwift, so that you can catch the error thrown by the Core Data operation closure:

contextManager.performAndSave({ context in
    let blog = try context.existingObject(with: objectID) as! Blog
    blog.isBlazeApproved = isBlazeApproved
}, completion: { (result: Result<Void, Error>) in
    completion?()
}, on: .main)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completion block can't be called here

Fixed! 20eddd8

We're only logging the error, so I decided to use guard let blog = try? ...

return
}

blog.isBlazeApproved = isBlazeApproved
DDLogInfo("Successfully updated isBlazeApproved value for blog: \(isBlazeApproved)")

}, completion: {
completion?()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completion block will be called regardless if the Core Data operation in the closure above success or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the desired behavior - I've amended the documentation to clarify this 🙇‍♀️

}, on: .main)
Expand Down