-
Notifications
You must be signed in to change notification settings - Fork 879
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
Hide auto-contribute section in brave://settings when region is JP #11014
Hide auto-contribute section in brave://settings when region is JP #11014
Conversation
9497ccd
to
6c4a0c5
Compare
|
||
const std::string external_wallet_type = | ||
rewards_service->GetExternalWalletType(); | ||
return RespondNow(OneArgument(base::Value(external_wallet_type))); |
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:
std::string external_wallet_type = rewards_service->GetExternalWalletType();
return RespondNow(OneArgument(base::Value(std::move(external_wallet_type))));
or even:
return RespondNow(OneArgument(base::Value(rewards_service->GetExternalWalletType())));
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.
Good idea, I went with your second suggestion!
d416fee
to
93989a7
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.
In this PR, "AC supported" is derived from the "current external wallet type". Although this is status quo, we might want to think about moving away from that and just surfacing "AC supported" as its own thing from the rewards service.
The primary advantage, to me, would be that we could get rid of the idea of "current external wallet type", which isn't really meaningful in the context of an unverified user and breaks down when there are more than one supported external wallet providers.
05244f1
to
0dbe895
Compare
0dbe895
to
079dd01
Compare
Restarting Mac CI (all other platforms passed). |
All CI passed, ready to merge. |
Verified
JP region
Confirmed there was no
Non-JP region
Confirmed there was an
|
Resolves brave/brave-browser#19355
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:
JP Region
--rewards=countryid=19024
option to simulate running in JP regionNon-JP Region