From 1c91b0613a1fda0226beb53628ecabf8be9a4f92 Mon Sep 17 00:00:00 2001 From: Olivia Hall Date: Tue, 11 Jul 2023 10:12:17 -0400 Subject: [PATCH] Bug 1837517 - Additional Print Telmetry and Nimbus setup This bug adds telemetry for the print from menu feature. It adds the probes print_failure, print_completed, print_tapped (for both share and browsr menu), and share_menu_action for the print button on the share sheet only. browser_menu_action for the print button was added in bug 1836780. Additionally, Nimbus control for the browser print button and share print button was added as browser-print-enabled and share-print-enabled. --- app/.experimenter.yaml | 11 ++ app/metrics.yaml | 89 +++++++++++ app/nimbus.fml.yaml | 12 ++ .../fenix/ui/robots/ThreeDotMenuMainRobot.kt | 3 +- .../components/toolbar/DefaultToolbarMenu.kt | 4 +- .../fenix/share/SaveToPDFMiddleware.kt | 142 ++++++++++++------ .../mozilla/fenix/share/ShareController.kt | 1 + .../org/mozilla/fenix/share/ShareFragment.kt | 4 +- .../fenix/share/SaveToPDFMiddlewareTest.kt | 132 ++++++++++++++++ .../fenix/share/ShareControllerTest.kt | 7 +- 10 files changed, 358 insertions(+), 47 deletions(-) diff --git a/app/.experimenter.yaml b/app/.experimenter.yaml index d238244465..0b51d040d9 100644 --- a/app/.experimenter.yaml +++ b/app/.experimenter.yaml @@ -125,6 +125,17 @@ pre-permission-notification-prompt: enabled: type: boolean description: "if true, the pre-permission notification prompt is shown to the user." +print: + description: A feature for printing from the share or browser menu. + hasExposure: true + exposureDescription: "" + variables: + browser-print-enabled: + type: boolean + description: "If true, a print button from the browser menu is available." + share-print-enabled: + type: boolean + description: "If true, a print button from the share menu is available." re-engagement-notification: description: A feature that shows the re-engagement notification if the user is inactive. hasExposure: true diff --git a/app/metrics.yaml b/app/metrics.yaml index 2283709a86..ba3a524134 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -563,6 +563,95 @@ events: metadata: tags: - Sharing + print_failure: + type: event + description: | + A user tapped the print in the menu or share sheet but an error occurred + and the process failed. + extra_keys: + source: + type: string + description: | + A string that indicates the type of document of pdf, non-pdf or unknown. + The default is unknown. + reason: + type: string + description: | + An error occurred while setting up for printing. + Default option is unknown, other options are no_settings_service, no_settings, + no_canonical_context, no_activity_context_delegate, no_activity_context, + no_print_delegate, and io_error. + bugs: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517#c3 + data_sensitivity: + - technical + notification_emails: + - android-probes@mozilla.com + expires: 122 + print_completed: + type: event + description: | + Printing from the share sheet or menu successfully completed. + extra_keys: + source: + type: string + description: | + A string that indicates the type of document of pdf, non-pdf or unknown. + The default is unknown. + bugs: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517#c3 + data_sensitivity: + - technical + notification_emails: + - android-probes@mozilla.com + expires: 122 + print_tapped: + type: event + description: | + A user tapped the print option in the menu or share sheet. + extra_keys: + source: + type: string + description: | + A string that indicates the type of document of pdf, non-pdf or unknown. + The default is unknown. + bugs: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517#c5 + data_sensitivity: + - interaction + notification_emails: + - android-probes@mozilla.com + expires: 122 + share_menu_action: + type: event + description: | + A share menu item was tapped. + The name of the item that the user tapped is stored in extras with the + key `item`. + extra_keys: + item: + description: | + A string containing the name of the item the user tapped. These items + currently include: print + type: string + bugs: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1837517#c5 + data_sensitivity: + - interaction + notification_emails: + - android-probes@mozilla.com + expires: 122 + metadata: + tags: + - Sharing share_to_app: type: event description: | diff --git a/app/nimbus.fml.yaml b/app/nimbus.fml.yaml index dacd4bfc73..71bd89ba33 100644 --- a/app/nimbus.fml.yaml +++ b/app/nimbus.fml.yaml @@ -342,6 +342,18 @@ features: value: enabled: true + print: + description: A feature for printing from the share or browser menu. + variables: + share-print-enabled: + description: If true, a print button from the share menu is available. + type: Boolean + default: true + browser-print-enabled: + description: If true, a print button from the browser menu is available. + type: Boolean + default: true + search-extra-params: description: A feature that provides additional args for search. variables: diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuMainRobot.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuMainRobot.kt index 44ee9a9479..73eb16c22d 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuMainRobot.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuMainRobot.kt @@ -47,6 +47,7 @@ import org.mozilla.fenix.helpers.TestHelper.mDevice import org.mozilla.fenix.helpers.TestHelper.packageName import org.mozilla.fenix.helpers.click import org.mozilla.fenix.helpers.ext.waitNotNull +import org.mozilla.fenix.nimbus.FxNimbus /** * Implementation of Robot Pattern for the three dot (main) menu. @@ -110,7 +111,7 @@ class ThreeDotMenuMainRobot { assertItemContainingTextExists( settingsButton(), ) - if (FeatureFlags.print) { + if (FeatureFlags.print && FxNimbus.features.print.value().browserPrintEnabled) { assertItemContainingTextExists(printContentButton) } assertItemWithDescriptionExists( diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt index 3ed4a72614..2fbb407e12 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt @@ -40,6 +40,7 @@ import org.mozilla.fenix.R import org.mozilla.fenix.components.accounts.FenixAccountManager import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.nimbus.FxNimbus import org.mozilla.fenix.theme.ThemeManager /** @@ -78,6 +79,7 @@ open class DefaultToolbarMenu( get() = store.state.selectedTab override val menuBuilder by lazy { + FxNimbus.features.print.recordExposure() WebExtensionBrowserMenuBuilder( items = coreMenuItems, endOfMenuAlwaysVisible = shouldUseBottomToolbar, @@ -391,7 +393,7 @@ open class DefaultToolbarMenu( installToHomescreen.apply { visible = ::canInstall }, if (shouldShowTopSites) addRemoveTopSitesItem else null, saveToCollectionItem, - if (FeatureFlags.print) printPageItem else null, + if (FeatureFlags.print && FxNimbus.features.print.value().browserPrintEnabled) printPageItem else null, BrowserMenuDivider(), settingsItem, if (shouldDeleteDataOnQuit) deleteDataOnQuit else null, diff --git a/app/src/main/java/org/mozilla/fenix/share/SaveToPDFMiddleware.kt b/app/src/main/java/org/mozilla/fenix/share/SaveToPDFMiddleware.kt index e8ae89af98..e040eb3e37 100644 --- a/app/src/main/java/org/mozilla/fenix/share/SaveToPDFMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/share/SaveToPDFMiddleware.kt @@ -48,13 +48,13 @@ class SaveToPDFMiddleware( ) { when (action) { is EngineAction.SaveToPdfAction -> { - postTelemetryTapped(ctx.state.findTab(action.tabId)) + postTelemetryTapped(ctx.state.findTab(action.tabId), isPrint = false) // Continue to generate the PDF, passing through here to add telemetry next(action) } is EngineAction.SaveToPdfCompleteAction -> { - postTelemetryCompleted(ctx.state.findTab(action.tabId)) + postTelemetryCompleted(ctx.state.findTab(action.tabId), isPrint = false) } is EngineAction.SaveToPdfExceptionAction -> { @@ -64,21 +64,24 @@ class SaveToPDFMiddleware( Toast.makeText(context, R.string.unable_to_save_to_pdf_error, LENGTH_LONG).show() } - postTelemetryFailed(ctx.state.findTab(action.tabId), action.throwable) + postTelemetryFailed(ctx.state.findTab(action.tabId), action.throwable, isPrint = false) } is EngineAction.PrintContentAction -> { + postTelemetryTapped(ctx.state.findTab(action.tabId), isPrint = true) + // Continue to print, passing through here to add telemetry next(action) - // Reserved for telemetry in bug 1837517 } is EngineAction.PrintContentCompletedAction -> { - // No-op, reserved for telemetry in bug 1837517 + postTelemetryCompleted(ctx.state.findTab(action.tabId), isPrint = true) } is EngineAction.PrintContentExceptionAction -> { // Bug 1840894 - will update this toast to a snackbar with new snackbar error component ThreadUtils.runOnUiThread { Toast.makeText(context, R.string.unable_to_print_error, LENGTH_LONG).show() } + + postTelemetryFailed(ctx.state.findTab(action.tabId), action.throwable, isPrint = true) } else -> { next(action) @@ -130,82 +133,135 @@ class SaveToPDFMiddleware( } /** - * Indicates the Save As PDF action was requested and posts telemetry via Glean. + * Indicates the Print or Save As PDF action was requested and posts telemetry via Glean. * + * @param isPrint - if the telemetry is for printing * @param tab - tab state to use for page source category */ - private fun postTelemetryTapped(tab: TabSessionState?) { + private fun postTelemetryTapped(tab: TabSessionState?, isPrint: Boolean) { mainScope.launch { tab?.engineState?.engineSession?.checkForPdfViewer( onResult = { isPdf -> - Events.saveToPdfTapped.record( - Events.SaveToPdfTappedExtra( - source = telemetrySource(isPdf), - ), - ) + if (isPrint) { + Events.printTapped.record( + Events.PrintTappedExtra( + source = telemetrySource(isPdf), + ), + ) + } else { + Events.saveToPdfTapped.record( + Events.SaveToPdfTappedExtra( + source = telemetrySource(isPdf), + ), + ) + } }, onException = { - Events.saveToPdfTapped.record( - Events.SaveToPdfTappedExtra( - source = telemetrySource(null), - ), - ) + if (isPrint) { + Events.printTapped.record( + Events.PrintTappedExtra( + source = telemetrySource(null), + ), + ) + } else { + Events.saveToPdfTapped.record( + Events.SaveToPdfTappedExtra( + source = telemetrySource(null), + ), + ) + } }, ) } } /** - * Indicates the Save As PDF action completed and generated a PDF and posts telemetry via Glean. + * Indicates the Print or Save As PDF action completed and generated a PDF and posts telemetry via Glean. * + * @param isPrint - if the telemetry is for printing * @param tab - tab state to use for page source category */ - private fun postTelemetryCompleted(tab: TabSessionState?) { + private fun postTelemetryCompleted(tab: TabSessionState?, isPrint: Boolean) { mainScope.launch { tab?.engineState?.engineSession?.checkForPdfViewer( onResult = { isPdf -> - Events.saveToPdfCompleted.record( - Events.SaveToPdfCompletedExtra( - source = telemetrySource(isPdf), - ), - ) + if (isPrint) { + Events.printCompleted.record( + Events.PrintCompletedExtra( + source = telemetrySource(isPdf), + ), + ) + } else { + Events.saveToPdfCompleted.record( + Events.SaveToPdfCompletedExtra( + source = telemetrySource(isPdf), + ), + ) + } }, onException = { - Events.saveToPdfCompleted.record( - Events.SaveToPdfCompletedExtra( - source = telemetrySource(null), - ), - ) + if (isPrint) { + Events.printCompleted.record( + Events.PrintCompletedExtra( + source = telemetrySource(null), + ), + ) + } else { + Events.saveToPdfCompleted.record( + Events.SaveToPdfCompletedExtra( + source = telemetrySource(null), + ), + ) + } }, ) } } /** - * Indicates the Save As PDF action failed and the reason for failure and posts telemetry via Glean. + * Indicates the Print or Save As PDF action failed and the reason for failure and posts telemetry via Glean. * + * @param isPrint - if the telemetry is for printing * @param tab - tab state to use for page source category * @param throwable - failure state to use for failure reason category */ - private fun postTelemetryFailed(tab: TabSessionState?, throwable: Throwable) { + private fun postTelemetryFailed(tab: TabSessionState?, throwable: Throwable, isPrint: Boolean) { val telFailureReason = telemetryErrorReason(throwable as Exception) mainScope.launch { tab?.engineState?.engineSession?.checkForPdfViewer( onResult = { isPdf -> - Events.saveToPdfFailure.record( - Events.SaveToPdfFailureExtra( - source = telemetrySource(isPdf), - reason = telFailureReason, - ), - ) + if (isPrint) { + Events.printFailure.record( + Events.PrintFailureExtra( + source = telemetrySource(isPdf), + reason = telFailureReason, + ), + ) + } else { + Events.saveToPdfFailure.record( + Events.SaveToPdfFailureExtra( + source = telemetrySource(isPdf), + reason = telFailureReason, + ), + ) + } }, onException = { - Events.saveToPdfFailure.record( - Events.SaveToPdfFailureExtra( - source = telemetrySource(null), - reason = telFailureReason, - ), - ) + if (isPrint) { + Events.printFailure.record( + Events.PrintFailureExtra( + source = telemetrySource(null), + reason = telFailureReason, + ), + ) + } else { + Events.saveToPdfFailure.record( + Events.SaveToPdfFailureExtra( + source = telemetrySource(null), + reason = telFailureReason, + ), + ) + } }, ) } diff --git a/app/src/main/java/org/mozilla/fenix/share/ShareController.kt b/app/src/main/java/org/mozilla/fenix/share/ShareController.kt index c43b555992..2ac1a18df1 100644 --- a/app/src/main/java/org/mozilla/fenix/share/ShareController.kt +++ b/app/src/main/java/org/mozilla/fenix/share/ShareController.kt @@ -157,6 +157,7 @@ class DefaultShareController( } override fun handlePrint(tabId: String?) { + Events.shareMenuAction.record(Events.ShareMenuActionExtra("print")) handleShareClosed() printUseCase.invoke(tabId) } diff --git a/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt b/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt index 5a66baf7f9..da28d9b588 100644 --- a/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt @@ -30,6 +30,7 @@ import org.mozilla.fenix.databinding.FragmentShareBinding import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.getRootView import org.mozilla.fenix.ext.requireComponents +import org.mozilla.fenix.nimbus.FxNimbus import org.mozilla.fenix.theme.FirefoxTheme import org.mozilla.fenix.theme.Theme @@ -132,7 +133,8 @@ class ShareFragment : AppCompatDialogFragment() { } } - if (FeatureFlags.print) { + FxNimbus.features.print.recordExposure() + if (FeatureFlags.print && FxNimbus.features.print.value().sharePrintEnabled) { binding.print.setContent { FirefoxTheme(theme = Theme.getTheme(allowPrivateTheme = false)) { PrintItem { diff --git a/app/src/test/java/org/mozilla/fenix/share/SaveToPDFMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/share/SaveToPDFMiddlewareTest.kt index b20425afcc..2a931b83c7 100644 --- a/app/src/test/java/org/mozilla/fenix/share/SaveToPDFMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/share/SaveToPDFMiddlewareTest.kt @@ -225,4 +225,136 @@ class SaveToPDFMiddlewareTest { assertEquals("non-pdf", middleware.telemetrySource(isPdfViewer = false)) assertEquals("unknown", middleware.telemetrySource(isPdfViewer = null)) } + + @Test + fun `GIVEN a print request WHEN it fails unexpectedly THEN unknown failure telemetry is sent`() = runTestOnMain { + val exceptionToThrow = RuntimeException("No Print Spooler") + val middleware = SaveToPDFMiddleware(testContext) + val mockEngineSession: EngineSession = mockk().apply { + every { + checkForPdfViewer(any(), any()) + } answers { + secondArg<(Throwable) -> Unit>().invoke(exceptionToThrow) + } + } + val browserStore = BrowserStore( + middleware = listOf(middleware), + initialState = BrowserState( + tabs = listOf( + createTab( + url = "https://mozilla.org", + id = "14", + engineSession = mockEngineSession, + ), + ), + ), + ) + browserStore.dispatch( + EngineAction.PrintContentExceptionAction("14", true, exceptionToThrow), + ) + browserStore.waitUntilIdle() + testScheduler.advanceUntilIdle() + val response = Events.printFailure.testGetValue()?.firstOrNull() + assertNotNull(response) + val reason = response?.extra?.get("reason") + assertEquals("unknown", reason) + val source = response?.extra?.get("source") + assertEquals("unknown", source) + } + + @Test + fun `GIVEN a print request WHEN it fails due to print exception THEN print exception failure telemetry is sent`() = runTestOnMain { + val exceptionToThrow = MockGeckoPrintException() + val middleware = SaveToPDFMiddleware(testContext) + val mockEngineSession: EngineSession = mockk().apply { + every { + checkForPdfViewer(any(), any()) + } answers { + secondArg<(Throwable) -> Unit>().invoke(exceptionToThrow) + } + } + val browserStore = BrowserStore( + middleware = listOf(middleware), + initialState = BrowserState( + tabs = listOf( + createTab( + url = "https://mozilla.org", + id = "14", + engineSession = mockEngineSession, + ), + ), + ), + ) + browserStore.dispatch(EngineAction.PrintContentExceptionAction("14", true, exceptionToThrow)) + browserStore.waitUntilIdle() + testScheduler.advanceUntilIdle() + val response = Events.printFailure.testGetValue()?.firstOrNull() + assertNotNull(response) + val reason = response?.extra?.get("reason") + assertEquals("no_settings_service", reason) + val source = response?.extra?.get("source") + assertEquals("unknown", source) + } + + @Test + fun `GIVEN a print request WHEN it completes THEN completed telemetry is sent`() = runTestOnMain { + val middleware = SaveToPDFMiddleware(testContext) + val mockEngineSession: EngineSession = mockk().apply { + every { + checkForPdfViewer(any(), any()) + } answers { + firstArg<(Boolean) -> Unit>().invoke(true) + } + } + val browserStore = BrowserStore( + middleware = listOf(middleware), + initialState = BrowserState( + tabs = listOf( + createTab( + url = "https://mozilla.org", + id = "14", + engineSession = mockEngineSession, + ), + ), + ), + ) + browserStore.dispatch(EngineAction.PrintContentCompletedAction("14")) + browserStore.waitUntilIdle() + testScheduler.advanceUntilIdle() + val response = Events.printCompleted.testGetValue() + assertNotNull(response) + val source = response?.firstOrNull()?.extra?.get("source") + assertEquals("pdf", source) + } + + @Test + fun `GIVEN a print request WHEN it the action begins THEN tapped telemetry is sent`() = runTestOnMain { + val middleware = SaveToPDFMiddleware(testContext) + val mockEngineSession: EngineSession = mockk().apply { + every { + checkForPdfViewer(any(), any()) + } answers { + firstArg<(Boolean) -> Unit>().invoke(false) + } + } + val browserStore = BrowserStore( + middleware = listOf(middleware), + initialState = BrowserState( + tabs = listOf( + createTab( + url = "https://mozilla.org", + id = "14", + engineSession = mockEngineSession, + ), + ), + ), + ) + browserStore.dispatch(EngineAction.PrintContentAction("14")) + browserStore.waitUntilIdle() + testScheduler.advanceUntilIdle() + val response = Events.printTapped.testGetValue() + assertNotNull(response) + val source = response?.firstOrNull()?.extra?.get("source") + assertEquals("non-pdf", source) + } } diff --git a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt index 17e9f91eea..a997014044 100644 --- a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt @@ -293,7 +293,7 @@ class ShareControllerTest { } @Test - fun `WHEN handlePrint close the dialog and print the page`() { + fun `WHEN handlePrint close the dialog and print the page AND send tapped telemetry`() { val testController = DefaultShareController( context = mockk(), shareSubject = shareSubject, @@ -315,6 +315,11 @@ class ShareControllerTest { printUseCase.invoke("tabID") dismiss(ShareController.Result.DISMISSED) } + + assertNotNull(Events.shareMenuAction.testGetValue()) + val printTapped = Events.shareMenuAction.testGetValue()!! + assertEquals(1, printTapped.size) + assertEquals("print", printTapped.single().extra?.getValue("item")) } @Test