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

BAT Confirmations should not be started if Rewards is disabled #5424

Merged
merged 1 commit into from
May 4, 2020
Merged

BAT Confirmations should not be started if Rewards is disabled #5424

merged 1 commit into from
May 4, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Apr 30, 2020

Resolves brave/brave-browser#9544

Submitter Checklist:

Test Plan:

  • Test upgrade from nightly (before the fix)
  • Test fresh install enabling rewards from the URL bar, then closing and restarting the browser
  • Test fresh install enabling rewards from on-boarding, then closing and restarting the browser
  • Test upgrade path from 0.61 (as the prefs to test if Rewards is enabled were introduced in commit-hash 13deb15e2bd2291398c81e83812b3d76278cad57 in 0.62) to the latest nightly build
  • Toggle rewards off and back on
  • Toggle rewards off, then restart the browser and switch rewards on
  • The change should be tested on iOS

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.

@tmancey tmancey self-assigned this Apr 30, 2020
tmancey
tmancey commented Apr 30, 2020
View reviewed changes
vendor/brave-ios/Ledger/BATBraveLedger.mm
@@ -1277,6 +1277,10 @@ - (void)setBooleanState:(const std::string&)name value:(bool)value
- (bool)getBooleanState:(const std::string&)name
{
const auto key = [NSString stringWithUTF8String:name.c_str()];
if (![self.prefs objectForKey:key]) {
Copy link
Collaborator Author

@tmancey tmancey Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@kylehickinson default to NO for unknown keys to support kStateEnabled and kStateEnabledMigrated boolean state from the desktop which are not supported on iOS

emerick
emerick approved these changes Apr 30, 2020
View reviewed changes
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

tmancey
tmancey commented Apr 30, 2020
View reviewed changes
vendor/bat-native-confirmations/src/bat/confirmations/internal/confirmations_impl.cc
}

DCHECK(wallet_info_.IsValid());
Copy link
Collaborator Author

@tmancey tmancey Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This DCHECK was causing issues when resolving this ticket as is not required as ads_rewards_->Update(wallet_info_, should_refresh); validates if the wallet is valid

@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Apr 30, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented Apr 30, 2020

macOS failed CI, restarting macOS

kylehickinson
kylehickinson approved these changes Apr 30, 2020
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Lgtm

NejcZdovc
NejcZdovc reviewed May 1, 2020
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

@tmancey could you please check this use case:

  • go to master
  • enable rewards
  • disable rewards
  • switch to this PR
  • make sure that confirmations are not initialized
  • enable rewards

As far as I can see from the logs when you re-enable rewards confirmations are not initialized as code for initialization is only in initialization flow and not in toggle flow

@tmancey tmancey removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels May 1, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented May 1, 2020

@tmancey could you please check this use case:

  • go to master
  • enable rewards
  • disable rewards
  • switch to this PR
  • make sure that confirmations are not initialized
  • enable rewards

As far as I can see from the logs when you re-enable rewards confirmations are not initialized as code for initialization is only in initialization flow and not in toggle flow

Fixed

@tmancey
Copy link
Collaborator Author

tmancey commented May 1, 2020

Android and macOS failed CI, restarting

@tmancey tmancey added CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels May 1, 2020
@NejcZdovc
Copy link
Contributor

@tmancey you need to rebase this one

@tmancey tmancey requested a review from NejcZdovc May 1, 2020 22:41
@tmancey tmancey force-pushed the issues/9544 branch 2 times, most recently from 5e3b5f9 to 7bb35b0 Compare May 3, 2020 04:17
@tmancey
Copy link
Collaborator Author

tmancey commented May 3, 2020

Failed CI for unrelated reasons due to test-security

@bsclifton
Copy link
Member

bsclifton commented May 4, 2020 •
edited
Loading

Security issue open with brave/brave-browser#9578 - trying to resolve that one (doesn't block merge for this!). Changes here LGTM - but will defer to others for proper review 😄

@tmancey
Copy link
Collaborator Author

tmancey commented May 4, 2020

CI failed due to unrelated rewards browser tests

@tmancey tmancey merged commit b2a3f9a into master May 4, 2020
@tmancey tmancey deleted the issues/9544 branch May 4, 2020 13:15
@tmancey tmancey added this to the 1.10.x - Nightly milestone May 4, 2020
@btlechowski btlechowski mentioned this pull request Jun 10, 2020
Closed
@LaurenWags LaurenWags mentioned this pull request Jun 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylehickinson kylehickinson kylehickinson approved these changes

@emerick emerick emerick approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@tmancey tmancey

Labels
CI/skip-ios Do not run CI builds for iOS feature/ads
Projects
None yet
Milestone
1.10.x - Release
Development

Successfully merging this pull request may close these issues.

BAT Confirmations should not be started if Rewards is disabled
5 participants
@tmancey @NejcZdovc @bsclifton @kylehickinson @emerick