-
Notifications
You must be signed in to change notification settings - Fork 862
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
Enable Brave ads for Australia, New Zealand and Ireland #2865
Conversation
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, does it require any test updates?
b1fea9d
to
7db2483
Compare
Tests are covered under english user models |
49d30c8
to
86a4b6b
Compare
72cf61b
to
0bea9a4
Compare
232e7af
to
e23eb4e
Compare
|
||
auto* rewards_service = | ||
brave_rewards::RewardsServiceFactory::GetForProfile(profile_); | ||
auto* rewards_notification_service = |
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 need to check if rewards_service
is null
brave_rewards::RewardsServiceFactory::GetForProfile(profile_); | ||
auto* rewards_notification_service = | ||
rewards_service->GetNotificationService(); | ||
rewards_notification_service->DeleteNotification( |
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 need to check if rewards_notification_service
is null
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.
Code wise looks good to me, CI is also passing
Fixes brave/brave-browser#5153
Requires #2928
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.