Add Pixels when app restarts in foreground by automatic data clearer #858

cmonfortep · 2020-06-12T08:01:17Z

Task/Issue URL: https://app.asana.com/0/0/1180090817363681/f
Tech Design URL:
CC:

Description:
This PR adds logic to send pixels when the app gets restarted due to automatic data clearer being triggered with the app in foreground.

We will send pixels to detect if the user opened the app from an external intent or without it. This will give us some insights about the impact of this issue: #780

Steps to test this PR:
In order to test this, we need to:

Disable WorkManager: comment line AutomaticDataClearer#121 scheduleBackgroundTimerToTriggerClear(clearWhenOption.durationMilliseconds())
Enable automatic data clearer in settings (5 seconds for testing purposes)

Test 1:

Start the app as normal after being inactive for 5 seconds
Ensure app restarts due to automatic dataclearer
Ensure pixel mf_r sent

Test 2:

Start the app from a home screen icon or any external link after being inactive for 5 seconds
Ensure app restarts due to automatic dataclearer
Ensure pixel mf_ri sent

Test 3:

Start the app from search widget after being inactive for 5 seconds
Ensure app restarts due to automatic dataclearer
Ensure pixel mf_ri sent

Test 3:

Start the app from an external link in less than 5 seconds
Ensure app didn't restart
Send the app to backgound (home button or turn off screen)
Bring back the app to foreground after being inactive for 5 seconds
Ensure app restarts due to automatic dataclearer
Ensure pixel mf_r sent

Internal references:

Software Engineering Expectations
Technical Design Template

… when app restarts due to data clearing onAppForeground

…ned from an external link restarts state.

malmstein

Just a couple of comments on error handling, but haven't tested this on a device

app/src/main/java/com/duckduckgo/app/fire/DataClearerForegroundAppRestartPixel.kt

malmstein · 2020-06-12T10:40:59Z

app/src/main/java/com/duckduckgo/app/fire/DataClearerForegroundAppRestartPixel.kt

+    fun firePendingPixels() {
+        firePendingPixels(pendingAppForegroundRestart, Pixel.PixelName.FORGET_ALL_AUTO_RESTART)
+        firePendingPixels(pendingAppForegroundRestartWithIntent, Pixel.PixelName.FORGET_ALL_AUTO_RESTART_WITH_INTENT)
+        resetCount()


This is called from the Application class and there is no crash handling. Specially because resetCount() uses preferences who might not have been initialized properly.

We talked about this offline, and we think it's fine to keep it as it's right now.

app/src/main/java/com/duckduckgo/app/systemsearch/SystemSearchActivity.kt

amaury901130 · 2020-06-20T01:01:16Z

app/src/main/java/com/duckduckgo/app/global/DuckDuckGoApplication.kt

            it.addObserver(defaultBrowserObserver)
            it.addObserver(appEnjoymentLifecycleObserver)
+            it.addObserver(dataClearerForegroundAppRestartPixel)
        }


You could use apply instance of also.

yes, they are equivalent, but no strong preference on styles here. So for now, it's fine to keep it as it is now and focus in related changes to our initial goal. But that was a nice catch. 👍

amaury901130 · 2020-06-20T01:04:35Z

app/src/main/java/com/duckduckgo/app/global/DuckDuckGoApplication.kt

+    private fun submitUnsentFireAppRestartedWithIntentPixels() {
+        dataClearerForegroundAppRestartPixel.firePendingPixels()
    }


Why you create a function just for this?

Yeah, why not 🤷‍♀️

app/src/main/java/com/duckduckgo/app/statistics/pixels/Pixel.kt

app/src/main/java/com/duckduckgo/app/fire/DataClearerForegroundAppRestartPixel.kt

CDRussell · 2020-06-22T13:21:25Z

app/src/main/java/com/duckduckgo/app/fire/DataClearerForegroundAppRestartPixel.kt

+        detectedUserIntent = false
+    }
+
+    fun registerIntent(intent: Intent?) {


can we find a better name for this? it's not clear from the name what it does or where/when to call it

I could come up with a better name, like I previously expected. No problem since this will be removed once we work on the fix at some point.

app/src/main/java/com/duckduckgo/app/fire/DataClearerForegroundAppRestartPixel.kt

…ns and improves readability.

cmonfortep added 7 commits June 10, 2020 17:45

Register when app is opened with a intent with params and Fire pixels…

3b63afb

… when app restarts due to data clearing onAppForeground

Merge branch 'develop' into feature/cristian/pixel_external_link_restart

3d89128

move DataClearerForegroundAppRestartPixel into its own class

c103abd

Added test cases for DataClearerForegroundAppRestartPixel

cc00e63

rename based on Class name

fc6b454

add test case to ensure sending the app to background after being ope…

24f1479

…ned from an external link restarts state.

clenaup imports

329a99a

cmonfortep changed the title ~~Feature/cristian/pixel external link restart~~ Add Pixels when app restarts in foreground by automatic data clearer Jun 12, 2020

cmonfortep marked this pull request as ready for review June 12, 2020 08:59

CDRussell self-assigned this Jun 12, 2020

malmstein reviewed Jun 12, 2020

View reviewed changes

cmonfortep added 4 commits June 12, 2020 14:21

fix failing test cases: use concrete class instead of mock

5fef399

remove timber logs

47ae684

Using lazy instead of get sharedpref multiple times

c7a309f

import clenaup

65455a8

amaury901130 reviewed Jun 20, 2020

View reviewed changes

CDRussell reviewed Jun 22, 2020

View reviewed changes

app/src/main/java/com/duckduckgo/app/statistics/pixels/Pixel.kt Outdated Show resolved Hide resolved

CDRussell approved these changes Jun 22, 2020

View reviewed changes

CDRussell mentioned this pull request Jun 22, 2020

Remove string for fire process name from non-default languages #866

Merged

cmonfortep added 3 commits June 25, 2020 15:03

use m pixels instead of mf

9191d98

fix typo

9bc638c

remove method with single method call. This reduces method redirectio…

dd9a07c

…ns and improves readability.

cmonfortep merged commit e1890ba into develop Jun 26, 2020

cmonfortep deleted the feature/cristian/pixel_external_link_restart branch June 26, 2020 09:05

Add Pixels when app restarts in foreground by automatic data clearer #858

Add Pixels when app restarts in foreground by automatic data clearer #858

Uh oh!

Conversation

cmonfortep commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Internal references:

Uh oh!

malmstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

malmstein Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amaury901130 Jun 20, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

amaury901130 Jun 20, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CDRussell Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cmonfortep commented Jun 12, 2020 •

edited

Loading

CDRussell Jun 22, 2020 •

edited

Loading