New control group and changes on when to show search widget cta #723

cmonfortep · 2020-02-26T09:42:14Z

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

Description:

VariantManager changes

Moving as a control group a variant with concept test + Ctas.
Dropping variant only showing concept test.

SearchWidget cta changes

Only show search widget Cta if the user has seen at least on nonSerp Cta.

Steps to test this PR:

VariantManager changes

Concept test variant ml: Concept test with old ctas

Hardcode into variantManager variant "ml"
Perform a fresh install
Run the app and ensure during the onboarding Default browser cta appears
Visit a web with trackers like cnn.com
Ensure dax dialog appears informing about trackers found
Click on "High five!"
Ensure no more Ctas are shown
Visit duckduckgo.com
Ensure dax dialog appears
Click on "Phew!"
Ensure no more Ctas are shown
Continue the Dax journey till the end (open new tab, visit a website, open a new tab)
Ensure Search widget cta appears in home screen

Concept test variant mh: concept test with ctas as dax dialogs:

Hardcode into variantManager variant "mh"
Perform a fresh install
Run the app and ensure onboarding doesn't show any default browser cta
Visit a web with trackers like cnn.com
Ensure dax dialog appears informing about trackers found
Click on "High five!"
Ensure DefaultBrowser cta appears
Click on "Ok"
Ensure Dialog asking to set ddg as default browser appears
Visit duckduckgo.com
Ensure dax dialog appears
Click on "Phew!"
Ensure SearchWidget cta appears
Click on "Add"
Ensure dialog appears asking the user to add the search widget

SearchWidget cta new user flow

Hardcode into variantManager variant "mh"
Perform a fresh install
Run the app and ensure onboarding doesn't show any default browser cta
Visit duckduckgo.com
Ensure dax dialog appears
Click on "Phew!"
Ensure SearchWidget cta DOES NOT appear
Visit a site with trackers, a major network site or a site without trackers (wikipedia.org)
Ensure nonSerp dax dialog appears
Accept or dismiss dialog
Visit duckduckgo.com again
Ensure SearchWidget cta appears

Side note: a more detailed way of testing all the different combinations is explained inside #716 . If the reviewer decides to use any test scenario from that PR, please ensure it's still valid taking into account the changes introduced in this PR.

Internal references:

Software Engineering Expectations
Technical Design Template

* Removing from experiment concept test as control group

cmonfortep · 2020-02-26T09:44:27Z

@subsymbolic I will move this PR over to you.
It has a change that affects variantManager and we talked about pinging you once you were able to test changes introduced also in #714.

subsymbolic

Great work, provided a few tidy ups in the comments.

subsymbolic · 2020-02-26T23:48:38Z

app/src/main/java/com/duckduckgo/app/statistics/VariantManager.kt

-                features = listOf(ConceptTest, SuppressHomeTabWidgetCta, SuppressOnboardingDefaultBrowserCta),
-                filterBy = { isEnglishLocale() }),
            Variant(
                key = "ml",


You'll need a new variant here, ml was recently reserved for the eu auction, see RESERVED_EU_AUCTION_VARIANT at the top. You should have been protected from this by the verifyNoDuplicateVariantNames test but it didn't quite happen because RESERVED_EU_AUCTION_VARIANT is a special case and not in the variants list. Let's add and protect the next person.

subsymbolic · 2020-02-27T00:15:38Z

app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelTest.kt

        )
    }

+    private fun givenSerpCtaShown() {


Nice! I like the way you have set these out.

subsymbolic · 2020-02-27T00:21:51Z

app/src/main/java/com/duckduckgo/app/cta/ui/CtaViewModel.kt

            // Is Serp
-            if (!daxDialogSerpShown() && isSerpUrl(it.url)) {
-                return DaxDialogCta.DaxSerpCta(onboardingStore, appInstallStore)
+            if (isSerpUrl(it.url)) {


Nice work making this self-documenting. Let's delete the // Is Serp comment, it's no longer needed.

subsymbolic · 2020-02-27T00:38:02Z

app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelTest.kt

+        setConceptTestFeature(SearchWidgetDaxCta)
+        givenSerpCtaShown()
+        givenAtLeastOneNonSerpCtaShown()
+        whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_SEARCH_WIDGET)).thenReturn(true)


If I set this condition to false, the test should fail but it doesn't. Update test to be identical to its opposite test above, other than this condition and assertion.

malmstein

couple of comments on readability

malmstein · 2020-02-27T08:14:03Z

app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelTest.kt

+        whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_SEARCH_WIDGET)).thenReturn(true)
+        val site = site(url = "http://www.duckduckgo.com")
+
+        val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)


I'd rather we don't test if something is null as result of a business logic function

Will this change only apply if DaxDialogCta.NotNeeded exists or also for null values in general?

When a null value is a possible and expected value os an API (whether it should be or not is another discussion) we should test it just like we would any other value. What's your reason for not wanting to test it @malmstein?

malmstein · 2020-02-27T08:15:24Z

app/src/main/java/com/duckduckgo/app/cta/ui/CtaViewModel.kt

+        return when {
+            !daxDialogSerpShown() -> DaxDialogCta.DaxSerpCta(onboardingStore, appInstallStore)
+            canShowWidgetDaxCta() -> DaxDialogCta.SearchWidgetCta(widgetCapabilities, onboardingStore, appInstallStore)
+            else -> null


can we return here something like DaxDialogCta.NotNeeded instead of null?

I agree with a different strategy to avoid returning null but I think we should agree first on a guideline or a tech design (using a new type inside Cta class like you suggested, and maybe Option in other cases).

Also, I think the change should be introduced in a different PR (LHF or inside the next project that comes here to introduce new Ctas). why? Because the change will affect how Ctas are handled in BrowserTabViewModel, BrowserTabFragment, and other methods in this same file + their own tests. The PR will be big enough to deserve its own PR so the reviewer can focus on the refactor.

what do you think?

Nullability is a well-defined concept in kotlin, the code is understandable, not lying and not duplicating state. This is a good suggestion however I place it in the "nice to have" rather than PR blocking category.

* Test refactor to guard from someone using EU_AUCTION key variant

If null variant, test should throw exception and be invalid. We can remove unnecessary !! when asserting values

subsymbolic · 2020-02-27T12:58:44Z

@cmonfortep I have approved these changes however there is an outstanding question from @malmstein who is out. You are welcome to wait until that is resolved or, if you're blocked, you are welcome to merge now but continue that conversation with a view to creating a followup PR if required.

…feature/cristian/experiment2_flow_changed # Conflicts: # app/src/androidTest/java/com/duckduckgo/app/statistics/VariantManagerTest.kt

cmonfortep · 2020-02-27T16:45:07Z

@malmstein moving forward with this PR, but happy to discuss your proposal offline. Thanks for the review.

* Dax dialog with two buttons (#713) * New variant: Showing DefaultBrowserCta and SearchWidgetCta as Dax dialogs (#714) * Insert DefaultBrowser Cta and SearchWidget Cta as Dax dialog during Dax Journey (#716) * New control group and changes on when to show search widget cta (#723)

cmonfortep added 2 commits February 25, 2020 23:04

Moving as control group variant of concept test + CTAs

ec31adc

* Removing from experiment concept test as control group

Show search widget cta only if at least one nonSerp Cta has been shown

03609c8

cmonfortep requested review from CDRussell, malmstein, marcosholgado and subsymbolic as code owners February 26, 2020 09:42

cmonfortep assigned subsymbolic Feb 26, 2020

subsymbolic reviewed Feb 27, 2020

View reviewed changes

malmstein requested changes Feb 27, 2020

View reviewed changes

cmonfortep added 4 commits February 27, 2020 10:11

Update key variant for variant using the same key as EU_AUCTION

608f00a

* Test refactor to guard from someone using EU_AUCTION key variant

Using first instead o firstOrNull

c2d52f9

If null variant, test should throw exception and be invalid. We can remove unnecessary !! when asserting values

remove comment as next line is self explanatory

9d94957

mock widgetCapabilities values to prepare the expected valid scenario

fefdc64

cmonfortep requested a review from subsymbolic February 27, 2020 09:18

subsymbolic approved these changes Feb 27, 2020

View reviewed changes

Merge branch 'feature/cristian/fb_insert_cta_concept_test_exp2' into …

b7ae00d

…feature/cristian/experiment2_flow_changed # Conflicts: # app/src/androidTest/java/com/duckduckgo/app/statistics/VariantManagerTest.kt

cmonfortep mentioned this pull request Feb 27, 2020

Insert CTAs on concept test Experiment#2 #724

Merged

cmonfortep merged commit 5baeadc into feature/cristian/fb_insert_cta_concept_test_exp2 Feb 27, 2020

cmonfortep deleted the feature/cristian/experiment2_flow_changed branch February 27, 2020 16:43

New control group and changes on when to show search widget cta #723

New control group and changes on when to show search widget cta #723

Uh oh!

Conversation

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

Uh oh!

Description:

Steps to test this PR:

Internal references:

Uh oh!

cmonfortep commented Feb 26, 2020

Uh oh!

subsymbolic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malmstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subsymbolic Feb 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subsymbolic commented Feb 27, 2020

Uh oh!

cmonfortep commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cmonfortep commented Feb 26, 2020 •

edited

Loading

subsymbolic Feb 27, 2020 •

edited

Loading