Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

do not show coinbase for unsupported countries #4615

Merged
merged 1 commit into from
Oct 11, 2016

Conversation

jkup
Copy link
Contributor

@jkup jkup commented Oct 7, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fix #4292

Test Plan:

Open Brave from a country where coinbase is not supported. I set my VPN to Israel. Then make sure the following changes occur in the add funds dialog.

  • Title goes from "Three ways to add.." to just "Add Bitcoin"
  • Coinbase logo disappears from the bottom
  • Click QR code and make sure coinbase logo is also gone from that footer.

@jkup jkup added the design A design change, especially one which needs input from the design team. label Oct 7, 2016
@jkup jkup added this to the 0.12.5dev milestone Oct 7, 2016
@jkup jkup force-pushed the bitcoin-without-coinbase branch from 3a50fef to 094d5ac Compare October 7, 2016 22:04
: null
}
<a href='https://itunes.apple.com/us/app/coinbase-bitcoin-wallet/id886427730?mt=8' target='_blank' id='appstoreLogo' />
<a href='https://play.google.com/store/apps/details?id=com.coinbase.android' target='_blank' id='playstoreLogo' />
Copy link
Member

Choose a reason for hiding this comment

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

why are we linking to the coinbase app for countries where coinbase is not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry! fixed.

<span className='coinbaseMessage' data-l10n-id='coinbaseMessage' />
<Button l10nId='done' className='pull-right whiteButton' onClick={this.props.hideParentOverlay} />
</div>
}
}
Copy link
Member

Choose a reason for hiding this comment

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

should return null otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@jkup jkup force-pushed the bitcoin-without-coinbase branch from 094d5ac to 18737a0 Compare October 10, 2016 19:42
@diracdeltas
Copy link
Member

when my country code is FR, it tells me to buy btc through cubits but still shows the coinbase logo and footer.
screen shot 2016-10-10 at 1 55 23 pm

@diracdeltas
Copy link
Member

when my country code is MD, the coinbase buy button is there, but clicking it returns a 403 from buy.coinbase.com and does nothing. the same thing happens on master FWIW. i think the list of country codes in this PR and the list of supported countries returned by ledger is not the same.

@mrose17

@jkup
Copy link
Contributor Author

jkup commented Oct 10, 2016

@diracdeltas hmm that's a good catch. Perhaps there is something wrong with our approach. @mrose17 found a list of "supported countries" here https://www.coinbase.com/global but they don't all seem to be supported.

@diracdeltas
Copy link
Member

diracdeltas commented Oct 10, 2016

2 options:

  1. assume js/constants/coinbaseCountries is authoritative and change BitcoinDashboard.exchangePanel accordingly
  2. assume ledgerInfo.exchangeInfo is authoritative and change the footer logo/links accordingly

i'm inclined to believe js/constants/coinbaseCountries is authoritative because sometimes the exchangePanel returns a 403 when it tries to go to Coinbase with a foreign IP

@mrose17
Copy link
Member

mrose17 commented Oct 11, 2016

@jkup - i agree with @diracdeltas

@jkup
Copy link
Contributor Author

jkup commented Oct 11, 2016

@mrose17 @diracdeltas just to make sure I'm clear. We should do away with all of the checks for the user being in the US and only have whether or not they are in constants/coinbaseCountries?

@diracdeltas
Copy link
Member

did i understand correctly today that the widget only works in the US but the coinbase apps work elsewhere? if so, this at least should change:

return this.coinbasePanel
.

@jkup jkup force-pushed the bitcoin-without-coinbase branch from 18737a0 to 067f6a7 Compare October 11, 2016 20:41
@jkup
Copy link
Contributor Author

jkup commented Oct 11, 2016

@diracdeltas I had a misunderstanding with the logic, should all be working now!

@diracdeltas
Copy link
Member

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team. feature/rewards l10n QA/checked-macOS QA/checked-Win32 QA/checked-Win64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants