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

Adds contribution queue #3658

Merged
merged 3 commits into from
Oct 17, 2019
Merged

Adds contribution queue #3658

merged 3 commits into from
Oct 17, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Oct 10, 2019

Resolves brave/brave-browser#6288

Submitter Checklist:

Test Plan:

  • enable rewards with short contribution interval
  • claim grant (30 BAT)
  • set AC amount to 20 BAT
  • add verified publishers to AC table
  • add verified recurring tips in this order (5, 10, 10, 10)
  • wait for monthly contribution to trigger
  • you should see 25 BAT goes through for monthly and 5 BAT from AC

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.

@NejcZdovc NejcZdovc added this to the 0.72.x - Nightly milestone Oct 10, 2019
@NejcZdovc NejcZdovc self-assigned this Oct 10, 2019
@NejcZdovc NejcZdovc force-pushed the recurring-tips branch 7 times, most recently from db8f72a to bbea531 Compare October 14, 2019 11:27
@NejcZdovc NejcZdovc marked this pull request as ready for review October 14, 2019 19:45
@NejcZdovc
Copy link
Contributor Author

marked it so that CI can do first pass

@NejcZdovc NejcZdovc 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 Oct 14, 2019
@NejcZdovc
Copy link
Contributor Author

@tmancey @kylehickinson not ready for a review yet

@NejcZdovc NejcZdovc added CI/skip Do not run CI builds (except noplatform) and 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 Oct 15, 2019
@NejcZdovc NejcZdovc force-pushed the recurring-tips branch 3 times, most recently from e5f4c0e to 617b42e Compare October 15, 2019 15:12
@NejcZdovc NejcZdovc requested a review from a team October 15, 2019 15:12
@NejcZdovc NejcZdovc removed the CI/skip Do not run CI builds (except noplatform) label Oct 15, 2019
// Make sure that balance is updated correctly
{
const std::string result = ElementInnerText("[data-test-id='balance']");
LOG(ERROR) << "NEJC actual: " << result;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have it in for now as test is failing on CI, will remove at the end

tmancey
tmancey previously approved these changes Oct 16, 2019
Copy link
Collaborator

@tmancey tmancey 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
Copy link
Contributor Author

CI is failing with this issue brave/brave-browser#6515. Everything else passed

@bsclifton bsclifton merged commit b8c71c9 into master Oct 17, 2019
@bsclifton bsclifton deleted the recurring-tips branch October 17, 2019 05:45
NejcZdovc pushed a commit that referenced this pull request Oct 17, 2019
@LaurenWags
Copy link
Member

Verified with

Brave 0.73.8 Chromium: 78.0.3904.50 (Official Build) nightly (64-bit)
Revision 2accdc52c79976e264cd2694df6db31d1fccd8e8-refs/branch-heads/3904@{#658}
OS macOS Version 10.13.6 (Build 17G5019)
  • Verified test plan from PR 3x, no issues. Tip that could not be completed was not put in the 'Pending' list as expected.

6288-ss1

  • Ran the following scenario. Verified the 3rd tip of 10 BAT was skipped but the tip for 1 BAT after it was completed. So a total of 26 BAT for recurring tips was contributed and 4 BAT to verified publishers only was contributed as expected.
  1. enable rewards with short contribution interval on staging
  2. claim grant (30 BAT)
  3. set AC amount to 20 BAT
  4. add verified and non verified publishers to AC table
  5. add verified recurring tips in this order (5, 10, 10, 10, 1)
  6. wait for monthly contribution to trigger
  7. you should see 26 BAT goes through for monthly and 4 BAT from AC

6288-ss2

  1. enable rewards with short contribution interval on staging
  2. claim grant (30 BAT)
  3. set AC amount to 20 BAT
  4. add verified and unverified publishers to AC table
  5. add verified recurring tips in this order (5, 10, 10, 10)
  6. Restart browser
    ---> See notification about Insufficient Funds. Dismiss it.
  7. wait for monthly contribution to trigger
  8. you should see 25 BAT goes through for monthly and 5 BAT from AC
    ---> Get notice that AC couldn't be completed, will try again in 30 days message. Note - both AC and recurring tips appear to have been contributed.

6288-msg1

6288-msg2

@NejcZdovc
Copy link
Contributor Author

yup saw 6545 for some time now, already asked for any hints from other teams as my current theory is that it's related to chromium bump. 6545 is not related to this PR

@LaurenWags
Copy link
Member

Not a conclusive test, but I did just run two auto contribute only tests (no monthly recurring tips) with the same build (0.73.8) and did not see the Left sig != right sig. Signature check failure. message at all. All expected BAT was transferred to the test publisher account I used.

Maybe it's related to recurring tips?

@NejcZdovc
Copy link
Contributor Author

@LaurenWags not sure, it's for now random for me, don't have STR yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change monthly contribution/payment sequencing
5 participants