Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malicious Site Protection - Make clientSideHit optional parameter #1199

Conversation

alessandroboron
Copy link
Contributor

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1206329551987282/1209242004458518/f
iOS PR: duckduckgo/iOS#3891
macOS PR: duckduckgo/macos-browser#3796
What kind of version bump will this require?: Minor

Description:

This PR makes MaliciousSiteProtection.Event.errorPageShown clientSideHit parameter optional. See dicussion in Asana Task

Steps to test this PR:

  1. Ensure logic is correct

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

private func fireErrorPageShown(threatKind: ThreatKind, clientSideHit: Bool) async {
let filterSet = await dataManager.dataSet(for: .filterSet(threatKind: threatKind))
// https://app.asana.com/0/0/1209113403594297/1209141231997704/f
let sanitisedClientSideHit = filterSet.filters.count < 100 ? clientSideHit : nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the Send Pixel clientSideHit parameter only if filterSet size is greater than 100 condition is inverted here
Also please add this as a comment in addition to the asana task URL for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 good catch! I also updated the comment

Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after resolving the inline comment

@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-ios branch from 33c141b to e3520f7 Compare January 31, 2025 01:19
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-pixels-client-side-hit-param branch from 407ba36 to 40910c9 Compare January 31, 2025 01:19
@alessandroboron alessandroboron force-pushed the alessandro/malicious-site-protection-pixels-client-side-hit-param branch from d3a3e39 to 0e76b07 Compare January 31, 2025 02:32
@alessandroboron alessandroboron merged commit 02091ea into alessandro/malicious-site-protection-ios Jan 31, 2025
7 checks passed
@alessandroboron alessandroboron deleted the alessandro/malicious-site-protection-pixels-client-side-hit-param branch January 31, 2025 04:57
alessandroboron added a commit that referenced this pull request Feb 4, 2025
)

**Required**:
Task/Issue URL:
https://app.asana.com/0/1206329551987282/1209242004458518/f
iOS PR: duckduckgo/iOS#3891
macOS PR: duckduckgo/macos-browser#3796
What kind of version bump will this require?: Minor

**Description**:
This PR makes `MaliciousSiteProtection.Event.errorPageShown` `clientSideHit` parameter optional. See dicussion in [AsanaTask](https://app.asana.com/0/1206329551987282/1209242004458518/f)
alessandroboron added a commit that referenced this pull request Feb 5, 2025
)

**Required**:
Task/Issue URL:
https://app.asana.com/0/1206329551987282/1209242004458518/f
iOS PR: duckduckgo/iOS#3891
macOS PR: duckduckgo/macos-browser#3796
What kind of version bump will this require?: Minor

**Description**:
This PR makes `MaliciousSiteProtection.Event.errorPageShown` `clientSideHit` parameter optional. See dicussion in [AsanaTask](https://app.asana.com/0/1206329551987282/1209242004458518/f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants