Maintains Ads UI preference #1337
Conversation
ryanml
force-pushed
the
fix-2888-2
branch
2 times, most recently
from
c92e7b4
to
670c19e
Compare
| if (ads_service_) { | ||
| if (rewards_enabled) { | ||
| // When Rewards are enabled, set Ads enabled to whatever UI value is | ||
| bool ads_ui_enabled = ads_service_->is_ui_enabled(); |
There was a problem hiding this comment.
I think it's better to use the buildflag for this. I would only use the ui_enabled stuff in js
There was a problem hiding this comment.
or is this something different? I guess I'm confused
There was a problem hiding this comment.
what is the difference between enabled and ui_enabled?
There was a problem hiding this comment.
actually I think I understand, but per the comment below I think ads service should have logic to check the status of rewards service enabled. Per discussions with @tmancey ads are going to be more tightly integrated inside the service process so eventually we might even move the check for rewards enabled/disabled into the lib
There was a problem hiding this comment.
So enabled essentially means, "is the ad service enabled and running?" ui_enabled means, "have ads been explicitly enabled in the UI by the user?"
This may make it more clear
User has Rewards enabled via the main toggle, and the ads box enabled:
enabled -> true
ui_enabled -> true
The user then disables the main Rewards toggle
enabled -> false (Ads service is off as a result of shutting off rewards)
ui_enabled -> true (This is unchanged as just Rewards was disabled, not explicitly Ads)
With the above when one re-enables Rewards via the main toggle, we have the preference for Ads being on saved in ui_enabled, so we know we can turn Ads back on along with Rewards
There was a problem hiding this comment.
yep, what I'm suggesting is that ads is_enabled should check both the ads enabled preference and also RewardsService enabled status. That makes the logic simpler for the UI handler and remove the extra pref. Ideally I think we'd do that before it went to beta channel because otherwise we'd need extra logic to handle the ui enabled pref.
There was a problem hiding this comment.
I think if ads is dependent on rewards then it makes sense to have AdsServiceFactory DependsOn for Rewards to make it explicit and keep the enable/disable logic in the service
There was a problem hiding this comment.
this is what I wanted to do as well (brave/brave-browser#2888 (comment)), but because of time constrains we just added new pefr for now so that we could ship it today
| ads_service_->set_ads_enabled(ads_ui_enabled); | ||
| } else { | ||
| // When Rewards are disabled, cache UI value and disable Ads | ||
| bool ads_enabled = ads_service_->is_enabled(); |
There was a problem hiding this comment.
I think it might be better to put this logic in rewards service and AdsServiceFactory should have a DependsOn for RewardsService
Fixes: brave/brave-browser#2888
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests) ongit rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
brave://rewards, disable Ads via the settings boxReviewer Checklist: