Replace context object with `CoreDataStack` in `NotificationSettingsService` #19422

crazytonyli · 2022-10-10T09:10:47Z

It's one of the initiatives to pass CoreDataStack instead of NSManagedContext. This PR updates NotificationSettingsService to do exactly that. With this change, one usage of newDerivedContext(another thing that we'd like to move away from) is also removed.

I've tested the Weekly round up feature(My site -> Stats -> Weeks) and the notification scheduler(see "To test" section in #17766). I couldn't find any issue with this change.

But there are two changes that I can spot in the code:

The blog property in NotificationSettings instances created by NotificationSettingsService is now in mainContext, instead of a derived context. This change is caused by the BlogQuery().blogs(in: ...) line in the diff.
I don't see this change causing any issue though: if the current code is okay with using a Blog which blogs to a discarded derived context, I don't see how it can break using a Blog which blogs to the main context.
context.performAndWait is removed when accessing Blog instance properties. The original code is added in Weekly Roundup Improvements (Core Data + Localization) #17766. But I don't think we need to use context.perform to guard access to a Core Data model's properties? @frosty I know it's been a while since you created PR#17766. but do you mind reviewing this change if you have some time? Thanks.

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)
Covered in the description above.
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.

…ervice`

wpmobilebot · 2022-10-10T09:39:03Z

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 pr19422-f095331 on your iPhone

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

wpmobilebot · 2022-10-10T09:41:07Z

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 pr19422-f095331 on your iPhone

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

frosty · 2022-10-10T13:21:44Z

WordPress/Classes/Utility/BackgroundTasks/WeeklyRoundupBackgroundTask.swift


-            context.performAndWait {


I'm pretty sure it's safest to ensure that these operations accessing the object happen on the same queue as its context. This change was added because originally we were setting crashes here when accessing siteTitle. If I recall we didn't ever really get to the bottom of what was causing the issue but I think this change improved things (can't say for certain though). I'd personally be hesitant to change it. Can we just call perform on the site's context if we don't want to pass one in?

👍 I was thinking the same thing, probably safer to use site.context than the one in arguments. I'll make the change.

Addressed in bd2f325

mokagio · 2022-10-10T23:54:54Z

RELEASE-NOTES.txt

@@ -6,6 +6,7 @@
 * [**] [Jetpack-only] Added a "Save as Draft" extension. Now users can save content to Jetpack through iOS's share sheet. This was previously only available on the WordPress app. [#19414]
 * [**] [Jetpack-only] Enables Rich Notifications for the Jetpack app. Now we display more details on most of the push notifications. This was previously only available on the WordPress app. [#19415]
 * [*] Reader: Comment Details have been redesigned. [#19387]
+* [*] [internal] A refactor in weekly roundup notification scheduler. [#19422]


mokagio · 2022-10-10T23:55:53Z

WordPress/Classes/Services/NotificationSettingsService.swift

-    fileprivate var blogService: BlogService {
-        return BlogService(managedObjectContext: managedObjectContext)
-    }
-


Well done finding this unused var 👍

mokagio · 2022-10-11T00:00:13Z

WordPress/Classes/Utility/PushNotificationsManager.swift

@@ -101,7 +101,7 @@ final public class PushNotificationsManager: NSObject {
        deviceToken = newToken

        // Register against WordPress.com
-        let noteService = NotificationSettingsService(managedObjectContext: ContextManager.sharedInstance().mainContext)
+        let noteService = NotificationSettingsService(coreDataStack: ContextManager.sharedInstance())


Did you consider adding an instance property to store ContextManager.sharedInstance()?

I don't think it would make much difference at runtime, given it would still access the shared instance. It might be useful in the future when/if we'll remove all the shared instances accesses in favor of Dependency Injection.

🤷‍♂️

I don't want to think too far ahead, even though we know DI is going to happen at some point, but we don't know how it's going to look like, which can affect how the code should be written.

Fair enough. 👌

mokagio · 2022-10-11T00:01:13Z

WordPress/Classes/Utility/BackgroundTasks/WeeklyRoundupBackgroundTask.swift

-            }
+        guard let dotComID = dotComID else {
+            // Error
+            return


Would it be useful to call the completion and pass an error about this to it?

I don't think this "else case" can actually happen, not in the current code at least. The non-WP.com sites have been filtered out beforehand in

WordPress-iOS/WordPress/Classes/Utility/BackgroundTasks/WeeklyRoundupBackgroundTask.swift

Lines 82 to 85 in e3c3e41

guard let dotComID = site.dotComID?.intValue else {

onError(DataRequestError.dotComSiteWithoutDotComID(site))

continue

}

Gotcha.

🤔 😞 It's disappointing the domain modeling evolved to leave the door open for this inconsistent state path, but I'd say tackling it is out scope for this PR.

mokagio · 2022-10-11T02:42:57Z

WordPress/Classes/Utility/BackgroundTasks/WeeklyRoundupBackgroundTask.swift

+            // Error
+            return


Given this code should be impossible to reach, what do you think of leaving a note about it?

Suggested change

// Error

return

// This code path should be impossible to reach.

// At the time of writing, there is code to filter non-WordPress.com sites before calling this method.

return

Alternatively, would this count as "developer error"?

Suggested change

// Error

return

fatalError("Inconsistent State: Non-WordPress.com sites should have been filtered before calling this method.")

🤔 this is how I see the comment I just posted 😓

If that's how you see it, too, you should be able to see my suggestions by looking at the comment source via the edit button. But I hope that's just a temporary hiccup.

The suggestion blocks look normal on my side 😸

I've added a fatalError in f095331

Replace context object with CoreDataStack in `NotificationSettingsS…

cdb9750

…ervice`

crazytonyli added the Core Data Issues related to Core Data label Oct 10, 2022

crazytonyli added this to the 21.1 milestone Oct 10, 2022

crazytonyli requested review from frosty and mokagio October 10, 2022 09:10

crazytonyli self-assigned this Oct 10, 2022

Add an internal note to the release notes

6f106f6

crazytonyli modified the milestones: 21.1, 21.0 Oct 10, 2022

frosty reviewed Oct 10, 2022

View reviewed changes

Use Blog instance's context to access its properties

bd2f325

crazytonyli requested a review from frosty October 10, 2022 20:54

mokagio reviewed Oct 10, 2022

View reviewed changes

mokagio approved these changes Oct 11, 2022

View reviewed changes

mokagio reviewed Oct 11, 2022

View reviewed changes

mokagio approved these changes Oct 11, 2022

View reviewed changes

Add an assertion that the site argument must be a WP.com site

f095331

crazytonyli enabled auto-merge October 11, 2022 04:11

crazytonyli merged commit 4f3a670 into trunk Oct 11, 2022

crazytonyli deleted the core-data-stack-in-notification-settings-service branch October 11, 2022 04:43

crazytonyli mentioned this pull request Oct 11, 2022

Remove newDerivedContext in ReaderCardService #19427

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace context object with `CoreDataStack` in `NotificationSettingsService` #19422

Replace context object with `CoreDataStack` in `NotificationSettingsService` #19422

crazytonyli commented Oct 10, 2022 •

edited

Loading

wpmobilebot commented Oct 10, 2022 •

edited

Loading

wpmobilebot commented Oct 10, 2022 •

edited

Loading

frosty Oct 10, 2022

crazytonyli Oct 10, 2022

crazytonyli Oct 10, 2022

mokagio Oct 10, 2022

mokagio Oct 10, 2022

mokagio Oct 11, 2022

crazytonyli Oct 11, 2022

mokagio Oct 11, 2022

mokagio Oct 11, 2022

crazytonyli Oct 11, 2022

mokagio Oct 11, 2022

mokagio Oct 11, 2022

mokagio Oct 11, 2022

crazytonyli Oct 11, 2022

	guard let dotComID = site.dotComID?.intValue else {
	onError(DataRequestError.dotComSiteWithoutDotComID(site))
	continue
	}

-            // Error
-            return
+            // This code path should be impossible to reach.
+            // At the time of writing, there is code to filter non-WordPress.com sites before calling this method.
+            return

	// Error
	return
	fatalError("Inconsistent State: Non-WordPress.com sites should have been filtered before calling this method.")

Replace context object with CoreDataStack in NotificationSettingsService #19422

Replace context object with CoreDataStack in NotificationSettingsService #19422

Conversation

crazytonyli commented Oct 10, 2022 • edited Loading

Regression Notes

wpmobilebot commented Oct 10, 2022 • edited Loading

wpmobilebot commented Oct 10, 2022 • edited Loading

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

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

Replace context object with `CoreDataStack` in `NotificationSettingsService` #19422

Replace context object with `CoreDataStack` in `NotificationSettingsService` #19422

crazytonyli commented Oct 10, 2022 •

edited

Loading

wpmobilebot commented Oct 10, 2022 •

edited

Loading

wpmobilebot commented Oct 10, 2022 •

edited

Loading