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

Handle 404 and 403 errors from uphold #5470

Merged
merged 1 commit into from
May 8, 2020
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 6, 2020

Resolves brave/brave-browser#9653

Submitter Checklist:

Test Plan:

  • enable rewards
  • claim grant
  • connect KYC wallet with some BAT in it
  • close browser
  • go to Preferences and change external_wallets->uphold->address to something random
  • start browser
  • go to rewards page
  • make sure that wallet is disconnected and that you see grant balance

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.10.x - Nightly milestone May 6, 2020
@NejcZdovc NejcZdovc requested a review from a team May 6, 2020 10:02
@NejcZdovc NejcZdovc self-assigned this May 6, 2020
@NejcZdovc NejcZdovc force-pushed the uphold-balance-fetch branch from c5bb277 to 3e87572 Compare May 6, 2020 11:25
@NejcZdovc NejcZdovc requested a review from gdregalo as a code owner May 6, 2020 11:25
@@ -1522,10 +1522,9 @@ void RewardsDOMHandler::OnFetchBalance(
base::DictionaryValue data;
data.SetIntKey("status", result);
base::Value balance_value(base::Value::Type::DICTIONARY);
balance_value.SetDoubleKey("total", balance->total);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since balance is always valid, do we need the && balance check in the if below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my mistake as balance can be null, so we should still have checks for it. Fixed

@NejcZdovc NejcZdovc force-pushed the uphold-balance-fetch branch 2 times, most recently from 7977759 to b2a4de0 Compare May 7, 2020 07:26
@NejcZdovc NejcZdovc added CI/skip-ios Do not run CI builds for iOS CI/skip-windows labels May 8, 2020
@NejcZdovc NejcZdovc force-pushed the uphold-balance-fetch branch from b2a4de0 to f2e9b10 Compare May 8, 2020 07:51
@NejcZdovc
Copy link
Contributor Author

CI passed on everything except brave/brave-browser#9687

@LaurenWags
Copy link
Member

Verified using

Brave | 1.10.56 Chromium: 81.0.4044.138 (Official Build) nightly (64-bit)
-- | --
Revision | 8c6c7ba89cc9453625af54f11fd83179e23450fa-refs/branch-heads/4044@{#999}
OS | macOS Version 10.14.6 (Build 18G3020)
  • Verified test plan from description

Prior to changing the address field in Preferences file:
Screen Shot 2020-05-14 at 11 23 44 AM

After changing the address field in Preferences file:

[ RESPONSE - OnFetchBalance ]
> time: 1589470441
> result: Failure
> http code: 404

Screen Shot 2020-05-14 at 11 28 59 AM

Screen Shot 2020-05-14 at 11 38 35 AM

Screen Shot 2020-05-14 at 11 29 11 AM

Note - attempted to reconnect after completing the test plan, but was unsuccessful. Reviewed and discussed with @NejcZdovc - logged brave/brave-browser#9780.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS feature/rewards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disconnect uphold if we get 403 from uphold
4 participants