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

Fix wallet usage ping parameter after updating to 1.36.x #12488

Merged
merged 1 commit into from
Mar 4, 2022
Merged

Fix wallet usage ping parameter after updating to 1.36.x #12488

merged 1 commit into from
Mar 4, 2022

Conversation

DJAndries
Copy link
Collaborator

@DJAndries DJAndries commented Mar 4, 2022 •
edited
Loading

Resolves brave/brave-browser#21476

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
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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:

  1. Set date to March 2.
  2. Install 1.35.x. Use the wallet.
  3. Advance system time by one day, open browser, verify wallet param in usage ping is 7
  4. Upgrade to 1.36.x with hotfix. Advance system time by one day. Open the browser.
  5. Using MITM proxy, verify that the wallet parameter of the usage ping is set to 0.
  6. Use wallet. Close browser.
  7. Advance system time by one day, open browser, verify wallet parameter is set to 7.

Another one:

  1. Set date to March 2.
  2. Install nightly version which does not include this change. Use the wallet. Close browser
  3. Advance system time by one day, open browser, verify wallet param in usage ping is 7.
  4. Upgrade to nightly which includes this change. Advance system time by one day. Open the browser.
  5. Using MITM proxy, verify that the wallet parameter of the usage ping is set to 0.
  6. Use wallet. Close browser.
  7. Advance system time by one day, open browser, verify wallet parameter is set to 7.

Regression testing of brave/brave-browser#20956 recommended

@DJAndries DJAndries requested a review from rillian March 4, 2022 19:54
rillian
rillian approved these changes Mar 4, 2022
View reviewed changes
browser/brave_stats/brave_stats_updater.cc Outdated
@@ -463,7 +463,8 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
registry->RegisterIntegerPref(kLastCheckMonth, 0);
registry->RegisterStringPref(kLastCheckYMD, std::string());
registry->RegisterStringPref(kWeekOfInstallation, std::string());
registry->RegisterTimePref(kBraveWalletPingReportedUnlockTime, base::Time());
registry->RegisterTimePref(kBraveWalletPingReportedUnlockTime,
base::Time::Now());
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a similar work around for kBraveWalletP3ALastReportTime in #12480?

Will this only help profiles upgrading from 1.35? It seems like this would have no effect if they're already launched a 1.36 release?

Copy link
Collaborator Author

@DJAndries DJAndries Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

totally separate issue, and yes you are correct! This will only fix upgrades to 1.36.x. With the current code, if they already used their wallet in 1.35.x, upgraded to 1.36.x and launched their browser for the first time after the upgrade, the damage has already been done. This will only fix future upgrades to 1.36.x (i.e. Linux, which is usually delayed).

@DJAndries DJAndries force-pushed the wallet-ping-update-fix branch from 4bbe4be to 198000e Compare March 4, 2022 21:17
@DJAndries DJAndries requested a review from a team as a code owner March 4, 2022 21:17
@DJAndries DJAndries requested a review from rillian March 4, 2022 21:18
DJAndries
DJAndries commented Mar 4, 2022
View reviewed changes
browser/brave_stats/brave_stats_updater_params.cc
@@ -219,7 +224,7 @@ GURL BraveStatsUpdaterParams::GetUpdateURL(
update_url =
net::AppendQueryParameter(update_url, "arch", GetProcessArchParam());
update_url =
net::AppendQueryParameter(update_url, "wallet", GetWalletEnabledParam());
net::AppendQueryParameter(update_url, "wallet2", GetWalletEnabledParam());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed the wallet param in the usage ping so we can filter out the erroneous data and start over for this month. Thoughts? cc @bbondy

Copy link
Member

Choose a reason for hiding this comment

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

I'm cool with it.
great name :P

rillian
rillian suggested changes Mar 4, 2022
View reviewed changes
browser/brave_stats/brave_stats_updater_params.cc Outdated
Comment on lines 109 to 111
if (is_last_wallet_unlock_v2_)
usage_bitset = UsageBitfieldFromTimestamp(wallet_last_unlocked_,
last_reported_wallet_unlock_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Curly braces around the conditional body, please. Direct bodies like this are harder to read an error prone.

@DJAndries DJAndries force-pushed the wallet-ping-update-fix branch from 198000e to c515123 Compare March 4, 2022 22:35
rillian
rillian approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

This seems simpler!

@DJAndries DJAndries merged commit aed22d8 into master Mar 4, 2022
@DJAndries DJAndries deleted the wallet-ping-update-fix branch March 4, 2022 23:59
brave-builds pushed a commit that referenced this pull request Mar 5, 2022
@brave-browser-releases
Uplift of #12488 (squashed) to beta
c428c66
brave-builds pushed a commit that referenced this pull request Mar 5, 2022
@brave-browser-releases
Uplift of #12488 (squashed) to release
f7ee430
This was referenced Mar 5, 2022
Merged
Merged
@DJAndries DJAndries added this to the 1.38.x - Nightly milestone Mar 5, 2022
@kjozwiak
Copy link
Member

kjozwiak commented Mar 6, 2022 •
edited
Loading

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.38.25 Chromium: 99.0.4844.51 (Official Build) nightly (64-bit)
--- | ---
Revision | d537ec02474b5afe23684e7963d538896c63ac77-refs/branch-heads/4844@{#875}
OS | Windows 11 Version 21H2 (Build 22000.527)

Using the second STR/Case mentioned via #12488 (comment) for Nightly, went through the following:

  • set the machine date to March 2, 2022 - ~9:30pm EST
  • installed 1.38.23 Chromium: 99.0.4844.51 and enabled the Brave Wallet via brave://wallet
  • closed Brave and set the date on the machine to March 3, 2022 - ~9:32pm EST

Ensured that wallet=7 as per the following:

GET https://laptop-updates.brave.com/1/usage/brave-core?platform=winx64-bc&channel=nightly&version=1.38.23&daily=true&weekly=false&monthly=false&first=false&woi=2022-02-28&dtoi=2022-03-02&ref=BRV001&adsEnabled=false&arch=&wallet=7 HTTP/1.1

  • closed the browser and updated from 1.38.23 Chromium: 99.0.4844.51 -> 1.38.25 Chromium: 99.0.4844.51
  • once the browser was updated, moved the date ahead again to March 4, 2022 - ~9:35pm EST
  • launched the browser (without interacting with Brave Wallet)

Ensured that wallet2=0 as per the following:

GET https://laptop-updates.brave.com/1/usage/brave-core?platform=winx64-bc&channel=nightly&version=1.38.25&daily=true&weekly=false&monthly=false&first=false&woi=2022-02-28&dtoi=2022-03-02&ref=BRV001&adsEnabled=false&arch=&wallet2=0 HTTP/1.1

  • after confirming that wallet2=0, opened brave://wallet and interacted with several settings/features
  • closed the browser and moved the date ahead to March 5, 2022 - ~9:40pm EST
  • launched the browser

Ensured that wallet2=7 as per the following:

GET https://laptop-updates.brave.com/1/usage/brave-core?platform=winx64-bc&channel=nightly&version=1.38.25&daily=true&weekly=false&monthly=false&first=false&woi=2022-02-28&dtoi=2022-03-02&ref=BRV001&adsEnabled=false&arch=&wallet2=7 HTTP/1.1

Example Example Example
initial second third

@srirambv srirambv mentioned this pull request Mar 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy approved these changes

@rillian rillian rillian approved these changes

@iefremov iefremov Awaiting requested review from iefremov

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@DJAndries DJAndries

Labels
None yet
Projects
None yet
Milestone
1.38.x - Release
Development

Successfully merging this pull request may close these issues.

Inaccurate usage ping wallet parameter after update to 1.36.x
4 participants
@DJAndries @kjozwiak @rillian @bbondy