-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add use our app flow #859
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
Add use our app flow #859
Conversation
…marcos/in_app_fb_usage_flow � Conflicts: � app/src/main/java/com/duckduckgo/app/di/DaggerWorkerFactory.kt � app/src/main/java/com/duckduckgo/app/di/NotificationModule.kt � app/src/main/java/com/duckduckgo/app/notification/AndroidNotificationScheduler.kt � app/src/main/java/com/duckduckgo/app/notification/NotificationHandlerService.kt � app/src/main/java/com/duckduckgo/app/notification/NotificationRegistrar.kt � app/src/main/java/com/duckduckgo/app/statistics/VariantManager.kt � app/src/main/res/values/string-untranslated.xml
…ew stage to the logic after onboarding depending on shortcut capabilities.
…marcos/in_app_fb_usage_flow
… github.com:duckduckgo/Android into feature/marcos/in_app_fb_usage_flow
… github.com:duckduckgo/Android into feature/marcos/in_app_fb_usage_flow � Conflicts: � app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
… github.com:duckduckgo/Android into feature/marcos/in_app_fb_usage_flow � Conflicts: � app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
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.
Small feedback related to testing:
Shortcut opens existing tab if url matches works following the steps in the description, but using the home screen icon created inside the FB flow opens a new time each time.
I will continue testing and reviewing the PR, no blocker.
…marcos/in_app_fb_usage_flow � Conflicts: � app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt � app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt � app/src/main/java/com/duckduckgo/app/statistics/pixels/Pixel.kt � 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.
Thanks @marcosholgado
Code reviewed, sending you some comments which 80% are about naming and 20% myself thinking out loud 🙏
I didn't do a deep QA because some parts are still missing. But I went through all the test steps from the description and everything went well (except for opening multiple tabs when clicking the home screen icon).
app/src/main/java/com/duckduckgo/app/global/events/db/UserEventEntity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/global/events/db/UserEventEntity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/global/events/db/UserEventsStore.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/global/db/AppDatabaseTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/shortcut/ShortcutReceiverTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/global/useourapp/UseOurAppDetector.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/notification/AndroidNotificationSchedulerTest.kt
Show resolved
Hide resolved
… github.com:duckduckgo/Android into feature/marcos/in_app_fb_usage_flow
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.
Code wise is great! I will proceed with testing later today
app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
Outdated
Show resolved
Hide resolved
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.
Testing results:
All tests worked as described 👍
Just two questions related to some extra behaviors I've seen:
- I dismissed by mistake the
Add Facebook Shortcut Dialog. So I added the shortcut manually via overflow menu. It triggered the pixelm_uoa_s_a, is that expected? - Related to that, I was testing "Update from previous version and no established", so the user is not included in the experience, and if I add the FB shorcut, a pixel is also sent. Don't know if we expect to receive pixels from users that are not part of the "experiment".
- While testing the first case, I had a tab with FB open. Then I received the first notification, I clicked on it, and
m_uoa_vanwas sent. According to the description, that pixel is supposed to be triggered when I tap on "Add Facebook Shortcut" (which also happened). I don't think is very important, but better to tell about that.
Let me know if everything is fine. If all good and no more changes are needed, I will approve PR.
|
@cmonfortep thanks for the review. I've added a couple new tests in the description at the bottom of it off the back of what we talked about earlier today. |
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.
💥 Thre's a crash when opening the home screen icon because accessing database from the main thread.
In any case, I continued testing after changing the code (the same I proposed you in the comment) and it works. So we are close to finish this 👍
app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt
Outdated
Show resolved
Hide resolved
… github.com:duckduckgo/Android into feature/marcos/in_app_fb_usage_flow
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.
LGTM 👍 ☀️
Task/Issue URL: https://app.asana.com/0/72649045549333/1177259750357651
Tech Design URL: See https://app.asana.com/0/72649045549333/1177259750357651
CC:
Description:
Introducing a new flow for users after onboarding to encourage the use of in-browser apps.
Before installing the app from this branch
Go to
CtaViewModelline 212/213 and change it toval days = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - timestampKey.timestamp)and change the value from 2 to 1 so it triggers after 1 minute.Go to
AndroidNotificationSchedulerline 70/71 and change it toTimeUnit.SECONDSand change the value to 20 so the notification will trigger after 20 seconds instead of 1 day.Go to
UseOurAppMigrationManagerand change the weight ofdefaultMigrationto 0.0 anduseOurAppMigrationto 1.0Steps to test this PR:
Update from previous version and pixels
AndroidNotificationScheduler).mnot_s_uoashould be sent (notification seen)mnot_l_uoashould be sent (notification launched)m_uoa_dshould be sent (use our app dialog seen)m_uoa_d_okshould be sent (use our app ok button pressed)m_uoa_vanshould be sent (use our app site visited after notification seen)m_uoa_s_ashould be sent. (use our app shortcut added)m_uoa_vasshould be sent (use our app site visited after adding shortcut).CtaViewModelthe next time you navigate to Facebook (page has to load) you should see a new Cta.m_uoa_ddshould be sent (use our app delete dialog seen)m_uoa_vadshould be sent (use our app site visited after delete cta).Pixel when visiting use our app URL
m_uoa_vadshould be sent (use our app site visited after delete cta).m_uoa_vadshould not be sent again.m_uoa_vadshould be sent (use our app site visited after delete cta).Keyboard shown when ignoring cta
AndroidNotificationScheduler).Shortcut opens existing tab if url matches
Update from previous version hide tips
Update from previous version and no established
Update from previous version but not able to see the experience
defaultMigrationto 1.0 anduseOurAppMigrationto 0.0NEW TESTS
Make sure the weight of
useOurAppMigrationis 1.0 and the default to 0.0Update from previous version as established and language not english
Creating a shortcut before seeing cta doesn't sent incorrect pixel
http://m.facebook.comm_sho_ashould be sent instead ofm_sho_uoa_am_sho_oshould be sent instead ofm_sho_uoa_oInternal references:
Software Engineering Expectations
Technical Design Template
0