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

Replace context object with CoreDataStack in NotificationSettingsService #19422

Merged
merged 4 commits into from
Oct 11, 2022
Merged

Replace context object with CoreDataStack in NotificationSettingsService #19422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdb9750
Replace context object with `CoreDataStack` in `NotificationSettingsS…
crazytonyli Oct 10, 2022
6f106f6
Add an internal note to the release notes
crazytonyli Oct 10, 2022
bd2f325
Use `Blog` instance's context to access its properties
crazytonyli Oct 10, 2022
f095331
Add an assertion that the site argument must be a WP.com site
crazytonyli Oct 11, 2022
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
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


20.9
-----
Expand Down
27 changes: 13 additions & 14 deletions WordPress/Classes/Services/NotificationSettingsService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import WordPressKit

/// This service encapsulates the Restful API related to WordPress Notifications.
///
open class NotificationSettingsService: LocalCoreDataService {
class NotificationSettingsService {
// MARK: - Aliases
public typealias Channel = NotificationSettings.Channel
public typealias Stream = NotificationSettings.Stream
Expand All @@ -14,14 +14,17 @@ open class NotificationSettingsService: LocalCoreDataService {
///
/// - Parameter managedObjectContext: A Reference to the MOC that should be used to interact with the Core Data Stack.
///
public override init(managedObjectContext context: NSManagedObjectContext) {
super.init(managedObjectContext: context)
public convenience init(coreDataStack: CoreDataStack) {
var remoteApi: WordPressComRestApi? = nil

if let defaultAccount = try? WPAccount.lookupDefaultWordPressComAccount(in: context),
if let defaultAccount = try? WPAccount.lookupDefaultWordPressComAccount(in: coreDataStack.mainContext),
defaultAccount.authToken != nil,
let restApi = defaultAccount.wordPressComRestApi {
remoteApi = restApi.hasCredentials() ? restApi : nil
let restApi = defaultAccount.wordPressComRestApi,
restApi.hasCredentials() {
remoteApi = restApi
}

self.init(coreDataStack: coreDataStack, wordPressComRestApi: remoteApi)
}

/// Convenience Initializer. Useful for Unit Testing
Expand All @@ -30,8 +33,8 @@ open class NotificationSettingsService: LocalCoreDataService {
/// - managedObjectContext: A Reference to the MOC that should be used to interact with the Core Data Stack.
/// - wordPressComRestApi: The WordPressComRestApi that should be used.
///
@objc public convenience init(managedObjectContext context: NSManagedObjectContext, wordPressComRestApi: WordPressComRestApi) {
self.init(managedObjectContext: context)
public init(coreDataStack: CoreDataStack, wordPressComRestApi: WordPressComRestApi?) {
self.coreDataStack = coreDataStack
self.remoteApi = wordPressComRestApi
}

Expand Down Expand Up @@ -177,7 +180,7 @@ open class NotificationSettingsService: LocalCoreDataService {
///
fileprivate func settingsFromRemote(_ remoteSettings: [RemoteNotificationSettings]) -> [NotificationSettings] {
var parsed = [NotificationSettings]()
let blogs = ((try? BlogQuery().blogs(in: managedObjectContext)) ?? []).filter { $0.dotComID != nil }
let blogs = ((try? BlogQuery().blogs(in: coreDataStack.mainContext)) ?? []).filter { $0.dotComID != nil }
let blogMap = Dictionary(blogs.map { ($0.dotComID!.intValue, $0) }, uniquingKeysWith: { _, new in new })

for remoteSetting in remoteSettings {
Expand Down Expand Up @@ -305,7 +308,7 @@ open class NotificationSettingsService: LocalCoreDataService {

// MARK: - Private Properties
fileprivate var remoteApi: WordPressComRestApi?

private let coreDataStack: CoreDataStack

// MARK: - Private Computed Properties
fileprivate var notificationsServiceRemote: NotificationSettingsServiceRemote? {
Expand All @@ -316,10 +319,6 @@ open class NotificationSettingsService: LocalCoreDataService {
return NotificationSettingsServiceRemote(wordPressComRestApi: remoteApi)
}

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

Comment on lines -319 to -322
Copy link
Contributor

Choose a reason for hiding this comment

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

Well done finding this unused var 👍

fileprivate var deviceId: String {
return PushNotificationsManager.shared.deviceId ?? String()
}
Expand Down
47 changes: 22 additions & 25 deletions WordPress/Classes/Utility/BackgroundTasks/WeeklyRoundupBackgroundTask.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import CoreData

/// The main data provider for Weekly Roundup information.
///
class WeeklyRoundupDataProvider {
private class WeeklyRoundupDataProvider {

// MARK: - Definitions

Expand All @@ -19,7 +19,7 @@ class WeeklyRoundupDataProvider {

// MARK: - Misc Properties

private let context: NSManagedObjectContext
private let coreDataStack: CoreDataStack

/// Method to report errors that won't interrupt the execution.
///
Expand All @@ -29,8 +29,8 @@ class WeeklyRoundupDataProvider {
///
private let debugSettings = WeeklyRoundupDebugScreen.Settings()

init(context: NSManagedObjectContext, onError: @escaping (Error) -> Void) {
self.context = context
init(coreDataStack: CoreDataStack, onError: @escaping (Error) -> Void) {
self.coreDataStack = coreDataStack
self.onError = onError
}

Expand All @@ -55,7 +55,7 @@ class WeeklyRoundupDataProvider {
}
}

func getTopSiteStats(from sites: [Blog], completion: @escaping (Result<SiteStats?, Error>) -> Void) {
private func getTopSiteStats(from sites: [Blog], completion: @escaping (Result<SiteStats?, Error>) -> Void) {
var endDateComponents = DateComponents()
endDateComponents.weekday = 1

Expand Down Expand Up @@ -164,7 +164,7 @@ class WeeklyRoundupDataProvider {
/// Filters the sites that have the Weekly Roundup notification setting enabled.
///
private func filterWeeklyRoundupEnabledSites(_ sites: [Blog], result: @escaping (Result<[Blog], Error>) -> Void) {
let noteService = NotificationSettingsService(managedObjectContext: context)
let noteService = NotificationSettingsService(coreDataStack: coreDataStack)

noteService.getAllSettings { settings in
let weeklyRoundupEnabledSites = sites.filter { site in
Expand Down Expand Up @@ -193,7 +193,7 @@ class WeeklyRoundupDataProvider {
]

do {
let result = try context.fetch(request)
let result = try coreDataStack.mainContext.fetch(request)
return .success(result)
} catch {
return .failure(DataRequestError.siteFetchingError(error))
Expand Down Expand Up @@ -382,8 +382,7 @@ class WeeklyRoundupBackgroundTask: BackgroundTask {
}
}

let context = ContextManager.shared.newDerivedContext()
let dataProvider = WeeklyRoundupDataProvider(context: context, onError: onError)
let dataProvider = WeeklyRoundupDataProvider(coreDataStack: ContextManager.shared, onError: onError)
var siteStats: [Blog: StatsSummaryData]? = nil

let requestData = BlockOperation {
Expand Down Expand Up @@ -428,8 +427,8 @@ class WeeklyRoundupBackgroundTask: BackgroundTask {
views: stats.viewsCount,
comments: stats.commentsCount,
likes: stats.likesCount,
periodEndDate: self.currentRunPeriodEndDate(),
context: context) { result in
periodEndDate: self.currentRunPeriodEndDate()
) { result in

switch result {
case .success:
Expand Down Expand Up @@ -538,21 +537,19 @@ class WeeklyRoundupNotificationScheduler {
comments: Int,
likes: Int,
periodEndDate: Date,
context: NSManagedObjectContext,
completion: @escaping (Result<Void, Error>) -> Void) {

var siteTitle: String?
var dotComID: Int?

context.performAndWait {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in bd2f325

dotComID = site.dotComID?.intValue
siteTitle = site.title
}
completion: @escaping (Result<Void, Error>) -> Void
) {
var siteTitle: String?
var dotComID: Int?

site.managedObjectContext?.performAndWait {
siteTitle = site.title
dotComID = site.dotComID?.intValue
}

guard let dotComID = dotComID else {
// Error
return
}
guard let dotComID = dotComID else {
fatalError("The argument site is not a WordPress.com site. Site: \(site)")
}

let title = notificationTitle(siteTitle)
let body = notificationBodyWith(views: views, comments: likes, likes: comments)
Expand Down
4 changes: 2 additions & 2 deletions WordPress/Classes/Utility/PushNotificationsManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. 👌


noteService.registerDeviceForPushNotifications(newToken, success: { deviceId in
DDLogVerbose("Successfully registered Device ID \(deviceId) for Push Notifications")
Expand Down Expand Up @@ -138,7 +138,7 @@ final public class PushNotificationsManager: NSObject {

ZendeskUtils.unregisterDevice()

let noteService = NotificationSettingsService(managedObjectContext: ContextManager.sharedInstance().mainContext)
let noteService = NotificationSettingsService(coreDataStack: ContextManager.sharedInstance())

noteService.unregisterDeviceForPushNotifications(knownDeviceId, success: {
DDLogInfo("Successfully unregistered Device ID \(knownDeviceId) for Push Notifications!")
Expand Down
3 changes: 1 addition & 2 deletions ...sses/ViewRelated/Notifications/Controllers/NotificationSettingDetailsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,7 @@ class NotificationSettingDetailsViewController: UITableViewController {
return
}

let context = ContextManager.sharedInstance().mainContext
let service = NotificationSettingsService(managedObjectContext: context)
let service = NotificationSettingsService(coreDataStack: ContextManager.shared)

service.updateSettings(settings!,
stream: stream!,
Expand Down
2 changes: 1 addition & 1 deletion ...ss/Classes/ViewRelated/Notifications/Controllers/NotificationSettingsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class NotificationSettingsViewController: UIViewController {
// MARK: - Service Helpers

fileprivate func reloadSettings() {
let service = NotificationSettingsService(managedObjectContext: ContextManager.sharedInstance().mainContext)
let service = NotificationSettingsService(coreDataStack: ContextManager.sharedInstance())

let dispatchGroup = DispatchGroup()
let siteService = ReaderTopicService(managedObjectContext: ContextManager.sharedInstance().mainContext)
Expand Down
5 changes: 2 additions & 3 deletions WordPress/WordPressTest/NotificationSettingsServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import Foundation
import XCTest
import WordPress
import OHHTTPStubs

@testable import WordPress

// MARK: - NotificationSettingsServiceTests
//
Expand All @@ -26,8 +26,7 @@ class NotificationSettingsServiceTests: CoreDataTestCase {
super.setUp()

remoteApi = WordPressComRestApi(oAuthToken: nil, userAgent: nil)
service = NotificationSettingsService(managedObjectContext: contextManager.mainContext,
wordPressComRestApi: remoteApi)
service = NotificationSettingsService(coreDataStack: contextManager, wordPressComRestApi: remoteApi)

stub(condition: { request in
return request.url?.absoluteString.range(of: self.settingsEndpoint) != nil
Expand Down