-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Quick Search Notification Experiment #742
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
Conversation
# Conflicts: # app/src/main/res/values/string-untranslated.xml
…s see the full notification
|
This is what I see on API 21 @subsymbolic. Nonetheless, I've changed the icons to be (+) and (X) for devices that show them. |
malmstein
left a comment
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.
Thanks for the feedback @cmonfortep @subsymbolic. I've made some changes and replied to your comments, please let me know if it's hard to read on Github and we can talk about it online
| } | ||
|
|
||
| @Test | ||
| fun searchNotificationEnabledVariantIsActive() { |
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.
Minor: mention feature e.g searchNotificationVariantIsActiveAndHasStickySearchNotificationFeature
| import dagger.Provides | ||
| import javax.inject.Singleton | ||
|
|
||
|
|
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.
Minor: Delete additional space
| val SEARCH = Channel( | ||
| "com.duckduckgo.search", | ||
| R.string.notificationChannelTutorials, | ||
| NotificationManagerCompat.IMPORTANCE_DEFAULT |
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.
Update: We discussed priorities over zoom. HIGH priority shows a partial peek which we don't want, so decided that DEFAULT provides the best compromise/combination of features.
| val id: String | ||
| val layoutId: Int | ||
| val priority: Int | ||
| val pressIntent: String |
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.
The launchIntent String could be used for both types in the notification later down the line, the logic is currently identical. I'm ok with your decision to keep them both though, keeps later changes more flexible 👍
|
|
||
| private fun buildPendingIntent(context: Context): PendingIntent { | ||
| val intent = SystemSearchActivity.intent(context, widgetSearch = true) | ||
| val intent = SystemSearchActivity.fromWidget(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.
Nice change!
app/src/main/java/com/duckduckgo/app/notification/model/SchedulableNotification.kt
Outdated
Show resolved
Hide resolved
subsymbolic
left a comment
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.
This is looking great, I can't wait to start using it. I added some final very minor cleanup comments but don't need to rereview those unless you start making bigger changes. Merge when you're ready provided @cmonfortep is happy that his comments have also been addressed
# Conflicts: # app/src/main/res/values/string-untranslated.xml
cmonfortep
left a comment
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.
Great work @malmstein. Nothing more to add, looking forward to seeing this on production, and curious about the experiment results. 🚀
# Conflicts: # app/src/androidTest/java/com/duckduckgo/app/settings/SettingsViewModelTest.kt # app/src/main/java/com/duckduckgo/app/global/ViewModelFactory.kt # app/src/main/java/com/duckduckgo/app/settings/SettingsActivity.kt # app/src/main/java/com/duckduckgo/app/settings/SettingsViewModel.kt # app/src/main/java/com/duckduckgo/app/settings/db/SettingsDataStore.kt # app/src/main/java/com/duckduckgo/app/statistics/pixels/Pixel.kt # app/src/main/res/layout/content_settings_general.xml # app/src/main/res/values/string-untranslated.xml

Task/Issue URL: https://app.asana.com/0/1157893581871903/1114784852684207
Experiment definition: https://app.asana.com/0/1157893581871903/1165956670845358
Description:
In previous tasks we saw that installing the widget to the home screen led to a significant increase in search and retention.
A sticky notification could work like a widget, however, unlike a widget would not be restricted to a home screen. It would be available at all times, across home screens as well as when users are in other apps; thus having the potential to increase search and retention further.
Instead of using CTA from Onboarding like we've done in the past, this time we are going to show a notification:
Two days after installing the application we'll show the "Quick Search Notification Prompt". Note that the time to show the notification is not reset if the app is not used. Should the user choose yes, the ongoing notification will be shown. The prompt is only shown once per install, but the option is always available in Settings
Steps to test this PR:
Concept test variant mf: Control group
Concept test variant mg: Quick Search Experience
Screenshots: