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

'Insufficient Funds' notification now will not show if balance will cover verified sites. #2208

Merged
merged 1 commit into from
May 29, 2019

Conversation

jasonrsadler
Copy link
Contributor

@jasonrsadler jasonrsadler commented Apr 12, 2019

Fixes brave/brave-browser#3707
Fixes brave/brave-browser#4104
Fixes brave/brave-browser#4090

brave-ui PR: brave/brave-ui#471

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 && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • 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. Start Brave clean profile with -- --rewards=reconcile-interval=20 (or some other value less than 4320 [3 days])
  2. Visit a verified site to add to AC and non-verified site to add to AC preferably with same time duration.
  3. Accept grant (at present time 30 BAT)
  4. Go to Rewards page and set AC budget to 50 BAT
  5. Quit browser and restart to reinitiate timer
  6. Wait a few moments and make sure that you don't see 'Insufficient Funds' notification
  7. Go to Rewards page and set AC budget to 100 BAT
  8. Quit Browser and restart.
  9. Wait a few moments and make sure you get BAT icon notification Re: 'Insufficient Funds'

Repeat the steps above with a different combination of verified and unverified sites and make sure notification does not show if the balance would cover the verified sites contribution.

Repeat the steps above using also a combination of verified and unverified recurring donations.
Verify that 'Insufficient Funds notifications are not shown if the combined reconcile amount of all verified publishers is under the wallet balance. Likewise make sure the notification is shown if the wallet balance is insufficient.

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

@jasonrsadler jasonrsadler force-pushed the ac-tips branch 5 times, most recently from 7ebc1c7 to 471778d Compare May 24, 2019 14:58
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.

Current call flow is like this:

  • HasSufficientBalance (calls AC list)
  • GetVerifiedAutoAmount (gets AC amount and calls recurring)
  • GetVerifiedRecurringAmount (gets recurring amount and calls balance)
  • OnFetchWalletProperties (return if sufficient balance)

I think this flow would be better

  • HasSufficientBalance (calls balance)
  • OnFetchWalletProperties (calls AC list)
  • GetVerifiedAutoAmount (gets AC amount; return if not enough balance; calls recurring if enough balance)
  • GetVerifiedRecurringAmount (gets recurring amount and return if sufficient balance)

With this approach we can exit sooner if we already know that AC is over the budget

this, _1, _2, total_reconcile_amount, callback));
}

void BatContribution::OnFetchWalletProperties(
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be named more HasSufficientBalance specific as we will probably had more FetchWalletProperties calls. Maybe something like OnSufficientBalanceWallet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

// static
double BatContribution::GetAmountFromVerifiedRecurring(
Copy link
Contributor

Choose a reason for hiding this comment

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

Move it before OnFetchWalletProperties. This is how it's defined in .h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jasonrsadler jasonrsadler force-pushed the ac-tips branch 4 times, most recently from 6fb59c5 to 6977a45 Compare May 28, 2019 02:58
@jasonrsadler
Copy link
Contributor Author

Modified flow to exit if AC balance does not meet contribution requirements

double total_reconcile_amount,
bool is_ac_list_empty,
ledger::HasSufficientBalanceToReconcileCallback callback) {
if (publisher_list.empty() && is_ac_list_empty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_ac_list_empty is still needed? We now exit in AC if over, so if publisher_list is empty we know that we have enough funds

Copy link
Contributor Author

@jasonrsadler jasonrsadler May 28, 2019

Choose a reason for hiding this comment

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

Removed.

@jasonrsadler jasonrsadler force-pushed the ac-tips branch 2 times, most recently from 2c2fe8a to 0a0573c Compare May 28, 2019 11:01
@jasonrsadler jasonrsadler force-pushed the ac-tips branch 2 times, most recently from 4dff4fa to 7daedeb Compare May 28, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants