-
Notifications
You must be signed in to change notification settings - Fork 985
Add support for scam protection #5853
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
Add support for scam protection #5853
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d18103b
to
68dccd5
Compare
68dccd5
to
4e93bfa
Compare
7af243e
to
f160884
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just few NITs.
We need to rebase develop to get the changes introduced with the ANR fix. After that I will re-review and test this.
@@ -153,7 +167,11 @@ class RealMaliciousSiteRepository @Inject constructor( | |||
) { | |||
val revision = latestRevision.getRevisionForFeed(feed) | |||
val data: T? = if (networkRevision > revision) { | |||
getFunction(revision) | |||
if (feed == SCAM && !maliciousSiteProtectionFeature.scamProtectionEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel this generic method is the right place to put the feature flag check. Can we do it earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to a different place, but it's not super relevant, as #5857 deals with this in a different way
@@ -103,13 +109,17 @@ class RealMaliciousSiteProtection @Inject constructor( | |||
(hostname == match.hostname) && | |||
(hash == match.hash) | |||
}?.feed?.let { feed: Feed -> | |||
Malicious(feed) | |||
if (feed != SCAM || maliciousSiteProtectionRCFeature.scamProtectionEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think it will be more readable (and easy to expand) if we move it as separate early checks
For example:
if (feed == SCAM && !maliciousSiteProtectionRCFeature.scamProtectionEnabled()) return@let Ignored
return@let Malicious(feed)
f160884
to
4a0805e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
b89d6bf
to
346d097
Compare
Translate strings to values-et
Task/Issue URL: https://app.asana.com/0/0/1209879813659870
Description
Steps to test this PR
Feature 1
revisions
table inmalicious_sites
dbFeature 2
@Toggle.DefaultValue(true)
forscamProtection
revisions
table inmalicious_sites
dbm_malicious-site-protection_error-page-shown
is fired withcategory=scam
m_malicious-site-protection_visit-site
is fired withcategory=scam
scamProtection
under Feature Flag Inventory and restart the appFrom #5857
Feature 1
revisions
table inmalicious_sites
dbFeature 2
scamProtection
under Feature Flag Inventory and restart the apprevisions
table inmalicious_sites
dbscamProtection
under Feature Flag Inventory and restart the appUI changes