add bitcoin receiving address to profile #1437

reedlaw · 2013-09-12T08:11:44Z

For issue #1164

chadwhitacre · 2013-09-12T10:57:43Z

@reedlaw This is a great first pull request, thanks! :D

The one thing we need here that isn't obvious or documented (yet! coming soon) is that we want to log changes to the columns in the participants table. The way we're doing that is with a rule that in this case would log to a bitcoin_addresses table. See schema.sql for examples. Please create a branch.sql file and put the new DDL in there.

Also, #gittip on Freenode is a great place to connect if you're comfortable with IRC and want to chat about this.

chadwhitacre · 2013-09-12T11:05:20Z

schema.sql

@@ -37,6 +37,7 @@ CREATE TABLE participants
 -- stored here as an optimization and sanity check.
 , balance               numeric(35,2)               DEFAULT 0.0
 , pending               numeric(35,2)               DEFAULT NULL
+, bitcoin_address       text                        DEFAULT NULL


Can't blame you because we haven't documented it yet (#1272), but schema changes need to go in a branch.sql file. schema.sql is append-only. Also, we need a rule that logs changes to participants.bitcoin_address into a bitcoin_addresses table. See schema.sql for examples.

reedlaw · 2013-09-12T14:46:28Z

I wasn't sure what the branch.sql should look like so I appended the rule to schema.sql.

chadwhitacre · 2013-09-12T16:14:16Z

@reedlaw I'm debating whether to ask you for tests on this. :-)

We don't have client-side tests in place yet, but there are tests for other json endpoints that you should be able to use as a starting point for testing www/bitcoin.json.spt. And now that I notice it, I believe the endpoint should be under www/%username/ to be parallel to other endpoints that the profile page uses.

wyze · 2013-09-12T16:16:43Z

@whit537 @reedlaw Yes, please add tests for any new JSON endpoints. Same guidelines, if you alter an existing JSON endpoint, make sure there are tests along with it.

chadwhitacre · 2013-09-12T16:17:57Z

@reedlaw Copy/paste the code you added to schema.sql into branch.sql and you're all set with that. Having the new DDL in branch.sql enables me to run \i branch.sql in psql to apply the schema change in production.

rummik · 2013-09-12T19:07:01Z

Can we get this in a key in public.json?

chadwhitacre · 2013-09-12T22:42:35Z

@rummik If @reedlaw doesn't tackle that here we'll do it in a separate PR.

rummik · 2013-09-12T23:27:05Z

@whit537 I figured I'd voice it now since it isn't a huge thing :P If it isn't added here, I'll volunteer to add it later :)

…anch.sql

reedlaw · 2013-09-13T01:49:36Z

Ok, I think I've addressed all the issues in my latest commit. Please have a look.

chadwhitacre · 2013-09-13T03:31:05Z

I've tested this in the UI and I can save this example address:

1JwSSubhmg6iPtRjtyqhUYYH7bZg3Lfy1T

I've asked @reedlaw in IRC to implement the UI as inline ajax based on the example in #1287 rather than as a separate page (as with the credit card and bank pages).

We also need to make sure that we have Unix line-endings (test_bitcoin_json.py appears to have CRLF?).

The tests are failing for me. I get 11 errors out (out of 285 tests) with ValueError: No JSON object could be decoded. This looks like something that either @zwn or @wyze or both saw before. Maybe they can shed some light?

reedlaw · 2013-09-13T05:59:10Z

I fixed the failing test. It was due to adding bitcoin_address to public.json without checking for null. The line-endings came from my making a copy test_statement_json.py which is DOS format.

Now all the tests are passing and the bitcoin address field is inline on the profile page.

@reedlaw

@reedlaw caught this in #1437

chadwhitacre · 2013-09-16T21:58:34Z

The line-endings came from my making a copy test_statement_json.py which is DOS format.

Ah, good catch, sorry. Looks like it's the only one:

[gittip] $ grep -lr $'\r' tests gittip www | grep -v assets | grep -v favicon
tests/test_statement_json.py

I've updated it on master.

chadwhitacre · 2013-09-16T22:08:14Z

Steps

Add a bitcoin address to one account.
Sign in using a different account.
View the profile of the first account, the one with the bitcoin address attached.
Right-click on the bitcoin address and "open in new tab/window"

Expected: New tab opens, profile tab stays unchanged.

Actual: New tab opens, bitcoin address is apparently editable on profile tab(!).

…n address)

reedlaw · 2013-09-18T11:32:44Z

@whit537 good catch! It was a javascript bug that I've now fixed. It didn't actually allow anyone to edit another user's bitcoin address. If you crafted the HTML to submit the form it would still only edit your own user's bitcoin address.

chadwhitacre · 2013-09-20T02:10:48Z

@reedlaw

steps

sign in
add a bitcoin address
click on the bitcoin address I just added

expected: profile page stays the same, and I am sent to a blockchain.info page

actual: profile page switches to "edit" mode for bitcoin address, and then I am sent to a blockchain.info page

chadwhitacre · 2013-10-03T20:06:14Z

@reedlaw How are we doin' here? Can we fix that last bug so we can land this? P.S. Would be good to sync master out to your branch since it's moved a bit.

Let's bring this in for a landing! ✈️

reedlaw · 2013-10-03T20:20:37Z

Still settling after a move. Hope I get a chance soon...

On Thu, Oct 3, 2013 at 4:06 PM, Chad Whitacre [email protected]:

@reedlaw https://github.com/reedlaw How are we doin' here? Can we fix
that last bug so we can land this? P.S. Would be good to sync master out to
your branch since it's moved a bit.

Let's bring this in for a landing! [image: ✈️]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1437#issuecomment-25653426
.

Reed Law
Senior Developer
Smashing Boxes

chadwhitacre · 2013-10-03T20:22:47Z

Thanks for the pingback, @reedlaw. Keep us posted. :-)

reedlaw · 2013-10-10T19:54:54Z

Let me know if that last commit works for you.

zbynekwinkler · 2013-10-11T20:11:31Z

Hey @reedlaw! Could you please

merge current master into your branch/repo/pull-request
fix failing test line 34, in test_github_user_info_status_handling (make sure the test suite passes)
fix logging to the bitcoin_addresses table

steps

sign in
add a bitcoin address
remove bitcoin address

expected: bitcoin_addresses contains two lines, one with the the address and one with empty string

actual: bitcoin_addresses contains one entry with empty string

See for example the goals table and how changes are recorded there when the goal is changed.

chadwhitacre · 2013-10-11T20:15:46Z

@zwn The failing test is due to branch drift from master?

Also, good call on logging bitcoin_address changes. @reedlaw Sorry we don't have better docs yet on how that's handled. Catch @zwn or I in IRC if you need more pointers.

zbynekwinkler · 2013-10-11T20:24:20Z

@whit537 I don't know. I'd first merge master and start fixing after that. It might be something spurious.

clone1018 · 2013-12-27T22:39:13Z

What are we doing now with this pull request?

chadwhitacre · 2013-12-28T03:13:14Z

It's pretty good, but we lost @reedlaw. Ideally we'd dust it off and land it. It's not too complicated and not too far from done.

reedlaw · 2013-12-30T03:10:03Z

@whit537 I'm sorry, I started this when I had a big chunk of time but since getting back to work I haven't had time to pursue it. I may have time in a month or so, or feel free to let someone take over and finish this.

chadwhitacre · 2013-12-31T02:41:34Z

@reedlaw No worries, thanks for checking in. Sorry I try harder to land this three months ago. :-/

galuszkak · 2014-01-02T16:15:02Z

@reedlaw @whit537 As proposed on IRC I will try to continue and finish this ticket.

chadwhitacre · 2014-01-02T16:22:34Z

!m @galuszkak

galuszkak · 2014-01-03T13:45:18Z

hey @zwn seems that test are passing now after merge without any problems...

I will create pull request with merge.

This introduces bitcoin address field in gittip profile

seanlinsley · 2014-01-05T21:46:39Z

Moving over to #1814

add bitcoin receiving address to profile

2124159

chadwhitacre reviewed Sep 12, 2013
View reviewed changes

Reed Law added 2 commits September 12, 2013 22:23

log changes to participants table

f6d8475

add db rule to log changes to bitcoin_address

f72b476

test bitcoin.json, validate bitcoin address, and move migration to br…

6c3639c

…anch.sql

refactor bitcoin form to make it inline on profile page

3711f45

chadwhitacre mentioned this pull request Sep 16, 2013

add a profile field for bitcoin address #1164

Closed

chadwhitacre added a commit that referenced this pull request Sep 16, 2013

Fix line-endings on test_statement_json.py

1811bfc

@reedlaw caught this in #1437

fix javascript issue that triggers edit form on a click event (bitcoi…

e07c11d

…n address)

fixes gratipay#1437 (comment)

a2d9b26

ghost assigned galuszkak Jan 2, 2014

galuszkak added a commit to Solution4Future/www.gittip.com that referenced this pull request Jan 4, 2014

Merging with master PR gratipay#1437 for fixing gratipay#1163

f18b06a

This introduces bitcoin address field in gittip profile

galuszkak mentioned this pull request Jan 5, 2014

add bitcoin receiving address to profile #1814

Merged

seanlinsley closed this Jan 5, 2014

chadwhitacre mentioned this pull request Jan 6, 2014

Document development process #1728

Closed

chadwhitacre mentioned this pull request Feb 27, 2014

connect Flattr account #104

Closed

chadwhitacre unassigned galuszkak Apr 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add bitcoin receiving address to profile #1437

add bitcoin receiving address to profile #1437

reedlaw commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

chadwhitacre Sep 12, 2013

reedlaw commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

wyze commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

rummik commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

rummik commented Sep 12, 2013

reedlaw commented Sep 13, 2013

chadwhitacre commented Sep 13, 2013

reedlaw commented Sep 13, 2013

chadwhitacre commented Sep 16, 2013

chadwhitacre commented Sep 16, 2013

reedlaw commented Sep 18, 2013

chadwhitacre commented Sep 20, 2013

chadwhitacre commented Oct 3, 2013

reedlaw commented Oct 3, 2013

chadwhitacre commented Oct 3, 2013

reedlaw commented Oct 10, 2013

zbynekwinkler commented Oct 11, 2013

chadwhitacre commented Oct 11, 2013

zbynekwinkler commented Oct 11, 2013

clone1018 commented Dec 27, 2013

chadwhitacre commented Dec 28, 2013

reedlaw commented Dec 30, 2013

chadwhitacre commented Dec 31, 2013

galuszkak commented Jan 2, 2014

chadwhitacre commented Jan 2, 2014

galuszkak commented Jan 3, 2014

seanlinsley commented Jan 5, 2014

add bitcoin receiving address to profile #1437

add bitcoin receiving address to profile #1437

Conversation

reedlaw commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

chadwhitacre Sep 12, 2013

Choose a reason for hiding this comment

reedlaw commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

wyze commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

rummik commented Sep 12, 2013

chadwhitacre commented Sep 12, 2013

rummik commented Sep 12, 2013

reedlaw commented Sep 13, 2013

chadwhitacre commented Sep 13, 2013

reedlaw commented Sep 13, 2013

chadwhitacre commented Sep 16, 2013

chadwhitacre commented Sep 16, 2013

reedlaw commented Sep 18, 2013

chadwhitacre commented Sep 20, 2013

chadwhitacre commented Oct 3, 2013

reedlaw commented Oct 3, 2013

chadwhitacre commented Oct 3, 2013

reedlaw commented Oct 10, 2013

zbynekwinkler commented Oct 11, 2013

chadwhitacre commented Oct 11, 2013

zbynekwinkler commented Oct 11, 2013

clone1018 commented Dec 27, 2013

chadwhitacre commented Dec 28, 2013

reedlaw commented Dec 30, 2013

chadwhitacre commented Dec 31, 2013

galuszkak commented Jan 2, 2014

chadwhitacre commented Jan 2, 2014

galuszkak commented Jan 3, 2014

seanlinsley commented Jan 5, 2014