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.

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

Fixes upgrade from 0.63.x to 1.3.x AC switch OFF and wallet creation failed #4449

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Jan 28, 2020

Fixes brave/brave-browser#7833

Submitter Checklist:

Test Plan:

See brave/brave-browser#7833 (resets Ads viewed count to zero is a known issue and was agreed that migration would not be attempted for brave/brave-browser#4047)

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

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor whitespace nit.

const double bat = choices_bat_value.GetDouble();
wallet_properties.parameters_choices.push_back(bat);
if (choices_bat_list) {
for (const auto&choices_bat_value : choices_bat_list->GetList()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor, but maybe a space after the &?

@bsclifton bsclifton added the CI/skip Do not run CI builds (except noplatform) label Jan 28, 2020
@bsclifton
Copy link
Member

CI run was completely green 🎉 I did a quick commit to address that nit, @emerick 😄 Should be ready to merge!

@bsclifton bsclifton merged commit c830249 into master Jan 28, 2020
@bsclifton bsclifton deleted the issues/7833 branch January 28, 2020 18:07
@tmancey
Copy link
Collaborator Author

tmancey commented Jan 28, 2020

Sanity tests of fix #4449 have shown that users who experienced this issue will have their data recovered after upgrading as long the user did not try to re-enable Brave Rewards. If a user did re-enable Rewards before upgrading to this fix then the wallet will be corrupted and is non-recoverable.

Sanity testing was from 0.63.55 to 1.3, 1.4 and 1.5, however, we should test other scenarios.

As a side note P3A is broken for wallet creation as it still references Probi etc. which does not exist in the JSON feed. I will raise a separate issue for this.

@LaurenWags
Copy link
Member

Verified using

Brave 1.5.69 Chromium: 79.0.3945.130 (Official Build) nightly (64-bit)
Revision e22de67c28798d98833a7137c0e22876237fc40a-refs/branch-heads/3945@{#1047}
OS macOS Version 10.14.6 (Build 18G103)

PASS Upgrading without hitting the issue first:

  1. 0.63.55

    • Install above version.
    • Enable Rewards, view an ad
    • Close. Rename profile to Nightly.
    • Launch 1.5.69
    • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected
  2. 0.64.77

    • Install above version.
    • Enable Rewards, view an ad
    • Close. Rename profile to Nightly.
    • Launch 1.5.69
    • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected

PASS Upgrading after hitting the issue first:
3. 0.63.55

  • Install above version.
  • Enable Rewards, view an ad
  • Close. Rename profile to Nightly. Download Nightly 1.5.53.
  • Launch 1.5.53 --> see issue but don't do anything.
  • Close. Update to 1.5.69 on Nightly and relaunch.
  • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected
  1. 0.64.77
    • Install above version.
    • Enable Rewards, view an ad
    • Close. Rename profile to Nightly. Download Nightly 1.5.53.
    • Launch 1.5.53 --> see issue but don't do anything.
    • Close. Update to 1.5.69 on Nightly and relaunch.
    • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected

Note - see #4449 (comment) for additional information

PASS Sanity check of other upgrades:
5. 0.69.135

  • Install above version.
  • Enable Rewards, view an ad
  • Close. Rename profile to Nightly.
  • Launch 1.5.69
  • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected
  1. 1.2.43
    • Install above version.
    • Enable Rewards, view an ad
    • Close. Rename profile to Nightly.
    • Launch 1.5.69
    • Ads panel displays as expected, AC is still on as expected, no error on wallet panel as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CI/skip Do not run CI builds (except noplatform)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from 0.63.x to 1.3.x resets Ads viewed count to zero, AC switch OFF and wallet creation failed
6 participants