Update privacy toggle to be per site #811

subsymbolic · 2020-05-04T18:31:14Z

Task/Issue URL: https://app.asana.com/0/72649045549333/559623300949085

Steps to test this PR:

TEST 1: Test new toggle text and behavior

Navigate to a page e.g cnn.com
Open the privacy dashboard and ensure that the toggle text is "Site Privacy Protection"
Switch privacy protection off and press back
Ensure page reloads
Ensure page is now in whitelist
Ensure that mp_wla pixel is fired
Navigate to another page e.g facebook.com
Check that privacy toggle is on
Navigate back cnn.com and ensure that the privacy/site protection toggle is back off
Switch the toggle back on
Ensure that mp_wlr pixel is fired
Ensure page is no longer in whitelist

TEST 2: Test new buttons

Navigate to a page and open the privacy Dashboard
Tap the Manage Whitelist button and ensure whitelist page is launched
Ensure the "mp_mw" pixel fires
Go back
Tap the Report Broken Site button and ensure that the broken site button in launched
Ensure the "mp_rb" pixel fires

TEST 3: Test new pixels browsing pixels

Browse to a site
From the browsing menu, add it to the whitelist and ensure that "mb_wla" fires
From the browsing menu, remove it from the whitelist and ensure that "mb_wlr" fires

TEST 4: Test new whitelist settings pixel

Open Settings
Tap Privacy Protection Whitelist
Ensure "ms_mw" fires

TEST 5: Test whitelist page

Open Settings
Tap Privacy Protection Whitelist
Add, edit and delete entries and ensure these behave as expected
Go back to settings and switch theme
Now repeat step 3 to ensure UI still works as expected (note dialogs are universally white as per bookmarks UI)
Repeat this test on API 21`

TEST 6: Test database migration

Cleanly install the app from develop and run the app to ensure the db is initialized
No install this branch over the top to ensure
Run the app and ensure the migration succeeds without anything breaking

Internal references:

Software Engineering Expectations
Technical Design Template

# Conflicts: # app/schemas/com.duckduckgo.app.global.db.AppDatabase/19.json # app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/java/com/duckduckgo/app/cta/ui/CtaViewModel.kt # app/src/main/java/com/duckduckgo/app/global/db/AppDatabase.kt # app/src/main/java/com/duckduckgo/app/privacy/ui/PrivacyDashboardActivity.kt # app/src/main/java/com/duckduckgo/app/settings/SettingsActivity.kt # app/src/main/res/values/string-untranslated.xml

# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

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

app/src/androidTest/java/com/duckduckgo/app/brokensite/BrokenSiteDataTest.kt

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

# Conflicts: # app/src/main/res/layout/view_bookmark_entry.xml

cmonfortep

Only tested the feature (not reviewed the code), I found some small things:

Testing on 21-23 API: overflow icon color in whitelist page is rendered with pink color, we should tint the imageview.
Testing with destroy activities enabled: Having a whitelisted site, every time I visit "whitelist page" or "report broken site" or "trackers found" from the privacy dashboard and then I going back to privacy dashboard screen, mp_wla is sent without any interaction.
Every time I visit privacy dashboard from a whitelisted site, I see the toggle moving from enabled to disabled, I guess it's related with some async query. But the top card image is rendered correctly from the beginning, so wondering if we can avoid that UI distraction.

Open questions:

Minor thing, but if I go to privacy dashboard and I switch the toggle twice (interacting but keeping the original state), and going back to the browser, page will reload. is that expected?
If I visit a website -> then I whitelist that site from settings -> I go back to the browser -> website doesn't reload. Is that something we expect? or should we detect the change and reload the site?
When switching the site privacy protection toggle, should we change somehow the privacy grade to the user? I know it's not possible, but maybe as a future feedback. Currently, I feel like I don't receive any feedback on what's happening.
Noticed we removed the trackers statistics from the privacy dashboard. I still haven't checked the code, but it's that something related to this feature?

app/src/main/res/layout/view_whitelist_entry.xml

# Conflicts: # app/src/main/res/drawable/ic_overflow.xml # app/src/main/res/drawable/ic_overflow_24dp.xml # app/src/main/res/drawable/ic_overflow_bookmarks_24dp.xml # app/src/main/res/layout/layout_browser_bottom_navigation_bar.xml # app/src/main/res/layout/layout_tabs_bottom_navigation_bar.xml # app/src/main/res/layout/view_bookmark_entry.xml

…m multiple threads

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

subsymbolic · 2020-05-08T14:18:05Z

Overflow icon color in whitelist page is rendered with pink color

Thanks! This was introduced by another PR and has subsequently been fixed globally by #814.

Testing with destroy activities enabled: Having a whitelisted site, every time I visit "whitelist page" or "report broken site" or "trackers found"...mp_wla is sent without any interaction

Good catch! Fixed.

Every time I visit privacy dashboard from a whitelisted site, I see the toggle moving from enabled to disabled

The visible ui update as the user enters the screen is preexisting behavior. We have had no feedback about and as we expect to either completely redesign or even delete this screen we have no plans to fix it.

If I go to privacy dashboard and I switch the toggle twice (interacting but keeping the original state), and going back to the browser, page will reload. is that expected?

Yes. It's it is an easier implementation this way and I don't think users will find it odd that the page reloads after they have interacted with the toggle. I like your idea of being smarter and only reloading when the value has truly changed (on an odd number of toggles) however I don't think it's worth investing in that level of polish when we are soon deprecating this screen. If we receive feedback that this is unacceptable, we can reconsider. Let me know if you feel strongly otherwise.

If I visit a website -> then I whitelist that site from settings -> I go back to the browser -> website doesn't reload. Is that something we expect? or should we detect the change and reload the site?

Yes this is expected. If the user is in the context of a site and they toggle protection either through the dash toggle or the whitelist menu button then we reload. If the go to whitelists screen via settings however and start adding and removing entries, the user is now outside the context of the current page. We could make this fancier but it also adds complexity. Let me know if you feel strongly about this though.

When switching the site privacy protection toggle, should we change somehow the privacy grade to the user

Again this is preexisting behavior and as we expect to be deprecate this screen this isn't something we plan to change right now.

Noticed we removed the trackers statistics from the privacy dashboard

Oh no that shouldn't happen, thanks for catching it. Fixed! Beware though there is another outstanding issue in develop in this area due to a change from percentage to ratio for prevalence in the backend API. To see any results you'll need to update Entiry.kt to have MAJOR_NETWORK_PREVALENCE = 0.7

cmonfortep

This is looking good, all are minor comments or suggestions, nothing blocking. There's only one question from my side about GlobalScope that I would like to see what @CDRussell thinks about.

Testing wise: I only found one small issue, already explained in one of the comments.

app/src/androidTest/java/com/duckduckgo/app/httpsupgrade/HttpsUpgraderTest.kt

app/src/androidTest/java/com/duckduckgo/app/privacy/db/UserWhitelistDaoTest.kt

app/src/main/java/com/duckduckgo/app/bookmarks/ui/BookmarksActivity.kt

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

app/src/main/java/com/duckduckgo/app/privacy/ui/ScorecardViewModel.kt

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

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

…ing async nature

…mp back to main thread to avoid posting live data

…her small tidy ups

app/src/main/java/com/duckduckgo/app/privacy/ui/WhitelistViewModel.kt

cmonfortep

Thanks for the tidy up and the work you did here. Tested on API 21, 23, 29. All working as described. 🚀

subsymbolic · 2020-05-12T15:38:07Z

Thanks for the awesome review 🙏

subsymbolic added 9 commits April 24, 2020 00:04

Delete privacy store add whitelist functionality and update toggle

353ecf3

Update pixels

263c9f3

New buttons

94398b6

Add whitelist

2ccff7d

Add db migration test

e374aef

Add browser menu item, error messages and more tests

0ae138a

Merge branch 'develop' into feature/mia/toggle

ae745be

# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

Lint tidy up

adfb5f2

subsymbolic commented May 4, 2020

View reviewed changes

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt Show resolved Hide resolved

app/src/androidTest/java/com/duckduckgo/app/brokensite/BrokenSiteDataTest.kt Show resolved Hide resolved

app/src/main/java/com/duckduckgo/app/global/UriString.kt Show resolved Hide resolved

subsymbolic marked this pull request as ready for review May 4, 2020 20:19

subsymbolic requested review from CDRussell, cmonfortep, malmstein and marcosholgado as code owners May 4, 2020 20:19

cmonfortep self-assigned this May 5, 2020

Merge branch 'develop' into feature/mia/toggle

e276c1d

# Conflicts: # app/src/main/res/layout/view_bookmark_entry.xml

cmonfortep reviewed May 6, 2020

View reviewed changes

app/src/main/res/layout/view_whitelist_entry.xml Outdated Show resolved Hide resolved

subsymbolic added 5 commits May 6, 2020 10:09

Add icon tint

bf5197d

Fix double pixel when activities not kept

7b271e7

Remove mixed viewstate set and post value and access of viewState fro…

528924c

…m multiple threads

Merge branch 'develop' into feature/mia/toggle

1ccd425

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

cmonfortep reviewed May 11, 2020

View reviewed changes

marcosholgado reviewed May 11, 2020

View reviewed changes

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt Outdated Show resolved Hide resolved

subsymbolic added 4 commits May 11, 2020 14:01

Update test to also check pixel

8bdbf4a

Cleanup unneeded annotations and client definition

356b1d8

Make db writes suspend functions to avoid future use without consider…

7e18623

…ing async nature

Update coroutines to use suspend functions for private methods and ju…

98e88f1

…mp back to main thread to avoid posting live data

Extract repeated isWhitelisted check into own suspect function and ot…

6b9d6d6

…her small tidy ups

subsymbolic commented May 11, 2020

View reviewed changes

app/src/main/java/com/duckduckgo/app/privacy/ui/WhitelistViewModel.kt Show resolved Hide resolved

subsymbolic requested a review from cmonfortep May 11, 2020 21:40

Merge branch 'develop' into feature/mia/toggle

fb9a63c

cmonfortep approved these changes May 12, 2020

View reviewed changes

subsymbolic merged commit 4fc5b1f into develop May 12, 2020

subsymbolic deleted the feature/mia/toggle branch May 12, 2020 15:37

Update privacy toggle to be per site #811

Update privacy toggle to be per site #811

Uh oh!

Conversation

subsymbolic commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Internal references:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

subsymbolic commented May 8, 2020

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

subsymbolic commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

subsymbolic commented May 4, 2020 •

edited

Loading