Bug 1805450 - Implement submit site support requests in Fenix. #1298

iorgamgabriel · 2023-03-17T15:13:47Z

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1805450

iorgamgabriel · 2023-03-21T11:19:31Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

What questions will you answer with this data?

How users interact with reporting a site domain when the cookie banner reducer is not working.

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?

This data will help us understand how the reporting a site domain feature is used.

What alternative methods did you consider to answer these questions? Why were they not sufficient?

There are no other alternatives.

Can current instrumentation answer these questions?

No.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki. 
Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description	Data Collection Category	Tracking Bug #
The user has reported a site domain	Category 2 - url	https://bugzilla.mozilla.org/show_bug.cgi?id=1805450
The user has pressed the report site domain cancel button from the cookie banner reducer details panel.	Category 2 - interaction data	https://bugzilla.mozilla.org/show_bug.cgi?id=1805450
The user has pressed the report site domain button from the cookie banner reducer details panel.	Category 2 - interaction data	https://bugzilla.mozilla.org/show_bug.cgi?id=1805450

Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.

https://dictionary.telemetry.mozilla.org/apps/fenix

How long will this data be collected? Choose one of the following:

Until version 119. Renewal then may be decided.

What populations will you measure?

All channels, all locales, all countries  that the feature is active for.

If this data collection is default on, what is the opt-out mechanism for users?

Default Glean SDK opt-out mechanism.

Please provide a general description of how you will analyze this data.

Glean and Amplitude.

Where do you intend to share the results of your analysis?

 Only on Glean, Amplitude, and with mobile teams.

Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection?

No third-party tools.

iorgamgabriel · 2023-03-21T11:22:49Z

@travis79 Can you help me please with a data review ? It's the same thing like in Focus. Thank you very much .

travis79

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

1. What questions will you answer with this data?


* How users interact with reporting a site domain when the cookie banner reducer is not working.


2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?


* This data will help us understand how the reporting a site domain feature is used.


3. What alternative methods did you consider to answer these questions? Why were they not sufficient?


* There are no other alternatives.


4. Can current instrumentation answer these questions?


* No.


5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.    
   Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description Data Collection Category Tracking Bug #
The user has reported a site domain Category 2 - url bugzilla.mozilla.org/show_bug.cgi?id=1805450
The user has pressed the report site domain cancel button from the cookie banner reducer details panel. Category 2 - interaction data bugzilla.mozilla.org/show_bug.cgi?id=1805450
The user has pressed the report site domain button from the cookie banner reducer details panel. Category 2 - interaction data bugzilla.mozilla.org/show_bug.cgi?id=1805450

6. Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.


* [dictionary.telemetry.mozilla.org/apps/fenix](https://dictionary.telemetry.mozilla.org/apps/fenix)


7. How long will this data be collected? Choose one of the following:


* Until version 119. Renewal then may be decided.


8. What populations will you measure?


* All channels, all locales, all countries    that the feature is active for.


9. If this data collection is default on, what is the opt-out mechanism for users?


* Default Glean SDK opt-out mechanism.


10. Please provide a general description of how you will analyze this data.


* Glean and Amplitude.


11. Where do you intend to share the results of your analysis?


* Only on Glean, Amplitude, and with mobile teams.


12. Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection?


* No third-party tools.

Data Review

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, through the metrics.yaml file and the Glean Dictionary.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, through the "Send Usage Data" preference in the application settings.

If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A, collection to end or be extended by version 119

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2. (The "reported_site_domain" may cross into Category 3, but it is only sent if the user explicitly opts to send it to us.)

Is the data collection request for default-on or default-off?

Default-on, but I would like to add an additional note that the reported site domain is only sent if the user opts to send it to us, similar to a crash report.

Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No

Is the data collection covered by the existing Firefox privacy notice?

Yes

Does the data collection use a third-party collection tool?

No

Result

data-review+

Amejia481 · 2023-03-22T19:16:07Z

...ponents/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

Let's try to avoid format on entire files, it make harder to understand what has been changed on a given patch.

...ponents/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

...ser/state/src/main/java/mozilla/components/browser/state/reducer/CookieBannerStateReducer.kt

.../browser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt

fenix/app/src/main/java/org/mozilla/fenix/settings/quicksettings/protections/ProtectionsView.kt

.../mozilla/fenix/settings/quicksettings/protections/cookiebanners/CookieBannerHandlingUtils.kt

fenix/app/src/main/java/org/mozilla/fenix/trackingprotection/ProtectionsStore.kt

Amejia481

Thanks for putting the PR!
Added a couple of suggestions that we need to improve.

iorgamgabriel · 2023-03-28T14:45:34Z

I made all the changes from this doc https://docs.google.com/document/d/1CstQ0eds7rPxtyYLpw4G4QKqV_qIufB4oWfDyydPVss/edit

Amejia481 · 2023-03-28T15:16:29Z

...illa/fenix/settings/quicksettings/protections/cookiebanners/CookieBannerDetailsController.kt

-                                )
-                            navController().navigate(directions)
-                        }
+                ioScope.launch(Dispatchers.Main) {


Let's also change this as we did with the other ones :)

Amejia481 · 2023-03-28T15:17:40Z

I made all the changes from this doc https://docs.google.com/document/d/1CstQ0eds7rPxtyYLpw4G4QKqV_qIufB4oWfDyydPVss/edit

Perfect, thanks for all the effort 💪🏽 , it's look really good, I'll do a round manual test today :)

fenix/app/src/main/java/org/mozilla/fenix/settings/quicksettings/QuickSettingsFragmentStore.kt

Amejia481 · 2023-03-29T22:00:10Z

fenix/app/src/main/java/org/mozilla/fenix/trackingprotection/ProtectionsStore.kt

+     * @param siteDomainToReport the site domain.
+     */
+    data class RequestReportSiteDomain(
+        val siteDomainToReport: String,


See https://github.com/mozilla-mobile/firefox-android/pull/1298/files#r1152523981

Amejia481 · 2023-03-29T22:02:54Z

fenix/app/src/main/java/org/mozilla/fenix/trackingprotection/ProtectionsStore.kt

+
+    /**
+     * SITE_NOT_SUPPORTED -  The site domain isn't added to the exceptions list,
+     * but the cookie banner reducer didn't work.


nit: Maybe we could re-word "The domain is not supported by cookie banner handling" ?

Amejia481 · 2023-03-29T22:06:51Z

fenix/app/src/main/java/org/mozilla/fenix/trackingprotection/ProtectionsStore.kt

+
+    /**
+     * REQUEST_UNSUPPORTED_SITE_SUBMITTED - The user has submitted the domain of
+     * the site where the cookie banner reducer didn't work.


Nit: We could re-word to "The user submitted a request for adding support for cookie banner handling for the domain"

Amejia481 · 2023-03-29T22:17:12Z

fenix/app/src/main/res/values-night/colors.xml

@@ -151,4 +151,7 @@
    <!-- Tab Counter colors -->
    <color name="mozac_ui_tabcounter_default_tint" tools:ignore="UnusedResources">@color/fx_mobile_icon_color_primary</color>
    <color name="mozac_ui_tabcounter_default_text" tools:ignore="UnusedResources">@color/fx_mobile_text_color_primary</color>
+
+    <!-- Cookie Banner Handling -->
+    <color name="cookie_banner_handling_details_report_a_site_text_color">#CB9EFF</color>


We don't add new colours for specific features of the UI, designers reuse the colour palette that we already defined. I think for this case we need to use Accent in Figma they include the name of the colour that we should use. @gabrielluong could add more details :).

Right, we have a design system with color variables in Fenix. So, there should be no need to add colors manually. We should be using @color/fx_mobile_text_color_accent

Amejia481 · 2023-03-29T22:19:46Z

...p/src/test/java/org/mozilla/fenix/settings/quicksettings/QuickSettingsFragmentReducerTest.kt

                listTrackers = listOf(),
                mode = Normal,
                lastAccessedCategory = "",
+                siteDomainToReport = "https://www.amazon.de",


Could you help me to clarify, I'm a bit confused about why do we need siteDomainToReport if we already have url. Do we have an use case where the user is on site and we are submitting a request for different one? Thanks in advance :)

Amejia481 · 2023-03-29T22:25:30Z

The behaviour is working like a charm, I added a couple of comments before the approve ✅ :)

Amejia481 · 2023-03-30T13:19:56Z

fenix/app/src/main/res/values-night/colors.xml

@@ -151,4 +151,7 @@
    <!-- Tab Counter colors -->
    <color name="mozac_ui_tabcounter_default_tint" tools:ignore="UnusedResources">@color/fx_mobile_icon_color_primary</color>
    <color name="mozac_ui_tabcounter_default_text" tools:ignore="UnusedResources">@color/fx_mobile_text_color_primary</color>
+
+    <!-- Cookie Banner Handling -->
+    <color name="cookie_banner_handling_details_report_a_site_text_color">@color/fx_mobile_text_color_accent</color>


Instead of adding this new colour, lets use directly in the UI `fx_mobile_text_color_accent , see #1298 (comment) for more details. Unless there is a specific use case for adding the new colour, that I maybe missing, in that case help to clarify :) ?

Done, I removed the color.

Amejia481 · 2023-03-30T13:21:27Z

fenix/app/src/main/res/values/colors.xml

@@ -340,4 +340,7 @@

    <!-- Material Design colors -->
    <color name="material_scrim_color">#52000000</color>
+
+    <!-- Cookie Banner Handling -->
+    <color name="cookie_banner_handling_details_report_a_site_text_color">@color/fx_mobile_text_color_accent</color>


Same as https://github.com/mozilla-mobile/firefox-android/pull/1298/files#r1153254413

Amejia481

Fantastic work 👏🏽 !!
Let's ship it !

iorgamgabriel · 2023-03-30T13:55:23Z

Thank you @Amejia481 and @travis79 for review !

iorgamgabriel marked this pull request as draft March 17, 2023 15:13

iorgamgabriel force-pushed the Bug_1805450 branch 4 times, most recently from 340c34c to d5f8fb4 Compare March 21, 2023 10:30

iorgamgabriel marked this pull request as ready for review March 21, 2023 11:14

iorgamgabriel force-pushed the Bug_1805450 branch from d5f8fb4 to ab92c1a Compare March 21, 2023 11:21

iorgamgabriel requested a review from Amejia481 March 21, 2023 11:21

iorgamgabriel added the 🕵️‍♂️ needs data-review PRs that need to be data-reviewed label Mar 21, 2023

iorgamgabriel requested a review from travis79 March 21, 2023 11:23

travis79 approved these changes Mar 21, 2023

View reviewed changes

travis79 removed the 🕵️‍♂️ needs data-review PRs that need to be data-reviewed label Mar 21, 2023

iorgamgabriel force-pushed the Bug_1805450 branch 2 times, most recently from 2c206e0 to eafbb65 Compare March 22, 2023 14:14