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.
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
Some edit reverted notification cleanup #4017
Conversation
- remove category and actions setup, will add back later - remove local notification scheduling for edit reverted - remove category validation when adding to database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major to test for now, just confirm it builds and runs.
It builds, it runs! One minor question below.
let modelChange = RemoteNotificationsModelChange(type: modelChangeType, notificationsGroupedByCategoryNumber: notificationsGroupedByCategoryNumber) | ||
let responseCoordinator = RemoteNotificationsModelChangeResponseCoordinator(modelChange: modelChange, modelController: self) | ||
DispatchQueue.main.async { | ||
NotificationCenter.default.post(name: RemoteNotificationsModelController.modelDidChangeNotification, object: responseCoordinator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I'm missing its usage in the subsequent PRs, but would it also make sense to remove line 45 which no longer appears to be used?
wikipedia-ios/WMF Framework/Remote Notifications/Model/RemoteNotificationsModelController.swift
Line 45 in 1c0ee40
public static let modelDidChangeNotification = NSNotification.Name(rawValue: "RemoteNotificationsModelDidChange") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! I pushed a commit removing this.
Phabricator: https://phabricator.wikimedia.org/T287298
Notes
This is part one of many for T287298. This task is just some early code cleanup I decided to do before digging into the RemoteNotifications database work. From this point forward I think it would be easier if we thought of edit reverted holistically with the other notification types than tiptoeing around this code. It also removes local notification scheduling for edit revert.
After review, please wait for #4013 to be reviewed and merged. Then update this with feature/notifications and merge.
Test Steps
Nothing major to test for now, just confirm it builds and runs.