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.

Sign up for GitHub

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

Jump to bottom

Decouple business logic for newly supported regions into ads library #3805

Merged
merged 2 commits into from
Nov 4, 2019
Merged

Decouple business logic for newly supported regions into ads library #3805

merged 2 commits into from
Nov 4, 2019

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Oct 29, 2019 •
edited
Loading

Fixes brave/brave-browser#6612

Submitter Checklist:

Test Plan:

  • Confirm ad notifications are displayed if ads are enabled
  • Confirm ad notifications are not displayed if ads are disabled
  • Confirm ads can be disabled and then re-enabled
  • Confirm ads work as expected for a fresh install on Android
  • Confirm ads work as expected for upgrade paths on Android
  • Confirm ads work as expected for a fresh install on Desktop
  • Confirm ads work as expected for upgrade paths on Desktop

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey self-assigned this Oct 29, 2019
@tmancey tmancey force-pushed the issues/6612 branch 7 times, most recently from 9bb0406 to c29f0df Compare October 29, 2019 15:11
@SergeyZhukovsky
Copy link
Member

@tmancey is it still a draft or can be reviewed?

@tmancey
Copy link
Collaborator Author

tmancey commented Oct 29, 2019 •
edited
Loading

@tmancey is it still a draft or can be reviewed?

Still in draft, as I need to merge #3770 first so I can rebase this ticket which will remove lots of files as I had to add browser tests in this PR.

@tmancey tmancey marked this pull request as ready for review October 29, 2019 23:26
kylehickinson
kylehickinson approved these changes Oct 30, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS changes look good

@tmancey tmancey force-pushed the issues/6612 branch 2 times, most recently from da2f115 to b1a44e2 Compare October 30, 2019 00:52
@tmancey tmancey requested a review from kylehickinson October 30, 2019 00:52
SergeyZhukovsky
SergeyZhukovsky approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

gdregalo
gdregalo suggested changes Oct 30, 2019
View reviewed changes
browser/ui/webui/brave_rewards_page_ui.cc
@@ -1063,8 +1063,8 @@ void RewardsDOMHandler::GetAdsData(const base::ListValue *args) {

base::DictionaryValue ads_data;

auto is_supported_region = ads_service_->IsSupportedRegion();
ads_data.SetBoolean("adsIsSupported", is_supported_region);
auto is_supported_locale = ads_service_->IsSupportedLocale();
Copy link
Contributor

Choose a reason for hiding this comment

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

From G C++ styleguide:
The fundamental rule is: use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Legacy code to be fixed in a future refactor ticket as not related to this change. However new code will/does follow the Google style guide going forward

components/brave_ads/browser/ads_service_impl.cc Outdated
return false;
}

const auto schema_version =
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above: implicit type deduction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

vendor/bat-native-ads/src/bat/ads/ads.cc
auto it = kSupportedRegions.find(region);
if (it == kSupportedRegions.end()) {
return false;
for (const auto& schema : kSupportedRegionsSchemas) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not to use std::find_if instead of loop

Copy link
Collaborator Author

@tmancey tmancey Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We need to iterate all schema_version elements in the std::map to see if each sub std::map contains the users region

vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated
auto locale = ads_client_->GetLocale();
auto region = helper::Locale::GetRegionCode(locale);
bool AdsImpl::ShouldClassifyPagesIfTargeted() const {
const auto locale = ads_client_->GetLocale();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above: implicit type deduction

Copy link
Collaborator Author

@tmancey tmancey Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed

@tmancey tmancey force-pushed the issues/6612 branch 2 times, most recently from 7b10ed6 to 4a99cca Compare October 30, 2019 16:56
@tmancey tmancey requested a review from kylehickinson October 30, 2019 16:56
@tmancey tmancey force-pushed the issues/6612 branch 6 times, most recently from 2f3502d to 42486f9 Compare November 3, 2019 09:32
NejcZdovc
NejcZdovc approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

rewards changes looks good, please make sure that CI test passes

@tmancey tmancey merged commit a22a5c6 into master Nov 4, 2019
@tmancey tmancey deleted the issues/6612 branch November 4, 2019 22:27
@NejcZdovc NejcZdovc added this to the 0.73.x - Dev milestone Nov 8, 2019
@LaurenWags LaurenWags mentioned this pull request Jan 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdregalo gdregalo gdregalo approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@deeppandya deeppandya Awaiting requested review from deeppandya

@SergeyZhukovsky SergeyZhukovsky Awaiting requested review from SergeyZhukovsky

Assignees

@tmancey tmancey

Labels
feature/ads
Projects
None yet
Milestone
1.2.x - Release
Development

Successfully merging this pull request may close these issues.

Decouple business logic for newly supported regions into ads library
5 participants
@tmancey @SergeyZhukovsky @kylehickinson @NejcZdovc @gdregalo