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 retries for unverified publishers #2512

Merged
merged 3 commits into from
Jun 5, 2019
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 28, 2019

Resolves brave/brave-browser#3741

UI PR: brave/brave-ui#482

Submitter Checklist:

Test Plan:

  • enable rewards
  • close browser
  • go to publisher_list and change second value for 3zsistemi.si to false from ["3zsistemi.si",true,false.... to ["3zsistemi.si",false,false....
  • start the browser
  • add tip to unverified site
  • add tip for 10, 10, 5 and 10 to 3zsistemi.si
  • add another tip to unverified site
  • go to 3zsistemi.si, open panel and click check again
  • go to settings page and observe the page after couple of minutes 3 pending contributions (10, 10, 5) should be processed and you should have two notifications (new publisher verified and out of funds)
  • open pending contributions dialog and make sure that you see 3 tips (2 to your unverified site and one to 3zsistemi for 10)

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.67.x - Nightly milestone May 28, 2019
@NejcZdovc NejcZdovc self-assigned this May 28, 2019
@NejcZdovc NejcZdovc force-pushed the unverified-retries branch 2 times, most recently from 4d1f115 to 55e698c Compare May 29, 2019 08:59
@NejcZdovc NejcZdovc changed the base branch from pending-mojo to master May 29, 2019 08:59
@NejcZdovc NejcZdovc force-pushed the unverified-retries branch 10 times, most recently from 52684a0 to 247e404 Compare June 5, 2019 08:20
@NejcZdovc NejcZdovc marked this pull request as ready for review June 5, 2019 08:20
@NejcZdovc NejcZdovc force-pushed the unverified-retries branch 2 times, most recently from e5397d6 to d0373cb Compare June 5, 2019 10:13
@NejcZdovc NejcZdovc requested a review from ryanml June 5, 2019 10:14
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.

Basically LGTM, just had a few comments.

emerick
emerick previously approved these changes Jun 5, 2019
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 had one suggestion.

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

@NejcZdovc NejcZdovc merged commit d923c76 into master Jun 5, 2019
@NejcZdovc NejcZdovc deleted the unverified-retries branch June 5, 2019 17:54
NejcZdovc added a commit that referenced this pull request Jun 5, 2019
Adds retries for unverified publishers
NejcZdovc added a commit that referenced this pull request Jun 13, 2019
Adds retries for unverified publishers
@LaurenWags
Copy link
Member

Tested the following items on Nightly - primarily using:

Brave 0.68.39 Chromium: 75.0.3770.87 (Official Build) nightly(64-bit)
Revision 9dc58a2353af60ab2b48bab98a25bc43ed59085d-refs/branch-heads/3770@{#982}
OS Mac OS X
  1. Ran test plan as is from above. Worked as described. Tried 3x.
  2. After running thru test plan, navigated to an unverified site and clicked 'Refresh Status'. Got 2nd Notification for 'Insufficient Funds - You have pending tips due to insufficient funds'. Per Retries for unverified Publishers  brave-browser#3741 (comment) was only expecting this once (one bullet pt in comment says "if tips fail due to lack of funds, notify first time ONLY when users retry"). - logged multiple Insufficient Funds due to pending tips notification - follow up to 3741 brave-browser#4757
  3. Verified all 'Types' (shown on pending contributions list - Auto Contribute, One-time Tip, Recurring Tip) are removed from pending list when publisher verifies. All are converted to 'One-time' tips in Tips panel. Note - AC values are rounded down to nearest whole number, i.e. 2.6 BAT in list becomes a 2.0 BAT tip. Logged pending AC values are rounded once publisher verifies - follow up to 3741 brave-browser#4886 for this.
  4. Re-ran test plan but substituted some of the Tips from test plan to be auto contribute values and recurring tips. All were removed from pending list and added to Tips panel as one-time tips expected.
  5. Verified sites were removed from 'Pending Contributions' list. Used the following general outline: populated pending contributions list. Closed Brave. Modified DB so that the 'Pending Until' date was today's date. Opened Brave, verified I could see the date changes on pending contributions list. Navigated to a site and clicked 'Refresh Status' in panel. Verified that when I went back to pending contributions list the sites I had changed were no longer displayed. Verified that the amount of pending BAT (on brave://rewards and visible from panel when accessed from BAT logo) decreased appropriately.
  6. Verified that if a YouTube, Twitch, or Twitter publisher is in the pending contributions list and the publisher verifies, they are removed from the list, and their pending contribution is converted to a one-time tip (listed in tips panel on brave://rewards). Note, the notification which shows on BAT logo when a publisher from pending list has verified, does not list platform - i.e. 'on YouTube', 'on Twitch', etc. Asked about this, was told it's expected due to space constraints.

cc @kjozwiak

@kjozwiak
Copy link
Member

Basically went through the same cases @LaurenWags did above on Windows during the weekend everything seems like it's working as expected. Thanks for the detailed summary @LaurenWags! It was really helpful when going through checks on me end 👍

NejcZdovc added a commit that referenced this pull request Jun 17, 2019
Merge pull request #2512 from brave/unverified-retries
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.

Retries for unverified Publishers
4 participants