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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a96c6bb
Decouple business logic for newly supported regions into ads library
tmancey Oct 25, 2019
0867eff
Refactor Brave ads browser tests to use Locale instead of Region
tmancey Oct 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion browser/brave_ads/android/brave_ads_native_helper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jboolean JNI_BraveAdsNativeHelper_IsLocaleValid(
return false;
}

return ads_service_->IsSupportedRegion();
return ads_service_->IsSupportedLocale();
}

void JNI_BraveAdsNativeHelper_SetAdsEnabled(
Expand Down
6 changes: 3 additions & 3 deletions browser/ui/webui/brave_rewards_page_ui.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,8 +1067,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

ads_data.SetBoolean("adsIsSupported", is_supported_locale);

auto is_enabled = ads_service_->IsEnabled();
ads_data.SetBoolean("adsEnabled", is_enabled);
Expand Down Expand Up @@ -1275,7 +1275,7 @@ void RewardsDOMHandler::SaveAdsSetting(const base::ListValue* args) {

if (key == "adsEnabled") {
const auto is_enabled =
value == "true" && ads_service_->IsSupportedRegion();
value == "true" && ads_service_->IsSupportedLocale();
ads_service_->SetEnabled(is_enabled);
} else if (key == "adsPerHour") {
ads_service_->SetAdsPerHour(std::stoull(value));
Expand Down
3 changes: 2 additions & 1 deletion components/brave_ads/browser/ads_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ class AdsService : public KeyedService {
public:
AdsService() = default;

virtual bool IsSupportedRegion() const = 0;
virtual bool IsSupportedLocale() const = 0;
virtual bool IsNewlySupportedLocale() = 0;

virtual bool IsEnabled() const = 0;
virtual void SetEnabled(
Expand Down
5 changes: 5 additions & 0 deletions components/brave_ads/browser/ads_service_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ void AdsServiceFactory::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterIntegerPref(prefs::kVersion, prefs::kCurrentVersionNumber);

registry->RegisterIntegerPref(prefs::kSupportedRegionsLastSchemaVersion, 0);

registry->RegisterIntegerPref(prefs::kSupportedRegionsSchemaVersion,
prefs::kSupportedRegionsSchemaVersionNumber);

registry->RegisterBooleanPref(prefs::kEnabled, false);

registry->RegisterUint64Pref(prefs::kAdsPerHour, 2);
Expand Down
38 changes: 31 additions & 7 deletions components/brave_ads/browser/ads_service_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,10 @@ AdsServiceImpl::AdsServiceImpl(Profile* profile) :
NotificationHandler::Type::BRAVE_ADS,
std::make_unique<AdsNotificationHandler>(this));

#if !defined(OS_ANDROID)
// TODO(tmancey): Refactor on-boarding to be platform agnostic
MaybeShowOnboarding();
#endif

MaybeStart(false);
}
Expand All @@ -403,9 +406,30 @@ AdsServiceImpl::~AdsServiceImpl() {
file_task_runner_->DeleteSoon(FROM_HERE, bundle_state_backend_.release());
}

bool AdsServiceImpl::IsSupportedRegion() const {
auto locale = LocaleHelper::GetInstance()->GetLocale();
return ads::Ads::IsSupportedRegion(locale);
bool AdsServiceImpl::IsSupportedLocale() const {
const std::string locale = GetLocale();
return ads::Ads::IsSupportedLocale(locale);
}

bool AdsServiceImpl::IsNewlySupportedLocale() {
if (!IsSupportedLocale()) {
return false;
}

const int schema_version =
GetIntegerPref(prefs::kSupportedRegionsSchemaVersion);
if (schema_version != prefs::kSupportedRegionsSchemaVersionNumber) {
SetIntegerPref(prefs::kSupportedRegionsLastSchemaVersion, schema_version);

SetIntegerPref(prefs::kSupportedRegionsSchemaVersion,
prefs::kSupportedRegionsSchemaVersionNumber);
}

const int last_schema_version =
GetIntegerPref(prefs::kSupportedRegionsLastSchemaVersion);

const std::string locale = GetLocale();
return ads::Ads::IsNewlySupportedLocale(locale, last_schema_version);
}

void AdsServiceImpl::SetEnabled(
Expand Down Expand Up @@ -668,8 +692,8 @@ bool AdsServiceImpl::StartService() {

void AdsServiceImpl::MaybeStart(
const bool should_restart) {
if (!IsSupportedRegion()) {
LOG(INFO) << GetLocale() << " locale does not support Ads";
if (!IsSupportedLocale()) {
LOG(INFO) << GetLocale() << " locale does not support ads";
Shutdown();
return;
}
Expand Down Expand Up @@ -1507,15 +1531,15 @@ void AdsServiceImpl::MaybeShowOnboarding() {
ShowOnboarding();
}

bool AdsServiceImpl::ShouldShowOnboarding() const {
bool AdsServiceImpl::ShouldShowOnboarding() {
auto is_ads_enabled = GetBooleanPref(prefs::kEnabled);

auto is_rewards_enabled =
GetBooleanPref(brave_rewards::prefs::kBraveRewardsEnabled);

auto should_show = GetBooleanPref(prefs::kShouldShowOnboarding);

return IsSupportedRegion() && !is_ads_enabled && is_rewards_enabled
return IsNewlySupportedLocale() && !is_ads_enabled && is_rewards_enabled
&& should_show;
}

Expand Down
5 changes: 3 additions & 2 deletions components/brave_ads/browser/ads_service_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ class AdsServiceImpl : public AdsService,
explicit AdsServiceImpl(Profile* profile);
~AdsServiceImpl() override;

bool IsSupportedRegion() const override;
bool IsSupportedLocale() const override;
bool IsNewlySupportedLocale() override;

void SetEnabled(
const bool is_enabled) override;
Expand Down Expand Up @@ -284,7 +285,7 @@ class AdsServiceImpl : public AdsService,
const uint64_t timestamp_in_seconds) const;

void MaybeShowOnboarding();
bool ShouldShowOnboarding() const;
bool ShouldShowOnboarding();
void ShowOnboarding();
void RemoveOnboarding();
void MaybeStartRemoveOnboardingTimer();
Expand Down
8 changes: 8 additions & 0 deletions components/brave_ads/common/pref_names.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ const char kOnboardingTimestamp[] =
const char kShouldShowMyFirstAdNotification[] =
"brave.brave_ads.should_show_my_first_ad_notification";

// Stores the supported regions current schema version number
const char kSupportedRegionsLastSchemaVersion[] =
"brave.brave_ads.supported_regions_last_schema_version_number";
const char kSupportedRegionsSchemaVersion[] =
"brave.brave_ads.supported_regions_schema_version_number";

const int kSupportedRegionsSchemaVersionNumber = 4;

// Stores the preferences version number
const char kVersion[] = "brave.brave_ads.prefs.version";

Expand Down
4 changes: 4 additions & 0 deletions components/brave_ads/common/pref_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ extern const char kOnboardingTimestamp[];

extern const char kShouldShowMyFirstAdNotification[];

extern const char kSupportedRegionsLastSchemaVersion[];
extern const char kSupportedRegionsSchemaVersion[];
extern const int kSupportedRegionsSchemaVersionNumber;
SergeyZhukovsky marked this conversation as resolved.
Show resolved Hide resolved

extern const char kVersion[];
extern const int kCurrentVersionNumber;
SergeyZhukovsky marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
Loading