Edit Notifications #2663
Edit Notifications #2663
Conversation
# Conflicts: # Wikipedia/Code/MWKDataStore.m
…ls and Mark as read calls
| @@ -122,3 +81,91 @@ class RemoteNotificationsOperationsController { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| final class RemoteNotificationsOperationsTimeController { | |||
| weak var viewContext: NSManagedObjectContext? | |||
There was a problem hiding this comment.
Is it better to keep this value on the viewContext or add WMFKeyValue to RemoteNotifications.xcdatamodeld (like in EventLogging.xcdatamodeld) and use the private context created by RemoteNotificationsModelController?
There was a problem hiding this comment.
It'd be better to use the private context
| @@ -15,6 +15,18 @@ import CoreData | |||
| } | |||
| } | |||
|
|
|||
| public struct RemoteNotificationState: OptionSet { | |||
There was a problem hiding this comment.
could these be modeled as discrete states? it seems like if a notification is excluded, it'll never be seen or read. also, it seems like read implies that it was seen. The states I'm imagining are nevermind this was just changedignored -> presented -> read
|
|
||
| private func request<T: Decodable>(_ queryParameters: Query.Parameters?, method: Session.Request.Method = .get, completion: @escaping (T?, URLResponse?, Bool?, Error?) -> Void) { | ||
| if method == .get { | ||
| let _ = Session.shared.jsonDecodableTask(host: NotificationsAPI.host, scheme: NotificationsAPI.scheme, method: .get, path: NotificationsAPI.path, queryParameters: queryParameters, completionHandler: completion) |
There was a problem hiding this comment.
Long term, it'd be good to have this take a session on init instead of relying on Session.shared. Short term could just be private let session = Session.shared
# Conflicts: # Wikipedia/Code/MWKDataObject.m
| let modelBundle = Bundle.wmf | ||
| guard let modelURL = modelBundle.url(forResource: modelName, withExtension: modelExtension) else { | ||
| assertionFailure("Couldn't find url for resource named \(modelName) with extension \(modelExtension) in bundle \(modelBundle); make sure you're providing the right name, extension and bundle") | ||
| abort() |
There was a problem hiding this comment.
Could this fallback on not creating the controller instead of crashing the app?
| } | ||
| guard let model = NSManagedObjectModel(contentsOf: modelURL) else { | ||
| assertionFailure("Couldn't create model with contents of \(modelURL); make sure \(modelURL) is the correct url for \(modelName)") | ||
| abort() |
| class RemoteNotificationsFetchOperation: RemoteNotificationsOperation { | ||
| override func execute() { | ||
| self.managedObjectContext.perform { | ||
| self.apiController.getAllUnreadNotifications(from: ["wikidata", "en"]) { fetchedNotifications, error in |
There was a problem hiding this comment.
should this be the list of app languages instead of "en"?
| private let modelController: RemoteNotificationsModelController? | ||
| private let deadlineController: RemoteNotificationsOperationsDeadlineController? | ||
|
|
||
| private let syncRepeatingTime: Int = 15 |
There was a problem hiding this comment.
Does this mean it's still checking every 15 seconds? Oh, related, should this polling be reachability aware?
There was a problem hiding this comment.
(on both counts can be follow-ups as needed)
There was a problem hiding this comment.
I don't think this needs to be shut off based on reachability as long as it handles network failures gracefully. There's also waitsForConnectivity on URLSession that we might be able to set for this and the saved articles fetcher.
There was a problem hiding this comment.
right now it's checking every 15 seconds, just like WMFReadingListsController. good point with reachability, I'm looking at solutions that don't involve AFNetworkReachabilityManager since we want to move away from AFNetworking
Things to look out for:
Doing
Merge after #2676
Testing:
Prerequisites:
If you have any old
revertednotifications in your account:RemoteNotificationsModelControllerI if you don't have any old
revertednotifications, you can use the test account in 1P