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

Fixes contribution that is split #5050

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Fixes contribution that is split #5050

merged 1 commit into from
Mar 25, 2020

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Mar 24, 2020 •
edited
Loading

Resolves brave/brave-browser#8828

Submitter Checklist:

Test Plan:

  • Enable rewards
  • Claim grant (20 BAT)
  • Connect KYC uphold
  • Set AC to 50 BAT and add verified publishers to AC table
  • Wait for AC to be triggered
  • Make sure that 20 BAT was taken from grant and 30 BAT from Uphold

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 1.8.x - Nightly milestone Mar 24, 2020
@NejcZdovc NejcZdovc requested a review from a team March 24, 2020 21:51
@NejcZdovc NejcZdovc self-assigned this Mar 24, 2020
@NejcZdovc NejcZdovc force-pushed the unblinded-contribution branch from ee952d6 to c369879 Compare March 24, 2020 22:09
@NejcZdovc NejcZdovc force-pushed the unblinded-contribution branch from c369879 to 1702176 Compare March 25, 2020 06:44
@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 and removed CI/skip-linux labels Mar 25, 2020
@NejcZdovc
Copy link
Contributor Author

Fix is small only in files contribution.cc and contribution_unblinded.cc. Majority of the code is related to adding tests

zenparsing
zenparsing approved these changes Mar 25, 2020
View reviewed changes
components/brave_rewards/resources/ui/components/modalActivity/index.tsx
@@ -185,6 +185,7 @@ export default class ModalActivity extends React.PureComponent<Props, State> {

return (
<TableTransactions
id={'transactionTable'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: noticed that you're using id here instead of data-test-id. Any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

id is prop that is accepted in the component. Would be good to refactor component to set data-test-id like you said

@NejcZdovc NejcZdovc merged commit 228a7b5 into master Mar 25, 2020
@NejcZdovc NejcZdovc deleted the unblinded-contribution branch March 25, 2020 14:49
@LaurenWags
Copy link
Member

Verified using a mix of

Brave 1.8.42 Chromium: 81.0.4044.69 (Official Build) nightly (64-bit)
Revision 6813546031a4bc83f717a2ef7cd4ac6ec1199132-refs/branch-heads/4044@{#776}
OS macOS Version 10.14.6 (Build 18G3020)

and

Brave 1.8.43 Chromium: 81.0.4044.69 (Official Build) nightly (64-bit)
Revision 6813546031a4bc83f717a2ef7cd4ac6ec1199132-refs/branch-heads/4044@{#776}
OS macOS Version 10.14.6 (Build 18G3020)
  • Verified test plan from PR. Confirmed that when a mix of UGP and Uphold BAT are available, the UGP BAT is used first, then the remainder of the Auto Contribute is pulled from Uphold BAT.

Screen Shot 2020-03-26 at 1 32 38 PM

Screen Shot 2020-03-26 at 1 33 47 PM

Note, there are two entries on Monthly Statement and two notifications. Per discussion with @NejcZdovc this is expected since we log both the contribution from UGP/VG and Uphold.

Screen Shot 2020-03-26 at 1 30 59 PM

Screen Shot 2020-03-26 at 1 33 21 PM

Screen Shot 2020-03-26 at 1 33 28 PM

  • Other scenarios checked:
  1. PASS Confirmed that if only Uphold BAT is available (KYC'd wallet), auto contribute worked as expected (note, this is not the case in 1.7.73 on Dev currently). General steps:
  • Clean profile, staging, short reconcile interval, enable rewards, don't claim UGP grant
  • Connect KYC'd Uphold account
  • Do one time tip
  • Set up recurring tip
  • Add verified sites to AC table
  • Wait for AC to occur
  • Confirmed recurring tip sent and AC budget deducted from Uphold wallet
  1. PASS Confirmed that if only UGP BAT is available (anon wallet, no KYC'd uphold account connected), auto contribute works as expected. General steps:
  • Clean profile, staging, short reconcile interval, enable rewards, claim UGP grant
  • Do one time tip
  • Set up recurring tip
  • Add verified sites to AC table
  • Wait for AC to occur
  • Confirmed recurring tip sent and AC contributed (wallet balance decreased)
  1. PASS Confirmed that if only BAT from a restored wallet (user funded) is available, auto contribute works as expected. General steps:
  • Clean profile, staging, short reconcile interval, enable rewards, don't claim UGP grant
  • Restore wallet with self-funded BAT
  • Do one time tip
  • Set up recurring tip
  • Add verified sites to AC table
  • Wait for AC to occur
  • Confirmed recurring tip sent and AC contributed (wallet balance decreased)

This was referenced Mar 26, 2020
Merged
Closed
@LaurenWags LaurenWags mentioned this pull request Mar 31, 2020
Closed
@bsclifton bsclifton mentioned this pull request Apr 1, 2020
Closed
petemill pushed a commit that referenced this pull request Jul 27, 2020
@PatOnTheBack @simonhong
Remove Identical Operand (#5050)
a9a9355 
Added `widevineConfig.fakeWidevineCdmLibFilePath`

This fixes a [warning found by lgtm](https://lgtm.com/projects/g/brave/brave-browser/snapshot/49c71729b7002b76ad3cade9853fce2f7e1ed149/files/lib/util.js?sort=name&dir=ASC&mode=heatmap).
The same operand was used twice in an if statement.
petemill pushed a commit that referenced this pull request Jul 28, 2020
@PatOnTheBack @simonhong
Remove Identical Operand (#5050)
d518b5e 
Added `widevineConfig.fakeWidevineCdmLibFilePath`

This fixes a [warning found by lgtm](https://lgtm.com/projects/g/brave/brave-browser/snapshot/49c71729b7002b76ad3cade9853fce2f7e1ed149/files/lib/util.js?sort=name&dir=ASC&mode=heatmap).
The same operand was used twice in an if statement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zenparsing zenparsing zenparsing approved these changes

Assignees

@NejcZdovc NejcZdovc

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS feature/rewards
Projects
None yet
Milestone
1.8.x - Release
Development

Successfully merging this pull request may close these issues.

Unblinded tokens not contributed correctly when not enough funds
3 participants
@NejcZdovc @LaurenWags @zenparsing