Remove context from a few service types #20010

crazytonyli · 2023-01-26T21:03:43Z

Use CoreDataStack instead of NSManagedObjectContext in a few service types. See the "Issue" and "Proposal" sections of #19893 for rational behind this change.

Regression Notes

Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)
None
What automated tests I added (or what prevented me from doing so)
None

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-01-26T21:44:17Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr20010-2b5f7d0 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2023-01-26T21:48:34Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr20010-2b5f7d0 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

crazytonyli · 2023-01-27T08:35:07Z

WordPress/WordPressTest/SiteManagementServiceTests.swift

@@ -115,8 +111,7 @@ class SiteManagementServiceTests: CoreDataTestCase {
        mockRemoteService.successBlockPassedIn?()
        waitForExpectations(timeout: 2, handler: nil)

-        let shouldBeRemoved = try? context.existingObject(with: blogObjectID)
-        XCTAssertFalse(shouldBeRemoved != nil, "Blog was not removed")
+        try XCTAssertEqual(context.count(for: Blog.fetchRequest()), 0)


Changed this assertion here because the blog will exist in the context until context.save() is called. Using NSFetchRequest is a more accurate way to check if the object is deleted from the database.

Sounds good 👍

Do you know whether there are more of this kind of tests that we should consider upgrading (in a dedicated all-in-one PR)?

I'm not sure, this is the first time I ran into this kind of issue, maybe I'll find more? I can only notice them if I happen to look at the test case or the test case fails during development. 😄

mokagio

Only nitpicks 👍

WordPress/Classes/Models/Role.swift

mokagio · 2023-01-30T05:37:03Z

WordPress/Classes/Services/ReaderSearchSuggestionService.swift

-            suggestion = NSEntityDescription.insertNewObject(forEntityName: ReaderSearchSuggestion.classNameWithoutNamespaces(),
-                                                                             into: managedObjectContext) as? ReaderSearchSuggestion
-            suggestion?.searchPhrase = phrase
+        self.coreDataStack.performAndSave { context in


Nitpick: Is it necessary to access coreDataStack from self here?

Suggested change

self.coreDataStack.performAndSave { context in

coreDataStack.performAndSave { context in

mokagio · 2023-01-30T05:47:32Z

WordPress/Classes/Services/ReaderSearchSuggestionService.swift

@@ -30,71 +40,35 @@ import CocoaLumberjack
    ///
    /// - Returns: A matching search phrase or nil.
    ///
-    @objc func findSuggestionForPhrase(_ phrase: String) -> ReaderSearchSuggestion? {
+    private func findSuggestionForPhrase(_ phrase: String, in context: NSManagedObjectContext) -> ReaderSearchSuggestion? {


Nitpick / Out of scope convention question:

This method use findSuggestionForPhrase(_ phrase: String, ... a similar method in a different file from this diff use lookup(withBlogID blogID: String, .... The style of the two is inconsistent.

I personally prefer the latter, but I think the Swift API Design Guidelines would recommend using the former, as the argument is part of a prepositional phrase.

"For" and "with" are both prepositions, ChatGPT told me so:

To be honest, I feel like breaking from the guidelines in this case. I find more value in not having to think whether to name the first parameter or not vs. using what might be better English grammar in the API names.

What do you think?

I think the Swift API Design Guidelines would recommend using the former

If I understand it correctly, using the former is a case of exception where there are two or more arguments? i.e. findSuggestionFor(phrase: "hello", author: "Tony")

The signature findSuggestion(forPhrase: "hello", in: context) looks pretty nature to me, so I've changed it in 754dae1

mokagio · 2023-01-30T05:51:35Z

WordPress/Classes/Services/ReaderSearchSuggestionService.swift

+            var suggestions = [ReaderSearchSuggestion]()
+            do {
+                suggestions = try context.fetch(fetchRequest) as! [ReaderSearchSuggestion]
+            } catch let error as NSError {
+                DDLogError("Error fetching search suggestion : \(error.localizedDescription)")
+            }
+            for suggestion in suggestions {
+                context.delete(suggestion)
+            }


Nitpick: Have you considered moving the for loop inside the do catch to avoid declaring a var?

Suggested change

var suggestions = [ReaderSearchSuggestion]()

do {

suggestions = try context.fetch(fetchRequest) as! [ReaderSearchSuggestion]

} catch let error as NSError {

DDLogError("Error fetching search suggestion : \(error.localizedDescription)")

}

for suggestion in suggestions {

context.delete(suggestion)

}

do {

try (context.fetch(fetchRequest) as! [ReaderSearchSuggestion]).forEach { context.delete($0) }

} catch let error as NSError {

DDLogError("Error fetching search suggestion : \(error.localizedDescription)")

}

👍 Addressed in 2b5f7d0

mokagio · 2023-01-30T05:52:59Z

WordPress/Classes/ViewRelated/People/PeopleViewController.swift

-        roleService.fetchRoles(success: {_ in
+        roleService.fetchRoles(success: {


mokagio · 2023-01-30T05:58:54Z

WordPress/Classes/ViewRelated/People/PeopleViewController.swift

-        guard let blog = blog,
-            let service = RoleService(blog: blog, context: viewContext) else {
-                return nil
+        guard let blog = blog else {


I was going to suggest guard let blog else but I got annoyed at myself for being so pedantic about things that could be automated...

SwiftLint supports this, I should add the check to our https://github.com/Automattic/swiftlint-config and integrate it here...

No worries. It's always good to have cleaner code. Updated in 4412706

mokagio · 2023-01-30T05:59:54Z

WordPress/WordPressTest/SiteManagementServiceTests.swift

@@ -115,8 +111,7 @@ class SiteManagementServiceTests: CoreDataTestCase {
        mockRemoteService.successBlockPassedIn?()
        waitForExpectations(timeout: 2, handler: nil)

-        let shouldBeRemoved = try? context.existingObject(with: blogObjectID)
-        XCTAssertFalse(shouldBeRemoved != nil, "Blog was not removed")
+        try XCTAssertEqual(context.count(for: Blog.fetchRequest()), 0)


Sounds good 👍

Do you know whether there are more of this kind of tests that we should consider upgrading (in a dedicated all-in-one PR)?

Co-authored-by: Gio Lodi <[email protected]>

crazytonyli added 3 commits January 26, 2023 21:30

Use CoreDataStack in RoleService

a10ecbc

Use CoreDataStack in ReaderSearchSuggestionService

8b2b3d9

Use CoreDataStack in site creation services

4f9dc21

Use CoreDataStack in SiteManagementService

e667bcb

crazytonyli force-pushed the remove-context-from-a-few-service-types branch from d2f1d2a to e667bcb Compare January 27, 2023 08:02

crazytonyli requested review from jkmassel and mokagio January 27, 2023 08:08

crazytonyli self-assigned this Jan 27, 2023

crazytonyli added the Core Data Issues related to Core Data label Jan 27, 2023

crazytonyli added this to the 21.7 milestone Jan 27, 2023

crazytonyli marked this pull request as ready for review January 27, 2023 08:10

crazytonyli commented Jan 27, 2023

View reviewed changes

mokagio approved these changes Jan 30, 2023

View reviewed changes

crazytonyli and others added 4 commits January 31, 2023 10:30

Delete unnecessary API doc

6e7775c

Co-authored-by: Gio Lodi <[email protected]>

Change a function signature

754dae1

Use modern if let

4412706

Remove an unnecessary local var

2b5f7d0

crazytonyli enabled auto-merge January 30, 2023 21:50

crazytonyli merged commit f164645 into trunk Jan 30, 2023

crazytonyli deleted the remove-context-from-a-few-service-types branch January 30, 2023 22:39

momo-ozawa mentioned this pull request Feb 9, 2023

Crash when tapping on People menu item #20112

Closed

crazytonyli mentioned this pull request Feb 9, 2023

Query the blog instance before using it to update a Role #20114

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove context from a few service types #20010

Remove context from a few service types #20010

crazytonyli commented Jan 26, 2023 •

edited

Loading

wpmobilebot commented Jan 26, 2023 •

edited

Loading

wpmobilebot commented Jan 26, 2023 •

edited

Loading

crazytonyli Jan 27, 2023

mokagio Jan 30, 2023

crazytonyli Jan 30, 2023

mokagio left a comment

mokagio Jan 30, 2023

mokagio Jan 30, 2023

crazytonyli Jan 30, 2023

mokagio Jan 30, 2023

crazytonyli Jan 30, 2023

mokagio Jan 30, 2023

mokagio Jan 30, 2023

crazytonyli Jan 30, 2023

mokagio Jan 30, 2023

	self.coreDataStack.performAndSave { context in
	coreDataStack.performAndSave { context in

		roleService.fetchRoles(success: {_ in
		roleService.fetchRoles(success: {

Remove context from a few service types #20010

Remove context from a few service types #20010

Conversation

crazytonyli commented Jan 26, 2023 • edited Loading

Regression Notes

wpmobilebot commented Jan 26, 2023 • edited Loading

wpmobilebot commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli commented Jan 26, 2023 •

edited

Loading

wpmobilebot commented Jan 26, 2023 •

edited

Loading

wpmobilebot commented Jan 26, 2023 •

edited

Loading