-
Notifications
You must be signed in to change notification settings - Fork 901
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
Brave Ads adaptive captcha support #9350
Conversation
b2421af
to
8388270
Compare
b3a328d
to
f298115
Compare
f87259b
to
74f4fdf
Compare
|
||
#include <string> | ||
|
||
#include "brave/components/brave_ads/browser/ads_tooltips_delegate.h" |
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.
You're including the file as a header for itself
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.
Oops, removed.
if (brave_adaptive_captcha_enabled) { | ||
sources += [ "ads_tooltips_delegate.h" ] | ||
deps += | ||
[ "//brave/components/brave_adaptive_captcha:brave_adaptive_captcha" ] |
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.
this file does not need a dep on brave_adaptive_captcha
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.
Removed.
vendor/bat-native-ledger/BUILD.gn
Outdated
visibility = [ ":*" ] | ||
visibility = [ | ||
":*", | ||
"//brave/vendor/bat-native-ads:ads", |
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.
these visibility lists exist to prevent people from doing this. This config is accessed as a public config on headers
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.
we probably want to split these defines out into a separate config that is allowed from //brave/vendor/bat-native-ads:ads
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.
they really shouldn't be in a public config anyway because GEMINI_OAUTH_STAGING_URL
and BITFLYER_STAGING_URL
aren't used outside of bat-native-ledger. The strictly internal ones above should go in the internal config and the new ones should go in a new config that has the :*
and //brave/vendor/bat-native-ads:ads
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.
OK, I think I got misled a bit by the name external_config
. Moved these into endpoints_config
.
vendor/bat-native-ads/BUILD.gn
Outdated
configs += [ ":internal_config" ] | ||
configs += [ | ||
":internal_config", | ||
"//brave/vendor/bat-native-ledger:external_config", |
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.
see above
ef6862f
to
75fd261
Compare
vendor/bat-native-ledger/BUILD.gn
Outdated
config("endpoints_config") { | ||
visibility = [ | ||
":*", | ||
"//brave/components/brave_adaptive_captcha:brave_adaptive_captcha", |
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.
why is this needed in more places now? It can still be a public config
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.
Made it a public config.
e3ab1a7
to
6f7b730
Compare
6f7b730
to
362d610
Compare
All CI passed (Windows had an unrelated test failure). |
Resolves brave/brave-browser#15600
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Setup
When testing on the staging server, run Brave with the following flags (plus whatever else you may need for testing):
You'll need to schedule a captcha for a particular payment ID. To do that, you can issue the following command from your command line, where
{paymentId}
represents the payment ID (wallet ID) as identified in brave://rewards-internals:Please ping me via DM for the
{authToken}
required above.The
type
parameter allows you to specify the type of captcha to schedule. This may be either0
or1
(the default is0
if this parameter is omitted).Note: Any time that you successfully solve a captcha for this payment ID, you'll need to schedule a new one using this command.
Solving the captcha
Snoozing the captcha