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

Implement User wallets (Uphold) on Android. (#8511) #5039

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Implement User wallets (Uphold) on Android. (#8511) #5039

merged 1 commit into from
Apr 23, 2020

Conversation

gdregalo
Copy link
Contributor

@gdregalo gdregalo commented Mar 24, 2020 •
edited by NejcZdovc
Loading

Resolves brave/brave-browser#8511

Submitter Checklist:

Test Plan:

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.

@gdregalo gdregalo self-assigned this Mar 24, 2020
@gdregalo
Copy link
Contributor Author

gdregalo commented Apr 4, 2020

rebased on 46fc3e0

@gdregalo gdregalo requested a review from bridiver as a code owner April 9, 2020 19:16
@gdregalo
Copy link
Contributor Author

Ready for review!
I will re-base once Android build is fixed.

@gdregalo gdregalo added the CI/skip-ios Do not run CI builds for iOS label Apr 13, 2020
@gdregalo
Copy link
Contributor Author

CI failed on unrelated issue:
https://bravesoftware.slack.com/archives/CA5FPHWLF/p1586829510286700?thread_ts=1586829436.286600&cid=CA5FPHWLF

@gdregalo
Copy link
Contributor Author

All green! Please review.

SergeyZhukovsky
SergeyZhukovsky requested changes Apr 15, 2020
View reviewed changes
android/java/org/chromium/chrome/browser/BraveRewardsExternalWallet.java Outdated Show resolved Hide resolved
android/java/org/chromium/chrome/browser/BraveRewardsExternalWallet.java Outdated Show resolved Hide resolved
android/java/org/chromium/chrome/browser/BraveRewardsUserWalletActivity.java Outdated Show resolved Hide resolved
android/java/org/chromium/chrome/browser/BraveRewardsVerifyWalletActivity.java Outdated Show resolved Hide resolved
android/java/org/chromium/chrome/browser/BraveUphold.java Outdated Show resolved Hide resolved
android/java/res/layout/verify_wallet_activity.xml Outdated Show resolved Hide resolved
android/java/res/layout/verify_wallet_activity.xml Outdated Show resolved Hide resolved
android/java/res/layout/verify_wallet_activity.xml Outdated Show resolved Hide resolved
android/java/res/values-h380dp/dimens.xml Outdated Show resolved Hide resolved
android/java/res/values-h730dp/dimens.xml Outdated Show resolved Hide resolved
@gdregalo
Copy link
Contributor Author

It's all green again. Please review.

NejcZdovc
NejcZdovc suggested changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. connected state is missing for publisher
    Connected state is missing on the panel and tip banner. You can see how it should work if you do this steps on desktop
  • enable rewards (staging flag)
  • connect KYC uphold wallet
  • visit bumpsmack.com (which is connected publisher)
    image
    image
    (note that if you still have VG we don't show this message, only when you run out of everything except uphold). You can test this on desktop as well

  1. logo should not be colored in the panel
    image

  2. amount is blank

  • enable rewards (staging)
  • connect to KYC wallet
  • claim promotions from the panel
  • amount in the panel is empty
  • you need to re-open panel for amount to be displayed
  1. no indication that anything happened.
    Right now when wallet is connected you are redirected to rewards page, but there is no indication about success or failure. Because decision was made to not do rewards page right away we should open panel so that we tell user what happened.

This was tested with
image

@gdregalo
Copy link
Contributor Author

gdregalo commented Apr 20, 2020 •
edited by w0ts0n
Loading

@NejcZdovc
Connected state is implemented and reflected on UI in accordance to state diagram at
gdrive link
There is nothing about Site banner there. The rewards html ui is not updated as discussed.

@NejcZdovc
Copy link
Contributor

@gdregalo diagram doesn't have publisher flows it only has uphold flows

@gdregalo
Copy link
Contributor Author

@NejcZdovc I guess I should have been provided the document describing the publisher flow in the beginning of the task instead of when I want to merge it.

@NejcZdovc
Copy link
Contributor

@gdregalo documents were provided to you by @kylehickinson

@gdregalo
Copy link
Contributor Author

@NejcZdovc

  1. Added publisher's CONNECTED state note for both panel and site banner.
  2. Used white Uphold icon.
  3. Known and hard to reproduce. It will be reviewed in a separate issue.
  4. The Uphold page has to be redirected somewhere. We can not close the tab because it might be the last one and it will cause the app to close too. Leaving it in indeterminate state is not an option either. The solution can be to open the rewards settings page where the latest wallet status is reflected. But it doesn't have it at the moment (WIP) . As temporary solution: opening both the panel and rewards page.

@gdregalo
Copy link
Contributor Author

It's green again. Please review.

SergeyZhukovsky
SergeyZhukovsky approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

samartnik
samartnik approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

@gdregalo
Copy link
Contributor Author

@bridiver
Created issue: brave/brave-browser#9395
"Rewrite TabbedModeTabDelegateFactory.java.patch using ASM"

bridiver
bridiver approved these changes Apr 22, 2020
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

patch is ok with follow up issue brave/brave-browser#9395

@gdregalo gdregalo merged commit d11d5db into master Apr 23, 2020
@gdregalo gdregalo deleted the android-uphold-base branch April 23, 2020 03:22
@NejcZdovc NejcZdovc added this to the 1.10.x - Nightly milestone Apr 23, 2020
This was referenced Apr 24, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@bridiver bridiver bridiver approved these changes

@deeppandya deeppandya Awaiting requested review from deeppandya deeppandya is a code owner

Assignees

@gdregalo gdregalo

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

Successfully merging this pull request may close these issues.

Implement User wallets (Uphold) on Android
5 participants
@gdregalo @NejcZdovc @bridiver @SergeyZhukovsky @samartnik