From 1ba070ce665293062a0313ad6fc8187f1a212516 Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Tue, 13 Jun 2023 23:54:14 +0700 Subject: [PATCH 1/7] Execute Core Data code in the correct context after getting notification settings UNNotificationCenter's `getNotificationSettings` method does not guarantee that the completion block is called in the original queue. This ensures that the completion block code that touches Core Data is run within the context's queue, as obtained from the managed object. --- .../Blogging Prompts/BloggingPromptCoordinator.swift | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Blog/Blogging Prompts/BloggingPromptCoordinator.swift b/WordPress/Classes/ViewRelated/Blog/Blogging Prompts/BloggingPromptCoordinator.swift index 0e9e87cfd18d..38deb61add14 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blogging Prompts/BloggingPromptCoordinator.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blogging Prompts/BloggingPromptCoordinator.swift @@ -112,6 +112,7 @@ private extension BloggingPromptCoordinator { guard let service = promptsServiceFactory.makeService(for: blog), let settings = service.localSettings, let reminderDays = settings.reminderDays, + let context = settings.managedObjectContext, settings.promptRemindersEnabled else { completion() return @@ -125,10 +126,12 @@ private extension BloggingPromptCoordinator { return } - // Reschedule the prompt reminders. - let schedule = BloggingRemindersScheduler.Schedule.weekdays(reminderDays.getActiveWeekdays()) - self.scheduler.schedule(schedule, for: blog, time: settings.reminderTimeDate()) { result in - completion() + context.performAndWait { + // Reschedule the prompt reminders. + let schedule = BloggingRemindersScheduler.Schedule.weekdays(reminderDays.getActiveWeekdays()) + self.scheduler.schedule(schedule, for: blog, time: settings.reminderTimeDate()) { result in + completion() + } } } } From 7dc0f5cf0779b06184ff084afbe8a5d0ec92c889 Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Tue, 13 Jun 2023 23:57:24 +0700 Subject: [PATCH 2/7] Execute Core Data code in the right queue after push authorization --- .../PromptRemindersScheduler.swift | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift index 45b94b6d624e..f33de1d7ed27 100644 --- a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift +++ b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift @@ -43,9 +43,15 @@ class PromptRemindersScheduler { /// - time: The user's preferred time to be notified. /// - completion: Closure called after the process completes. func schedule(_ schedule: BloggingRemindersScheduler.Schedule, for blog: Blog, time: Date? = nil, completion: @escaping (Result) -> Void) { + guard let context = blog.managedObjectContext else { + return + } + guard schedule != .none else { - // If there's no schedule, then we don't need to request authorization - processSchedule(schedule, blog: blog, time: time, completion: completion) + context.performAndWait { + // If there's no schedule, then we don't need to request authorization + processSchedule(schedule, blog: blog, time: time, completion: completion) + } return } @@ -59,7 +65,9 @@ class PromptRemindersScheduler { return } - self.processSchedule(schedule, blog: blog, time: time, completion: completion) + context.performAndWait { + self.processSchedule(schedule, blog: blog, time: time, completion: completion) + } } } From c5cf208ea8dde34b0666338efd31e12d9614d23a Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Wed, 14 Jun 2023 00:37:36 +0700 Subject: [PATCH 3/7] Improve thread safety of BloggingPromptsService init method --- .../Services/BloggingPromptsService.swift | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Services/BloggingPromptsService.swift b/WordPress/Classes/Services/BloggingPromptsService.swift index bd68c13a6df3..c7abaa7bd955 100644 --- a/WordPress/Classes/Services/BloggingPromptsService.swift +++ b/WordPress/Classes/Services/BloggingPromptsService.swift @@ -179,17 +179,45 @@ class BloggingPromptsService { // MARK: - Init + /// Initializes a service for blogging prompts. + /// + /// - Parameters: + /// - contextManager: The CoreDataStack instance. + /// - remote: When supplied, the service will use the specified remote service. + /// Otherwise, a remote service with the default account's credentials will be used. + /// - blog: When supplied, the service will perform blogging prompts requests for this specified blog. + /// Otherwise, this falls back to the default account's primary blog. required init?(contextManager: CoreDataStackSwift = ContextManager.shared, remote: BloggingPromptsServiceRemote? = nil, blog: Blog? = nil) { - guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: contextManager.mainContext), - let siteID = blog?.dotComID ?? account.primaryBlogID else { + var remoteInstance = remote + var siteID: NSNumber? = nil + + // if a blog exists, then try to use the blog's ID. + if let blog, + let blogContext = blog.managedObjectContext { + blogContext.performAndWait { + siteID = blog.dotComID + } + } + + // fetch the default account and fall back to default values as needed. + contextManager.performQuery { mainContext in + guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: mainContext) else { + return + } + siteID = siteID ?? account.primaryBlogID + remoteInstance = remoteInstance ?? .init(wordPressComRestApi: account.wordPressComRestV2Api) + } + + guard let siteID, + let remoteInstance else { return nil } self.contextManager = contextManager self.siteID = siteID - self.remote = remote ?? .init(wordPressComRestApi: account.wordPressComRestV2Api) + self.remote = remoteInstance } } From 6f89aa7fd9c2d541f46cea7e486d36774bf236d7 Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Wed, 14 Jun 2023 16:38:16 +0700 Subject: [PATCH 4/7] Remove direct access to Blog after fetching in processSchedule Refactor `addLocalNotification` and `addStaticNotifications` to operate with value types instead of dealing with `Blog` directly. There are still loose property reads to `Blog` within the method, but `processSchedule` is private and its callers are guaranteeing that the method is called from the Blog's context queue. Further improvements will be made in the next iteration. --- .../PromptRemindersScheduler.swift | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift index f33de1d7ed27..4a383100951c 100644 --- a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift +++ b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift @@ -157,6 +157,7 @@ private extension PromptRemindersScheduler { return } + let title = blog.title let reminderTime = Time(from: time) ?? Constants.defaultTime let currentDate = currentDateProvider.date() promptsService.fetchPrompts(from: currentDate, number: Constants.promptsToFetch) { [weak self] prompts in @@ -185,7 +186,7 @@ private extension PromptRemindersScheduler { var lastScheduledPrompt: BloggingPrompt? = nil var notificationIds = [String]() promptsToSchedule.forEach { prompt in - guard let identifier = self.addLocalNotification(for: prompt, blog: blog, at: reminderTime) else { + guard let identifier = self.addLocalNotification(for: prompt, siteID: siteID, siteTitle: title, at: reminderTime) else { return } notificationIds.append(identifier) @@ -205,7 +206,10 @@ private extension PromptRemindersScheduler { return lastReminderDate }() - if let staticNotificationIds = self.addStaticNotifications(after: lastReminderDate, with: schedule, time: reminderTime, blog: blog) { + if let staticNotificationIds = self.addStaticNotifications(after: lastReminderDate, + with: schedule, + time: reminderTime, + siteID: siteID) { notificationIds.append(contentsOf: staticNotificationIds) } @@ -232,17 +236,13 @@ private extension PromptRemindersScheduler { /// - blog: The user's blog. /// - time: The preferred reminder time for the notification. /// - Returns: String representing the notification identifier. - func addLocalNotification(for prompt: BloggingPrompt, blog: Blog, at time: Time) -> String? { - guard blog.dotComID != nil else { - return nil - } - + func addLocalNotification(for prompt: BloggingPrompt, siteID: Int, siteTitle: String? = nil, at time: Time) -> String? { let content = UNMutableNotificationContent() content.title = Constants.notificationTitle - content.subtitle = blog.title ?? String() + content.subtitle = siteTitle ?? String() content.body = prompt.text content.categoryIdentifier = InteractiveNotificationsManager.NoteCategoryDefinition.bloggingPrompt.rawValue - content.userInfo = notificationPayload(for: blog, prompt: prompt) + content.userInfo = notificationPayload(for: siteID, prompt: prompt) guard let reminderDateComponents = reminderDateComponents(for: prompt, at: time) else { return nil @@ -290,12 +290,11 @@ private extension PromptRemindersScheduler { func addStaticNotifications(after afterDate: Date, with schedule: Schedule, time: Time, - blog: Blog, + siteID: Int, maxDays: Int = Constants.staticNotificationMaxDays) -> [String]? { guard case .weekdays(let weekdays) = schedule, maxDays > 0, - let maxDate = Calendar.current.date(byAdding: .day, value: maxDays, to: afterDate), - blog.dotComID != nil else { + let maxDate = Calendar.current.date(byAdding: .day, value: maxDays, to: afterDate) else { return nil } @@ -305,7 +304,7 @@ private extension PromptRemindersScheduler { content.title = Constants.notificationTitle content.body = Constants.staticNotificationContent content.categoryIdentifier = InteractiveNotificationsManager.NoteCategoryDefinition.bloggingPrompt.rawValue - content.userInfo = notificationPayload(for: blog) + content.userInfo = notificationPayload(for: siteID) var date = afterDate var identifiers = [String]() @@ -366,11 +365,7 @@ private extension PromptRemindersScheduler { return identifier } - func notificationPayload(for blog: Blog, prompt: BloggingPrompt? = nil) -> [AnyHashable: Any] { - guard let siteID = blog.dotComID?.intValue else { - return [:] - } - + func notificationPayload(for siteID: Int, prompt: BloggingPrompt? = nil) -> [AnyHashable: Any] { var userInfo: [AnyHashable: Any] = [BloggingPrompt.NotificationKeys.siteID: siteID] if let prompt = prompt { From efdad6e4a7db74ac3503246eae8cab9f8f14f332 Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Wed, 14 Jun 2023 16:49:51 +0700 Subject: [PATCH 5/7] Simplify the init method by utilizing the return value from performQuery --- .../Services/BloggingPromptsService.swift | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/WordPress/Classes/Services/BloggingPromptsService.swift b/WordPress/Classes/Services/BloggingPromptsService.swift index c7abaa7bd955..cb37cae0487c 100644 --- a/WordPress/Classes/Services/BloggingPromptsService.swift +++ b/WordPress/Classes/Services/BloggingPromptsService.swift @@ -190,24 +190,23 @@ class BloggingPromptsService { required init?(contextManager: CoreDataStackSwift = ContextManager.shared, remote: BloggingPromptsServiceRemote? = nil, blog: Blog? = nil) { - var remoteInstance = remote - var siteID: NSNumber? = nil - - // if a blog exists, then try to use the blog's ID. - if let blog, - let blogContext = blog.managedObjectContext { - blogContext.performAndWait { - siteID = blog.dotComID + let blogObjectID = blog?.objectID + let (siteID, remoteInstance) = contextManager.performQuery { mainContext in + // if a blog exists, then try to use the blog's ID. + var blogInContext: Blog? = nil + if let blogObjectID { + blogInContext = (try? mainContext.existingObject(with: blogObjectID)) as? Blog } - } - // fetch the default account and fall back to default values as needed. - contextManager.performQuery { mainContext in + // fetch the default account and fall back to default values as needed. guard let account = try? WPAccount.lookupDefaultWordPressComAccount(in: mainContext) else { - return + return (blogInContext?.dotComID, remote) } - siteID = siteID ?? account.primaryBlogID - remoteInstance = remoteInstance ?? .init(wordPressComRestApi: account.wordPressComRestV2Api) + + return ( + blogInContext?.dotComID ?? account.primaryBlogID, + remote ?? .init(wordPressComRestApi: account.wordPressComRestV2Api) + ) } guard let siteID, From 241e0ed5d0430a816bf76e76798cf2698b00ccff Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Wed, 14 Jun 2023 16:55:13 +0700 Subject: [PATCH 6/7] Call the completion block when the guard condition fails --- .../Utility/Blogging Prompts/PromptRemindersScheduler.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift index 4a383100951c..847abf648fda 100644 --- a/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift +++ b/WordPress/Classes/Utility/Blogging Prompts/PromptRemindersScheduler.swift @@ -44,6 +44,7 @@ class PromptRemindersScheduler { /// - completion: Closure called after the process completes. func schedule(_ schedule: BloggingRemindersScheduler.Schedule, for blog: Blog, time: Date? = nil, completion: @escaping (Result) -> Void) { guard let context = blog.managedObjectContext else { + completion(.failure(Errors.unknown)) return } From a9d5782042c7cc1cc58a2a755c14d2cd1b084093 Mon Sep 17 00:00:00 2001 From: David Christiandy <1299411+dvdchr@users.noreply.github.com> Date: Thu, 15 Jun 2023 16:14:29 +0700 Subject: [PATCH 7/7] style: remove extra spacing --- WordPress/Classes/Services/BloggingPromptsService.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Classes/Services/BloggingPromptsService.swift b/WordPress/Classes/Services/BloggingPromptsService.swift index cb37cae0487c..3b3ec5d8af52 100644 --- a/WordPress/Classes/Services/BloggingPromptsService.swift +++ b/WordPress/Classes/Services/BloggingPromptsService.swift @@ -190,7 +190,7 @@ class BloggingPromptsService { required init?(contextManager: CoreDataStackSwift = ContextManager.shared, remote: BloggingPromptsServiceRemote? = nil, blog: Blog? = nil) { - let blogObjectID = blog?.objectID + let blogObjectID = blog?.objectID let (siteID, remoteInstance) = contextManager.performQuery { mainContext in // if a blog exists, then try to use the blog's ID. var blogInContext: Blog? = nil