Add support for P3A wallet state (Q11) #8641

emerick · 2021-04-26T15:47:35Z

Resolves brave/brave-browser#7634
Resolves brave/brave-browser#13996

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Grants test

Clean profile, staging server
Visit brave://rewards
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 0 in brave://local-state
Enable Rewards
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 1 in brave://local-state
Claim UGP grant
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 2 in brave://local-state
Disable Rewards by turning off ads and auto-contribute
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 5 in brave://local-state

Funds test

Clean profile, staging server
Visit brave://rewards
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 0 in brave://local-state
Enable Rewards
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 1 in brave://local-state
Restore wallet with user funds
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 3 in brave://local-state
Claim UGP grant
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 4 in brave://local-state
Disable Rewards by turning off ads and auto-contribute
Wait 10 or so seconds
Verify that Brave.Rewards.WalletState is set to 5 in brave://local-state

components/brave_rewards/browser/rewards_p3a.h

zenparsing

I think the namespace really helps with readability here.

zenparsing · 2021-04-26T17:13:20Z

components/brave_rewards/browser/rewards_service_impl.cc

+      } else {
+        p3a::RecordWalletState({.wallet_created = true,
+                                .rewards_enabled = true,
+                                .grants_claimed = false,


Nit: it looks like these two branches can be combined now, if you want, with:

.grants_claimed = pref_service->GetBoolean(prefs::kUserHasClaimedGrant),

Ah yeah, will do.

tmancey

LGTM

iefremov · 2021-04-27T12:49:32Z

can we add browsertests for these metrics? I'm afraid that they are quite fragile and can be damaged by any refactoring. Histograms can be tested using base::HistogramTester

iefremov · 2021-04-28T12:56:12Z

two small things: (1) pls don't forget to update the p3a wiki page (2) it maybe worth adding some steps that restart the browser to the test plan

emerick · 2021-04-28T20:11:09Z

CI passed on iOS, Windows. Failed on dist step in Android and failed with expected test failures on Mac. Restarting Linux.

emerick · 2021-04-28T23:40:01Z

Linux CI passed; ready to merge.

bsclifton · 2021-04-30T06:21:41Z

Updated https://github.com/brave/brave-browser/wiki/P3A#q11-what-is-the-state-of-your-brave-rewards-wallet - can you take a look @emerick to verify it looks correct? I wasn't sure if this worked on Android or not so I left a question mark there

emerick · 2021-04-30T09:49:44Z

@bsclifton Ah thank you, yes I'll check that out today!

bridiver · 2024-05-13T20:19:00Z

components/brave_rewards/browser/test/rewards_p3a_browsertest.cc

+    run_loop.Run();
+  }
+
+  void WaitForRewardsInitialization() {


I'm having a hard time understanding how this waits for rewards initialization. It just checks once and then runs an empty RunLoop which will just finish immediately. I'm seeing failures when running this in Debug mode and I believe it's because there is a race condition here and we're not actually waiting for rewards to be initialized

@brave/rewards-client

I think this is doing the right thing in this specific case, but I wouldn't doubt that there may be some other issues in this test (or the Rewards tests in general). But I think the call to Run here will block until the OnRewardsInitialized observer fires and calls Quit on this run loop. What may have caused confusion is that this PR did not include that call to Quit; that mistake was caught in a follow-up PR. We could clean this up to use the nicer Watcher paradigm from upstream, which would also help to clarify the intent. Apologies though if I'm missing a deeper problem.

emerick requested a review from iefremov as a code owner April 26, 2021 15:47

emerick self-assigned this Apr 26, 2021

emerick requested a review from zenparsing April 26, 2021 15:52

zenparsing reviewed Apr 26, 2021

View reviewed changes

components/brave_rewards/browser/rewards_p3a.h Outdated Show resolved Hide resolved

components/brave_rewards/browser/rewards_p3a.h Outdated Show resolved Hide resolved

emerick force-pushed the rewards-p3a-wallet-state branch from 3f892b7 to 4a0a484 Compare April 26, 2021 17:05

emerick requested a review from tmancey as a code owner April 26, 2021 17:05

zenparsing approved these changes Apr 26, 2021

View reviewed changes

tmancey approved these changes Apr 26, 2021

View reviewed changes

emerick force-pushed the rewards-p3a-wallet-state branch from 4a0a484 to 8ec53bf Compare April 26, 2021 18:07

emerick force-pushed the rewards-p3a-wallet-state branch from 8ec53bf to 0922a71 Compare April 27, 2021 21:52

emerick requested a review from a team as a code owner April 27, 2021 21:52

emerick requested a review from deeppandya April 27, 2021 21:52

iefremov approved these changes Apr 28, 2021

View reviewed changes

emerick force-pushed the rewards-p3a-wallet-state branch 3 times, most recently from d17e10f to b1f1904 Compare April 28, 2021 15:57

emerick added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Apr 28, 2021

emerick added 2 commits April 28, 2021 16:12

Add support for P3A wallet state (Q11)

98c27ab

Add tests for WalletState

2d95b4d

emerick force-pushed the rewards-p3a-wallet-state branch from b1f1904 to 2d95b4d Compare April 28, 2021 20:12

emerick merged commit b4c15cc into master Apr 28, 2021

emerick deleted the rewards-p3a-wallet-state branch April 28, 2021 23:40

emerick removed the CI/skip-android Do not run CI builds for Android label Apr 28, 2021

emerick removed CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Apr 28, 2021

emerick added this to the 1.26.x - Nightly milestone Apr 28, 2021

This was referenced May 28, 2021

Implement P3A metric "Q11: What is the state of your Brave Rewards wallet" brave/brave-browser#7634

Closed

Properly implement rewards/ads related P3A questions brave/brave-browser#13996

Closed

bridiver reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for P3A wallet state (Q11) #8641

Add support for P3A wallet state (Q11) #8641

emerick commented Apr 26, 2021 •

edited by bsclifton

Loading

zenparsing left a comment

zenparsing Apr 26, 2021

emerick Apr 26, 2021

tmancey left a comment

iefremov commented Apr 27, 2021

iefremov commented Apr 28, 2021

emerick commented Apr 28, 2021

emerick commented Apr 28, 2021

bsclifton commented Apr 30, 2021

emerick commented Apr 30, 2021

bridiver May 13, 2024

Miyayes May 14, 2024

emerick May 14, 2024

Add support for P3A wallet state (Q11) #8641

Add support for P3A wallet state (Q11) #8641

Conversation

emerick commented Apr 26, 2021 • edited by bsclifton Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Grants test

Funds test

zenparsing left a comment

Choose a reason for hiding this comment

zenparsing Apr 26, 2021

Choose a reason for hiding this comment

emerick Apr 26, 2021

Choose a reason for hiding this comment

tmancey left a comment

Choose a reason for hiding this comment

iefremov commented Apr 27, 2021

iefremov commented Apr 28, 2021

emerick commented Apr 28, 2021

emerick commented Apr 28, 2021

bsclifton commented Apr 30, 2021

emerick commented Apr 30, 2021

bridiver May 13, 2024

Choose a reason for hiding this comment

Miyayes May 14, 2024

Choose a reason for hiding this comment

emerick May 14, 2024

Choose a reason for hiding this comment

emerick commented Apr 26, 2021 •

edited by bsclifton

Loading