-
Notifications
You must be signed in to change notification settings - Fork 35
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
Keep Remote Messages in database if they were shown #911
Conversation
@@ -86,16 +83,23 @@ public final class RemoteMessagingStore: RemoteMessagingStoring { | |||
os_log("Remote messaging config - save processed version: %d", log: log, type: .debug, processorResult.version) | |||
|
|||
let context = database.makeContext(concurrencyType: .privateQueueConcurrencyType, name: Constants.privateContextName) | |||
saveRemoteMessagingConfig(withVersion: processorResult.version, in: context) | |||
context.performAndWait { |
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.
I've updated how DB context is used in the store. Currently all changes triggered externally (all public API of RemoteMessagingStore) always happen within one database transaction. All private API takes context as parameter but never calls save
.
os_log("Failed to save remote message entity: %@", log: log, type: .error, error.localizedDescription) | ||
if remoteMessageManagedObject.isInserted { | ||
remoteMessageManagedObject.id = remoteMessage.id | ||
remoteMessageManagedObject.shown = false |
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.
We only set shown
to false
when it's a new message. For reused message IDs we don't clear shown
flag (this is in line with Android implementation).
if let remoteMessage = processorResult.message { | ||
addOrUpdate(remoteMessage: remoteMessage, in: context) | ||
} else { | ||
markScheduledMessagesAsDone(in: context) | ||
} | ||
deleteNotShownDoneMessages(in: context) |
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.
If there's a new message to be saved, add or update it. Update, in case that ID has already been shown to the user, in which case we're reusing the database object.
If there's no message in the config, we mark scheduled messages as done.
After that, delete all messages that were previously scheduled and never shown (that includes messages just marked as done in markScheduledMessagesAsDone
).
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.
Overall looks good, left one question about making sure the save config pixel still has enough information to debug with, but otherwise the functionality has worked as expected. Great job!
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/1207619243206445/1207864976004819/f
iOS PR: duckduckgo/iOS#3136
macOS PR: duckduckgo/macos-browser#3012
What kind of version bump will this require?: Major
Description:
Update RemoteMessagingStore to keep track of messages that have ever been shown to the user,
also if the user didn't interact with them (by dismissing or clicking any buttons).
Steps to test this PR:
See platform PRs for steps to test.
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template