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

Update Blog.dotComID value if needed before saving #23884

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Update Blog.dotComID value if needed before saving #23884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
707cd08
Add an unit test to reproduce an issue with querying blog by ID
crazytonyli Dec 11, 2024
934da1c
Update Blog.dotComID value if needed before saving
crazytonyli Dec 11, 2024
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
11 changes: 11 additions & 0 deletions WordPress/Classes/Models/Blog/Blog.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,17 @@ @implementation Blog

#pragma mark - NSManagedObject subclass methods

- (void)willSave {
[super willSave];

// The `dotComID` getter has a speicial code to _update_ `blogID` value.
// This is a weird patch to make sure `blogID` is set to a correct value.
//
// It's important that calling `[self dotComID]` repeatedly only updates
// `Blog` instance once, which is the case at the moment.
[self dotComID];
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a good place to do it: "This method can have “side effects” on persistent values. You can use it to, for example, compute persistent values from other transient or scratchpad values." 👍

Having said that, this rename to dotComID has been a real PITA. It's be nice to do a migration and remove the original field. I plan to wrap up this #23788 this week and add more stuff that can be removed form the database. And there are a ton of existing deprecations, especially in Blog.

}

- (void)prepareForDeletion
{
[super prepareForDeletion];
Expand Down
11 changes: 7 additions & 4 deletions WordPress/WordPressTest/BlogBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@ final class BlogBuilder {

private let context: NSManagedObjectContext

init(_ context: NSManagedObjectContext) {
init(_ context: NSManagedObjectContext, dotComID: NSNumber? = NSNumber(value: arc4random_uniform(999_999))) {
self.context = context

blog = NSEntityDescription.insertNewObject(forEntityName: Blog.entityName(), into: context) as! Blog

if let dotComID {
blog.dotComID = dotComID
}

// Non-null properties in Core Data
blog.dotComID = NSNumber(value: arc4random_uniform(999_999))
blog.url = "https://\(blog.dotComID!).example.com"
blog.xmlrpc = "https://\(blog.dotComID!).example.com/xmlrpc.php"
blog.url = "https://\(blog.dotComID?.stringValue ?? "wwww").example.com"
blog.xmlrpc = "\(blog.url!)/xmlrpc.php"
}

func with(atomic: Bool) -> Self {
Expand Down
16 changes: 16 additions & 0 deletions WordPress/WordPressTest/BlogTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,20 @@ final class BlogTests: CoreDataTestCase {

XCTAssertEqual(try blog.wordPressClientParsedUrl().url(), "http://example.com/")
}

func testDotComIdShouldBeJetpackSiteID() throws {
let blog = BlogBuilder(mainContext, dotComID: nil)
.set(blogOption: "jetpack_client_id", value: "123")
.build()
XCTAssertEqual(blog.jetpack?.siteID?.int64Value, 123)

try XCTAssertNil(Blog.lookup(withID: 123, in: mainContext))
try mainContext.save()

try XCTAssertNotNil(Blog.lookup(withID: 123, in: mainContext))

contextManager.performAndSave { context in
try? XCTAssertNotNil(Blog.lookup(withID: 123, in: context))
}
}
}
Loading