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

Enables Ads on rewards opt-in via panel and welcome page #1282

Merged
merged 1 commit into from
Jan 11, 2019
Merged

Enables Ads on rewards opt-in via panel and welcome page #1282

merged 1 commit into from
Jan 11, 2019

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jan 10, 2019 •
edited by jasonrsadler
Loading

Fixes: brave/brave-browser#2877

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Run Brave from this PR with a clean profile
  2. Navigate to brave://rewards, create wallet via the welcome page
  3. Once the Rewards Settings page loads, confirm via the UI and console logs that Ads is fired up
  4. Stop Brave, remove profile and run again
  5. Enable Rewards via the panel
  6. Navigate to brave://rewards and repeat step 3

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

@ryanml ryanml self-assigned this Jan 10, 2019
@ryanml ryanml mentioned this pull request Jan 10, 2019
Closed
@ryanml ryanml requested a review from NejcZdovc January 10, 2019 06:55
@NejcZdovc
Copy link
Contributor

let's wait with merging this one as their is some discussion to not have this one in

@ryanml
Copy link
Contributor Author

ryanml commented Jan 10, 2019

Blocker removed as we are moving forward with this one

@ryanml ryanml requested a review from NejcZdovc January 10, 2019 21:55
diracdeltas
diracdeltas reviewed Jan 10, 2019
View reviewed changes
...brave_rewards/resources/extension/brave_rewards/background/reducers/rewards_panel_reducer.ts
@@ -41,6 +41,7 @@ export const rewardsPanelReducer = (state: RewardsExtension.State | undefined, a
case types.ON_WALLET_CREATED:
state = { ...state }
state.walletCreated = true
chrome.braveRewards.saveAdsSetting('adsEnabled', 'true')
Copy link
Member

Choose a reason for hiding this comment

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

is this necessary? what if someone wants to create a new wallet (ex: after deleting their original one) but don't want to opt into ads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very good point, I'll have to dig around and see if we have something existing that can differentiate between a first time opt-in and what is technically a recreation, as in both cases they are handled more or less the same from the creation side

diracdeltas
diracdeltas reviewed Jan 10, 2019
View reviewed changes
components/brave_rewards/resources/ui/reducers/wallet_reducer.ts
@@ -17,6 +17,7 @@ const createWallet = (state: Rewards.State) => {

chrome.send('brave_rewards.getReconcileStamp')
chrome.send('brave_rewards.getAddresses')
chrome.send('brave_rewards.saveAdsSetting', ['adsEnabled', 'true'])
Copy link
Member

Choose a reason for hiding this comment

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

same concern as https://github.com/brave/brave-core/pull/1282/files#r246943290. i feel it is likely we will want to allow wallet creation independent of changing adsEnabled to true.

@ryanml ryanml requested a review from diracdeltas January 11, 2019 00:13
@ryanml
Copy link
Contributor Author

ryanml commented Jan 11, 2019

@diracdeltas per our conversation over slack, I've renamed/reorganized some of the involved redux actions to be a bit more semantic, hopefully clearer about what is happening when an opt-in occurs

@ryanml ryanml force-pushed the fix-2877 branch 2 times, most recently from 9c8ff2c to c38c880 Compare January 11, 2019 00:33
@ryanml
Copy link
Contributor Author

ryanml commented Jan 11, 2019 •
edited
Loading

per further discussion with @diracdeltas, we can move forward with this pr as is. We should have a discussion on how we can decouple the wallet creation action from opt-in, as there will be cases in the future where a user can re-create a wallet (after deleting one), but they are not necessarily re-opting in to Rewards as a whole. Wallet creation should be an independent action.

cc: @NejcZdovc

jasonrsadler
jasonrsadler approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Aside from discussion above, lgtm

@ryanml ryanml merged commit 2c9f78a into master Jan 11, 2019
@ryanml ryanml deleted the fix-2877 branch January 11, 2019 04:53
@ryanml ryanml added the 0.61.x label Jan 11, 2019
@ryanml ryanml mentioned this pull request Jan 11, 2019
Merged
@bbondy bbondy added this to the 0.61.x - Nightly milestone Jan 14, 2019
@LaurenWags LaurenWags mentioned this pull request Feb 5, 2019
Closed
This was referenced Apr 19, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler approved these changes

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

@diracdeltas diracdeltas Awaiting requested review from diracdeltas

Assignees

@ryanml ryanml

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

Successfully merging this pull request may close these issues.

Enabling Rewards for the first time should enable Ads
5 participants
@ryanml @NejcZdovc @diracdeltas @jasonrsadler @bbondy