Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

add bitcoin receiving address to profile #1814

Merged
merged 14 commits into from
Jan 13, 2014
Merged

Conversation

galuszkak
Copy link
Contributor

Rebase to master PR #1437 for fixing #1163

Please review. :)

Tests seems that works.

@chadwhitacre
Copy link
Contributor

I changed the title to the old one from #1437 for better scanability in the PR listing.

@ghost ghost assigned zwn Jan 6, 2014
@zwn
Copy link
Contributor

zwn commented Jan 6, 2014

I am reviewing this PR. Do we want to keep our own copy of bitcoin validation code in bitcoin.json.spt? I see that there exists python-bitcoinaddress package that we could use. The validation code in this PR is pretty simplistic - it throws ValueError if there is a character in the bc address that is not allowed, thus creating false report in Sentry (logs all exceptions in production). I would also appreciate having the input box large enough to actually see the address I am entering (maybe put the save & cancel buttons under the input box instead of next to it?), also the edit link is not very touch friendly (too small).

We need to

@galuszkak
Copy link
Contributor Author

@zwn +1 for validation.

I didn't think about all these things. My first step was to integrate all @reedlaw changes to master. I think using python-bitcoinaddress is really good idea.

To UI changes I think they are much more easy to add. :)

Thanks for input @zwn

@zwn
Copy link
Contributor

zwn commented Jan 7, 2014

@galuszkak To add a new package to gittip you download it from pypi and put the tarball to vendor directory and a reference to it to the requirements.txt file (see the file for examples). The goal being here that we have everything at hand at deploy time and it is also pinned so it does not change without us noticing. If something is not obvious feel free to ask question (here or at irc).

@zwn
Copy link
Contributor

zwn commented Jan 7, 2014

One more thing - the current validation code allows me to prepend additional characters and still considers the address valid. That does not seem right (but I know close to nothing about bitcoin).

>>> check_bc('1AGNa15ZQXAZUgFiqJ2i7Z2DPU2J6hW62i')
True
>>> check_bc('111111111AGNa15ZQXAZUgFiqJ2i7Z2DPU2J6hW62i')
True

@zwn
Copy link
Contributor

zwn commented Jan 7, 2014

Could you please also factor out the changes done to schema.sql into a separate branch.sql? Are you familiar with this concept already? We need to add a description somewhere #1846. Changes to db schema are introduced in a separate file branch.sql and only folded to schema.sql after a merge to master. We need this for deployment of the changes and easy updates of the production database (sourcing the branch.sql file into psql for updates). I would have done it myself but you are not working on a branch where I do not have access to.

@galuszkak
Copy link
Contributor Author

@zwn no problem. I was curious why this was in seperate file like branch.sql. Ok I will add this there.

@ghost ghost assigned galuszkak Jan 8, 2014
@galuszkak
Copy link
Contributor Author

@zwn updated.

@whit537 when I was fixing test I was little confused what changed in tests api. This should be noted somewhere for other people.

@zwn
Copy link
Contributor

zwn commented Jan 12, 2014

@galuszkak Thanks! I am almost happy. Can we do something about the tiny size of the edit link? I can hardly hit it with a mouse. I can't imagine what a user with a touch screen would do.

@seanlinsley Do I remember right that that you were the one trying to make the site more responsive and touch friendly? Maybe you could help here too. Thanks.

@galuszkak
Copy link
Contributor Author

@zwn no problem. I will update

@galuszkak
Copy link
Contributor Author

@zwn updated. please check

@zwn
Copy link
Contributor

zwn commented Jan 12, 2014

Thanks. Since no one has voiced any concerns about the UI (I am not an UI person) I am going to merge.
So, last call 😉

@seanlinsley
Copy link
Contributor

I just tested the UI locally, and it looks good to me 🐱

@@ -70,7 +70,7 @@ a.mini-user:hover {

#accounts {
margin-bottom: 12pt;

width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be 100%?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't need more that is why I didn't used 100% because 400px was ok and whole address was seen in input very clearly.

zwn added a commit that referenced this pull request Jan 13, 2014
add bitcoin receiving address to profile
@zwn zwn merged commit 4033c9b into gratipay:master Jan 13, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants