Skip to content

Commit 6c44673

Browse files
lagheelaghee
committed
Address review comments
1 parent bc22f9d commit 6c44673

File tree

11 files changed

+122
-67
lines changed

11 files changed

+122
-67
lines changed

‎app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt

+1
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,7 @@ class BrowserTabViewModelTest {
608608
duckPlayer = mockDuckPlayer,
609609
brokenSitePrompt = mockBrokenSitePrompt,
610610
userBrowserProperties = mockUserBrowserProperties,
611+
refreshPixelSender = refreshPixelSender,
611612
)
612613

613614
val siteFactory = SiteFactoryImpl(

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

+35-27
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import androidx.room.Room
2424
import androidx.test.platform.app.InstrumentationRegistry
2525
import com.duckduckgo.app.browser.DuckDuckGoUrlDetectorImpl
2626
import com.duckduckgo.app.browser.R
27+
import com.duckduckgo.app.browser.refreshpixels.RefreshPixelSender
2728
import com.duckduckgo.app.cta.db.DismissedCtaDao
2829
import com.duckduckgo.app.cta.model.CtaId
2930
import com.duckduckgo.app.cta.model.DismissedCta
@@ -115,6 +116,8 @@ class CtaViewModelTest {
115116

116117
private val mockBrokenSitePrompt: BrokenSitePrompt = mock()
117118

119+
private val mockRefreshPixelSender: RefreshPixelSender = mock()
120+
118121
private val mockUserBrowserProperties: UserBrowserProperties = mock()
119122

120123
private val requiredDaxOnboardingCtas: List<CtaId> = listOf(
@@ -151,6 +154,7 @@ class CtaViewModelTest {
151154
whenever(mockDuckPlayer.isSimulatedYoutubeNoCookie(any())).thenReturn(false)
152155
whenever(mockBrokenSitePrompt.shouldShowBrokenSitePrompt(any())).thenReturn(false)
153156
whenever(mockBrokenSitePrompt.isFeatureEnabled()).thenReturn(false)
157+
whenever(mockBrokenSitePrompt.getUserRefreshesCount()).thenReturn(emptySet())
154158
whenever(mockSubscriptions.isEligible()).thenReturn(false)
155159

156160
testee = CtaViewModel(
@@ -169,6 +173,7 @@ class CtaViewModelTest {
169173
subscriptions = mockSubscriptions,
170174
duckPlayer = mockDuckPlayer,
171175
brokenSitePrompt = mockBrokenSitePrompt,
176+
refreshPixelSender = mockRefreshPixelSender,
172177
userBrowserProperties = mockUserBrowserProperties,
173178
)
174179
}
@@ -279,7 +284,7 @@ class CtaViewModelTest {
279284

280285
@Test
281286
fun whenRefreshCtaWhileBrowsingAndSiteIsNullThenReturnNull() = runTest {
282-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = null, showBrokenSitePrompt = false)
287+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = null)
283288
assertNull(value)
284289
}
285290

@@ -288,25 +293,28 @@ class CtaViewModelTest {
288293
whenever(mockSettingsDataStore.hideTips).thenReturn(true)
289294
val site = site(url = "http://www.facebook.com", entity = TestEntity("Facebook", "Facebook", 9.0))
290295

291-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
296+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
292297
assertNull(value)
293298
}
294299

295300
@Test
296-
fun whenRefreshCtaWhileBrowsingAndHideTipsIsTrueAndShouldShowBrokenSitePromptThenReturnBrokenSitePrompt() = runTest {
301+
fun whenRefreshCtaWhileBrowsingAndHideTipsIsTrueAndShouldShowBrokenSitePromptThenReturnBrokenSitePromptAndFireRefreshPixels() = runTest {
297302
whenever(mockSettingsDataStore.hideTips).thenReturn(true)
298303
val site = site(url = "http://www.facebook.com", entity = TestEntity("Facebook", "Facebook", 9.0))
304+
whenever(mockBrokenSitePrompt.getUserRefreshesCount()).thenReturn(setOf(3))
305+
whenever(mockBrokenSitePrompt.shouldShowBrokenSitePrompt(any())).thenReturn(true)
299306

300-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = true)
307+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
301308
assertTrue(value is BrokenSitePromptDialogCta)
309+
verify(mockRefreshPixelSender).onRefreshPatternDetected(setOf(3))
302310
}
303311

304312
@Test
305313
fun whenRefreshCtaOnHomeTabAndHideTipsIsTrueAndWidgetCompatibleThenReturnWidgetCta() = runTest {
306314
whenever(mockSettingsDataStore.hideTips).thenReturn(true)
307315
whenever(mockWidgetCapabilities.supportsAutomaticWidgetAdd).thenReturn(true)
308316

309-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
317+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
310318
assertTrue(value is HomePanelCta.AddWidgetAuto)
311319
}
312320

@@ -316,7 +324,7 @@ class CtaViewModelTest {
316324
whenever(mockUserAllowListRepository.isDomainInUserAllowList(any())).thenReturn(true)
317325
val site = site(url = "http://www.facebook.com", entity = TestEntity("Facebook", "Facebook", 9.0))
318326

319-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
327+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
320328
assertNull(value)
321329
}
322330

@@ -326,7 +334,7 @@ class CtaViewModelTest {
326334
whenever(mockWidgetCapabilities.supportsAutomaticWidgetAdd).thenReturn(true)
327335
whenever(mockWidgetCapabilities.hasInstalledWidgets).thenReturn(false)
328336

329-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
337+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
330338
assertTrue(value is HomePanelCta.AddWidgetAuto)
331339
}
332340

@@ -336,7 +344,7 @@ class CtaViewModelTest {
336344
whenever(mockWidgetCapabilities.supportsAutomaticWidgetAdd).thenReturn(false)
337345
whenever(mockWidgetCapabilities.hasInstalledWidgets).thenReturn(false)
338346

339-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
347+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
340348
assertTrue(value is HomePanelCta.AddWidgetInstructions)
341349
}
342350

@@ -351,7 +359,7 @@ class CtaViewModelTest {
351359
"Facebook",
352360
)
353361

354-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false) as OnboardingDaxDialogCta
362+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site) as OnboardingDaxDialogCta
355363

356364
assertTrue(value is OnboardingDaxDialogCta.DaxMainNetworkCta)
357365
val actualText = (value as OnboardingDaxDialogCta.DaxMainNetworkCta).getTrackersDescription(context)
@@ -362,7 +370,7 @@ class CtaViewModelTest {
362370
fun whenRefreshCtaWhileBrowsingOnSiteOwnedByMajorTrackerThenReturnNetworkCta() = runTest {
363371
givenDaxOnboardingActive()
364372
val site = site(url = "http://m.instagram.com", entity = TestEntity("Facebook", "Facebook", 9.0))
365-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false) as OnboardingDaxDialogCta
373+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site) as OnboardingDaxDialogCta
366374
val expectedCtaText = context.resources.getString(
367375
R.string.daxMainNetworkOwnedCtaText,
368376
"Facebook",
@@ -388,7 +396,7 @@ class CtaViewModelTest {
388396
type = TrackerType.OTHER,
389397
)
390398
val site = site(url = "http://www.cnn.com", trackerCount = 1, events = listOf(trackingEvent))
391-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
399+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
392400

393401
assertTrue(value is OnboardingDaxDialogCta.DaxTrackersBlockedCta)
394402
}
@@ -406,7 +414,7 @@ class CtaViewModelTest {
406414
type = TrackerType.OTHER,
407415
)
408416
val site = site(url = "http://www.cnn.com", trackerCount = 1, events = listOf(trackingEvent))
409-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
417+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
410418

411419
assertTrue(value is OnboardingDaxDialogCta.DaxTrackersBlockedCta)
412420
}
@@ -415,7 +423,7 @@ class CtaViewModelTest {
415423
fun whenRefreshCtaWhileBrowsingAndNoTrackersInformationThenReturnNoTrackersCta() = runTest {
416424
givenDaxOnboardingActive()
417425
val site = site(url = "http://www.cnn.com", trackerCount = 1)
418-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
426+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
419427

420428
assertTrue(value is OnboardingDaxDialogCta.DaxNoTrackersCta)
421429
}
@@ -424,7 +432,7 @@ class CtaViewModelTest {
424432
fun whenRefreshCtaOnSerpWhileBrowsingThenReturnSerpCta() = runTest {
425433
givenDaxOnboardingActive()
426434
val site = site(url = "http://www.duckduckgo.com")
427-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
435+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
428436

429437
assertTrue(value is OnboardingDaxDialogCta.DaxSerpCta)
430438
}
@@ -433,7 +441,7 @@ class CtaViewModelTest {
433441
fun whenRefreshCtaWhileBrowsingThenReturnNoTrackersCta() = runTest {
434442
givenDaxOnboardingActive()
435443
val site = site(url = "http://www.wikipedia.com")
436-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
444+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
437445

438446
assertTrue(value is OnboardingDaxDialogCta.DaxNoTrackersCta)
439447
}
@@ -442,7 +450,7 @@ class CtaViewModelTest {
442450
fun whenRefreshCtaWhileBrowsingAndNoAutoconsentThenReturnOtherCta() = runTest {
443451
givenDaxOnboardingActive()
444452
val site = site(url = "http://www.wikipedia.com")
445-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
453+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
446454

447455
assertTrue(value is OnboardingDaxDialogCta.DaxNoTrackersCta)
448456
}
@@ -451,7 +459,7 @@ class CtaViewModelTest {
451459
fun whenRefreshCtaOnHomeTabThenValueReturnedIsNotDaxDialogCtaType() = runTest {
452460
givenDaxOnboardingActive()
453461
val site = site(url = "http://www.wikipedia.com")
454-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, site = site, showBrokenSitePrompt = false)
462+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, site = site)
455463

456464
assertFalse(value is OnboardingDaxDialogCta)
457465
}
@@ -463,7 +471,7 @@ class CtaViewModelTest {
463471
whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_TRACKERS_FOUND)).thenReturn(true)
464472

465473
val site = site(url = "http://www.cnn.com", trackerCount = 1)
466-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
474+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
467475
assertFalse(value is OnboardingDaxDialogCta.DaxEndCta)
468476
}
469477

@@ -475,7 +483,7 @@ class CtaViewModelTest {
475483
whenever(mockDismissedCtaDao.exists(CtaId.DAX_FIRE_BUTTON)).thenReturn(true)
476484

477485
val site = site(url = "http://www.cnn.com", trackerCount = 1)
478-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
486+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
479487
assertTrue(value is OnboardingDaxDialogCta.DaxEndCta)
480488
}
481489

@@ -485,7 +493,7 @@ class CtaViewModelTest {
485493
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO)).thenReturn(false)
486494
whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_SERP)).thenReturn(true)
487495

488-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
496+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
489497
assertTrue(value is DaxBubbleCta.DaxIntroSearchOptionsCta)
490498
}
491499

@@ -494,7 +502,7 @@ class CtaViewModelTest {
494502
givenDaxOnboardingActive()
495503
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO)).thenReturn(true)
496504

497-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
505+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
498506
assertTrue(value is DaxBubbleCta.DaxIntroVisitSiteOptionsCta)
499507
}
500508

@@ -505,7 +513,7 @@ class CtaViewModelTest {
505513
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO_VISIT_SITE)).thenReturn(true)
506514
givenAtLeastOneDaxDialogCtaShown()
507515

508-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
516+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
509517
assertTrue(value is DaxBubbleCta.DaxEndCta)
510518
}
511519

@@ -514,15 +522,15 @@ class CtaViewModelTest {
514522
givenShownDaxOnboardingCtas(listOf(CtaId.DAX_INTRO))
515523
givenUserIsEstablished()
516524

517-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, showBrokenSitePrompt = false)
525+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true)
518526
assertNull(value)
519527
}
520528

521529
@Test
522530
fun whenRefreshCtaWhileDaxOnboardingActiveAndBrowsingOnDuckDuckGoEmailUrlThenReturnNull() = runTest {
523531
givenDaxOnboardingActive()
524532
val site = site(url = "https://duckduckgo.com/email/")
525-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site, showBrokenSitePrompt = false)
533+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = true, site = site)
526534
assertNull(value)
527535
}
528536

@@ -659,7 +667,7 @@ class CtaViewModelTest {
659667
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO)).thenReturn(false)
660668
whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_SERP)).thenReturn(true)
661669

662-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
670+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
663671
assertTrue(value is DaxBubbleCta.DaxIntroSearchOptionsCta)
664672
}
665673

@@ -680,7 +688,7 @@ class CtaViewModelTest {
680688
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO)).thenReturn(true)
681689
whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_TRACKERS_FOUND)).thenReturn(false)
682690

683-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
691+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
684692
assertTrue(value is DaxBubbleCta.DaxIntroVisitSiteOptionsCta)
685693
}
686694

@@ -690,7 +698,7 @@ class CtaViewModelTest {
690698
whenever(mockDismissedCtaDao.exists(CtaId.DAX_INTRO)).thenReturn(true)
691699
whenever(mockDismissedCtaDao.exists(CtaId.DAX_DIALOG_TRACKERS_FOUND)).thenReturn(true)
692700

693-
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false, showBrokenSitePrompt = false)
701+
val value = testee.refreshCta(coroutineRule.testDispatcher, isBrowserShowing = false)
694702
assertFalse(value is DaxBubbleCta.DaxIntroVisitSiteOptionsCta)
695703
}
696704

‎app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

+9-13
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,6 @@ import com.duckduckgo.app.surrogates.SurrogateResponse
265265
import com.duckduckgo.app.tabs.model.TabEntity
266266
import com.duckduckgo.app.tabs.model.TabRepository
267267
import com.duckduckgo.app.tabs.store.TabStatsBucketing
268-
import com.duckduckgo.app.trackerdetection.blocklist.BlockListPixelsPlugin
269268
import com.duckduckgo.app.trackerdetection.model.TrackingEvent
270269
import com.duckduckgo.app.usage.search.SearchCountDao
271270
import com.duckduckgo.autofill.api.AutofillCapabilityChecker
@@ -469,7 +468,6 @@ class BrowserTabViewModel @Inject constructor(
469468
private val defaultBrowserPromptsExperiment: DefaultBrowserPromptsExperiment,
470469
private val swipingTabsFeature: SwipingTabsFeatureProvider,
471470
private val visualDesignExperimentDataStore: VisualDesignExperimentDataStore,
472-
private val blockListPixelsPlugin: BlockListPixelsPlugin,
473471
) : WebViewClientListener,
474472
EditSavedSiteListener,
475473
DeleteBookmarkListener,
@@ -2761,13 +2759,11 @@ class BrowserTabViewModel @Inject constructor(
27612759
val isBrowserShowing = currentBrowserViewState().browserShowing
27622760
val isErrorShowing = currentBrowserViewState().maliciousSiteBlocked
27632761
if (hasCtaBeenShownForCurrentPage.get() && isBrowserShowing) return null
2764-
val showBrokenSitePrompt = shouldShowPromptForBrokenSiteReport()
27652762
val cta = withContext(dispatchers.io()) {
27662763
ctaViewModel.refreshCta(
27672764
dispatchers.io(),
27682765
isBrowserShowing && !isErrorShowing,
27692766
siteLiveData.value,
2770-
showBrokenSitePrompt,
27712767
)
27722768
}
27732769
val isOnboardingComplete = withContext(dispatchers.io()) {
@@ -3950,15 +3946,15 @@ class BrowserTabViewModel @Inject constructor(
39503946
newTabPixels.get().fireNewTabDisplayed()
39513947
}
39523948

3953-
private suspend fun shouldShowPromptForBrokenSiteReport(): Boolean {
3954-
val detectedRefreshPatterns = brokenSitePrompt.getUserRefreshesCount()
3955-
refreshPixelSender.sendBreakageRefreshPixels(detectedRefreshPatterns)
3956-
if (3 in detectedRefreshPatterns) {
3957-
Timber.d("KateTest--> should show bc 3 refreshes")
3958-
return site?.url?.let { brokenSitePrompt.shouldShowBrokenSitePrompt(it) } ?: false
3959-
}
3960-
return false
3961-
}
3949+
// private suspend fun shouldShowPromptForBrokenSiteReport(): Boolean {
3950+
// val detectedRefreshPatterns = brokenSitePrompt.getUserRefreshesCount()
3951+
// refreshPixelSender.sendBreakageRefreshPixels(detectedRefreshPatterns)
3952+
// if (3 in detectedRefreshPatterns) {
3953+
// Timber.d("KateTest--> should show bc 3 refreshes")
3954+
// return site?.url?.let { brokenSitePrompt.shouldShowBrokenSitePrompt(it) } ?: false
3955+
// }
3956+
// return false
3957+
// }
39623958

39633959
fun handleMenuRefreshAction() {
39643960
refreshPixelSender.sendMenuRefreshPixels()

‎app/src/main/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSender.kt

+2-2
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ interface RefreshPixelSender {
3737
fun sendMenuRefreshPixels()
3838
fun sendCustomTabRefreshPixel()
3939
fun sendPullToRefreshPixels()
40-
fun sendBreakageRefreshPixels(patternsDetected: Set<Int>)
40+
fun onRefreshPatternDetected(patternsDetected: Set<Int>)
4141
}
4242

4343
@ContributesBinding(AppScope::class)
@@ -63,7 +63,7 @@ class DuckDuckGoRefreshPixelSender @Inject constructor(
6363
pixel.fire(CustomTabPixelNames.CUSTOM_TABS_MENU_REFRESH)
6464
}
6565

66-
override fun sendBreakageRefreshPixels(patternsDetected: Set<Int>) {
66+
override fun onRefreshPatternDetected(patternsDetected: Set<Int>) {
6767
appCoroutineScope.launch(dispatcherProvider.io()) {
6868
patternsDetected.forEach { pattern ->
6969
when (pattern) {

0 commit comments

Comments
 (0)