Balanced rev1.1 #2036

mahmoudimus · 2014-02-14T05:22:40Z

This starts the process of upgrading Gittip to revision 1.1 of the API and moves Gittip away from "legacy Accounts" to "Customers"

mahmoudimus · 2014-02-14T05:22:59Z

All credit here is to the good work of @matthewfl

mahmoudimus · 2014-02-14T05:23:26Z

@gittip & @whit537 et. al, we should start the code review process.

zbynekwinkler · 2014-02-14T15:59:04Z

Could we drop the email while we are changing the code around it? Please see #453.

mjallday · 2014-02-14T17:51:26Z

gittip/billing/__init__.py

-        things = getattr(self._account, self.thing_type+'s').all()
-        things = [thing for thing in things if thing.is_valid]
+        things = getattr(self._customer, self.thing_type+'s')\
+            .filter(is_valid=True).all()


in 1.1 is_valid is a default filter, no need to explicitly pass it unless you want is_valid=False

mjallday · 2014-02-14T18:01:41Z

@matthewfl looking good! I suggest we add a backwards compatibility test since this isn't testing an account uri being used with a customer.

If you create a customer and munge the stored href from /customers/CU123123 to /v1/marketplaces/MP123123/customers/CU123123 and then add a card on them and do the same for charging a card i think that will be good since that's what's going to happen for all existing customers on gittip.

chadwhitacre · 2014-02-14T20:04:06Z

gittip/billing/__init__.py

    except balanced.exc.HTTPError as err:
-        error = err.message.decode('UTF-8')  # XXX UTF-8?
+        error = err.message.message.decode('UTF-8')  # XXX UTF-8?


message.message

chadwhitacre · 2014-02-14T20:05:55Z

@mahmoudimus @matthewfl @mjallday Thanks for the PR! :-)

chadwhitacre · 2014-02-14T20:18:03Z

gittip/billing/__init__.py

@@ -35,15 +35,16 @@ def get_balanced_account(db, username, balanced_account_uri):
    # XXX Balanced requires an email address
    # https://github.com/balanced/balanced-api/issues/20
    # quote to work around https://github.com/gittip/www.gittip.com/issues/781
+    # emails are not required for customers any more


+1 to dropping email if we can here, as @zwn suggests at #2036 (comment). Our ticket for this: #453.

matthewfl · 2014-02-14T20:22:35Z

I will drop the email requirement as @zwn pointed out. I plan to add a few more tests for the payday scripts as I noticed that there were a number of lines that were not being covered.

matthewfl · 2014-02-15T01:03:22Z

Update: I have removed the email constraint, and have tested adding cards and bank accounts and they both appear to work. Additionally, I added a Last four of ssn field on the bank account. I am not sure what the communities opinion is with respect to having this information, however if a user is having a hard time confirming their identity, then including the last four digits of their ssn often greatly improves their chances of a match.

As for tests, there is a test for balanced which checks that old urls can be used. Balanced has a fix for this going public shortly, but in the mean time this test is expected to fail.

chadwhitacre · 2014-02-15T04:55:10Z

Additionally, I added a Last four of ssn field on the bank account.

Currently we direct the user to Balanced if the identity verification fails with basic info. I would love it if we could keep the user on our site for this escalation, but I'd rather not present them with an SSN field unless verification fails without it.

chadwhitacre · 2014-02-15T04:55:44Z

@matthewfl Can you look into why Travis is failing?

matthewfl · 2014-02-15T07:16:57Z

@whit537 Balanced is removing the support for the redirect to our site. With customers, instead of throwing an error when the match fails, the merchant status on the customer will simply report there is not a match. This means the site can continue to resubmit information.

One of the failures is the test that I referenced above

chadwhitacre · 2014-02-15T13:00:55Z

Balanced is removing the support for the redirect to our site.

Ah, got it, okay. I'll take a look at the UX for this ...

mjallday · 2014-02-16T01:13:05Z

@matthewfl there's a couple of python libraries similar to ruby's VCR which would fit the build here. Betamax and VCRpy both can make the actual http call and record the response for later testing. Best of both worlds potentially?

On Feb 15, 2014, at 15:38, Matthew Francis-Landau notifications@github.com wrote:

Sorry about the slow down, I guess I never the major slow down. As for the reason, I decided to make the test actually use the balanced api when testing the payments system. I felt that actually testing against the api ensure that the actions are going to happen as we expect instead of how we mocked.

To fix this I am thinking of two possibilities, first we could remock all of the balanced api and stop making api requests during the test. Alternately, we could split the tests into those that make api requests (slow) and those that don't, then you would only need to run the slower tests when you were specifically working on that specific subsystem. We could then travis run both sets of tests. This second option adds a little bit of complexity by splitting the tests into two different parts, however it start a framework for which you would be able to build other network based tests.

—
Reply to this email directly or view it on GitHub.

matthewfl · 2014-02-18T21:22:16Z

=================== test session starts ===================
platform linux2 -- Python 2.7.6 -- pytest-2.3.4
plugins: cache
collected 336 items 

tests/test_anonymous_json.py .....
tests/test_associate.py .....
tests/test_billing.py ...............
tests/test_billing_payday.py ...............................................
tests/test_bitcoin_json.py ...
tests/test_charts_json.py ........
tests/test_communities.py ......
tests/test_communities_json.py ......
tests/test_elsewhere.py ......
tests/test_elsewhere_bitbucket.py ..
tests/test_elsewhere_bountysource.py .
tests/test_elsewhere_github.py ..
tests/test_elsewhere_openstreetmap.py ..
tests/test_elsewhere_twitter.py ..
tests/test_fake_data.py .
tests/test_goal_json.py ..........
tests/test_hooks.py ....
tests/test_is_suspicious.py ....
tests/test_lookup_json.py ...
tests/test_membername_json.py .............
tests/test_pages.py .....................
tests/test_participant.py .......................................................................
tests/test_public_json.py ............
tests/test_record_an_exchange.py ............
tests/test_statement_json.py ....
tests/test_stats.py ..........
tests/test_take.py ........
tests/test_teams.py ............
tests/test_tip_json.py ..
tests/test_tips_json.py .......
tests/test_user.py ..................
tests/test_username_json.py .......
tests/test_utils.py .......
===================== 336 passed in 41.96 seconds =====================

@whit537 using @mjallday suggestion, I was able to reduce the time it takes the tests to run. For me, this is even a little faster than running the tests on the master branch.

chadwhitacre · 2014-02-19T15:25:09Z

That's awesome @matthewfl, thank you.

@seanlinsley Weren't you talking about VCR in some other context? Was that you?

seanlinsley · 2014-02-19T16:54:32Z

I've used the Ruby version of VCR before, but I don't think I've mentioned it here 🐙

chadwhitacre · 2014-02-20T05:39:26Z

VCR is way cool. :-)

$ rm -rf tests/fixtures/*
$ make test
... 336 passed in 320.69 seconds
$ make test
... 336 passed in 18.37 seconds

Balanced rev1.1

In #2036 we sprouted the ability to record HTTP sessions during testing and then reuse the result of those sessions in subsequent tests. This is done via the VCR library (a port from Ruby). Storing the cache in the repo doesn't seem quite right to me, so in this PR I'm proposing we take it out. The implication is that the first run of the test suite will take a loooong time to prime the cache. Subsequent runs should be an order of magnitude faster (back down near where we were before).

In #2036 we upgraded to the 1.1 version of the Balanced API. This commit propagates nomenclature changes down into the database, for a consistent developer experience.

In #2036 we started using VCR to freeze HTTP API calls into test fixtures. This is great! Let's do more of that! This commit moves the VCR configuration upstream from where it began (in test_billing.py) into the gittip.testing.Harness class. Now all tests can make expensive HTTP API calls with impunity. In fact, this knocks out a XXX comment, because we had one API call elsewhere in the test suite. VCR transparently took care of it for us! I was quite pleasantly surprised to find TestPages.yml. Yay VCR! :-) My first motivation in doing this, however, was to be able to write a test for regression I'm seeing with payday (#2057). Onward!

These address regressions introduced by #2036.

seanlinsley · 2014-03-04T04:40:00Z

tests/test_billing.py

+    mp = balanced.Marketplace.my_marketplace
+    if not mp:
+        mp = balanced.Marketplace().save()
+    o.balanced_marketplace = mp


Coming from: #2112 (comment)

Is it really the best idea to build a new API key & secret every time you run the test suite? It seems like that produces unnecessary churn in the git history.

At Balanced we do this on every test run. It's an easy way to ensure that we've got a clean slate before we begin and hitting the API means you're guaranteed that whatever the server is doing is compatible with your code. Generally I'd recommend doing that if you're interacting with the Balanced API, the fact that gittip is checking in fixture data makes the commits really large whenever it changes however.

As of cde5468 / 6319794 we're ignoring VCR cassettes on Travis, ensuring our CI does real API requests. I suppose we need a system that will create a blank slate on Travis, but will use a predefined key in development.

With the upgrade to the balanced library in #2036 we no longer need this workaround for balanced/balanced-python#5. Removing at this point because I'm ripping out all of our direct os.environ usage and this applies.

matthewfl added 4 commits February 13, 2014 16:10

cp, working on upgrading to balanced rev 1.1 and upgrading tests atm

e9961b8

cp, test_billing tests passing

72549d8

all tests currently passing

d934284

some changes for js, but jstests are running on my computer

f36456a

rm dead code

a57e67d

patcon mentioned this pull request Feb 14, 2014

upgrade to Balanced's 1.1 API #2033

Closed

zbynekwinkler added the DevX label Feb 14, 2014

mjallday reviewed Feb 14, 2014
View reviewed changes

chadwhitacre reviewed Feb 14, 2014
View reviewed changes

catch error message in if server can't get port

4719618

chadwhitacre reviewed Feb 14, 2014
View reviewed changes

matthewfl added 4 commits February 14, 2014 13:26

test for payday that actually uses the balanced test marketplace

1702cdd

currently failing test to check for being able to upgrade old style urls

2f3a057

adding cards working

36081ec

adding bank account working

f7a6d4f

fixing the other test

eafb4a3

All tests passing, and using vcr to make tests fast again

8f66ab6

kyungmin mentioned this pull request Feb 20, 2014

Coinbase #2052

Closed

chadwhitacre mentioned this pull request Feb 20, 2014

Coinbase #2053

Closed

chadwhitacre added a commit that referenced this pull request Feb 20, 2014

Merge pull request #2036 from matthewfl/balanced-rev1.1

401dc6b

Balanced rev1.1

chadwhitacre merged commit 401dc6b into gratipay:master Feb 20, 2014

chadwhitacre mentioned this pull request Feb 20, 2014

take VCR cassettes out of repo #2055

Closed

This was referenced Feb 20, 2014

keep database name in sync with Balanced API #2056

Merged

run Gittip 90 #2057

Closed

chadwhitacre mentioned this pull request Feb 20, 2014

Move VCR setup further up, onto Harness class #2058

Merged

chadwhitacre added a commit that referenced this pull request Feb 20, 2014

Here's two failing tests for some payday bugs

a97d1be

These address regressions introduced by #2036.

This was referenced Feb 27, 2014

balanced_customer_href is corrupted #2085

Closed

Error on bank account config #2111

Closed

Fix bank account regression #2112

Merged

seanlinsley reviewed Mar 4, 2014
View reviewed changes

This was referenced Mar 17, 2014

error connecting a credit card #2155

Closed

500 from credit card page #2161

Closed

fix 500 on credit card page #2162

Merged

chadwhitacre mentioned this pull request Mar 25, 2014

remove network access from tests #1586

Closed

chadwhitacre mentioned this pull request Apr 2, 2014

Take out configure_payments hack #2232

Merged

chadwhitacre mentioned this pull request Apr 3, 2014

Create policy doc for in what ways we can transact with vendors gratipay/inside.gratipay.com#15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Balanced rev1.1 #2036

Balanced rev1.1 #2036

mahmoudimus commented Feb 14, 2014

mahmoudimus commented Feb 14, 2014

mahmoudimus commented Feb 14, 2014

zbynekwinkler commented Feb 14, 2014

mjallday Feb 14, 2014

mjallday commented Feb 14, 2014

chadwhitacre Feb 14, 2014

chadwhitacre commented Feb 14, 2014

chadwhitacre Feb 14, 2014

matthewfl commented Feb 14, 2014

matthewfl commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

matthewfl commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

mjallday commented Feb 16, 2014

matthewfl commented Feb 18, 2014

chadwhitacre commented Feb 19, 2014

seanlinsley commented Feb 19, 2014

chadwhitacre commented Feb 20, 2014

seanlinsley Mar 4, 2014

mjallday Mar 5, 2014

seanlinsley Mar 5, 2014

Balanced rev1.1 #2036

Balanced rev1.1 #2036

Conversation

mahmoudimus commented Feb 14, 2014

mahmoudimus commented Feb 14, 2014

mahmoudimus commented Feb 14, 2014

zbynekwinkler commented Feb 14, 2014

mjallday Feb 14, 2014

Choose a reason for hiding this comment

mjallday commented Feb 14, 2014

chadwhitacre Feb 14, 2014

Choose a reason for hiding this comment

chadwhitacre commented Feb 14, 2014

chadwhitacre Feb 14, 2014

Choose a reason for hiding this comment

matthewfl commented Feb 14, 2014

matthewfl commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

matthewfl commented Feb 15, 2014

chadwhitacre commented Feb 15, 2014

mjallday commented Feb 16, 2014

matthewfl commented Feb 18, 2014

chadwhitacre commented Feb 19, 2014

seanlinsley commented Feb 19, 2014

chadwhitacre commented Feb 20, 2014

seanlinsley Mar 4, 2014

Choose a reason for hiding this comment

mjallday Mar 5, 2014

Choose a reason for hiding this comment

seanlinsley Mar 5, 2014

Choose a reason for hiding this comment