privacy protection dashboard by brindy · Pull Request #184 · duckduckgo/iOS

brindy · 2017-11-05T18:35:09Z

Reviewer: Mia
Asana: https://app.asana.com/0/414235014887631/457416080701390
CC:

Description:

Removes old content blocking popover and replaces it with a new privacy protection dashboard.

Note: Zeppelin is not the source of truth yet. Assets used were provided and the site grade in the omnibar as per discussions in Asana.

Steps to test this PR:

Test 1:

Perform a search and tap on the site grade in the omnibar while it's loading
The overview appears and shows the unknown grade with privacy protection on or off as appropriate
When the page finishes loading, the site grade is updated

Test 2:

Go to Evans Cycles and open the privacy protection dashboard
The grade should be a B
Switch privacy protection off
The dashboard shows the "protection off" version of the icons
The message "privacy protection disabled" is shown
The page reloads in the background and the grade updates once finished
The number of trackers text changes from "blocked" to "found"
Switch privacy protection on
The dashboard shows the "protection on" version of the icons
The page reloads in the background, the grade updates once finished and shows "upgraded from D to B"
Add the site to the whitelist and open the dashboard
The "privacy protection paused" message is shown

Test 3:

Go to a page and open the privacy protection dashboard
Tap the site grade and the dashboard closes

Test 4:

Go to a page and open the privacy protection dashboard
Enter a search query and submit
The privacy protection dashboard closes and shows the page loading

Test 5:

Go to a page and open the privacy protection dashboard
Tap the menu icon
The privacy protection dashboard closes and the menu appears

Test 6:

Go to duckduckgo and open the dashboard
The "good privacy practices" text is shown
The "encrypted connection" text is shown

Test 7:

Go to private.brindy.org.uk and open the dashboard
The "unknown privacy practices" text is shown
The "unencrypted connection" text is shown

Test 8:

Go to amazon.com and open the dashboard
The "some privacy practices" message is displayed

Test 9:

Visit a range of sites with various grades
Open privacy protection dashboard and ensure the right grade is displayed
Ensure the number of trackers and major trackers looks correct by comparing with the console

Reviewer Checklist:

Ensure the PR solves the problem
Review every line of code
Ensure the PR does no harm by testing the changes thoroughly
Get help if you're uncomfortable with any of the above!
Determine if there are any quick wins that improve the implementation

PR DRI Checklist:

Get advice or leverage existing code
Agree on technical approach with reviewer (if the changes are nuanced)
Ensure that there is a testing strategy (and documented non-automated tests)
Ensure there is a documented monitoring strategy (if necessary)
Consider systems implications (Database connections, Grafana stats, CPU)

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj

subsymbolic

Nice work Chris, this is coming along nicely, I've added a few comments and am ignoring assets for now as I know you're still waiting on them.

subsymbolic · 2017-11-07T10:12:11Z

    }

-    private let suitName: String
+    private let suiteName: String


subsymbolic · 2017-11-07T10:43:46Z

    }
-
+
+    func testWhenUsingBlockedOnlCacheIsNotUsed() {


subsymbolic · 2017-11-07T10:44:31Z

+        XCTAssertEqual(4, testee.siteScore(blockedOnly: false))
    }

    // Test all the adverse contions together


Typo "contions"

subsymbolic · 2017-11-07T10:48:55Z

+"privacy.protection.major.trackers.found" = "%d Major Tracker Networks Found";
+"privacy.protection.tos.unknown" = "Unknown Privacy Practices";
+"privacy.protection.tos.good" = "Good Privacy Practices";
+"privacy.protection.tos.some" = "Some Privacy Practices";


Should the "Some" be "Mixed"? https://app.asana.com/0/search/472359718529713/468767597251709

subsymbolic · 2017-11-07T11:07:07Z

        return trackersBlocked.reduce(0) { $0 + $1.value }
    }
+
+    private func majorTrackers(trackers: [Tracker: Int]) -> Int {


Same again, rename to uniqueMajorTrackerNetworks?

subsymbolic · 2017-11-07T11:08:31Z

-        return trackersDetected.contains(where: { $0.key.fromMajorNetwork } )
+
+    public var majorTrackersDetected: Int {
+        return majorTrackers(trackers: trackersDetected)


This is really a count of the unique major tracker networks detected (not a total). Maybe rename to `uniqueMajorTrackerNetworksDetected?

subsymbolic · 2017-11-07T11:09:18Z


-    public var contrainsIpTracker: Bool {
-        return trackersDetected.contains(where: { $0.key.isIpTracker } )
+    public var majorTrackersBlocked: Int {


Same as before, rename to uniqueMajorTrackerNetworksBlocked?

subsymbolic · 2017-11-07T11:11:54Z


    public func length() -> Int {
-        return characters.count
+        return count


subsymbolic · 2017-11-07T11:45:40Z

    fileprivate func onSiteRatingChanged() {
        delegate?.tab(self, didChangeSiteRating: siteRating)
-        contentBlockerPopover?.updateSiteRating(siteRating: siteRating!)
+        // contentBlockerPopover?.updateSiteRating(siteRating: siteRating!)


Delete unused code

subsymbolic · 2017-11-07T11:52:51Z

        delegate?.tab(self, didChangeSiteRating: siteRating)
-        contentBlockerPopover?.updateSiteRating(siteRating: siteRating!)
+        // contentBlockerPopover?.updateSiteRating(siteRating: siteRating!)
+        privacyDashboard?.updateSiteRating(siteRating!)


This predates your code but while this is being edited let's protect it with a let statement rather than a force unwrap and maybe dismiss or clear the privacyDashboard for the nil case?

subsymbolic · 2017-11-07T12:02:52Z

I've noticed a slight weirdness with the detection counts. We are currently counting items as detected even if we choose not to block them (maybe because they are first party). This seems odd when comparing the protect on / off results for facebook as we're telling the user that there is a tracker and then not blocking it. There was some discussion about this ages ago, you might need to check the latest algorithm that extensions are using for this

brindy · 2017-11-07T15:29:21Z

@subsymbolic that's ready for review again.

I think that's more of a UX issue tbh. It's accurate in terms of what the text says. I'll address it as part of the algorithm review task I've got.

Thanks again!

subsymbolic · 2017-11-07T15:53:36Z

-        delegate?.tab(self, didChangeSiteRating: siteRating)
-        contentBlockerPopover?.updateSiteRating(siteRating: siteRating!)
+        if let siteRating = siteRating {
+            delegate?.tab(self, didChangeSiteRating: siteRating)


didChangeSiteRating accepts an optional (as nil is a valid case) so should be outside the let check.

subsymbolic · 2017-11-07T16:08:55Z

-    public var containsMajorTracker: Bool {
-        return trackersDetected.contains(where: { $0.key.fromMajorNetwork } )
+
+    public var uniqueMajorTrackersDetected: Int {


I still think the label for these is misleading. It's the number of unique major tracker networks detected not number of trackers. Sorry! If uniqueMajorTrackerNetworksDetected is too much how about uniqueMajorNetworksDetected? Happy with any label that captures the meaning!

brindy · 2017-11-07T17:08:54Z

Back to you @subsymbolic 🏸 :)

subsymbolic

Excellent work!

brindy added 20 commits October 30, 2017 12:40

start of ui work

8095f21

show dashboard modally

73a5337

use embedded view for navigation controller

357a27a

add transition for showing privacy protection dashboard under omni bar

22a19da

add more privacy protection ui and fix a bug with transitions

584e9f9

add placeholder for toggle

2e08d63

support kerning of text

81d428f

Merge branch 'develop' into feature/privacy_protection_dashboard

6cb746b

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj

Merge branch 'develop' into feature/privacy_protection_dashboard

6fc18c4

add structure for navigating in the privacy protection screens

418a39d

add more ui setup for privacy protection

51c0092

show privacy grade and update it when it changes

ac7f3b7

add additional updates to the ui based on site rating

d204970

update trackers info on the overview screen

dd66963

update privacy practices

80802d0

disable table selections

58c73b3

show upgraded status of the protection

085a802

fix broken tests and add one for blocked only

2026072

remove unused content blocker ui

701e04e

Merge branch 'develop' into feature/privacy_protection_dashboard

132f47a

brindy requested a review from subsymbolic November 5, 2017 18:35

remove content blocking toggle from settings

0a537ac

nilnilnil assigned brindy Nov 6, 2017

subsymbolic reviewed Nov 7, 2017

View reviewed changes

address PR feedback

e589b89

subsymbolic reviewed Nov 7, 2017

View reviewed changes

more pr feedback and a rename, as discussed

1fa54b0

subsymbolic approved these changes Nov 7, 2017

View reviewed changes

brindy merged commit 65e544b into develop Nov 7, 2017

brindy deleted the feature/privacy_protection_dashboard branch November 7, 2017 20:41

NOUIY mentioned this pull request Jul 13, 2023

[Snyk] Upgrade @duckduckgo/autoconsent from 4.1.1 to 4.3.3 NOUIY/iOS#36

Open

NOUIY mentioned this pull request Aug 1, 2023

[Snyk] Upgrade @duckduckgo/autoconsent from 4.1.1 to 4.4.0 NOUIY/iOS#37

Open

Conversation

brindy commented Nov 5, 2017

Reviewer Checklist:

PR DRI Checklist:

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!

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!

Choose a reason for hiding this comment

Uh oh!

subsymbolic commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brindy commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brindy commented Nov 7, 2017

Uh oh!

subsymbolic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

subsymbolic commented Nov 7, 2017 •

edited

Loading